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
