Re: [PATCH net-next v2 7/8] dt-bindings: net: pse-pd: Add bindings for PD692x0 PSE controller

[Oleksij Rempel <o.rempel@pengutronix.de>]

On Tue, Dec 05, 2023 at 02:31:23PM +0100, Köry Maincent wrote:
> On Tue, 5 Dec 2023 11:15:01 +0100
> Köry Maincent <kory.maincent@bootlin.com> wrote:
> 
> > On Tue, 5 Dec 2023 07:36:06 +0100
> > Oleksij Rempel <o.rempel@pengutronix.de> wrote:
>  
> > > I would expect a devicetree like this:
> > > 
> > >         ethernet-pse@3c {
> > > 	  // controller compatible should be precise
> > >           compatible = "microchip,pd69210";
> > >           reg = <0x3c>;
> > >           #pse-cells = <1>;
> > >           
> > > 	  managers {
> > > 	    manager@0 {
> > > 	      // manager compatible should be included, since we are
> > > 	      // able to campare it with communication results
> > > 	      compatible = "microchip,pd69208t4"
> > > 	      // addressing corresponding to the chip select addressing
> > > 	      reg = <0>;
> > > 
> > > 	      physical-ports {
> > > 	        phys0: port@0 {
> > > 		  // each of physical ports is actually a regulator
> 
> If this phys0 is a regulator, which device will be the consumer of this
> regulator? log_port0 as the phys0 regulator consumer seems a bit odd!

Why?

> A 8P8C node should be the consumer.

PHY is not actual consumer of this regulator. State of the Ethernet PHY
is not related to the power supply. We should deliver power independent
of network interface state. There is no other local consumer we can
use in this case.

> > > 		  reg = <0>;
> > > 		};
> > > 	        phys1: port@1 {
> > > 		  reg = <1>;
> > > 		};
> > > 	        phys2: port@2 {
> > > 		  reg = <2>;
> > > 		};
> > > 
> > >                ...
> > > 	      }
> > > 
> > >           // port matrix can be calculated by using this information
> > >           logical-ports {
> > > 	    log_port0: port@0 {
> > > 	      // PoE4 port
> > > 	      physical-ports = <&phys0, &phys1>;
> > > 	    };
> > > 	    log_port1: port@1 {
> > > 	      // PoE2 port
> > > 	      physical-ports = <&phys2>;
> > > 	    };
> > > 	  };
> > > 
> > > ....
> > >    ethernet-phy@1 {
> > >      reg = <1>;
> > >      pses = <&log_port0>;
> > >    }
> > >    ethernet-phy@2 {
> > >      reg = <2>;
> > >      pses = <&log_port1>;
> > >    }  
> 
> In fact if we want to really fit the PoE architecture topology we should wait
> for the support of 8P8C connector from Maxime. Then it will look like that:
> SoC  --- i2c/uart --> controller -- spi --> manager0 -- phys_port0 --> 8P8C_connector0 (PoE4)
>                                          |           \- phys_port1 --> 8P8C_connector0 (PoE4)
> 					 |	     \- phys_port2 --> 8P8C_connector1 (PoE2)
> 					 |	     \- phys_port3 --> 8P8C_connector2 (PoE2)
>                                          \- manager1 -- phys_port0 ..
> 
> With this type of devicetree:
>         ethernet-pse@3c {
> 	  // controller compatible should be precise
>           compatible = "microchip,pd69210";
>           reg = <0x3c>;
>           #pse-cells = <1>;
>           
> 	  managers {
> 	    manager@0 {
> 	      // manager compatible should be included, since we are
> 	      // able to compare it with communication results
> 	      compatible = "microchip,pd69208t4"
> 	      // addressing corresponding to the chip select addressing
> 	      reg = <0>;
> 
> 	      physical-ports {
> 	        phys_port0: port@0 {
> 		  // each of physical ports is actually a regulator
> 		  reg = <0>;
> 		};
> 	        phy_port1: port@1 {
> 		  reg = <1>;
> 		};
> 	        phy_port2: port@2 {
> 		  reg = <2>;
> 		};
> 
>                ...
> 	      }
> 	    manager@1 {
>             ...
>             };
>           };
> 	};
> 
> ....
>   rj45_0:8p8c@0 {
>     pses = <&phy_port0, &phy_port1>;
>   };
>   rj45_1:8p8c@1 {
>     pses = <&phy_port2>;
>   };
>   ethernet-phy@1 {
>     reg = <1>;
>     connector = <&rj45_0>;
>   };
>   ethernet-phy@2 {
>     reg = <2>;
>     connector = <&rj45_1>;
>   }
> 
> 
> The drawback is that I don't really know for now, how port matrix can be
> calculated with this. Maybe by adding a "conf_pse_cell()" callback, call
> in the of_pse_control_get() for each ports.
> 
> For now 8p8c connector are not supported, we could keep using pse phandle
> in the phy node like you described but for physical port:
>    ethernet-phy@1 {
>      reg = <1>;
>      pses = <&phy_port0, &phy_port1>;
>    }
>    ethernet-phy@2 {
>      reg = <2>;
>      pses = <&phy_port2>;
>    }
> 
> 
> 
> Finally, the devicetree would not know the matrix between logical port and
> physical port, this would be cleaner.
> 
> Did I miss something?

In case different PSE suppliers are linked withing the PHY node, we
loose most of information needed for PSE functionality. For example how
we will know if our log_port supports PoE4 and PoE2 mode, or only PoE2.
This information is vital for proper PSE configuration, this is why I
suggested to have logica-ports subnodes. With the price of hawing huge
DT on a switch with 48 ports.

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |