| Message ID | 20250507071315.394857-1-herve.codina@bootlin.com |
|---|---|
| Headers | show |
| Series | lan966x pci device: Add support for SFPs | expand |
On Wed, May 07, 2025 at 09:13:03AM +0200, Herve Codina wrote: > The lan966x_pci.dtso file contains descriptions related to both the > LAN966x PCI device chip and the LAN966x PCI device board where the chip > is soldered. > > Split the file in order to have: > - lan966x_pci.dtsi > The description related to the PCI chip. > > - lan966x_pci.dtso > The description of the PCI board. > > Signed-off-by: Herve Codina <herve.codina@bootlin.com> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
On Wed, May 07, 2025 at 09:13:05AM +0200, Herve Codina wrote: > Only one device-tree overlay (lan966x_evb_lan9662_nic.dtbo) is handled > and this overlay is directly referenced in lan966x_pci_load_overlay(). > > This avoid to use the code for an other board. > > In order to be more generic and to allow support for other boards (PCI > Vendor/Device IDs), introduce the lan966x_pci_info structure and attach > it to PCI Vendor/Device IDs handled by the driver. > > This structure contains information related to the PCI board such as > information related to the dtbo describing the board we have to load. > > Signed-off-by: Herve Codina <herve.codina@bootlin.com> How big is the dtbo ? This is going in the right direction. I'm just wondering if each dtbo should be wrapped in its own very slim PCI driver, which simply registers its lan966x_pci_info structure to a core driver. Only the needed dtbo will then be loaded into memory as a module, not them all. Pretty much all the pieces are here, so it can be done later. Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
On Thu, 8 May 2025 at 00:24, Andrew Lunn <andrew@lunn.ch> wrote: > On Wed, May 07, 2025 at 09:13:05AM +0200, Herve Codina wrote: > > Only one device-tree overlay (lan966x_evb_lan9662_nic.dtbo) is handled > > and this overlay is directly referenced in lan966x_pci_load_overlay(). > > > > This avoid to use the code for an other board. > > > > In order to be more generic and to allow support for other boards (PCI > > Vendor/Device IDs), introduce the lan966x_pci_info structure and attach > > it to PCI Vendor/Device IDs handled by the driver. > > > > This structure contains information related to the PCI board such as > > information related to the dtbo describing the board we have to load. > > > > Signed-off-by: Herve Codina <herve.codina@bootlin.com> > > How big is the dtbo ? > > This is going in the right direction. I'm just wondering if each dtbo > should be wrapped in its own very slim PCI driver, which simply > registers its lan966x_pci_info structure to a core driver. Only the > needed dtbo will then be loaded into memory as a module, not them all. Alternatively, the dtbo could be loaded through request_firmware(). That could lead to a generic support option in the PCI core, which would fallback to loading pci-<vid>-<pid>.dtbo when no driver is available. > Pretty much all the pieces are here, so it can be done later. Exactly. Gr{oetje,eeting}s, Geert
On Wed, 7 May 2025 18:10:40 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Wed, May 07, 2025 at 09:12:51AM +0200, Herve Codina wrote: > > The code set directly dev->fwnode. > > > > Use the dedicated helper to perform this operation. > > ... > > > @@ -1046,7 +1046,7 @@ static void mock_companion(struct acpi_device *adev, struct device *dev) > > { > > device_initialize(&adev->dev); > > fwnode_init(&adev->fwnode, NULL); > > - dev->fwnode = &adev->fwnode; > > + device_set_node(dev, &adev->fwnode); > > adev->fwnode.dev = dev; > > } > > This code is questionable to begin with. Can the original author explain what > is the motivation behind this as the only callers of fwnode_init() are deep > core pieces _and_ this only module. Why?! > More likely to happen if CXL folk are +CC. Added. Dan, maybe one for you?
On Wed, May 07, 2025 at 09:12:47AM +0200, Herve Codina wrote: > The simple-pm-bus drivers handles several simple bus. When it is used bus --> busses ? > 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. ... > if (match && match->data) { > if (of_property_match_string(np, "compatible", match->compatible) == 0) Side note, there is an fwnode_is_device_compatible() API for such cases. And IIRC there is also OF variant of it. > - return 0; > + goto populate; > else > return -ENODEV; > } ... > + if (pdev->dev.of_node) Why do you need this check? AFAICS it dups the one the call has already in it. > + of_platform_depopulate(&pdev->dev);
On Wed, May 07, 2025 at 09:12:59AM +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 are removed OR consumer > 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, a first approach > would be to use a finer grain and disable this support only for the > possible problematic subset of x86 systems (at least OLPC and CE4100). > > This first approach could still leads to issues. Indeed, the list of > possible problematic system and the way to identify them using Kconfig > symbols is not well defined and so some system can be missed leading to > kernel regressions on those missing systems. > > Use an other way and enable the support on x86 system only when this > support is needed by some specific feature. The usage of a device-tree > overlay by a PCI driver and thus the creation of PCI device-tree nodes > is a feature that needs it. > > Signed-off-by: Herve Codina <herve.codina@bootlin.com> > link: https://lore.kernel.org/lkml/3c1f2473-92ad-bfc4-258e-a5a08ad73dd0@web.de/ [0] Link: (mind capitalisation) Otherwise LGTM, FWIW, Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
On Wed, May 07, 2025 at 09:13:05AM +0200, Herve Codina wrote: > Only one device-tree overlay (lan966x_evb_lan9662_nic.dtbo) is handled > and this overlay is directly referenced in lan966x_pci_load_overlay(). > > This avoid to use the code for an other board. > > In order to be more generic and to allow support for other boards (PCI > Vendor/Device IDs), introduce the lan966x_pci_info structure and attach > it to PCI Vendor/Device IDs handled by the driver. > > This structure contains information related to the PCI board such as > information related to the dtbo describing the board we have to load. ... > static struct pci_device_id lan966x_pci_ids[] = { > - { PCI_DEVICE(PCI_VENDOR_ID_EFAR, 0x9660) }, > + { PCI_VDEVICE(EFAR, 0x9660), (kernel_ulong_t)&evb_lan9662_nic_info }, PCI_DEVICE_DATA() ? > { } > };
Hi Herve, On Wed, May 07, 2025 at 09:13:01AM +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. > > Signed-off-by: Herve Codina <herve.codina@bootlin.com> Acked-by: Andi Shyti <andi.shyti@kernel.org> Thanks, Andi
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 to 11 introduce and use fw_devlink_set_device() in already existing code. Patches 12 and 13 are related also to fw_devlink but specific to PCI and the device-tree nodes created during enumeration. Patches 14, 15 and 16 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 17 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 enabling fw_devlink on *all* x86 system or on *all* x86 system except on those where it leads to issue, enable it only on system where it is needed. Patches 18 and 19 allow to build clock and i2c controller used by the LAN966x PCI device when the LAN966x PCI device is enabled. Patches 20 to 23 are specific to the LAN966x. They touch the current dtso, split it in dtsi/dtso files, rename the dtso and improve the driver to allow easier support for other boards. The next patch (patch 24) update the LAN966x device-tree overlay itself to have the SPF ports and devices they depends on described. The last two patches (patches 25 and 26) sort the existing drivers in the needed driver list available in the Kconfig help and add new drivers in this list keep the 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/ Compare to previous iteration, this v2 series mainly: - Remove unneeded 'From' tag from commit logs - Add 'Reviewed-by' tags - Update commit logs - Introduce fw_devlink_set_device() - Split the dtso in dtsi/dtso files and rename the dtso Best regards, Hervé Changes: v1 -> v2 v1: https://lore.kernel.org/lkml/20250407145546.270683-1-herve.codina@bootlin.com/ - Patch 1 and 3 Remove 'From' tag from the commit log - Patch 2 Add 'Reviewed-by: Andy Shevchenko' Add 'Reviewed-by: Saravana Kannan' Add 'Reviewed-by: Luca Ceresoli' - Patch 4 and 5 No changes - Patch 6 (new in v2) Introduce fw_devlink_set_device() - Patch 7 (new in v2) Use existing device_set_node() helper. - Patch 8 to 11 (new in v2) Use fw_devlink_set_device() in existing code. - Patch 12 (6 in v1) Use fw_devlink_add_device() - Patch 13 (7 in v1) No changes - Patch 14 (8 in v1) Update commit log Use 'physdev' instead of 'supplier' Minor fixes in i2c_get_adapter_physdev() kdoc - Patch 15 and 16 (9 and 10 in v1) Use 'physdev' instead of 'supplier' (commit log, title and code) - Patch 17 (11 in v2) Enable fw_devlink on x86 only if PCI_DYNAMIC_OF_NODES is enabled. Rework commit log. - Patch 18, 19 and 20 (12, 13 and 14 in v1) No changes - Patch 21 (new in v2) Split dtso in dtsi/dtso - Patch 22 (new in v2) Rename lan966x_pci.dtso using the specific board name - Patch 23 (new in v2) Improve the driver introducing board specific data to ease support for other boards (avoid the direct dtbo reference in the function loading the dtbo). - Patch 24 (15 in v1) Refactor due to dtso split in dtsi/dtso - Patch 25 (new in v2) Sort exist driver list in Kconfig help - Patch 26 (16 in v1) Keep alphanumeric order for new drivers added in Kconfig help Herve Codina (24): 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 driver core: fw_devlink: Introduce fw_devlink_set_device() drivers: core: Use fw_devlink_set_device() pinctrl: cs42l43: Use fw_devlink_set_device() cxl/test: Use device_set_node() cxl/test: Use fw_devlink_set_device() PCI: of: Use fw_devlink_set_device() PCI: of: Set fwnode device 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_physdev() i2c: mux: Set adapter physical device i2c: mux: Create missing devlink between mux and adapter physical device of: property: Allow fw_devlink device-tree on x86 when PCI device-tree node creation is enabled 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: Split dtso in dtsi/dtso misc: lan966x_pci: Rename lan966x_pci.dtso to lan966x_evb_lan9662_nic.dtso misc: lan966x_pci: Introduce board specific data misc: lan966x_pci: Add dtsi/dtso nodes in order to support SFPs misc: lan966x_pci: Sort the drivers list in Kconfig help 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 MAINTAINERS | 3 +- drivers/base/core.c | 97 +++++++++--- 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 | 11 +- drivers/misc/Makefile | 2 +- drivers/misc/lan966x_evb_lan9662_nic.dtso | 167 ++++++++++++++++++++ drivers/misc/lan966x_pci.c | 30 +++- drivers/misc/lan966x_pci.dtsi | 172 +++++++++++++++++++++ drivers/misc/lan966x_pci.dtso | 177 ---------------------- drivers/of/dynamic.c | 1 - drivers/of/overlay.c | 15 ++ drivers/of/platform.c | 5 - drivers/of/property.c | 2 +- drivers/pci/of.c | 10 +- drivers/pinctrl/cirrus/pinctrl-cs42l43.c | 2 +- drivers/spi/spi.c | 5 - include/linux/fwnode.h | 7 + include/linux/i2c.h | 3 + tools/testing/cxl/test/cxl.c | 4 +- 25 files changed, 539 insertions(+), 249 deletions(-) create mode 100644 drivers/misc/lan966x_evb_lan9662_nic.dtso create mode 100644 drivers/misc/lan966x_pci.dtsi delete mode 100644 drivers/misc/lan966x_pci.dtso