| Message ID | 20250407145546.270683-1-herve.codina@bootlin.com |
|---|---|
| Headers | show |
| Series | lan966x pci device: Add support for SFPs | expand |
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é
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
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
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
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
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
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
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
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
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(-)