diff mbox series

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

Message ID 20231126015346.25208-2-ansuelsmth@gmail.com (mailing list archive)
State New, archived
Headers show
Series net: phy: Support DT PHY package | expand

Commit Message

Christian Marangi Nov. 26, 2023, 1:53 a.m. UTC
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

Comments

Rob Herring (Arm) Nov. 27, 2023, 10:16 p.m. UTC | #1
On Sun, Nov 26, 2023 at 02:53:39AM +0100, 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

In case I wasn't clear, but that compatible is a NAK.

> +
> +  reg:
> +    minimum: 0
> +    maximum: 31

Pretty sure the bus binding already provides these constraints.

> +    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

You are implementing a secondary MDIO bus within this node. It needs a 
$ref to mdio.yaml instead of defining the bus again implicitly.

> +
> +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>;
> +            };

This example on its own doesn't make sense. It can't be fully validated 
because you allow any additional properties. Drop it.

> +        };
> +    };
> -- 
> 2.40.1
>
Andrew Lunn Nov. 28, 2023, 12:09 a.m. UTC | #2
> > +    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
> 
> You are implementing a secondary MDIO bus within this node. It needs a 
> $ref to mdio.yaml instead of defining the bus again implicitly.

This is where i think this is questionable. It is not implemented in
the kernel as a secondary bus. The devices within this container are
just devices on the MDIO bus. The value of reg inside the container
and outside the container refer to the same bus.

However, i do agree about referring to mdio.yaml inside the container.

> > +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";

Christian, this needs a specific compatible to the
package. e.g. 'qca807x-package', and that needs its own .yaml file
indicating what properties this package can have.

	   Andrew
Sergey Ryazanov Jan. 7, 2024, 6 p.m. UTC | #3
Hi Christian and Robert,

thank you for this great work!

Let me ask a couple of questions regarding the phy package conception. 
Please find them below.

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?


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?

I'm asking because it looks like we have got a real hardware. Luo 
currently trying to push QCA8084 (multi-phy/switch chip) support, and 
this chip exactly contains a huge clock/reset controller [2,3].


1. https://lore.kernel.org/lkml/20231124165923.p2iozsrnwlogjzua@skbuf // 
Re: [net-next RFC PATCH 03/14] dt-bindings: net: document ethernet PHY 
package nodes
2. 
https://lore.kernel.org/lkml/20231215074005.26976-1-quic_luoj@quicinc.com 
  // [PATCH v8 00/14] add qca8084 ethernet phy driver
3. 
https://lore.kernel.org/lkml/20231104034858.9159-1-quic_luoj@quicinc.com 
  // [PATCH v12 0/4] add clock controller of qca8386/qca8084

--
Sergey
Christian Marangi Jan. 7, 2024, 6:30 p.m. UTC | #4
On Sun, Jan 07, 2024 at 08:00:33PM +0200, Sergey Ryazanov wrote:
> Hi Christian and Robert,
> 
> thank you for this great work!
> 
> Let me ask a couple of questions regarding the phy package conception.
> Please find them below.
> 
> 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.)

> 
> 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?

> I'm asking because it looks like we have got a real hardware. Luo currently
> trying to push QCA8084 (multi-phy/switch chip) support, and this chip
> exactly contains a huge clock/reset controller [2,3].
> 
> 
> 1. https://lore.kernel.org/lkml/20231124165923.p2iozsrnwlogjzua@skbuf // Re:
> [net-next RFC PATCH 03/14] dt-bindings: net: document ethernet PHY package
> nodes
> 2. https://lore.kernel.org/lkml/20231215074005.26976-1-quic_luoj@quicinc.com
> // [PATCH v8 00/14] add qca8084 ethernet phy driver
> 3. https://lore.kernel.org/lkml/20231104034858.9159-1-quic_luoj@quicinc.com
> // [PATCH v12 0/4] add clock controller of qca8386/qca8084
> 
> --
> Sergey
Andrew Lunn Jan. 7, 2024, 6:31 p.m. UTC | #5
> 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?
> 
> I'm asking because it looks like we have got a real hardware. Luo currently
> trying to push QCA8084 (multi-phy/switch chip) support, and this chip
> exactly contains a huge clock/reset controller [2,3].

Ideally the reset controller is modelled as a Linux reset
controller. The clock part of it is modelled using the common clock
framework. When done correctly, the PHY should not really care. All it
does is ask for its clock to be enabled, and its reset to be disabled.

Also, given how difficult it is proving to be to make any progress at
all, i want to get one little part correctly described, the pure
PHY. Once we have that, we can start on the next little part. So long
as we keep to the Linux architecture of blocks or hardware with
standard Linux drivers, and DT to glue them together, a small step by
step approach should work out.

     Andrew
Andrew Lunn Jan. 7, 2024, 6:44 p.m. UTC | #6
> 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...

That is really core of the problem here. We have no reliable
description of the hardware. The datasheet often starts with a block
diagram of the PHY package. Sometimes there is a product brief with
the same diagram. We need that published. I'm not asking for the full
data sheet, just the introductory text which gives a high level
overview of what the hardware actually is. Then we can sketch out a
high level Linux architecture for the PHY package.

     Andrew
Christian Marangi Jan. 7, 2024, 9:45 p.m. UTC | #7
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 like the ideas of ranges and I will try to propose it... Another idea
might be declare something like <0x16 0x4> where the additional cell
would declare the amount of address occupiaed by the package. But I
think your way is much better and less ""custom"". Yes would also love
some feedback from Rob about this.

> > > 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
>
Sergey Ryazanov Jan. 7, 2024, 9:49 p.m. UTC | #8
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.

>> 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
Sergey Ryazanov Jan. 7, 2024, 9:57 p.m. UTC | #9
Hi Andrew,

On 07.01.2024 20:44, Andrew Lunn wrote:
>> 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...
> 
> That is really core of the problem here. We have no reliable
> description of the hardware. The datasheet often starts with a block
> diagram of the PHY package. Sometimes there is a product brief with
> the same diagram. We need that published. I'm not asking for the full
> data sheet, just the introductory text which gives a high level
> overview of what the hardware actually is. Then we can sketch out a
> high level Linux architecture for the PHY package.

True. I am with you on this. Can we put these words over a mailing list 
entrance door? It will save tons of time for both maintainers and 
submitters.

--
Sergey
Luo Jie Jan. 8, 2024, 11:13 a.m. UTC | #10
On 1/8/2024 2:31 AM, Andrew Lunn wrote:
>> 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?
>>
>> I'm asking because it looks like we have got a real hardware. Luo currently
>> trying to push QCA8084 (multi-phy/switch chip) support, and this chip
>> exactly contains a huge clock/reset controller [2,3].
> 
> Ideally the reset controller is modelled as a Linux reset
> controller. The clock part of it is modelled using the common clock
> framework. When done correctly, the PHY should not really care. All it
> does is ask for its clock to be enabled, and its reset to be disabled.
> 
> Also, given how difficult it is proving to be to make any progress at
> all, i want to get one little part correctly described, the pure
> PHY. Once we have that, we can start on the next little part. So long
> as we keep to the Linux architecture of blocks or hardware with
> standard Linux drivers, and DT to glue them together, a small step by
> step approach should work out.
> 
>       Andrew

Since qca8075 PHY is also multiple port PHY, which is same as qca8084,
but qca8084 also includes the integrated clock controller, this is the
first qcom PHY chip integrating the clock controller internally.
can we also consider designing the clocks and resets DT models in the
PHY package DT.

For qca8084 PURE PHY chip, which is the quad PHY chip and two PCSes,
it integrates the clock controller that generates the clocks to be used
by the link of PHYs, the integrated controller also provides the resets
to the PHY,  the clock controller(NSSCC) driver of qca8084 works at the
same way of the GCC of SoC(IPQ), qca8084 needs to be initialized with
the clocks and resets for the qca8084 PHY package, these clocks and
resets are generated by the NSSCC, even for PURE phy chip qca8084, there
is also some PHY package level clocks needs to be initialized.

here is the diagram of qca8084.
__| |_______________| |__
| PCS0 |          |PCS1 |
|______|          |_____|
|_________________      |
|                |      |
|     NSSCC      |      |
|________________|      |
|_______________________|
|     |     |     |     |
|PHY1 |PHY2 |PHY3 |PHY4 |
|_____|_____|_____|_____|

let me example the initial clocks and resets for the pure PHY chip 
qca8084 as below, the clocks and resets should be put into the first
MDIO node to be initialized firstly before qca8084 PHY will work.

ethernet-phy-package@0 { 

         #address-cells = <1>; 

         #size-cells = <0>; 

         compatible = "ethernet-phy-package"; 

         reg = <0>; 

 

         /* initial PHY package level clocks */ 

         clocks = <&qca8k_nsscc NSS_CC_APB_BRIDGE_CLK>, 

                <&qca8k_nsscc NSS_CC_AHB_CLK>, 

                <&qca8k_nsscc NSS_CC_SEC_CTRL_AHB_CLK>, 

                <&qca8k_nsscc NSS_CC_TLMM_CLK>, 

                <&qca8k_nsscc NSS_CC_TLMM_AHB_CLK>, 

                <&qca8k_nsscc NSS_CC_CNOC_AHB_CLK>, 

                <&qca8k_nsscc NSS_CC_MDIO_AHB_CLK>; 

         clock-names = "apb_bridge", 

                 "ahb", 

                 "sec_ctrl_ahb", 

                 "tlmm", 

                 "tlmm_ahb", 

                 "cnoc_ahb", 

                 "mdio_ahb"; 

 

         /* initial PHY package level reset */ 

         resets = <&qca8k_nsscc NSS_CC_DSP_ARES>; 

         reset-names = "gephy_dsp"; 

 

         /* initial clocks and resets for first phy */ 

         phy0 { 

                 reg = <0>; 

                 clocks = <&qca8k_nsscc NSS_CC_GEPHY0_SYS_CLK>; 

                 clock-names = "gephy0_sys"; 

                 resets = <&qca8k_nsscc NSS_CC_GEPHY0_SYS_ARES>, 

                        <&qca8k_nsscc NSS_CC_GEPHY0_ARES>; 

                 reset-names = "gephy0_sys", 

                         "gephy0_soft"; 

         }; 

 

         /* initial clocks and resets for second phy */ 

         phy1 { 

                 reg = <1>; 

                 clocks = <&qca8k_nsscc NSS_CC_GEPHY1_SYS_CLK>; 

                 clock-names = "gephy1_sys"; 

                 resets = <&qca8k_nsscc NSS_CC_GEPHY1_SYS_ARES>, 

                        <&qca8k_nsscc NSS_CC_GEPHY1_ARES>; 

                 reset-names = "gephy1_sys", 

                         "gephy1_soft"; 

         }; 

 

         /* initial clocks and resets for third phy */ 

         phy2 { 

                 reg = <2>; 

                 clocks = <&qca8k_nsscc NSS_CC_GEPHY2_SYS_CLK>; 

                 clock-names = "gephy2_sys";
                 resets = <&qca8k_nsscc NSS_CC_GEPHY2_SYS_ARES>, 

                        <&qca8k_nsscc NSS_CC_GEPHY2_ARES>; 

                 reset-names = "gephy2_sys", 

                         "gephy2_soft"; 

         }; 

 

         /* initial clocks and resets for fourth phy */ 

         phy3 { 

                 reg = <3>; 

                 clocks = <&qca8k_nsscc NSS_CC_GEPHY3_SYS_CLK>; 

                 clock-names = "gephy3_sys"; 

                 resets = <&qca8k_nsscc NSS_CC_GEPHY3_SYS_ARES>, 

                        <&qca8k_nsscc NSS_CC_GEPHY3_ARES>; 

                 reset-names = "gephy3_sys", 

                         "gephy3_soft"; 

         }; 

 

         /* initial clocks and resets for pcs0. */ 

         pcs0 { 

                 reg = <4>; 

                 clocks = <&qca8k_nsscc NSS_CC_SRDS0_SYS_CLK>; 

                 clock-names = "srds0_sys"; 

                 resets = <&qca8k_nsscc NSS_CC_SRDS0_SYS_ARES>; 

                 reset-names = "srds0_sys"; 

         }; 

 

         /* initial clocks and resets for pcs1. */ 

         pcs1 { 

                 reg = <5>; 

                 clocks = <&qca8k_nsscc NSS_CC_SRDS1_SYS_CLK>; 

                 clock-names = "srds1_sys"; 

                 resets = <&qca8k_nsscc NSS_CC_SRDS1_SYS_ARES>; 

                 reset-names = "srds1_sys"; 

         }; 
 

};

appreciated for the PHY package level DT model design.
Andrew Lunn Jan. 8, 2024, 1:19 p.m. UTC | #11
> Since qca8075 PHY is also multiple port PHY, which is same as qca8084,
> but qca8084 also includes the integrated clock controller, this is the
> first qcom PHY chip integrating the clock controller internally.
> can we also consider designing the clocks and resets DT models in the
> PHY package DT.
> 
> For qca8084 PURE PHY chip, which is the quad PHY chip and two PCSes,
> it integrates the clock controller that generates the clocks to be used
> by the link of PHYs, the integrated controller also provides the resets
> to the PHY,  the clock controller(NSSCC) driver of qca8084 works at the
> same way of the GCC of SoC(IPQ), qca8084 needs to be initialized with
> the clocks and resets for the qca8084 PHY package, these clocks and
> resets are generated by the NSSCC, even for PURE phy chip qca8084, there
> is also some PHY package level clocks needs to be initialized.
> 
> here is the diagram of qca8084.
> __| |_______________| |__
> | PCS0 |          |PCS1 |
> |______|          |_____|
> |_________________      |
> |                |      |
> |     NSSCC      |      |
> |________________|      |
> |_______________________|
> |     |     |     |     |
> |PHY1 |PHY2 |PHY3 |PHY4 |
> |_____|_____|_____|_____|

Please add to the diagram the external clocks and external resets.

Additionally, add the resets and clocks between the NSSCC and the
individual PHYs. Typically, the internal clocks and resets are not in
DT, at last not for a single PHY. For a quad PHY in a package, it
might make sense to add them. Before we can decide that, we need a
clear idea what the hardware looks like.

> let me example the initial clocks and resets for the pure PHY chip qca8084
> as below, the clocks and resets should be put into the first
> MDIO node to be initialized firstly before qca8084 PHY will work.
> 
> ethernet-phy-package@0 {
> 
>         #address-cells = <1>;
> 
>         #size-cells = <0>;
> 
>         compatible = "ethernet-phy-package";
> 
>         reg = <0>;
> 
> 
> 
>         /* initial PHY package level clocks */
> 
>         clocks = <&qca8k_nsscc NSS_CC_APB_BRIDGE_CLK>,
> 
>                <&qca8k_nsscc NSS_CC_AHB_CLK>,
> 
>                <&qca8k_nsscc NSS_CC_SEC_CTRL_AHB_CLK>,
> 
>                <&qca8k_nsscc NSS_CC_TLMM_CLK>,
> 
>                <&qca8k_nsscc NSS_CC_TLMM_AHB_CLK>,
> 
>                <&qca8k_nsscc NSS_CC_CNOC_AHB_CLK>,
> 
>                <&qca8k_nsscc NSS_CC_MDIO_AHB_CLK>;

Device tree effectively defined devices on bus, in a tree, and how
they interconnect. Does the NSSCC have its own address on the MDIO
bus? Or does it share an address with one of the PHYs? It could be we
want to describe the NSSCC as a DT node of its own within the
package. It is probably both a clock consumer, and a clock provider.
The individual PHYs are then clock consumers, of the clocks the NSSCC
exports. Same for resets.

> 
>         clock-names = "apb_bridge",
> 
>                 "ahb",
> 
>                 "sec_ctrl_ahb",
> 
>                 "tlmm",
> 
>                 "tlmm_ahb",
> 
>                 "cnoc_ahb",
> 
>                 "mdio_ahb";
> 
> 
> 
>         /* initial PHY package level reset */
> 
>         resets = <&qca8k_nsscc NSS_CC_DSP_ARES>;
> 
>         reset-names = "gephy_dsp";
> 
> 
> 
>         /* initial clocks and resets for first phy */
> 
>         phy0 {
> 
>                 reg = <0>;
> 
>                 clocks = <&qca8k_nsscc NSS_CC_GEPHY0_SYS_CLK>;
> 
>                 clock-names = "gephy0_sys";
> 
>                 resets = <&qca8k_nsscc NSS_CC_GEPHY0_SYS_ARES>,
> 
>                        <&qca8k_nsscc NSS_CC_GEPHY0_ARES>;
> 
>                 reset-names = "gephy0_sys",
> 
>                         "gephy0_soft";
> 
>         };
> 
> 
> 
>         /* initial clocks and resets for second phy */
> 
>         phy1 {
> 
>                 reg = <1>;
> 
>                 clocks = <&qca8k_nsscc NSS_CC_GEPHY1_SYS_CLK>;
> 
>                 clock-names = "gephy1_sys";
> 
>                 resets = <&qca8k_nsscc NSS_CC_GEPHY1_SYS_ARES>,
> 
>                        <&qca8k_nsscc NSS_CC_GEPHY1_ARES>;
> 
>                 reset-names = "gephy1_sys",
> 
>                         "gephy1_soft";
> 
>         };
> 
> 
> 
>         /* initial clocks and resets for third phy */
> 
>         phy2 {
> 
>                 reg = <2>;
> 
>                 clocks = <&qca8k_nsscc NSS_CC_GEPHY2_SYS_CLK>;
> 
>                 clock-names = "gephy2_sys";
>                 resets = <&qca8k_nsscc NSS_CC_GEPHY2_SYS_ARES>,
> 
>                        <&qca8k_nsscc NSS_CC_GEPHY2_ARES>;
> 
>                 reset-names = "gephy2_sys",
> 
>                         "gephy2_soft";
> 
>         };
> 
> 
> 
>         /* initial clocks and resets for fourth phy */
> 
>         phy3 {
> 
>                 reg = <3>;
> 
>                 clocks = <&qca8k_nsscc NSS_CC_GEPHY3_SYS_CLK>;
> 
>                 clock-names = "gephy3_sys";
> 
>                 resets = <&qca8k_nsscc NSS_CC_GEPHY3_SYS_ARES>,
> 
>                        <&qca8k_nsscc NSS_CC_GEPHY3_ARES>;
> 
>                 reset-names = "gephy3_sys",
> 
>                         "gephy3_soft";
> 
>         };

This is starting to look O.K.

>         /* initial clocks and resets for pcs0. */
> 
>         pcs0 {
> 
>                 reg = <4>;
> 
>                 clocks = <&qca8k_nsscc NSS_CC_SRDS0_SYS_CLK>;
> 
>                 clock-names = "srds0_sys";
> 
>                 resets = <&qca8k_nsscc NSS_CC_SRDS0_SYS_ARES>;
> 
>                 reset-names = "srds0_sys";
> 
>         };
> 
> 
> 
>         /* initial clocks and resets for pcs1. */
> 
>         pcs1 {
> 
>                 reg = <5>;
> 
>                 clocks = <&qca8k_nsscc NSS_CC_SRDS1_SYS_CLK>;
> 
>                 clock-names = "srds1_sys";
> 
>                 resets = <&qca8k_nsscc NSS_CC_SRDS1_SYS_ARES>;
> 
>                 reset-names = "srds1_sys";
> 
>         };

PCS will need further work and thinking about. Typically, they are not
described in DT for a PHY. In general, a PCS in a PHY does not have a
driver of its own, the firmware in the PHY mostly controls it, not
Linux. For the moment, lets leave them as they are, and we will come
back to them once we get the clocks and resets correctly described.

     Andrew
Luo Jie Jan. 9, 2024, 11:44 a.m. UTC | #12
On 1/8/2024 9:19 PM, Andrew Lunn wrote:
>> Since qca8075 PHY is also multiple port PHY, which is same as qca8084,
>> but qca8084 also includes the integrated clock controller, this is the
>> first qcom PHY chip integrating the clock controller internally.
>> can we also consider designing the clocks and resets DT models in the
>> PHY package DT.
>>
>> For qca8084 PURE PHY chip, which is the quad PHY chip and two PCSes,
>> it integrates the clock controller that generates the clocks to be used
>> by the link of PHYs, the integrated controller also provides the resets
>> to the PHY,  the clock controller(NSSCC) driver of qca8084 works at the
>> same way of the GCC of SoC(IPQ), qca8084 needs to be initialized with
>> the clocks and resets for the qca8084 PHY package, these clocks and
>> resets are generated by the NSSCC, even for PURE phy chip qca8084, there
>> is also some PHY package level clocks needs to be initialized.
>>
>> here is the diagram of qca8084.
>> __| |_______________| |__
>> | PCS0 |          |PCS1 |
>> |______|          |_____|
>> |_________________      |
>> |                |      |
>> |     NSSCC      |      |
>> |________________|      |
>> |_______________________|
>> |     |     |     |     |
>> |PHY1 |PHY2 |PHY3 |PHY4 |
>> |_____|_____|_____|_____|
> 
> Please add to the diagram the external clocks and external resets.

__| |_______________| |__
| PCS0 |          |PCS1 |
|______|          |_____|
|_______                |<---- REF clock 50MHZ
|      |------------    |
|NSSCC |    |clks  |rsts|<---- GPIO reset
|______|    |      |    |
|           V      V    |
|_______________________|
|     |     |     |     |
|PHY1 |PHY2 |PHY3 |PHY4 |
|_____|_____|_____|_____|

There are difference clock trees generated from NSSCC for the different
PHYs, all clocks and resets for qca8084 CHIP working are internally
provided by the NSSCC.

> 
> Additionally, add the resets and clocks between the NSSCC and the
> individual PHYs. Typically, the internal clocks and resets are not in
> DT, at last not for a single PHY. For a quad PHY in a package, it
> might make sense to add them. Before we can decide that, we need a
> clear idea what the hardware looks like.

Yes, that is true.
The reason why i add the DTs for the clocks and resets is these clocks
and resets are provided by the internal NSSCC provider driver, in this
chip, both the clock provider and clock consumer are located in the
qca8084 PHY chip.

> 
>> let me example the initial clocks and resets for the pure PHY chip qca8084
>> as below, the clocks and resets should be put into the first
>> MDIO node to be initialized firstly before qca8084 PHY will work.
>>
>> ethernet-phy-package@0 {
>>
>>          #address-cells = <1>;
>>
>>          #size-cells = <0>;
>>
>>          compatible = "ethernet-phy-package";
>>
>>          reg = <0>;
>>
>>
>>
>>          /* initial PHY package level clocks */
>>
>>          clocks = <&qca8k_nsscc NSS_CC_APB_BRIDGE_CLK>,
>>
>>                 <&qca8k_nsscc NSS_CC_AHB_CLK>,
>>
>>                 <&qca8k_nsscc NSS_CC_SEC_CTRL_AHB_CLK>,
>>
>>                 <&qca8k_nsscc NSS_CC_TLMM_CLK>,
>>
>>                 <&qca8k_nsscc NSS_CC_TLMM_AHB_CLK>,
>>
>>                 <&qca8k_nsscc NSS_CC_CNOC_AHB_CLK>,
>>
>>                 <&qca8k_nsscc NSS_CC_MDIO_AHB_CLK>;
> 
> Device tree effectively defined devices on bus, in a tree, and how
> they interconnect. Does the NSSCC have its own address on the MDIO
> bus? Or does it share an address with one of the PHYs? It could be we
> want to describe the NSSCC as a DT node of its own within the
> package. It is probably both a clock consumer, and a clock provider.
> The individual PHYs are then clock consumers, of the clocks the NSSCC
> exports. Same for resets.

Yes, Andrew, the NSSCC provider driver is probed based on the MDIO
device, the PHY CHIP occupies the MDIO addresses, so the NSSCC is
registered as the MDIO device.

The following is the DT of the NSSCC driver, normally we should not
enable the clocks in the clock provider driver. The clocks also can't
be configured during the NSSCC driver probed because of the initial
clocks and resets(mentioned in this DT example ethernet-phy-package@0)
are not configured.

DT of the NSSCC device node:
mdio {
       #address-cells = <1>;
       #size-cells = <0>;

       clock-controller@18 {
         compatible = "qcom,qca8084-nsscc";
         reg = <0x18>;
         clocks = <&qca8k_xo>,
                  <&qca8k_uniphy0_rx>,
                  <&qca8k_uniphy0_tx>,
                  <&qca8k_uniphy1_rx>,
                  <&qca8k_uniphy1_tx>,
                  <&qca8k_uniphy1_rx312p5m>,
                  <&qca8k_uniphy1_tx312p5m>;
         #clock-cells = <1>;
         #reset-cells = <1>;
         #power-domain-cells = <1>;
       };
     };


> 
>>
>>          clock-names = "apb_bridge",
>>
>>                  "ahb",
>>
>>                  "sec_ctrl_ahb",
>>
>>                  "tlmm",
>>
>>                  "tlmm_ahb",
>>
>>                  "cnoc_ahb",
>>
>>                  "mdio_ahb";
>>
>>
>>
>>          /* initial PHY package level reset */
>>
>>          resets = <&qca8k_nsscc NSS_CC_DSP_ARES>;
>>
>>          reset-names = "gephy_dsp";
>>
>>
>>
>>          /* initial clocks and resets for first phy */
>>
>>          phy0 {
>>
>>                  reg = <0>;
>>
>>                  clocks = <&qca8k_nsscc NSS_CC_GEPHY0_SYS_CLK>;
>>
>>                  clock-names = "gephy0_sys";
>>
>>                  resets = <&qca8k_nsscc NSS_CC_GEPHY0_SYS_ARES>,
>>
>>                         <&qca8k_nsscc NSS_CC_GEPHY0_ARES>;
>>
>>                  reset-names = "gephy0_sys",
>>
>>                          "gephy0_soft";
>>
>>          };
>>
>>
>>
>>          /* initial clocks and resets for second phy */
>>
>>          phy1 {
>>
>>                  reg = <1>;
>>
>>                  clocks = <&qca8k_nsscc NSS_CC_GEPHY1_SYS_CLK>;
>>
>>                  clock-names = "gephy1_sys";
>>
>>                  resets = <&qca8k_nsscc NSS_CC_GEPHY1_SYS_ARES>,
>>
>>                         <&qca8k_nsscc NSS_CC_GEPHY1_ARES>;
>>
>>                  reset-names = "gephy1_sys",
>>
>>                          "gephy1_soft";
>>
>>          };
>>
>>
>>
>>          /* initial clocks and resets for third phy */
>>
>>          phy2 {
>>
>>                  reg = <2>;
>>
>>                  clocks = <&qca8k_nsscc NSS_CC_GEPHY2_SYS_CLK>;
>>
>>                  clock-names = "gephy2_sys";
>>                  resets = <&qca8k_nsscc NSS_CC_GEPHY2_SYS_ARES>,
>>
>>                         <&qca8k_nsscc NSS_CC_GEPHY2_ARES>;
>>
>>                  reset-names = "gephy2_sys",
>>
>>                          "gephy2_soft";
>>
>>          };
>>
>>
>>
>>          /* initial clocks and resets for fourth phy */
>>
>>          phy3 {
>>
>>                  reg = <3>;
>>
>>                  clocks = <&qca8k_nsscc NSS_CC_GEPHY3_SYS_CLK>;
>>
>>                  clock-names = "gephy3_sys";
>>
>>                  resets = <&qca8k_nsscc NSS_CC_GEPHY3_SYS_ARES>,
>>
>>                         <&qca8k_nsscc NSS_CC_GEPHY3_ARES>;
>>
>>                  reset-names = "gephy3_sys",
>>
>>                          "gephy3_soft";
>>
>>          };
> 
> This is starting to look O.K.
> 
>>          /* initial clocks and resets for pcs0. */
>>
>>          pcs0 {
>>
>>                  reg = <4>;
>>
>>                  clocks = <&qca8k_nsscc NSS_CC_SRDS0_SYS_CLK>;
>>
>>                  clock-names = "srds0_sys";
>>
>>                  resets = <&qca8k_nsscc NSS_CC_SRDS0_SYS_ARES>;
>>
>>                  reset-names = "srds0_sys";
>>
>>          };
>>
>>
>>
>>          /* initial clocks and resets for pcs1. */
>>
>>          pcs1 {
>>
>>                  reg = <5>;
>>
>>                  clocks = <&qca8k_nsscc NSS_CC_SRDS1_SYS_CLK>;
>>
>>                  clock-names = "srds1_sys";
>>
>>                  resets = <&qca8k_nsscc NSS_CC_SRDS1_SYS_ARES>;
>>
>>                  reset-names = "srds1_sys";
>>
>>          };
> 
> PCS will need further work and thinking about. Typically, they are not
> described in DT for a PHY. In general, a PCS in a PHY does not have a
> driver of its own, the firmware in the PHY mostly controls it, not
> Linux. For the moment, lets leave them as they are, and we will come
> back to them once we get the clocks and resets correctly described.
> 
>       Andrew

Got it.
Since there is no firmware located in the qca8084 PHY, the qca8084 PHY
is only configured by the Linux driver to make it working.

Thanks Andrew for the comments and suggestions.
Andrew Lunn Jan. 9, 2024, 1:48 p.m. UTC | #13
> 
> __| |_______________| |__
> | PCS0 |          |PCS1 |
> |______|          |_____|
> |_______                |<---- REF clock 50MHZ
> |      |------------    |
> |NSSCC |    |clks  |rsts|<---- GPIO reset
> |______|    |      |    |
> |           V      V    |
> |_______________________|
> |     |     |     |     |
> |PHY1 |PHY2 |PHY3 |PHY4 |
> |_____|_____|_____|_____|

Not the best of improvements. So the ref clock goes to the package,
and then magically somehow gets to the NSSCC? Are there any more
blocks it goes through before reaching the NSSCC? How does the GPIO
reset get converted into multiple reset inside the package? Details,
details, details.

> There are difference clock trees generated from NSSCC for the different
> PHYs, all clocks and resets for qca8084 CHIP working are internally
> provided by the NSSCC.

So show this in the block diagram.

> Yes, Andrew, the NSSCC provider driver is probed based on the MDIO
> device, the PHY CHIP occupies the MDIO addresses, so the NSSCC is
> registered as the MDIO device.
> 
> DT of the NSSCC device node:
> mdio {
>       #address-cells = <1>;
>       #size-cells = <0>;
> 
>       clock-controller@18 {
>         compatible = "qcom,qca8084-nsscc";
>         reg = <0x18>;
>         clocks = <&qca8k_xo>,
>                  <&qca8k_uniphy0_rx>,
>                  <&qca8k_uniphy0_tx>,
>                  <&qca8k_uniphy1_rx>,
>                  <&qca8k_uniphy1_tx>,
>                  <&qca8k_uniphy1_rx312p5m>,
>                  <&qca8k_uniphy1_tx312p5m>;
>         #clock-cells = <1>;
>         #reset-cells = <1>;
>         #power-domain-cells = <1>;
>       };
>     };
 
This does not make any sense. You have one clock input, 50MHz. So why
are you listing 6 consumer clocks, not one? And where are the clocks
this clock controller provides, clock-output-names=<...>;

I give up. Please consider this PHY driver NACKed.

Get Linaro, or some other organisation with a lot of experience with
mainline to take over the work.

	 Andrew
Luo Jie Jan. 10, 2024, 3:13 a.m. UTC | #14
On 1/9/2024 9:48 PM, Andrew Lunn wrote:
>>
>> __| |_______________| |__
>> | PCS0 |          |PCS1 |
>> |______|          |_____|
>> |_______                |<---- REF clock 50MHZ
>> |      |------------    |
>> |NSSCC |    |clks  |rsts|<---- GPIO reset
>> |______|    |      |    |
>> |           V      V    |
>> |_______________________|
>> |     |     |     |     |
>> |PHY1 |PHY2 |PHY3 |PHY4 |
>> |_____|_____|_____|_____|
> 
> Not the best of improvements. So the ref clock goes to the package,
> and then magically somehow gets to the NSSCC? Are there any more
> blocks it goes through before reaching the NSSCC? How does the GPIO
> reset get converted into multiple reset inside the package? Details,
> details, details.

The GPIO reset for the whole CHIP, like a GPIO reset on the single port
PHY qca8081, after the GPIO reset, the whole qca8084 chip is in the cold
reset state, so the initial clocks ans resets need to be initialized as
we are designing the clock model here.

As for the reference clock 50M input, this reference clock 50MHZ is
PLLed by the PCS1 block of qca8084, then there are fix 312.5MHZ is
generated to the NSSCC, NSSCC includes multiple RCG clock trees to
generate the clocks to the PHY and PCS, all these blocks(PCS and PHY)
are located in the qca8084 PHY chip, sorry for missed this diagram
info, updated the diagram as below, since the PCS generates the clocks
to NSSCC, there are initial clocks ans resets included for PCS need
to be configured before CHIP to work.

So the parent clocks are PCS0(uniphy0) and PCS1(uniphy1) for NSSCC,
as i provide the DT of NSSCC provider below, the parent clock rate
of PCS is changed according to the PHY link speed, and there are
also fix clock rate 312.5MHZ from PCS1, these parent clocks are
the root clock of RCG clock tree in NSSCC, the output clocks of
the RCG are consumed by the PHYs(PHY1-PHY4 and PCS0, PCS1).

    REF 50MHZ
       |
       |
______V__________________
| PCS1 |       ____|PCS0|
|______|      |    |____|
|  |  ________|         |
|  | |                  |<----------GPIO reset
|__V_V__                |
|      |------------    |
|NSSCC |    |clks  |rsts|
|______|    |      |    |
|           V      V    |
|_______________________|
|     |     |     |     |
|PHY1 |PHY2 |PHY3 |PHY4 |
|_____|_____|_____|_____|

> 
>> There are difference clock trees generated from NSSCC for the different
>> PHYs, all clocks and resets for qca8084 CHIP working are internally
>> provided by the NSSCC.
> 
> So show this in the block diagram.
> 
>> Yes, Andrew, the NSSCC provider driver is probed based on the MDIO
>> device, the PHY CHIP occupies the MDIO addresses, so the NSSCC is
>> registered as the MDIO device.
>>
>> DT of the NSSCC device node:
>> mdio {
>>        #address-cells = <1>;
>>        #size-cells = <0>;
>>
>>        clock-controller@18 {
>>          compatible = "qcom,qca8084-nsscc";
>>          reg = <0x18>;
>>          clocks = <&qca8k_xo>,
>>                   <&qca8k_uniphy0_rx>,
>>                   <&qca8k_uniphy0_tx>,
>>                   <&qca8k_uniphy1_rx>,
>>                   <&qca8k_uniphy1_tx>,
>>                   <&qca8k_uniphy1_rx312p5m>,
>>                   <&qca8k_uniphy1_tx312p5m>;
>>          #clock-cells = <1>;
>>          #reset-cells = <1>;
>>          #power-domain-cells = <1>;
>>        };
>>      };
>   
> This does not make any sense. You have one clock input, 50MHz. So why
> are you listing 6 consumer clocks, not one? And where are the clocks
> this clock controller provides, clock-output-names=<...>;

Hi Andrew,
Sorry for the confusion.
the DT of the NSSCC device node is from the NSSCC provider driver, which
is under review currently as the link below.
https://lore.kernel.org/lkml/20231104034858.9159-2-quic_luoj@quicinc.com/T/#m204c22d14be8f9dda7cd7f666ed726b8fc3301ef

the property "clocks" are the parent clock list of RCG clock tree in
the NSSCC driver, for qca8084 chip, there are two UNIPHY(PCS) as
mentioned in the diagram, there are also fix rate clocks 312.5MHZ,
the output clocks of NSSCC are not listed in the DT, which is same
as the GCC(IPQ SoC) DT.

The output clocks of NSSCC are provided by the DT of the clock consumer
such as the DT of qca8084 as below.
phy0 {
         reg = <0>;
         clocks = <&qca8k_nsscc NSS_CC_GEPHY0_SYS_CLK>;
         clock-names = "gephy0_sys";
         resets = <&qca8k_nsscc NSS_CC_GEPHY0_SYS_ARES>,
                  <&qca8k_nsscc NSS_CC_GEPHY0_ARES>;
         reset-names = "gephy0_sys", "gephy0_soft";
      };

In the DT node above, the qca8k_nssc refers to the NSSCC clock provider
DT node "clock-controller@18", the clock ID "NSS_CC_GEPHY0_SYS_CLK" is
also provided by the NSSCC clock provider driver.

The different PHY(PHY1-PHY4) of qca8084 has the different clock
tree(RCG), the clock consumer driver needs to define the DT properties
clocks and clcok-names to get the clocks from the NSSCC clock provider.

> 
> I give up. Please consider this PHY driver NACKed.
> 
> Get Linaro, or some other organisation with a lot of experience with
> mainline to take over the work.
> 
> 	 Andrew
Christian Marangi Jan. 17, 2024, 12:36 a.m. UTC | #15
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)

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.)

> > > 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
>
Sergey Ryazanov Jan. 17, 2024, 7:39 p.m. UTC | #16
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
Christian Marangi Jan. 17, 2024, 8:03 p.m. UTC | #17
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
Andrew Lunn Jan. 17, 2024, 8:05 p.m. UTC | #18
> 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 don't have the datasheet for this specific PHY. But the concept of a
quad PHY in one package is well known. Take for example the
VSC8584. The datasheet is open on Microchips website. It has four
strapping pins to determine the addresses of the PHYs in the
package. The PHY number, 0 - 3 determine bits 0-1 of the MDIO
address. The 4 strapping pins then determine bit 2-5 of the address.

In theory, each PHY could have its own strapping pins, allowing it to
be set to any address, and two PHYs could even have the same
address. But i doubt anybody actually builds hardware like that. I
expect the base addresses is set at the package level, and then PHYs
are just offsets from this.

So to me, a range property does seem reasonable. However, I agree, we
need acceptance from the DT Maintainers.

     Andrew
diff mbox series

Patch

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>;
+            };
+        };
+    };