| Message ID | 20250407145546.270683-16-herve.codina@bootlin.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | lan966x pci device: Add support for SFPs | expand |
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
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 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
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 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
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
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 {
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(+)