Message-ID: <0f4ec2ff-4ef7-4667-adef-d065cfbc0a91@gmail.com>
Date: Sun, 7 Jan 2024 23:49:12 +0200
From: Sergey Ryazanov <ryazanov.s.a@...il.com>
To: Christian Marangi <ansuelsmth@...il.com>
Cc: Robert Marko <robert.marko@...tura.hr>, Andrew Lunn <andrew@...n.ch>,
 Vladimir Oltean <olteanv@...il.com>, Rob Herring <robh+dt@...nel.org>,
 Luo Jie <quic_luoj@...cinc.com>, "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
 Conor Dooley <conor+dt@...nel.org>, Andy Gross <agross@...nel.org>,
 Bjorn Andersson <andersson@...nel.org>,
 Konrad Dybcio <konrad.dybcio@...aro.org>,
 Heiner Kallweit <hkallweit1@...il.com>, Russell King
 <linux@...linux.org.uk>, Matthias Brugger <matthias.bgg@...il.com>,
 AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
 netdev@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-mediatek@...ts.infradead.org
Subject: Re: [net-next PATCH RFC v3 1/8] dt-bindings: net: document ethernet
 PHY package nodes

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@...il.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@...il.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.

>> And I would like to ask you about another issue raised by Vladimir [1].
>> These phy chips become SoC with all these built-in PHYs, PCSs, clocks,
>> interrupt controllers, etc. Should we address this now? Or should we go with
>> the proposed solution for now and postpone modeling of other peripherals
>> until we get a real hardware, as Andrew suggested?
> 
> Honestly I would postpone untile we have a clear idea of what is
> actually part of the PHY and what can be handled externally... Example
> setting the clock in gcc, writing a specific driver...
> 
> It's a random idea but maybe most of the stuff required for that PHY is
> just when it's connected to a switch... In that case it would all be
> handled in the switch driver (tobe extended qca8k) and all these extra
> stuff would be placed in that node instead of bloating phy nodes with
> all kind of clk and other stuff.
> 
> This series still require 2 more series (at803x splint and cleanup) to be
> actually proposed so we have some time to better define this.
> 
> What do you think?

Fair enough! Let's postpone until we really need it. I noticed this 
PHY-like-SoC discussion in the V1 comments, and it was not finished 
there neither addressed in the latest patch comment. So I asked just to 
be sure that we were finished with this. Thank you for the clarification.

--
Sergey

Powered by blists - more mailing lists