Re: [PATCH net-next 03/13] net: phy: Introduce PHY ports representation

[Kory Maincent <kory.maincent@bootlin.com>]

On Tue, 11 Feb 2025 14:42:43 +0100
Maxime Chevallier <maxime.chevallier@bootlin.com> wrote:

> Hi Köry,
> 
> On Tue, 11 Feb 2025 14:32:09 +0100
> Kory Maincent <kory.maincent@bootlin.com> wrote:
> 
> > On Fri,  7 Feb 2025 23:36:22 +0100
> > Maxime Chevallier <maxime.chevallier@bootlin.com> wrote:
> >   
> > > Ethernet provides a wide variety of layer 1 protocols and standards for
> > > data transmission. The front-facing ports of an interface have their own
> > > complexity and configurability.
> > > 
> > > Introduce a representation of these front-facing ports. The current code
> > > is minimalistic and only support ports controlled by PHY devices, but
> > > the plan is to extend that to SFP as well as raw Ethernet MACs that
> > > don't use PHY devices.
> > > 
> > > This minimal port representation allows describing the media and number
> > > of lanes of a port. From that information, we can derive the linkmodes
> > > usable on the port, which can be used to limit the capabilities of an
> > > interface.
> > > 
> > > For now, the port lanes and medium is derived from devicetree, defined
> > > by the PHY driver, or populated with default values (as we assume that
> > > all PHYs expose at least one port).
> > > 
> > > The typical example is 100M ethernet. 100BaseT can work using only 2
> > > lanes on a Cat 5 cables. However, in the situation where a 10/100/1000
> > > capable PHY is wired to its RJ45 port through 2 lanes only, we have no
> > > way of detecting that. The "max-speed" DT property can be used, but a
> > > more accurate representation can be used :
> > > 
> > > mdi {
> > > 	port@0 {
> > > 		media = "BaseT";
> > > 		lanes = <2>;
> > > 	};
> > > };
> > > 
> > > From that information, we can derive the max speed reachable on the
> > > port.
> > > 
> > > Another benefit of having that is to avoid vendor-specific DT properties
> > > (micrel,fiber-mode or ti,fiber-mode).
> > > 
> > > This basic representation is meant to be expanded, by the introduction
> > > of port ops, userspace listing of ports, and support for multi-port
> > > devices.    
> > 
> > This patch is tackling the support of ports only for the PHY API. Keeping in
> > mind that this port abstraction support will also be of interest to the
> > NICs. Isn't it preferable to handle port in a standalone API?  
> 
> The way I see it, nothing prevents from using the port definition in
> ethernet-port.yml in DSA/raw nics.
> 
> > With net drivers having PHY managed by the firmware or DSA, there is no
> > linux description of their PHYs. On that case, if we want to use port
> > abstraction, what is the best? Register a virtual phy_device to use the
> > abstraction port or use the port abstraction API directly which meant that
> > it is not related to any PHY?  
> 
> I think the next steps will be to have net_device have a list of ports
> (maintained in the phy_link_topology) that aggregates ports from all
> its PHYs/SFPs/raw interfaces. in that case net_device will be the
> direct parent. I haven't worked on the bindings for that though,
> especially for DSA :'(

Having it under phy_link_topology is a great idea!
 
> I don't think the virtual phydev is going to be helpful. I'm hitting
> the 15 patches limit, but a possible extension is to make so that
> phylink also creates a port when it finds an SFP (hence, when upstream
> is a MAC).

I would say not only for SFP but phylink should create a port when it can find
a mdi description in the devicetree. Port with PoE, leds or whatever future
supported features should be created by phylink. 

> This is why phy_port has these fields :
> 
> 
> enum phy_port_parent {
> 	PHY_PORT_PHY,
> };
> 
> struct phy_port {
> 	...
> 	enum phy_port_parent parent_type;
> 	union {
> 		struct phy_device *phy;
> 	};
> 
> };
> 
> The parent type may (will) be extended with PORT_PHY_MAC, and that's
> also why the parent pointer is in a union :)

Ok for me!
 
> I'm trying hard to make so that phy_port doesn't depend on phylib
> (altough, phylib depends on phy_port). There's a dependency on some
> core stuff (converting from medium => linkmodes) and phylink
> (converting the interfaces list to linkmodes), but we can extract these
> fairly easily.
> 
> You're correct in that for now, the integration is with phylib only
> though, but let's make sure this will also work for phy-less devices.
> 
> Thanks a lot for your input,

Thanks for your work, it will be really helpful to add support for PoE in DSA. 

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com