[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 at 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 at 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 at 0 {
> > +					reg = <0>;
> > +					#address-cells = <1>;
> > +					#size-cells = <0>;
> > +				};
> > +
> > +				i2c103: i2c at 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é