Re: [PATCH 16/17] mfd: Add support for LAN966x PCI device

[<Steen.Hegelund@microchip.com>]

Hi Herve,

On Tue Apr 30, 2024 at 10:37 AM CEST, Herve Codina wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> Add a PCI driver that handles the LAN966x PCI device using a device-tree
> overlay. This overlay is applied to the PCI device DT node and allows to
> describe components that are present in the device.
>
> The memory from the device-tree is remapped to the BAR memory thanks to
> "ranges" properties computed at runtime by the PCI core during the PCI
> enumeration.
> The PCI device itself acts as an interrupt controller and is used as the
> parent of the internal LAN966x interrupt controller to route the
> interrupts to the assigned PCI INTx interrupt.
>
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---
>  drivers/mfd/Kconfig          |  24 ++++
>  drivers/mfd/Makefile         |   4 +
>  drivers/mfd/lan966x_pci.c    | 229 +++++++++++++++++++++++++++++++++++
>  drivers/mfd/lan966x_pci.dtso | 167 +++++++++++++++++++++++++
>  drivers/pci/quirks.c         |   1 +
>  5 files changed, 425 insertions(+)
>  create mode 100644 drivers/mfd/lan966x_pci.c
>  create mode 100644 drivers/mfd/lan966x_pci.dtso
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 4b023ee229cf..e5f5d2986dd3 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -144,6 +144,30 @@ config MFD_ATMEL_FLEXCOM
>           by the probe function of this MFD driver according to a device tree
>           property.
>
> +config MFD_LAN966X_PCI
> +       tristate "Microchip LAN966x PCIe Support"
> +       depends on PCI
> +       select OF
> +       select OF_OVERLAY
> +       select IRQ_DOMAIN
> +       help
> +         This enables the support for the LAN966x PCIe device.
> +         This is used to drive the LAN966x PCIe device from the host system
> +         to which it is connected.
> +
> +         This driver uses an overlay to load other drivers to support for
> +         LAN966x internal components.
> +         Even if this driver does not depend on these other drivers, in order
> +         to have a fully functional board, the following drivers are needed:
> +           - fixed-clock (COMMON_CLK)
> +           - lan966x-oic (LAN966X_OIC)
> +           - lan966x-cpu-syscon (MFD_SYSCON)
> +           - lan966x-switch-reset (RESET_MCHP_SPARX5)
> +           - lan966x-pinctrl (PINCTRL_OCELOT)
> +           - lan966x-serdes (PHY_LAN966X_SERDES)
> +           - lan966x-miim (MDIO_MSCC_MIIM)
> +           - lan966x-switch (LAN966X_SWITCH)
> +
>  config MFD_ATMEL_HLCDC
>         tristate "Atmel HLCDC (High-end LCD Controller)"
>         select MFD_CORE
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index c66f07edcd0e..165a9674ff48 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -284,3 +284,7 @@ rsmu-i2c-objs                       := rsmu_core.o rsmu_i2c.o
>  rsmu-spi-objs                  := rsmu_core.o rsmu_spi.o
>  obj-$(CONFIG_MFD_RSMU_I2C)     += rsmu-i2c.o
>  obj-$(CONFIG_MFD_RSMU_SPI)     += rsmu-spi.o
> +
> +lan966x-pci-objs               := lan966x_pci.o
> +lan966x-pci-objs               += lan966x_pci.dtbo.o
> +obj-$(CONFIG_MFD_LAN966X_PCI)  += lan966x-pci.o
> diff --git a/drivers/mfd/lan966x_pci.c b/drivers/mfd/lan966x_pci.c
> new file mode 100644
> index 000000000000..d9d886a1948f
> --- /dev/null
> +++ b/drivers/mfd/lan966x_pci.c
> @@ -0,0 +1,229 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Microchip LAN966x PCI driver
> + *
> + * Copyright (c) 2024 Microchip Technology Inc. and its subsidiaries.
> + *
> + * Authors:
> + *     Clément Léger <clement.leger@bootlin.com>
> + *     Hervé Codina <herve.codina@bootlin.com>
> + */
> +
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/pci.h>
> +#include <linux/slab.h>
> +
> +/* Embedded dtbo symbols created by cmd_wrap_S_dtb in scripts/Makefile.lib */
> +extern char __dtbo_lan966x_pci_begin[];
> +extern char __dtbo_lan966x_pci_end[];
> +
> +struct pci_dev_intr_ctrl {
> +       struct pci_dev *pci_dev;
> +       struct irq_domain *irq_domain;
> +       int irq;
> +};
> +
> +static int pci_dev_irq_domain_map(struct irq_domain *d, unsigned int virq, irq_hw_number_t hw)
> +{
> +       irq_set_chip_and_handler(virq, &dummy_irq_chip, handle_simple_irq);
> +       return 0;
> +}
> +
> +static const struct irq_domain_ops pci_dev_irq_domain_ops = {
> +       .map = pci_dev_irq_domain_map,
> +       .xlate = irq_domain_xlate_onecell,
> +};
> +
> +static irqreturn_t pci_dev_irq_handler(int irq, void *data)
> +{
> +       struct pci_dev_intr_ctrl *intr_ctrl = data;
> +       int ret;
> +
> +       ret = generic_handle_domain_irq(intr_ctrl->irq_domain, 0);
> +       return ret ? IRQ_NONE : IRQ_HANDLED;
> +}
> +
> +static struct pci_dev_intr_ctrl *pci_dev_create_intr_ctrl(struct pci_dev *pdev)
> +{
> +       struct pci_dev_intr_ctrl *intr_ctrl;
> +       struct fwnode_handle *fwnode;
> +       int ret;
> +
> +       if (!pdev->irq)
> +               return ERR_PTR(-EOPNOTSUPP);
> +
> +       fwnode = dev_fwnode(&pdev->dev);
> +       if (!fwnode)
> +               return ERR_PTR(-ENODEV);
> +
> +       intr_ctrl = kmalloc(sizeof(*intr_ctrl), GFP_KERNEL);
> +       if (!intr_ctrl)
> +               return ERR_PTR(-ENOMEM);
> +
> +       intr_ctrl->pci_dev = pdev;
> +
> +       intr_ctrl->irq_domain = irq_domain_create_linear(fwnode, 1, &pci_dev_irq_domain_ops,
> +                                                        intr_ctrl);
> +       if (!intr_ctrl->irq_domain) {
> +               pci_err(pdev, "Failed to create irqdomain\n");
> +               ret = -ENOMEM;
> +               goto err_free_intr_ctrl;
> +       }
> +
> +       ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_LEGACY);
> +       if (ret < 0) {
> +               pci_err(pdev, "Unable alloc irq vector (%d)\n", ret);
> +               goto err_remove_domain;
> +       }
> +       intr_ctrl->irq = pci_irq_vector(pdev, 0);
> +       ret = request_irq(intr_ctrl->irq, pci_dev_irq_handler, IRQF_SHARED,
> +                         dev_name(&pdev->dev), intr_ctrl);
> +       if (ret) {
> +               pci_err(pdev, "Unable to request irq %d (%d)\n", intr_ctrl->irq, ret);
> +               goto err_free_irq_vector;
> +       }
> +
> +       return intr_ctrl;
> +
> +err_free_irq_vector:
> +       pci_free_irq_vectors(pdev);
> +err_remove_domain:
> +       irq_domain_remove(intr_ctrl->irq_domain);
> +err_free_intr_ctrl:
> +       kfree(intr_ctrl);
> +       return ERR_PTR(ret);
> +}
> +
> +static void pci_dev_remove_intr_ctrl(struct pci_dev_intr_ctrl *intr_ctrl)
> +{
> +       free_irq(intr_ctrl->irq, intr_ctrl);
> +       pci_free_irq_vectors(intr_ctrl->pci_dev);
> +       irq_dispose_mapping(irq_find_mapping(intr_ctrl->irq_domain, 0));
> +       irq_domain_remove(intr_ctrl->irq_domain);
> +       kfree(intr_ctrl);
> +}
> +

It looks like the two functions below (and their helper functions) are so
generic that they could be part of the pci driver core support.
Any plans for that?

> +static void devm_pci_dev_remove_intr_ctrl(void *data)
> +{
> +       struct pci_dev_intr_ctrl *intr_ctrl = data;
> +
> +       pci_dev_remove_intr_ctrl(intr_ctrl);
> +}
> +
> +static int devm_pci_dev_create_intr_ctrl(struct pci_dev *pdev)
> +{
> +       struct pci_dev_intr_ctrl *intr_ctrl;
> +
> +       intr_ctrl = pci_dev_create_intr_ctrl(pdev);
> +
> +       if (IS_ERR(intr_ctrl))
> +               return PTR_ERR(intr_ctrl);
> +
> +       return devm_add_action_or_reset(&pdev->dev, devm_pci_dev_remove_intr_ctrl, intr_ctrl);
> +}
> +

...
[snip]
...

> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index eff7f5df08e2..9933f245b781 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -6241,6 +6241,7 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0xa76e, dpc_log_size);
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_XILINX, 0x5020, of_pci_make_dev_node);
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_XILINX, 0x5021, of_pci_make_dev_node);
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_REDHAT, 0x0005, of_pci_make_dev_node);
> +DECLARE_PCI_FIXUP_FINAL(0x1055, 0x9660, of_pci_make_dev_node);
>
>  /*
>   * Devices known to require a longer delay before first config space access
> --
> 2.44.0

Best Regards
Steen