Message-ID: <20241115101901.4369e0da@fedora.home>
Date: Fri, 15 Nov 2024 10:19:01 +0100
From: Maxime Chevallier <maxime.chevallier@...tlin.com>
To: Robert Joslyn <robert_joslyn@...inc.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

Hello Robert,

> > 
> > 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 ?

> 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://elixir.bootlin.com/linux/v6.12-rc7/source/drivers/net/phy/marvell.c#L3611). 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://lore.kernel.org/netdev/20240701-b4-dp83869-sfp-v1-0-a71d6d0ad5f8@bootlin.com/

> 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://lore.kernel.org/netdev/20240911161054.4494-3-Raju.Lakkaraju@microchip.com/

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://lore.kernel.org/netdev/20241014124636.24221-1-herve.codina@bootlin.com/

Hopefully this will help you a bit in the process of figuring this out,
agreed that's not an easy task :)

Best regards,

Maxime

Powered by blists - more mailing lists