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

[Sergey Ryazanov <ryazanov.s.a@gmail.com>]

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.

> 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