Message-ID:
 <PH0PR22MB3809373A2119097A3D2484DFE5242@PH0PR22MB3809.namprd22.prod.outlook.com>
Date: Fri, 15 Nov 2024 23:51:03 +0000
From: Robert Joslyn <robert_joslyn@...inc.com>
To: Maxime Chevallier <maxime.chevallier@...tlin.com>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "lee@...nel.org"
	<lee@...nel.org>,
        Romain Gantois <romain.gantois@...tlin.com>,
        Herve Codina
	<herve.codina@...tlin.com>
Subject: RE: [RFC PATCH 2/2] net: selpcimac: Add driver for SEL PCIe network
 adapter

> > > I haven't reviewed the code itself as this is a biiiiig patch, I
> > > suggest you try to split it into more digestable patches, focusing
> > > on individual aspects of the driver.
> > >
> > > One thing is the PHY support as you mention in the cover-letter, in
> > > the current state this driver re-implements PHY drivers from what I
> > > understand. You definitely need to use the kernel infra for PHY handling.
> > >
> > > As it seems this driver also re-implements SFP entirely, I suggest
> > > you look into phylink [1]. This will help you supporting the PHYs and SFPs.
> > > You can take a look at the mvneta.c and mvpp2 drivers for examples.
> >
> > I've been working through migrating to phylib and phylink, and I have the
> simple case of copper ports working. Where I've gotten stuck is in trying to
> handle SFPs due to how the hardware is implemented.
> >
> > This hardware is a PCIe card, either as a typical add-on card or embedded on
> the mainboard of an x86 computer. The card is setup as follows:
> >
> > PCIe Bus <--> FPGA MAC <--> PHY <--> Copper or SFP cage
> >
> > The phy can be one of three different phys, a BCM5482, Marvell M88E1510,
> or a TI DP83869. The interface between MAC and PHY is always RGMII. The
> MAC doesn't know if the port is copper or SFP until an SFP is plugged in. The
> RFC patch, which has fully internal PHY/SFP handling, assumes the port is
> copper until an SFP is detected via an interrupt. When that interrupt is
> received, it probes the SFP over the I2C bus through the FPGA to determine
> the SFP type, then reconfigures the PHY as needed for that type of SFP.
> 
> So do you have 2 different layouts possible, or are you in a situation where the
> RJ45 copper port AND the SFP are always wired to the PHY, and you perform
> media detection to chose the interface to use ?

Mostly the second. The PCIe card can be configured as 4 RJ45 copper ports, 4 SFP
ports, or a combination of 2 RJ45 and 2 SFP ports. The PCB is the same between
them and has pads/holes for both an RJ45 and SFP cage for each port, but only
one of them is actually populated, so the only physical difference is whether the
RJ45 port is soldered to the board or an SFP cage for a given port. The output of
the PHY is connected to both sets of pads/holes.

Because of this layout, the driver/MAC can't tell the difference between a port
that has an RJ45 soldered and one that has an empty SFP cage. If the port is RJ45
copper, the FPGA never senses an SFP so we always treat that case as copper. If
the port is an SFP cage, we only know it once an SFP is inserted and causes a
module presence pin to change state, which fires the interrupt the driver sees.
That interrupt is the driver's signal to probe the SFP and reconfigure the PHY for
whatever mode is needed.

> > After porting to phylink, in the copper case, the PHY gets configured correctly
> and it works. In the SFP case, I don't know how to reconfigure the PHY to act
> as a media converter with the correct interface for whatever kind of SFP is
> attached. The M88E1510 driver, for example, seems to have support for this in
> the form of struct sfp_upstream_ops callbacks
> (https://urldefense.com/v3/__https://elixir.bootlin.com/linux/v6.12-
> rc7/source/drivers/net/phy/marvell.c*L3611__;Iw!!O7uE89YCNVw!MvrH2lYi
> 3aIcC63u48SePatqavaBlTe1zVqB121oIdmn7YXJzbaw8ZN0yXQs6cNhlaIJWdA
> mwTzgypDVWjTZdtA5o7LUnoLY$ ). It looks like phylink_create will make use
> of that by looking at the fwnode passed in, but I don't know how to use that
> to define the layout of my hardware. I assume this is mainly used with device
> tree and that would define the topology, but I'm using a PCI device on x86.
> The Broadcom and TI phys don't have the sfp_upstream_ops support as far as
> I can see, so I've focused on the Marvell phy for the time being.
> 
> There's ongoing work to get the DP83869 to support SFP downstream
> interfaces done by Romain Gantois (in CC) :
> https://urldefense.com/v3/__https://lore.kernel.org/netdev/20240701-b4-
> dp83869-sfp-v1-0-
> a71d6d0ad5f8@...tlin.com/__;!!O7uE89YCNVw!MvrH2lYi3aIcC63u48SePa
> tqavaBlTe1zVqB121oIdmn7YXJzbaw8ZN0yXQs6cNhlaIJWdAmwTzgypDVWjTZ
> dtA5oxeQAG6H$
> 
> > How do I describe my hardware layout such that phylink can see that there is
> an SFP attached and communicate with it? Is there a way to manually create
> the fwnodes that phylink_create and other functions use? I think this would
> need to show the topology of the MAC -> PHY -> SFP interface, as well as the
> I2C bus to use to talk to the SFP (I would have to expose the I2C bus, it's
> presently internal to this driver).
> 
> There are a few other devices in this case.
> 
> One approach is to describe the SFP cage in your driver using the swnode API,
> such an approach was considered for the LAN743x in PCI mode :
> 
> https://urldefense.com/v3/__https://lore.kernel.org/netdev/202409111610
> 54.4494-3-
> Raju.Lakkaraju@...rochip.com/__;!!O7uE89YCNVw!MvrH2lYi3aIcC63u48Se
> PatqavaBlTe1zVqB121oIdmn7YXJzbaw8ZN0yXQs6cNhlaIJWdAmwTzgypDVWj
> TZdtA5o-Bs6l4K$
> 
> Another approach that is considered is to load a DT overlay that describes the
> hardware including mdio busses, i2c busses, PHYs, SFPs, etc. when the PCI
> driver is used. See this patchset here :
> 
> https://urldefense.com/v3/__https://lore.kernel.org/netdev/202410141246
> 36.24221-1-
> herve.codina@...tlin.com/__;!!O7uE89YCNVw!MvrH2lYi3aIcC63u48SePatq
> avaBlTe1zVqB121oIdmn7YXJzbaw8ZN0yXQs6cNhlaIJWdAmwTzgypDVWjTZdt
> A5o5hG-_OR$
> 
> Hopefully this will help you a bit in the process of figuring this out, agreed
> that's not an easy task :)
> 
> Best regards,
> 
> Maxime

I'll read through these links and see what I can do, looks like there's some options I
can explore.

Thanks!
Robert

Powered by blists - more mailing lists