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

From: Köry Maincent
Date: Fri Dec 08 2023 - 04:06:36 EST


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

> On Tue, 5 Dec 2023 15:21:47 +0100
> Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx> 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@xxxxxxxxxxx> wrote:
> > >
> > > > On Tue, 5 Dec 2023 07:36:06 +0100
> > > > Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx> 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