Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Date:   Fri, 8 Dec 2023 10:06:27 +0100
From:   =?UTF-8?B?S8O2cnk=?= Maincent <kory.maincent@bootlin.com>
To:     Oleksij Rempel <o.rempel@pengutronix.de>
Cc:     "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Jonathan Corbet <corbet@lwn.net>,
        Luis Chamberlain <mcgrof@kernel.org>,
        Russ Weight <russ.weight@linux.dev>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-doc@vger.kernel.org, devicetree@vger.kernel.org,
        Dent Project <dentproject@linuxfoundation.org>,
        Liam Girdwood <lgirdwood@gmail.com>,
        Mark Brown <broonie@kernel.org>
Subject: Re: [PATCH net-next v2 7/8] dt-bindings: net: pse-pd: Add bindings
 for PD692x0 PSE controller
Message-ID: <20231208100627.2a78e720@kmaincent-XPS-13-7390>
In-Reply-To: <20231205162321.4bd165eb@kmaincent-XPS-13-7390>
References: <20231201-feature_poe-v2-0-56d8cac607fa@bootlin.com>
 <20231201-feature_poe-v2-7-56d8cac607fa@bootlin.com>
 <20231204230845.GH981228@pengutronix.de>
 <20231205063606.GI981228@pengutronix.de>
 <20231205111501.43f80846@kmaincent-XPS-13-7390>
 <20231205143123.703589c8@kmaincent-XPS-13-7390>
 <20231205142147.GL981228@pengutronix.de>
 <20231205162321.4bd165eb@kmaincent-XPS-13-7390>
Organization: bootlin
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

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

> On Tue, 5 Dec 2023 15:21:47 +0100
> Oleksij Rempel <o.rempel@pengutronix.de> wrote:
>=20
> > On Tue, Dec 05, 2023 at 02:31:23PM +0100, K=C3=B6ry Maincent wrote: =20
> > > On Tue, 5 Dec 2023 11:15:01 +0100
> > > K=C3=B6ry Maincent <kory.maincent@bootlin.com> wrote:
> > >    =20
> > > > On Tue, 5 Dec 2023 07:36:06 +0100
> > > > Oleksij Rempel <o.rempel@pengutronix.de> wrote:   =20
> > >     =20
> > > > > I would expect a devicetree like this:
> > > > >=20
> > > > >         ethernet-pse@3c {
> > > > > 	  // controller compatible should be precise
> > > > >           compatible =3D "microchip,pd69210";
> > > > >           reg =3D <0x3c>;
> > > > >           #pse-cells =3D <1>;
> > > > >          =20
> > > > > 	  managers {
> > > > > 	    manager@0 {
> > > > > 	      // manager compatible should be included, since we are
> > > > > 	      // able to campare it with communication results
> > > > > 	      compatible =3D "microchip,pd69208t4"
> > > > > 	      // addressing corresponding to the chip select
> > > > > addressing reg =3D <0>;
> > > > >=20
> > > > > 	      physical-ports {
> > > > > 	        phys0: port@0 {
> > > > > 		  // each of physical ports is actually a regulator
> > > > > =20
> > >=20
> > > If this phys0 is a regulator, which device will be the consumer of th=
is
> > > regulator? log_port0 as the phys0 regulator consumer seems a bit odd!=
   =20
> >=20
> > Why?
> >  =20
> > > A 8P8C node should be the consumer.   =20
> >=20
> > 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. =20
>=20
> 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?
>=20
> > > Finally, the devicetree would not know the matrix between logical por=
t and
> > > physical port, this would be cleaner.
> > >=20
> > > Did I miss something?   =20
> >=20
> > 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. =20
>=20
> 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 mem=
ber
> in the pse_control struct to store the mode.
> PoE2 and PoE4 is not a parameter of the logical port, it depends of the n=
umber
> of PSE ports wired to an 8P8C connector.=20
>=20
> 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 confi=
gured
> to one physical port. Two physical ports (which mean two logical ports) c=
an
> 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 log=
ical
> ports (two pse_control->id).
>=20
> So with the tps23881 driver we will need two phandle in the "pses" parame=
ter
> to have PoE4, that's why my proposition seems relevant.
>=20
> 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 pha=
ndle
in "pses" property.
Here is the current binding proposition:
        ethernet-pse@3c {
	  // controller compatible should be precise
          compatible =3D "microchip,pd69210";
          reg =3D <0x3c>;
          #pse-cells =3D <1>;
         =20
	  managers {
	    manager@0 {
	      // manager compatible should be included, since we are
	      // able to compare it with communication results
	      compatible =3D "microchip,pd69208t4"
	      // addressing corresponding to the chip select addressing
	      reg =3D <0>;

	      physical-ports {
	        phys_port0: port@0 {
		  // each of physical ports is actually a regulator
		  reg =3D <0>;
		};
	        phy_port1: port@1 {
		  reg =3D <1>;
		};
	        phy_port2: port@2 {
		  reg =3D <2>;
		};

               ...
	      }
	    manager@1 {
            ...
            };
          };
	};

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

Regards,
--=20
K=C3=B6ry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com