Re: [net-next PATCH RFC v3 1/8] dt-bindings: net: document ethernet PHY package nodes

[Christian Marangi <ansuelsmth@gmail.com>]

On Wed, Jan 17, 2024 at 09:39:25PM +0200, Sergey Ryazanov wrote:
> Hi Christian,
> 
> On 17.01.2024 02:36, Christian Marangi wrote:
> > On Sun, Jan 07, 2024 at 11:49:12PM +0200, Sergey Ryazanov wrote:
> > > Hi Christian,
> > > 
> > > On 07.01.2024 20:30, Christian Marangi wrote:
> > > > On Sun, Jan 07, 2024 at 08:00:33PM +0200, Sergey Ryazanov wrote:
> > > > > On 26.11.2023 03:53, Christian Marangi wrote:
> > > > > > Document ethernet PHY package nodes used to describe PHY shipped in
> > > > > > bundle of 4-5 PHY. The special node describe a container of PHY that
> > > > > > share common properties. This is a generic schema and PHY package
> > > > > > should create specialized version with the required additional shared
> > > > > > properties.
> > > > > > 
> > > > > > Example are PHY package that have some regs only in one PHY of the
> > > > > > package and will affect every other PHY in the package, for example
> > > > > > related to PHY interface mode calibration or global PHY mode selection.
> > > > > > 
> > > > > > The PHY package node MUST declare the base address used by the PHY driver
> > > > > > for global configuration by calculating the offsets of the global PHY
> > > > > > based on the base address of the PHY package and declare the
> > > > > > "ethrnet-phy-package" compatible.
> > > > > > 
> > > > > > Each reg of the PHY defined in the PHY package node is absolute and will
> > > > > > reference the real address of the PHY on the bus.
> > > > > > 
> > > > > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > > > > > ---
> > > > > >     .../bindings/net/ethernet-phy-package.yaml    | 75 +++++++++++++++++++
> > > > > >     1 file changed, 75 insertions(+)
> > > > > >     create mode 100644 Documentation/devicetree/bindings/net/ethernet-phy-package.yaml
> > > > > > 
> > > > > > diff --git a/Documentation/devicetree/bindings/net/ethernet-phy-package.yaml b/Documentation/devicetree/bindings/net/ethernet-phy-package.yaml
> > > > > > new file mode 100644
> > > > > > index 000000000000..244d4bc29164
> > > > > > --- /dev/null
> > > > > > +++ b/Documentation/devicetree/bindings/net/ethernet-phy-package.yaml
> > > > > > @@ -0,0 +1,75 @@
> > > > > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > > > > > +%YAML 1.2
> > > > > > +---
> > > > > > +$id: http://devicetree.org/schemas/net/ethernet-phy-package.yaml#
> > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > > +
> > > > > > +title: Ethernet PHY Package Common Properties
> > > > > > +
> > > > > > +maintainers:
> > > > > > +  - Christian Marangi <ansuelsmth@gmail.com>
> > > > > > +
> > > > > > +description:
> > > > > > +  This schema describe PHY package as simple container for
> > > > > > +  a bundle of PHYs that share the same properties and
> > > > > > +  contains the PHYs of the package themself.
> > > > > > +
> > > > > > +  Each reg of the PHYs defined in the PHY package node is
> > > > > > +  absolute and describe the real address of the PHY on the bus.
> > > > > > +
> > > > > > +properties:
> > > > > > +  $nodename:
> > > > > > +    pattern: "^ethernet-phy-package(@[a-f0-9]+)?$"
> > > > > > +
> > > > > > +  compatible:
> > > > > > +    const: ethernet-phy-package
> > > > > > +
> > > > > > +  reg:
> > > > > > +    minimum: 0
> > > > > > +    maximum: 31
> > > > > > +    description:
> > > > > > +      The base ID number for the PHY package.
> > > > > > +      Commonly the ID of the first PHY in the PHY package.
> > > > > > +
> > > > > > +      Some PHY in the PHY package might be not defined but
> > > > > > +      still exist on the device (just not attached to anything).
> > > > > > +      The reg defined in the PHY package node might differ and
> > > > > > +      the related PHY might be not defined.
> > > > > > +
> > > > > > +  '#address-cells':
> > > > > > +    const: 1
> > > > > > +
> > > > > > +  '#size-cells':
> > > > > > +    const: 0
> > > > > > +
> > > > > > +patternProperties:
> > > > > > +  ^ethernet-phy(@[a-f0-9]+)?$:
> > > > > > +    $ref: ethernet-phy.yaml#
> > > > > > +
> > > > > > +required:
> > > > > > +  - compatible
> > > > > > +  - reg
> > > > > > +
> > > > > > +additionalProperties: true
> > > > > > +
> > > > > > +examples:
> > > > > > +  - |
> > > > > > +    mdio {
> > > > > > +        #address-cells = <1>;
> > > > > > +        #size-cells = <0>;
> > > > > > +
> > > > > > +        ethernet-phy-package@16 {
> > > > > > +            #address-cells = <1>;
> > > > > > +            #size-cells = <0>;
> > > > > > +            compatible = "ethernet-phy-package";
> > > > > > +            reg = <0x16>;
> > > > > > +
> > > > > > +            ethernet-phy@16 {
> > > > > > +              reg = <0x16>;
> > > > > > +            };
> > > > > > +
> > > > > > +            phy4: ethernet-phy@1a {
> > > > > > +              reg = <0x1a>;
> > > > > > +            };
> > > > > > +        };
> > > > > > +    };
> > > > > 
> > > > > So, we ended up on a design where we use the predefined compatible string
> > > > > 'ethernet-phy-package' to recognize a phy package inside the
> > > > > of_mdiobus_register() function. During the V1 discussion, Vladimir came up
> > > > > with the idea of 'ranges' property usage [1]. Can we use 'ranges' to
> > > > > recognize a phy package in of_mdiobus_register()? IMHO this will give us a
> > > > > clear DT solution. I mean 'ranges' clearly indicates that child nodes are in
> > > > > the same address range as the parent node. Also we can list all child
> > > > > addresses in 'reg' to mark them occupied.
> > > > > 
> > > > >     mdio {
> > > > >       ...
> > > > > 
> > > > >       ethernet-phy-package@16 {
> > > > >         compatible = "qcom,qca8075";
> > > > >         reg = <0x16>, <0x17>, <0x18>, <0x19>, <0x1a>;
> > > > >         ranges;
> > > > >         ...
> > > > > 
> > > > >         ethernet-phy@16 {
> > > > >           reg = <0x16>;
> > > > >         };
> > > > > 
> > > > >         ethernet-phy@1a {
> > > > >           reg = <0x1a>;
> > > > >         };
> > > > >       };
> > > > >     };
> > > > > 
> > > > > Did you find some issues with the 'ranges' conception?
> > > > 
> > > > Nope it's ok but it might pose some confusion with the idea that the
> > > > very first element MUST be THE STARTING ADDR of the PHY package. (people
> > > > might think that it's just the list of the PHYs in the package and
> > > > remove the hardware unconnected ones... but that would be fault of who
> > > > write the DT anyway.)
> > > 
> > > Make sense. I do not insist on addresses listing. Mainly I'm thinking of a
> > > proper way to show that child nodes are accessible directly on the parent
> > > bus, and introducing the special compatibility string, while we already have
> > > the 'ranges' property.
> > > 
> > > But it's good to know Rob's opinion on whether it is conceptually right to
> > > use 'ranges' here.
> > > 
> > 
> > I wonder if something like this might make sense... Thing is that with
> > the ranges property we would have the define the address in the PHY
> > Package node as offsets...
> > 
> > An example would be
> > 
> >      mdio {
> >          #address-cells = <1>;
> >          #size-cells = <0>;
> > 
> >          ethernet-phy-package@10 {
> >              #address-cells = <1>;
> >              #size-cells = <0>;
> >              compatible = "qcom,qca807x-package";
> >              reg = <0x10>;
> >              ranges = <0x0 0x10 0x5>;
> > 
> >              qcom,package-mode = "qsgmii";
> > 
> >              ethernet-phy@0 {
> >                  reg = <0>;
> > 
> >                  leds {
> > 
> > 		...
> > 
> > With a PHY Package at 0x10, that span 5 address and the child starts at
> > 0x0 offset.
> > 
> > This way we would be very precise on describing the amount of address
> > used by the PHY Package without having to define the PHY not actually
> > connected.
> > 
> > PHY needs to be at an offset to make sense of the ranges first element
> > property (0x0). With a non offset way we would have to have something
> > like
> > 
> > ranges = <0x10 0x10 0x5>;
> > 
> > With the child and tha parent always matching.
> > 
> > (this is easy to handle in the parsing and probe as we will just
> > calculate the real address based on the base address of the PHY package
> > + offset)
> 
> On one hand it makes sense and looks useful for software development. On
> another, it looks like a violation of the main DT designing rule, when DT
> should be used to describe that hardware properties, which can not be learnt
> from other sources.
> 
> As far as I understand this specific chip, each of embedded PHYs has its own
> MDIO bus address and not an offset from a main common address. Correct me
> please, if I am got it wrong.
>

Correct, it's a quad PHY but each PHY have it's own address and they
can't be changed (they are incremental from the base one)

We have lots of device that have this (it's present in 99% of the the
ipq40xx and ipq807x SoC) and the common setup is putting the quad PHY
and connect only some port to it. (the address are occupiaed anyway just
the ethernet port is not connected)

Honestly I don't think it's a violation, IMHO quite the opposite. If DT
is there to describe the hardware, defining each PHY of the package as a
separate one is wrong and actually cause confusion by dropping entirely
any description that they are indeed in a package and that on the BUS
there may be address used without anything connected.

I'm more convinced with the ranges property as we can't put a size to
the reg property. (due to mdio node having address-cell to 0, changing
this will affect also every other phy node and that is problematic)

> > I hope Rob can give more feedback about this, is this what you were
> > thinking with the usage of ranges property?
> > 
> > (this has also the bonus point of introducing some validation in the PHY
> > core code to make sure the right amount of PHY are defined in the
> > package by checking if the number of PHY doesn't exceed the value set in
> > ranges.)
> 
> Yep, I am also would like to hear some clarification from Rob regarding
> acceptable 'range' property usage and may be some advice on how to specify
> the size of occupied addresses. Rob?
> 
> --
> Sergey

-- 
	Ansuel