Re: [PATCH net-next RFC 0/5] net: phy: Introduce a port representation

["Russell King (Oracle)" <linux@armlinux.org.uk>]

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@pengutronix.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 */
> > > > 
> > > >         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.

> mdis {
> 
>     /* 1000BaseT Port with Ethernet and PoE */
>     mdi0: ethernet-mdi@0 {
>         reg = <0>; /* Port index */
>         label = "ETH0"; /* Physical label on the device */
>         connector = "RJ45"; /* Connector type */
>         supported-modes = <10BaseT 100BaseTX 1000BaseT>; /* Supported modes */
>         lanes = <2>;
>         variant = "MDI-X"; /* MDI or MDI-X */
>         pse = <&pse1>;
>     };
> };

We already manage well enough without anything like this level of
detail of a RJ45 socket, so why don't we start off with something
very simple. I'm thinking that even giving the supported-modes
argument is too much here - have we *ever* had the case where an
ethernet port can't support all the speeds that it's associated
PHY supports?

> We can also add led, thermal and fuse subnodes later.
> Let's begin with something simple for the initial support, considering
> that it has places for additional details in the future.

What I think we both fear is having a complex DT description of a
port that the kernel mostly ignores. While we can come out with the
"but DT describes the hardware" claptrap, it's no good trying to
describe the hardware in a firmware description unless there is some
way to validate that the firmware description is correct - which
means there must be something that depends on it in order to work.

If we describe stuff that doesn't get used, there's no way to know
if it is actually correct. We then end up with a lot of buggy DT
descriptions with properties that can't be relied upon to be
correct, and that makes those properties utterly useless.

I'm sure DT maintainers will disagree due to the "DT describes the
hardware" but... I've said it here, and if we end up with stuff
over-described wrongly creating a mess, I'll be able to point back
at this!

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!