[PATCH 00/16] lan966x pci device: Add support for SFPs

[PATCH 00/16] lan966x pci device: Add support for SFPs

[Herve Codina]

Hi,

This series add support for SFPs ports available on the LAN966x PCI
device. In order to have the SFPs supported, additional devices are
needed such as clock controller and I2C.

As a reminder, the LAN966x PCI device driver use a device-tree overlay
to describe devices available on the PCI board. Adding support for SFPs
ports consists in adding more devices in the already existing
device-tree overlay.

With those devices added, the device-tree overlay is more complex and
some consumer/supplier relationship are needed in order to remove
devices in correct order when the LAN966x PCI driver is removed.

Those links are typically provided by fw_devlink and we faced some
issues with fw_devlink and overlays.

This series gives the big picture related to the SFPs support from
fixing issues to adding new devices. Of course, it can be split if
needed.

The first part of the series (patch 1, 2 and 3) fixes fw_devlink when it
is used with overlay. Patches 1 and 3 were previously sent by Saravana
[0]. I just rebased them on top of v6.15-rc1 and added patch 2 in order
to take into account feedback received on the series sent by Saravana.

Those modification were not sufficient in our case and so, on top of
that, patch 4 and 5 fix some more issues related to fw_devlink.

Patches 6 and 7 are related also to fw_devlink but specific to PCI and
the device-tree nodes created during enumeration.

Patches 8, 9 and 10 are related fw_devlink too but specific to I2C
muxes. Patches purpose is to correctly set a link between an adapter
supplier and its consumer. Indeed, an i2c mux adapter's parent is not
the i2c mux supplier but the adapter the i2c mux is connected to. Adding
a new link between the adapter supplier involved when i2c muxes are used
avoid a freeze observed during device removal.

Patch 11 adds support for fw_delink on x86. fw_devlink is needed to have
the consumer/supplier relationship between devices in order to ensure a
correct device removal order. Adding fw_devlink support for x86 has been
tried in the past but was reverted [1] because it broke some systems.
Instead of disabling fw_devlink on *all* x86 system, use a finer grain
and disable it only on system which could be broken.

Patches 12 and 13 allow to build clock and i2c controller used by the
LAN966x PCI device when the LAN966x PCI device is enabled.

The next 2 patches (patches 14 and 15) update the LAN966x device-tree
overlay itself to have the SPF ports and devices they depends on
described.

The last patch (patch 16) adds new drivers in the needed driver list
available in the Kconfig help to keep this list up to date with the
devices described in the device-tree overlay.

Once again, this series gives the big picture and can be split if
needed. Let me know.

[0] https://lore.kernel.org/lkml/20240411235623.1260061-1-saravanak@google.com/
[1] https://lore.kernel.org/lkml/3c1f2473-92ad-bfc4-258e-a5a08ad73dd0@web.de/

Best regards,
Hervé

Herve Codina (14):
  driver core: Rename get_dev_from_fwnode() wrapper to
    get_device_from_fwnode()
  driver core: Avoid warning when removing a device while its supplier
    is unbinding
  bus: simple-pm-bus: Populate child nodes at probe
  PCI: of: Set fwnode.dev of newly created PCI device nodes
  PCI: of: Remove fwnode_dev_initialized() call for a PCI root bridge
    node
  i2c: core: Introduce i2c_get_adapter_supplier()
  i2c: mux: Set adapter supplier
  i2c: mux: Create missing devlink between mux and adapter supplier
  of: property: Allow fw_devlink device-tree support for x86
  clk: lan966x: Add MCHP_LAN966X_PCI dependency
  i2c: busses: at91: Add MCHP_LAN966X_PCI dependency
  misc: lan966x_pci: Fix dtso nodes ordering
  misc: lan966x_pci: Add dtso nodes in order to support SFPs
  misc: lan966x_pci: Add drivers needed to support SFPs in Kconfig help

Saravana Kannan (2):
  Revert "treewide: Fix probing of devices in DT overlays"
  of: dynamic: Fix overlayed devices not probing because of fw_devlink

 drivers/base/core.c           |  93 ++++++++++++---
 drivers/bus/imx-weim.c        |   6 -
 drivers/bus/simple-pm-bus.c   |  23 ++--
 drivers/clk/Kconfig           |   2 +-
 drivers/i2c/busses/Kconfig    |   2 +-
 drivers/i2c/i2c-core-base.c   |  16 +++
 drivers/i2c/i2c-core-of.c     |   5 -
 drivers/i2c/i2c-mux.c         |  21 ++++
 drivers/misc/Kconfig          |   5 +
 drivers/misc/lan966x_pci.dtso | 206 ++++++++++++++++++++++++++--------
 drivers/of/dynamic.c          |   1 -
 drivers/of/overlay.c          |  15 +++
 drivers/of/platform.c         |   5 -
 drivers/of/property.c         |   2 +-
 drivers/pci/of.c              |   8 +-
 drivers/spi/spi.c             |   5 -
 include/linux/fwnode.h        |   1 +
 include/linux/i2c.h           |   3 +
 18 files changed, 318 insertions(+), 101 deletions(-)

-- 
2.49.0

[PATCH 01/16] Revert "treewide: Fix probing of devices in DT overlays"

[Herve Codina]

From: Saravana Kannan <saravanak@google.com>

From: Saravana Kannan <saravanak@google.com>

This reverts commit 1a50d9403fb90cbe4dea0ec9fd0351d2ecbd8924.

While the commit fixed fw_devlink overlay handling for one case, it
broke it for another case. So revert it and redo the fix in a separate
patch.

Fixes: 1a50d9403fb9 ("treewide: Fix probing of devices in DT overlays")
Reported-by: Herve Codina <herve.codina@bootlin.com>
Closes: https://lore.kernel.org/lkml/CAMuHMdXEnSD4rRJ-o90x4OprUacN_rJgyo8x6=9F9rZ+-KzjOg@mail.gmail.com/
Closes: https://lore.kernel.org/all/20240221095137.616d2aaa@bootlin.com/
Closes: https://lore.kernel.org/lkml/20240312151835.29ef62a0@bootlin.com/
Signed-off-by: Saravana Kannan <saravanak@google.com>
Link: https://lore.kernel.org/lkml/20240411235623.1260061-2-saravanak@google.com/
Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/bus/imx-weim.c    | 6 ------
 drivers/i2c/i2c-core-of.c | 5 -----
 drivers/of/dynamic.c      | 1 -
 drivers/of/platform.c     | 5 -----
 drivers/spi/spi.c         | 5 -----
 5 files changed, 22 deletions(-)

diff --git a/drivers/bus/imx-weim.c b/drivers/bus/imx-weim.c
index 83d623d97f5f..87070155b057 100644
--- a/drivers/bus/imx-weim.c
+++ b/drivers/bus/imx-weim.c
@@ -327,12 +327,6 @@ static int of_weim_notify(struct notifier_block *nb, unsigned long action,
 				 "Failed to setup timing for '%pOF'\n", rd->dn);
 
 		if (!of_node_check_flag(rd->dn, OF_POPULATED)) {
-			/*
-			 * Clear the flag before adding the device so that
-			 * fw_devlink doesn't skip adding consumers to this
-			 * device.
-			 */
-			rd->dn->fwnode.flags &= ~FWNODE_FLAG_NOT_DEVICE;
 			if (!of_platform_device_create(rd->dn, NULL, &pdev->dev)) {
 				dev_err(&pdev->dev,
 					"Failed to create child device '%pOF'\n",
diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c
index 02feee6c9ba9..2b8d02b496fa 100644
--- a/drivers/i2c/i2c-core-of.c
+++ b/drivers/i2c/i2c-core-of.c
@@ -177,11 +177,6 @@ static int of_i2c_notify(struct notifier_block *nb, unsigned long action,
 			return NOTIFY_OK;
 		}
 
-		/*
-		 * Clear the flag before adding the device so that fw_devlink
-		 * doesn't skip adding consumers to this device.
-		 */
-		rd->dn->fwnode.flags &= ~FWNODE_FLAG_NOT_DEVICE;
 		client = of_i2c_register_device(adap, rd->dn);
 		if (IS_ERR(client)) {
 			dev_err(&adap->dev, "failed to create client for '%pOF'\n",
diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 0aba760f7577..6a117e1b6798 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -225,7 +225,6 @@ static void __of_attach_node(struct device_node *np)
 	np->sibling = np->parent->child;
 	np->parent->child = np;
 	of_node_clear_flag(np, OF_DETACHED);
-	np->fwnode.flags |= FWNODE_FLAG_NOT_DEVICE;
 
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index f77cb19973a5..ef9445ba168b 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -739,11 +739,6 @@ static int of_platform_notify(struct notifier_block *nb,
 		if (of_node_check_flag(rd->dn, OF_POPULATED))
 			return NOTIFY_OK;
 
-		/*
-		 * Clear the flag before adding the device so that fw_devlink
-		 * doesn't skip adding consumers to this device.
-		 */
-		rd->dn->fwnode.flags &= ~FWNODE_FLAG_NOT_DEVICE;
 		/* pdev_parent may be NULL when no bus platform device */
 		pdev_parent = of_find_device_by_node(parent);
 		pdev = of_platform_device_create(rd->dn, NULL,
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 90e27729ef6b..01a4e558a4fe 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -4798,11 +4798,6 @@ static int of_spi_notify(struct notifier_block *nb, unsigned long action,
 			return NOTIFY_OK;
 		}
 
-		/*
-		 * Clear the flag before adding the device so that fw_devlink
-		 * doesn't skip adding consumers to this device.
-		 */
-		rd->dn->fwnode.flags &= ~FWNODE_FLAG_NOT_DEVICE;
 		spi = of_register_spi_device(ctlr, rd->dn);
 		put_device(&ctlr->dev);
 
-- 
2.49.0

[PATCH 02/16] driver core: Rename get_dev_from_fwnode() wrapper to get_device_from_fwnode()

[Herve Codina]

get_dev_from_fwnode() calls get_device() and so it acquires a reference
on the device returned.

In order to be more obvious that this wrapper is a get_device() variant,
rename it to get_device_from_fwnode().

Suggested-by: Mark Brown <broonie@kernel.org>
Link: https://lore.kernel.org/lkml/CAGETcx97QjnjVR8Z5g0ndLHpK96hLd4aYSV=iEkKPNbNOccYmA@mail.gmail.com/
Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/base/core.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index d2f9d3a59d6b..f30260fd3031 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1881,7 +1881,7 @@ static void fw_devlink_unblock_consumers(struct device *dev)
 	device_links_write_unlock();
 }
 
-#define get_dev_from_fwnode(fwnode)	get_device((fwnode)->dev)
+#define get_device_from_fwnode(fwnode)	get_device((fwnode)->dev)
 
 static bool fwnode_init_without_drv(struct fwnode_handle *fwnode)
 {
@@ -1891,7 +1891,7 @@ static bool fwnode_init_without_drv(struct fwnode_handle *fwnode)
 	if (!(fwnode->flags & FWNODE_FLAG_INITIALIZED))
 		return false;
 
-	dev = get_dev_from_fwnode(fwnode);
+	dev = get_device_from_fwnode(fwnode);
 	ret = !dev || dev->links.status == DL_DEV_NO_DRIVER;
 	put_device(dev);
 
@@ -1960,7 +1960,7 @@ static struct device *fwnode_get_next_parent_dev(const struct fwnode_handle *fwn
 	struct device *dev;
 
 	fwnode_for_each_parent_node(fwnode, parent) {
-		dev = get_dev_from_fwnode(parent);
+		dev = get_device_from_fwnode(parent);
 		if (dev) {
 			fwnode_handle_put(parent);
 			return dev;
@@ -2016,8 +2016,8 @@ static bool __fw_devlink_relax_cycles(struct fwnode_handle *con_handle,
 		goto out;
 	}
 
-	sup_dev = get_dev_from_fwnode(sup_handle);
-	con_dev = get_dev_from_fwnode(con_handle);
+	sup_dev = get_device_from_fwnode(sup_handle);
+	con_dev = get_device_from_fwnode(con_handle);
 	/*
 	 * If sup_dev is bound to a driver and @con hasn't started binding to a
 	 * driver, sup_dev can't be a consumer of @con. So, no need to check
@@ -2156,7 +2156,7 @@ static int fw_devlink_create_devlink(struct device *con,
 	if (sup_handle->flags & FWNODE_FLAG_NOT_DEVICE)
 		sup_dev = fwnode_get_next_parent_dev(sup_handle);
 	else
-		sup_dev = get_dev_from_fwnode(sup_handle);
+		sup_dev = get_device_from_fwnode(sup_handle);
 
 	if (sup_dev) {
 		/*
@@ -2225,7 +2225,7 @@ static void __fw_devlink_link_to_consumers(struct device *dev)
 		bool own_link = true;
 		int ret;
 
-		con_dev = get_dev_from_fwnode(link->consumer);
+		con_dev = get_device_from_fwnode(link->consumer);
 		/*
 		 * If consumer device is not available yet, make a "proxy"
 		 * SYNC_STATE_ONLY link from the consumer's parent device to
-- 
2.49.0

[PATCH 03/16] of: dynamic: Fix overlayed devices not probing because of fw_devlink

[Herve Codina]

From: Saravana Kannan <saravanak@google.com>

From: Saravana Kannan <saravanak@google.com>

When an overlay is applied, if the target device has already probed
successfully and bound to a device, then some of the fw_devlink logic
that ran when the device was probed needs to be rerun. This allows newly
created dangling consumers of the overlayed device tree nodes to be
moved to become consumers of the target device.

Fixes: 1a50d9403fb9 ("treewide: Fix probing of devices in DT overlays")
Reported-by: Herve Codina <herve.codina@bootlin.com>
Closes: https://lore.kernel.org/lkml/CAMuHMdXEnSD4rRJ-o90x4OprUacN_rJgyo8x6=9F9rZ+-KzjOg@mail.gmail.com/
Closes: https://lore.kernel.org/all/20240221095137.616d2aaa@bootlin.com/
Closes: https://lore.kernel.org/lkml/20240312151835.29ef62a0@bootlin.com/
Signed-off-by: Saravana Kannan <saravanak@google.com>
Link: https://lore.kernel.org/lkml/20240411235623.1260061-3-saravanak@google.com/
[Herve: Rebase on top of recent kernel and use get_device_from_fwnode()]
Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/base/core.c    | 78 ++++++++++++++++++++++++++++++++++++------
 drivers/of/overlay.c   | 15 ++++++++
 include/linux/fwnode.h |  1 +
 3 files changed, 83 insertions(+), 11 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index f30260fd3031..5d6687f38d00 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -47,6 +47,8 @@ static bool fw_devlink_drv_reg_done;
 static bool fw_devlink_best_effort;
 static struct workqueue_struct *device_link_wq;
 
+#define get_device_from_fwnode(fwnode)	get_device((fwnode)->dev)
+
 /**
  * __fwnode_link_add - Create a link between two fwnode_handles.
  * @con: Consumer end of the link.
@@ -235,6 +237,70 @@ static void __fw_devlink_pickup_dangling_consumers(struct fwnode_handle *fwnode,
 		__fw_devlink_pickup_dangling_consumers(child, new_sup);
 }
 
+static void fw_devlink_pickup_dangling_consumers(struct device *dev)
+{
+	struct fwnode_handle *child;
+
+	guard(mutex)(&fwnode_link_lock);
+
+	fwnode_for_each_available_child_node(dev->fwnode, child)
+		__fw_devlink_pickup_dangling_consumers(child, dev->fwnode);
+	__fw_devlink_link_to_consumers(dev);
+}
+
+/**
+ * fw_devlink_refresh_fwnode - Recheck the tree under this firmware node
+ * @fwnode: The fwnode under which the fwnode tree has changed
+ *
+ * This function is mainly meant to adjust the supplier/consumer dependencies
+ * after a fwnode tree overlay has occurred.
+ */
+void fw_devlink_refresh_fwnode(struct fwnode_handle *fwnode)
+{
+	struct device *dev;
+
+	/*
+	 * Find the closest ancestor fwnode that has been converted to a device
+	 * that can bind to a driver (bus device).
+	 */
+	fwnode_handle_get(fwnode);
+	do {
+		if (fwnode->flags & FWNODE_FLAG_NOT_DEVICE)
+			continue;
+
+		dev = get_device_from_fwnode(fwnode);
+		if (!dev)
+			continue;
+
+		if (dev->bus)
+			break;
+
+		put_device(dev);
+	} while ((fwnode = fwnode_get_next_parent(fwnode)));
+
+	/*
+	 * If none of the ancestor fwnodes have (yet) been converted to a device
+	 * that can bind to a driver, there's nothing to fix up.
+	 */
+	if (!fwnode)
+		return;
+
+	WARN(device_is_bound(dev) && dev->links.status != DL_DEV_DRIVER_BOUND,
+	     "Don't multithread overlaying and probing the same device!\n");
+
+	/*
+	 * If the device has already bound to a driver, then we need to redo
+	 * some of the work that was done after the device was bound to a
+	 * driver. If the device hasn't bound to a driver, running thing too
+	 * soon would incorrectly pick up consumers that it shouldn't.
+	 */
+	if (dev->links.status == DL_DEV_DRIVER_BOUND)
+		fw_devlink_pickup_dangling_consumers(dev);
+
+	put_device(dev);
+	fwnode_handle_put(fwnode);
+}
+
 static DEFINE_MUTEX(device_links_lock);
 DEFINE_STATIC_SRCU(device_links_srcu);
 
@@ -1313,16 +1379,8 @@ void device_links_driver_bound(struct device *dev)
 	 * child firmware node.
 	 */
 	if (dev->fwnode && dev->fwnode->dev == dev) {
-		struct fwnode_handle *child;
-
 		fwnode_links_purge_suppliers(dev->fwnode);
-
-		guard(mutex)(&fwnode_link_lock);
-
-		fwnode_for_each_available_child_node(dev->fwnode, child)
-			__fw_devlink_pickup_dangling_consumers(child,
-							       dev->fwnode);
-		__fw_devlink_link_to_consumers(dev);
+		fw_devlink_pickup_dangling_consumers(dev);
 	}
 	device_remove_file(dev, &dev_attr_waiting_for_supplier);
 
@@ -1881,8 +1939,6 @@ static void fw_devlink_unblock_consumers(struct device *dev)
 	device_links_write_unlock();
 }
 
-#define get_device_from_fwnode(fwnode)	get_device((fwnode)->dev)
-
 static bool fwnode_init_without_drv(struct fwnode_handle *fwnode)
 {
 	struct device *dev;
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 1af6f52d0708..6bd93153d695 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -185,6 +185,15 @@ static int overlay_notify(struct overlay_changeset *ovcs,
 	return 0;
 }
 
+static void overlay_fw_devlink_refresh(struct overlay_changeset *ovcs)
+{
+	for (int i = 0; i < ovcs->count; i++) {
+		struct device_node *np = ovcs->fragments[i].target;
+
+		fw_devlink_refresh_fwnode(of_fwnode_handle(np));
+	}
+}
+
 /*
  * The values of properties in the "/__symbols__" node are paths in
  * the ovcs->overlay_root.  When duplicating the properties, the paths
@@ -951,6 +960,12 @@ static int of_overlay_apply(struct overlay_changeset *ovcs,
 		pr_err("overlay apply changeset entry notify error %d\n", ret);
 	/* notify failure is not fatal, continue */
 
+	/*
+	 * Needs to happen after changeset notify to give the listeners a chance
+	 * to finish creating all the devices they need to create.
+	 */
+	overlay_fw_devlink_refresh(ovcs);
+
 	ret_tmp = overlay_notify(ovcs, OF_OVERLAY_POST_APPLY);
 	if (ret_tmp)
 		if (!ret)
diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
index 6fa0a268d538..af76de93bee2 100644
--- a/include/linux/fwnode.h
+++ b/include/linux/fwnode.h
@@ -223,6 +223,7 @@ int fwnode_link_add(struct fwnode_handle *con, struct fwnode_handle *sup,
 		    u8 flags);
 void fwnode_links_purge(struct fwnode_handle *fwnode);
 void fw_devlink_purge_absent_suppliers(struct fwnode_handle *fwnode);
+void fw_devlink_refresh_fwnode(struct fwnode_handle *fwnode);
 bool fw_devlink_is_strict(void);
 
 #endif
-- 
2.49.0

[PATCH 04/16] driver core: Avoid warning when removing a device while its supplier is unbinding

[Herve Codina]

During driver removal, the following warning can appear:
   WARNING: CPU: 1 PID: 139 at drivers/base/core.c:1497 __device_links_no_driver+0xcc/0xfc
   ...
   Call trace:
     __device_links_no_driver+0xcc/0xfc (P)
     device_links_driver_cleanup+0xa8/0xf0
     device_release_driver_internal+0x208/0x23c
     device_links_unbind_consumers+0xe0/0x108
     device_release_driver_internal+0xec/0x23c
     device_links_unbind_consumers+0xe0/0x108
     device_release_driver_internal+0xec/0x23c
     device_links_unbind_consumers+0xe0/0x108
     device_release_driver_internal+0xec/0x23c
     driver_detach+0xa0/0x12c
     bus_remove_driver+0x6c/0xbc
     driver_unregister+0x30/0x60
     pci_unregister_driver+0x20/0x9c
     lan966x_pci_driver_exit+0x18/0xa90 [lan966x_pci]

This warning is triggered when a consumer is removed because the links
status of its supplier is not DL_DEV_DRIVER_BOUND and the link flag
DL_FLAG_SYNC_STATE_ONLY is not set.

The topology in terms of consumers/suppliers used was the following
(consumer ---> supplier):

      i2c -----------> OIC ----> PCI device
       |                ^
       |                |
       +---> pinctrl ---+

When the PCI device is removed, the OIC (interrupt controller) has to be
removed. In order to remove the OIC, pinctrl and i2c need to be removed
and to remove pinctrl, i2c need to be removed. The removal order is:
  1) i2c
  2) pinctrl
  3) OIC
  4) PCI device

In details, the removal sequence is the following (with 0000:01:00.0 the
PCI device):
  driver_detach: call device_release_driver_internal(0000:01:00.0)...
    device_links_busy(0000:01:00.0):
      links->status = DL_DEV_UNBINDING
    device_links_unbind_consumers(0000:01:00.0):
      0000:01:00.0--oic link->status = DL_STATE_SUPPLIER_UNBIND
      call device_release_driver_internal(oic)...
        device_links_busy(oic):
          links->status = DL_DEV_UNBINDING
        device_links_unbind_consumers(oic):
          oic--pinctrl link->status = DL_STATE_SUPPLIER_UNBIND
          call device_release_driver_internal(pinctrl)...
            device_links_busy(pinctrl):
              links->status = DL_DEV_UNBINDING
            device_links_unbind_consumers(pinctrl):
              pinctrl--i2c link->status = DL_STATE_SUPPLIER_UNBIND
              call device_release_driver_internal(i2c)...
                device_links_busy(i2c): links->status = DL_DEV_UNBINDING
                __device_links_no_driver(i2c)...
                  pinctrl--i2c link->status is DL_STATE_SUPPLIER_UNBIND
                  oic--i2c link->status is DL_STATE_ACTIVE
                  oic--i2c link->supplier->links.status is DL_DEV_UNBINDING

The warning is triggered by the i2c removal because the OIC (supplier)
links status is not DL_DEV_DRIVER_BOUND. Its links status is indeed set
to DL_DEV_UNBINDING.

It is perfectly legit to have the links status set to DL_DEV_UNBINDING
in that case. Indeed we had started to unbind the OIC which triggered
the consumer unbinding and didn't finish yet when the i2c is unbound.

Avoid the warning when the supplier links status is set to
DL_DEV_UNBINDING and thus support this removal sequence without any
warnings.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/base/core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 5d6687f38d00..b44f9d371d47 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1494,7 +1494,8 @@ static void __device_links_no_driver(struct device *dev)
 		if (link->supplier->links.status == DL_DEV_DRIVER_BOUND) {
 			WRITE_ONCE(link->status, DL_STATE_AVAILABLE);
 		} else {
-			WARN_ON(!(link->flags & DL_FLAG_SYNC_STATE_ONLY));
+			if (link->supplier->links.status != DL_DEV_UNBINDING)
+				WARN_ON(!(link->flags & DL_FLAG_SYNC_STATE_ONLY));
 			WRITE_ONCE(link->status, DL_STATE_DORMANT);
 		}
 	}
-- 
2.49.0

[PATCH 05/16] bus: simple-pm-bus: Populate child nodes at probe

[Herve Codina]

The simple-pm-bus drivers handles several simple bus. When it is used
with busses other than a compatible "simple-pm-bus", it don't populate
its child devices during its probe.

This confuses fw_devlink and results in wrong or missing devlinks.

Once a driver is bound to a device and the probe() has been called,
device_links_driver_bound() is called.

This function performs operation based on the following assumption:
    If a child firmware node of the bound device is not added as a
    device, it will never be added.

Among operations done on fw_devlinks of those "never be added" devices,
device_links_driver_bound() changes their supplier.

With devices attached to a simple-bus compatible device, this change
leads to wrong devlinks where supplier of devices points to the device
parent (i.e. simple-bus compatible device) instead of the device itself
(i.e. simple-bus child).

When the device attached to the simple-bus is removed, because devlinks
are not correct, its consumers are not removed first.

In order to have correct devlinks created, make the simple-pm-bus driver
compliant with the devlink assumption and create its child devices
during its probe.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/bus/simple-pm-bus.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/bus/simple-pm-bus.c b/drivers/bus/simple-pm-bus.c
index d8e029e7e53f..93c6ba605d7a 100644
--- a/drivers/bus/simple-pm-bus.c
+++ b/drivers/bus/simple-pm-bus.c
@@ -42,14 +42,14 @@ static int simple_pm_bus_probe(struct platform_device *pdev)
 	match = of_match_device(dev->driver->of_match_table, dev);
 	/*
 	 * These are transparent bus devices (not simple-pm-bus matches) that
-	 * have their child nodes populated automatically.  So, don't need to
-	 * do anything more. We only match with the device if this driver is
-	 * the most specific match because we don't want to incorrectly bind to
-	 * a device that has a more specific driver.
+	 * have their child nodes populated automatically. So, don't need to
+	 * do anything more except populate child nodes. We only match with the
+	 * device if this driver is the most specific match because we don't
+	 * want to incorrectly bind to a device that has a more specific driver.
 	 */
 	if (match && match->data) {
 		if (of_property_match_string(np, "compatible", match->compatible) == 0)
-			return 0;
+			goto populate;
 		else
 			return -ENODEV;
 	}
@@ -64,13 +64,14 @@ static int simple_pm_bus_probe(struct platform_device *pdev)
 
 	dev_set_drvdata(&pdev->dev, bus);
 
-	dev_dbg(&pdev->dev, "%s\n", __func__);
-
 	pm_runtime_enable(&pdev->dev);
 
+populate:
 	if (np)
 		of_platform_populate(np, NULL, lookup, &pdev->dev);
 
+	dev_dbg(&pdev->dev, "%s\n", __func__);
+
 	return 0;
 }
 
@@ -78,12 +79,16 @@ static void simple_pm_bus_remove(struct platform_device *pdev)
 {
 	const void *data = of_device_get_match_data(&pdev->dev);
 
-	if (pdev->driver_override || data)
+	if (pdev->driver_override)
 		return;
 
 	dev_dbg(&pdev->dev, "%s\n", __func__);
 
-	pm_runtime_disable(&pdev->dev);
+	if (pdev->dev.of_node)
+		of_platform_depopulate(&pdev->dev);
+
+	if (!data)
+		pm_runtime_disable(&pdev->dev);
 }
 
 static int simple_pm_bus_runtime_suspend(struct device *dev)
-- 
2.49.0

[PATCH 06/16] PCI: of: Set fwnode.dev of newly created PCI device nodes

[Herve Codina]

Device-tree node can be created when CONFIG_PCI_DYNAMIC_OF_NODES. Those
node are created and filled based on PCI core information but the
fwnode.dev field is not set.

When later an overlay is applied, this consuses fw_devlink. Indeed,
without any device attached to the node, fw_devlink considers that this
node will never become a device. When this node is pointed as a
supplier, devlink looks at its ancestors in order to find a node with a
device that could be used as the supplier.

In the PCI use case, this leads to links that wrongly use the PCI root
bridge device as the supplier instead of the expected PCI device.

Setting fwnode.dev to the dev of the PCI device allows devlink to use
this device as a supplier and so, correct links are created.

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

diff --git a/drivers/pci/of.c b/drivers/pci/of.c
index ab7a8252bf41..ac6c4e1d68e5 100644
--- a/drivers/pci/of.c
+++ b/drivers/pci/of.c
@@ -709,6 +709,13 @@ void of_pci_make_dev_node(struct pci_dev *pdev)
 	if (ret)
 		goto out_free_node;
 
+	/*
+	 * Set the fwnode.dev in order to have fw_devlink creating links
+	 * pointing to this PCI device instead of walking up to the PCI host
+	 * bridge.
+	 */
+	np->fwnode.dev = &pdev->dev;
+
 	ret = of_changeset_apply(cset);
 	if (ret)
 		goto out_free_node;
-- 
2.49.0

[PATCH 07/16] PCI: of: Remove fwnode_dev_initialized() call for a PCI root bridge node

[Herve Codina]

During the instantiation of devices described by a device-tree overlay
applied on a PCI device, devlink displays the following kind of debug
messages instead of creating the expected links:
   'Not linking xxxx - might never become dev'

Without those expected links, the device removal order cannot be
correct.

Those debug traces are printed by fw_devlink_create_devlink(). In our
use case, they are all printed because the supplier of the link has at
least one of its ancestor with its fwnode flag FWNODE_FLAG_INITIALIZED
set.

The culprit ancestor is the PCI root bridge.

The fwnode related to the PCI root bridge is created dynamically by the
of_pci_make_host_bridge_node() function. During this creation
fwnode_dev_initialized() is called which set the FWNODE_FLAG_INITIALIZED
flag.

Calling fwnode_dev_initialized() tells devlink that the device related
to this node is handled out of the driver core. This is not correct in
our case. Indeed the device related to this firmware node is handled
using driver core mechanisms and is fully compliant devlink
expectations.

Simply remove the fwnode_dev_initialized() call. With that done, the
devlink debug messages are no more displayed and links that were missing
are correctly created.

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

diff --git a/drivers/pci/of.c b/drivers/pci/of.c
index ac6c4e1d68e5..5ea55a6416c0 100644
--- a/drivers/pci/of.c
+++ b/drivers/pci/of.c
@@ -811,7 +811,6 @@ void of_pci_make_host_bridge_node(struct pci_host_bridge *bridge)
 	 */
 	of_node_set_flag(np, OF_POPULATED);
 	np->fwnode.dev = &bridge->dev;
-	fwnode_dev_initialized(&np->fwnode, true);
 
 	ret = of_changeset_apply(cset);
 	if (ret)
-- 
2.49.0

[PATCH 08/16] i2c: core: Introduce i2c_get_adapter_supplier()

[Herve Codina]

The supplier device of an I2C adapter is the device that calls
i2c_add_adapter() or variants and i2c_del_adapter().

Most of the time this supplier device is the parent of the adapter dev.

Exceptions exist with i2c muxes. Indeed, in case of i2c muxes, the
parent of the adapter dev points to the adapter dev the mux is connected
to instead of the supplier of this adapter.

Introduce i2c_get_adapter_supplier() and a new supplier field in the
adapter structure in order to ease the adapter supplier retrieval.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/i2c/i2c-core-base.c | 16 ++++++++++++++++
 include/linux/i2c.h         |  3 +++
 2 files changed, 19 insertions(+)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 7ad1ad5c8c3f..e3eeac0b2b49 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1917,6 +1917,22 @@ struct i2c_adapter *i2c_get_adapter_by_fwnode(struct fwnode_handle *fwnode)
 }
 EXPORT_SYMBOL(i2c_get_adapter_by_fwnode);
 
+/**
+ * i2c_get_adapter_supplier() - Get the supplier of an adapter
+ * @adapter: the adapter to get the supplier from
+ *
+ * return:
+ * Look up and return the &struct device corresponding to the device supplying
+ * this adapter.
+ *
+ * The user must call put_device() once done with the supplier returned.
+ */
+struct device *i2c_get_adapter_supplier(struct i2c_adapter *adapter)
+{
+	return get_device(adapter->supplier ?: adapter->dev.parent);
+}
+EXPORT_SYMBOL(i2c_get_adapter_supplier);
+
 static void i2c_parse_timing(struct device *dev, char *prop_name, u32 *cur_val_p,
 			    u32 def_val, bool use_def)
 {
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 2e4903b7f7bc..04b85703bcd6 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -746,6 +746,7 @@ struct i2c_adapter {
 	int timeout;			/* in jiffies */
 	int retries;
 	struct device dev;		/* the adapter device */
+	struct device *supplier;	/* the device that supply this adapter */
 	unsigned long locked_flags;	/* owned by the I2C core */
 #define I2C_ALF_IS_SUSPENDED		0
 #define I2C_ALF_SUSPEND_REPORTED	1
@@ -913,6 +914,8 @@ struct i2c_adapter *i2c_get_adapter(int nr);
 void i2c_put_adapter(struct i2c_adapter *adap);
 unsigned int i2c_adapter_depth(struct i2c_adapter *adapter);
 
+struct device *i2c_get_adapter_supplier(struct i2c_adapter *adap);
+
 void i2c_parse_fw_timings(struct device *dev, struct i2c_timings *t, bool use_defaults);
 
 /* Return the functionality mask */
-- 
2.49.0

[PATCH 09/16] i2c: mux: Set adapter supplier

[Herve Codina]

For i2c muxes, the parent of the mux adapter dev is the adapter dev the
mux is connected to.

This parent is not the supplier of the mux adapter. Indeed, the supplier
of the mux adapter is the mux device itself.

Fill the adap.supplier with the mux device.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/i2c/i2c-mux.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
index fda72e8be885..d8bdb3b40acf 100644
--- a/drivers/i2c/i2c-mux.c
+++ b/drivers/i2c/i2c-mux.c
@@ -318,6 +318,7 @@ int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
 	priv->adap.algo = &priv->algo;
 	priv->adap.algo_data = priv;
 	priv->adap.dev.parent = &parent->dev;
+	priv->adap.supplier = muxc->dev;
 	priv->adap.retries = parent->retries;
 	priv->adap.timeout = parent->timeout;
 	priv->adap.quirks = parent->quirks;
-- 
2.49.0

[PATCH 10/16] i2c: mux: Create missing devlink between mux and adapter supplier

[Herve Codina]

When removing an i2c controller device handling an i2c bus where an i2c
mux is connected to, the removal process hangs and is stuck in the
wait_completion() call done in i2c_del_adapter().

The i2c_del_adapter() tries to removed the i2c adapter related to the
i2c controller device and the wait_completion() is waiting for the i2c
adapter dev release. This release is performed when the device is no
more used (i.e. refcount reaches zero).

When an i2c mux is involved in an i2c path, the struct dev topology is
the following:
    +----------------+                +-------------------+
    | i2c controller |                |      i2c mux      |
    |     device     |                |      device       |
    |       ^        |                |                   |
    |       |        |                |                   |
    |  dev's parent  |                |                   |
    |       |        |                |                   |
    |   i2c adapter  |                | i2c adapter chanX |
    |     device  <---- dev's parent ------  device       |
    |   (no driver)  |                |    (no driver)    |
    +----------------+                +-------------------+

When an i2c mux device creates an i2c adapter for its downstream
channel, a reference is taken its adapter dev's parent. This parent is
the i2c mux upstream adapter device.

No relationship exists between the i2c mux device itself and the i2c
controller device in order to have the i2c mux device calling
i2c_del_adapter() to remove its downtream adapters and so, release
references taken to the upstream adapter.

This consumer/supplier relationship is typically a devlink relationship.

Also, i2c muxes can be chained and so, the upstream adapter can be
supplied by either an i2c controller device or an other i2c mux device.

In order to get the supplier of the adapter a mux is connected to,
rely on the newly introduced i2c_adapter_get_supplier() and create the
missing devlink between the i2c mux device and the supplier of the
adapter the mux is connected to.

With that done, the i2c mux device is removed before the device
handling the upstream i2c adapter (i2c controller device or i2c mux
device). All references are released and the i2c_del_adapter() call
performed by driver handling the upstream adapter device is not blocking
anymore.

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

diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
index d8bdb3b40acf..84ffd58e4d6d 100644
--- a/drivers/i2c/i2c-mux.c
+++ b/drivers/i2c/i2c-mux.c
@@ -271,7 +271,9 @@ int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
 			u32 force_nr, u32 chan_id)
 {
 	struct i2c_adapter *parent = muxc->parent;
+	struct device *parent_supplier;
 	struct i2c_mux_priv *priv;
+	struct device_link *dl;
 	char symlink_name[20];
 	int ret;
 
@@ -378,6 +380,24 @@ int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
 				      ACPI_COMPANION(muxc->dev),
 				      chan_id);
 
+	/*
+	 * There is no relationship set between the mux dev and the device
+	 * handling the parent adapter. Create this missing relationship in
+	 * order to remove the i2c mux device (consumer) and so the dowstream
+	 * channel adapters before removing the device (supplier) which handles
+	 * the i2c mux upstream adapter.
+	 */
+	parent_supplier = i2c_get_adapter_supplier(parent);
+	dl = device_link_add(muxc->dev, parent_supplier, DL_FLAG_AUTOREMOVE_CONSUMER);
+	if (!dl) {
+		dev_err(muxc->dev, "failed to create device link to %s\n",
+			dev_name(parent_supplier));
+		put_device(parent_supplier);
+		ret = -EINVAL;
+		goto err_free_priv;
+	}
+	put_device(parent_supplier);
+
 	if (force_nr) {
 		priv->adap.nr = force_nr;
 		ret = i2c_add_numbered_adapter(&priv->adap);
-- 
2.49.0

[PATCH 11/16] of: property: Allow fw_devlink device-tree support for x86

[Herve Codina]

PCI drivers can use a device-tree overlay to describe the hardware
available on the PCI board. This is the case, for instance, of the
LAN966x PCI device driver.

Adding some more nodes in the device-tree overlay adds some more
consumer/supplier relationship between devices instantiated from this
overlay.

Those fw_node consumer/supplier relationships are handled by fw_devlink
and are created based on the device-tree parsing done by the
of_fwnode_add_links() function.

Those consumer/supplier links are needed in order to ensure a correct PM
runtime management and a correct removal order between devices.

For instance, without those links a supplier can be removed before its
consumers is removed leading to all kind of issue if this consumer still
want the use the already removed supplier.

The support for the usage of an overlay from a PCI driver has been added
on x86 systems in commit 1f340724419ed ("PCI: of: Create device tree PCI
host bridge node").

In the past, support for fw_devlink on x86 had been tried but this
support has been removed in commit 4a48b66b3f52 ("of: property: Disable
fw_devlink DT support for X86"). Indeed, this support was breaking some
x86 systems such as OLPC system and the regression was reported in [0].

Instead of disabling this support for all x86 system, use a finer grain
and disable this support only for the possible problematic subset of x86
system mixing ACPI and device-tree at boot time (i.e. OLPC and CE4100).

[0] https://lore.kernel.org/lkml/3c1f2473-92ad-bfc4-258e-a5a08ad73dd0@web.de/

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/of/property.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/of/property.c b/drivers/of/property.c
index c1feb631e383..a4b367d056b8 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1605,7 +1605,7 @@ static int of_fwnode_add_links(struct fwnode_handle *fwnode)
 	const struct property *p;
 	struct device_node *con_np = to_of_node(fwnode);
 
-	if (IS_ENABLED(CONFIG_X86))
+	if (IS_ENABLED(CONFIG_X86_INTEL_CE) || IS_ENABLED(CONFIG_OLPC))
 		return 0;
 
 	if (!con_np)
-- 
2.49.0

[PATCH 12/16] clk: lan966x: Add MCHP_LAN966X_PCI dependency

[Herve Codina]

The lan966x clock controller depends on the LAN969x architecture or the
LAN966x SoC.

This clock controller can be used by the LAN966x PCI device and so it
needs to be available when the LAN966x PCI device is enabled.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/clk/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 713573b6c86c..de4fe180cd4b 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -270,7 +270,7 @@ config COMMON_CLK_LAN966X
 	tristate "Generic Clock Controller driver for LAN966X SoC"
 	depends on HAS_IOMEM
 	depends on OF
-	depends on SOC_LAN966 || ARCH_LAN969X || COMPILE_TEST
+	depends on SOC_LAN966 || ARCH_LAN969X || MCHP_LAN966X_PCI || COMPILE_TEST
 	help
 	  This driver provides support for Generic Clock Controller(GCK) on
 	  LAN966X SoC. GCK generates and supplies clock to various peripherals
-- 
2.49.0

[PATCH 13/16] i2c: busses: at91: Add MCHP_LAN966X_PCI dependency

[Herve Codina]

The AT91 I2C driver depends on ARCH_AT91.

This I2C controller can be used by the LAN966x PCI device and so
it needs to be available when the LAN966x PCI device is enabled.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/i2c/busses/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 83c88c79afe2..148f9f66d5f3 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -414,7 +414,7 @@ config I2C_ASPEED
 
 config I2C_AT91
 	tristate "Atmel AT91 I2C Two-Wire interface (TWI)"
-	depends on ARCH_AT91 || COMPILE_TEST
+	depends on ARCH_AT91 || MCHP_LAN966X_PCI || COMPILE_TEST
 	help
 	  This supports the use of the I2C interface on Atmel AT91
 	  processors.
-- 
2.49.0

[PATCH 14/16] misc: lan966x_pci: Fix dtso nodes ordering

[Herve Codina]

Nodes available in the dtso are not ordered by their unit address.

Fix that re-ordering them according to their unit address.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/misc/lan966x_pci.dtso | 99 +++++++++++++++++------------------
 1 file changed, 49 insertions(+), 50 deletions(-)

diff --git a/drivers/misc/lan966x_pci.dtso b/drivers/misc/lan966x_pci.dtso
index 7b196b0a0eb6..94a967b384f3 100644
--- a/drivers/misc/lan966x_pci.dtso
+++ b/drivers/misc/lan966x_pci.dtso
@@ -59,6 +59,50 @@ pci-ep-bus@0 {
 				ranges = <0xe2000000 0x00 0x00 0x00 0x2000000
 				          0xe0000000 0x01 0x00 0x00 0x1000000>;
 
+				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)>;
+						};
+					};
+				};
+
+				cpu_ctrl: syscon@e00c0000 {
+					compatible = "microchip,lan966x-cpu-syscon", "syscon";
+					reg = <0xe00c0000 0xa8>;
+				};
+
 				oic: oic@e00c0120 {
 					compatible = "microchip,lan966x-oic";
 					#interrupt-cells = <2>;
@@ -67,11 +111,6 @@ oic: oic@e00c0120 {
 					reg = <0xe00c0120 0x190>;
 				};
 
-				cpu_ctrl: syscon@e00c0000 {
-					compatible = "microchip,lan966x-cpu-syscon", "syscon";
-					reg = <0xe00c0000 0xa8>;
-				};
-
 				reset: reset@e200400c {
 					compatible = "microchip,lan966x-switch-reset";
 					reg = <0xe200400c 0x4>, <0xe00c0000 0xa8>;
@@ -104,14 +143,6 @@ fc0_a_pins: fcb4-i2c-pins {
 						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 {
@@ -133,43 +164,11 @@ lan966x_phy1: ethernet-lan966x_phy@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)>;
-						};
-					};
+				serdes: serdes@e202c000 {
+					compatible = "microchip,lan966x-serdes";
+					reg = <0xe202c000 0x9c>,
+					      <0xe2004010 0x4>;
+					#phy-cells = <2>;
 				};
 			};
 		};
-- 
2.49.0

[PATCH 15/16] misc: lan966x_pci: Add dtso nodes in order to support SFPs

[Herve Codina]

Add device-tree nodes needed to support SFPs.
Those nodes are:
 - the clock controller
 - the i2c controller
 - the i2c mux
 - the SFPs themselves and their related ports in the switch

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/misc/lan966x_pci.dtso | 111 ++++++++++++++++++++++++++++++++++
 1 file changed, 111 insertions(+)

diff --git a/drivers/misc/lan966x_pci.dtso b/drivers/misc/lan966x_pci.dtso
index 94a967b384f3..a2015b46cd44 100644
--- a/drivers/misc/lan966x_pci.dtso
+++ b/drivers/misc/lan966x_pci.dtso
@@ -47,6 +47,47 @@ sys_clk: clock-15625000 {
 				clock-frequency = <15625000>;  /* System clock = 15.625MHz */
 			};
 
+			i2c0_emux: i2c0-emux {
+				compatible = "i2c-mux-pinctrl";
+				#address-cells = <1>;
+				#size-cells = <0>;
+				i2c-parent = <&i2c0>;
+				pinctrl-names = "i2c102", "i2c103", "idle";
+				pinctrl-0 = <&i2cmux_0>;
+				pinctrl-1 = <&i2cmux_1>;
+				pinctrl-2 = <&i2cmux_pins>;
+
+				i2c102: i2c@0 {
+					reg = <0>;
+					#address-cells = <1>;
+					#size-cells = <0>;
+				};
+
+				i2c103: i2c@1 {
+					reg = <1>;
+					#address-cells = <1>;
+					#size-cells = <0>;
+				};
+			};
+
+			sfp2: sfp2 {
+				compatible = "sff,sfp";
+				i2c-bus = <&i2c102>;
+				tx-disable-gpios = <&gpio 0 GPIO_ACTIVE_HIGH>;
+				los-gpios = <&gpio 25 GPIO_ACTIVE_HIGH>;
+				mod-def0-gpios = <&gpio 18 GPIO_ACTIVE_LOW>;
+				tx-fault-gpios = <&gpio 2 GPIO_ACTIVE_HIGH>;
+			};
+
+			sfp3: sfp3 {
+				compatible = "sff,sfp";
+				i2c-bus = <&i2c103>;
+				tx-disable-gpios = <&gpio 1 GPIO_ACTIVE_HIGH>;
+				los-gpios = <&gpio 26 GPIO_ACTIVE_HIGH>;
+				mod-def0-gpios = <&gpio 19 GPIO_ACTIVE_LOW>;
+				tx-fault-gpios = <&gpio 3 GPIO_ACTIVE_HIGH>;
+			};
+
 			pci-ep-bus@0 {
 				compatible = "simple-bus";
 				#address-cells = <1>;
@@ -95,6 +136,50 @@ port1: port@1 {
 							phy-mode = "gmii";
 							phys = <&serdes 1 CU(1)>;
 						};
+
+						port2: port@2 {
+							reg = <2>;
+							phy-mode = "sgmii";
+							phys = <&serdes 2 SERDES6G(0)>;
+							sfp = <&sfp2>;
+							managed = "in-band-status";
+						};
+
+						port3: port@3 {
+							reg = <3>;
+							phy-mode = "sgmii";
+							phys = <&serdes 3 SERDES6G(1)>;
+							sfp = <&sfp3>;
+							managed = "in-band-status";
+						};
+					};
+				};
+
+				flx0: flexcom@e0040000 {
+					compatible = "atmel,sama5d2-flexcom";
+					reg = <0xe0040000 0x100>;
+					clocks = <&clks GCK_ID_FLEXCOM0>;
+					#address-cells = <1>;
+					#size-cells = <1>;
+					ranges = <0x0 0xe0040000 0x800>;
+
+					atmel,flexcom-mode = <ATMEL_FLEXCOM_MODE_TWI>;
+
+					i2c0: i2c@600 {
+						compatible = "microchip,sam9x60-i2c";
+						reg = <0x600 0x200>;
+						interrupt-parent = <&oic>;
+						interrupts = <48 IRQ_TYPE_LEVEL_HIGH>;
+						#address-cells = <1>;
+						#size-cells = <0>;
+						clocks = <&clks GCK_ID_FLEXCOM0>;
+						assigned-clocks = <&clks GCK_ID_FLEXCOM0>;
+						assigned-clock-rates = <20000000>;
+						pinctrl-0 = <&fc0_a_pins>;
+						pinctrl-names = "default";
+						i2c-analog-filter;
+						i2c-digital-filter;
+						i2c-digital-filter-width-ns = <35>;
 					};
 				};
 
@@ -103,6 +188,14 @@ cpu_ctrl: syscon@e00c0000 {
 					reg = <0xe00c0000 0xa8>;
 				};
 
+				clks: clock-controller@e00c00a8 {
+					compatible = "microchip,lan966x-gck";
+					#clock-cells = <1>;
+					clocks = <&cpu_clk>, <&ddr_clk>, <&sys_clk>;
+					clock-names = "cpu", "ddr", "sys";
+					reg = <0xe00c00a8 0x38>, <0xe00c02cc 0x4>;
+				};
+
 				oic: oic@e00c0120 {
 					compatible = "microchip,lan966x-oic";
 					#interrupt-cells = <2>;
@@ -143,6 +236,24 @@ fc0_a_pins: fcb4-i2c-pins {
 						pins = "GPIO_9", "GPIO_10";
 						function = "fc0_a";
 					};
+
+					i2cmux_pins: i2cmux-pins {
+						pins = "GPIO_76", "GPIO_77";
+						function = "twi_slc_gate";
+						output-low;
+					};
+
+					i2cmux_0: i2cmux-0 {
+						pins = "GPIO_76";
+						function = "twi_slc_gate";
+						output-high;
+					};
+
+					i2cmux_1: i2cmux-1 {
+						pins = "GPIO_77";
+						function = "twi_slc_gate";
+						output-high;
+					};
 				};
 
 				mdio1: mdio@e200413c {
-- 
2.49.0

[PATCH 16/16] misc: lan966x_pci: Add drivers needed to support SFPs in Kconfig help

[Herve Codina]

Recently, new device-tree nodes were added in the overlay to add support
for SFPs on LAN966x PCI device.

The LAN966X Kconfig help section mentions drivers related to devices
added based on the overlay description.

Add drivers related to devices described by those new nodes in the
already existing driver list.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/misc/Kconfig | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 6b37d61150ee..fb3e48543453 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -631,6 +631,11 @@ config MCHP_LAN966X_PCI
 	    - lan966x-serdes (PHY_LAN966X_SERDES)
 	    - lan966x-miim (MDIO_MSCC_MIIM)
 	    - lan966x-switch (LAN966X_SWITCH)
+	    - lan966x-gck (COMMON_CLK_LAN966X)
+	    - i2c-mux-pinctrl (I2C_MUX_PINCTRL)
+	    - sama5d2-flexcom (MFD_ATMEL_FLEXCOM)
+	    - sam9x60-i2c (I2C_AT91)
+	    - sfp (SFP)
 
 source "drivers/misc/c2port/Kconfig"
 source "drivers/misc/eeprom/Kconfig"
-- 
2.49.0

Re: [PATCH 01/16] Revert "treewide: Fix probing of devices in DT overlays"

[Andy Shevchenko]

On Mon, Apr 07, 2025 at 04:55:30PM +0200, Herve Codina wrote:
> From: Saravana Kannan <saravanak@google.com>
> 
> From: Saravana Kannan <saravanak@google.com>

No need to have this in the commit message.

> This reverts commit 1a50d9403fb90cbe4dea0ec9fd0351d2ecbd8924.
> 
> While the commit fixed fw_devlink overlay handling for one case, it
> broke it for another case. So revert it and redo the fix in a separate
> patch.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 03/16] of: dynamic: Fix overlayed devices not probing because of fw_devlink

[Andy Shevchenko]

On Mon, Apr 07, 2025 at 04:55:32PM +0200, Herve Codina wrote:
> From: Saravana Kannan <saravanak@google.com>
> 
> From: Saravana Kannan <saravanak@google.com>

Same here.

> When an overlay is applied, if the target device has already probed
> successfully and bound to a device, then some of the fw_devlink logic
> that ran when the device was probed needs to be rerun. This allows newly
> created dangling consumers of the overlayed device tree nodes to be
> moved to become consumers of the target device.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 02/16] driver core: Rename get_dev_from_fwnode() wrapper to get_device_from_fwnode()

[Andy Shevchenko]

On Mon, Apr 07, 2025 at 04:55:31PM +0200, Herve Codina wrote:
> get_dev_from_fwnode() calls get_device() and so it acquires a reference
> on the device returned.
> 
> In order to be more obvious that this wrapper is a get_device() variant,
> rename it to get_device_from_fwnode().

+1 to the change.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 08/16] i2c: core: Introduce i2c_get_adapter_supplier()

[Andy Shevchenko]

On Mon, Apr 07, 2025 at 04:55:37PM +0200, Herve Codina wrote:
> The supplier device of an I2C adapter is the device that calls
> i2c_add_adapter() or variants and i2c_del_adapter().
> 
> Most of the time this supplier device is the parent of the adapter dev.
> 
> Exceptions exist with i2c muxes. Indeed, in case of i2c muxes, the
> parent of the adapter dev points to the adapter dev the mux is connected

dev --> device (in both cases)

> to instead of the supplier of this adapter.
> 
> Introduce i2c_get_adapter_supplier() and a new supplier field in the
> adapter structure in order to ease the adapter supplier retrieval.

...

> +/**
> + * i2c_get_adapter_supplier() - Get the supplier of an adapter
> + * @adapter: the adapter to get the supplier from
> + *
> + * return:

Return:

> + * Look up and return the &struct device corresponding to the device supplying
> + * this adapter.

@adapter

> + * The user must call put_device() once done with the supplier returned.
> + */
> +struct device *i2c_get_adapter_supplier(struct i2c_adapter *adapter)
> +{
> +	return get_device(adapter->supplier ?: adapter->dev.parent);

What will be the meaning when both are set? Why dev.parent is not the same
as supplier in this case?  Looking at the commit message example, it seems
like you want to provide a physdev or sysdev (as term supplier seems more
devlink:ish), like it's done elsewhere. And in the same way _always_ initialise
it. In such a case, the ambiguity will be gone.

> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 06/16] PCI: of: Set fwnode.dev of newly created PCI device nodes

[Andy Shevchenko]

On Mon, Apr 07, 2025 at 04:55:35PM +0200, Herve Codina wrote:
> Device-tree node can be created when CONFIG_PCI_DYNAMIC_OF_NODES. Those
> node are created and filled based on PCI core information but the
> fwnode.dev field is not set.
> 
> When later an overlay is applied, this consuses fw_devlink. Indeed,
> without any device attached to the node, fw_devlink considers that this
> node will never become a device. When this node is pointed as a
> supplier, devlink looks at its ancestors in order to find a node with a
> device that could be used as the supplier.
> 
> In the PCI use case, this leads to links that wrongly use the PCI root
> bridge device as the supplier instead of the expected PCI device.
> 
> Setting fwnode.dev to the dev of the PCI device allows devlink to use
> this device as a supplier and so, correct links are created.

...

> +	/*
> +	 * Set the fwnode.dev in order to have fw_devlink creating links
> +	 * pointing to this PCI device instead of walking up to the PCI host
> +	 * bridge.
> +	 */
> +	np->fwnode.dev = &pdev->dev;

This is too invasive. I suppose here should be a helper for this kind of
operation. If not, create one.

	fw_devlink_set_device(...);


or alike.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 11/16] of: property: Allow fw_devlink device-tree support for x86

[Andy Shevchenko]

On Mon, Apr 07, 2025 at 04:55:40PM +0200, Herve Codina wrote:
> PCI drivers can use a device-tree overlay to describe the hardware
> available on the PCI board. This is the case, for instance, of the
> LAN966x PCI device driver.
> 
> Adding some more nodes in the device-tree overlay adds some more
> consumer/supplier relationship between devices instantiated from this
> overlay.
> 
> Those fw_node consumer/supplier relationships are handled by fw_devlink
> and are created based on the device-tree parsing done by the
> of_fwnode_add_links() function.
> 
> Those consumer/supplier links are needed in order to ensure a correct PM
> runtime management and a correct removal order between devices.
> 
> For instance, without those links a supplier can be removed before its
> consumers is removed leading to all kind of issue if this consumer still
> want the use the already removed supplier.
> 
> The support for the usage of an overlay from a PCI driver has been added
> on x86 systems in commit 1f340724419ed ("PCI: of: Create device tree PCI
> host bridge node").
> 
> In the past, support for fw_devlink on x86 had been tried but this
> support has been removed in commit 4a48b66b3f52 ("of: property: Disable
> fw_devlink DT support for X86"). Indeed, this support was breaking some
> x86 systems such as OLPC system and the regression was reported in [0].
> 
> Instead of disabling this support for all x86 system, use a finer grain
> and disable this support only for the possible problematic subset of x86

> system mixing ACPI and device-tree at boot time (i.e. OLPC and CE4100).

This is incorrect, they never had ACPI to begin with. Also there is third
platform that are using DT on x86 core — SpreadTrum based phones.

And not sure about AMD stuff (Geode?).

> [0] https://lore.kernel.org/lkml/3c1f2473-92ad-bfc4-258e-a5a08ad73dd0@web.de/

Can you make this to be a Link tag?

Link: https://lore.kernel.org/lkml/3c1f2473-92ad-bfc4-258e-a5a08ad73dd0@web.de/ [0]

> Signed-off-by: Herve Codina <herve.codina@bootlin.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 12/16] clk: lan966x: Add MCHP_LAN966X_PCI dependency

[Andy Shevchenko]

On Mon, Apr 07, 2025 at 04:55:41PM +0200, Herve Codina wrote:
> The lan966x clock controller depends on the LAN969x architecture or the
> LAN966x SoC.
> 
> This clock controller can be used by the LAN966x PCI device and so it
> needs to be available when the LAN966x PCI device is enabled.

...

>  	depends on HAS_IOMEM
>  	depends on OF
> -	depends on SOC_LAN966 || ARCH_LAN969X || COMPILE_TEST
> +	depends on SOC_LAN966 || ARCH_LAN969X || MCHP_LAN966X_PCI || COMPILE_TEST

This doesn't seem to scale. Why not simply

	depends on HAS_IOMEM
	depends on OF || COMPILE_TEST

?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 13/16] i2c: busses: at91: Add MCHP_LAN966X_PCI dependency

[Andy Shevchenko]

On Mon, Apr 07, 2025 at 04:55:42PM +0200, Herve Codina wrote:
> The AT91 I2C driver depends on ARCH_AT91.
> 
> This I2C controller can be used by the LAN966x PCI device and so
> it needs to be available when the LAN966x PCI device is enabled.

...

>  config I2C_AT91
>  	tristate "Atmel AT91 I2C Two-Wire interface (TWI)"
> -	depends on ARCH_AT91 || COMPILE_TEST
> +	depends on ARCH_AT91 || MCHP_LAN966X_PCI || COMPILE_TEST

I would drop it altogether in similar way as suggested for the clock driver.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 16/16] misc: lan966x_pci: Add drivers needed to support SFPs in Kconfig help

[Andy Shevchenko]

On Mon, Apr 07, 2025 at 04:55:45PM +0200, Herve Codina wrote:
> Recently, new device-tree nodes were added in the overlay to add support
> for SFPs on LAN966x PCI device.
> 
> The LAN966X Kconfig help section mentions drivers related to devices
> added based on the overlay description.
> 
> Add drivers related to devices described by those new nodes in the
> already existing driver list.

...

>  	    - lan966x-serdes (PHY_LAN966X_SERDES)
>  	    - lan966x-miim (MDIO_MSCC_MIIM)
>  	    - lan966x-switch (LAN966X_SWITCH)
> +	    - lan966x-gck (COMMON_CLK_LAN966X)

> +	    - i2c-mux-pinctrl (I2C_MUX_PINCTRL)

Perhaps keep it alphabetically ordered?

> +	    - sama5d2-flexcom (MFD_ATMEL_FLEXCOM)
> +	    - sam9x60-i2c (I2C_AT91)
> +	    - sfp (SFP)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 15/16] misc: lan966x_pci: Add dtso nodes in order to support SFPs

[Andrew Lunn]

On Mon, Apr 07, 2025 at 04:55:44PM +0200, Herve Codina wrote:
> Add device-tree nodes needed to support SFPs.
> Those nodes are:
>  - the clock controller
>  - the i2c controller
>  - the i2c mux
>  - the SFPs themselves and their related ports in the switch
> 
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---
>  drivers/misc/lan966x_pci.dtso | 111 ++++++++++++++++++++++++++++++++++
>  1 file changed, 111 insertions(+)
> 
> diff --git a/drivers/misc/lan966x_pci.dtso b/drivers/misc/lan966x_pci.dtso
> index 94a967b384f3..a2015b46cd44 100644
> --- a/drivers/misc/lan966x_pci.dtso
> +++ b/drivers/misc/lan966x_pci.dtso

What exactly does this DTSO file represent?


> @@ -47,6 +47,47 @@ sys_clk: clock-15625000 {
>  				clock-frequency = <15625000>;  /* System clock = 15.625MHz */
>  			};
>  
> +			i2c0_emux: i2c0-emux {
> +				compatible = "i2c-mux-pinctrl";
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +				i2c-parent = <&i2c0>;
> +				pinctrl-names = "i2c102", "i2c103", "idle";
> +				pinctrl-0 = <&i2cmux_0>;
> +				pinctrl-1 = <&i2cmux_1>;
> +				pinctrl-2 = <&i2cmux_pins>;
> +
> +				i2c102: i2c@0 {
> +					reg = <0>;
> +					#address-cells = <1>;
> +					#size-cells = <0>;
> +				};
> +
> +				i2c103: i2c@1 {
> +					reg = <1>;
> +					#address-cells = <1>;
> +					#size-cells = <0>;
> +				};
> +			};
> +
> +			sfp2: sfp2 {
> +				compatible = "sff,sfp";
> +				i2c-bus = <&i2c102>;
> +				tx-disable-gpios = <&gpio 0 GPIO_ACTIVE_HIGH>;
> +				los-gpios = <&gpio 25 GPIO_ACTIVE_HIGH>;
> +				mod-def0-gpios = <&gpio 18 GPIO_ACTIVE_LOW>;
> +				tx-fault-gpios = <&gpio 2 GPIO_ACTIVE_HIGH>;

DT files are generally hierarchical. There is a soc .dtsi file which
describes everything internal to the SoC.  And then we have .dts file
which describes the board the SoC is placed on.

We have a slightly different setup here. A PCI chip instead of a SoC.
And a PCI card in a slot, which could be seen as the board.

The SFP cage is on the board. How the GPIOs and i2c busses are wired
to the SFP cage is a board property, not a SoC/PCI chip
property. Different boards could wire them up differently. So to me,
it seems wrong these nodes are here. They should be in a dtso file
which represents the PCIe board in the slot, and that .dtso file
imports the .dtso file which represents the PCIe chip.

I suppose this comes down to, what do the PCIe IDs represent, since
that is what is used for probing? The PCIe chip, or the board as a
whole. When somebody purchases the chips from Microchip, and builds
their own board, are they required to have their own PCIe IDs, and a
.dtso file representing their board design? The PCIe chip part should
be reusable, so we are talking about stacked dtso files, or at least
included .dtso files.

      Andrew

Re: [PATCH 02/16] driver core: Rename get_dev_from_fwnode() wrapper to get_device_from_fwnode()

[Saravana Kannan]

On Mon, Apr 7, 2025 at 7:56 AM Herve Codina <herve.codina@bootlin.com> wrote:
>
> get_dev_from_fwnode() calls get_device() and so it acquires a reference
> on the device returned.
>
> In order to be more obvious that this wrapper is a get_device() variant,
> rename it to get_device_from_fwnode().
>
> Suggested-by: Mark Brown <broonie@kernel.org>
> Link: https://lore.kernel.org/lkml/CAGETcx97QjnjVR8Z5g0ndLHpK96hLd4aYSV=iEkKPNbNOccYmA@mail.gmail.com/
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>

Thank you!

Reviewed-by: Saravana Kannan <saravanak@google.com>

> ---
>  drivers/base/core.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index d2f9d3a59d6b..f30260fd3031 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -1881,7 +1881,7 @@ static void fw_devlink_unblock_consumers(struct device *dev)
>         device_links_write_unlock();
>  }
>
> -#define get_dev_from_fwnode(fwnode)    get_device((fwnode)->dev)
> +#define get_device_from_fwnode(fwnode) get_device((fwnode)->dev)
>
>  static bool fwnode_init_without_drv(struct fwnode_handle *fwnode)
>  {
> @@ -1891,7 +1891,7 @@ static bool fwnode_init_without_drv(struct fwnode_handle *fwnode)
>         if (!(fwnode->flags & FWNODE_FLAG_INITIALIZED))
>                 return false;
>
> -       dev = get_dev_from_fwnode(fwnode);
> +       dev = get_device_from_fwnode(fwnode);
>         ret = !dev || dev->links.status == DL_DEV_NO_DRIVER;
>         put_device(dev);
>
> @@ -1960,7 +1960,7 @@ static struct device *fwnode_get_next_parent_dev(const struct fwnode_handle *fwn
>         struct device *dev;
>
>         fwnode_for_each_parent_node(fwnode, parent) {
> -               dev = get_dev_from_fwnode(parent);
> +               dev = get_device_from_fwnode(parent);
>                 if (dev) {
>                         fwnode_handle_put(parent);
>                         return dev;
> @@ -2016,8 +2016,8 @@ static bool __fw_devlink_relax_cycles(struct fwnode_handle *con_handle,
>                 goto out;
>         }
>
> -       sup_dev = get_dev_from_fwnode(sup_handle);
> -       con_dev = get_dev_from_fwnode(con_handle);
> +       sup_dev = get_device_from_fwnode(sup_handle);
> +       con_dev = get_device_from_fwnode(con_handle);
>         /*
>          * If sup_dev is bound to a driver and @con hasn't started binding to a
>          * driver, sup_dev can't be a consumer of @con. So, no need to check
> @@ -2156,7 +2156,7 @@ static int fw_devlink_create_devlink(struct device *con,
>         if (sup_handle->flags & FWNODE_FLAG_NOT_DEVICE)
>                 sup_dev = fwnode_get_next_parent_dev(sup_handle);
>         else
> -               sup_dev = get_dev_from_fwnode(sup_handle);
> +               sup_dev = get_device_from_fwnode(sup_handle);
>
>         if (sup_dev) {
>                 /*
> @@ -2225,7 +2225,7 @@ static void __fw_devlink_link_to_consumers(struct device *dev)
>                 bool own_link = true;
>                 int ret;
>
> -               con_dev = get_dev_from_fwnode(link->consumer);
> +               con_dev = get_device_from_fwnode(link->consumer);
>                 /*
>                  * If consumer device is not available yet, make a "proxy"
>                  * SYNC_STATE_ONLY link from the consumer's parent device to
> --
> 2.49.0
>

Re: [PATCH 02/16] driver core: Rename get_dev_from_fwnode() wrapper to get_device_from_fwnode()

[Luca Ceresoli]

On Mon,  7 Apr 2025 16:55:31 +0200
Herve Codina <herve.codina@bootlin.com> wrote:

> get_dev_from_fwnode() calls get_device() and so it acquires a reference
> on the device returned.
> 
> In order to be more obvious that this wrapper is a get_device() variant,
> rename it to get_device_from_fwnode().
> 
> Suggested-by: Mark Brown <broonie@kernel.org>
> Link: https://lore.kernel.org/lkml/CAGETcx97QjnjVR8Z5g0ndLHpK96hLd4aYSV=iEkKPNbNOccYmA@mail.gmail.com/
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>

Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 06/16] PCI: of: Set fwnode.dev of newly created PCI device nodes

[Herve Codina]

Hi Andy,

On Mon, 7 Apr 2025 18:30:13 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Mon, Apr 07, 2025 at 04:55:35PM +0200, Herve Codina wrote:
> > Device-tree node can be created when CONFIG_PCI_DYNAMIC_OF_NODES. Those
> > node are created and filled based on PCI core information but the
> > fwnode.dev field is not set.
> > 
> > When later an overlay is applied, this consuses fw_devlink. Indeed,
> > without any device attached to the node, fw_devlink considers that this
> > node will never become a device. When this node is pointed as a
> > supplier, devlink looks at its ancestors in order to find a node with a
> > device that could be used as the supplier.
> > 
> > In the PCI use case, this leads to links that wrongly use the PCI root
> > bridge device as the supplier instead of the expected PCI device.
> > 
> > Setting fwnode.dev to the dev of the PCI device allows devlink to use
> > this device as a supplier and so, correct links are created.  
> 
> ...
> 
> > +	/*
> > +	 * Set the fwnode.dev in order to have fw_devlink creating links
> > +	 * pointing to this PCI device instead of walking up to the PCI host
> > +	 * bridge.
> > +	 */
> > +	np->fwnode.dev = &pdev->dev;  
> 
> This is too invasive. I suppose here should be a helper for this kind of
> operation. If not, create one.
> 
> 	fw_devlink_set_device(...);
> 
> 
> or alike.

Yes, I will add
  void fw_devlink_set_device(struct fwnode_handle *fwnode, struct device *dev);

Also, I will probably add a new patch in this series in order to use
the new fw_devlink_set_device() when relevant in the already existing code.

Best regards,
Hervé

Re: [PATCH 08/16] i2c: core: Introduce i2c_get_adapter_supplier()

[Herve Codina]

Hi Andy,

On Mon, 7 Apr 2025 18:27:07 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Mon, Apr 07, 2025 at 04:55:37PM +0200, Herve Codina wrote:
> > The supplier device of an I2C adapter is the device that calls
> > i2c_add_adapter() or variants and i2c_del_adapter().
> > 
> > Most of the time this supplier device is the parent of the adapter dev.
> > 
> > Exceptions exist with i2c muxes. Indeed, in case of i2c muxes, the
> > parent of the adapter dev points to the adapter dev the mux is connected  
> 
> dev --> device (in both cases)

Will be updated in the newt iteration.

> 
> > to instead of the supplier of this adapter.
> > 
> > Introduce i2c_get_adapter_supplier() and a new supplier field in the
> > adapter structure in order to ease the adapter supplier retrieval.  
> 
> ...
> 
> > +/**
> > + * i2c_get_adapter_supplier() - Get the supplier of an adapter
> > + * @adapter: the adapter to get the supplier from
> > + *
> > + * return:  
> 
> Return:

Will be updated.

> 
> > + * Look up and return the &struct device corresponding to the device supplying
> > + * this adapter.  
> 
> @adapter

Will be updated.

> 
> > + * The user must call put_device() once done with the supplier returned.
> > + */
> > +struct device *i2c_get_adapter_supplier(struct i2c_adapter *adapter)
> > +{
> > +	return get_device(adapter->supplier ?: adapter->dev.parent);  
> 
> What will be the meaning when both are set? Why dev.parent is not the same
> as supplier in this case?  Looking at the commit message example, it seems
> like you want to provide a physdev or sysdev (as term supplier seems more
> devlink:ish), like it's done elsewhere. And in the same way _always_ initialise
> it. In such a case, the ambiguity will be gone.

When both are set (this is case for i2c muxes), the adapter->supplier the
device that register the I2C adapter using i2c_add_adapter() or variant.
In other word, the device that creates the I2C adapter.

The adapter->dev.parent is most of the time the device that register the
I2C adapter except for i2c muxes. For I2C muxes, this adapter->dev.parent
is the adapter the i2c mux is connected to.

Between physdev and sysdev, I really prefer physdev and, if renaming from
supplier to physdev is still needed (and wanted), I will rename it. Let me
know.

For initialization, I don't want to modify all the I2C controller drivers.
What I can do is to initialize adapter->supplier using adapter->dev.parent
during the i2c_register_adapter() call if it was not already initialize by
the caller (i.e. the I2C controller driver).

Does it make sense ?

Best regards,
Hervé

Re: [PATCH 08/16] i2c: core: Introduce i2c_get_adapter_supplier()

[Andy Shevchenko]

On Tue, Apr 08, 2025 at 03:08:36PM +0200, Herve Codina wrote:
> On Mon, 7 Apr 2025 18:27:07 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > On Mon, Apr 07, 2025 at 04:55:37PM +0200, Herve Codina wrote:

...

> > > +	return get_device(adapter->supplier ?: adapter->dev.parent);  
> > 
> > What will be the meaning when both are set? Why dev.parent is not the same
> > as supplier in this case?  Looking at the commit message example, it seems
> > like you want to provide a physdev or sysdev (as term supplier seems more
> > devlink:ish), like it's done elsewhere. And in the same way _always_ initialise
> > it. In such a case, the ambiguity will be gone.
> 
> When both are set (this is case for i2c muxes), the adapter->supplier the
> device that register the I2C adapter using i2c_add_adapter() or variant.
> In other word, the device that creates the I2C adapter.
> 
> The adapter->dev.parent is most of the time the device that register the
> I2C adapter except for i2c muxes. For I2C muxes, this adapter->dev.parent
> is the adapter the i2c mux is connected to.
> 
> Between physdev and sysdev, I really prefer physdev and, if renaming from
> supplier to physdev is still needed (and wanted), I will rename it. Let me
> know.

The terms supplier/consumer are widely used in terms of power and devlink.
I think here should not be used the term supplier.

> For initialization, I don't want to modify all the I2C controller drivers.
> What I can do is to initialize adapter->supplier using adapter->dev.parent
> during the i2c_register_adapter() call if it was not already initialize by
> the caller (i.e. the I2C controller driver).

This can be done in the I²C core, but I'm not insisting on this part.
We can start from your function only and then decide later on how to
proceed (depending on how many users of that field appear and what
they want to do with it).

> Does it make sense ?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 11/16] of: property: Allow fw_devlink device-tree support for x86

[Herve Codina]

Hi Andy,

On Mon, 7 Apr 2025 18:36:28 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Mon, Apr 07, 2025 at 04:55:40PM +0200, Herve Codina wrote:
> > PCI drivers can use a device-tree overlay to describe the hardware
> > available on the PCI board. This is the case, for instance, of the
> > LAN966x PCI device driver.
> > 
> > Adding some more nodes in the device-tree overlay adds some more
> > consumer/supplier relationship between devices instantiated from this
> > overlay.
> > 
> > Those fw_node consumer/supplier relationships are handled by fw_devlink
> > and are created based on the device-tree parsing done by the
> > of_fwnode_add_links() function.
> > 
> > Those consumer/supplier links are needed in order to ensure a correct PM
> > runtime management and a correct removal order between devices.
> > 
> > For instance, without those links a supplier can be removed before its
> > consumers is removed leading to all kind of issue if this consumer still
> > want the use the already removed supplier.
> > 
> > The support for the usage of an overlay from a PCI driver has been added
> > on x86 systems in commit 1f340724419ed ("PCI: of: Create device tree PCI
> > host bridge node").
> > 
> > In the past, support for fw_devlink on x86 had been tried but this
> > support has been removed in commit 4a48b66b3f52 ("of: property: Disable
> > fw_devlink DT support for X86"). Indeed, this support was breaking some
> > x86 systems such as OLPC system and the regression was reported in [0].
> > 
> > Instead of disabling this support for all x86 system, use a finer grain
> > and disable this support only for the possible problematic subset of x86  
> 
> > system mixing ACPI and device-tree at boot time (i.e. OLPC and CE4100).  
> 
> This is incorrect, they never had ACPI to begin with. Also there is third
> platform that are using DT on x86 core — SpreadTrum based phones.

I will rework the commit log to avoid 'mixing ACPI and device-tree'

For "SpreadTrum based phones", do you have an idea about the Kconfig symbol
I could use to filter our this x86 systems?

Anything I find upstream related to SpreadTrum seems base on ARM cpus.
I probably miss something.

> 
> And not sure about AMD stuff (Geode?).

Same here, if some AMD devices need to be filtered out, is there a specific
Kconfig symbol I can use ?

> 
> > [0] https://lore.kernel.org/lkml/3c1f2473-92ad-bfc4-258e-a5a08ad73dd0@web.de/  
> 
> Can you make this to be a Link tag?
> 
> Link: https://lore.kernel.org/lkml/3c1f2473-92ad-bfc4-258e-a5a08ad73dd0@web.de/ [0]
> 

Yes, of course, I will do that in the next iteration.

Best regards,
Hervé




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

Re: [PATCH 12/16] clk: lan966x: Add MCHP_LAN966X_PCI dependency

[Herve Codina]

Hi Andy,

On Mon, 7 Apr 2025 18:38:30 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Mon, Apr 07, 2025 at 04:55:41PM +0200, Herve Codina wrote:
> > The lan966x clock controller depends on the LAN969x architecture or the
> > LAN966x SoC.
> > 
> > This clock controller can be used by the LAN966x PCI device and so it
> > needs to be available when the LAN966x PCI device is enabled.  
> 
> ...
> 
> >  	depends on HAS_IOMEM
> >  	depends on OF
> > -	depends on SOC_LAN966 || ARCH_LAN969X || COMPILE_TEST
> > +	depends on SOC_LAN966 || ARCH_LAN969X || MCHP_LAN966X_PCI || COMPILE_TEST  
> 
> This doesn't seem to scale. Why not simply
> 
> 	depends on HAS_IOMEM
> 	depends on OF || COMPILE_TEST
> 
> ?
> 

With your proposal, if we configure a kernel without SOC_LAN966x or
ARCH_LAN969x or MCHP_LAN966X_PCI, in other words we configure a kernel
without a real needed for this clock controller driver, the user will be
asked about this driver.

This was already reported by Geert
   https://lore.kernel.org/all/369233dfded88ff6fb342e03794fe31985d84d82.1737383314.git.geert+renesas@glider.be/

I agreed with Geert that asking the user about those driver the LAN966x
depends on was not a good things and leads to confusion.

So, to prevent asking the user about this driver, I followed the same
strategy and added the dependencies.

IMHO, we should keep those dependencies here.

Best regards,
Hervé

Re: [PATCH 13/16] i2c: busses: at91: Add MCHP_LAN966X_PCI dependency

[Herve Codina]

Hi Andy,

On Mon, 7 Apr 2025 18:42:55 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Mon, Apr 07, 2025 at 04:55:42PM +0200, Herve Codina wrote:
> > The AT91 I2C driver depends on ARCH_AT91.
> > 
> > This I2C controller can be used by the LAN966x PCI device and so
> > it needs to be available when the LAN966x PCI device is enabled.  
> 
> ...
> 
> >  config I2C_AT91
> >  	tristate "Atmel AT91 I2C Two-Wire interface (TWI)"
> > -	depends on ARCH_AT91 || COMPILE_TEST
> > +	depends on ARCH_AT91 || MCHP_LAN966X_PCI || COMPILE_TEST  
> 
> I would drop it altogether in similar way as suggested for the clock driver.
> 

IMHO, they should be kept for the reason mentionned for the clock driver.

Best regards,
Hervé

Re: [PATCH 15/16] misc: lan966x_pci: Add dtso nodes in order to support SFPs

[Herve Codina]

Hi Andrew,

On Mon, 7 Apr 2025 22:05:31 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> On Mon, Apr 07, 2025 at 04:55:44PM +0200, Herve Codina wrote:
> > Add device-tree nodes needed to support SFPs.
> > Those nodes are:
> >  - the clock controller
> >  - the i2c controller
> >  - the i2c mux
> >  - the SFPs themselves and their related ports in the switch
> > 
> > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> > ---
> >  drivers/misc/lan966x_pci.dtso | 111 ++++++++++++++++++++++++++++++++++
> >  1 file changed, 111 insertions(+)
> > 
> > diff --git a/drivers/misc/lan966x_pci.dtso b/drivers/misc/lan966x_pci.dtso
> > index 94a967b384f3..a2015b46cd44 100644
> > --- a/drivers/misc/lan966x_pci.dtso
> > +++ b/drivers/misc/lan966x_pci.dtso  
> 
> What exactly does this DTSO file represent?

The dsto represents de board connected to the PCI slot and identified
by its PCI vendor/device IDs.

> 
> 
> > @@ -47,6 +47,47 @@ sys_clk: clock-15625000 {
> >  				clock-frequency = <15625000>;  /* System clock = 15.625MHz */
> >  			};
> >  
> > +			i2c0_emux: i2c0-emux {
> > +				compatible = "i2c-mux-pinctrl";
> > +				#address-cells = <1>;
> > +				#size-cells = <0>;
> > +				i2c-parent = <&i2c0>;
> > +				pinctrl-names = "i2c102", "i2c103", "idle";
> > +				pinctrl-0 = <&i2cmux_0>;
> > +				pinctrl-1 = <&i2cmux_1>;
> > +				pinctrl-2 = <&i2cmux_pins>;
> > +
> > +				i2c102: i2c@0 {
> > +					reg = <0>;
> > +					#address-cells = <1>;
> > +					#size-cells = <0>;
> > +				};
> > +
> > +				i2c103: i2c@1 {
> > +					reg = <1>;
> > +					#address-cells = <1>;
> > +					#size-cells = <0>;
> > +				};
> > +			};
> > +
> > +			sfp2: sfp2 {
> > +				compatible = "sff,sfp";
> > +				i2c-bus = <&i2c102>;
> > +				tx-disable-gpios = <&gpio 0 GPIO_ACTIVE_HIGH>;
> > +				los-gpios = <&gpio 25 GPIO_ACTIVE_HIGH>;
> > +				mod-def0-gpios = <&gpio 18 GPIO_ACTIVE_LOW>;
> > +				tx-fault-gpios = <&gpio 2 GPIO_ACTIVE_HIGH>;  
> 
> DT files are generally hierarchical. There is a soc .dtsi file which
> describes everything internal to the SoC.  And then we have .dts file
> which describes the board the SoC is placed on.
> 
> We have a slightly different setup here. A PCI chip instead of a SoC.
> And a PCI card in a slot, which could be seen as the board.
> 
> The SFP cage is on the board. How the GPIOs and i2c busses are wired
> to the SFP cage is a board property, not a SoC/PCI chip
> property. Different boards could wire them up differently. So to me,
> it seems wrong these nodes are here. They should be in a dtso file
> which represents the PCIe board in the slot, and that .dtso file
> imports the .dtso file which represents the PCIe chip.

The PCI vendor/device ID identifies the board. This is the PCI
peripheral connected to the PCI slot and enumerated. This dtso
describes the board.

The dtso in that case is the equivalent of the dts.

We can move the PCI chip in a dtsi included by this dtso but in the
end this leads to the exact same representation. Further more, moving
out the PCI chip description in its own dtsi out of this dtso can be
done in a second step when an other dtso uses the same chip.

This dtso, describing the board, is applied on the PCI device node.
SDP, i2c mux, ... are described at the same level as the fixed-clock
component for instance.

> 
> I suppose this comes down to, what do the PCIe IDs represent, since
> that is what is used for probing? The PCIe chip, or the board as a
> whole. When somebody purchases the chips from Microchip, and builds
> their own board, are they required to have their own PCIe IDs, and a
> .dtso file representing their board design? The PCIe chip part should
> be reusable, so we are talking about stacked dtso files, or at least
> included .dtso files.
> 

Staked dtso implies stacked overlays and I think is is not a correct
description. There is only one piece of hardware that can be plugged or
un-plugged: the PCI board. This piece of hardware should be fully
described by only one overlay (one dtso).

IMHO, the only reason I can see to have multiple overlay is to apply a
first overlay which help to identify the board connected. For instance,
an eeprom description where some ids are stored and needed to identify
the board. In the PCI case, this is not needed, the PCI vendor/device
IDs are here to identify the board.

Best regards,
Hervé

Re: [PATCH 08/16] i2c: core: Introduce i2c_get_adapter_supplier()

[Herve Codina]

On Tue, 8 Apr 2025 16:47:51 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Tue, Apr 08, 2025 at 03:08:36PM +0200, Herve Codina wrote:
> > On Mon, 7 Apr 2025 18:27:07 +0300
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:  
> > > On Mon, Apr 07, 2025 at 04:55:37PM +0200, Herve Codina wrote:  
> 
> ...
> 
> > > > +	return get_device(adapter->supplier ?: adapter->dev.parent);    
> > > 
> > > What will be the meaning when both are set? Why dev.parent is not the same
> > > as supplier in this case?  Looking at the commit message example, it seems
> > > like you want to provide a physdev or sysdev (as term supplier seems more
> > > devlink:ish), like it's done elsewhere. And in the same way _always_ initialise
> > > it. In such a case, the ambiguity will be gone.  
> > 
> > When both are set (this is case for i2c muxes), the adapter->supplier the
> > device that register the I2C adapter using i2c_add_adapter() or variant.
> > In other word, the device that creates the I2C adapter.
> > 
> > The adapter->dev.parent is most of the time the device that register the
> > I2C adapter except for i2c muxes. For I2C muxes, this adapter->dev.parent
> > is the adapter the i2c mux is connected to.
> > 
> > Between physdev and sysdev, I really prefer physdev and, if renaming from
> > supplier to physdev is still needed (and wanted), I will rename it. Let me
> > know.  
> 
> The terms supplier/consumer are widely used in terms of power and devlink.
> I think here should not be used the term supplier.

physdev seems good.
I will use that.

> 
> > For initialization, I don't want to modify all the I2C controller drivers.
> > What I can do is to initialize adapter->supplier using adapter->dev.parent
> > during the i2c_register_adapter() call if it was not already initialize by
> > the caller (i.e. the I2C controller driver).  
> 
> This can be done in the I²C core, but I'm not insisting on this part.
> We can start from your function only and then decide later on how to
> proceed (depending on how many users of that field appear and what
> they want to do with it).
> 

Right I think I can keep my function as it.

Wolfram any opinion?

Best regards,
Hervé

Re: [PATCH 11/16] of: property: Allow fw_devlink device-tree support for x86

[Andy Shevchenko]

On Tue, Apr 08, 2025 at 03:49:25PM +0200, Herve Codina wrote:
> On Mon, 7 Apr 2025 18:36:28 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > On Mon, Apr 07, 2025 at 04:55:40PM +0200, Herve Codina wrote:

...

> > This is incorrect, they never had ACPI to begin with. Also there is third
> > platform that are using DT on x86 core — SpreadTrum based phones.
> 
> I will rework the commit log to avoid 'mixing ACPI and device-tree'
> 
> For "SpreadTrum based phones", do you have an idea about the Kconfig symbol
> I could use to filter our this x86 systems?

Hmm... good question. I don't think it was anything. The Airmont core just
works and doesn't require anything special to be set. And platform is x86 with
the devices that are established on ARM, so nothing special in device tree
either, I suppose. Basically any x86 platform with OF should be excluded,
rather think of what should be included. But I see that as opposite
requirements to the same function. I have no idea how to solve this. Perhaps
find that SpreadTrum Intel Atom-based device? Would be really hard, I believe.
Especially if we want to install a custom kernel there...

> Anything I find upstream related to SpreadTrum seems base on ARM cpus.
> I probably miss something.

There were two SoCs that were Intel Atom based [1]. And some patches [2] to x86
DT code were made to support those cases.

> > And not sure about AMD stuff (Geode?).
> 
> Same here, if some AMD devices need to be filtered out, is there a specific
> Kconfig symbol I can use ?

This is question to AMD people. I have no clue.

[1]: https://www.anandtech.com/show/11196/mwc-2017-spreadtrum-launches-8core-intel-airmontbased-soc-with-cat-7-lte-for-smartphones

[2]: 4e07db9c8db8 ("x86/devicetree: Use CPU description from Device Tree")
and co. `git log --no-merges 4e07db9c8db8 -- arch/x86/kernel/devicetree.c

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 15/16] misc: lan966x_pci: Add dtso nodes in order to support SFPs

[Andrew Lunn]

On Tue, Apr 08, 2025 at 04:26:03PM +0200, Herve Codina wrote:
> Hi Andrew,
> 
> On Mon, 7 Apr 2025 22:05:31 +0200
> Andrew Lunn <andrew@lunn.ch> wrote:
> 
> > On Mon, Apr 07, 2025 at 04:55:44PM +0200, Herve Codina wrote:
> > > Add device-tree nodes needed to support SFPs.
> > > Those nodes are:
> > >  - the clock controller
> > >  - the i2c controller
> > >  - the i2c mux
> > >  - the SFPs themselves and their related ports in the switch
> > > 
> > > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> > > ---
> > >  drivers/misc/lan966x_pci.dtso | 111 ++++++++++++++++++++++++++++++++++
> > >  1 file changed, 111 insertions(+)
> > > 
> > > diff --git a/drivers/misc/lan966x_pci.dtso b/drivers/misc/lan966x_pci.dtso
> > > index 94a967b384f3..a2015b46cd44 100644
> > > --- a/drivers/misc/lan966x_pci.dtso
> > > +++ b/drivers/misc/lan966x_pci.dtso  
> > 
> > What exactly does this DTSO file represent?
> 
> The dsto represents de board connected to the PCI slot and identified
> by its PCI vendor/device IDs.

Then i think the name lan966x_pci.dtso is too generic. It should be
named after whatever microchip calls the RDK.

> We can move the PCI chip in a dtsi included by this dtso but in the
> end this leads to the exact same representation. Further more, moving
> out the PCI chip description in its own dtsi out of this dtso can be
> done in a second step when an other dtso uses the same chip.

And what would you call this pulled out dtsi file? lan966x_pci.dtsi?
That is going to be confusing.

Naming is hard, but we should assume this PCIe device is going to be
successful, and a number of OEMs will build cards around it, so there
needs to be space within the naming scheme for them.

	Andrew

Re: [PATCH 15/16] misc: lan966x_pci: Add dtso nodes in order to support SFPs

[Thomas Petazzoni]

Andrew, Hervé,

On Tue Apr 8, 2025 at 4:26 PM CEST, Herve Codina wrote:

>> What exactly does this DTSO file represent?
>
> The dsto represents de board connected to the PCI slot and identified
> by its PCI vendor/device IDs.

If I may extend on that by providing what I believe is a more
accurate/precise definition.

The DTSO doesn't represent the board, rather it describes the HW
topology of the devices inside the PCI endpoint. Indeed, the PCI
endpoint is a full-blown SoC with lots of different HW blocks that
already have drivers in the kernel (because the same chip can be used
with Linux running on an ARM core embedded in the SoC, rather than
access as a PCI endpoint). So the DTSO describes the full topology of
the HW blocks inside this complex PCI endpoint, just like the DTS
describes the full topology of the HW blocks inside an SoC.

Please see:

  https://lpc.events/event/17/contributions/1421/attachments/1337/2680/LPC2023%20Non-discoverable%20devices%20in%20PCI.pdf

And most notably slide 6.

Best regards,

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com

Re: [PATCH 15/16] misc: lan966x_pci: Add dtso nodes in order to support SFPs

[Andrew Lunn]

On Tue, Apr 08, 2025 at 05:13:54PM +0200, Thomas Petazzoni wrote:
> Andrew, Hervé,
> 
> On Tue Apr 8, 2025 at 4:26 PM CEST, Herve Codina wrote:
> 
> >> What exactly does this DTSO file represent?
> >
> > The dsto represents de board connected to the PCI slot and identified
> > by its PCI vendor/device IDs.
> 
> If I may extend on that by providing what I believe is a more
> accurate/precise definition.
> 
> The DTSO doesn't represent the board, rather it describes the HW
> topology of the devices inside the PCI endpoint. Indeed, the PCI
> endpoint is a full-blown SoC with lots of different HW blocks that
> already have drivers in the kernel (because the same chip can be used
> with Linux running on an ARM core embedded in the SoC, rather than
> access as a PCI endpoint). So the DTSO describes the full topology of
> the HW blocks inside this complex PCI endpoint, just like the DTS
> describes the full topology of the HW blocks inside an SoC.

"HW blocks inside an SoC." That would be the SoC .dtsi file. Anything
outside of the SoC is in the .dts file. OEM vendors take the SoC,
build a board around it, and name there .dts file after the board,
describing how the board components are connected to the SoC.

So..

So by PCI endpoint, you mean the PCIe chip? So it sounds like there
should be a .dtsi file describing the chip.

Everything outside of the chip, like the SFP cages, are up to the
vendor building the board. I would say that should be described in a
.dtso file, which describes how the board components are connected to
the PCIe chip? And that .dtso file should be named after the board,
since there are going to many of them, from different OEM vendors.

	Andrew

Re: [PATCH 15/16] misc: lan966x_pci: Add dtso nodes in order to support SFPs

[Thomas Petazzoni]

On Tue Apr 8, 2025 at 5:38 PM CEST, Andrew Lunn wrote:

> "HW blocks inside an SoC." That would be the SoC .dtsi file. Anything
> outside of the SoC is in the .dts file. OEM vendors take the SoC,
> build a board around it, and name there .dts file after the board,
> describing how the board components are connected to the SoC.
>
> So..
>
> So by PCI endpoint, you mean the PCIe chip? So it sounds like there
> should be a .dtsi file describing the chip.
>
> Everything outside of the chip, like the SFP cages, are up to the
> vendor building the board. I would say that should be described in a
> .dtso file, which describes how the board components are connected to
> the PCIe chip? And that .dtso file should be named after the board,
> since there are going to many of them, from different OEM vendors.

Indeed, that makes sense. So if I get correctly your suggestion,
instead of having a .dtso that describes everything, it should be
split between:

 - A .dtsi that describes what's inside the LAN996x when used in PCI
   endpoint mode

 - A .dtso that includes the above .dtsi, and that describes what on
   the PCI board around the LAN966x.

Correct?

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com

Re: [PATCH 15/16] misc: lan966x_pci: Add dtso nodes in order to support SFPs

[Geert Uytterhoeven]

On Wed, 9 Apr 2025 at 09:44, Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
> On Tue Apr 8, 2025 at 5:38 PM CEST, Andrew Lunn wrote:
> > "HW blocks inside an SoC." That would be the SoC .dtsi file. Anything
> > outside of the SoC is in the .dts file. OEM vendors take the SoC,
> > build a board around it, and name there .dts file after the board,
> > describing how the board components are connected to the SoC.
> >
> > So..
> >
> > So by PCI endpoint, you mean the PCIe chip? So it sounds like there
> > should be a .dtsi file describing the chip.
> >
> > Everything outside of the chip, like the SFP cages, are up to the
> > vendor building the board. I would say that should be described in a
> > .dtso file, which describes how the board components are connected to
> > the PCIe chip? And that .dtso file should be named after the board,
> > since there are going to many of them, from different OEM vendors.
>
> Indeed, that makes sense. So if I get correctly your suggestion,
> instead of having a .dtso that describes everything, it should be
> split between:
>
>  - A .dtsi that describes what's inside the LAN996x when used in PCI
>    endpoint mode
>
>  - A .dtso that includes the above .dtsi, and that describes what on
>    the PCI board around the LAN966x.
>
> Correct?

Sounds good to me!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 15/16] misc: lan966x_pci: Add dtso nodes in order to support SFPs

[Andrew Lunn]

On Wed, Apr 09, 2025 at 09:44:25AM +0200, Thomas Petazzoni wrote:
> On Tue Apr 8, 2025 at 5:38 PM CEST, Andrew Lunn wrote:
> 
> > "HW blocks inside an SoC." That would be the SoC .dtsi file. Anything
> > outside of the SoC is in the .dts file. OEM vendors take the SoC,
> > build a board around it, and name there .dts file after the board,
> > describing how the board components are connected to the SoC.
> >
> > So..
> >
> > So by PCI endpoint, you mean the PCIe chip? So it sounds like there
> > should be a .dtsi file describing the chip.
> >
> > Everything outside of the chip, like the SFP cages, are up to the
> > vendor building the board. I would say that should be described in a
> > .dtso file, which describes how the board components are connected to
> > the PCIe chip? And that .dtso file should be named after the board,
> > since there are going to many of them, from different OEM vendors.
> 
> Indeed, that makes sense. So if I get correctly your suggestion,
> instead of having a .dtso that describes everything, it should be
> split between:
> 
>  - A .dtsi that describes what's inside the LAN996x when used in PCI
>    endpoint mode
> 
>  - A .dtso that includes the above .dtsi, and that describes what on
>    the PCI board around the LAN966x.
> 
> Correct?

Yes.

And you need some way to map the PCI ID to the correct .dtso file.
Maybe that is just a lookup table in the driver, or maybe you can pack
the .dtso file into a kernel module with the correct
MODULE_DEVICE_TABLE(pci, ...) so that PCI probing pulls in the
specific driver module with the .dtso, which via dependencies pulls in
the core driver which can actually make use of the .dtso?

    Andrew

Re: [PATCH 15/16] misc: lan966x_pci: Add dtso nodes in order to support SFPs

[Thomas Petazzoni]

On Wed, 9 Apr 2025 16:04:51 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> And you need some way to map the PCI ID to the correct .dtso file.
> Maybe that is just a lookup table in the driver, or maybe you can pack
> the .dtso file into a kernel module with the correct
> MODULE_DEVICE_TABLE(pci, ...) so that PCI probing pulls in the
> specific driver module with the .dtso, which via dependencies pulls in
> the core driver which can actually make use of the .dtso?

Well, check the already upstream driver:

  https://elixir.bootlin.com/linux/v6.13.7/source/drivers/misc/lan966x_pci.c

It indeed binds on the PCI ID, and the driver bundles the .dtbo.

Best regards,

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com

Re: [PATCH 15/16] misc: lan966x_pci: Add dtso nodes in order to support SFPs

[Andrew Lunn]

On Wed, Apr 09, 2025 at 04:14:44PM +0200, Thomas Petazzoni wrote:
> On Wed, 9 Apr 2025 16:04:51 +0200
> Andrew Lunn <andrew@lunn.ch> wrote:
> 
> > And you need some way to map the PCI ID to the correct .dtso file.
> > Maybe that is just a lookup table in the driver, or maybe you can pack
> > the .dtso file into a kernel module with the correct
> > MODULE_DEVICE_TABLE(pci, ...) so that PCI probing pulls in the
> > specific driver module with the .dtso, which via dependencies pulls in
> > the core driver which can actually make use of the .dtso?
> 
> Well, check the already upstream driver:
> 
>   https://elixir.bootlin.com/linux/v6.13.7/source/drivers/misc/lan966x_pci.c
> 
> It indeed binds on the PCI ID, and the driver bundles the .dtbo.

So it only supports a single .dtbo. In its current form it does not
scale to multiple .dtso files for multiple different boards built
around the PCIe chip.

At the moment, that is not really an issue, but when the second board
comes along, some refactoring will be needed.

      Andrew

Re: [PATCH 15/16] misc: lan966x_pci: Add dtso nodes in order to support SFPs

[Thomas Petazzoni]

On Wed, 9 Apr 2025 17:03:45 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> So it only supports a single .dtbo. In its current form it does not
> scale to multiple .dtso files for multiple different boards built
> around the PCIe chip.
> 
> At the moment, that is not really an issue, but when the second board
> comes along, some refactoring will be needed.

Indeed, but that's really an implementation detail. It doesn't change
anything to the overall approach. The only thing that would have to
change is how the driver gets the .dtbo. We could bundle several .dtbos
in the driver, we could fall back to request_firmware(), etc.

Best regards,

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com

Re: [PATCH 15/16] misc: lan966x_pci: Add dtso nodes in order to support SFPs

[Herve Codina]

Hi Thomas, Andrew,

On Thu, 10 Apr 2025 08:48:09 +0200
Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote:

> On Wed, 9 Apr 2025 17:03:45 +0200
> Andrew Lunn <andrew@lunn.ch> wrote:
> 
> > So it only supports a single .dtbo. In its current form it does not
> > scale to multiple .dtso files for multiple different boards built
> > around the PCIe chip.
> > 
> > At the moment, that is not really an issue, but when the second board
> > comes along, some refactoring will be needed.  
> 
> Indeed, but that's really an implementation detail. It doesn't change
> anything to the overall approach. The only thing that would have to
> change is how the driver gets the .dtbo. We could bundle several .dtbos
> in the driver, we could fall back to request_firmware(), etc.
> 

Not sure we need to split right now the existing dtso file nor rename it
even if it is updated in the series.

This could be done later when an other user of the LAN996x PCI chip is
there.

Doing something right now will probably need other modification when this
potential other user comes in. Indeed, depending on specificities of this
future user, what is done now could not match the need of this future user.

Any opinion?

Best regards,
Hervé

Re: [PATCH 15/16] misc: lan966x_pci: Add dtso nodes in order to support SFPs

[Andrew Lunn]

On Wed, Apr 16, 2025 at 11:18:19AM +0200, Herve Codina wrote:
> Hi Thomas, Andrew,
> 
> On Thu, 10 Apr 2025 08:48:09 +0200
> Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote:
> 
> > On Wed, 9 Apr 2025 17:03:45 +0200
> > Andrew Lunn <andrew@lunn.ch> wrote:
> > 
> > > So it only supports a single .dtbo. In its current form it does not
> > > scale to multiple .dtso files for multiple different boards built
> > > around the PCIe chip.
> > > 
> > > At the moment, that is not really an issue, but when the second board
> > > comes along, some refactoring will be needed.  
> > 
> > Indeed, but that's really an implementation detail. It doesn't change
> > anything to the overall approach. The only thing that would have to
> > change is how the driver gets the .dtbo. We could bundle several .dtbos
> > in the driver, we could fall back to request_firmware(), etc.
> > 
> 
> Not sure we need to split right now the existing dtso file nor rename it
> even if it is updated in the series.
> 
> This could be done later when an other user of the LAN996x PCI chip is
> there.
> 
> Doing something right now will probably need other modification when this
> potential other user comes in. Indeed, depending on specificities of this
> future user, what is done now could not match the need of this future user.
> 
> Any opinion?

I agree support for multiple .dtso can be done later.

But i would do the split between the .dtsi file for the PCIe device,
and the .dtso file for the board now. That is a pretty fundamental
concept in Linux DT support.

	Andrew

Re: [PATCH 11/16] of: property: Allow fw_devlink device-tree support for x86

[Herve Codina]

Hi Andy,

On Tue, 8 Apr 2025 17:34:54 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Tue, Apr 08, 2025 at 03:49:25PM +0200, Herve Codina wrote:
> > On Mon, 7 Apr 2025 18:36:28 +0300
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:  
> > > On Mon, Apr 07, 2025 at 04:55:40PM +0200, Herve Codina wrote:  
> 
> ...
> 
> > > This is incorrect, they never had ACPI to begin with. Also there is third
> > > platform that are using DT on x86 core — SpreadTrum based phones.  
> > 
> > I will rework the commit log to avoid 'mixing ACPI and device-tree'
> > 
> > For "SpreadTrum based phones", do you have an idea about the Kconfig symbol
> > I could use to filter our this x86 systems?  
> 
> Hmm... good question. I don't think it was anything. The Airmont core just
> works and doesn't require anything special to be set. And platform is x86 with
> the devices that are established on ARM, so nothing special in device tree
> either, I suppose. Basically any x86 platform with OF should be excluded,
> rather think of what should be included. But I see that as opposite
> requirements to the same function. I have no idea how to solve this. Perhaps
> find that SpreadTrum Intel Atom-based device? Would be really hard, I believe.
> Especially if we want to install a custom kernel there...
> 
> > Anything I find upstream related to SpreadTrum seems base on ARM cpus.
> > I probably miss something.  
> 
> There were two SoCs that were Intel Atom based [1]. And some patches [2] to x86
> DT code were made to support those cases.
> 
> > > And not sure about AMD stuff (Geode?).  
> > 
> > Same here, if some AMD devices need to be filtered out, is there a specific
> > Kconfig symbol I can use ?  
> 
> This is question to AMD people. I have no clue.
> 
> [1]: https://www.anandtech.com/show/11196/mwc-2017-spreadtrum-launches-8core-intel-airmontbased-soc-with-cat-7-lte-for-smartphones
> 
> [2]: 4e07db9c8db8 ("x86/devicetree: Use CPU description from Device Tree")
> and co. `git log --no-merges 4e07db9c8db8 -- arch/x86/kernel/devicetree.c
> 

I have tried to find a solution for this topic.

Indeed, this patch enables fw_devlink based on device-tree on all x86
platform except OLPC and CE4100.

You have mentioned some other x86 based system that could have issues with
fw_devlink and it seems to be hard to have a complete list of systems for
which we should not enable fw_devlink (potential issues and so regression
against current kernel behavior).

As you also proposed, we can thing on the opposite direction and enable
fw_devlink on x86 systems that need it.

We need it because we need the device-tree description over PCI device feature
(CONFIG_PCI_DYNAMIC_OF_NODES) on x86 in order to support the LAN966x use case.

What do you think about the following condition?

	if (IS_ENABLED(CONFIG_X86) && !IS_ENABLED(CONFIG_PCI_DYNAMIC_OF_NODES))
 		return 0; /* Not enabled */

CONFIG_PCI_DYNAMIC_OF_NODES has already to set explicitly by the user.


Do you think it makes sense and could be a good alternative instead of
filtering out a list of x86 systems ?

Best regards,
Hervé

Re: [PATCH 11/16] of: property: Allow fw_devlink device-tree support for x86

[Andy Shevchenko]

On Fri, Apr 18, 2025 at 03:10:36PM +0200, Herve Codina wrote:
> On Tue, 8 Apr 2025 17:34:54 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > On Tue, Apr 08, 2025 at 03:49:25PM +0200, Herve Codina wrote:
> > > On Mon, 7 Apr 2025 18:36:28 +0300
> > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:  
> > > > On Mon, Apr 07, 2025 at 04:55:40PM +0200, Herve Codina wrote:  

...

> > > > This is incorrect, they never had ACPI to begin with. Also there is third
> > > > platform that are using DT on x86 core — SpreadTrum based phones.  
> > > 
> > > I will rework the commit log to avoid 'mixing ACPI and device-tree'
> > > 
> > > For "SpreadTrum based phones", do you have an idea about the Kconfig symbol
> > > I could use to filter our this x86 systems?  
> > 
> > Hmm... good question. I don't think it was anything. The Airmont core just
> > works and doesn't require anything special to be set. And platform is x86 with
> > the devices that are established on ARM, so nothing special in device tree
> > either, I suppose. Basically any x86 platform with OF should be excluded,
> > rather think of what should be included. But I see that as opposite
> > requirements to the same function. I have no idea how to solve this. Perhaps
> > find that SpreadTrum Intel Atom-based device? Would be really hard, I believe.
> > Especially if we want to install a custom kernel there...
> > 
> > > Anything I find upstream related to SpreadTrum seems base on ARM cpus.
> > > I probably miss something.  
> > 
> > There were two SoCs that were Intel Atom based [1]. And some patches [2] to x86
> > DT code were made to support those cases.
> > 
> > > > And not sure about AMD stuff (Geode?).  
> > > 
> > > Same here, if some AMD devices need to be filtered out, is there a specific
> > > Kconfig symbol I can use ?  
> > 
> > This is question to AMD people. I have no clue.
> > 
> > [1]: https://www.anandtech.com/show/11196/mwc-2017-spreadtrum-launches-8core-intel-airmontbased-soc-with-cat-7-lte-for-smartphones
> > 
> > [2]: 4e07db9c8db8 ("x86/devicetree: Use CPU description from Device Tree")
> > and co. `git log --no-merges 4e07db9c8db8 -- arch/x86/kernel/devicetree.c
> 
> I have tried to find a solution for this topic.
> 
> Indeed, this patch enables fw_devlink based on device-tree on all x86
> platform except OLPC and CE4100.
> 
> You have mentioned some other x86 based system that could have issues with
> fw_devlink and it seems to be hard to have a complete list of systems for
> which we should not enable fw_devlink (potential issues and so regression
> against current kernel behavior).
> 
> As you also proposed, we can thing on the opposite direction and enable
> fw_devlink on x86 systems that need it.
> 
> We need it because we need the device-tree description over PCI device feature
> (CONFIG_PCI_DYNAMIC_OF_NODES) on x86 in order to support the LAN966x use case.
> 
> What do you think about the following condition?
> 
> 	if (IS_ENABLED(CONFIG_X86) && !IS_ENABLED(CONFIG_PCI_DYNAMIC_OF_NODES))
>  		return 0; /* Not enabled */
> 
> CONFIG_PCI_DYNAMIC_OF_NODES has already to set explicitly by the user.
> 
> Do you think it makes sense and could be a good alternative instead of
> filtering out a list of x86 systems ?

At least this won't break old platforms that won't set that configuration
option. Ideally, of course, it would be nice to have some kind of detection
at run-time...

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 11/16] of: property: Allow fw_devlink device-tree support for x86

[Arnd Bergmann]

On Tue, Apr 8, 2025, at 15:49, Herve Codina wrote:
> On Mon, 7 Apr 2025 18:36:28 +0300
>> On Mon, Apr 07, 2025 at 04:55:40PM +0200, Herve Codina wrote:
>> 
>> This is incorrect, they never had ACPI to begin with. Also there is third
>> platform that are using DT on x86 core — SpreadTrum based phones.
>
> I will rework the commit log to avoid 'mixing ACPI and device-tree'
>
> For "SpreadTrum based phones", do you have an idea about the Kconfig symbol
> I could use to filter our this x86 systems?
>
> Anything I find upstream related to SpreadTrum seems base on ARM cpus.
> I probably miss something.

This is the Intel SOFIA platform with chips from both Spreadtrum (now
Unisoc) and Rockchips, using a port of the arch/arm/ code DT on x86,
about 10 years ago.

That code was never upstreamed, and is long abandoned by everyone.

>> And not sure about AMD stuff (Geode?).
>
> Same here, if some AMD devices need to be filtered out, is there a specific
> Kconfig symbol I can use ?

The only one I can think of is CONFIG_OLPC, the XO-1 was a Geode LX,
while XO-1.5 used a VIA C7, both with actual open firmware (not just
fdt).

       Arnd