| Message ID | 20231120135041.15259-4-ansuelsmth@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: Support DT PHY package | expand |
On Mon, Nov 20, 2023 at 10:41:33AM -0700, Rob Herring wrote: > On Mon, Nov 20, 2023 at 02:50:30PM +0100, Christian Marangi wrote: > > Document ethernet PHY package nodes used to describe PHY shipped in > > bundle of 4-5 PHY. These particular PHY require specific PHY in the > > package for global onfiguration of the PHY package. > > > > 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 should use the global-phys property and the > > global-phy-names to define PHY in the package required by the PHY driver > > for global configuration. > > > > It's also possible to specify the property phy-mode to specify that the > > PHY package sets a global PHY interface mode and every PHY of the > > package requires to have the same PHY interface mode. > > > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > > --- > > .../bindings/net/ethernet-phy-package.yaml | 86 +++++++++++++++++++ > > 1 file changed, 86 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..2aa109e155d9 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/net/ethernet-phy-package.yaml > > @@ -0,0 +1,86 @@ > > +# 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 > > Missing a '>' > > > + > > +properties: > > + $nodename: > > + pattern: "^ethernet-phy-package(-[0-9]+)?$" > > + > > + compatible: > > + const: ethernet-phy-package > > + > > + '#address-cells': > > + description: number of address cells for the MDIO bus > > + const: 1 > > + > > + '#size-cells': > > + description: number of size cells on the MDIO bus > > + const: 0 > > + > > + global-phys: > > + $ref: /schemas/types.yaml#/definitions/phandle-array > > + minItems: 1 > > + maxItems: 31 > > + description: > > + List of phandle to the PHY in the package required and > > + used to configure the PHY package. > > + > > + global-phy-names: > > + $ref: /schemas/types.yaml#/definitions/string-array > > + minItems: 1 > > + maxItems: 31 > > + description: > > + List of names of the PHY defined in global-phys. > > + > > + phy-connection-type: > > + $ref: /schemas/net/ethernet-phy-mode-types.yaml#definitions/phy-connection-type > > + description: > > + Specifies global interface type for the PHY package. > > + > > + phy-mode: > > + $ref: "#/properties/phy-connection-type" > > + > > +patternProperties: > > + ^ethernet-phy(@[a-f0-9]+)?$: > > + $ref: /schemas/net/ethernet-phy.yaml# > > + > > +required: > > + - compatible > > + > > +dependencies: > > + global-phy-names: [global-phys] > > + > > +unevaluatedProperties: false > > + > > +examples: > > + - | > > + ethernet { > > You mean 'mdio' here, right? > > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + ethernet-phy-package { > > This doesn't work. Child nodes of MDIO bus must be an MDIO device with > an address. What you need is a node with all the addresses of the > device: > > mdio { > ... > > ethernet-phy@1 { > compatible = "vendor,specifc-compatible-for-device"; > reg = <1>, <4>; > ... > }; > }; > > There's also some MDIO devices which define a secondary address as a > child device. Maybe those are similar to your situation. I don't recall > which ones offhand. > Ehh this is not really a situation. We really need a way to describe PHY package. (In the sense of device that expose multiple PHY package, as they can be treated as single one but they are actually in bulk of 2-4-5 PHY) qca807x is one example, quickinc is trying to push another PHY with just a similar implementation and Maxime Chevallier just pointed out that Marvell Alaska 88e1543 PHY also have this kind of configuration. I feel defining PHY in subnode is a MUST and using ethernet-phy might be confusing to describe PHY package (so I think a brand new node name might be a better solution) About the reg, I wonder if it would like it more if the PHY package node would include the reg as the first address of the package and the reg property as a list of all the reg the PHY package use. Something like this? ethernet-phy-package@1 { compatible = "ethernet-phy-package"; #address-cells = <1>; #size-cells = <0>; reg = <1>, <2>, <3>, <4>; global-phys = <&phy4>; global-phy-names = "base"; ethernet-phy@1 { compatible = "ethernet-phy-ieee802.3-c22"; reg = <1>; }; phy4: ethernet-phy@4 { compatible = "ethernet-phy-ieee802.3-c22"; reg = <4>; }; }; Thanks a lot for the review and I hope we can find a good and correct way to model this. Just hope we don't have to add all kind of proprerty to describe the idea of PHY package. (I think the current example makes it very clear that the PHY under the node are all part of a single piece on the device) > > + compatible = "ethernet-phy-package"; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + global-phys = <&phy4>; > > + global-phy-names = "base"; > > + > > + ethernet-phy@1 { > > + compatible = "ethernet-phy-ieee802.3-c22"; > > + reg = <1>; > > + }; > > + > > + phy4: ethernet-phy@4 { > > + compatible = "ethernet-phy-ieee802.3-c22"; > > + reg = <4>; > > + }; > > + }; > > + }; > > -- > > 2.40.1 > >
On Mon, Nov 20, 2023 at 02:50:30PM +0100, Christian Marangi wrote: > Document ethernet PHY package nodes used to describe PHY shipped in > bundle of 4-5 PHY. These particular PHY require specific PHY in the > package for global onfiguration of the PHY package. > > 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 should use the global-phys property and the > global-phy-names to define PHY in the package required by the PHY driver > for global configuration. > > It's also possible to specify the property phy-mode to specify that the > PHY package sets a global PHY interface mode and every PHY of the > package requires to have the same PHY interface mode. > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > --- > .../bindings/net/ethernet-phy-package.yaml | 86 +++++++++++++++++++ > 1 file changed, 86 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..2aa109e155d9 > --- /dev/null > +++ b/Documentation/devicetree/bindings/net/ethernet-phy-package.yaml > @@ -0,0 +1,86 @@ > +# 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 Missing a '>' > + > +properties: > + $nodename: > + pattern: "^ethernet-phy-package(-[0-9]+)?$" > + > + compatible: > + const: ethernet-phy-package > + > + '#address-cells': > + description: number of address cells for the MDIO bus > + const: 1 > + > + '#size-cells': > + description: number of size cells on the MDIO bus > + const: 0 > + > + global-phys: > + $ref: /schemas/types.yaml#/definitions/phandle-array > + minItems: 1 > + maxItems: 31 > + description: > + List of phandle to the PHY in the package required and > + used to configure the PHY package. > + > + global-phy-names: > + $ref: /schemas/types.yaml#/definitions/string-array > + minItems: 1 > + maxItems: 31 > + description: > + List of names of the PHY defined in global-phys. > + > + phy-connection-type: > + $ref: /schemas/net/ethernet-phy-mode-types.yaml#definitions/phy-connection-type > + description: > + Specifies global interface type for the PHY package. > + > + phy-mode: > + $ref: "#/properties/phy-connection-type" > + > +patternProperties: > + ^ethernet-phy(@[a-f0-9]+)?$: > + $ref: /schemas/net/ethernet-phy.yaml# > + > +required: > + - compatible > + > +dependencies: > + global-phy-names: [global-phys] > + > +unevaluatedProperties: false > + > +examples: > + - | > + ethernet { You mean 'mdio' here, right? > + #address-cells = <1>; > + #size-cells = <0>; > + > + ethernet-phy-package { This doesn't work. Child nodes of MDIO bus must be an MDIO device with an address. What you need is a node with all the addresses of the device: mdio { ... ethernet-phy@1 { compatible = "vendor,specifc-compatible-for-device"; reg = <1>, <4>; ... }; }; There's also some MDIO devices which define a secondary address as a child device. Maybe those are similar to your situation. I don't recall which ones offhand. > + compatible = "ethernet-phy-package"; > + #address-cells = <1>; > + #size-cells = <0>; > + > + global-phys = <&phy4>; > + global-phy-names = "base"; > + > + ethernet-phy@1 { > + compatible = "ethernet-phy-ieee802.3-c22"; > + reg = <1>; > + }; > + > + phy4: ethernet-phy@4 { > + compatible = "ethernet-phy-ieee802.3-c22"; > + reg = <4>; > + }; > + }; > + }; > -- > 2.40.1 >
On Mon, Nov 20, 2023 at 09:44:58PM +0100, Andrew Lunn wrote: > On Mon, Nov 20, 2023 at 02:50:30PM +0100, Christian Marangi wrote: > > Document ethernet PHY package nodes used to describe PHY shipped in > > bundle of 4-5 PHY. These particular PHY require specific PHY in the > > package for global onfiguration of the PHY package. > > > > 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. > > I think you are being overly narrow here. The 'global' registers could > be spread over multiple addresses. Particularly for a C22 PHY. I > suppose they could even be in a N+1 address space, where there is no > PHY at all. > > Where the global registers are is specific to a PHY package > vendor/model. The PHY driver should know this. All the PHY driver > needs to know is some sort of base offset. PHY0 in this package is > using address X. It can then use relative addressing from this base to > access the global registers for this package. Yes that would also work but adds extra fragile code in PHY driver. An idea might be define PHY package node with a reg that is the base addr... and if we really want every PHY in the PHY package node is an offset of the base addr. > > > It's also possible to specify the property phy-mode to specify that the > > PHY package sets a global PHY interface mode and every PHY of the > > package requires to have the same PHY interface mode. > > I don't think it is what simple. See the QCA8084 for example. 3 of the > 4 PHYs must use QXGMII. The fourth PHY can also use QXGMII but it can > be multiplexed to a different PMA and use 1000BaseX, SGMII or > 2500BaseX. Yes that is totally a problem but I think it can only be handled with some validation in the PHY driver... I assume probe_once would validate the modes? > > I do think we need somewhere to put package properties. But i don't > think phy-mode is such a property. At the moment, i don't have a good > example of a package property. > And this is the main problem with this thing... Find a good way to define them that everyone is OK with. Another idea might be introduce to each PHY a property that point to the PHY package node (phandle) with all the info... But where to place that??? Outside mdio node? That would be confusing... This is why I like this subnode way. I know it deviates a bit from the normal way of defining small node in the mdio node one for each PHY. > > +examples: > > + - | > > + ethernet { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + ethernet-phy-package { > > + compatible = "ethernet-phy-package"; > > + #address-cells = <1>; > > + #size-cells = <0>; > > You have the PHYs within the Ethernet node. This is allowed by DT, for > historic reasons. However, i don't remember the last time a patch was > submitted that actually used this method. Now a days, PHYs are on an > MDIO bus, and they are children of that bus in the DT representation. > However you represent the package needs to work with MDIO busses. > Using the ethernet node was an oversight and actually this is defined as a subnode in the mdio node. A real DT that use this is (ipq807x): &mdio { status = "okay"; pinctrl-0 = <&mdio_pins>; pinctrl-names = "default"; reset-gpios = <&tlmm 37 GPIO_ACTIVE_LOW>; ethernet-phy-package { compatible = "ethernet-phy-package"; phy-mode = "psgmii"; global-phys = <&qca8075_4>, <&qca8075_psgmii>; global-phy-names = "combo", "analog_psgmii"; qca8075_0: ethernet-phy@0 { compatible = "ethernet-phy-ieee802.3-c22"; reg = <0>; }; qca8075_1: ethernet-phy@1 { compatible = "ethernet-phy-ieee802.3-c22"; reg = <1>; }; qca8075_2: ethernet-phy@2 { compatible = "ethernet-phy-ieee802.3-c22"; reg = <2>; }; qca8075_3: ethernet-phy@3 { compatible = "ethernet-phy-ieee802.3-c22"; reg = <3>; }; qca8075_4: ethernet-phy@4 { compatible = "ethernet-phy-ieee802.3-c22"; reg = <4>; }; qca8075_psgmii: ethernet-phy@5 { compatible = "ethernet-phy-ieee802.3-c22"; reg = <5>; }; }; qca8081: ethernet-phy@28 { compatible = "ethernet-phy-id004d.d101"; reg = <28>; reset-gpios = <&tlmm 31 GPIO_ACTIVE_LOW>; }; aqr113c: ethernet-phy@8 { compatible = "ethernet-phy-ieee802.3-c45"; reg = <8>; reset-gpios = <&tlmm 63 GPIO_ACTIVE_LOW>; }; };
On Mon, Nov 20, 2023 at 10:25:10PM +0100, Andrew Lunn wrote: > > A real DT that use this is (ipq807x): > > > > &mdio { > > status = "okay"; > > pinctrl-0 = <&mdio_pins>; > > pinctrl-names = "default"; > > reset-gpios = <&tlmm 37 GPIO_ACTIVE_LOW>; > > > > ethernet-phy-package { > > compatible = "ethernet-phy-package"; > > phy-mode = "psgmii"; > > > > global-phys = <&qca8075_4>, <&qca8075_psgmii>; > > global-phy-names = "combo", "analog_psgmii"; > > > > qca8075_0: ethernet-phy@0 { > > compatible = "ethernet-phy-ieee802.3-c22"; > > reg = <0>; > > }; > > ... > > > }; > > > > qca8081: ethernet-phy@28 { > > compatible = "ethernet-phy-id004d.d101"; > > reg = <28>; > > reset-gpios = <&tlmm 31 GPIO_ACTIVE_LOW>; > > }; > > I've no idea if DT allows this. The issue is that reg is the same for > both nodes within the ethernet-phy-package container, and > ethernet-phy@28. They are all addresses on the same MDIO bus. We are > parsing this bus structure ourselves in __of_mdiobus_register(), so we > could make it work, but i don't know if we should make it work. > And that is why I have some reserve on the idea of defining a reg for ethernet-phy-package. Adding a reg would create some duplicate. Is it really a problem to have a node with no reg in the mdio node? (patch 04 of this series already updates the parsing function to check one level deeper in the presence of the ethernet-phy-compatible treating any node found as it was defined in the upper mdio node)
On Mon, Nov 20, 2023 at 02:50:30PM +0100, Christian Marangi wrote: > Document ethernet PHY package nodes used to describe PHY shipped in > bundle of 4-5 PHY. These particular PHY require specific PHY in the > package for global onfiguration of the PHY package. > > 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. I think you are being overly narrow here. The 'global' registers could be spread over multiple addresses. Particularly for a C22 PHY. I suppose they could even be in a N+1 address space, where there is no PHY at all. Where the global registers are is specific to a PHY package vendor/model. The PHY driver should know this. All the PHY driver needs to know is some sort of base offset. PHY0 in this package is using address X. It can then use relative addressing from this base to access the global registers for this package. > It's also possible to specify the property phy-mode to specify that the > PHY package sets a global PHY interface mode and every PHY of the > package requires to have the same PHY interface mode. I don't think it is what simple. See the QCA8084 for example. 3 of the 4 PHYs must use QXGMII. The fourth PHY can also use QXGMII but it can be multiplexed to a different PMA and use 1000BaseX, SGMII or 2500BaseX. I do think we need somewhere to put package properties. But i don't think phy-mode is such a property. At the moment, i don't have a good example of a package property. > +examples: > + - | > + ethernet { > + #address-cells = <1>; > + #size-cells = <0>; > + > + ethernet-phy-package { > + compatible = "ethernet-phy-package"; > + #address-cells = <1>; > + #size-cells = <0>; You have the PHYs within the Ethernet node. This is allowed by DT, for historic reasons. However, i don't remember the last time a patch was submitted that actually used this method. Now a days, PHYs are on an MDIO bus, and they are children of that bus in the DT representation. However you represent the package needs to work with MDIO busses. Andrew --- pw-bot: cr
> A real DT that use this is (ipq807x): > > &mdio { > status = "okay"; > pinctrl-0 = <&mdio_pins>; > pinctrl-names = "default"; > reset-gpios = <&tlmm 37 GPIO_ACTIVE_LOW>; > > ethernet-phy-package { > compatible = "ethernet-phy-package"; > phy-mode = "psgmii"; > > global-phys = <&qca8075_4>, <&qca8075_psgmii>; > global-phy-names = "combo", "analog_psgmii"; > > qca8075_0: ethernet-phy@0 { > compatible = "ethernet-phy-ieee802.3-c22"; > reg = <0>; > }; ... > }; > > qca8081: ethernet-phy@28 { > compatible = "ethernet-phy-id004d.d101"; > reg = <28>; > reset-gpios = <&tlmm 31 GPIO_ACTIVE_LOW>; > }; I've no idea if DT allows this. The issue is that reg is the same for both nodes within the ethernet-phy-package container, and ethernet-phy@28. They are all addresses on the same MDIO bus. We are parsing this bus structure ourselves in __of_mdiobus_register(), so we could make it work, but i don't know if we should make it work. Andrew
On Mon, Nov 20, 2023 at 09:44:58PM +0100, Andrew Lunn wrote: > On Mon, Nov 20, 2023 at 02:50:30PM +0100, Christian Marangi wrote: > > Document ethernet PHY package nodes used to describe PHY shipped in > > bundle of 4-5 PHY. These particular PHY require specific PHY in the > > package for global onfiguration of the PHY package. > > > > 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. > > I think you are being overly narrow here. The 'global' registers could > be spread over multiple addresses. Particularly for a C22 PHY. I > suppose they could even be in a N+1 address space, where there is no > PHY at all. > > Where the global registers are is specific to a PHY package > vendor/model. For this reason in particular, the package needs a specific compatible. > The PHY driver should know this. All the PHY driver > needs to know is some sort of base offset. PHY0 in this package is > using address X. It can then use relative addressing from this base to > access the global registers for this package. > > > It's also possible to specify the property phy-mode to specify that the > > PHY package sets a global PHY interface mode and every PHY of the > > package requires to have the same PHY interface mode. > > I don't think it is what simple. See the QCA8084 for example. 3 of the > 4 PHYs must use QXGMII. The fourth PHY can also use QXGMII but it can > be multiplexed to a different PMA and use 1000BaseX, SGMII or > 2500BaseX. > > I do think we need somewhere to put package properties. But i don't > think phy-mode is such a property. At the moment, i don't have a good > example of a package property. What about power supplies and reset/enable lines? Rob
> > I do think we need somewhere to put package properties. But i don't > > think phy-mode is such a property. At the moment, i don't have a good > > example of a package property. > > What about power supplies and reset/enable lines? Yes, good point. I can imagine some packages sharing regulators. Reset might also be shared, but it makes things messy to handle. Andrew
On Tue, Nov 21, 2023 at 03:45:42PM +0100, Andrew Lunn wrote: > > > I do think we need somewhere to put package properties. But i don't > > > think phy-mode is such a property. At the moment, i don't have a good > > > example of a package property. > > > > What about power supplies and reset/enable lines? > > Yes, good point. I can imagine some packages sharing regulators. Reset > might also be shared, but it makes things messy to handle. > Sooooo.... Sorry if I insist but I would really love to have something ""stable"" to move this further. (the changes are easy enough so it's really a matter of finding a good DT structure) Maybe a good idea would be summarize the concern and see what solution was proposed: Concern list: 1. ethernet-phy-package MUST be placed in mdio node (not in ethernet, the example was wrong anyway) and MUST have an addr Current example doesn't have an addr. I would prefer this way but no problem in changing this. Solution: - Add reg to the ethernet-phy-package node with the base address of the PHY package (base address = the first PHY address of the package) We will have a PHY node with the same address of the PHY package node. Each PHY node in the PHY package node will have reg set to the REAL address in the mdio bus. 2. global-phys are redundant and can be dropped. They are used to facilitate and make it less obscure how the PHY package is described. Can totally be handled internally by the PHY driver. Still I would prefer to keep them as is. Solution: - Drop the thing and leave the PHY driver handle it with hardcoded values. Due to point 1, the shared struct will have the base address of the PHY package and will be handle to reference the global PHY at an offset from the base address. 3. phy-mode is problematic. It's an optional value to enforce a specific mode for each PHY in the package. For complex configuration the mode won't be defined. Solution: - Rename it to package-phy-mode to make it less confusing. - Add an additional function that PHY package can use to make custom validation on the mode for every PHY attached (in the PHY package). Would make it less clear but more flexible for complex configuration. Maybe both solution can be implemented and the special function is used if the mode is not defined? 4. Not finding a correct place to put PHY package info. I'm still convinced the mdio node is the correct place. - PHY package are PHY in bundle so they are actual PHY - We already have in the mdio node special handling (every DSA switch use custom compatible and PHY ID is not used to probe them normally) - Node this way won't be treated as PHY as they won't match the PHY node name pattern and also won't have the compatible pattern for PHY. Solution: - ethernet-phy-package node is OK given a reg is defined. These are the 4 concern we have currently, hoping I didn't miss any, I hope we can sort those so I can send a v2 and make some progress on this.
On Wed, Nov 22, 2023 at 07:32:22PM +0100, Christian Marangi wrote: > On Tue, Nov 21, 2023 at 03:45:42PM +0100, Andrew Lunn wrote: > > > > I do think we need somewhere to put package properties. But i don't > > > > think phy-mode is such a property. At the moment, i don't have a good > > > > example of a package property. > > > > > > What about power supplies and reset/enable lines? > > > > Yes, good point. I can imagine some packages sharing regulators. Reset > > might also be shared, but it makes things messy to handle. > > > > Sooooo.... Sorry if I insist but I would really love to have something > ""stable"" to move this further. (the changes are easy enough so it's > really a matter of finding a good DT structure) > > Maybe a good idea would be summarize the concern and see what solution > was proposed: > > Concern list: > 1. ethernet-phy-package MUST be placed in mdio node (not in ethernet, > the example was wrong anyway) and MUST have an addr > > Current example doesn't have an addr. I would prefer this way but > no problem in changing this. > > Solution: > - Add reg to the ethernet-phy-package node with the base address of > the PHY package (base address = the first PHY address of the > package) Yes. > We will have a PHY node with the same address of the PHY package > node. Each PHY node in the PHY package node will have reg set to > the REAL address in the mdio bus. Basically Yes. I actually think the first sentence is not 100% correct. It could be all the package configuration registers are in the base address, without an actual PHY. The PHYs then follow at addresses above it. I can also imagine a case where the first PHY in the package is not used, so its not listed at all. So i think it should be "We often have a PHY node with the same address of the PHY package node." > 3. phy-mode is problematic. > > It's an optional value to enforce a specific mode for each PHY in the > package. For complex configuration the mode won't be defined. > > Solution: > - Rename it to package-phy-mode to make it less confusing. > > - Add an additional function that PHY package can use to make custom > validation on the mode for every PHY attached (in the PHY package). > > Would make it less clear but more flexible for complex > configuration. Maybe both solution can be implemented and the > special function is used if the mode is not defined? The description you give is that there are two SERDES, and both could be connected to a MAC. What does package-phy-mode represent then? Luo Jie patch for the qca8084 seems to have the same issue. It has two SERDES/PMA, and both could be connected to the MACs. So it seems like QCA devices don't actually fit this model. If we want to describe the package link mode, we probably need to list each PMA, and have a property in the PMA node indicating what link mode the PMA is operating at. At least for the moment, its not clear we actually need this at all. It seems like the phy-mode is all we need. The PHY driver should know what values are valid per port, and so can validate the value. > 4. Not finding a correct place to put PHY package info. > > I'm still convinced the mdio node is the correct place. > - PHY package are PHY in bundle so they are actual PHY > - We already have in the mdio node special handling (every DSA switch > use custom compatible and PHY ID is not used to probe them > normally) > - Node this way won't be treated as PHY as they won't match the PHY > node name pattern and also won't have the compatible pattern for > PHY. We do need a compatible for the package. The kernel is unlikely to use it, but the validation tools will. Each package can have its own DT properties, so we need a .yaml to describe those properties. So i would expect to have a "qca807x-package" compatible, which then lists the tx driver strength etc. The PHYs within the package don't need compatible, they are just linux PHYs, probed using the same code as PHYs outside of a package. Andrew
On Thu, Nov 23, 2023 at 04:30:48AM +0100, Andrew Lunn wrote: > On Wed, Nov 22, 2023 at 07:32:22PM +0100, Christian Marangi wrote: > > On Tue, Nov 21, 2023 at 03:45:42PM +0100, Andrew Lunn wrote: > > > > > I do think we need somewhere to put package properties. But i don't > > > > > think phy-mode is such a property. At the moment, i don't have a good > > > > > example of a package property. > > > > > > > > What about power supplies and reset/enable lines? > > > > > > Yes, good point. I can imagine some packages sharing regulators. Reset > > > might also be shared, but it makes things messy to handle. > > > > > > > Sooooo.... Sorry if I insist but I would really love to have something > > ""stable"" to move this further. (the changes are easy enough so it's > > really a matter of finding a good DT structure) > > > > Maybe a good idea would be summarize the concern and see what solution > > was proposed: > > > > Concern list: > > 1. ethernet-phy-package MUST be placed in mdio node (not in ethernet, > > the example was wrong anyway) and MUST have an addr > > > > Current example doesn't have an addr. I would prefer this way but > > no problem in changing this. > > > > Solution: > > - Add reg to the ethernet-phy-package node with the base address of > > the PHY package (base address = the first PHY address of the > > package) > > Yes. > Thanks. > > We will have a PHY node with the same address of the PHY package > > node. Each PHY node in the PHY package node will have reg set to > > the REAL address in the mdio bus. > > Basically Yes. I actually think the first sentence is not 100% > correct. It could be all the package configuration registers are in > the base address, without an actual PHY. The PHYs then follow at > addresses above it. I can also imagine a case where the first PHY in > the package is not used, so its not listed at all. So i think it > should be "We often have a PHY node with the same address of the PHY > package node." > Just to add details, also the opposite can happen. Where the last PHY in the bundle is used for global configuration but is not defined. This is another reason I wanted the global-phy proprerty, having to reference them even if they are not used better describe the actual PHY address used in the mdio bus. If the PHY is handled internally and omitted in DT then people might think that address is not used. > > 3. phy-mode is problematic. > > > > It's an optional value to enforce a specific mode for each PHY in the > > package. For complex configuration the mode won't be defined. > > > > Solution: > > - Rename it to package-phy-mode to make it less confusing. > > > > - Add an additional function that PHY package can use to make custom > > validation on the mode for every PHY attached (in the PHY package). > > > > Would make it less clear but more flexible for complex > > configuration. Maybe both solution can be implemented and the > > special function is used if the mode is not defined? > > The description you give is that there are two SERDES, and both could > be connected to a MAC. What does package-phy-mode represent then? > > Luo Jie patch for the qca8084 seems to have the same issue. It has two > SERDES/PMA, and both could be connected to the MACs. So it seems like > QCA devices don't actually fit this model. If we want to describe the > package link mode, we probably need to list each PMA, and have a > property in the PMA node indicating what link mode the PMA is > operating at. > > At least for the moment, its not clear we actually need this at > all. It seems like the phy-mode is all we need. The PHY driver should > know what values are valid per port, and so can validate the value. > Just to be more precise qca807x can operate in 3 different mode: (this is controlled by the MODE_CFG bits) - QSGMII: 5 copper port - PSGMII: 5 copper port - PSGMII: 4 copper port + 1 combo (that can be both fiber or copper) When mode is set to QSGMII each PHY needs to have the matching mode or we have no traffic. It makes it difficult driver side to understand what mode should be enforced with all kind of parsing or magic in private struct. Maybe for this specific case would be ok to introduce a simple specific proprerty? Like qca,qsgmii_mode ? > > 4. Not finding a correct place to put PHY package info. > > > > I'm still convinced the mdio node is the correct place. > > - PHY package are PHY in bundle so they are actual PHY > > - We already have in the mdio node special handling (every DSA switch > > use custom compatible and PHY ID is not used to probe them > > normally) > > - Node this way won't be treated as PHY as they won't match the PHY > > node name pattern and also won't have the compatible pattern for > > PHY. > > We do need a compatible for the package. The kernel is unlikely to use > it, but the validation tools will. Each package can have its own DT > properties, so we need a .yaml to describe those properties. So i > would expect to have a "qca807x-package" compatible, which then lists > the tx driver strength etc. The PHYs within the package don't need > compatible, they are just linux PHYs, probed using the same code as > PHYs outside of a package. > Since the idea is add OF support for the PHY we also need a generic compatible for it. Is it ok to have something like compatible = "ethernet-phy-package", "qca807x-phy-package"; With "ethernet-phy-package" a must and "qca807x-phy-package" used only if additional property are used. My current idea was to use select and base everything on the possible PHY compatible (and it does work, tested by adding bloat in the DT example and seeing if the schema was rejected). Had this idea since the compatible would never be used.
> Just to be more precise qca807x can operate in 3 different mode: > (this is controlled by the MODE_CFG bits) > - QSGMII: 5 copper port 4 slots over QSGMII, plus the second SERDES is connected to the MAC using SGMII/1000BaseX? > - PSGMII: 5 copper port 5 slots over QSGMII, the second SERDES is idle? > - PSGMII: 4 copper port + 1 combo (that can be both fiber or copper) 5 slots over QSGMII, with the second SERDES connected to an SFP cage. Are ports 1-4 always connected to the P/Q SGMII. Its only port 5 which can use the second SERDES? Does changing between QSGMII and PSGMII really change the protocol run over the multiplex link? The clock rate is slower, there are only 4 multiplexed slots vs five? Or does it keep using PSGMII and leaves one slot I can see how it is messy to validate, if you only have phy-mode. So maybe MODE_CFG is a package property. You then can validate the phy-mode against MODE_CFG. Andrew
On Thu, Nov 23, 2023 at 03:27:05PM +0100, Andrew Lunn wrote: > > Just to be more precise qca807x can operate in 3 different mode: > > (this is controlled by the MODE_CFG bits) > > > - QSGMII: 5 copper port > > 4 slots over QSGMII, plus the second SERDES is connected to the MAC > using SGMII/1000BaseX? > > > - PSGMII: 5 copper port > > 5 slots over QSGMII, the second SERDES is idle? > > > - PSGMII: 4 copper port + 1 combo (that can be both fiber or copper) > > 5 slots over QSGMII, with the second SERDES connected to an SFP cage. > > Are ports 1-4 always connected to the P/Q SGMII. Its only port 5 which > can use the second SERDES? I think what would really help here is if there was an ascii table to describe the configurations, rather than trying to put it into words.
On Thu, Nov 23, 2023 at 02:35:31PM +0000, Russell King (Oracle) wrote: > On Thu, Nov 23, 2023 at 03:27:05PM +0100, Andrew Lunn wrote: > > > Just to be more precise qca807x can operate in 3 different mode: > > > (this is controlled by the MODE_CFG bits) > > > > > - QSGMII: 5 copper port > > > > 4 slots over QSGMII, plus the second SERDES is connected to the MAC > > using SGMII/1000BaseX? > > > > > - PSGMII: 5 copper port > > > > 5 slots over QSGMII, the second SERDES is idle? > > > > > - PSGMII: 4 copper port + 1 combo (that can be both fiber or copper) > > > > 5 slots over QSGMII, with the second SERDES connected to an SFP cage. > > > > Are ports 1-4 always connected to the P/Q SGMII. Its only port 5 which > > can use the second SERDES? > > I think what would really help here is if there was an ascii table to > describe the configurations, rather than trying to put it into words. Yes. And also for ipq4019. We need to merge these two threads of conversation, since in the end they are probably the same driver, same device tree etc. Andrew
> compatible = "ethernet-phy-package", "qca807x-phy-package"; > > With "ethernet-phy-package" a must and "qca807x-phy-package" used only > if additional property are used. > > My current idea was to use select and base everything on the possible > PHY compatible (and it does work, tested by adding bloat in the DT > example and seeing if the schema was rejected). Had this idea since the > compatible would never be used. The DT people are unhappy with PHYs don't use compatibles, so validation does not work. It probably too late to add compatibles to very PHY driver. But this is new development work, we don't have any history. So we can add a compatible per package to make the validation tools work. So for parsing the tree in the kernel we look for 'ethernet-phy-package'. For validating the tree using the yaml tools we use the 'qca807x-phy-package'. Andrew
On Thu, Nov 23, 2023 at 03:57:58PM +0100, Andrew Lunn wrote: > On Thu, Nov 23, 2023 at 02:35:31PM +0000, Russell King (Oracle) wrote: > > On Thu, Nov 23, 2023 at 03:27:05PM +0100, Andrew Lunn wrote: > > > > Just to be more precise qca807x can operate in 3 different mode: > > > > (this is controlled by the MODE_CFG bits) > > > > > > > - QSGMII: 5 copper port > > > > > > 4 slots over QSGMII, plus the second SERDES is connected to the MAC > > > using SGMII/1000BaseX? > > > > > > > - PSGMII: 5 copper port > > > > > > 5 slots over QSGMII, the second SERDES is idle? > > > > > > > - PSGMII: 4 copper port + 1 combo (that can be both fiber or copper) > > > > > > 5 slots over QSGMII, with the second SERDES connected to an SFP cage. > > > > > > Are ports 1-4 always connected to the P/Q SGMII. Its only port 5 which > > > can use the second SERDES? > > > > I think what would really help here is if there was an ascii table to > > describe the configurations, rather than trying to put it into words. > > Yes. > > And also for ipq4019. We need to merge these two threads of > conversation, since in the end they are probably the same driver, same > device tree etc. > For everyone that missed Robert response in patch 12 let me quote him also here. " Hi Andrew, I think that the description is confusing. QCA807x supports 3 different modes: 1. PSGMII (5 copper ports) 2. PSGMII (4 copper ports + 1 combo port) 3. QSGMII+SGMII So, in case option 2 is selected then the combo port can also be used for 1000Base-X and 100Base-FX modules or copper and it will autodetect the exact media. This is supported via the SFP op-s and I have been using it without issues for a while. I have not tested option 3 in combination with SFP to the copper module so I cant say whether that works. From what I can gather from the typical usage examples in the datasheet, this QSMII+SGMII mode is basically intended as a backward compatibility thing as only QCA SoC-s have PSGMII support so that you could still use SoC-s with QSGMII and SGMII support only. So there is no way to control the SerDes-es individually, only the global mode can be changed via the Chip configuration register in the Combo port. You can see the block diagram of this PHY in this public PDF on page 2[1]. [1] https://content.codico.com/fileadmin/media/download/datasheets/qualcomm/qualcomm_qca8075.pdf "
On Thu, Nov 23, 2023 at 04:07:14PM +0100, Andrew Lunn wrote: > > compatible = "ethernet-phy-package", "qca807x-phy-package"; > > > > With "ethernet-phy-package" a must and "qca807x-phy-package" used only > > if additional property are used. > > > > My current idea was to use select and base everything on the possible > > PHY compatible (and it does work, tested by adding bloat in the DT > > example and seeing if the schema was rejected). Had this idea since the > > compatible would never be used. > > The DT people are unhappy with PHYs don't use compatibles, so > validation does not work. It probably too late to add compatibles to > very PHY driver. But this is new development work, we don't have any > history. So we can add a compatible per package to make the validation > tools work. > > So for parsing the tree in the kernel we look for > 'ethernet-phy-package'. For validating the tree using the yaml tools > we use the 'qca807x-phy-package'. > Ok clear, what about the generic ethernet-phy-package.yaml? Idea was to describe common properties there and then specific PHY package would add every common property with $ref and add their special ones on top of that. Would that be ok? (similar to the current implementation with ethernet-phy-package and qcom,qca807x with the only difference that qcom,qca807x.yaml would also have the compatible set (currently missing from this RFC)
On 11/24/2023 3:33 AM, Christian Marangi wrote: > On Thu, Nov 23, 2023 at 03:57:58PM +0100, Andrew Lunn wrote: >> On Thu, Nov 23, 2023 at 02:35:31PM +0000, Russell King (Oracle) wrote: >>> On Thu, Nov 23, 2023 at 03:27:05PM +0100, Andrew Lunn wrote: >>>>> Just to be more precise qca807x can operate in 3 different mode: >>>>> (this is controlled by the MODE_CFG bits) >>>> >>>>> - QSGMII: 5 copper port >>>> >>>> 4 slots over QSGMII, plus the second SERDES is connected to the MAC >>>> using SGMII/1000BaseX? >>>> >>>>> - PSGMII: 5 copper port >>>> >>>> 5 slots over QSGMII, the second SERDES is idle? >>>> >>>>> - PSGMII: 4 copper port + 1 combo (that can be both fiber or copper) >>>> >>>> 5 slots over QSGMII, with the second SERDES connected to an SFP cage. >>>> >>>> Are ports 1-4 always connected to the P/Q SGMII. Its only port 5 which >>>> can use the second SERDES? >>> >>> I think what would really help here is if there was an ascii table to >>> describe the configurations, rather than trying to put it into words. >> >> Yes. >> >> And also for ipq4019. We need to merge these two threads of >> conversation, since in the end they are probably the same driver, same >> device tree etc. >> > > For everyone that missed Robert response in patch 12 let me quote him > also here. > > " > Hi Andrew, > I think that the description is confusing. > QCA807x supports 3 different modes: > 1. PSGMII (5 copper ports) > 2. PSGMII (4 copper ports + 1 combo port) > 3. QSGMII+SGMII > > So, in case option 2 is selected then the combo port can also be used for > 1000Base-X and 100Base-FX modules or copper and it will autodetect the > exact media. > This is supported via the SFP op-s and I have been using it without issues > for a while. > > I have not tested option 3 in combination with SFP to the copper > module so I cant > say whether that works. For the option3, the last PHY works on SGMII mode, then it can't be used with SFP, only copper is supported in this mode. > From what I can gather from the typical usage examples in the > datasheet, this QSMII+SGMII > mode is basically intended as a backward compatibility thing as only > QCA SoC-s have PSGMII > support so that you could still use SoC-s with QSGMII and SGMII support only. > > So there is no way to control the SerDes-es individually, only the > global mode can be changed via > the Chip configuration register in the Combo port. > > You can see the block diagram of this PHY in this public PDF on page 2[1]. > > [1] https://content.codico.com/fileadmin/media/download/datasheets/qualcomm/qualcomm_qca8075.pdf > " >
On Fri, Nov 24, 2023 at 07:49:27PM +0800, Jie Luo wrote: > > > On 11/24/2023 3:33 AM, Christian Marangi wrote: > > On Thu, Nov 23, 2023 at 03:57:58PM +0100, Andrew Lunn wrote: > > > On Thu, Nov 23, 2023 at 02:35:31PM +0000, Russell King (Oracle) wrote: > > > > On Thu, Nov 23, 2023 at 03:27:05PM +0100, Andrew Lunn wrote: > > > > > > Just to be more precise qca807x can operate in 3 different mode: > > > > > > (this is controlled by the MODE_CFG bits) > > > > > > > > > > > - QSGMII: 5 copper port > > > > > > > > > > 4 slots over QSGMII, plus the second SERDES is connected to the MAC > > > > > using SGMII/1000BaseX? > > > > > > > > > > > - PSGMII: 5 copper port > > > > > > > > > > 5 slots over QSGMII, the second SERDES is idle? > > > > > > > > > > > - PSGMII: 4 copper port + 1 combo (that can be both fiber or copper) > > > > > > > > > > 5 slots over QSGMII, with the second SERDES connected to an SFP cage. > > > > > > > > > > Are ports 1-4 always connected to the P/Q SGMII. Its only port 5 which > > > > > can use the second SERDES? > > > > > > > > I think what would really help here is if there was an ascii table to > > > > describe the configurations, rather than trying to put it into words. > > > > > > Yes. > > > > > > And also for ipq4019. We need to merge these two threads of > > > conversation, since in the end they are probably the same driver, same > > > device tree etc. > > > > > > > For everyone that missed Robert response in patch 12 let me quote him > > also here. > > > > " > > Hi Andrew, > > I think that the description is confusing. > > QCA807x supports 3 different modes: > > 1. PSGMII (5 copper ports) > > 2. PSGMII (4 copper ports + 1 combo port) > > 3. QSGMII+SGMII > > > > So, in case option 2 is selected then the combo port can also be used for > > 1000Base-X and 100Base-FX modules or copper and it will autodetect the > > exact media. > > This is supported via the SFP op-s and I have been using it without issues > > for a while. > > > > I have not tested option 3 in combination with SFP to the copper > > module so I cant > > say whether that works. > > For the option3, the last PHY works on SGMII mode, then it can't be > used with SFP, only copper is supported in this mode. So, from what you've written, and looking at the PDF for QCA8075: First Serdes mode Second Serdes mode Option 1 PSGMII for copper Disabled ports 0-4 Option 2 PSGMII for copper 1000BASE-X / 100BASE-FX ports 0-4 Option 3 QSGMII for copper SGMII for ports 0-3 copper port 4 In all cases, ports 0-4 have a copper (baseT/baseTx/baseTe) port available. This is a much easier to understand presentation than writing out a longwinded description of the three modes. Please include the table and the statement below the table in the commit description as that is necessary to describe the hardware setup being addressed in these patches. Thanks.
> First Serdes mode Second Serdes mode > Option 1 PSGMII for copper Disabled > ports 0-4 > Option 2 PSGMII for copper 1000BASE-X / 100BASE-FX > ports 0-4 > Option 3 QSGMII for copper SGMII for > ports 0-3 copper port 4 With option 2, can the second SERDES also do SGMII? You are likely to need that when a Copper SFP module is inserted into the cage. Andrew
On Fri, Nov 24, 2023 at 03:44:20PM +0100, Andrew Lunn wrote: > > First Serdes mode Second Serdes mode > > Option 1 PSGMII for copper Disabled > > ports 0-4 > > Option 2 PSGMII for copper 1000BASE-X / 100BASE-FX > > ports 0-4 > > Option 3 QSGMII for copper SGMII for > > ports 0-3 copper port 4 > > With option 2, can the second SERDES also do SGMII? You are likely to > need that when a Copper SFP module is inserted into the cage. The document states "The fiber port supports 1000BASE-X/100BASE-FX". The same is true of Marvell 88x3310's fiber port - it supports only fiber not SGMII. This is actually something else that - when the patches for stacked PHYs mature - will need to be addressed. If we have a 1G copper SFP plugged into an interface that only supports 1000base-X then we need a way to switch the PHY on the SFP module to 1000base-X if it's in SGMII mode. Some copper SFPs come up in 1000base-X mode, and we currently rely on the 88e1111 driver to switch them to SGMII mode. Others do want SGMII mode (like Mikrotik RJ01 where the PHY is inaccessible and thus can't be reconfigured.)
On Fri, Nov 24, 2023 at 06:59:23PM +0200, Vladimir Oltean wrote: > On Wed, Nov 22, 2023 at 07:32:22PM +0100, Christian Marangi wrote: > > Sooooo.... Sorry if I insist but I would really love to have something > > ""stable"" to move this further. (the changes are easy enough so it's > > really a matter of finding a good DT structure) > > > > Maybe a good idea would be summarize the concern and see what solution > > was proposed: > > Sorry, I didn't follow the entire discussion. I hope I'm not too far off > with my understanding of your problems. > > I think you are hitting some of the same points I have hit with DSA. > The PHY package could be considered an SoC with lots of peripherals on > it, for which you'd want separate drivers. Just like a DSA switch would. > I don't think it's exactly phylib's place to deal with that, just like > it's not DSA's place to deal with complex SoCs, just with the switching > IP (like the Ethernet PHY IP for phylib). > https://lore.kernel.org/lkml/20221222134844.lbzyx5hz7z5n763n@skbuf/ > > Why does the ethernet-phy-package DT binding attempt to be so grand and > generic? I would expect the 180 degree opposite. Make it have a _single_ > compatible of "qcom,qca807x" (but don't use an "x" wildcard, do specify > the full package name). > > Make it have a "reg" property which is the base MDIO address of the package. > > Write an mdio_device driver that probes on that. The PHY core already > knows that if a child on the MDIO bus has a compatible string of the > normal form (not like "ethernet-phy-id004d.d0b2"), then it's looking at > an mdio_device. > > Make the OF node of the package have an "mdio" child with its own > compatible string, which represents the place where PHYs are. The driver > for the "mdio" child has a very simple implementation of the mii_bus > ops, which just calls into the device parent (it can assume a certain > parent implementation and private data structures). > > Lateral to the "mdio" child node of the "qcom,qca807x" package node, you > could put any other device tree properties that you want. > > Make the mdio_device driver for "qcom,qca807x" use shared code if you > want - but keep the device tree structure hardware-specific. Look at the > compatible strings that e.g. the drivers/mfd/simple-mfd-i2c.c driver > probes on. You could always change the driver implementation for a > certain compatible string, but you'll be stuck with the ultra-generic > compatible = "ethernet-phy-package", which has the problems that you > mention. > The main reason is the fact that PHY package are already a thing and API are already there (phy_package_join/leave...) so we just lack any way to support this in DT without using specialized code in the PHY driver. This is really completing the feature. The only reason for the generic "ethernet-phy-package" compatible is to have a solid detection of the node in PHY core. (I assume parsing the node name might be problematic? Or maybe not now that we require adding a reg to it) Also I don't expect tons of special properties for PHY package, with the current probe/config implementation, a PHY driver have lots of flexibility in all kind of validation. Consider that the additional global-phys and global-phy-names are already expected to be dropped. (we know the PHY package base address and we can calculate the offset of the global phy from there in the PHY package probe) And even the phy-mode has been scrapped for more specific solution... (some assumption can be done on probe by checking the PHY mode and set stuff accordingly or even do parsing in the PHY package node as we pass the OF node of the phy package) The PHY package node would be reduced to a simple compatible (and even this can be dropped) and a reg. I feel there is a big chance here to generalize it and prevent any kind of mess with all kind of similar/equal code that just do the same thing. (and we already have an example with the PHY package API usage with every PHY having the same exact pattern for probe/config and nothing describing that the PHY are actually a package in DT) Hope it all makes sense to you. > > > > Concern list: > > 1. ethernet-phy-package MUST be placed in mdio node (not in ethernet, > > the example was wrong anyway) and MUST have an addr > > > > Current example doesn't have an addr. I would prefer this way but > > no problem in changing this. > > > > Solution: > > - Add reg to the ethernet-phy-package node with the base address of > > the PHY package (base address = the first PHY address of the > > package) > > Correct, what I'm saying is compatible with this. > > > > > We will have a PHY node with the same address of the PHY package > > node. Each PHY node in the PHY package node will have reg set to > > the REAL address in the mdio bus. > > If the real PHY IPs are children of the package rather than on the same > level with it, I don't think this will be a problem. I wonder if some > address translation could be done with the "ranges" device tree property. > I've never seen this with MDIO though. > I can check it, I would love some way to describe the address used by the PHY package. (since everything will be handled internally with offsets, would be good to define in DT that (for example) addrs from 0 to 5 are used). Some PHY might be not attached but still used for global configuration of the PHY package. > > 4. Not finding a correct place to put PHY package info. > > > > I'm still convinced the mdio node is the correct place. > > - PHY package are PHY in bundle so they are actual PHY > > - We already have in the mdio node special handling (every DSA switch > > use custom compatible and PHY ID is not used to probe them > > normally) > > - Node this way won't be treated as PHY as they won't match the PHY > > node name pattern and also won't have the compatible pattern for > > PHY. > > > > Solution: > > - ethernet-phy-package node is OK given a reg is defined. > > I agree that it should sit under the MDIO node. I disagree with the idea > of a standardized binding for PHY packages.
On Fri, Nov 24, 2023 at 4:16 PM Russell King (Oracle) <linux@armlinux.org.uk> wrote: > > On Fri, Nov 24, 2023 at 03:44:20PM +0100, Andrew Lunn wrote: > > > First Serdes mode Second Serdes mode > > > Option 1 PSGMII for copper Disabled > > > ports 0-4 > > > Option 2 PSGMII for copper 1000BASE-X / 100BASE-FX > > > ports 0-4 > > > Option 3 QSGMII for copper SGMII for > > > ports 0-3 copper port 4 > > > > With option 2, can the second SERDES also do SGMII? You are likely to > > need that when a Copper SFP module is inserted into the cage. > > The document states "The fiber port supports 1000BASE-X/100BASE-FX". > > The same is true of Marvell 88x3310's fiber port - it supports only > fiber not SGMII. This is actually something else that - when the > patches for stacked PHYs mature - will need to be addressed. If we > have a 1G copper SFP plugged into an interface that only supports > 1000base-X then we need a way to switch the PHY on the SFP module > to 1000base-X if it's in SGMII mode. > > Some copper SFPs come up in 1000base-X mode, and we currently rely > on the 88e1111 driver to switch them to SGMII mode. Others do want > SGMII mode (like Mikrotik RJ01 where the PHY is inaccessible and > thus can't be reconfigured.) I can confirm that SGMII mode doesn't work with Option 2, I have tested this a while ago with Mikrotik RJ01 (I think it has AR803x, but its not accessible as you pointed out) and it was somewhat working but in half duplex only and dropping packets. Currently, SFP mode is checked and only 1000Base-X and 100Base-FX are accepted, otherwise the module insert will return and error for unsupported mode. Regards, Robert > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Wed, Nov 22, 2023 at 07:32:22PM +0100, Christian Marangi wrote: > Sooooo.... Sorry if I insist but I would really love to have something > ""stable"" to move this further. (the changes are easy enough so it's > really a matter of finding a good DT structure) > > Maybe a good idea would be summarize the concern and see what solution > was proposed: Sorry, I didn't follow the entire discussion. I hope I'm not too far off with my understanding of your problems. I think you are hitting some of the same points I have hit with DSA. The PHY package could be considered an SoC with lots of peripherals on it, for which you'd want separate drivers. Just like a DSA switch would. I don't think it's exactly phylib's place to deal with that, just like it's not DSA's place to deal with complex SoCs, just with the switching IP (like the Ethernet PHY IP for phylib). https://lore.kernel.org/lkml/20221222134844.lbzyx5hz7z5n763n@skbuf/ Why does the ethernet-phy-package DT binding attempt to be so grand and generic? I would expect the 180 degree opposite. Make it have a _single_ compatible of "qcom,qca807x" (but don't use an "x" wildcard, do specify the full package name). Make it have a "reg" property which is the base MDIO address of the package. Write an mdio_device driver that probes on that. The PHY core already knows that if a child on the MDIO bus has a compatible string of the normal form (not like "ethernet-phy-id004d.d0b2"), then it's looking at an mdio_device. Make the OF node of the package have an "mdio" child with its own compatible string, which represents the place where PHYs are. The driver for the "mdio" child has a very simple implementation of the mii_bus ops, which just calls into the device parent (it can assume a certain parent implementation and private data structures). Lateral to the "mdio" child node of the "qcom,qca807x" package node, you could put any other device tree properties that you want. Make the mdio_device driver for "qcom,qca807x" use shared code if you want - but keep the device tree structure hardware-specific. Look at the compatible strings that e.g. the drivers/mfd/simple-mfd-i2c.c driver probes on. You could always change the driver implementation for a certain compatible string, but you'll be stuck with the ultra-generic compatible = "ethernet-phy-package", which has the problems that you mention. > > Concern list: > 1. ethernet-phy-package MUST be placed in mdio node (not in ethernet, > the example was wrong anyway) and MUST have an addr > > Current example doesn't have an addr. I would prefer this way but > no problem in changing this. > > Solution: > - Add reg to the ethernet-phy-package node with the base address of > the PHY package (base address = the first PHY address of the > package) Correct, what I'm saying is compatible with this. > > We will have a PHY node with the same address of the PHY package > node. Each PHY node in the PHY package node will have reg set to > the REAL address in the mdio bus. If the real PHY IPs are children of the package rather than on the same level with it, I don't think this will be a problem. I wonder if some address translation could be done with the "ranges" device tree property. I've never seen this with MDIO though. > 4. Not finding a correct place to put PHY package info. > > I'm still convinced the mdio node is the correct place. > - PHY package are PHY in bundle so they are actual PHY > - We already have in the mdio node special handling (every DSA switch > use custom compatible and PHY ID is not used to probe them > normally) > - Node this way won't be treated as PHY as they won't match the PHY > node name pattern and also won't have the compatible pattern for > PHY. > > Solution: > - ethernet-phy-package node is OK given a reg is defined. I agree that it should sit under the MDIO node. I disagree with the idea of a standardized binding for PHY packages.
On Fri, Nov 24, 2023 at 05:25:16PM +0100, Christian Marangi wrote: > The main reason is the fact that PHY package are already a thing and API > are already there (phy_package_join/leave...) so we just lack any way to > support this in DT without using specialized code in the PHY driver. > > This is really completing the feature. Hmm, I see struct phy_package_shared as a mechanism to tell phylib that multiple device structures are actually related with each other, because the device core, and their parent bus, has no idea. If you're under control of the parent bus code and you can probe PHY devices in any order you want and do whatever you want before probing them, I don't see why you would need struct phy_package_shared any longer? I don't see why this feature needs to be completed, if that involves changes to the device tree structure. PHY packages assumed no changes to the device tree (they rely on a hacky interpretation of the MDIO address AFAIU). If we change that basic premise, all implementation options are on the table, I think. > The only reason for the generic "ethernet-phy-package" compatible is to > have a solid detection of the node in PHY core. (I assume parsing the > node name might be problematic? Or maybe not now that we require adding > a reg to it) Our opinions seem to differ, but I don't think that the package needs a solid detection of the node in the PHY core :) I think phy_devices and mdio_devices already cover everything that's necessary to build a solution. > Also I don't expect tons of special properties for PHY package, with the > current probe/config implementation, a PHY driver have lots of > flexibility in all kind of validation. > > Consider that the additional global-phys and global-phy-names are > already expected to be dropped. > (we know the PHY package base address and we can calculate the offset of > the global phy from there in the PHY package probe) > > And even the phy-mode has been scrapped for more specific solution... > (some assumption can be done on probe by checking the PHY mode and set > stuff accordingly or even do parsing in the PHY package node as we pass > the OF node of the phy package) > > The PHY package node would be reduced to a simple compatible (and even > this can be dropped) and a reg. So why does it need to be described in DT, at this stage? :) > I feel there is a big chance here to generalize it and prevent any kind > of mess with all kind of similar/equal code that just do the same thing. > (and we already have an example with the PHY package API usage with > every PHY having the same exact pattern for probe/config and nothing > describing that the PHY are actually a package in DT) > > Hope it all makes sense to you.
> I think you are hitting some of the same points I have hit with DSA. > The PHY package could be considered an SoC with lots of peripherals on > it, for which you'd want separate drivers. At least at the moment, this is not true. The package does just contain PHYs. But it also has some properties which are shared across those PHYs, e.g. reset. What you describe might become true in the future. e.g. The LED/GPIO controller is currently part of the PHY, and each PHY has its own. I could however imagine that becomes a block of its own, outside of the PHY address space, and maybe it might want its own class LED driver. Some PHYs have temperature sensors, which could be a package sensor, so could in theory be an individual hwmon driver. However, i've not yet seen such a package. Do we consider this now? At the moment i don't see an MFD style system is required. We could crystal ball gaze and come up with some requirements, but i would prefer to have some real devices and datasheets. Without them, we will get the requirements wrong. I also think we are not that far away from it, in terms of DT, if you consider the later comments. I suggested we need a phy package specific compatible. At the moment, it will be ignored by the kernel, the kernel does not need it, it probes the PHYs in the current way, using the ID registers. But it could in future be used to probe a real driver, which could be an MFD style driver. We need to see updated DT binding examples, but i don't see why we cannot slot it in at a later date. Andrew
On Fri, Nov 24, 2023 at 07:35:35PM +0100, Andrew Lunn wrote: > > I think you are hitting some of the same points I have hit with DSA. > > The PHY package could be considered an SoC with lots of peripherals on > > it, for which you'd want separate drivers. > > At least at the moment, this is not true. The package does just > contain PHYs. But it also has some properties which are shared across > those PHYs, e.g. reset. > > What you describe might become true in the future. e.g. The LED/GPIO > controller is currently part of the PHY, and each PHY has its own. I > could however imagine that becomes a block of its own, outside of the > PHY address space, and maybe it might want its own class LED > driver. Some PHYs have temperature sensors, which could be a package > sensor, so could in theory be an individual hwmon driver. However, > i've not yet seen such a package. > > Do we consider this now? At the moment i don't see an MFD style system > is required. We could crystal ball gaze and come up with some > requirements, but i would prefer to have some real devices and > datasheets. Without them, we will get the requirements wrong. > > I also think we are not that far away from it, in terms of DT, if you > consider the later comments. I suggested we need a phy package > specific compatible. At the moment, it will be ignored by the kernel, > the kernel does not need it, it probes the PHYs in the current way, > using the ID registers. But it could in future be used to probe a real > driver, which could be an MFD style driver. We need to see updated DT > binding examples, but i don't see why we cannot slot it in at a later > date. I'm not suggesting to go for MFD right away. Just with a structure that is extensible to possibly cover that. For now, a package node with a Qualcomm compatible, with the most minimal driver that forwards MDIO access to PHY children. I can't speak for the future of PHY drivers, since I don't know enough about PHYs. I'm just coming from the DSA background where I really wish we had this sort of infrastructure earlier. Now I have the SJA1110 which still lacks support for the interrupt controller for its integrated PHYs, and a bunch of other IP blocks in the package, because it's so incredibly hard to make the driver support the old-style and the new-style device trees.
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..2aa109e155d9 --- /dev/null +++ b/Documentation/devicetree/bindings/net/ethernet-phy-package.yaml @@ -0,0 +1,86 @@ +# 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 + +properties: + $nodename: + pattern: "^ethernet-phy-package(-[0-9]+)?$" + + compatible: + const: ethernet-phy-package + + '#address-cells': + description: number of address cells for the MDIO bus + const: 1 + + '#size-cells': + description: number of size cells on the MDIO bus + const: 0 + + global-phys: + $ref: /schemas/types.yaml#/definitions/phandle-array + minItems: 1 + maxItems: 31 + description: + List of phandle to the PHY in the package required and + used to configure the PHY package. + + global-phy-names: + $ref: /schemas/types.yaml#/definitions/string-array + minItems: 1 + maxItems: 31 + description: + List of names of the PHY defined in global-phys. + + phy-connection-type: + $ref: /schemas/net/ethernet-phy-mode-types.yaml#definitions/phy-connection-type + description: + Specifies global interface type for the PHY package. + + phy-mode: + $ref: "#/properties/phy-connection-type" + +patternProperties: + ^ethernet-phy(@[a-f0-9]+)?$: + $ref: /schemas/net/ethernet-phy.yaml# + +required: + - compatible + +dependencies: + global-phy-names: [global-phys] + +unevaluatedProperties: false + +examples: + - | + ethernet { + #address-cells = <1>; + #size-cells = <0>; + + ethernet-phy-package { + compatible = "ethernet-phy-package"; + #address-cells = <1>; + #size-cells = <0>; + + global-phys = <&phy4>; + global-phy-names = "base"; + + ethernet-phy@1 { + compatible = "ethernet-phy-ieee802.3-c22"; + reg = <1>; + }; + + phy4: ethernet-phy@4 { + compatible = "ethernet-phy-ieee802.3-c22"; + reg = <4>; + }; + }; + };
Document ethernet PHY package nodes used to describe PHY shipped in bundle of 4-5 PHY. These particular PHY require specific PHY in the package for global onfiguration of the PHY package. 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 should use the global-phys property and the global-phy-names to define PHY in the package required by the PHY driver for global configuration. It's also possible to specify the property phy-mode to specify that the PHY package sets a global PHY interface mode and every PHY of the package requires to have the same PHY interface mode. Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> --- .../bindings/net/ethernet-phy-package.yaml | 86 +++++++++++++++++++ 1 file changed, 86 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/ethernet-phy-package.yaml