| Message ID | 20240430083730.134918-1-herve.codina@bootlin.com |
|---|---|
| Headers | show |
| Series | Add support for the LAN966x PCI device using a DT overlay | expand |
> -----Original Message----- > From: Herve Codina <herve.codina@bootlin.com> > Sent: Tuesday, April 30, 2024 2:07 PM > To: Herve Codina <herve.codina@bootlin.com>; Thomas Gleixner > <tglx@linutronix.de>; Rob Herring <robh@kernel.org>; Krzysztof Kozlowski > <krzk+dt@kernel.org>; Conor Dooley <conor+dt@kernel.org>; David S. Miller > <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub > Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Lee Jones > <lee@kernel.org>; Arnd Bergmann <arnd@arndb.de>; Horatiu Vultur > <horatiu.vultur@microchip.com>; UNGLinuxDriver@microchip.com; Andrew > Lunn <andrew@lunn.ch>; Heiner Kallweit <hkallweit1@gmail.com>; Russell > King <linux@armlinux.org.uk>; Saravana Kannan <saravanak@google.com>; > Bjorn Helgaas <bhelgaas@google.com>; Philipp Zabel > <p.zabel@pengutronix.de>; Lars Povlsen <lars.povlsen@microchip.com>; > Steen Hegelund <Steen.Hegelund@microchip.com>; Daniel Machon > <daniel.machon@microchip.com>; Alexandre Belloni > <alexandre.belloni@bootlin.com> > Cc: linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; > netdev@vger.kernel.org; linux-pci@vger.kernel.org; linux-arm- > kernel@lists.infradead.org; Allan Nielsen <allan.nielsen@microchip.com>; > Steen Hegelund <steen.hegelund@microchip.com>; Luca Ceresoli > <luca.ceresoli@bootlin.com>; Thomas Petazzoni > <thomas.petazzoni@bootlin.com> > Subject: [PATCH 07/17] net: mdio: mscc-miim: Handle the switch > reset > > The mscc-miim device can be impacted by the switch reset, at least when this > device is part of the LAN966x PCI device. > > Handle this newly added (optional) resets property. > > Signed-off-by: Herve Codina <herve.codina@bootlin.com> > --- > drivers/net/mdio/mdio-mscc-miim.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/net/mdio/mdio-mscc-miim.c b/drivers/net/mdio/mdio- > mscc-miim.c > index c29377c85307..6a6c1768f533 100644 > --- a/drivers/net/mdio/mdio-mscc-miim.c > +++ b/drivers/net/mdio/mdio-mscc-miim.c > @@ -19,6 +19,7 @@ > #include <linux/platform_device.h> > #include <linux/property.h> > #include <linux/regmap.h> > +#include <linux/reset.h> > > #define MSCC_MIIM_REG_STATUS 0x0 > #define MSCC_MIIM_STATUS_STAT_PENDING BIT(2) > @@ -270,11 +271,18 @@ static int mscc_miim_probe(struct platform_device > *pdev) { > struct device_node *np = pdev->dev.of_node; > struct regmap *mii_regmap, *phy_regmap; > + struct reset_control *reset; Please follow reverse x-mass tree order > struct device *dev = &pdev->dev; > struct mscc_miim_dev *miim; > struct mii_bus *bus; > int ret; > > + reset = devm_reset_control_get_optional_shared(dev, "switch"); > + if (IS_ERR(reset)) > + return dev_err_probe(dev, PTR_ERR(reset), "Failed to get > reset\n"); > + > + reset_control_reset(reset); > + > mii_regmap = ocelot_regmap_from_resource(pdev, 0, > > &mscc_miim_regmap_config); > if (IS_ERR(mii_regmap)) > -- > 2.44.0 >
On Tue, Apr 30, 2024 at 10:37:09AM +0200, Herve Codina wrote: > Hi, > > This series adds support for the LAN966x chip when used as a PCI > device. > This patch series for now adds a Device Tree overlay that describes an > initial subset of the devices available over PCI in the LAN996x, and > follow-up patch series will add support for more once this initial > support has landed. What host systems have you tested with? Are they all native DT, or have you tested on an ACPI system? I'm just wondering how well DT overlay works if i were to plug a LAN966x device into something like an x86 ComExpress board? Andrew
On Tue, Apr 30, 2024 at 10:37:16AM +0200, Herve Codina wrote: > The mscc-miim device can be impacted by the switch reset, at least when > this device is part of the LAN966x PCI device. Just to be sure i understand this correctly. The MDIO bus master can be reset using the "switch" reset. So you are adding code to ensure the "switch" reset is out of reset state, so the MDIO bus master works. Given that this is a new property, maybe give it a better name to indicate it resets both the switch and the MDIO bus master? Andrew
On Tue, Apr 30, 2024 at 10:37:17AM +0200, Herve Codina wrote: > A debugfs directory entry is create early during probe(). This entry is > not removed on error path leading to some "already present" issues in > case of EPROBE_DEFER. > > Create this entry later in the probe() code to avoid the need to change > many 'return' in 'goto' and add the removal in the already present error > path. > > Fixes: 942814840127 ("net: lan966x: Add VCAP debugFS support") > Cc: <stable@vger.kernel.org> > Signed-off-by: Herve Codina <herve.codina@bootlin.com> I know you plan to split this patchset up and submit via different subsystems. When you do, please post this for net, not net-next, since it is a fix. Andrew
On Tue, Apr 30, 2024 at 10:37:17AM +0200, Herve Codina wrote: > A debugfs directory entry is create early during probe(). This entry is > not removed on error path leading to some "already present" issues in > case of EPROBE_DEFER. > > Create this entry later in the probe() code to avoid the need to change > many 'return' in 'goto' and add the removal in the already present error > path. > > Fixes: 942814840127 ("net: lan966x: Add VCAP debugFS support") > Cc: <stable@vger.kernel.org> > Signed-off-by: Herve Codina <herve.codina@bootlin.com> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
Hi Andrew, On Tue, 30 Apr 2024 15:46:18 +0200 Andrew Lunn <andrew@lunn.ch> wrote: > On Tue, Apr 30, 2024 at 10:37:16AM +0200, Herve Codina wrote: > > The mscc-miim device can be impacted by the switch reset, at least when > > this device is part of the LAN966x PCI device. > > Just to be sure i understand this correctly. The MDIO bus master can > be reset using the "switch" reset. So you are adding code to ensure > the "switch" reset is out of reset state, so the MDIO bus master > works. Exactly. > > Given that this is a new property, maybe give it a better name to > indicate it resets both the switch and the MDIO bus master? > Replied in the patch in the same series introducing the property [PATCH 06/17] dt-bindings: net: mscc-miim: Add resets property Thanks for the feedback. Best regards, Hervé
Hi Andrew, On Tue, 30 Apr 2024 15:40:16 +0200 Andrew Lunn <andrew@lunn.ch> wrote: > On Tue, Apr 30, 2024 at 10:37:09AM +0200, Herve Codina wrote: > > Hi, > > > > This series adds support for the LAN966x chip when used as a PCI > > device. > > > This patch series for now adds a Device Tree overlay that describes an > > initial subset of the devices available over PCI in the LAN996x, and > > follow-up patch series will add support for more once this initial > > support has landed. > > What host systems have you tested with? Are they all native DT, or > have you tested on an ACPI system? I'm just wondering how well DT > overlay works if i were to plug a LAN966x device into something like > an x86 ComExpress board? I tested on a native DT system (marvell board). Also I tested on a x86 system (basically a simple PC). Not all components are available upstream to have it working on a x86 (ACPI) system. The missing component is not related to the LAN966x PCI driver itself but in the way DT node are created up to the PCI device. A DT node at PCI device is needed to have a DT node parent (target) for the overlay. The DT node chain is also important because BARs address translations are done using the 'ranges' property available in each node in the chain. On a full DT system, the DT looks like (simplified in naming and without the node properties): / soc { ... pci_root_bridge { <-- Present in base dts pci_to_pci_bridge { <-- Created at runtime based on PCI enumeration pci_device { <-- Created at runtime based on PCI enumeration } } pci_device { <-- Created at runtime based on PCI enumeration }; }; }; On x86: - A DT root empty node is created. This is already upstream from a first proposal [1] and then second one [2]. - PCI-to-PCI bridge and device DT nodes are created at runtime. This is the same on DT base systems and this feature is available upstream too (last proposal at [3]). The DT node missing in the chain is the node for the PCI root bridge. On ACPI, no "base" dts describes this node. The PCI root bridge is described by ACPI. I have some draft code that create this DT node when a PCI host bridge is register if a DT node is not already present and so fill the hole in the DT node chain. With that the DT in an ACPI system looks like this: / pci_root_bridge { <-- Created at runtime when a PCI host bridge is registered pci_to_pci_bridge { <-- Created at runtime based on PCI enumeration pci_device { <-- Created at runtime based on PCI enumeration } } pci_device { <-- Created at runtime based on PCI enumeration }; }; With this node added, the driver works the same way in both DT and ACPI systems without any modification. I plan to send the code creating the PCI host bridge node when this current series is landed in order to add support for DT overlay over PCI on x86 systems. Also an other series (under review [4]) is needed to avoid struct device duplication related to the DT nodes creation. [1] https://lore.kernel.org/lkml/20230317053415.2254616-1-frowand.list@gmail.com/#r [2] https://lore.kernel.org/lkml/20240217010557.2381548-1-sboyd@kernel.org/ [3] https://lore.kernel.org/lkml/1692120000-46900-1-git-send-email-lizhi.hou@amd.com/ [4] https://lore.kernel.org/lkml/20240423145703.604489-1-herve.codina@bootlin.com/ Best regards, Hervé
> Also I tested on a x86 system (basically a simple PC). > Not all components are available upstream to have it working on a x86 (ACPI) > system. The missing component is not related to the LAN966x PCI driver itself > but in the way DT node are created up to the PCI device. Good to hear it nearly "just works". There does not seem to be any interest in describing complex network devices like this using ACPI, which is many years behind what we have in DT in terms of building blocks for networking devices. Like many PCIe devices, the LAN966x is pretty much self contained, so fits DT overlays nicely. There is also a slowly growing trend to have PCIe network devices which Linux controls, rather than offloading to firmware. The wangxun drivers are another example. So it is great to see the remaining pieces being put in place to support this. Thanks Andrew
On Tue, Apr 30, 2024 at 10:37:21AM +0200, Herve Codina wrote: > The Microchip LAN966x outband interrupt controller (OIC) maps the > internal interrupt sources of the LAN966x device to an external > interrupt. > When the LAN966x device is used as a PCI device, the external interrupt > is routed to the PCI interrupt. > > Signed-off-by: Herve Codina <herve.codina@bootlin.com> Hi Herve, > +static int lan966x_oic_probe(struct platform_device *pdev) > +{ > + struct device_node *node = pdev->dev.of_node; > + struct lan966x_oic_data *lan966x_oic; > + struct device *dev = &pdev->dev; > + struct irq_chip_generic *gc; > + int ret; > + int i; > + > + lan966x_oic = devm_kmalloc(dev, sizeof(*lan966x_oic), GFP_KERNEL); > + if (!lan966x_oic) > + return -ENOMEM; > + > + lan966x_oic->regs = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(lan966x_oic->regs)) > + return dev_err_probe(dev, PTR_ERR(lan966x_oic->regs), > + "failed to map resource\n"); > + > + lan966x_oic->domain = irq_domain_alloc_linear(of_node_to_fwnode(node), > + LAN966X_OIC_NR_IRQ, > + &irq_generic_chip_ops, NULL); nit: Please consider limiting lines to 80 columns wide in Networking code. > + if (!lan966x_oic->domain) { > + dev_err(dev, "failed to create an IRQ domain\n"); > + return -EINVAL; > + } > + > + lan966x_oic->irq = platform_get_irq(pdev, 0); > + if (lan966x_oic->irq < 0) { > + dev_err_probe(dev, lan966x_oic->irq, "failed to get the IRQ\n"); > + goto err_domain_free; Hi, This will result in the function returning ret. However, ret is uninitialised here. Flagged by W=1 builds with clang-18, and Smatch. > + } > + > + ret = irq_alloc_domain_generic_chips(lan966x_oic->domain, 32, 1, "lan966x-oic", > + handle_level_irq, 0, 0, 0); > + if (ret) { > + dev_err_probe(dev, ret, "failed to alloc irq domain gc\n"); > + goto err_domain_free; > + } > + > + /* Init chips */ > + BUILD_BUG_ON(DIV_ROUND_UP(LAN966X_OIC_NR_IRQ, 32) != ARRAY_SIZE(lan966x_oic_chip_regs)); > + for (i = 0; i < ARRAY_SIZE(lan966x_oic_chip_regs); i++) { > + gc = irq_get_domain_generic_chip(lan966x_oic->domain, i * 32); > + lan966x_oic_chip_init(lan966x_oic, gc, &lan966x_oic_chip_regs[i]); > + } > + > + irq_set_chained_handler_and_data(lan966x_oic->irq, lan966x_oic_irq_handler, > + lan966x_oic->domain); > + > + irq_domain_publish(lan966x_oic->domain); > + platform_set_drvdata(pdev, lan966x_oic); > + return 0; > + > +err_domain_free: > + irq_domain_free(lan966x_oic->domain); > + return ret; > +} > + > +static void lan966x_oic_remove(struct platform_device *pdev) > +{ > + struct lan966x_oic_data *lan966x_oic = platform_get_drvdata(pdev); > + struct irq_chip_generic *gc; > + int i; > + > + for (i = 0; i < ARRAY_SIZE(lan966x_oic_chip_regs); i++) { > + gc = irq_get_domain_generic_chip(lan966x_oic->domain, i * 32); > + lan966x_oic_chip_exit(gc); > + } > + > + irq_set_chained_handler_and_data(lan966x_oic->irq, NULL, NULL); > + > + for (i = 0; i < LAN966X_OIC_NR_IRQ; i++) > + irq_dispose_mapping(irq_find_mapping(lan966x_oic->domain, i)); > + > + irq_domain_unpublish(lan966x_oic->domain); > + > + for (i = 0; i < ARRAY_SIZE(lan966x_oic_chip_regs); i++) { > + gc = irq_get_domain_generic_chip(lan966x_oic->domain, i * 32); > + irq_remove_generic_chip(gc, ~0, 0, 0); > + } > + > + kfree(lan966x_oic->domain->gc); > + irq_domain_free(lan966x_oic->domain); > +} ...
On Tue, Apr 30, 2024 at 10:37:10AM +0200, Herve Codina wrote: > From: Clément Léger <clement.leger@bootlin.com> > > Syscon releasing is not supported. > Without release function, unbinding a driver that uses syscon whether > explicitly or due to a module removal left the used syscon in a in-use > state. > > For instance a syscon_node_to_regmap() call from a consumer retrieve a > syscon regmap instance. Internally, syscon_node_to_regmap() can create > syscon instance and add it to the existing syscon list. No API is > available to release this syscon instance, remove it from the list and > free it when it is not used anymore. > > Introduce reference counting in syscon in order to keep track of syscon > usage using syscon_{get,put}() and add a device managed version of > syscon_regmap_lookup_by_phandle(), to automatically release the syscon > instance on the consumer removal. > > Signed-off-by: Clément Léger <clement.leger@bootlin.com> > Signed-off-by: Herve Codina <herve.codina@bootlin.com> ... > diff --git a/include/linux/mfd/syscon.h b/include/linux/mfd/syscon.h > index c315903f6dab..164b9bcb49c3 100644 > --- a/include/linux/mfd/syscon.h > +++ b/include/linux/mfd/syscon.h > @@ -15,6 +15,7 @@ > #include <linux/errno.h> > > struct device_node; > +struct device; > > #ifdef CONFIG_MFD_SYSCON > struct regmap *device_node_to_regmap(struct device_node *np); > @@ -28,6 +29,11 @@ struct regmap *syscon_regmap_lookup_by_phandle_args(struct device_node *np, > unsigned int *out_args); > struct regmap *syscon_regmap_lookup_by_phandle_optional(struct device_node *np, > const char *property); > +void syscon_put_regmap(struct regmap *regmap); > + > +struct regmap *devm_syscon_regmap_lookup_by_phandle(struct device *dev, > + struct device_node *np, > + const char *property); > #else > static inline struct regmap *device_node_to_regmap(struct device_node *np) > { > @@ -67,6 +73,18 @@ static inline struct regmap *syscon_regmap_lookup_by_phandle_optional( > return NULL; > } > > +static intline void syscon_put_regmap(struct regmap *regmap) intline -> inline > +{ > +} ...
On Tue, Apr 30 2024 at 10:37, Herve Codina wrote: > /** > * irq_domain_xlate_onecell() - Generic xlate for direct one cell bindings > + * @d: IRQ domain involved in the translation Please write out 'Interrupt domain' > + * @ctrlr: the DT node for the device whose interrupt we're translating Device tree node. And we are not translating anything. > + * @intspec: the interrupt specifier data from the DT > + * @intsize: the number of entries in @intspec > + * @out_hwirq: pointer at which to write the hwirq number Pointer to starage for the hardware interrupt number > + * @out_type: pointer at which to write the interrupt type ... Please align these in tabular fashion: + * @d: Interrupt domain involved in the translation + * @ctrlr: The device tree node for the device whose interrupt is translated + * @intspec: The interrupt specifier data from the device tree + * @intsize: The number of entries in @intspec + * @out_hwirq: Pointer to storage for the hardware interrupt number + * @out_type: Pointer to storage for the interrupt type > /** > * irq_domain_translate_onecell() - Generic translate for direct one cell > * bindings > + * @d: IRQ domain involved in the translation > + * @fwspec: FW interrupt specifier to translate Firmware interrupt specifier Thanks, tglx
Hi Simon, On Tue, 30 Apr 2024 21:24:51 +0100 Simon Horman <horms@kernel.org> wrote: > On Tue, Apr 30, 2024 at 10:37:21AM +0200, Herve Codina wrote: > > The Microchip LAN966x outband interrupt controller (OIC) maps the > > internal interrupt sources of the LAN966x device to an external > > interrupt. > > When the LAN966x device is used as a PCI device, the external interrupt > > is routed to the PCI interrupt. > > > > Signed-off-by: Herve Codina <herve.codina@bootlin.com> > > Hi Herve, > > > +static int lan966x_oic_probe(struct platform_device *pdev) > > +{ > > + struct device_node *node = pdev->dev.of_node; > > + struct lan966x_oic_data *lan966x_oic; > > + struct device *dev = &pdev->dev; > > + struct irq_chip_generic *gc; > > + int ret; > > + int i; > > + > > + lan966x_oic = devm_kmalloc(dev, sizeof(*lan966x_oic), GFP_KERNEL); > > + if (!lan966x_oic) > > + return -ENOMEM; > > + > > + lan966x_oic->regs = devm_platform_ioremap_resource(pdev, 0); > > + if (IS_ERR(lan966x_oic->regs)) > > + return dev_err_probe(dev, PTR_ERR(lan966x_oic->regs), > > + "failed to map resource\n"); > > + > > + lan966x_oic->domain = irq_domain_alloc_linear(of_node_to_fwnode(node), > > + LAN966X_OIC_NR_IRQ, > > + &irq_generic_chip_ops, NULL); > > nit: Please consider limiting lines to 80 columns wide in Networking code. This will be done in the next iteration. > > > + if (!lan966x_oic->domain) { > > + dev_err(dev, "failed to create an IRQ domain\n"); > > + return -EINVAL; > > + } > > + > > + lan966x_oic->irq = platform_get_irq(pdev, 0); > > + if (lan966x_oic->irq < 0) { > > + dev_err_probe(dev, lan966x_oic->irq, "failed to get the IRQ\n"); > > + goto err_domain_free; > > Hi, > > This will result in the function returning ret. > However, ret is uninitialised here. > > Flagged by W=1 builds with clang-18, and Smatch. Indeed, this fill be fixed in the next iteration. Best regards, Hervé
Hi Sai, On Tue, 30 Apr 2024 09:21:57 +0000 Sai Krishna Gajula <saikrishnag@marvell.com> wrote: ... > > @@ -270,11 +271,18 @@ static int mscc_miim_probe(struct platform_device > > *pdev) { > > struct device_node *np = pdev->dev.of_node; > > struct regmap *mii_regmap, *phy_regmap; > > + struct reset_control *reset; > > Please follow reverse x-mass tree order > Sure, this will be fixed in the next iteration. Best regards, Hervé
Hi Simon, On Tue, 30 Apr 2024 21:34:43 +0100 Simon Horman <horms@kernel.org> wrote: ... > > +static intline void syscon_put_regmap(struct regmap *regmap) > > intline -> inline Sure, this will be fixed in the next iteration. Best regards, Hervé
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 > > The Microchip LAN966x outband interrupt controller (OIC) maps the > internal interrupt sources of the LAN966x device to an external > interrupt. > When the LAN966x device is used as a PCI device, the external interrupt > is routed to the PCI interrupt. > > Signed-off-by: Herve Codina <herve.codina@bootlin.com> > --- > drivers/irqchip/Kconfig | 12 ++ > drivers/irqchip/Makefile | 1 + > drivers/irqchip/irq-lan966x-oic.c | 301 ++++++++++++++++++++++++++++++ > 3 files changed, 314 insertions(+) > create mode 100644 drivers/irqchip/irq-lan966x-oic.c > > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig > index 72c07a12f5e1..973eebc8d1d1 100644 > --- a/drivers/irqchip/Kconfig > +++ b/drivers/irqchip/Kconfig > @@ -169,6 +169,18 @@ config IXP4XX_IRQ > select IRQ_DOMAIN > select SPARSE_IRQ > > +config LAN966X_OIC > + tristate "Microchip LAN966x OIC Support" > + select GENERIC_IRQ_CHIP > + select IRQ_DOMAIN > + help > + Enable support for the LAN966x Outbound Interrupt Controller. > + This controller is present on the Microchip LAN966x PCI device and > + maps the internal interrupts sources to PCIe interrupt. > + > + To compile this driver as a module, choose M here: the module > + will be called irq-lan966x-oic. > + > config MADERA_IRQ > tristate > > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile > index ec4a18380998..762d3078aa3b 100644 > --- a/drivers/irqchip/Makefile > +++ b/drivers/irqchip/Makefile > @@ -101,6 +101,7 @@ obj-$(CONFIG_IMX_IRQSTEER) += irq-imx-irqsteer.o > obj-$(CONFIG_IMX_INTMUX) += irq-imx-intmux.o > obj-$(CONFIG_IMX_MU_MSI) += irq-imx-mu-msi.o > obj-$(CONFIG_MADERA_IRQ) += irq-madera.o > +obj-$(CONFIG_LAN966X_OIC) += irq-lan966x-oic.o > obj-$(CONFIG_LS1X_IRQ) += irq-ls1x.o > obj-$(CONFIG_TI_SCI_INTR_IRQCHIP) += irq-ti-sci-intr.o > obj-$(CONFIG_TI_SCI_INTA_IRQCHIP) += irq-ti-sci-inta.o > diff --git a/drivers/irqchip/irq-lan966x-oic.c b/drivers/irqchip/irq-lan966x-oic.c > new file mode 100644 > index 000000000000..342f0dbf16e3 > --- /dev/null > +++ b/drivers/irqchip/irq-lan966x-oic.c > @@ -0,0 +1,301 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Driver for the Microchip LAN966x outbound interrupt controller > + * > + * Copyright (c) 2024 Technology Inc. and its subsidiaries. > + * > + * Authors: > + * Horatiu Vultur <horatiu.vultur@microchip.com> > + * Clément Léger <clement.leger@bootlin.com> > + * Herve Codina <herve.codina@bootlin.com> > + */ > + > +#include <linux/bitops.h> > +#include <linux/build_bug.h> > +#include <linux/interrupt.h> > +#include <linux/irqchip/chained_irq.h> > +#include <linux/irqchip.h> > +#include <linux/irq.h> > +#include <linux/iopoll.h> > +#include <linux/mfd/core.h> > +#include <linux/module.h> > +#include <linux/pci.h> > +#include <linux/delay.h> > + > +struct lan966x_oic_chip_regs { > + int reg_off_ena_set; > + int reg_off_ena_clr; > + int reg_off_sticky; > + int reg_off_ident; > + int reg_off_map; > +}; > + > +struct lan966x_oic_data { > + struct irq_domain *domain; > + void __iomem *regs; > + int irq; > +}; > + > +#define LAN966X_OIC_NR_IRQ 86 > + > +/* Interrupt sticky status */ > +#define LAN966X_OIC_INTR_STICKY 0x30 > +#define LAN966X_OIC_INTR_STICKY1 0x34 > +#define LAN966X_OIC_INTR_STICKY2 0x38 > + > +/* Interrupt enable */ > +#define LAN966X_OIC_INTR_ENA 0x48 > +#define LAN966X_OIC_INTR_ENA1 0x4c > +#define LAN966X_OIC_INTR_ENA2 0x50 > + > +/* Atomic clear of interrupt enable */ > +#define LAN966X_OIC_INTR_ENA_CLR 0x54 > +#define LAN966X_OIC_INTR_ENA_CLR1 0x58 > +#define LAN966X_OIC_INTR_ENA_CLR2 0x5c > + > +/* Atomic set of interrupt */ > +#define LAN966X_OIC_INTR_ENA_SET 0x60 > +#define LAN966X_OIC_INTR_ENA_SET1 0x64 > +#define LAN966X_OIC_INTR_ENA_SET2 0x68 > + > +/* Mapping of source to destination interrupts (_n = 0..8) */ Are the indices really needed on LAN966X_OIC_DST_INTR_MAP* and _IDENT* You do not appear to be using them? > +#define LAN966X_OIC_DST_INTR_MAP(_n) 0x78 > +#define LAN966X_OIC_DST_INTR_MAP1(_n) 0x9c > +#define LAN966X_OIC_DST_INTR_MAP2(_n) 0xc0 > + > +/* Currently active interrupt sources per destination (_n = 0..8) */ > +#define LAN966X_OIC_DST_INTR_IDENT(_n) 0xe4 > +#define LAN966X_OIC_DST_INTR_IDENT1(_n) 0x108 > +#define LAN966X_OIC_DST_INTR_IDENT2(_n) 0x12c > + > +static unsigned int lan966x_oic_irq_startup(struct irq_data *data) > +{ > + struct irq_chip_generic *gc = irq_data_get_irq_chip_data(data); > + struct irq_chip_type *ct = irq_data_get_chip_type(data); > + struct lan966x_oic_chip_regs *chip_regs = gc->private; > + u32 map; > + > + irq_gc_lock(gc); > + > + /* Map the source interrupt to the destination */ > + map = irq_reg_readl(gc, chip_regs->reg_off_map); > + map |= data->mask; > + irq_reg_writel(gc, map, chip_regs->reg_off_map); > + > + irq_gc_unlock(gc); > + > + ct->chip.irq_ack(data); > + ct->chip.irq_unmask(data); > + > + return 0; > +} > + > +static void lan966x_oic_irq_shutdown(struct irq_data *data) > +{ > + struct irq_chip_generic *gc = irq_data_get_irq_chip_data(data); > + struct irq_chip_type *ct = irq_data_get_chip_type(data); > + struct lan966x_oic_chip_regs *chip_regs = gc->private; > + u32 map; > + > + ct->chip.irq_mask(data); > + > + irq_gc_lock(gc); > + > + /* Unmap the interrupt */ > + map = irq_reg_readl(gc, chip_regs->reg_off_map); > + map &= ~data->mask; > + irq_reg_writel(gc, map, chip_regs->reg_off_map); > + > + irq_gc_unlock(gc); > +} > + > +static int lan966x_oic_irq_set_type(struct irq_data *data, unsigned int flow_type) > +{ > + if (flow_type != IRQ_TYPE_LEVEL_HIGH) { > + pr_err("lan966x oic doesn't support flow type %d\n", flow_type); > + return -EINVAL; > + } > + > + return 0; > +} > + > +static void lan966x_oic_irq_handler_domain(struct irq_domain *d, u32 first_irq) > +{ > + struct irq_chip_generic *gc = irq_get_domain_generic_chip(d, first_irq); > + struct lan966x_oic_chip_regs *chip_regs = gc->private; > + unsigned long ident; > + unsigned int hwirq; > + > + ident = irq_reg_readl(gc, chip_regs->reg_off_ident); > + if (!ident) > + return; > + > + for_each_set_bit(hwirq, &ident, 32) > + generic_handle_domain_irq(d, hwirq + first_irq); > +} > + > +static void lan966x_oic_irq_handler(struct irq_desc *desc) > +{ > + struct irq_domain *d = irq_desc_get_handler_data(desc); > + struct irq_chip *chip = irq_desc_get_chip(desc); > + > + chained_irq_enter(chip, desc); > + lan966x_oic_irq_handler_domain(d, 0); > + lan966x_oic_irq_handler_domain(d, 32); > + lan966x_oic_irq_handler_domain(d, 64); > + chained_irq_exit(chip, desc); > +} > + > +static struct lan966x_oic_chip_regs lan966x_oic_chip_regs[3] = { > + { > + .reg_off_ena_set = LAN966X_OIC_INTR_ENA_SET, > + .reg_off_ena_clr = LAN966X_OIC_INTR_ENA_CLR, > + .reg_off_sticky = LAN966X_OIC_INTR_STICKY, > + .reg_off_ident = LAN966X_OIC_DST_INTR_IDENT(0), > + .reg_off_map = LAN966X_OIC_DST_INTR_MAP(0), > + }, { > + .reg_off_ena_set = LAN966X_OIC_INTR_ENA_SET1, > + .reg_off_ena_clr = LAN966X_OIC_INTR_ENA_CLR1, > + .reg_off_sticky = LAN966X_OIC_INTR_STICKY1, > + .reg_off_ident = LAN966X_OIC_DST_INTR_IDENT1(0), > + .reg_off_map = LAN966X_OIC_DST_INTR_MAP1(0), > + }, { > + .reg_off_ena_set = LAN966X_OIC_INTR_ENA_SET2, > + .reg_off_ena_clr = LAN966X_OIC_INTR_ENA_CLR2, > + .reg_off_sticky = LAN966X_OIC_INTR_STICKY2, > + .reg_off_ident = LAN966X_OIC_DST_INTR_IDENT2(0), > + .reg_off_map = LAN966X_OIC_DST_INTR_MAP2(0), > + } > +}; > + > +static void lan966x_oic_chip_init(struct lan966x_oic_data *lan966x_oic, > + struct irq_chip_generic *gc, > + struct lan966x_oic_chip_regs *chip_regs) > +{ > + gc->reg_base = lan966x_oic->regs; > + gc->chip_types[0].regs.enable = chip_regs->reg_off_ena_set; > + gc->chip_types[0].regs.disable = chip_regs->reg_off_ena_clr; > + gc->chip_types[0].regs.ack = chip_regs->reg_off_sticky; > + gc->chip_types[0].chip.irq_startup = lan966x_oic_irq_startup; > + gc->chip_types[0].chip.irq_shutdown = lan966x_oic_irq_shutdown; > + gc->chip_types[0].chip.irq_set_type = lan966x_oic_irq_set_type; > + gc->chip_types[0].chip.irq_mask = irq_gc_mask_disable_reg; > + gc->chip_types[0].chip.irq_unmask = irq_gc_unmask_enable_reg; > + gc->chip_types[0].chip.irq_ack = irq_gc_ack_set_bit; > + gc->private = chip_regs; > + > + /* Disable all interrupts handled by this chip */ > + irq_reg_writel(gc, ~0, chip_regs->reg_off_ena_clr); > +} > + > +static void lan966x_oic_chip_exit(struct irq_chip_generic *gc) > +{ > + /* Disable and ack all interrupts handled by this chip */ > + irq_reg_writel(gc, ~0, gc->chip_types[0].regs.disable); > + irq_reg_writel(gc, ~0, gc->chip_types[0].regs.ack); > +} > + > +static int lan966x_oic_probe(struct platform_device *pdev) > +{ > + struct device_node *node = pdev->dev.of_node; > + struct lan966x_oic_data *lan966x_oic; > + struct device *dev = &pdev->dev; > + struct irq_chip_generic *gc; > + int ret; > + int i; > + > + lan966x_oic = devm_kmalloc(dev, sizeof(*lan966x_oic), GFP_KERNEL); > + if (!lan966x_oic) > + return -ENOMEM; > + > + lan966x_oic->regs = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(lan966x_oic->regs)) > + return dev_err_probe(dev, PTR_ERR(lan966x_oic->regs), > + "failed to map resource\n"); > + > + lan966x_oic->domain = irq_domain_alloc_linear(of_node_to_fwnode(node), > + LAN966X_OIC_NR_IRQ, > + &irq_generic_chip_ops, NULL); > + if (!lan966x_oic->domain) { > + dev_err(dev, "failed to create an IRQ domain\n"); > + return -EINVAL; > + } > + > + lan966x_oic->irq = platform_get_irq(pdev, 0); > + if (lan966x_oic->irq < 0) { > + dev_err_probe(dev, lan966x_oic->irq, "failed to get the IRQ\n"); > + goto err_domain_free; > + } > + > + ret = irq_alloc_domain_generic_chips(lan966x_oic->domain, 32, 1, "lan966x-oic", > + handle_level_irq, 0, 0, 0); > + if (ret) { > + dev_err_probe(dev, ret, "failed to alloc irq domain gc\n"); > + goto err_domain_free; > + } > + > + /* Init chips */ > + BUILD_BUG_ON(DIV_ROUND_UP(LAN966X_OIC_NR_IRQ, 32) != ARRAY_SIZE(lan966x_oic_chip_regs)); > + for (i = 0; i < ARRAY_SIZE(lan966x_oic_chip_regs); i++) { > + gc = irq_get_domain_generic_chip(lan966x_oic->domain, i * 32); > + lan966x_oic_chip_init(lan966x_oic, gc, &lan966x_oic_chip_regs[i]); > + } > + > + irq_set_chained_handler_and_data(lan966x_oic->irq, lan966x_oic_irq_handler, > + lan966x_oic->domain); > + > + irq_domain_publish(lan966x_oic->domain); > + platform_set_drvdata(pdev, lan966x_oic); > + return 0; > + > +err_domain_free: > + irq_domain_free(lan966x_oic->domain); > + return ret; > +} > + > +static void lan966x_oic_remove(struct platform_device *pdev) > +{ > + struct lan966x_oic_data *lan966x_oic = platform_get_drvdata(pdev); > + struct irq_chip_generic *gc; > + int i; > + > + for (i = 0; i < ARRAY_SIZE(lan966x_oic_chip_regs); i++) { > + gc = irq_get_domain_generic_chip(lan966x_oic->domain, i * 32); > + lan966x_oic_chip_exit(gc); > + } > + > + irq_set_chained_handler_and_data(lan966x_oic->irq, NULL, NULL); > + > + for (i = 0; i < LAN966X_OIC_NR_IRQ; i++) > + irq_dispose_mapping(irq_find_mapping(lan966x_oic->domain, i)); > + > + irq_domain_unpublish(lan966x_oic->domain); > + > + for (i = 0; i < ARRAY_SIZE(lan966x_oic_chip_regs); i++) { > + gc = irq_get_domain_generic_chip(lan966x_oic->domain, i * 32); > + irq_remove_generic_chip(gc, ~0, 0, 0); > + } > + > + kfree(lan966x_oic->domain->gc); > + irq_domain_free(lan966x_oic->domain); > +} > + > +static const struct of_device_id lan966x_oic_of_match[] = { > + { .compatible = "microchip,lan966x-oic" }, > + {} /* sentinel */ > +}; > +MODULE_DEVICE_TABLE(of, lan966x_oic_of_match); > + > +static struct platform_driver lan966x_oic_driver = { > + .probe = lan966x_oic_probe, > + .remove_new = lan966x_oic_remove, > + .driver = { > + .name = "lan966x-oic", > + .of_match_table = lan966x_oic_of_match, > + }, > +}; > +module_platform_driver(lan966x_oic_driver); > + > +MODULE_AUTHOR("Herve Codina <herve.codina@bootlin.com>"); > +MODULE_DESCRIPTION("Microchip LAN966x OIC driver"); > +MODULE_LICENSE("GPL"); > -- > 2.44.0 Best regards Steen
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
Hi Steen, On Wed, 8 May 2024 08:08:30 +0000 <Steen.Hegelund@microchip.com> wrote: ... > > +/* Mapping of source to destination interrupts (_n = 0..8) */ > > Are the indices really needed on LAN966X_OIC_DST_INTR_MAP* and _IDENT* > You do not appear to be using them? > > > > +#define LAN966X_OIC_DST_INTR_MAP(_n) 0x78 Indeed, I missed them. These registers are defined from 0 to 8 in the document: https://microchip-ung.github.io/lan9662_reginfo/reginfo_LAN9662.html?select=cpu,intr The code use only the indice 0. In the next iteration, I will keep indices and update the definition of registers like that: #define LAN966X_OIC_DST_INTR_MAP(_n) (0x78 + (_n) * 4) Best regards Hervé
Hi Steen, On Wed, 8 May 2024 08:20:04 +0000 <Steen.Hegelund@microchip.com> wrote: ... > > + > > +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? Indeed, I tried to write them in a generic way. Right now, at least for the next iteration of this series, I don't plan to move them as part of the PCI code. This piece of code did not get any feedback and I would prefer to keep them here for the moment. Of course, they could be move out of the LAN966x PCI driver later. > > > +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); > > +} > > + > Best regards, Hervé
Hi, This series adds support for the LAN966x chip when used as a PCI device. For reference, the LAN996x chip is a System-on-chip that integrates an Ethernet switch and a number of other traditional hardware blocks such as a GPIO controller, I2C controllers, SPI controllers, etc. The LAN996x can be used in two different modes: - With Linux running on its Linux built-in ARM cores. This mode is already supported by the upstream Linux kernel, with the LAN996x described as a standard ARM Device Tree in arch/arm/boot/dts/microchip/lan966x.dtsi. Thanks to this support, all hardware blocks in the LAN996x already have drivers in the upstream Linux kernel. - As a PCI device, thanks to its built-in PCI endpoint controller. In this case, the LAN996x ARM cores are not used, but all peripherals of the LAN996x can be accessed by the PCI host using memory-mapped I/O through the PCI BARs. This series aims at supporting this second use-case. As all peripherals of the LAN996x already have drivers in the Linux kernel, our goal is to re-use them as-is to support this second use-case. Therefore, this patch series introduces a PCI driver that binds on the LAN996x PCI VID/PID, and when probed, instantiates all devices that are accessible through the PCI BAR. As the list and characteristics of such devices are non-discoverable, this PCI driver loads a Device Tree overlay that allows to teach the kernel about which devices are available, and allows to probe the relevant drivers in kernel, re-using all existing drivers with no change. This patch series for now adds a Device Tree overlay that describes an initial subset of the devices available over PCI in the LAN996x, and follow-up patch series will add support for more once this initial support has landed. In order to add this PCI driver, a number of preparation changes are needed: - Patches 1 to 5 allow the reset driver used for the LAN996x to be built as a module. Indeed, in the case where Linux runs on the ARM cores, it is common to have the reset driver built-in. However, when the LAN996x is used as a PCI device, it makes sense that all its drivers can be loaded as modules. - Patches 6 and 7 improve the MDIO controller driver to properly handle its reset signal. - Patch 8 fixes an issue in the LAN996x switch driver. - Patches 9 to 13 introduce the internal interrupt controller used in the LAN996x. It is one of the few peripherals in the LAN996x that are only relevant when the LAN996x is used as a PCI device, which is why this interrupt controller did not have a driver so far. - Patches 14 and 15 make some small additions to the OF core and PCI/OF core to consider the PCI device as an interrupt controller. This topic was previously mentioned in [1] to avoid the need of phandle interrupt parents which are not available at some points. - Patches 16 and 17 introduce the LAN996x PCI driver itself, together with its DT bindings. We believe all items from the above list can be merged separately, with no build dependencies. We expect: - Patches 1 to 5 to be taken by reset maintainers - Patches 6, 7 and 8 to be taken by network driver maintainers - Patches 9 to 13 to be taken by irqchip maintainers - Patch 14/15 to be taken by DT/PCI maintainers - Patch 16/17 by the MFD maintainers Additionally, we also believe all preparation items in this patch series can be taken even before there's a final agreement on the last part of the series (the MFD driver itself). [1] https://lore.kernel.org/all/CAL_Jsq+je7+9ATR=B6jXHjEJHjn24vQFs4Tvi9=vhDeK9n42Aw@mail.gmail.com/ Best regards, Hervé Clément Léger (5): mfd: syscon: Add reference counting and device managed support reset: mchp: sparx5: Remove dependencies and allow building as a module reset: mchp: sparx5: Release syscon when not use anymore reset: core: add get_device()/put_device on rcdev reset: mchp: sparx5: set the dev member of the reset controller Herve Codina (12): dt-bindings: net: mscc-miim: Add resets property net: mdio: mscc-miim: Handle the switch reset net: lan966x: remove debugfs directory in probe() error path dt-bindings: interrupt-controller: Add support for Microchip LAN966x OIC irqdomain: Add missing parameter descriptions in docs irqdomain: Introduce irq_domain_alloc() and irq_domain_publish() irqchip: Add support for LAN966x OIC MAINTAINERS: Add the Microchip LAN966x OIC driver entry of: dynamic: Introduce of_changeset_add_prop_bool() pci: of_property: Add the interrupt-controller property in PCI device nodes mfd: Add support for LAN966x PCI device MAINTAINERS: Add the Microchip LAN966x PCI driver entry .../microchip,lan966x-oic.yaml | 55 ++++ .../devicetree/bindings/net/mscc,miim.yaml | 8 + MAINTAINERS | 12 + drivers/irqchip/Kconfig | 12 + drivers/irqchip/Makefile | 1 + drivers/irqchip/irq-lan966x-oic.c | 301 ++++++++++++++++++ drivers/mfd/Kconfig | 24 ++ drivers/mfd/Makefile | 4 + drivers/mfd/lan966x_pci.c | 229 +++++++++++++ drivers/mfd/lan966x_pci.dtso | 167 ++++++++++ drivers/mfd/syscon.c | 145 ++++++++- .../ethernet/microchip/lan966x/lan966x_main.c | 6 +- drivers/net/mdio/mdio-mscc-miim.c | 8 + drivers/of/dynamic.c | 25 ++ drivers/pci/of_property.c | 24 ++ drivers/pci/quirks.c | 1 + drivers/reset/Kconfig | 3 +- drivers/reset/core.c | 2 + drivers/reset/reset-microchip-sparx5.c | 11 +- include/linux/irqdomain.h | 16 + include/linux/mfd/syscon.h | 18 ++ include/linux/of.h | 3 + kernel/irq/irqdomain.c | 118 +++++-- 23 files changed, 1149 insertions(+), 44 deletions(-) create mode 100644 Documentation/devicetree/bindings/interrupt-controller/microchip,lan966x-oic.yaml create mode 100644 drivers/irqchip/irq-lan966x-oic.c create mode 100644 drivers/mfd/lan966x_pci.c create mode 100644 drivers/mfd/lan966x_pci.dtso