Message-ID: <20250107171507.06908d71@fedora.home>
Date: Tue, 7 Jan 2025 17:15:07 +0100
From: Maxime Chevallier <maxime.chevallier@...tlin.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: Kory Maincent <kory.maincent@...tlin.com>, Oleksij Rempel
 <o.rempel@...gutronix.de>, davem@...emloft.net, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, thomas.petazzoni@...tlin.com, Andrew Lunn
 <andrew@...n.ch>, Jakub Kicinski <kuba@...nel.org>, Eric Dumazet
 <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
 linux-arm-kernel@...ts.infradead.org, Christophe Leroy
 <christophe.leroy@...roup.eu>, Herve Codina <herve.codina@...tlin.com>,
 Florian Fainelli <f.fainelli@...il.com>, Heiner Kallweit
 <hkallweit1@...il.com>, Vladimir Oltean <vladimir.oltean@....com>, Marek
 Behún <kabel@...nel.org>, Nicolò Veronese
 <nicveronese@...il.com>, Simon Horman <horms@...nel.org>,
 mwojtas@...omium.org, Antoine Tenart <atenart@...nel.org>
Subject: Re: [PATCH net-next RFC 0/5] net: phy: Introduce a port
 representation

Hi Oleksij, Köry, Russell,

Sorry for the silence so far, i'm now back from a restful holiday
break, and I'm catching-up on this discussion.

I'm replying here, but I've read the discussion so far, and thanks a
lot Oleksji for all the suggestions, it's nice to see that this is
interesting for you !

On Tue, 7 Jan 2025 15:12:20 +0000
"Russell King (Oracle)" <linux@...linux.org.uk> wrote:

> On Tue, Jan 07, 2025 at 02:26:05PM +0100, Kory Maincent wrote:
> > On Thu, 2 Jan 2025 18:03:52 +0100
> > Oleksij Rempel <o.rempel@...gutronix.de> wrote:
> >   
> > > On Thu, Jan 02, 2025 at 10:48:05AM +0000, Russell King (Oracle) wrote:  
> > > > On Sun, Dec 22, 2024 at 07:54:37PM +0100, Oleksij Rempel wrote:    
> > > > > Here is updated version:
> > > > > 
> > > > > ports {
> > > > >     /* 1000BaseT Port with Ethernet and simple PoE */
> > > > >     port0: ethernet-port@0 {
> > > > >         reg = <0>; /* Port index */
> > > > >         label = "ETH0"; /* Physical label on the device */
> > > > >         connector = "RJ45"; /* Connector type */
> > > > >         supported-modes = <10BaseT 100BaseTX 1000BaseT>; /* Supported
> > > > > modes */

So for these supported modes, this is something I've tried to avoid
actually, and for 2 reasons :

 - With the usual argument that DT is a HW description, supported-modes
doesn't work here. What matters is :

   - The medium used (is it twisted copper pairs ? backplane ? One of
the may flavors of fiber through a dedicated connector ?) and as a
complement, the number of lanes. This in itself isn't strictly
necessary. A BaseT1 PHY will obviously have only one lane on its MDI
for example. The reason I have included the lanes is mainly for BaseT,
where BaseT1 has one lane while the typical gigabit port has 4.

  I have however seen devices that have a 1G PHY connected to a RJ45
port with 2 lanes only, thus limiting the max achievable speed to 100M.
Here, we would explicietly describe the port has having 2 lanes. 

 - The supported modes that can be achieved on a port is a bit
redundant considering that we already have a great deal of capability
discovery implemented in phylib, phylink, the SFP stack, etc.

> > > > > 
> > > > >         transformer {
> > > > >             model = "ABC123"; /* Transformer model number */
> > > > >             manufacturer = "TransformerCo"; /* Manufacturer name */
> > > > > 
> > > > >             pairs {
> > > > >                 pair@0 {
> > > > >                     name = "A"; /* Pair A */
> > > > >                     pins = <1 2>; /* Connector pins */
> > > > >                     phy-mapping = <PHY_TX0_P PHY_TX0_N>; /* PHY pin
> > > > > mapping */ center-tap = "CT0"; /* Central tap identifier */
> > > > >                     pse-negative = <PSE_GND>; /* CT0 connected to GND */
> > > > >                 };
> > > > >                 pair@1 {
> > > > >                     name = "B"; /* Pair B */
> > > > >                     pins = <3 6>; /* Connector pins */
> > > > >                     phy-mapping = <PHY_RX0_P PHY_RX0_N>;
> > > > >                     center-tap = "CT1"; /* Central tap identifier */
> > > > >                     pse-positive = <PSE_OUT0>; /* CT1 connected to
> > > > > PSE_OUT0 */ };
> > > > >                 pair@2 {
> > > > >                     name = "C"; /* Pair C */
> > > > >                     pins = <4 5>; /* Connector pins */
> > > > >                     phy-mapping = <PHY_TXRX1_P PHY_TXRX1_N>; /* PHY
> > > > > connection only */ center-tap = "CT2"; /* Central tap identifier */
> > > > >                     /* No power connection to CT2 */
> > > > >                 };
> > > > >                 pair@3 {
> > > > >                     name = "D"; /* Pair D */
> > > > >                     pins = <7 8>; /* Connector pins */
> > > > >                     phy-mapping = <PHY_TXRX2_P PHY_TXRX2_N>; /* PHY
> > > > > connection only */ center-tap = "CT3"; /* Central tap identifier */
> > > > >                     /* No power connection to CT3 */
> > > > >                 };
> > > > >             };
> > > > >         };    
> > 
> > Couldn't we begin with something simple like the following and add all the
> > transformers and pairs information as you described later if the community feels
> > we need it?  
> 
> +1.

+1 as well. One thing is that I'd like to make so that describing the
port is some last-resort solution and make it really not mandatory.

That means, the internal port representation that the kernel maintains
should be able to be populated with sane defaults based on whatever
drives the port can do. As Russell says, things are working pretty well
so far, especially for single-port PHYs that can already indicated
pretty precisely what they can output.

Having a port representation in DT is useful for corner cases such as :

 - For a single-port PHY, when the PHY can't figure-out by itself what
the MDI is. For example, PHYs that can drive either copper or fiber and
that can't reliably derive the mode to use from its straps

 - For weirdnesses in the way the PHY port is wired in HW. As I
mentionned, the only case I encountered was 2 lanes on a 1G PHY, thus
limiting the speed to 100M max.

 - As an "attachment point" for things like PSE, that are really about
ports and not the PHY.

One could argue that this description isn't even needed, but OTHO with
the recent addition of vendor properties like "micrel,fiber-mode" or
"ti,fiber-mode", it appears that there's a gap to fill.

I do think that there's value in Oleksij's ideas, but this discussion
doesn't even include DT people who could help draw a line somewhere.

All that being said, if the status-quo from this discussion is "let's
keep it simple", I have some things I'd still like to discuss.

First, should we keep the lanes ?

And second, I'm confused about the way we deal with BaseX right now, as
we treat it both as a medium and as an MII mode.

If we look at 10G in comparison, we have a clear difference between the
phy_interface_mode "10GBaseR" and the linkmodes "10000BaseKR",
"10000BaseSR", etc.

We don't really have that for baseX, and it may already be too late,
but should we introduce the "1000BaseXS / 1000BaseLX / 1000BaseCX"
modes ?

Thanks everyone,

Maxime

Powered by blists - more mailing lists