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

["Köry Maincent" <kory.maincent@bootlin.com>]

On Tue, 5 Dec 2023 16:23:21 +0100
Köry Maincent <kory.maincent@bootlin.com> wrote:

> On Tue, 5 Dec 2023 15:21:47 +0100
> Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> 
> > 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.  
> 
> Just to be clear, are you saying we should use the regulator framework or is
> it simply a way of speaking as it behaves like regulator?
> 
> > > 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.  
> 
> It could be known in the of_pse_control_get() function if there is two
> phandles in the "pses" parameter. Then we add a new enum c33_pse_mode member
> in the pse_control struct to store the mode.
> PoE2 and PoE4 is not a parameter of the logical port, it depends of the number
> of PSE ports wired to an 8P8C connector. 
> 
> In fact I am also working on the tps23881 driver which aimed to be added to
> this series soon. In the tps23881 case the logical port can only be configured
> to one physical port. Two physical ports (which mean two logical ports) can
> still be used to have PoE4 mode.
> For PoE4, in the pd692x0 driver we use one logical port (one pse_control->id)
> configured to two physical ports but in the tps23881 we will need two logical
> ports (two pse_control->id).
> 
> So with the tps23881 driver we will need two phandle in the "pses" parameter
> to have PoE4, that's why my proposition seems relevant.
> 
> The same goes with your pse-regulator driver, you can't do PoE4 if two
> regulators is needed for each two pairs group.

Oleksij, what your thought for the binding I have proposed in the thread.
For the PoE4 we could add a "pses-poe4" bool property alongside the two phandle
in "pses" property.
Here is the current binding proposition:
        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 {
            ...
            };
          };
	};

....
  ethernet-phy@1 {
    reg = <1>;
    pses-poe4;
    pses = <&phy_port0, &phy_port1>;
  };
  ethernet-phy@2 {
    reg = <2>;
    pses = <&phy_port2>;
  }

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