Re: [PATCH v2 3/3] PCI: xilinx-xdma: Add Xilinx XDMA Root Port driver

[Bjorn Helgaas]

On Fri, May 12, 2023 at 11:57:25AM +0530, Thippeswamy Havalige wrote:
> Add support for Xilinx XDMA Soft IP core as Root Port.
> 
> The Zynq UltraScale+ MPSoCs devices support XDMA soft IP module in
> programmable logic.
> 
> The integrated XDMA soft IP block has integrated bridge function that
> can act as PCIe Root Port.

> +config PCIE_XILINX_DMA
> +	bool "Xilinx DMA PL PCIe host bridge support"

Whatever name/text you settle on, make sure it's in alpha order in the
config menu seen by users.  As-is, this patch would make it:

  Xilinx AXI PCIe controller
  Xilinx NWL PCIe controller
  Xilinx Versal CPM PCI controller
  Xilinx DMA PL PCIe host bridge support

which is not in alpha order.

> +	  Say 'Y' here if you want kernel to enable support for the
> +	  XILINX PL PCIe host bridge support, this PCIe controller
> +	  includes DMA PL component.

> +obj-$(CONFIG_PCIE_XILINX_DMA) += pcie-xdma-pl.o

I think this filename needs to include xilinx somehow, not just "xdma".

Since the probe function calls pci_host_probe() in addition to the DMA
setup, I guess this is a fourth Xilinx host bridge, a peer of AXI,
CPM, and NWL, and independent of them?

Is the "xdma" or ("DMA PL" as used in Kconfig) name also a peer to
"CPM" and "NWL"?  The Kconfig text, especially, should use names that
users will recognize.  "DMA" or "XDMA" seems a little generic.  The
commit log mentions "Zynq" and "Ultrascale+", neither of which appears
in Kconfig, so there are a lot of names in play here, which is
confusing.

> +struct xilinx_pcie_dma {

git grep "^struct .*pcie.*" drivers/pci/controller/ says the typical
names are "<driver>_pcie".  Please do the same.

> +	void __iomem			*reg_base;
> +	u32				irq;
> +	struct pci_config_window	*cfg;
> +	struct device			*dev;

Please use typical order, i.e., "dev" first, followed by "reg_base",
etc.  Look at other drivers and make this similar.  No need to be
creatively different.

> +static inline bool xilinx_pcie_dma_linkup(struct xilinx_pcie_dma *port)

Please use the *_pcie_link_up() naming scheme used elsewhere in
drivers/pci/controller/.

> +static bool xilinx_pcie_dma_valid_device(struct pci_bus *bus, unsigned int devfn)

Similarly, *_pcie_valid_device().  Lots more instances below.  Don't
split the "pcie" from the rest of the generic parts of the name.

> +static struct pci_ecam_ops xilinx_pcie_dma_ops = {

const *_ecam_ops

> +static void xilinx_mask_leg_irq(struct irq_data *data)
> +static void xilinx_unmask_leg_irq(struct irq_data *data)
> +static struct irq_chip xilinx_leg_irq_chip = {
> +	.name           = "INTx",
> +	.irq_mask       = xilinx_mask_leg_irq,
> +	.irq_unmask     = xilinx_unmask_leg_irq,
> +};

You use "intx" in the names below.  Please also use "intx" instead of
"leg" in the names above.  No need for two different names for the
same concept.

> +static const struct irq_domain_ops intx_domain_ops = {
> +	.map = xilinx_pcie_dma_intx_map,

> +	/* Enable the Bridge enable bit */

"Set the ... enable bit"

> +	pcie_write(port, pcie_read(port, XILINX_PCIE_DMA_REG_RPSC) |

> +static int xilinx_pcie_dma_parse_dt(struct xilinx_pcie_dma *port,
> +				    struct resource *bus_range)
> +{
> +	struct device *dev = port->dev;
> +	int err;
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct resource *res;

Weird ordering.  Suggest order of use:

  struct device *dev = port->dev;
  struct platform_device *pdev = to_platform_device(dev);
  struct resource *res;
  int err;

> +static int xilinx_pcie_dma_probe(struct platform_device *pdev)
> +{
> +	struct xilinx_pcie_dma *port;
> +	struct device *dev = &pdev->dev;
> +	struct pci_host_bridge *bridge;
> +	struct resource_entry *bus;
> +	int err;

Would order "struct device *dev" first.

Bjorn