| Message ID | 20231126015346.25208-2-ansuelsmth@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | net: phy: Support DT PHY package | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
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 >
> > + 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
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
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
> 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
> 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
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 >
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
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
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.
> 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
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.
> > __| |_______________| |__ > | 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
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
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 >
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
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
> 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 --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>; + }; + }; + };
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