[PATCH v2 00/19] Add support for the LAN966x PCI device using a DT overlay

[PATCH v2 00/19] Add support for the LAN966x PCI device using a DT overlay

[Herve Codina]

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.

 - Patches 8 to 12 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 13 to 16 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 17 and 18 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 and 7 to be taken by network driver maintainers

 - Patches 8 to 12 to be taken by irqchip maintainers

 - Patch 13 to 17 to be taken by DT/PCI maintainers

 - Patch 18 and 19 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/

Compare to the previous iteration:
  https://lore.kernel.org/lkml/20240430183301.46568e35@bootlin.com/
this v2 series mainly:
  - Adds tests for of_changeset_add_prop_*() function family
  - Fixes typos, coding style and doc descriptions
  - Fixes a missing ret value assignment in the OIC .probe()
  - Uses indices in the OIC register definition
  - Removes Patch 8 (sent alone to net)
  - Rebases on top of v6.10-rc1

Best regards,
Hervé

Changes v1 -> v2
  - Patch 1
    Fix a typo in syscon.h (s/intline/inline/)

  - Patches 2..5
    No changes

  - Patch 6
    Improve the reset property description

  - Patch 7
    Fix a wrong reverse x-mass tree declaration

  - Patch 8 removed (sent alone to net)
    https://lore.kernel.org/lkml/20240513111853.58668-1-herve.codina@bootlin.com/

  - Patch 8 (v1 patch 9)
    Add 'Reviewed-by: Rob Herring (Arm) <robh@kernel.org>'

  - Patch 9 (v1 patch 10)
    Rephrase and ident parameters descriptions

  - Patch 10 (v1 patch 11)
    No changes

  - Patch 11 (v1 patch 12)
    Fix a missing ret value assignment before a goto in .probe()
    Limit lines to 80 columns
    Use indices in register offset definitions

  - Patch 13 and 14 (new patches in v2)
    Add new test cases for existing of_changeset_add_prop_*()

  - Patch 15 (v1 patch 14)
    No changes

  - Patch 16 (new patches in v2)
    Add tests for of_changeset_add_prop_bool()

  - Patch 17 (v1 patch 15)
    Update commit subject
    Rewrap a paragraph in commit log

  - Patch 18 (v1 patch 16)
    Use PCI_IRQ_INTX instead of PCI_IRQ_LEGACY

  - Patch 19 (v1 patch 17)
    No changes

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 (14):
  dt-bindings: net: mscc-miim: Add resets property
  net: mdio: mscc-miim: Handle the switch reset
  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: Constify parameter in
    of_changeset_add_prop_string_array()
  of: unittest: Add tests for changeset properties adding
  of: dynamic: Introduce of_changeset_add_prop_bool()
  of: unittest: Add a test case for of_changeset_add_prop_bool()
  PCI: of_property: Add 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    |  11 +
 MAINTAINERS                                   |  12 +
 drivers/irqchip/Kconfig                       |  12 +
 drivers/irqchip/Makefile                      |   1 +
 drivers/irqchip/irq-lan966x-oic.c             | 308 ++++++++++++++++++
 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 ++++++++-
 drivers/net/mdio/mdio-mscc-miim.c             |   8 +
 drivers/of/dynamic.c                          |  27 +-
 drivers/of/unittest.c                         | 166 ++++++++++
 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                            |   5 +-
 kernel/irq/irqdomain.c                        | 118 +++++--
 23 files changed, 1323 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

-- 
2.45.0

[PATCH v2 01/19] mfd: syscon: Add reference counting and device managed support

[Herve Codina]

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>
---
 drivers/mfd/syscon.c       | 145 ++++++++++++++++++++++++++++++++++---
 include/linux/mfd/syscon.h |  18 +++++
 2 files changed, 154 insertions(+), 9 deletions(-)

diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
index 7d0e91164cba..86898831b842 100644
--- a/drivers/mfd/syscon.c
+++ b/drivers/mfd/syscon.c
@@ -34,6 +34,7 @@ struct syscon {
 	struct regmap *regmap;
 	struct reset_control *reset;
 	struct list_head list;
+	struct kref refcount;
 };
 
 static const struct regmap_config syscon_regmap_config = {
@@ -147,6 +148,8 @@ static struct syscon *of_syscon_register(struct device_node *np, bool check_res)
 
 	syscon->regmap = regmap;
 	syscon->np = np;
+	of_node_get(syscon->np);
+	kref_init(&syscon->refcount);
 
 	spin_lock(&syscon_list_slock);
 	list_add_tail(&syscon->list, &syscon_list);
@@ -168,7 +171,30 @@ static struct syscon *of_syscon_register(struct device_node *np, bool check_res)
 	return ERR_PTR(ret);
 }
 
-static struct regmap *device_node_get_regmap(struct device_node *np,
+static void syscon_free(struct kref *kref)
+{
+	struct syscon *syscon = container_of(kref, struct syscon, refcount);
+
+	spin_lock(&syscon_list_slock);
+	list_del(&syscon->list);
+	spin_unlock(&syscon_list_slock);
+
+	regmap_exit(syscon->regmap);
+	of_node_put(syscon->np);
+	kfree(syscon);
+}
+
+static void syscon_get(struct syscon *syscon)
+{
+	kref_get(&syscon->refcount);
+}
+
+static void syscon_put(struct syscon *syscon)
+{
+	kref_put(&syscon->refcount, syscon_free);
+}
+
+static struct syscon *device_node_get_syscon(struct device_node *np,
 					     bool check_res)
 {
 	struct syscon *entry, *syscon = NULL;
@@ -183,9 +209,23 @@ static struct regmap *device_node_get_regmap(struct device_node *np,
 
 	spin_unlock(&syscon_list_slock);
 
-	if (!syscon)
+	if (!syscon) {
 		syscon = of_syscon_register(np, check_res);
+		if (IS_ERR(syscon))
+			return ERR_CAST(syscon);
+	} else {
+		syscon_get(syscon);
+	}
+
+	return syscon;
+}
 
+static struct regmap *device_node_get_regmap(struct device_node *np,
+					     bool check_res)
+{
+	struct syscon *syscon;
+
+	syscon = device_node_get_syscon(np, check_res);
 	if (IS_ERR(syscon))
 		return ERR_CAST(syscon);
 
@@ -198,12 +238,23 @@ struct regmap *device_node_to_regmap(struct device_node *np)
 }
 EXPORT_SYMBOL_GPL(device_node_to_regmap);
 
-struct regmap *syscon_node_to_regmap(struct device_node *np)
+static struct syscon *syscon_node_to_syscon(struct device_node *np)
 {
 	if (!of_device_is_compatible(np, "syscon"))
 		return ERR_PTR(-EINVAL);
 
-	return device_node_get_regmap(np, true);
+	return device_node_get_syscon(np, true);
+}
+
+struct regmap *syscon_node_to_regmap(struct device_node *np)
+{
+	struct syscon *syscon;
+
+	syscon = syscon_node_to_syscon(np);
+	if (IS_ERR(syscon))
+		return ERR_CAST(syscon);
+
+	return syscon->regmap;
 }
 EXPORT_SYMBOL_GPL(syscon_node_to_regmap);
 
@@ -223,11 +274,11 @@ struct regmap *syscon_regmap_lookup_by_compatible(const char *s)
 }
 EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_compatible);
 
-struct regmap *syscon_regmap_lookup_by_phandle(struct device_node *np,
-					const char *property)
+static struct syscon *syscon_lookup_by_phandle(struct device_node *np,
+					       const char *property)
 {
 	struct device_node *syscon_np;
-	struct regmap *regmap;
+	struct syscon *syscon;
 
 	if (property)
 		syscon_np = of_parse_phandle(np, property, 0);
@@ -237,12 +288,24 @@ struct regmap *syscon_regmap_lookup_by_phandle(struct device_node *np,
 	if (!syscon_np)
 		return ERR_PTR(-ENODEV);
 
-	regmap = syscon_node_to_regmap(syscon_np);
+	syscon = syscon_node_to_syscon(syscon_np);
 
 	if (property)
 		of_node_put(syscon_np);
 
-	return regmap;
+	return syscon;
+}
+
+struct regmap *syscon_regmap_lookup_by_phandle(struct device_node *np,
+					       const char *property)
+{
+	struct syscon *syscon;
+
+	syscon = syscon_lookup_by_phandle(np, property);
+	if (IS_ERR(syscon))
+		return ERR_CAST(syscon);
+
+	return syscon->regmap;
 }
 EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_phandle);
 
@@ -293,6 +356,70 @@ struct regmap *syscon_regmap_lookup_by_phandle_optional(struct device_node *np,
 }
 EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_phandle_optional);
 
+static struct syscon *syscon_from_regmap(struct regmap *regmap)
+{
+	struct syscon *entry, *syscon = NULL;
+
+	spin_lock(&syscon_list_slock);
+
+	list_for_each_entry(entry, &syscon_list, list)
+		if (entry->regmap == regmap) {
+			syscon = entry;
+			break;
+		}
+
+	spin_unlock(&syscon_list_slock);
+
+	return syscon;
+}
+
+void syscon_put_regmap(struct regmap *regmap)
+{
+	struct syscon *syscon;
+
+	syscon = syscon_from_regmap(regmap);
+	if (!syscon)
+		return;
+
+	syscon_put(syscon);
+}
+EXPORT_SYMBOL_GPL(syscon_put_regmap);
+
+static void devm_syscon_release(struct device *dev, void *res)
+{
+	syscon_put(*(struct syscon **)res);
+}
+
+static struct regmap *__devm_syscon_get(struct device *dev,
+					struct syscon *syscon)
+{
+	struct syscon **ptr;
+
+	if (IS_ERR(syscon))
+		return ERR_CAST(syscon);
+
+	ptr = devres_alloc(devm_syscon_release, sizeof(struct syscon *), GFP_KERNEL);
+	if (!ptr) {
+		syscon_put(syscon);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	*ptr = syscon;
+	devres_add(dev, ptr);
+
+	return syscon->regmap;
+}
+
+struct regmap *devm_syscon_regmap_lookup_by_phandle(struct device *dev,
+						    struct device_node *np,
+						    const char *property)
+{
+	struct syscon *syscon = syscon_lookup_by_phandle(np, property);
+
+	return __devm_syscon_get(dev, syscon);
+}
+EXPORT_SYMBOL_GPL(devm_syscon_regmap_lookup_by_phandle);
+
 static int syscon_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
diff --git a/include/linux/mfd/syscon.h b/include/linux/mfd/syscon.h
index c315903f6dab..f742d865a37a 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 inline void syscon_put_regmap(struct regmap *regmap)
+{
+}
+
+static inline
+struct regmap *devm_syscon_regmap_lookup_by_phandle(struct device *dev,
+						    struct device_node *np,
+						    const char *property)
+{
+	return NULL;
+}
+
 #endif
 
 #endif /* __LINUX_MFD_SYSCON_H__ */
-- 
2.45.0

Re: [PATCH v2 01/19] mfd: syscon: Add reference counting and device managed support

[Arnd Bergmann]

On Mon, May 27, 2024, at 18:14, 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>

This all looks correct from an implementation perspective,
but it does add a lot of complexity if now every syscon user
feels compelled to actually free up their resources again,
while nothing else should actually depend on this.

The only reference I found in your series here is the
reset controller, and it only does a single update to
the regmap in the probe function.

Would it be possible to just make the syscon support in
the reset driver optional and instead poke the register
in the mfd driver itself when this is used as a pci device?
Or do you expect to see the syscon get used in other
places in the future for the PCI case?

      Arnd

Re: [PATCH v2 01/19] mfd: syscon: Add reference counting and device managed support

[Herve Codina]

Hi Arnd,

On Tue, 18 Jun 2024 16:53:30 +0200
"Arnd Bergmann" <arnd@arndb.de> wrote:

> On Mon, May 27, 2024, at 18:14, 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>  
> 
> This all looks correct from an implementation perspective,
> but it does add a lot of complexity if now every syscon user
> feels compelled to actually free up their resources again,
> while nothing else should actually depend on this.
> 
> The only reference I found in your series here is the
> reset controller, and it only does a single update to
> the regmap in the probe function.
> 
> Would it be possible to just make the syscon support in
> the reset driver optional and instead poke the register
> in the mfd driver itself when this is used as a pci device?
> Or do you expect to see the syscon get used in other
> places in the future for the PCI case?
> 

IMHO, I don't think that poking the register in the mfd driver and so
avoiding syscon usage is the right solution.

Indeed, additional devices can be added in the DT overlay and an other
syscon user can be present.

Also, overlays can be used on other PCI devices in the future and these PCI
devices can use drivers that are syscon users. In that case, the same kind
of workaround will be needed and maybe a quite more complex one depending on
syscon users.

The root issue is that syscon does not support removal.
I prefer fixing this root issue instead of finding a kind of workaround.

Even if all syscon users are not fixed right now (and probably don't need to
be fixed), a solution with the new devm_syscon_regmap_lookup_by_phandle() is
available and drivers that need to release their resources because of a
device removal can easily move to devm_syscon_regmap_lookup_by_phandle().

Best regards,
Hervé

[PATCH v2 02/19] reset: mchp: sparx5: Remove dependencies and allow building as a module

[Herve Codina]

From: Clément Léger <clement.leger@bootlin.com>

The sparx5 reset controller depends on the SPARX5 architecture or the
LAN966x SoC.

This reset controller can be used by the LAN966x PCI device and so it
needs to be available on all architectures.
Also the LAN966x PCI device driver can be built as a module and this
reset controller driver has no reason to be a builtin driver in that
case.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/reset/Kconfig                  | 3 +--
 drivers/reset/reset-microchip-sparx5.c | 2 ++
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
index 7112f5932609..fb9005e2f5b5 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -124,8 +124,7 @@ config RESET_LPC18XX
 	  This enables the reset controller driver for NXP LPC18xx/43xx SoCs.
 
 config RESET_MCHP_SPARX5
-	bool "Microchip Sparx5 reset driver"
-	depends on ARCH_SPARX5 || SOC_LAN966 || COMPILE_TEST
+	tristate "Microchip Sparx5 reset driver"
 	default y if SPARX5_SWITCH
 	select MFD_SYSCON
 	help
diff --git a/drivers/reset/reset-microchip-sparx5.c b/drivers/reset/reset-microchip-sparx5.c
index 636e85c388b0..69915c7b4941 100644
--- a/drivers/reset/reset-microchip-sparx5.c
+++ b/drivers/reset/reset-microchip-sparx5.c
@@ -158,6 +158,7 @@ static const struct of_device_id mchp_sparx5_reset_of_match[] = {
 	},
 	{ }
 };
+MODULE_DEVICE_TABLE(of, mchp_sparx5_reset_of_match);
 
 static struct platform_driver mchp_sparx5_reset_driver = {
 	.probe = mchp_sparx5_reset_probe,
@@ -180,3 +181,4 @@ postcore_initcall(mchp_sparx5_reset_init);
 
 MODULE_DESCRIPTION("Microchip Sparx5 switch reset driver");
 MODULE_AUTHOR("Steen Hegelund <steen.hegelund@microchip.com>");
+MODULE_LICENSE("GPL");
-- 
2.45.0

[PATCH v2 03/19] reset: mchp: sparx5: Release syscon when not use anymore

[Herve Codina]

From: Clément Léger <clement.leger@bootlin.com>

The sparx5 reset controller does not release syscon when it is not used
anymore.

This reset controller is used by the LAN966x PCI device driver.
It can be removed from the system at runtime and needs to release its
consumed syscon on removal.

Use the newly introduced devm_syscon_regmap_lookup_by_phandle() in order
to get the syscon and automatically release it on removal.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/reset/reset-microchip-sparx5.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/reset/reset-microchip-sparx5.c b/drivers/reset/reset-microchip-sparx5.c
index 69915c7b4941..c4fe65291a43 100644
--- a/drivers/reset/reset-microchip-sparx5.c
+++ b/drivers/reset/reset-microchip-sparx5.c
@@ -65,15 +65,11 @@ static const struct reset_control_ops sparx5_reset_ops = {
 static int mchp_sparx5_map_syscon(struct platform_device *pdev, char *name,
 				  struct regmap **target)
 {
-	struct device_node *syscon_np;
+	struct device *dev = &pdev->dev;
 	struct regmap *regmap;
 	int err;
 
-	syscon_np = of_parse_phandle(pdev->dev.of_node, name, 0);
-	if (!syscon_np)
-		return -ENODEV;
-	regmap = syscon_node_to_regmap(syscon_np);
-	of_node_put(syscon_np);
+	regmap = devm_syscon_regmap_lookup_by_phandle(dev, dev->of_node, name);
 	if (IS_ERR(regmap)) {
 		err = PTR_ERR(regmap);
 		dev_err(&pdev->dev, "No '%s' map: %d\n", name, err);
-- 
2.45.0

[PATCH v2 04/19] reset: core: add get_device()/put_device on rcdev

[Herve Codina]

From: Clément Léger <clement.leger@bootlin.com>

Since the rcdev structure is allocated by the reset controller drivers
themselves, they need to exists as long as there is a consumer. A call to
module_get() is already existing but that does not work when using
device-tree overlays. In order to guarantee that the underlying reset
controller device does not vanish while using it, add a get_device() call
when retrieving a reset control from a reset controller device and a
put_device() when releasing that control.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/reset/core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index dba74e857be6..999c3c41cf21 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -812,6 +812,7 @@ __reset_control_get_internal(struct reset_controller_dev *rcdev,
 	kref_init(&rstc->refcnt);
 	rstc->acquired = acquired;
 	rstc->shared = shared;
+	get_device(rcdev->dev);
 
 	return rstc;
 }
@@ -826,6 +827,7 @@ static void __reset_control_release(struct kref *kref)
 	module_put(rstc->rcdev->owner);
 
 	list_del(&rstc->list);
+	put_device(rstc->rcdev->dev);
 	kfree(rstc);
 }
 
-- 
2.45.0

[PATCH v2 05/19] reset: mchp: sparx5: set the dev member of the reset controller

[Herve Codina]

From: Clément Léger <clement.leger@bootlin.com>

In order to guarantee the device will not be deleted by the reset
controller consumer, set the dev member of the reset controller.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/reset/reset-microchip-sparx5.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/reset/reset-microchip-sparx5.c b/drivers/reset/reset-microchip-sparx5.c
index c4fe65291a43..1ef2aa1602e3 100644
--- a/drivers/reset/reset-microchip-sparx5.c
+++ b/drivers/reset/reset-microchip-sparx5.c
@@ -117,6 +117,7 @@ static int mchp_sparx5_reset_probe(struct platform_device *pdev)
 		return err;
 
 	ctx->rcdev.owner = THIS_MODULE;
+	ctx->rcdev.dev = &pdev->dev;
 	ctx->rcdev.nr_resets = 1;
 	ctx->rcdev.ops = &sparx5_reset_ops;
 	ctx->rcdev.of_node = dn;
-- 
2.45.0

[PATCH v2 06/19] dt-bindings: net: mscc-miim: Add resets property

[Herve Codina]

Add the (optional) resets property.
The mscc-miim device is impacted by the switch reset especially when the
mscc-miim device is used as part of the LAN966x PCI device.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 Documentation/devicetree/bindings/net/mscc,miim.yaml | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/mscc,miim.yaml b/Documentation/devicetree/bindings/net/mscc,miim.yaml
index 5b292e7c9e46..c67e8caa36cf 100644
--- a/Documentation/devicetree/bindings/net/mscc,miim.yaml
+++ b/Documentation/devicetree/bindings/net/mscc,miim.yaml
@@ -38,6 +38,17 @@ properties:
 
   clock-frequency: true
 
+  resets:
+    items:
+      - description:
+          Optional shared switch reset.
+          This reset is shared with all blocks attached to the Switch Core
+          Register Bus (CSR) including VRAP slave.
+
+  reset-names:
+    items:
+      - const: switch
+
 required:
   - compatible
   - reg
-- 
2.45.0

Re: [PATCH v2 06/19] dt-bindings: net: mscc-miim: Add resets property

[Andrew Lunn]

On Mon, May 27, 2024 at 06:14:33PM +0200, Herve Codina wrote:
> Add the (optional) resets property.
> The mscc-miim device is impacted by the switch reset especially when the
> mscc-miim device is used as part of the LAN966x PCI device.
> 
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v2 06/19] dt-bindings: net: mscc-miim: Add resets property

[Krzysztof Kozlowski]

On 27/05/2024 18:14, Herve Codina wrote:
> Add the (optional) resets property.
> The mscc-miim device is impacted by the switch reset especially when the
> mscc-miim device is used as part of the LAN966x PCI device.
> 
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---
>  Documentation/devicetree/bindings/net/mscc,miim.yaml | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/mscc,miim.yaml b/Documentation/devicetree/bindings/net/mscc,miim.yaml
> index 5b292e7c9e46..c67e8caa36cf 100644
> --- a/Documentation/devicetree/bindings/net/mscc,miim.yaml
> +++ b/Documentation/devicetree/bindings/net/mscc,miim.yaml
> @@ -38,6 +38,17 @@ properties:
>  
>    clock-frequency: true
>  
> +  resets:
> +    items:
> +      - description:
> +          Optional shared switch reset.

If there is going to be resend - drop "optional". You are repeating the
schema in free-form text. No need to resend only for this. Actually drop
entire sentence - it says nothing more than what next sentence is saying.

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

[PATCH v2 07/19] net: mdio: mscc-miim: Handle the switch reset

[Herve Codina]

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..62c47e0dd142 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)
@@ -271,10 +272,17 @@ static int mscc_miim_probe(struct platform_device *pdev)
 	struct device_node *np = pdev->dev.of_node;
 	struct regmap *mii_regmap, *phy_regmap;
 	struct device *dev = &pdev->dev;
+	struct reset_control *reset;
 	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.45.0

Re: [PATCH v2 07/19] net: mdio: mscc-miim: Handle the switch reset

[Andrew Lunn]

On Mon, May 27, 2024 at 06:14:34PM +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.
> 
> Handle this newly added (optional) resets property.
> 
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH v2 08/19] dt-bindings: interrupt-controller: Add support for Microchip LAN966x OIC

[Herve Codina]

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>
Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
---
 .../microchip,lan966x-oic.yaml                | 55 +++++++++++++++++++
 1 file changed, 55 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/microchip,lan966x-oic.yaml

diff --git a/Documentation/devicetree/bindings/interrupt-controller/microchip,lan966x-oic.yaml b/Documentation/devicetree/bindings/interrupt-controller/microchip,lan966x-oic.yaml
new file mode 100644
index 000000000000..b2adc7174177
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/microchip,lan966x-oic.yaml
@@ -0,0 +1,55 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/interrupt-controller/microchip,lan966x-oic.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Microchip LAN966x outband interrupt controller
+
+maintainers:
+  - Herve Codina <herve.codina@bootlin.com>
+
+allOf:
+  - $ref: /schemas/interrupt-controller.yaml#
+
+description: |
+  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.
+
+properties:
+  compatible:
+    const: microchip,lan966x-oic
+
+  '#interrupt-cells':
+    const: 2
+
+  interrupt-controller: true
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+required:
+  - compatible
+  - '#interrupt-cells'
+  - interrupt-controller
+  - interrupts
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    interrupt-controller@e00c0120 {
+        compatible = "microchip,lan966x-oic";
+        reg = <0xe00c0120 0x190>;
+        #interrupt-cells = <2>;
+        interrupt-controller;
+        interrupts = <0>;
+        interrupt-parent = <&intc>;
+    };
+...
-- 
2.45.0

[PATCH v2 09/19] irqdomain: Add missing parameter descriptions in docs

[Herve Codina]

During compilation, several warning of the following form were raised:
  Function parameter or struct member 'x' not described in 'yyy'

Add the missing function parameter descriptions.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 kernel/irq/irqdomain.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index aadc8891cc16..86f8b91b0d3a 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -111,6 +111,7 @@ EXPORT_SYMBOL_GPL(__irq_domain_alloc_fwnode);
 
 /**
  * irq_domain_free_fwnode - Free a non-OF-backed fwnode_handle
+ * @fwnode: fwnode_handle to free
  *
  * Free a fwnode_handle allocated with irq_domain_alloc_fwnode.
  */
@@ -982,6 +983,12 @@ EXPORT_SYMBOL_GPL(__irq_resolve_mapping);
 
 /**
  * irq_domain_xlate_onecell() - Generic xlate for direct one cell bindings
+ * @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
  *
  * Device Tree IRQ specifier translation function which works with one cell
  * bindings where the cell value maps directly to the hwirq number.
@@ -1000,6 +1007,12 @@ EXPORT_SYMBOL_GPL(irq_domain_xlate_onecell);
 
 /**
  * irq_domain_xlate_twocell() - Generic xlate for direct two cell bindings
+ * @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
  *
  * Device Tree IRQ specifier translation function which works with two cell
  * bindings where the cell values map directly to the hwirq number
@@ -1018,6 +1031,12 @@ EXPORT_SYMBOL_GPL(irq_domain_xlate_twocell);
 
 /**
  * irq_domain_xlate_onetwocell() - Generic xlate for one or two cell bindings
+ * @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
  *
  * Device Tree IRQ specifier translation function which works with either one
  * or two cell bindings where the cell values map directly to the hwirq number
@@ -1051,6 +1070,10 @@ EXPORT_SYMBOL_GPL(irq_domain_simple_ops);
 /**
  * irq_domain_translate_onecell() - Generic translate for direct one cell
  * bindings
+ * @d:		Interrupt domain involved in the translation
+ * @fwspec:	The firmware interrupt specifier to translate
+ * @out_hwirq:	Pointer to storage for the hardware interrupt number
+ * @out_type:	Pointer to storage for the interrupt type
  */
 int irq_domain_translate_onecell(struct irq_domain *d,
 				 struct irq_fwspec *fwspec,
@@ -1068,6 +1091,10 @@ EXPORT_SYMBOL_GPL(irq_domain_translate_onecell);
 /**
  * irq_domain_translate_twocell() - Generic translate for direct two cell
  * bindings
+ * @d:		Interrupt domain involved in the translation
+ * @fwspec:	The firmware interrupt specifier to translate
+ * @out_hwirq:	Pointer to storage for the hardware interrupt number
+ * @out_type:	Pointer to storage for the interrupt type
  *
  * Device Tree IRQ specifier translation function which works with two cell
  * bindings where the cell values map directly to the hwirq number
-- 
2.45.0

Re: [PATCH v2 09/19] irqdomain: Add missing parameter descriptions in docs

[Thomas Gleixner]

On Mon, May 27 2024 at 18:14, Herve Codina wrote:
> During compilation, several warning of the following form were raised:
>   Function parameter or struct member 'x' not described in 'yyy'
>
> Add the missing function parameter descriptions.

Sigh. Why is such a cleanup burried in the middle of patch series which
tries to add a network driver?

Re: [PATCH v2 09/19] irqdomain: Add missing parameter descriptions in docs

[Andy Shevchenko]

Mon, May 27, 2024 at 06:14:36PM +0200, Herve Codina kirjoitti:
> During compilation, several warning of the following form were raised:
>   Function parameter or struct member 'x' not described in 'yyy'
> 
> Add the missing function parameter descriptions.

...

>  /**
>   * irq_domain_translate_onecell() - Generic translate for direct one cell
>   * bindings
> + * @d:		Interrupt domain involved in the translation
> + * @fwspec:	The firmware interrupt specifier to translate
> + * @out_hwirq:	Pointer to storage for the hardware interrupt number
> + * @out_type:	Pointer to storage for the interrupt type

(kernel-doc perhaps will complain on something missing here)

>   */
>  int irq_domain_translate_onecell(struct irq_domain *d,

You can go further and run

	scripts/kernel-doc -v -none -Wall ...

against this file and fix more issues, like I believe in the above excerpt.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 09/19] irqdomain: Add missing parameter descriptions in docs

[Herve Codina]

Hi Andy,

On Wed, 5 Jun 2024 23:02:30 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> Mon, May 27, 2024 at 06:14:36PM +0200, Herve Codina kirjoitti:
> > During compilation, several warning of the following form were raised:
> >   Function parameter or struct member 'x' not described in 'yyy'
> > 
> > Add the missing function parameter descriptions.  
> 
> ...
> 
> >  /**
> >   * irq_domain_translate_onecell() - Generic translate for direct one cell
> >   * bindings
> > + * @d:		Interrupt domain involved in the translation
> > + * @fwspec:	The firmware interrupt specifier to translate
> > + * @out_hwirq:	Pointer to storage for the hardware interrupt number
> > + * @out_type:	Pointer to storage for the interrupt type  
> 
> (kernel-doc perhaps will complain on something missing here)
> 
> >   */
> >  int irq_domain_translate_onecell(struct irq_domain *d,  
> 
> You can go further and run
> 
> 	scripts/kernel-doc -v -none -Wall ...
> 
> against this file and fix more issues, like I believe in the above excerpt.
> 

Yes indeed, I missed the return values.
Will be updated in the next iteration.

Best regards,
Hervé

Re: [PATCH v2 09/19] irqdomain: Add missing parameter descriptions in docs

[Andy Shevchenko]

On Thu, Jun 6, 2024 at 10:14 AM Herve Codina <herve.codina@bootlin.com> wrote:
> On Wed, 5 Jun 2024 23:02:30 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

...

> Yes indeed, I missed the return values.
> Will be updated in the next iteration.

Note, Thomas already applied this version, so it should be just a follow up.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 09/19] irqdomain: Add missing parameter descriptions in docs

[Herve Codina]

On Thu, 6 Jun 2024 11:46:25 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Thu, Jun 6, 2024 at 10:14 AM Herve Codina <herve.codina@bootlin.com> wrote:
> > On Wed, 5 Jun 2024 23:02:30 +0300
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:  
> 
> ...
> 
> > Yes indeed, I missed the return values.
> > Will be updated in the next iteration.  
> 
> Note, Thomas already applied this version, so it should be just a follow up.

Indeed, I saw that.

Thanks,
Hervé

[PATCH v2 10/19] irqdomain: Introduce irq_domain_alloc() and irq_domain_publish()

[Herve Codina]

The irq_domain_add_*() family functions create an irq_domain and also
publish this newly created to domain. Once an irq_domain is published,
consumers can request IRQ in order to use them.

Some interrupt controller drivers have to perform some more operations
with the created irq_domain in order to have it ready to be used.
For instance:
  - Allocate generic irq chips with irq_alloc_domain_generic_chips()
  - Retrieve the generic irq chips with irq_get_domain_generic_chip()
  - Initialize retrieved chips: set register base address and offsets,
    set several hooks such as irq_mask, irq_unmask, ...

To avoid a window where the domain is published but not yet ready to be
used, introduce irq_domain_alloc_*() family functions to create the
irq_domain and irq_domain_publish() to publish the irq_domain.
With this new functions, any additional initialisation can then be done
between the call creating the irq_domain and the call publishing it.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 include/linux/irqdomain.h | 16 +++++++
 kernel/irq/irqdomain.c    | 91 ++++++++++++++++++++++++++++-----------
 2 files changed, 82 insertions(+), 25 deletions(-)

diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
index 21ecf582a0fe..86203e7e6659 100644
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -257,6 +257,22 @@ static inline struct fwnode_handle *irq_domain_alloc_fwnode(phys_addr_t *pa)
 }
 
 void irq_domain_free_fwnode(struct fwnode_handle *fwnode);
+struct irq_domain *irq_domain_alloc(struct fwnode_handle *fwnode, unsigned int size,
+				    irq_hw_number_t hwirq_max, int direct_max,
+				    const struct irq_domain_ops *ops,
+				    void *host_data);
+
+static inline struct irq_domain *irq_domain_alloc_linear(struct fwnode_handle *fwnode,
+							 unsigned int size,
+							 const struct irq_domain_ops *ops,
+							 void *host_data)
+{
+	return irq_domain_alloc(fwnode, size, size, 0, ops, host_data);
+}
+
+void irq_domain_free(struct irq_domain *domain);
+void irq_domain_publish(struct irq_domain *domain);
+void irq_domain_unpublish(struct irq_domain *domain);
 struct irq_domain *__irq_domain_add(struct fwnode_handle *fwnode, unsigned int size,
 				    irq_hw_number_t hwirq_max, int direct_max,
 				    const struct irq_domain_ops *ops,
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 86f8b91b0d3a..06c3e1b03a1d 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -231,7 +231,38 @@ static struct irq_domain *__irq_domain_create(struct fwnode_handle *fwnode,
 	return domain;
 }
 
-static void __irq_domain_publish(struct irq_domain *domain)
+struct irq_domain *irq_domain_alloc(struct fwnode_handle *fwnode, unsigned int size,
+				    irq_hw_number_t hwirq_max, int direct_max,
+				    const struct irq_domain_ops *ops,
+				    void *host_data)
+{
+	return __irq_domain_create(fwnode, size, hwirq_max, direct_max, ops,
+				   host_data);
+}
+EXPORT_SYMBOL_GPL(irq_domain_alloc);
+
+/**
+ * irq_domain_free() - Free an irq domain.
+ * @domain: domain to free
+ *
+ * This routine is used to free an irq domain. The caller must ensure
+ * that the domain is not published.
+ */
+void irq_domain_free(struct irq_domain *domain)
+{
+	fwnode_dev_initialized(domain->fwnode, false);
+	fwnode_handle_put(domain->fwnode);
+	if (domain->flags & IRQ_DOMAIN_NAME_ALLOCATED)
+		kfree(domain->name);
+	kfree(domain);
+}
+EXPORT_SYMBOL_GPL(irq_domain_free);
+
+/**
+ * irq_domain_publish() - Publish an irq domain.
+ * @domain: domain to publish
+ */
+void irq_domain_publish(struct irq_domain *domain)
 {
 	mutex_lock(&irq_domain_mutex);
 	debugfs_add_domain_dir(domain);
@@ -240,6 +271,36 @@ static void __irq_domain_publish(struct irq_domain *domain)
 
 	pr_debug("Added domain %s\n", domain->name);
 }
+EXPORT_SYMBOL_GPL(irq_domain_publish);
+
+/**
+ * irq_domain_unpublish() - Unpublish an irq domain.
+ * @domain: domain to unpublish
+ *
+ * This routine is used to unpublish an irq domain. The caller must ensure
+ * that all mappings within the domain have been disposed of prior to
+ * use, depending on the revmap type.
+ */
+void irq_domain_unpublish(struct irq_domain *domain)
+{
+	mutex_lock(&irq_domain_mutex);
+	debugfs_remove_domain_dir(domain);
+
+	WARN_ON(!radix_tree_empty(&domain->revmap_tree));
+
+	list_del(&domain->link);
+
+	/*
+	 * If the going away domain is the default one, reset it.
+	 */
+	if (unlikely(irq_default_domain == domain))
+		irq_set_default_host(NULL);
+
+	mutex_unlock(&irq_domain_mutex);
+
+	pr_debug("Removed domain %s\n", domain->name);
+}
+EXPORT_SYMBOL_GPL(irq_domain_unpublish);
 
 /**
  * __irq_domain_add() - Allocate a new irq_domain data structure
@@ -264,7 +325,7 @@ struct irq_domain *__irq_domain_add(struct fwnode_handle *fwnode, unsigned int s
 	domain = __irq_domain_create(fwnode, size, hwirq_max, direct_max,
 				     ops, host_data);
 	if (domain)
-		__irq_domain_publish(domain);
+		irq_domain_publish(domain);
 
 	return domain;
 }
@@ -280,28 +341,8 @@ EXPORT_SYMBOL_GPL(__irq_domain_add);
  */
 void irq_domain_remove(struct irq_domain *domain)
 {
-	mutex_lock(&irq_domain_mutex);
-	debugfs_remove_domain_dir(domain);
-
-	WARN_ON(!radix_tree_empty(&domain->revmap_tree));
-
-	list_del(&domain->link);
-
-	/*
-	 * If the going away domain is the default one, reset it.
-	 */
-	if (unlikely(irq_default_domain == domain))
-		irq_set_default_host(NULL);
-
-	mutex_unlock(&irq_domain_mutex);
-
-	pr_debug("Removed domain %s\n", domain->name);
-
-	fwnode_dev_initialized(domain->fwnode, false);
-	fwnode_handle_put(domain->fwnode);
-	if (domain->flags & IRQ_DOMAIN_NAME_ALLOCATED)
-		kfree(domain->name);
-	kfree(domain);
+	irq_domain_unpublish(domain);
+	irq_domain_free(domain);
 }
 EXPORT_SYMBOL_GPL(irq_domain_remove);
 
@@ -1184,7 +1225,7 @@ struct irq_domain *irq_domain_create_hierarchy(struct irq_domain *parent,
 		domain->parent = parent;
 		domain->flags |= flags;
 
-		__irq_domain_publish(domain);
+		irq_domain_publish(domain);
 	}
 
 	return domain;
-- 
2.45.0

Re: [PATCH v2 10/19] irqdomain: Introduce irq_domain_alloc() and irq_domain_publish()

[Thomas Gleixner]

Herve!

On Mon, May 27 2024 at 18:14, Herve Codina wrote:

Sorry I missed V1 somehow. I'll review this tomorrow.

Thanks,

        tglx

Re: [PATCH v2 10/19] irqdomain: Introduce irq_domain_alloc() and irq_domain_publish()

[Thomas Gleixner]

On Mon, May 27 2024 at 18:14, Herve Codina wrote:
> The irq_domain_add_*() family functions create an irq_domain and also
> publish this newly created to domain. Once an irq_domain is published,
> consumers can request IRQ in order to use them.
>
> Some interrupt controller drivers have to perform some more operations
> with the created irq_domain in order to have it ready to be used.
> For instance:
>   - Allocate generic irq chips with irq_alloc_domain_generic_chips()
>   - Retrieve the generic irq chips with irq_get_domain_generic_chip()
>   - Initialize retrieved chips: set register base address and offsets,
>     set several hooks such as irq_mask, irq_unmask, ...
>
> To avoid a window where the domain is published but not yet ready to be

I can see the point, but why is this suddenly a problem? There are tons
of interrupt chip drivers which have exactly that pattern.

Also why is all of this burried in a series which aims to add a network
driver and touches the world and some more. If you had sent the two irq
domain patches seperately w/o spamming 100 people on CC then this would
have been solved long ago. That's documented clearly, no?

>  void irq_domain_free_fwnode(struct fwnode_handle *fwnode);
> +struct irq_domain *irq_domain_alloc(struct fwnode_handle *fwnode, unsigned int size,
> +				    irq_hw_number_t hwirq_max, int direct_max,
> +				    const struct irq_domain_ops *ops,
> +				    void *host_data);
> +
> +static inline struct irq_domain *irq_domain_alloc_linear(struct fwnode_handle *fwnode,
> +							 unsigned int size,
> +							 const struct irq_domain_ops *ops,
> +							 void *host_data)
> +{
> +	return irq_domain_alloc(fwnode, size, size, 0, ops, host_data);
> +}

So this creates exactly one wrapper, which means we'll grow another ton
of wrappers if that becomes popular for whatever reason. We have already
too many of variants for creating domains.

But what's worse is that this does not work for hierarchical domains and
is just an ad hoc scratch my itch solution.

Also looking at the irq chip drivers which use generic interrupt
chips. There are 24 instances of irq_alloc_domain_generic_chips() and
most of this code is just boilerplate.

So what we really want is a proper solution to get rid of this mess
instead of creating interfaces which just proliferate and extend it.

Something like the uncompiled below allows to convert all the
boilerplate into a template based setup/remove.

I just converted a random driver over to it and the result is pretty
neutral in terms of lines, but the amount of code to get wrong is
significantly smaller. I'm sure that more complex drivers will benefit
even more and your problem should be completely solved by that.

The below is just an initial sketch which allows further consolidation
in the irqdomain space. You get the idea.

Thanks,

        tglx
---
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -1106,6 +1106,7 @@ enum irq_gc_flags {
  * @irq_flags_to_set:	IRQ* flags to set on irq setup
  * @irq_flags_to_clear:	IRQ* flags to clear on irq setup
  * @gc_flags:		Generic chip specific setup flags
+ * FIXME
  * @gc:			Array of pointers to generic interrupt chips
  */
 struct irq_domain_chip_generic {
@@ -1114,9 +1115,26 @@ struct irq_domain_chip_generic {
 	unsigned int		irq_flags_to_clear;
 	unsigned int		irq_flags_to_set;
 	enum irq_gc_flags	gc_flags;
+	void			(*destroy)(struct irq_chip_generic *c);
 	struct irq_chip_generic	*gc[];
 };
 
+/**
+ * irq_domain_chip_generic_info - Init structure
+ * FIXME
+ */
+struct irq_domain_chip_generic_info {
+	const char		*name;
+	irq_flow_handler_t	handler;
+	void			(*init)(struct irq_chip_generic *d);
+	void			(*destroy)(struct irq_chip_generic *c);
+	unsigned int		irqs_per_chip;
+	unsigned int		num_chips;
+	unsigned int		irq_flags_to_clear;
+	unsigned int		irq_flags_to_set;
+	enum irq_gc_flags	gc_flags;
+};
+
 /* Generic chip callback functions */
 void irq_gc_noop(struct irq_data *d);
 void irq_gc_mask_disable_reg(struct irq_data *d);
@@ -1153,6 +1171,9 @@ int devm_irq_setup_generic_chip(struct d
 
 struct irq_chip_generic *irq_get_domain_generic_chip(struct irq_domain *d, unsigned int hw_irq);
 
+int irq_domain_alloc_generic_chips(struct irq_domain *d, struct irq_domain_chip_generic_info *info);
+void irq_domain_remove_generic_chips(struct irq_domain *d);
+
 int __irq_alloc_domain_generic_chips(struct irq_domain *d, int irqs_per_chip,
 				     int num_ct, const char *name,
 				     irq_flow_handler_t handler,
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -41,13 +41,13 @@ struct device_node;
 struct fwnode_handle;
 struct irq_domain;
 struct irq_chip;
+struct irq_chip_generic;
 struct irq_data;
 struct irq_desc;
 struct cpumask;
 struct seq_file;
 struct irq_affinity_desc;
 struct msi_parent_ops;
-
 #define IRQ_DOMAIN_IRQ_SPEC_PARAMS 16
 
 /**
@@ -169,6 +169,7 @@ struct irq_domain {
 #ifdef CONFIG_GENERIC_MSI_IRQ
 	const struct msi_parent_ops	*msi_parent_ops;
 #endif
+	void				(*destroy)(struct irq_domain *d);
 
 	/* reverse map data. The linear map gets appended to the irq_domain */
 	irq_hw_number_t			hwirq_max;
@@ -208,6 +209,9 @@ enum {
 	/* Irq domain is a MSI device domain */
 	IRQ_DOMAIN_FLAG_MSI_DEVICE	= (1 << 9),
 
+	/* Irq domain must destroy generic chips when removed */
+	IRQ_DOMAIN_FLAG_DESTROY_GC	= (1 << 10),
+
 	/*
 	 * Flags starting from IRQ_DOMAIN_FLAG_NONCORE are reserved
 	 * for implementation specific purposes and ignored by the
@@ -257,6 +261,28 @@ static inline struct fwnode_handle *irq_
 }
 
 void irq_domain_free_fwnode(struct fwnode_handle *fwnode);
+
+struct irq_domain_chip_generic_info;
+
+/**
+ * irq_domain_info - Init structure
+ * FIXME
+ */
+struct irq_domain_info {
+	struct fwnode_handle			*fwnode;
+	unsigned int				domain_flags;
+	unsigned int				size;
+	irq_hw_number_t				hwirq_max;
+	int					direct_max;
+	enum irq_domain_bus_token		bus_token;
+	const struct irq_domain_ops		*ops;
+	void					*host_data;
+	struct irq_domain_chip_generic_info	*gc_info;
+	void					(*init)(struct irq_domain *d);
+};
+
+struct irq_domain *irq_domain_instantiate(struct irq_domain_info *info);
+
 struct irq_domain *__irq_domain_add(struct fwnode_handle *fwnode, unsigned int size,
 				    irq_hw_number_t hwirq_max, int direct_max,
 				    const struct irq_domain_ops *ops,
--- a/kernel/irq/generic-chip.c
+++ b/kernel/irq/generic-chip.c
@@ -274,23 +274,11 @@ irq_gc_init_mask_cache(struct irq_chip_g
 			*mskptr = irq_reg_readl(gc, mskreg);
 	}
 }
-
 /**
- * __irq_alloc_domain_generic_chips - Allocate generic chips for an irq domain
- * @d:			irq domain for which to allocate chips
- * @irqs_per_chip:	Number of interrupts each chip handles (max 32)
- * @num_ct:		Number of irq_chip_type instances associated with this
- * @name:		Name of the irq chip
- * @handler:		Default flow handler associated with these chips
- * @clr:		IRQ_* bits to clear in the mapping function
- * @set:		IRQ_* bits to set in the mapping function
- * @gcflags:		Generic chip specific setup flags
+ * irq_domain_alloc_generic_chips - Allocate generic chips for an irq domain
+ * FIXME
  */
-int __irq_alloc_domain_generic_chips(struct irq_domain *d, int irqs_per_chip,
-				     int num_ct, const char *name,
-				     irq_flow_handler_t handler,
-				     unsigned int clr, unsigned int set,
-				     enum irq_gc_flags gcflags)
+int irq_domain_alloc_generic_chips(struct irq_domain *d, struct irq_domain_chip_generic_info *info)
 {
 	struct irq_domain_chip_generic *dgc;
 	struct irq_chip_generic *gc;
@@ -304,23 +292,24 @@ int __irq_alloc_domain_generic_chips(str
 	if (d->gc)
 		return -EBUSY;
 
-	numchips = DIV_ROUND_UP(d->revmap_size, irqs_per_chip);
+	numchips = DIV_ROUND_UP(d->revmap_size, info->irqs_per_chip);
 	if (!numchips)
 		return -EINVAL;
 
 	/* Allocate a pointer, generic chip and chiptypes for each chip */
-	gc_sz = struct_size(gc, chip_types, num_ct);
+	gc_sz = struct_size(gc, chip_types, info->num_ct);
 	dgc_sz = struct_size(dgc, gc, numchips);
 	sz = dgc_sz + numchips * gc_sz;
 
 	tmp = dgc = kzalloc(sz, GFP_KERNEL);
 	if (!dgc)
 		return -ENOMEM;
-	dgc->irqs_per_chip = irqs_per_chip;
+	dgc->irqs_per_chip = info->irqs_per_chip;
 	dgc->num_chips = numchips;
-	dgc->irq_flags_to_set = set;
-	dgc->irq_flags_to_clear = clr;
-	dgc->gc_flags = gcflags;
+	dgc->irq_flags_to_set = info->irq_flags_to_set;
+	dgc->irq_flags_to_clear = info->irq_flags_to_clear;
+	dgc->gc_flags = info->gcflags;
+	dgc->destroy = info->destroy;
 	d->gc = dgc;
 
 	/* Calc pointer to the first generic chip */
@@ -337,6 +326,9 @@ int __irq_alloc_domain_generic_chips(str
 			gc->reg_writel = &irq_writel_be;
 		}
 
+		if (info->init)
+			info->init(gc);
+
 		raw_spin_lock_irqsave(&gc_lock, flags);
 		list_add_tail(&gc->list, &gc_list);
 		raw_spin_unlock_irqrestore(&gc_lock, flags);
@@ -345,6 +337,56 @@ int __irq_alloc_domain_generic_chips(str
 	}
 	return 0;
 }
+
+/**
+ * irq_domain_remove_generic_chips - Remove generic chips from an interrupt domain
+ * FIXME
+ */
+void irq_domain_remove_generic_chips(struct irq_domain *d)
+{
+	struct irq_domain_chip_generic *dgc = d->gc;
+	struct irq_domain_ops *ops = d->ops;
+
+	if (!dgc)
+		return;
+
+	for (unsigned int i = 0; i < dgc->num_chips, i++) {
+		if (dgc->destroy)
+			dgc->destroy_gc(dgc->gc + i);
+		irq_remove_generic_chip(dgc->gc + i, ~0U, 0, 0);
+	}
+	d->dgc = NULL;
+	kfree(dgc);
+}
+
+/**
+ * __irq_alloc_domain_generic_chips - Allocate generic chips for an irq domain
+ * @d:			irq domain for which to allocate chips
+ * @irqs_per_chip:	Number of interrupts each chip handles (max 32)
+ * @num_ct:		Number of irq_chip_type instances associated with this
+ * @name:		Name of the irq chip
+ * @handler:		Default flow handler associated with these chips
+ * @clr:		IRQ_* bits to clear in the mapping function
+ * @set:		IRQ_* bits to set in the mapping function
+ * @gcflags:		Generic chip specific setup flags
+ */
+int __irq_alloc_domain_generic_chips(struct irq_domain *d, int irqs_per_chip,
+				     int num_ct, const char *name,
+				     irq_flow_handler_t handler,
+				     unsigned int clr, unsigned int set,
+				     enum irq_gc_flags gcflags)
+{
+	struct irq_domain_chip_generic_info info = {
+		.name			= name,
+		.handler		= handler,
+		.irqs_per_chip		= irqs_per_chip,
+		.irq_flags_to_set	= set,
+		.irq_flags_to_clear	= clr,
+		.gc_flags		= gcflags,
+	};
+
+	return irq_domain_alloc_generic_chips(d, &info);
+}
 EXPORT_SYMBOL_GPL(__irq_alloc_domain_generic_chips);
 
 static struct irq_chip_generic *
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -240,6 +240,47 @@ static void __irq_domain_publish(struct
 	pr_debug("Added domain %s\n", domain->name);
 }
 
+static void irq_domain_free(struct irq_domain *domain)
+{
+	fwnode_dev_initialized(domain->fwnode, false);
+	fwnode_handle_put(domain->fwnode);
+	if (domain->flags & IRQ_DOMAIN_NAME_ALLOCATED)
+		kfree(domain->name);
+	kfree(domain);
+}
+
+/**
+ * irq_domain_instantiate() - Instantiate a new irq_domain data structure
+ * FIXME
+ */
+struct irq_domain *irq_domain_instantiate(struct irq_domain_info *info)
+{
+	struct irq_domain *domain;
+
+	// FIXME: Convert irq_domain_create() to use @info
+	domain = __irq_domain_create(info->fwnode, info->size, info->hwirq_max, info->direct_max,
+				     info->ops, info->host_data);
+	if (!domain)
+		return NULL;
+
+	domain->flags |= info->domain_flags;
+
+	if (info->gc_info) {
+		if (!irq_domain_alloc_generic_chips(domain, info->gc_info)) {
+			irq_domain_remove(domain);
+			return NULL;
+		}
+	}
+	if (info->init)
+		info->init(domain);
+	__irq_domain_publish(domain);
+
+	// FIXME: Make this part of irq_domain_create()
+	if (info->bus_token)
+		irq_domain_update_bus_token(domain, info->bus_token);
+	return domain;
+}
+
 /**
  * __irq_domain_add() - Allocate a new irq_domain data structure
  * @fwnode: firmware node for the interrupt controller
@@ -279,6 +320,9 @@ EXPORT_SYMBOL_GPL(__irq_domain_add);
  */
 void irq_domain_remove(struct irq_domain *domain)
 {
+	if (domain->destroy)
+		domain->destroy(domain);
+
 	mutex_lock(&irq_domain_mutex);
 	debugfs_remove_domain_dir(domain);
 
@@ -294,13 +338,11 @@ void irq_domain_remove(struct irq_domain
 
 	mutex_unlock(&irq_domain_mutex);
 
-	pr_debug("Removed domain %s\n", domain->name);
+	if (domain->flags & IRQ_DOMAIN_FLAG_DESTROY_GC)
+		irq_domain_remove_generic_chips(domain);
 
-	fwnode_dev_initialized(domain->fwnode, false);
-	fwnode_handle_put(domain->fwnode);
-	if (domain->flags & IRQ_DOMAIN_NAME_ALLOCATED)
-		kfree(domain->name);
-	kfree(domain);
+	pr_debug("Removed domain %s\n", domain->name);
+	irq_domain_free(domain);
 }
 EXPORT_SYMBOL_GPL(irq_domain_remove);
 
--- a/drivers/irqchip/irq-al-fic.c
+++ b/drivers/irqchip/irq-al-fic.c
@@ -133,32 +133,10 @@ static int al_fic_irq_retrigger(struct i
 	return 1;
 }
 
-static int al_fic_register(struct device_node *node,
-			   struct al_fic *fic)
+static void al_fic_gc_init(struct irq_chip_generic *gc)
 {
-	struct irq_chip_generic *gc;
-	int ret;
+	struct al_fic *fic = gc->domain->host_data;
 
-	fic->domain = irq_domain_add_linear(node,
-					    NR_FIC_IRQS,
-					    &irq_generic_chip_ops,
-					    fic);
-	if (!fic->domain) {
-		pr_err("fail to add irq domain\n");
-		return -ENOMEM;
-	}
-
-	ret = irq_alloc_domain_generic_chips(fic->domain,
-					     NR_FIC_IRQS,
-					     1, fic->name,
-					     handle_level_irq,
-					     0, 0, IRQ_GC_INIT_MASK_CACHE);
-	if (ret) {
-		pr_err("fail to allocate generic chip (%d)\n", ret);
-		goto err_domain_remove;
-	}
-
-	gc = irq_get_domain_generic_chip(fic->domain, 0);
 	gc->reg_base = fic->base;
 	gc->chip_types->regs.mask = AL_FIC_MASK;
 	gc->chip_types->regs.ack = AL_FIC_CAUSE;
@@ -169,16 +147,37 @@ static int al_fic_register(struct device
 	gc->chip_types->chip.irq_retrigger = al_fic_irq_retrigger;
 	gc->chip_types->chip.flags = IRQCHIP_SKIP_SET_WAKE;
 	gc->private = fic;
+}
+
+static void al_fic_domain_init(struct irq_domain *d)
+{
+	struct al_fic *fic = d->host_data;
 
-	irq_set_chained_handler_and_data(fic->parent_irq,
-					 al_fic_irq_handler,
-					 fic);
-	return 0;
+	irq_set_chained_handler_and_data(fic->parent_irq, al_fic_irq_handler, fic);
+}
 
-err_domain_remove:
-	irq_domain_remove(fic->domain);
+static int al_fic_register(struct device_node *node, struct al_fic *fic)
+{
+	struct irq_domain_chip_generic_info gc_info = {
+		.irqs_per_chip		= NR_FIC_IRQS,
+		.num_chips		= 1,
+		.name			= fic->name,
+		.handler		= handle_level_irq,
+		.gc_flags		= IRQ_GC_INIT_MASK_CACHE,
+		.init			= al_fic_gc_init,
+	};
+	struct irq_domain_info info = {
+		.fwnode			= of_node_to_fwnode(node),
+		.size			= NR_FIC_IRQS,
+		.hwirq_max		= NR_FIC_IRQS,
+		.ops			= &irq_generic_chip_ops,
+		.host_data		= fic,
+		.gc_info		= &gc_info,
+		.init			= al_fic_domain_init,
+	};
 
-	return ret;
+	fic->domain = irq_domain_instantiate(&info);
+	return fic->domain ? 0 : -ENOMEM;
 }
 
 /*

Re: [PATCH v2 10/19] irqdomain: Introduce irq_domain_alloc() and irq_domain_publish()

[Herve Codina]

Hi Thomas,

On Wed, 05 Jun 2024 15:02:46 +0200
Thomas Gleixner <tglx@linutronix.de> wrote:

> On Mon, May 27 2024 at 18:14, Herve Codina wrote:
> > The irq_domain_add_*() family functions create an irq_domain and also
> > publish this newly created to domain. Once an irq_domain is published,
> > consumers can request IRQ in order to use them.
> >
> > Some interrupt controller drivers have to perform some more operations
> > with the created irq_domain in order to have it ready to be used.
> > For instance:
> >   - Allocate generic irq chips with irq_alloc_domain_generic_chips()
> >   - Retrieve the generic irq chips with irq_get_domain_generic_chip()
> >   - Initialize retrieved chips: set register base address and offsets,
> >     set several hooks such as irq_mask, irq_unmask, ...
> >
> > To avoid a window where the domain is published but not yet ready to be  
> 
> I can see the point, but why is this suddenly a problem? There are tons
> of interrupt chip drivers which have exactly that pattern.
> 

I thing the issue was not triggered because these interrupt chip driver
are usually builtin compiled and the probe sequence is the linear one
done at boot time. Consumers/supplier are probe sequentially without any
parallel execution issues.

In the LAN966x PCI device driver use case, the drivers were built as
modules. Modules loading and drivers .probe() calls for the irqs supplier
and irqs consumers are done in parallel. This reveals the race condition.

> Also why is all of this burried in a series which aims to add a network
> driver and touches the world and some more. If you had sent the two irq
> domain patches seperately w/o spamming 100 people on CC then this would
> have been solved long ago. That's documented clearly, no?

Yes, the main idea of the series, as mentioned in the cover letter, is to
give the big picture of the LAN966x PCI device use case in order to have
all the impacted subsystems and drivers maintainers be aware of the global
use case: DT overlay on top of PCI device.
Of course, the plan is to split this series into smaller ones once parts
get discussed in the DT overlay on top of PCI use case and reach some kind
of maturity at least on the way to implement a solution.

Thomas, do you prefer to have all the IRQ related patches extracted right
now from this big picture series ?

> 
> >  void irq_domain_free_fwnode(struct fwnode_handle *fwnode);
> > +struct irq_domain *irq_domain_alloc(struct fwnode_handle *fwnode, unsigned int size,
> > +				    irq_hw_number_t hwirq_max, int direct_max,
> > +				    const struct irq_domain_ops *ops,
> > +				    void *host_data);
> > +
> > +static inline struct irq_domain *irq_domain_alloc_linear(struct fwnode_handle *fwnode,
> > +							 unsigned int size,
> > +							 const struct irq_domain_ops *ops,
> > +							 void *host_data)
> > +{
> > +	return irq_domain_alloc(fwnode, size, size, 0, ops, host_data);
> > +}  
> 
> So this creates exactly one wrapper, which means we'll grow another ton
> of wrappers if that becomes popular for whatever reason. We have already
> too many of variants for creating domains.
> 
> But what's worse is that this does not work for hierarchical domains and
> is just an ad hoc scratch my itch solution.
> 
> Also looking at the irq chip drivers which use generic interrupt
> chips. There are 24 instances of irq_alloc_domain_generic_chips() and
> most of this code is just boilerplate.
> 
> So what we really want is a proper solution to get rid of this mess
> instead of creating interfaces which just proliferate and extend it.
> 
> Something like the uncompiled below allows to convert all the
> boilerplate into a template based setup/remove.
> 
> I just converted a random driver over to it and the result is pretty
> neutral in terms of lines, but the amount of code to get wrong is
> significantly smaller. I'm sure that more complex drivers will benefit
> even more and your problem should be completely solved by that.
> 
> The below is just an initial sketch which allows further consolidation
> in the irqdomain space. You get the idea.

Got it, thanks a lot for the idea, the sketch and the way to use it in
drivers. I will rework my patches in that way.

Thanks,
Hervé

Re: [PATCH v2 10/19] irqdomain: Introduce irq_domain_alloc() and irq_domain_publish()

[Thomas Gleixner]

Herve!

On Thu, Jun 06 2024 at 17:52, Herve Codina wrote:
> On Wed, 05 Jun 2024 15:02:46 +0200
> Thomas Gleixner <tglx@linutronix.de> wrote:
>> On Mon, May 27 2024 at 18:14, Herve Codina wrote:
>> > To avoid a window where the domain is published but not yet ready to be  
>> 
>> I can see the point, but why is this suddenly a problem? There are tons
>> of interrupt chip drivers which have exactly that pattern.
>
> I thing the issue was not triggered because these interrupt chip driver
> are usually builtin compiled and the probe sequence is the linear one
> done at boot time. Consumers/supplier are probe sequentially without any
> parallel execution issues.
>
> In the LAN966x PCI device driver use case, the drivers were built as
> modules. Modules loading and drivers .probe() calls for the irqs supplier
> and irqs consumers are done in parallel. This reveals the race condition.

So how is that supposed to work? There is clearly a requirement that the
interrupt controller is ready to use when the network driver is probed, no?

>> Also why is all of this burried in a series which aims to add a network
>> driver and touches the world and some more. If you had sent the two irq
>> domain patches seperately w/o spamming 100 people on CC then this would
>> have been solved long ago. That's documented clearly, no?
>
> Yes, the main idea of the series, as mentioned in the cover letter, is to
> give the big picture of the LAN966x PCI device use case in order to have
> all the impacted subsystems and drivers maintainers be aware of the global
> use case: DT overlay on top of PCI device.
> Of course, the plan is to split this series into smaller ones once parts
> get discussed in the DT overlay on top of PCI use case and reach some kind
> of maturity at least on the way to implement a solution.

Fair enough.

> Thomas, do you prefer to have all the IRQ related patches extracted right
> now from this big picture series ?

I think the interrupt controller problem is completely orthogonal to the
PCI/DT issue.

So yes, please split them out as preparatory work which is probably also
not that interesting for the PCI/DT/net folks.

If the template approach holds, then the infrastructure change is
definitely worth it on its own and the actual driver just falls in place
and is off your backlog list.

Thanks,

        tglx

Re: [PATCH v2 10/19] irqdomain: Introduce irq_domain_alloc() and irq_domain_publish()

[Herve Codina]

On Thu, 06 Jun 2024 20:11:23 +0200
Thomas Gleixner <tglx@linutronix.de> wrote:

> Herve!
> 
> On Thu, Jun 06 2024 at 17:52, Herve Codina wrote:
> > On Wed, 05 Jun 2024 15:02:46 +0200
> > Thomas Gleixner <tglx@linutronix.de> wrote:  
> >> On Mon, May 27 2024 at 18:14, Herve Codina wrote:  
> >> > To avoid a window where the domain is published but not yet ready to be    
> >> 
> >> I can see the point, but why is this suddenly a problem? There are tons
> >> of interrupt chip drivers which have exactly that pattern.  
> >
> > I thing the issue was not triggered because these interrupt chip driver
> > are usually builtin compiled and the probe sequence is the linear one
> > done at boot time. Consumers/supplier are probe sequentially without any
> > parallel execution issues.
> >
> > In the LAN966x PCI device driver use case, the drivers were built as
> > modules. Modules loading and drivers .probe() calls for the irqs supplier
> > and irqs consumers are done in parallel. This reveals the race condition.  
> 
> So how is that supposed to work? There is clearly a requirement that the
> interrupt controller is ready to use when the network driver is probed, no?

Yes, EPROBE_DEFER mecanism.
The race condition window leads to an other error code instead of the
expected EPROBE_DEFER.

> 
> >> Also why is all of this burried in a series which aims to add a network
> >> driver and touches the world and some more. If you had sent the two irq
> >> domain patches seperately w/o spamming 100 people on CC then this would
> >> have been solved long ago. That's documented clearly, no?  
> >
> > Yes, the main idea of the series, as mentioned in the cover letter, is to
> > give the big picture of the LAN966x PCI device use case in order to have
> > all the impacted subsystems and drivers maintainers be aware of the global
> > use case: DT overlay on top of PCI device.
> > Of course, the plan is to split this series into smaller ones once parts
> > get discussed in the DT overlay on top of PCI use case and reach some kind
> > of maturity at least on the way to implement a solution.  
> 
> Fair enough.
> 
> > Thomas, do you prefer to have all the IRQ related patches extracted right
> > now from this big picture series ?  
> 
> I think the interrupt controller problem is completely orthogonal to the
> PCI/DT issue.
> 
> So yes, please split them out as preparatory work which is probably also
> not that interesting for the PCI/DT/net folks.
> 

Will do that.

Best regards,
Hervé

[PATCH v2 11/19] irqchip: Add support for LAN966x OIC

[Herve Codina]

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 | 308 ++++++++++++++++++++++++++++++
 3 files changed, 321 insertions(+)
 create mode 100644 drivers/irqchip/irq-lan966x-oic.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 14464716bacb..348f34525d23 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 d9dc3d99aaa8..9f6f88274bec 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -104,6 +104,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..a5f64610e62d
--- /dev/null
+++ b/drivers/irqchip/irq-lan966x-oic.c
@@ -0,0 +1,308 @@
+// 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) */
+#define LAN966X_OIC_DST_INTR_MAP(_n)	(0x78 + (_n) * 4)
+#define LAN966X_OIC_DST_INTR_MAP1(_n)	(0x9c + (_n) * 4)
+#define LAN966X_OIC_DST_INTR_MAP2(_n)	(0xc0 + (_n) * 4)
+
+/* Currently active interrupt sources per destination (_n = 0..8) */
+#define LAN966X_OIC_DST_INTR_IDENT(_n)	(0xe4 + (_n) * 4)
+#define LAN966X_OIC_DST_INTR_IDENT1(_n)	(0x108 + (_n) * 4)
+#define LAN966X_OIC_DST_INTR_IDENT2(_n)	(0x12c + (_n) * 4)
+
+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) {
+		ret = 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.45.0

Re: [PATCH v2 11/19] irqchip: Add support for LAN966x OIC

[Thomas Gleixner]

On Mon, May 27 2024 at 18:14, Herve Codina wrote:
> +struct lan966x_oic_data {
> +	struct irq_domain *domain;
> +	void __iomem *regs;
> +	int irq;
> +};

Please read Documentation/process/maintainers-tip.rst

> +static int lan966x_oic_irq_set_type(struct irq_data *data,
> +				    unsigned int flow_type)

Please use the 100 character limit

> +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),

Please make this tabular. See doc.

> +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);

~0U
  
> +	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;

int ret, 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) {
> +		ret = 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;

This is exactly what can be avoided.

> +
> +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));

This is just wrong. You cannot remove the chip when there are still interrupts
mapped.

I just did a quick conversion to the template approach. Unsurprisingly
it removes 30 lines of boiler plate code:

+static void lan966x_oic_chip_init(struct irq_chip_generic *gc)
+{
+	struct lan966x_oic_data *lan966x_oic = gc->domain->host_data;
+	struct lan966x_oic_chip_regs *chip_regs;
+
+	gc->reg_base = lan966x_oic->regs;
+
+	chip_regs = lan966x_oic_chip_regs + gc->irq_base / 32;
+	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 void lan966x_oic_domain_init(struct irq_domain *d)
+{
+	struct lan966x_oic_data *lan966x_oic = d->host_data;
+
+	irq_set_chained_handler_and_data(lan966x_oic->irq, lan966x_oic_irq_handler, d);
+}
+
+static int lan966x_oic_probe(struct platform_device *pdev)
+{
+	struct irq_domain_chip_generic_info gc_info = {
+		.irqs_per_chip		= 32,
+		.num_chips		= 1,
+		.name			= "lan966x-oic"
+		.handler		= handle_level_irq,
+		.init			= lan966x_oic_chip_init,
+		.destroy		= lan966x_oic_chip_exit,
+	};
+
+	struct irq_domain_info info = {
+		.fwnode			= of_node_to_fwnode(pdev->dev.of_node),
+		.size			= LAN966X_OIC_NR_IRQ,
+		.hwirq_max		= LAN966X_OIC_NR_IRQ,
+		.ops			= &irq_generic_chip_ops,
+		.gc_info		= &gc_info,
+		.init			= lan966x_oic_domain_init,
+	};
+	struct lan966x_oic_data *lan966x_oic;
+	struct device *dev = &pdev->dev;
+
+	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->irq = platform_get_irq(pdev, 0);
+	if (lan966x_oic->irq < 0)
+		return dev_err_probe(dev, lan966x_oic->irq, "failed to get the IRQ\n");
+
+	lan966x_oic->domain = irq_domain_instantiate(&info);
+	if (!lan966x_oic->domain)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, lan966x_oic);
+	return 0;
+}
+
+static void lan966x_oic_remove(struct platform_device *pdev)
+{
+	struct lan966x_oic_data *lan966x_oic = platform_get_drvdata(pdev);
+
+	irq_set_chained_handler_and_data(lan966x_oic->irq, NULL, NULL);
+	irq_domain_remove(lan966x_oic->domain);
+}

See?

Thanks,

        tglx

Re: [PATCH v2 11/19] irqchip: Add support for LAN966x OIC

[Andy Shevchenko]

Wed, Jun 05, 2024 at 04:17:53PM +0200, Thomas Gleixner kirjoitti:
> On Mon, May 27 2024 at 18:14, Herve Codina wrote:

...

> > +	irq_reg_writel(gc, ~0, gc->chip_types[0].regs.disable);
> 
> ~0U
>   
> > +	irq_reg_writel(gc, ~0, gc->chip_types[0].regs.ack);

...

Below just to annoy people a bit :)

(Yes, I understand that this is a prototype, it's just a pre-review in case one
want to blindly copy'n'paste it).

Other than that, I like the result!

> I just did a quick conversion to the template approach. Unsurprisingly
> it removes 30 lines of boiler plate code:
> 
> +static void lan966x_oic_chip_init(struct irq_chip_generic *gc)
> +{
> +	struct lan966x_oic_data *lan966x_oic = gc->domain->host_data;
> +	struct lan966x_oic_chip_regs *chip_regs;
> +
> +	gc->reg_base = lan966x_oic->regs;
> +
> +	chip_regs = lan966x_oic_chip_regs + gc->irq_base / 32;
> +	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);

~0U :-)

But I, for example, think that GENMASK() even better as it shows exactly what
bits we set for the HW writes.

> +}
> +
> +static void lan966x_oic_domain_init(struct irq_domain *d)
> +{
> +	struct lan966x_oic_data *lan966x_oic = d->host_data;
> +
> +	irq_set_chained_handler_and_data(lan966x_oic->irq, lan966x_oic_irq_handler, d);
> +}
> +
> +static int lan966x_oic_probe(struct platform_device *pdev)
> +{
> +	struct irq_domain_chip_generic_info gc_info = {
> +		.irqs_per_chip		= 32,
> +		.num_chips		= 1,
> +		.name			= "lan966x-oic"
> +		.handler		= handle_level_irq,
> +		.init			= lan966x_oic_chip_init,
> +		.destroy		= lan966x_oic_chip_exit,
> +	};
> +
> +	struct irq_domain_info info = {

> +		.fwnode			= of_node_to_fwnode(pdev->dev.of_node),

It's as simple as dev_fwnode()

> +		.size			= LAN966X_OIC_NR_IRQ,
> +		.hwirq_max		= LAN966X_OIC_NR_IRQ,
> +		.ops			= &irq_generic_chip_ops,
> +		.gc_info		= &gc_info,
> +		.init			= lan966x_oic_domain_init,
> +	};
> +	struct lan966x_oic_data *lan966x_oic;
> +	struct device *dev = &pdev->dev;
> +
> +	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->irq = platform_get_irq(pdev, 0);
> +	if (lan966x_oic->irq < 0)
> +		return dev_err_probe(dev, lan966x_oic->irq, "failed to get the IRQ\n");
> +
> +	lan966x_oic->domain = irq_domain_instantiate(&info);
> +	if (!lan966x_oic->domain)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, lan966x_oic);
> +	return 0;
> +}
> +
> +static void lan966x_oic_remove(struct platform_device *pdev)
> +{
> +	struct lan966x_oic_data *lan966x_oic = platform_get_drvdata(pdev);
> +
> +	irq_set_chained_handler_and_data(lan966x_oic->irq, NULL, NULL);
> +	irq_domain_remove(lan966x_oic->domain);
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 11/19] irqchip: Add support for LAN966x OIC

[Herve Codina]

Hi Thomas,

On Wed, 05 Jun 2024 16:17:53 +0200
Thomas Gleixner <tglx@linutronix.de> wrote:

> On Mon, May 27 2024 at 18:14, Herve Codina wrote:
> > +struct lan966x_oic_data {
> > +	struct irq_domain *domain;
> > +	void __iomem *regs;
> > +	int irq;
> > +};  
> 
> Please read Documentation/process/maintainers-tip.rst

I suppose you pointed out the un-tabular struct member names here.
I will fix that in the next iteration.

> 
> > +static int lan966x_oic_irq_set_type(struct irq_data *data,
> > +				    unsigned int flow_type)  
> 
> Please use the 100 character limit

Sure, will be fixed.

> 
> > +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),  
> 
> Please make this tabular. See doc.

Will be fixed.

> 
> > +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);  
> 
> ~0U

Will be changed.

>   
> > +	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;  
> 
> int ret, i;

Will be changed.

> 
> > +
> > +	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) {
> > +		ret = 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;  
> 
> This is exactly what can be avoided.
> 
> > +
> > +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));  
> 
> This is just wrong. You cannot remove the chip when there are still interrupts
> mapped.
> 
> I just did a quick conversion to the template approach. Unsurprisingly
> it removes 30 lines of boiler plate code:
> 
> +static void lan966x_oic_chip_init(struct irq_chip_generic *gc)
> +{
> +	struct lan966x_oic_data *lan966x_oic = gc->domain->host_data;
> +	struct lan966x_oic_chip_regs *chip_regs;
> +
> +	gc->reg_base = lan966x_oic->regs;
> +
> +	chip_regs = lan966x_oic_chip_regs + gc->irq_base / 32;
> +	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 void lan966x_oic_domain_init(struct irq_domain *d)
> +{
> +	struct lan966x_oic_data *lan966x_oic = d->host_data;
> +
> +	irq_set_chained_handler_and_data(lan966x_oic->irq, lan966x_oic_irq_handler, d);
> +}
> +
> +static int lan966x_oic_probe(struct platform_device *pdev)
> +{
> +	struct irq_domain_chip_generic_info gc_info = {
> +		.irqs_per_chip		= 32,
> +		.num_chips		= 1,
> +		.name			= "lan966x-oic"
> +		.handler		= handle_level_irq,
> +		.init			= lan966x_oic_chip_init,
> +		.destroy		= lan966x_oic_chip_exit,
> +	};
> +
> +	struct irq_domain_info info = {
> +		.fwnode			= of_node_to_fwnode(pdev->dev.of_node),
> +		.size			= LAN966X_OIC_NR_IRQ,
> +		.hwirq_max		= LAN966X_OIC_NR_IRQ,
> +		.ops			= &irq_generic_chip_ops,
> +		.gc_info		= &gc_info,
> +		.init			= lan966x_oic_domain_init,
> +	};
> +	struct lan966x_oic_data *lan966x_oic;
> +	struct device *dev = &pdev->dev;
> +
> +	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->irq = platform_get_irq(pdev, 0);
> +	if (lan966x_oic->irq < 0)
> +		return dev_err_probe(dev, lan966x_oic->irq, "failed to get the IRQ\n");
> +
> +	lan966x_oic->domain = irq_domain_instantiate(&info);
> +	if (!lan966x_oic->domain)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, lan966x_oic);
> +	return 0;
> +}
> +
> +static void lan966x_oic_remove(struct platform_device *pdev)
> +{
> +	struct lan966x_oic_data *lan966x_oic = platform_get_drvdata(pdev);
> +
> +	irq_set_chained_handler_and_data(lan966x_oic->irq, NULL, NULL);
> +	irq_domain_remove(lan966x_oic->domain);
> +}
> 
> See?

Perfectly.
I will rework patches in this way.
Again, thanks for pointing out this solution.

Best regards,
Hervé

> 
> Thanks,
> 
>         tglx



-- 
Hervé Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH v2 12/19] MAINTAINERS: Add the Microchip LAN966x OIC driver entry

[Herve Codina]

After contributing the driver, add myself as the maintainer for the
Microchip LAN966x OIC driver.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 MAINTAINERS | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index d6c90161c7bf..baeb307344cd 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14727,6 +14727,12 @@ L:	netdev@vger.kernel.org
 S:	Maintained
 F:	drivers/net/ethernet/microchip/lan966x/*
 
+MICROCHIP LAN966X OIC DRIVER
+M:	Herve Codina <herve.codina@bootlin.com>
+S:	Maintained
+F:	Documentation/devicetree/bindings/interrupt-controller/microchip,lan966x-oic.yaml
+F:	drivers/irqchip/irq-lan966x-oic.c
+
 MICROCHIP LCDFB DRIVER
 M:	Nicolas Ferre <nicolas.ferre@microchip.com>
 L:	linux-fbdev@vger.kernel.org
-- 
2.45.0

[PATCH v2 13/19] of: dynamic: Constify parameter in of_changeset_add_prop_string_array()

[Herve Codina]

The str_array parameter has no reason to be an un-const array.
Indeed, elements of the 'str_array' array are not changed by the code.

Constify the 'str_array' array parameter.
With this const qualifier added, the following construction is allowed:
  static const char * const tab_str[] = { "string1", "string2" };
  of_changeset_add_prop_string_array(..., tab_str, ARRAY_SIZE(tab_str));

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/of/dynamic.c | 2 +-
 include/linux/of.h   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index dda6092e6d3a..70011a98c500 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -984,7 +984,7 @@ EXPORT_SYMBOL_GPL(of_changeset_add_prop_string);
 int of_changeset_add_prop_string_array(struct of_changeset *ocs,
 				       struct device_node *np,
 				       const char *prop_name,
-				       const char **str_array, size_t sz)
+				       const char * const *str_array, size_t sz)
 {
 	struct property prop;
 	int i, ret;
diff --git a/include/linux/of.h b/include/linux/of.h
index a0bedd038a05..ee9a385a13db 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -1639,7 +1639,7 @@ int of_changeset_add_prop_string(struct of_changeset *ocs,
 int of_changeset_add_prop_string_array(struct of_changeset *ocs,
 				       struct device_node *np,
 				       const char *prop_name,
-				       const char **str_array, size_t sz);
+				       const char * const *str_array, size_t sz);
 int of_changeset_add_prop_u32_array(struct of_changeset *ocs,
 				    struct device_node *np,
 				    const char *prop_name,
-- 
2.45.0

[PATCH v2 14/19] of: unittest: Add tests for changeset properties adding

[Herve Codina]

No test cases are present to test the of_changes_add_prop_*() function
family.

Add a new test to fill this lack.
Functions tested are:
  - of_changes_add_prop_string()
  - of_changes_add_prop_string_array()
  - of_changeset_add_prop_u32()
  - of_changeset_add_prop_u32_array()

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/of/unittest.c | 155 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 155 insertions(+)

diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index 445ad13dab98..f8edc96db680 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -917,6 +917,160 @@ static void __init of_unittest_changeset(void)
 #endif
 }
 
+static void __init __maybe_unused changeset_check_string(struct device_node *np,
+							 const char *prop_name,
+							 const char *expected_str)
+{
+	const char *str;
+	int ret;
+
+	ret = of_property_read_string(np, prop_name, &str);
+	if (unittest(ret == 0, "failed to read %s\n", prop_name))
+		return;
+
+	unittest(strcmp(str, expected_str) == 0,
+		 "%s value mismatch (read '%s', exp '%s')\n",
+		 prop_name, str, expected_str);
+}
+
+static void __init __maybe_unused changeset_check_string_array(struct device_node *np,
+							       const char *prop_name,
+							       const char * const *expected_array,
+							       unsigned int count)
+{
+	const char *str;
+	unsigned int i;
+	int ret;
+	int cnt;
+
+	cnt = of_property_count_strings(np, prop_name);
+	if (unittest(cnt >= 0, "failed to get %s count\n", prop_name))
+		return;
+
+	if (unittest(cnt == count,
+		     "%s count mismatch (read %d, exp %u)\n",
+		     prop_name, cnt, count))
+		return;
+
+	for (i = 0; i < count; i++) {
+		ret = of_property_read_string_index(np, prop_name, i, &str);
+		if (unittest(ret == 0, "failed to read %s[%d]\n", prop_name, i))
+			continue;
+
+		unittest(strcmp(str, expected_array[i]) == 0,
+			 "%s[%d] value mismatch (read '%s', exp '%s')\n",
+			 prop_name, i, str, expected_array[i]);
+	}
+}
+
+static void __init __maybe_unused changeset_check_u32(struct device_node *np,
+						      const char *prop_name,
+						      u32 expected_u32)
+{
+	u32 val32;
+	int ret;
+
+	ret = of_property_read_u32(np, prop_name, &val32);
+	if (unittest(ret == 0, "failed to read %s\n", prop_name))
+		return;
+
+	unittest(val32 == expected_u32,
+		 "%s value mismatch (read '%u', exp '%u')\n",
+		 prop_name, val32, expected_u32);
+}
+
+static void __init __maybe_unused changeset_check_u32_array(struct device_node *np,
+							    const char *prop_name,
+							    const u32 *expected_array,
+							    unsigned int count)
+{
+	unsigned int i;
+	u32 val32;
+	int ret;
+	int cnt;
+
+	cnt = of_property_count_u32_elems(np, prop_name);
+	if (unittest(cnt >= 0, "failed to get %s count\n", prop_name))
+		return;
+
+	if (unittest(cnt == count,
+		     "%s count mismatch (read %d, exp %u)\n",
+		     prop_name, cnt, count))
+		return;
+
+	for (i = 0; i < count; i++) {
+		ret = of_property_read_u32_index(np, prop_name, i, &val32);
+		if (unittest(ret == 0, "failed to read %s[%d]\n", prop_name, i))
+			continue;
+
+		unittest(val32 == expected_array[i],
+			 "%s[%d] value mismatch (read '%u', exp '%u')\n",
+			 prop_name, i, val32, expected_array[i]);
+	}
+}
+
+static void __init of_unittest_changeset_prop(void)
+{
+#ifdef CONFIG_OF_DYNAMIC
+	static const char * const str_array[] = { "abc", "defg", "hij" };
+	static const u32 u32_array[] = { 123, 4567, 89, 10, 11 };
+	struct device_node *nchangeset, *np;
+	struct of_changeset chgset;
+	int ret;
+
+	nchangeset = of_find_node_by_path("/testcase-data/changeset");
+	if (!nchangeset) {
+		pr_err("missing testcase data\n");
+		return;
+	}
+
+	of_changeset_init(&chgset);
+
+	np = of_changeset_create_node(&chgset, nchangeset, "test-prop");
+	if (unittest(np, "failed to create test-prop node\n"))
+		goto end_changeset_destroy;
+
+	ret = of_changeset_add_prop_string(&chgset, np, "prop-string", "abcde");
+	unittest(ret == 0, "failed to add prop-string\n");
+
+	ret = of_changeset_add_prop_string_array(&chgset, np, "prop-string-array",
+						 str_array, ARRAY_SIZE(str_array));
+	unittest(ret == 0, "failed to add prop-string-array\n");
+
+	ret = of_changeset_add_prop_u32(&chgset, np, "prop-u32", 1234);
+	unittest(ret == 0, "failed to add prop-u32\n");
+
+	ret = of_changeset_add_prop_u32_array(&chgset, np, "prop-u32-array",
+					      u32_array, ARRAY_SIZE(u32_array));
+	unittest(ret == 0, "failed to add prop-u32-array\n");
+
+	of_node_put(np);
+
+	ret = of_changeset_apply(&chgset);
+	if (unittest(ret == 0, "failed to apply changeset\n"))
+		goto end_changeset_destroy;
+
+	np = of_find_node_by_path("/testcase-data/changeset/test-prop");
+	if (unittest(np, "failed to find test-prop node\n"))
+		goto end_revert_changeset;
+
+	changeset_check_string(np, "prop-string", "abcde");
+	changeset_check_string_array(np, "prop-string-array", str_array, ARRAY_SIZE(str_array));
+	changeset_check_u32(np, "prop-u32", 1234);
+	changeset_check_u32_array(np, "prop-u32-array", u32_array, ARRAY_SIZE(u32_array));
+
+	of_node_put(np);
+
+end_revert_changeset:
+	ret = of_changeset_revert(&chgset);
+	unittest(ret == 0, "failed to revert changeset\n");
+
+end_changeset_destroy:
+	of_changeset_destroy(&chgset);
+	of_node_put(nchangeset);
+#endif
+}
+
 static void __init of_unittest_dma_get_max_cpu_address(void)
 {
 	struct device_node *np;
@@ -4101,6 +4255,7 @@ static int __init of_unittest(void)
 	of_unittest_property_string();
 	of_unittest_property_copy();
 	of_unittest_changeset();
+	of_unittest_changeset_prop();
 	of_unittest_parse_interrupts();
 	of_unittest_parse_interrupts_extended();
 	of_unittest_dma_get_max_cpu_address();
-- 
2.45.0

[PATCH v2 15/19] of: dynamic: Introduce of_changeset_add_prop_bool()

[Herve Codina]

APIs to add some properties in a changeset exist but nothing to add a DT
boolean property (i.e. a property without any values).

Fill this lack with of_changeset_add_prop_bool().

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/of/dynamic.c | 25 +++++++++++++++++++++++++
 include/linux/of.h   |  3 +++
 2 files changed, 28 insertions(+)

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 70011a98c500..110104a936d9 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -1047,3 +1047,28 @@ int of_changeset_add_prop_u32_array(struct of_changeset *ocs,
 	return of_changeset_add_prop_helper(ocs, np, &prop);
 }
 EXPORT_SYMBOL_GPL(of_changeset_add_prop_u32_array);
+
+/**
+ * of_changeset_add_prop_bool - Add a boolean property (i.e. a property without
+ * any values) to a changeset.
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @prop_name:	name of the property to be added
+ *
+ * Create a boolean property and add it to a changeset.
+ *
+ * Return: 0 on success, a negative error value in case of an error.
+ */
+int of_changeset_add_prop_bool(struct of_changeset *ocs, struct device_node *np,
+			       const char *prop_name)
+{
+	struct property prop;
+
+	prop.name = (char *)prop_name;
+	prop.length = 0;
+	prop.value = NULL;
+
+	return of_changeset_add_prop_helper(ocs, np, &prop);
+}
+EXPORT_SYMBOL_GPL(of_changeset_add_prop_bool);
diff --git a/include/linux/of.h b/include/linux/of.h
index ee9a385a13db..13cf7a43b473 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -1652,6 +1652,9 @@ static inline int of_changeset_add_prop_u32(struct of_changeset *ocs,
 	return of_changeset_add_prop_u32_array(ocs, np, prop_name, &val, 1);
 }
 
+int of_changeset_add_prop_bool(struct of_changeset *ocs, struct device_node *np,
+			       const char *prop_name);
+
 #else /* CONFIG_OF_DYNAMIC */
 static inline int of_reconfig_notifier_register(struct notifier_block *nb)
 {
-- 
2.45.0

[PATCH v2 16/19] of: unittest: Add a test case for of_changeset_add_prop_bool()

[Herve Codina]

Improve of_unittest_changeset_prop() to have a test case for the
newly introduced of_changeset_add_prop_bool().

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/of/unittest.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index f8edc96db680..c830f346df45 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -1009,6 +1009,13 @@ static void __init __maybe_unused changeset_check_u32_array(struct device_node *
 	}
 }
 
+static void __init __maybe_unused changeset_check_bool(struct device_node *np,
+						       const char *prop_name)
+{
+	unittest(of_property_read_bool(np, prop_name),
+		 "%s value mismatch (read 'false', exp 'true')\n", prop_name);
+}
+
 static void __init of_unittest_changeset_prop(void)
 {
 #ifdef CONFIG_OF_DYNAMIC
@@ -1044,6 +1051,9 @@ static void __init of_unittest_changeset_prop(void)
 					      u32_array, ARRAY_SIZE(u32_array));
 	unittest(ret == 0, "failed to add prop-u32-array\n");
 
+	ret = of_changeset_add_prop_bool(&chgset, np, "prop-bool");
+	unittest(ret == 0, "failed to add prop-bool\n");
+
 	of_node_put(np);
 
 	ret = of_changeset_apply(&chgset);
@@ -1058,6 +1068,7 @@ static void __init of_unittest_changeset_prop(void)
 	changeset_check_string_array(np, "prop-string-array", str_array, ARRAY_SIZE(str_array));
 	changeset_check_u32(np, "prop-u32", 1234);
 	changeset_check_u32_array(np, "prop-u32-array", u32_array, ARRAY_SIZE(u32_array));
+	changeset_check_bool(np, "prop-bool");
 
 	of_node_put(np);
 
-- 
2.45.0

[PATCH v2 17/19] PCI: of_property: Add interrupt-controller property in PCI device nodes

[Herve Codina]

PCI devices and bridges DT nodes created during the PCI scan are created
with the interrupt-map property set to handle interrupts.

In order to set this interrupt-map property at a specific level, a
phandle to the parent interrupt controller is needed. On systems that
are not fully described by a device-tree, the parent interrupt
controller may be unavailable (i.e. not described by the device-tree).

As mentioned in the [1], avoiding the use of the interrupt-map property
and considering a PCI device as an interrupt controller itself avoid the
use of a parent interrupt phandle.

In that case, the PCI device itself as an interrupt controller is
responsible for routing the interrupts described in the device-tree
world (DT overlay) to the PCI interrupts.

Add the 'interrupt-controller' property in the PCI device DT node.

[1]: https://lore.kernel.org/lkml/CAL_Jsq+je7+9ATR=B6jXHjEJHjn24vQFs4Tvi9=vhDeK9n42Aw@mail.gmail.com/

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/pci/of_property.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/pci/of_property.c b/drivers/pci/of_property.c
index 03539e505372..5a0b98e69795 100644
--- a/drivers/pci/of_property.c
+++ b/drivers/pci/of_property.c
@@ -183,6 +183,26 @@ static int of_pci_prop_interrupts(struct pci_dev *pdev,
 	return of_changeset_add_prop_u32(ocs, np, "interrupts", (u32)pin);
 }
 
+static int of_pci_prop_intr_ctrl(struct pci_dev *pdev, struct of_changeset *ocs,
+				 struct device_node *np)
+{
+	int ret;
+	u8 pin;
+
+	ret = pci_read_config_byte(pdev, PCI_INTERRUPT_PIN, &pin);
+	if (ret != 0)
+		return ret;
+
+	if (!pin)
+		return 0;
+
+	ret = of_changeset_add_prop_u32(ocs, np, "#interrupt-cells", 1);
+	if (ret)
+		return ret;
+
+	return of_changeset_add_prop_bool(ocs, np, "interrupt-controller");
+}
+
 static int of_pci_prop_intr_map(struct pci_dev *pdev, struct of_changeset *ocs,
 				struct device_node *np)
 {
@@ -336,6 +356,10 @@ int of_pci_add_properties(struct pci_dev *pdev, struct of_changeset *ocs,
 		ret = of_pci_prop_intr_map(pdev, ocs, np);
 		if (ret)
 			return ret;
+	} else {
+		ret = of_pci_prop_intr_ctrl(pdev, ocs, np);
+		if (ret)
+			return ret;
 	}
 
 	ret = of_pci_prop_ranges(pdev, ocs, np);
-- 
2.45.0

Re: [PATCH v2 17/19] PCI: of_property: Add interrupt-controller property in PCI device nodes

[Bjorn Helgaas]

On Mon, May 27, 2024 at 06:14:44PM +0200, Herve Codina wrote:
> PCI devices and bridges DT nodes created during the PCI scan are created
> with the interrupt-map property set to handle interrupts.
> 
> In order to set this interrupt-map property at a specific level, a
> phandle to the parent interrupt controller is needed. On systems that
> are not fully described by a device-tree, the parent interrupt
> controller may be unavailable (i.e. not described by the device-tree).
> 
> As mentioned in the [1], avoiding the use of the interrupt-map property
> and considering a PCI device as an interrupt controller itself avoid the
> use of a parent interrupt phandle.
> 
> In that case, the PCI device itself as an interrupt controller is
> responsible for routing the interrupts described in the device-tree
> world (DT overlay) to the PCI interrupts.
> 
> Add the 'interrupt-controller' property in the PCI device DT node.
> 
> [1]: https://lore.kernel.org/lkml/CAL_Jsq+je7+9ATR=B6jXHjEJHjn24vQFs4Tvi9=vhDeK9n42Aw@mail.gmail.com/
> 
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>

No objection from me, but I'd like an ack/review from Rob.

> ---
>  drivers/pci/of_property.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/pci/of_property.c b/drivers/pci/of_property.c
> index 03539e505372..5a0b98e69795 100644
> --- a/drivers/pci/of_property.c
> +++ b/drivers/pci/of_property.c
> @@ -183,6 +183,26 @@ static int of_pci_prop_interrupts(struct pci_dev *pdev,
>  	return of_changeset_add_prop_u32(ocs, np, "interrupts", (u32)pin);
>  }
>  
> +static int of_pci_prop_intr_ctrl(struct pci_dev *pdev, struct of_changeset *ocs,
> +				 struct device_node *np)
> +{
> +	int ret;
> +	u8 pin;
> +
> +	ret = pci_read_config_byte(pdev, PCI_INTERRUPT_PIN, &pin);
> +	if (ret != 0)
> +		return ret;
> +
> +	if (!pin)
> +		return 0;
> +
> +	ret = of_changeset_add_prop_u32(ocs, np, "#interrupt-cells", 1);
> +	if (ret)
> +		return ret;
> +
> +	return of_changeset_add_prop_bool(ocs, np, "interrupt-controller");
> +}
> +
>  static int of_pci_prop_intr_map(struct pci_dev *pdev, struct of_changeset *ocs,
>  				struct device_node *np)
>  {
> @@ -336,6 +356,10 @@ int of_pci_add_properties(struct pci_dev *pdev, struct of_changeset *ocs,
>  		ret = of_pci_prop_intr_map(pdev, ocs, np);
>  		if (ret)
>  			return ret;
> +	} else {
> +		ret = of_pci_prop_intr_ctrl(pdev, ocs, np);
> +		if (ret)
> +			return ret;
>  	}
>  
>  	ret = of_pci_prop_ranges(pdev, ocs, np);
> -- 
> 2.45.0
> 

Re: [PATCH v2 17/19] PCI: of_property: Add interrupt-controller property in PCI device nodes

[Rob Herring]

On Thu, Jun 06, 2024 at 02:26:12PM -0500, Bjorn Helgaas wrote:
> On Mon, May 27, 2024 at 06:14:44PM +0200, Herve Codina wrote:
> > PCI devices and bridges DT nodes created during the PCI scan are created
> > with the interrupt-map property set to handle interrupts.
> > 
> > In order to set this interrupt-map property at a specific level, a
> > phandle to the parent interrupt controller is needed. On systems that
> > are not fully described by a device-tree, the parent interrupt
> > controller may be unavailable (i.e. not described by the device-tree).
> > 
> > As mentioned in the [1], avoiding the use of the interrupt-map property
> > and considering a PCI device as an interrupt controller itself avoid the
> > use of a parent interrupt phandle.
> > 
> > In that case, the PCI device itself as an interrupt controller is
> > responsible for routing the interrupts described in the device-tree
> > world (DT overlay) to the PCI interrupts.
> > 
> > Add the 'interrupt-controller' property in the PCI device DT node.
> > 
> > [1]: https://lore.kernel.org/lkml/CAL_Jsq+je7+9ATR=B6jXHjEJHjn24vQFs4Tvi9=vhDeK9n42Aw@mail.gmail.com/
> > 
> > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> 
> No objection from me, but I'd like an ack/review from Rob.

Given it is more DT patches in the series, how about I take them and 
this one with your ack instead?

Rob

Re: [PATCH v2 17/19] PCI: of_property: Add interrupt-controller property in PCI device nodes

[Bjorn Helgaas]

On Mon, Jun 10, 2024 at 03:37:35PM -0600, Rob Herring wrote:
> On Thu, Jun 06, 2024 at 02:26:12PM -0500, Bjorn Helgaas wrote:
> > On Mon, May 27, 2024 at 06:14:44PM +0200, Herve Codina wrote:
> > > PCI devices and bridges DT nodes created during the PCI scan are created
> > > with the interrupt-map property set to handle interrupts.
> > > 
> > > In order to set this interrupt-map property at a specific level, a
> > > phandle to the parent interrupt controller is needed. On systems that
> > > are not fully described by a device-tree, the parent interrupt
> > > controller may be unavailable (i.e. not described by the device-tree).
> > > 
> > > As mentioned in the [1], avoiding the use of the interrupt-map property
> > > and considering a PCI device as an interrupt controller itself avoid the
> > > use of a parent interrupt phandle.
> > > 
> > > In that case, the PCI device itself as an interrupt controller is
> > > responsible for routing the interrupts described in the device-tree
> > > world (DT overlay) to the PCI interrupts.
> > > 
> > > Add the 'interrupt-controller' property in the PCI device DT node.
> > > 
> > > [1]: https://lore.kernel.org/lkml/CAL_Jsq+je7+9ATR=B6jXHjEJHjn24vQFs4Tvi9=vhDeK9n42Aw@mail.gmail.com/
> > > 
> > > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> > 
> > No objection from me, but I'd like an ack/review from Rob.
> 
> Given it is more DT patches in the series, how about I take them and 
> this one with your ack instead?

Sure.  There's very little PCI content here, so I didn't plan to take
them; I just thought this needed at least your ack :)

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

[PATCH v2 18/19] mfd: Add support for LAN966x PCI device

[Herve Codina]

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 266b4f54af60..15db144bc09b 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..a0a59860928f
--- /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_INTX);
+	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);
+}
+
+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);
+}
+
+struct lan966x_pci {
+	struct device *dev;
+	struct pci_dev *pci_dev;
+	int ovcs_id;
+};
+
+static int lan966x_pci_load_overlay(struct lan966x_pci *data)
+{
+	u32 dtbo_size = __dtbo_lan966x_pci_end - __dtbo_lan966x_pci_begin;
+	void *dtbo_start = __dtbo_lan966x_pci_begin;
+	int ret;
+
+	ret = of_overlay_fdt_apply(dtbo_start, dtbo_size, &data->ovcs_id, data->dev->of_node);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static void lan966x_pci_unload_overlay(struct lan966x_pci *data)
+{
+	of_overlay_remove(&data->ovcs_id);
+}
+
+static int lan966x_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
+{
+	struct device *dev = &pdev->dev;
+	struct lan966x_pci *data;
+	int ret;
+
+	if (!dev->of_node) {
+		dev_err(dev, "Missing of_node for device\n");
+		return -EINVAL;
+	}
+
+	/* Need to be done before devm_pci_dev_create_intr_ctrl.
+	 * It allocates an IRQ and so pdev->irq is updated
+	 */
+	ret = pcim_enable_device(pdev);
+	if (ret)
+		return ret;
+
+	ret = devm_pci_dev_create_intr_ctrl(pdev);
+	if (ret)
+		return ret;
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	dev_set_drvdata(dev, data);
+	data->dev = dev;
+	data->pci_dev = pdev;
+
+	ret = lan966x_pci_load_overlay(data);
+	if (ret)
+		return ret;
+
+	pci_set_master(pdev);
+
+	ret = of_platform_default_populate(dev->of_node, NULL, dev);
+	if (ret)
+		goto err_unload_overlay;
+
+	return 0;
+
+err_unload_overlay:
+	lan966x_pci_unload_overlay(data);
+	return ret;
+}
+
+static void lan966x_pci_remove(struct pci_dev *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct lan966x_pci *data = dev_get_drvdata(dev);
+
+	of_platform_depopulate(dev);
+
+	lan966x_pci_unload_overlay(data);
+
+	pci_clear_master(pdev);
+}
+
+static struct pci_device_id lan966x_pci_ids[] = {
+	{ PCI_DEVICE(0x1055, 0x9660) },
+	{ 0, }
+};
+MODULE_DEVICE_TABLE(pci, lan966x_pci_ids);
+
+static struct pci_driver lan966x_pci_driver = {
+	.name = "mchp_lan966x_pci",
+	.id_table = lan966x_pci_ids,
+	.probe = lan966x_pci_probe,
+	.remove = lan966x_pci_remove,
+};
+module_pci_driver(lan966x_pci_driver);
+
+MODULE_AUTHOR("Herve Codina <herve.codina@bootlin.com>");
+MODULE_DESCRIPTION("Microchip LAN966x PCI driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/mfd/lan966x_pci.dtso b/drivers/mfd/lan966x_pci.dtso
new file mode 100644
index 000000000000..041f4319e4cd
--- /dev/null
+++ b/drivers/mfd/lan966x_pci.dtso
@@ -0,0 +1,167 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2022 Microchip UNG
+ */
+
+#include <dt-bindings/clock/microchip,lan966x.h>
+#include <dt-bindings/interrupt-controller/irq.h>
+#include <dt-bindings/mfd/atmel-flexcom.h>
+#include <dt-bindings/phy/phy-lan966x-serdes.h>
+#include <dt-bindings/gpio/gpio.h>
+
+/dts-v1/;
+/plugin/;
+
+/ {
+	fragment@0 {
+		target-path="";
+		__overlay__ {
+			#address-cells = <3>;
+			#size-cells = <2>;
+
+			pci-ep-bus@0 {
+				compatible = "simple-bus";
+				#address-cells = <1>;
+				#size-cells = <1>;
+
+				/*
+				 * map @0xe2000000 (32MB) to BAR0 (CPU)
+				 * map @0xe0000000 (16MB) to BAR1 (AMBA)
+				 */
+				ranges = <0xe2000000 0x00 0x00 0x00 0x2000000
+				          0xe0000000 0x01 0x00 0x00 0x1000000>;
+
+				oic: oic@e00c0120 {
+					compatible = "microchip,lan966x-oic";
+					#interrupt-cells = <2>;
+					interrupt-controller;
+					interrupts = <0>; /* PCI INTx assigned interrupt */
+					reg = <0xe00c0120 0x190>;
+				};
+
+				cpu_clk: cpu_clk {
+					compatible = "fixed-clock";
+					#clock-cells = <0>;
+					clock-frequency = <600000000>;  // CPU clock = 600MHz
+				};
+
+				ddr_clk: ddr_clk {
+					compatible = "fixed-clock";
+					#clock-cells = <0>;
+					clock-frequency = <30000000>;  // Fabric clock = 30MHz
+				};
+
+				sys_clk: sys_clk {
+					compatible = "fixed-clock";
+					#clock-cells = <0>;
+					clock-frequency = <15625000>;  // System clock = 15.625MHz
+				};
+
+				cpu_ctrl: syscon@e00c0000 {
+					compatible = "microchip,lan966x-cpu-syscon", "syscon";
+					reg = <0xe00c0000 0xa8>;
+				};
+
+				reset: reset@e200400c {
+					compatible = "microchip,lan966x-switch-reset";
+					reg = <0xe200400c 0x4>;
+					reg-names = "gcb";
+					#reset-cells = <1>;
+					cpu-syscon = <&cpu_ctrl>;
+				};
+
+				gpio: pinctrl@e2004064 {
+					compatible = "microchip,lan966x-pinctrl";
+					reg = <0xe2004064 0xb4>,
+					      <0xe2010024 0x138>;
+					resets = <&reset 0>;
+					reset-names = "switch";
+					gpio-controller;
+					#gpio-cells = <2>;
+					gpio-ranges = <&gpio 0 0 78>;
+					interrupt-parent = <&oic>;
+					interrupt-controller;
+					interrupts = <17 IRQ_TYPE_LEVEL_HIGH>;
+					#interrupt-cells = <2>;
+
+					tod_pins: tod_pins {
+						pins = "GPIO_36";
+						function = "ptpsync_1";
+					};
+
+					fc0_a_pins: fcb4-i2c-pins {
+						/* RXD, TXD */
+						pins = "GPIO_9", "GPIO_10";
+						function = "fc0_a";
+					};
+
+				};
+
+				serdes: serdes@e202c000 {
+					compatible = "microchip,lan966x-serdes";
+					reg = <0xe202c000 0x9c>,
+					      <0xe2004010 0x4>;
+					#phy-cells = <2>;
+				};
+
+				mdio1: mdio@e200413c {
+					#address-cells = <1>;
+					#size-cells = <0>;
+					compatible = "microchip,lan966x-miim";
+					reg = <0xe200413c 0x24>,
+					      <0xe2010020 0x4>;
+
+					resets = <&reset 0>;
+					reset-names = "switch";
+
+					lan966x_phy0: ethernet-lan966x_phy@1 {
+						reg = <1>;
+					};
+
+					lan966x_phy1: ethernet-lan966x_phy@2 {
+						reg = <2>;
+					};
+				};
+
+				switch: switch@e0000000 {
+					compatible = "microchip,lan966x-switch";
+					reg = <0xe0000000 0x0100000>,
+					      <0xe2000000 0x0800000>;
+					reg-names = "cpu", "gcb";
+
+					interrupt-parent = <&oic>;
+					interrupts = <12 IRQ_TYPE_LEVEL_HIGH>,
+						     <9 IRQ_TYPE_LEVEL_HIGH>;
+					interrupt-names = "xtr", "ana";
+
+					resets = <&reset 0>;
+					reset-names = "switch";
+
+					pinctrl-names = "default";
+					pinctrl-0 = <&tod_pins>;
+
+					ethernet-ports {
+						#address-cells = <1>;
+						#size-cells = <0>;
+
+						port0: port@0 {
+							phy-handle = <&lan966x_phy0>;
+
+							reg = <0>;
+							phy-mode = "gmii";
+							phys = <&serdes 0 CU(0)>;
+						};
+
+						port1: port@1 {
+							phy-handle = <&lan966x_phy1>;
+
+							reg = <1>;
+							phy-mode = "gmii";
+							phys = <&serdes 1 CU(1)>;
+						};
+					};
+				};
+			};
+		};
+	};
+};
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 568410e64ce6..4bfc3f2aafa4 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.45.0

Re: [PATCH v2 18/19] mfd: Add support for LAN966x PCI device

[Andy Shevchenko]

Mon, May 27, 2024 at 06:14:45PM +0200, Herve Codina kirjoitti:
> 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.

...

> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>

> +#include <linux/kernel.h>

Why do you need this?

> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/pci.h>
> +#include <linux/slab.h>

General comment to the headers (in all your patches), try to follow IWYU
principle, i.e. include what you use explicitly and don't use "proxy" headers
such as kernel.h which basically shouldn't be used at all in the drivers.

...

> +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;

There is a macro for that IRQ_RETVAL() IIRC.

> +}

...

> +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);

> +

Redundant blank line.

> +	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);
> +}

...

> +static int lan966x_pci_load_overlay(struct lan966x_pci *data)
> +{
> +	u32 dtbo_size = __dtbo_lan966x_pci_end - __dtbo_lan966x_pci_begin;
> +	void *dtbo_start = __dtbo_lan966x_pci_begin;
> +	int ret;
> +
> +	ret = of_overlay_fdt_apply(dtbo_start, dtbo_size, &data->ovcs_id, data->dev->of_node);

dev_of_node() ?

> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}

...

> +static int lan966x_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct lan966x_pci *data;
> +	int ret;

> +	if (!dev->of_node) {
> +		dev_err(dev, "Missing of_node for device\n");
> +		return -EINVAL;
> +	}

Why do you need this? The code you have in _create_intr_ctrl() will take care
already for this case.

> +	/* Need to be done before devm_pci_dev_create_intr_ctrl.
> +	 * It allocates an IRQ and so pdev->irq is updated

Missing period at the end.

> +	 */
> +	ret = pcim_enable_device(pdev);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_pci_dev_create_intr_ctrl(pdev);
> +	if (ret)
> +		return ret;
> +
> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	dev_set_drvdata(dev, data);
> +	data->dev = dev;
> +	data->pci_dev = pdev;
> +
> +	ret = lan966x_pci_load_overlay(data);
> +	if (ret)
> +		return ret;

> +	pci_set_master(pdev);

You don't use MSI, what is this for?

> +	ret = of_platform_default_populate(dev->of_node, NULL, dev);

dev_of_node()

> +	if (ret)
> +		goto err_unload_overlay;
> +
> +	return 0;
> +
> +err_unload_overlay:
> +	lan966x_pci_unload_overlay(data);
> +	return ret;
> +}

...

> +static void lan966x_pci_remove(struct pci_dev *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct lan966x_pci *data = dev_get_drvdata(dev);

platform_get_drvdata()

> +	of_platform_depopulate(dev);
> +
> +	lan966x_pci_unload_overlay(data);

> +	pci_clear_master(pdev);

No need to call this excplicitly when pcim_enable_device() was called.

> +}

...

> +static struct pci_device_id lan966x_pci_ids[] = {
> +	{ PCI_DEVICE(0x1055, 0x9660) },

Don't you have VENDOR_ID defined somewhere?

> +	{ 0, }

Unneeded ' 0, ' part

> +};

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 18/19] mfd: Add support for LAN966x PCI device

[Herve Codina]

My bad, I wrongly answered first in private.
-> Resend my answers with people in Cc

Andy, I will also resend your reply.

Sorry for this mistake.

Herve

On Thu, 20 Jun 2024 17:56:46 +0200
Herve Codina <herve.codina@bootlin.com> wrote:

> Hi Andy,
> 
> On Wed, 5 Jun 2024 23:24:43 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> 
> > Mon, May 27, 2024 at 06:14:45PM +0200, Herve Codina kirjoitti:
> > > 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.  
> > 
> > ...
> > 
> > > +#include <linux/irq.h>
> > > +#include <linux/irqdomain.h>  
> > 
> > > +#include <linux/kernel.h>  
> > 
> > Why do you need this?
> > 
> > > +#include <linux/module.h>
> > > +#include <linux/of.h>
> > > +#include <linux/of_platform.h>
> > > +#include <linux/pci.h>
> > > +#include <linux/slab.h>  
> > 
> > General comment to the headers (in all your patches), try to follow IWYU
> > principle, i.e. include what you use explicitly and don't use "proxy" headers
> > such as kernel.h which basically shouldn't be used at all in the drivers.
> 
> Sure, I will remove unneeded header inclusion.
> 
> > 
> > ...
> > 
> > > +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;  
> > 
> > There is a macro for that IRQ_RETVAL() IIRC.
> 
> Didn't known about that. Thanks for pointing out!
> I will use it :)
> 
> > 
> > > +}  
> > 
> > ...
> > 
> > > +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);  
> > 
> > > +  
> > 
> > Redundant blank line.
> 
> Will be removed.
> 
> > 
> > > +	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);
> > > +}  
> > 
> > ...
> > 
> > > +static int lan966x_pci_load_overlay(struct lan966x_pci *data)
> > > +{
> > > +	u32 dtbo_size = __dtbo_lan966x_pci_end - __dtbo_lan966x_pci_begin;
> > > +	void *dtbo_start = __dtbo_lan966x_pci_begin;
> > > +	int ret;
> > > +
> > > +	ret = of_overlay_fdt_apply(dtbo_start, dtbo_size, &data->ovcs_id, data->dev->of_node);  
> > 
> > dev_of_node() ?
> 
> Yes indeed.
> 
> > 
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	return 0;
> > > +}  
> > 
> > ...
> > 
> > > +static int lan966x_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > > +{
> > > +	struct device *dev = &pdev->dev;
> > > +	struct lan966x_pci *data;
> > > +	int ret;  
> > 
> > > +	if (!dev->of_node) {
> > > +		dev_err(dev, "Missing of_node for device\n");
> > > +		return -EINVAL;
> > > +	}  
> > 
> > Why do you need this? The code you have in _create_intr_ctrl() will take care
> > already for this case.
> 
> The code in _create_intr_ctrl checks for fwnode and not an of_node.
> 
> The check here is to ensure that an of_node is available as it will be use
> for DT overlay loading.
> 
> I will keep the check here and use dev_of_node() instead of dev->of_node.
> 
> > 
> > > +	/* Need to be done before devm_pci_dev_create_intr_ctrl.
> > > +	 * It allocates an IRQ and so pdev->irq is updated  
> > 
> > Missing period at the end.
> 
> Will be added.
> 
> > 
> > > +	 */
> > > +	ret = pcim_enable_device(pdev);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = devm_pci_dev_create_intr_ctrl(pdev);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> > > +	if (!data)
> > > +		return -ENOMEM;
> > > +
> > > +	dev_set_drvdata(dev, data);
> > > +	data->dev = dev;
> > > +	data->pci_dev = pdev;
> > > +
> > > +	ret = lan966x_pci_load_overlay(data);
> > > +	if (ret)
> > > +		return ret;  
> > 
> > > +	pci_set_master(pdev);  
> > 
> > You don't use MSI, what is this for?
> 
> DMA related.
> Allows the PCI device to be master on the bus and so initiate transactions.
> 
> Did I misunderstood ?
> 
> > 
> > > +	ret = of_platform_default_populate(dev->of_node, NULL, dev);  
> > 
> > dev_of_node()
> 
> Yes, sure.
> 
> > 
> > > +	if (ret)
> > > +		goto err_unload_overlay;
> > > +
> > > +	return 0;
> > > +
> > > +err_unload_overlay:
> > > +	lan966x_pci_unload_overlay(data);
> > > +	return ret;
> > > +}  
> > 
> > ...
> > 
> > > +static void lan966x_pci_remove(struct pci_dev *pdev)
> > > +{
> > > +	struct device *dev = &pdev->dev;
> > > +	struct lan966x_pci *data = dev_get_drvdata(dev);  
> > 
> > platform_get_drvdata()
> 
> platform_get_drvdata() is related to platform_device.
> There is no platform_device here but a pci_dev.
> 
> I will use pci_get_drvdata() here and update probe() to
> use pci_set_drvdata() for consistency.
> 
> > 
> > > +	of_platform_depopulate(dev);
> > > +
> > > +	lan966x_pci_unload_overlay(data);  
> > 
> > > +	pci_clear_master(pdev);  
> > 
> > No need to call this excplicitly when pcim_enable_device() was called.
> 
> You're right. I will remove this call.
> 
> > 
> > > +}  
> > 
> > ...
> > 
> > > +static struct pci_device_id lan966x_pci_ids[] = {
> > > +	{ PCI_DEVICE(0x1055, 0x9660) },  
> > 
> > Don't you have VENDOR_ID defined somewhere?
> 
> No and 0x1055 is taken by PCI_VENDOR_ID_EFAR in pci-ids.h
> but SMSC acquired EFAR late 1990's and MCHP acquired SMSC in 2012
> https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/microchip/lan743x_main.h#L851
> 
> I will patch pci-ids.h to create:
>   #define PCI_VENDOR_ID_SMSC PCI_VENDOR_ID_EFAR
>   #define PCI_VENDOR_ID_MCHP PCI_VENDOR_ID_SMSC
> As part of this patch, I will update lan743x_main.h to remove its own #define
> 
> And use PCI_VENDOR_ID_MCHP in this series.
> 
> > 
> > > +	{ 0, }  
> > 
> > Unneeded ' 0, ' part
> 
> Will be removed.
> 
> > 
> > > +};  
> > 
> 
> Thanks a lot for your review.
> 
> Best regards,
> Hervé

Re: [PATCH v2 18/19] mfd: Add support for LAN966x PCI device

[Herve Codina]

My bad, I wrongly answered first in private.
I already eesend my answers with people in Cc

Now, this is the Andy's your reply.

Sorry for this mistake.

Herve

On Thu, 20 Jun 2024 18:07:16 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Thu, Jun 20, 2024 at 5:56 PM Herve Codina <herve.codina@bootlin.com> wrote:
> > On Wed, 5 Jun 2024 23:24:43 +0300
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:  
> > > Mon, May 27, 2024 at 06:14:45PM +0200, Herve Codina kirjoitti:  
> 
> ...
> 
> > > > +   if (!dev->of_node) {
> > > > +           dev_err(dev, "Missing of_node for device\n");
> > > > +           return -EINVAL;
> > > > +   }  
> > >
> > > Why do you need this? The code you have in _create_intr_ctrl() will take care
> > > already for this case.  
> >
> > The code in _create_intr_ctrl checks for fwnode and not an of_node.
> >
> > The check here is to ensure that an of_node is available as it will be use
> > for DT overlay loading.  
> 
> So, what exactly do you want to check? fwnode check covers this.
> 
> > I will keep the check here and use dev_of_node() instead of dev->of_node.  
> 
> It needs to be well justified as from a coding point of view this is a
> duplication.
> 
> ...
> 
> > > > +   pci_set_master(pdev);  
> > >
> > > You don't use MSI, what is this for?  
> >
> > DMA related.
> > Allows the PCI device to be master on the bus and so initiate transactions.
> >
> > Did I misunderstood ?  
> 
> So, you mean that the PCI device may initiate DMA transactions and
> they are not related to MSI, correct?
> 
> ...
> 
> > > > +static struct pci_device_id lan966x_pci_ids[] = {
> > > > +   { PCI_DEVICE(0x1055, 0x9660) },  
> > >
> > > Don't you have VENDOR_ID defined somewhere?  
> >
> > No and 0x1055 is taken by PCI_VENDOR_ID_EFAR in pci-ids.h
> > but SMSC acquired EFAR late 1990's and MCHP acquired SMSC in 2012
> > https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/microchip/lan743x_main.h#L851
> >
> > I will patch pci-ids.h to create:
> >   #define PCI_VENDOR_ID_SMSC PCI_VENDOR_ID_EFAR
> >   #define PCI_VENDOR_ID_MCHP PCI_VENDOR_ID_SMSC
> > As part of this patch, I will update lan743x_main.h to remove its own #define
> >
> > And use PCI_VENDOR_ID_MCHP in this series.  
> 
> Okay, but I don't think (but I haven't checked) we have something like
> this ever done there. In any case it's up to Bjorn how to implement
> this.
> 
> > > > +   { 0, }  
> > >
> > > Unneeded ' 0, ' part  
> >
> > Will be removed.
> >  
> > > > +};  
> 

Re: [PATCH v2 18/19] mfd: Add support for LAN966x PCI device

[Herve Codina]

Hi Andy,

On Thu, 20 Jun 2024 18:43:09 +0200
Herve Codina <herve.codina@bootlin.com> wrote:

> My bad, I wrongly answered first in private.
> I already eesend my answers with people in Cc
> 
> Now, this is the Andy's your reply.
> 
> Sorry for this mistake.
> 
> Herve
> 
> On Thu, 20 Jun 2024 18:07:16 +0200
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> 
> > On Thu, Jun 20, 2024 at 5:56 PM Herve Codina <herve.codina@bootlin.com> wrote:  
> > > On Wed, 5 Jun 2024 23:24:43 +0300
> > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:    
> > > > Mon, May 27, 2024 at 06:14:45PM +0200, Herve Codina kirjoitti:    
> > 
> > ...
> >   
> > > > > +   if (!dev->of_node) {
> > > > > +           dev_err(dev, "Missing of_node for device\n");
> > > > > +           return -EINVAL;
> > > > > +   }    
> > > >
> > > > Why do you need this? The code you have in _create_intr_ctrl() will take care
> > > > already for this case.    
> > >
> > > The code in _create_intr_ctrl checks for fwnode and not an of_node.
> > >
> > > The check here is to ensure that an of_node is available as it will be use
> > > for DT overlay loading.    
> > 
> > So, what exactly do you want to check? fwnode check covers this.
> >   
> > > I will keep the check here and use dev_of_node() instead of dev->of_node.    
> > 
> > It needs to be well justified as from a coding point of view this is a
> > duplication.

On DT based system, if a fwnode is set it is an of_node.
On ACPI, if a fwnode is set is is an acpi_node.

The core PCI, when it successfully creates the DT node for a device
(CONFIG_PCI_DYNAMIC_OF_NODES) set the of_node of this device.
So we can have a device with:
 - fwnode from ACPI
 - of_node from core PCI creation

This driver needs the of_node to load the overlay.
Even if the core PCI cannot create a DT node for the PCI device right
now, I don't expect this LAN855x PCI driver updated when the core PCI
is able to create this PCI device DT node.

> > 
> > ...
> >   
> > > > > +   pci_set_master(pdev);    
> > > >
> > > > You don't use MSI, what is this for?    
> > >
> > > DMA related.
> > > Allows the PCI device to be master on the bus and so initiate transactions.
> > >
> > > Did I misunderstood ?    
> > 
> > So, you mean that the PCI device may initiate DMA transactions and
> > they are not related to MSI, correct?

That's my understanding.
Right now, the internal LAN966x DMA controller is not used but it will be
used in a near future.

> > 
> > ...
> >   
> > > > > +static struct pci_device_id lan966x_pci_ids[] = {
> > > > > +   { PCI_DEVICE(0x1055, 0x9660) },    
> > > >
> > > > Don't you have VENDOR_ID defined somewhere?    
> > >
> > > No and 0x1055 is taken by PCI_VENDOR_ID_EFAR in pci-ids.h
> > > but SMSC acquired EFAR late 1990's and MCHP acquired SMSC in 2012
> > > https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/microchip/lan743x_main.h#L851
> > >
> > > I will patch pci-ids.h to create:
> > >   #define PCI_VENDOR_ID_SMSC PCI_VENDOR_ID_EFAR
> > >   #define PCI_VENDOR_ID_MCHP PCI_VENDOR_ID_SMSC
> > > As part of this patch, I will update lan743x_main.h to remove its own #define
> > >
> > > And use PCI_VENDOR_ID_MCHP in this series.    
> > 
> > Okay, but I don't think (but I haven't checked) we have something like
> > this ever done there. In any case it's up to Bjorn how to implement
> > this.

Right, I wait for Bjorn reply before changing anything.

Best regards,
Hervé

Re: [PATCH v2 18/19] mfd: Add support for LAN966x PCI device

[Andy Shevchenko]

On Thu, Jun 20, 2024 at 7:19 PM Herve Codina <herve.codina@bootlin.com> wrote:
> On Thu, 20 Jun 2024 18:43:09 +0200
> Herve Codina <herve.codina@bootlin.com> wrote:
> > On Thu, 20 Jun 2024 18:07:16 +0200
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > On Thu, Jun 20, 2024 at 5:56 PM Herve Codina <herve.codina@bootlin.com> wrote:
> > > > On Wed, 5 Jun 2024 23:24:43 +0300
> > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > > Mon, May 27, 2024 at 06:14:45PM +0200, Herve Codina kirjoitti:

...

> > > > > > +   if (!dev->of_node) {
> > > > > > +           dev_err(dev, "Missing of_node for device\n");
> > > > > > +           return -EINVAL;
> > > > > > +   }
> > > > >
> > > > > Why do you need this? The code you have in _create_intr_ctrl() will take care
> > > > > already for this case.
> > > >
> > > > The code in _create_intr_ctrl checks for fwnode and not an of_node.
> > > >
> > > > The check here is to ensure that an of_node is available as it will be use
> > > > for DT overlay loading.
> > >
> > > So, what exactly do you want to check? fwnode check covers this.
> > >
> > > > I will keep the check here and use dev_of_node() instead of dev->of_node.
> > >
> > > It needs to be well justified as from a coding point of view this is a
> > > duplication.
>
> On DT based system, if a fwnode is set it is an of_node.
> On ACPI, if a fwnode is set is is an acpi_node.
>
> The core PCI, when it successfully creates the DT node for a device
> (CONFIG_PCI_DYNAMIC_OF_NODES) set the of_node of this device.
> So we can have a device with:
>  - fwnode from ACPI
>  - of_node from core PCI creation

Does PCI device creation not set fwnode?

> This driver needs the of_node to load the overlay.
> Even if the core PCI cannot create a DT node for the PCI device right
> now, I don't expect this LAN855x PCI driver updated when the core PCI
> is able to create this PCI device DT node.

If it's really needed, I think the correct call here is is_of_node()
to show exactly why it's not a duplication. It also needs a comment on
top of this call.

...

> > > > > > +static struct pci_device_id lan966x_pci_ids[] = {
> > > > > > +   { PCI_DEVICE(0x1055, 0x9660) },
> > > > >
> > > > > Don't you have VENDOR_ID defined somewhere?
> > > >
> > > > No and 0x1055 is taken by PCI_VENDOR_ID_EFAR in pci-ids.h
> > > > but SMSC acquired EFAR late 1990's and MCHP acquired SMSC in 2012
> > > > https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/microchip/lan743x_main.h#L851
> > > >
> > > > I will patch pci-ids.h to create:
> > > >   #define PCI_VENDOR_ID_SMSC PCI_VENDOR_ID_EFAR
> > > >   #define PCI_VENDOR_ID_MCHP PCI_VENDOR_ID_SMSC
> > > > As part of this patch, I will update lan743x_main.h to remove its own #define
> > > >
> > > > And use PCI_VENDOR_ID_MCHP in this series.
> > >
> > > Okay, but I don't think (but I haven't checked) we have something like
> > > this ever done there. In any case it's up to Bjorn how to implement
> > > this.
>
> Right, I wait for Bjorn reply before changing anything.

But we already have the vendor ID with the same value. Even if the
company was acquired, the old ID still may be used. In that case an
update on PCI IDs can go in a separate change justifying it. In any
case, I would really want to hear from Bjorn on this and if nothing
happens, to use the existing vendor ID for now to speed up the series
to be reviewed/processed.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 18/19] mfd: Add support for LAN966x PCI device

[Bjorn Helgaas]

On Fri, Jun 21, 2024 at 05:45:05PM +0200, Andy Shevchenko wrote:
> On Thu, Jun 20, 2024 at 7:19 PM Herve Codina <herve.codina@bootlin.com> wrote:
> > On Thu, 20 Jun 2024 18:43:09 +0200
> > Herve Codina <herve.codina@bootlin.com> wrote:
> > > On Thu, 20 Jun 2024 18:07:16 +0200
> > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > On Thu, Jun 20, 2024 at 5:56 PM Herve Codina <herve.codina@bootlin.com> wrote:
> > > > > On Wed, 5 Jun 2024 23:24:43 +0300
> > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > > > Mon, May 27, 2024 at 06:14:45PM +0200, Herve Codina kirjoitti:

> > > > > > > +static struct pci_device_id lan966x_pci_ids[] = {
> > > > > > > +   { PCI_DEVICE(0x1055, 0x9660) },
> > > > > >
> > > > > > Don't you have VENDOR_ID defined somewhere?
> > > > >
> > > > > No and 0x1055 is taken by PCI_VENDOR_ID_EFAR in pci-ids.h
> > > > > but SMSC acquired EFAR late 1990's and MCHP acquired SMSC in 2012
> > > > > https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/microchip/lan743x_main.h#L851
> > > > >
> > > > > I will patch pci-ids.h to create:
> > > > >   #define PCI_VENDOR_ID_SMSC PCI_VENDOR_ID_EFAR
> > > > >   #define PCI_VENDOR_ID_MCHP PCI_VENDOR_ID_SMSC
> > > > > As part of this patch, I will update lan743x_main.h to remove its own #define
> > > > >
> > > > > And use PCI_VENDOR_ID_MCHP in this series.
> > > >
> > > > Okay, but I don't think (but I haven't checked) we have something like
> > > > this ever done there. In any case it's up to Bjorn how to implement
> > > > this.
> >
> > Right, I wait for Bjorn reply before changing anything.
> 
> But we already have the vendor ID with the same value. Even if the
> company was acquired, the old ID still may be used. In that case an
> update on PCI IDs can go in a separate change justifying it. In any
> case, I would really want to hear from Bjorn on this and if nothing
> happens, to use the existing vendor ID for now to speed up the series
> to be reviewed/processed.

We have "#define PCI_VENDOR_ID_EFAR 0x1055" in pci_ids.h, but
https://pcisig.com/membership/member-companies?combine=1055 shows no
results, so it *looks* like EFAR/SMSC/MCHP are currently squatting on
that ID without it being officially assigned.

I think MCHP needs to register 0x1055 with the PCI-SIG
(administration@pcisig.com) if it wants to continue using it.
The vendor is responsible for managing the Device ID space, so this
registration includes the burden of tracking all the Device IDs that
were assigned by EFAR and SMSC and now MCHP so there are no conflicts.

I don't want to change the existing PCI_VENDOR_ID_EFAR, and I also
don't want to add a PCI_VENDOR_ID_MCHP for 0x1055 until that ID has
been registered with the PCI-SIG.

So I propose that you use PCI_VENDOR_ID_EFAR for now, and if/when MCHP
registers 0x1055 with PCI-SIG so it is unambiguously owned by MCHP, we
can add "#define PCI_VENDOR_ID_MCHP PCI_VENDOR_ID_EFAR" or similar.
As Andy points out, this would be a separate logical change in its own
patch.

Bjorn

Re: [PATCH v2 18/19] mfd: Add support for LAN966x PCI device

[Steen Hegelund]

Hi Bjorn,

I am not sure what went wrong here.

I have seen that lspci lists 'Microchip / SMSC' for the 0x1055 Vendor
ID value and as mentioned previously there has been a number of
aquicisions over the years, so that the ID has been absorbed but not
necessarily re-registered.

Anyway I have started an investigation, so we can determine what
up/down in this.

I agree that for now this will have to be PCI_VENDOR_ID_EFAR, and I
will return with an update as soon as I know more.

Best Regards
Steen

On Fri, 2024-06-21 at 13:49 -0500, Bjorn Helgaas wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On Fri, Jun 21, 2024 at 05:45:05PM +0200, Andy Shevchenko wrote:
> > On Thu, Jun 20, 2024 at 7:19 PM Herve Codina
> > <herve.codina@bootlin.com> wrote:
> > > On Thu, 20 Jun 2024 18:43:09 +0200
> > > Herve Codina <herve.codina@bootlin.com> wrote:
> > > > On Thu, 20 Jun 2024 18:07:16 +0200
> > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > > On Thu, Jun 20, 2024 at 5:56 PM Herve Codina
> > > > > <herve.codina@bootlin.com> wrote:
> > > > > > On Wed, 5 Jun 2024 23:24:43 +0300
> > > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > > > > Mon, May 27, 2024 at 06:14:45PM +0200, Herve Codina
> > > > > > > kirjoitti:
> 
> > > > > > > > +static struct pci_device_id lan966x_pci_ids[] = {
> > > > > > > > +   { PCI_DEVICE(0x1055, 0x9660) },
> > > > > > > 
> > > > > > > Don't you have VENDOR_ID defined somewhere?
> > > > > > 
> > > > > > No and 0x1055 is taken by PCI_VENDOR_ID_EFAR in pci-ids.h
> > > > > > but SMSC acquired EFAR late 1990's and MCHP acquired SMSC
> > > > > > in 2012
> > > > > > https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/microchip/lan743x_main.h#L851
> > > > > > 
> > > > > > I will patch pci-ids.h to create:
> > > > > >   #define PCI_VENDOR_ID_SMSC PCI_VENDOR_ID_EFAR
> > > > > >   #define PCI_VENDOR_ID_MCHP PCI_VENDOR_ID_SMSC
> > > > > > As part of this patch, I will update lan743x_main.h to
> > > > > > remove its own #define
> > > > > > 
> > > > > > And use PCI_VENDOR_ID_MCHP in this series.
> > > > > 
> > > > > Okay, but I don't think (but I haven't checked) we have
> > > > > something like
> > > > > this ever done there. In any case it's up to Bjorn how to
> > > > > implement
> > > > > this.
> > > 
> > > Right, I wait for Bjorn reply before changing anything.
> > 
> > But we already have the vendor ID with the same value. Even if the
> > company was acquired, the old ID still may be used. In that case an
> > update on PCI IDs can go in a separate change justifying it. In any
> > case, I would really want to hear from Bjorn on this and if nothing
> > happens, to use the existing vendor ID for now to speed up the
> > series
> > to be reviewed/processed.
> 
> We have "#define PCI_VENDOR_ID_EFAR 0x1055" in pci_ids.h, but
> https://pcisig.com/membership/member-companies?combine=1055 shows no
> results, so it *looks* like EFAR/SMSC/MCHP are currently squatting on
> that ID without it being officially assigned.
> 
> I think MCHP needs to register 0x1055 with the PCI-SIG
> (administration@pcisig.com) if it wants to continue using it.
> The vendor is responsible for managing the Device ID space, so this
> registration includes the burden of tracking all the Device IDs that
> were assigned by EFAR and SMSC and now MCHP so there are no
> conflicts.
> 
> I don't want to change the existing PCI_VENDOR_ID_EFAR, and I also
> don't want to add a PCI_VENDOR_ID_MCHP for 0x1055 until that ID has
> been registered with the PCI-SIG.
> 
> So I propose that you use PCI_VENDOR_ID_EFAR for now, and if/when
> MCHP
> registers 0x1055 with PCI-SIG so it is unambiguously owned by MCHP,
> we
> can add "#define PCI_VENDOR_ID_MCHP PCI_VENDOR_ID_EFAR" or similar.
> As Andy points out, this would be a separate logical change in its
> own
> patch.
> 
> Bjorn

Re: [PATCH v2 18/19] mfd: Add support for LAN966x PCI device

[Herve Codina]

Hi Steen, Bjorn, Andy,

On Mon, 24 Jun 2024 13:46:32 +0200
Steen Hegelund <steen.hegelund@microchip.com> wrote:

> Hi Bjorn,
> 
> I am not sure what went wrong here.
> 
> I have seen that lspci lists 'Microchip / SMSC' for the 0x1055 Vendor
> ID value and as mentioned previously there has been a number of
> aquicisions over the years, so that the ID has been absorbed but not
> necessarily re-registered.
> 
> Anyway I have started an investigation, so we can determine what
> up/down in this.
> 
> I agree that for now this will have to be PCI_VENDOR_ID_EFAR, and I
> will return with an update as soon as I know more.
> 

Right, PCI_VENDOR_ID_EFAR will be directly used in the next iteration.

Best regards,
Hervé

Re: [PATCH v2 18/19] mfd: Add support for LAN966x PCI device

[Steen Hegelund]

Hi Bjorn, and Herve,

Bill Mahany from Microchip has now been in contact with PCI-SIG, and
has been able to get confirmation that Vendor ID 0x1055 is still
belonging to Microchip even though this is not visible on their
webpage.

I have attached a snippet of the conversation.

I hope this settles the matter of the Vendor ID 0x1055.

Best Regards
Steen

=== Copied from the conversation ===

   Hi Bill,
    
   Thank you for your email. We can confirm VID 4181 (1055 Hex) is
   Microchip’s. This is not listed on the website because we are only
   able to list one VID per company.
    
   Please feel free to contact us if you have any questions.
   
   Best Regards,
   
   PCI-SIG Administration
   
   Main: 503-619-0569 | Fax: 503-644-6708
   3855 SW 153rd Drive, Beaverton, OR 97003 USA
   Email: administration@pcisig.com
   
   
    
   www.pcisig.com | Connect with us on LinkedIn and Twitter @pci_sig
    
   From: bill.mahany@microchip.com (administration)
   <administration@pcisig.com>
   Sent: Monday, June 24, 2024 11:07 AM
   To: administration@pcisig.com
   Subject: RE: vendor id missing from member companies list
    
   Hello once again.
    
   Please refer to the below. Could you please reverify that Microchip
   Technology is still in control of 4181 (0x1055).
    
   Apparently, the lack of a search result for 4181 (0x1055) at
   https://pcisig.com/membership/member-companiesis
   Is still causing issues in the Linux community -
   https://lore.kernel.org/all/20240621184923.GA1398370@bhelgaas/
    
   Thanks
    
   Bill Mahany
    
On Mon, 2024-06-24 at 13:46 +0200, Steen Hegelund wrote:
> Hi Bjorn,
> 
> I am not sure what went wrong here.
> 
> I have seen that lspci lists 'Microchip / SMSC' for the 0x1055 Vendor
> ID value and as mentioned previously there has been a number of
> aquicisions over the years, so that the ID has been absorbed but not
> necessarily re-registered.
> 
> Anyway I have started an investigation, so we can determine what
> up/down in this.
> 
> I agree that for now this will have to be PCI_VENDOR_ID_EFAR, and I
> will return with an update as soon as I know more.
> 
> Best Regards
> Steen
> 
> On Fri, 2024-06-21 at 13:49 -0500, Bjorn Helgaas wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > know the content is safe
> > 
> > On Fri, Jun 21, 2024 at 05:45:05PM +0200, Andy Shevchenko wrote:
> > > On Thu, Jun 20, 2024 at 7:19 PM Herve Codina
> > > <herve.codina@bootlin.com> wrote:
> > > > On Thu, 20 Jun 2024 18:43:09 +0200
> > > > Herve Codina <herve.codina@bootlin.com> wrote:
> > > > > On Thu, 20 Jun 2024 18:07:16 +0200
> > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > > > On Thu, Jun 20, 2024 at 5:56 PM Herve Codina
> > > > > > <herve.codina@bootlin.com> wrote:
> > > > > > > On Wed, 5 Jun 2024 23:24:43 +0300
> > > > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > > > > > Mon, May 27, 2024 at 06:14:45PM +0200, Herve Codina
> > > > > > > > kirjoitti:
> > 
> > > > > > > > > +static struct pci_device_id lan966x_pci_ids[] = {
> > > > > > > > > +   { PCI_DEVICE(0x1055, 0x9660) },
> > > > > > > > 
> > > > > > > > Don't you have VENDOR_ID defined somewhere?
> > > > > > > 
> > > > > > > No and 0x1055 is taken by PCI_VENDOR_ID_EFAR in pci-ids.h
> > > > > > > but SMSC acquired EFAR late 1990's and MCHP acquired SMSC
> > > > > > > in 2012
> > > > > > > https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/microchip/lan743x_main.h#L851
> > > > > > > 
> > > > > > > I will patch pci-ids.h to create:
> > > > > > >   #define PCI_VENDOR_ID_SMSC PCI_VENDOR_ID_EFAR
> > > > > > >   #define PCI_VENDOR_ID_MCHP PCI_VENDOR_ID_SMSC
> > > > > > > As part of this patch, I will update lan743x_main.h to
> > > > > > > remove its own #define
> > > > > > > 
> > > > > > > And use PCI_VENDOR_ID_MCHP in this series.
> > > > > > 
> > > > > > Okay, but I don't think (but I haven't checked) we have
> > > > > > something like
> > > > > > this ever done there. In any case it's up to Bjorn how to
> > > > > > implement
> > > > > > this.
> > > > 
> > > > Right, I wait for Bjorn reply before changing anything.
> > > 
> > > But we already have the vendor ID with the same value. Even if
> > > the
> > > company was acquired, the old ID still may be used. In that case
> > > an
> > > update on PCI IDs can go in a separate change justifying it. In
> > > any
> > > case, I would really want to hear from Bjorn on this and if
> > > nothing
> > > happens, to use the existing vendor ID for now to speed up the
> > > series
> > > to be reviewed/processed.
> > 
> > We have "#define PCI_VENDOR_ID_EFAR 0x1055" in pci_ids.h, but
> > https://pcisig.com/membership/member-companies?combine=1055 shows
> > no
> > results, so it *looks* like EFAR/SMSC/MCHP are currently squatting
> > on
> > that ID without it being officially assigned.
> > 
> > I think MCHP needs to register 0x1055 with the PCI-SIG
> > (administration@pcisig.com) if it wants to continue using it.
> > The vendor is responsible for managing the Device ID space, so this
> > registration includes the burden of tracking all the Device IDs
> > that
> > were assigned by EFAR and SMSC and now MCHP so there are no
> > conflicts.
> > 
> > I don't want to change the existing PCI_VENDOR_ID_EFAR, and I also
> > don't want to add a PCI_VENDOR_ID_MCHP for 0x1055 until that ID has
> > been registered with the PCI-SIG.
> > 
> > So I propose that you use PCI_VENDOR_ID_EFAR for now, and if/when
> > MCHP
> > registers 0x1055 with PCI-SIG so it is unambiguously owned by MCHP,
> > we
> > can add "#define PCI_VENDOR_ID_MCHP PCI_VENDOR_ID_EFAR" or similar.
> > As Andy points out, this would be a separate logical change in its
> > own
> > patch.
> > 
> > Bjorn
> 

Re: [PATCH v2 18/19] mfd: Add support for LAN966x PCI device

[Herve Codina]

On Fri, 21 Jun 2024 17:45:05 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Thu, Jun 20, 2024 at 7:19 PM Herve Codina <herve.codina@bootlin.com> wrote:
> > On Thu, 20 Jun 2024 18:43:09 +0200
> > Herve Codina <herve.codina@bootlin.com> wrote:  
> > > On Thu, 20 Jun 2024 18:07:16 +0200
> > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:  
> > > > On Thu, Jun 20, 2024 at 5:56 PM Herve Codina <herve.codina@bootlin.com> wrote:  
> > > > > On Wed, 5 Jun 2024 23:24:43 +0300
> > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:  
> > > > > > Mon, May 27, 2024 at 06:14:45PM +0200, Herve Codina kirjoitti:  
> 
> ...
> 
> > > > > > > +   if (!dev->of_node) {
> > > > > > > +           dev_err(dev, "Missing of_node for device\n");
> > > > > > > +           return -EINVAL;
> > > > > > > +   }  
> > > > > >
> > > > > > Why do you need this? The code you have in _create_intr_ctrl() will take care
> > > > > > already for this case.  
> > > > >
> > > > > The code in _create_intr_ctrl checks for fwnode and not an of_node.
> > > > >
> > > > > The check here is to ensure that an of_node is available as it will be use
> > > > > for DT overlay loading.  
> > > >
> > > > So, what exactly do you want to check? fwnode check covers this.
> > > >  
> > > > > I will keep the check here and use dev_of_node() instead of dev->of_node.  
> > > >
> > > > It needs to be well justified as from a coding point of view this is a
> > > > duplication.  
> >
> > On DT based system, if a fwnode is set it is an of_node.
> > On ACPI, if a fwnode is set is is an acpi_node.
> >
> > The core PCI, when it successfully creates the DT node for a device
> > (CONFIG_PCI_DYNAMIC_OF_NODES) set the of_node of this device.
> > So we can have a device with:
> >  - fwnode from ACPI
> >  - of_node from core PCI creation  
> 
> Does PCI device creation not set fwnode?

No and IMHO it is correct.
This device has the fwnode that point to an ACPI node: The description used
for device creation.

The of_node set is created based on PCI known information.
This of_node, at PCI level is not used to create the PCI device but is created
based on an already existing PCI device.

> 
> > This driver needs the of_node to load the overlay.
> > Even if the core PCI cannot create a DT node for the PCI device right
> > now, I don't expect this LAN855x PCI driver updated when the core PCI
> > is able to create this PCI device DT node.  
> 
> If it's really needed, I think the correct call here is is_of_node()
> to show exactly why it's not a duplication. It also needs a comment on
> top of this call.

is_of_node() will not returns the expected result.
It will return false as the fwnode->ops of the device is not related to 
of_node ops but ACPI node ops :(

What do you thing it I keep the of_node test using dev_of_node() and add the
following comment:
	--- 8< ---
	/*
	 * On ACPI system, fwnode can point to the ACPI node.
	 * This driver needs an of_node to be used as the device-tree overlay
	 * target. This of_node should be set by the PCI core if it succeeds in
	 * creating it (CONFIG_PCI_DYNAMIC_OF_NODES feature).
	 * Check here for the validity of the of_node.
	 */
	if (!dev_of_node(dev)) {
		dev_err(dev, "Missing of_node for device\n");
		return -EINVAL;
	}
	--- 8< ---

Let me know if this can be ok.

Hervé

Re: [PATCH v2 18/19] mfd: Add support for LAN966x PCI device

[Bjorn Helgaas]

On Mon, May 27, 2024 at 06:14:45PM +0200, Herve Codina wrote:
> 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.

Several patches in this series have things like this where it appears
the last two sentences are intended to be separate paragraphs, but the
only way to tell is that the first ends with a short line, which is
annoying to read.

Perhaps either add a blank line between or rewrap the whole thing into
a single paragraph that fills 75 columns or so?

[PATCH v2 19/19] MAINTAINERS: Add the Microchip LAN966x PCI driver entry

[Herve Codina]

After contributing the driver, add myself as the maintainer for the
Microchip LAN966x PCI driver.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 MAINTAINERS | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index baeb307344cd..c84ec27ccbe4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14733,6 +14733,12 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/interrupt-controller/microchip,lan966x-oic.yaml
 F:	drivers/irqchip/irq-lan966x-oic.c
 
+MICROCHIP LAN966X PCI DRIVER
+M:	Herve Codina <herve.codina@bootlin.com>
+S:	Maintained
+F:	drivers/mfd/lan966x_pci.c
+F:	drivers/mfd/lan966x_pci.dtso
+
 MICROCHIP LCDFB DRIVER
 M:	Nicolas Ferre <nicolas.ferre@microchip.com>
 L:	linux-fbdev@vger.kernel.org
-- 
2.45.0

Re: [PATCH v2 00/19] Add support for the LAN966x PCI device using a DT overlay

[Jakub Kicinski]

On Mon, 27 May 2024 18:14:27 +0200 Herve Codina wrote:
>  - Patches 6 and 7 to be taken by network driver maintainers

Could you repost these two separately and address the nit from
Krzysztof? Easier for us to apply that way.

Re: [PATCH v2 00/19] Add support for the LAN966x PCI device using a DT overlay

[Rob Herring]

On Mon, May 27, 2024 at 06:14:27PM +0200, Herve Codina wrote:
> 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.
> 
>  - Patches 8 to 12 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 13 to 16 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 17 and 18 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 and 7 to be taken by network driver maintainers
> 
>  - Patches 8 to 12 to be taken by irqchip maintainers
> 
>  - Patch 13 to 17 to be taken by DT/PCI maintainers

I've applied patches 13-17.

Rob