| Message ID | 1453853499-11248-3-git-send-email-andrew@lunn.ch |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
On 26/01/16 16:11, Andrew Lunn wrote: > PHY devices may only list clause 22, 45, and their PHY identifier > values as compatible values. No other compatible strings are allowed. > Make this clear in the documentation, and remove examples where > make/model compatible strings are listed. Humm, should not we rather require Ethernet PHY Device Tree nodes to have *at least* a "ethernet-phy-ieee802.3-c22" or "ethernet-phy-ieee802.3-c45", and any other compatible string which further specifies the hardware is also welcome? Do you think this deserves another commit when net-next opens up again?
On Tue, Jan 26, 2016 at 04:33:11PM -0800, Florian Fainelli wrote: > On 26/01/16 16:11, Andrew Lunn wrote: > > PHY devices may only list clause 22, 45, and their PHY identifier > > values as compatible values. No other compatible strings are allowed. > > Make this clear in the documentation, and remove examples where > > make/model compatible strings are listed. > > Humm, should not we rather require Ethernet PHY Device Tree nodes to > have *at least* a "ethernet-phy-ieee802.3-c22" or > "ethernet-phy-ieee802.3-c45", and any other compatible string which > further specifies the hardware is also welcome? At the moment, "ethernet-phy-ieee802.3-c45" is used, we look for it and act upon it. "ethernet-phy-ieee802.3-c22" is not used anywhere, other than my new of_mdiobus_child_is_phy(). Also, for backwards compatibility with older blobs, we can never assume one or the other will be present. So you are suggesting we change around 200 ethphy nodes to add in "ethernet-phy-ieee802.3-c22", yet we don't actually do anything with it? Andrew
On 26/01/16 17:06, Andrew Lunn wrote: > On Tue, Jan 26, 2016 at 04:33:11PM -0800, Florian Fainelli wrote: >> On 26/01/16 16:11, Andrew Lunn wrote: >>> PHY devices may only list clause 22, 45, and their PHY identifier >>> values as compatible values. No other compatible strings are allowed. >>> Make this clear in the documentation, and remove examples where >>> make/model compatible strings are listed. >> >> Humm, should not we rather require Ethernet PHY Device Tree nodes to >> have *at least* a "ethernet-phy-ieee802.3-c22" or >> "ethernet-phy-ieee802.3-c45", and any other compatible string which >> further specifies the hardware is also welcome? > > At the moment, "ethernet-phy-ieee802.3-c45" is used, we look for it > and act upon it. "ethernet-phy-ieee802.3-c22" is not used anywhere, > other than my new of_mdiobus_child_is_phy(). Also, for backwards > compatibility with older blobs, we can never assume one or the other > will be present. > > So you are suggesting we change around 200 ethphy nodes to add in > "ethernet-phy-ieee802.3-c22", yet we don't actually do anything with > it? Well, we do now, since that is one of the results used by of_mdiobus_child_is_phy(), but you are right, this does not scale. What I would prefer seeing though is not removing nodes that have at least two compatible strings, including one that is "ethernet-phy-ieee802.3-c22", but those which have only one, like the marvell ones that you patch, should have either an additional "ethernet-phy-ieee802.3-c22", or none. Does that make sense?
> Well, we do now, since that is one of the results used by > of_mdiobus_child_is_phy(), It uses it, but do not rely on it, since for backwards compatibility, we cannot assume it is there. You can never change an optional parameter to a mandatory parameter in DT. To do so breaks backwards compatibility. > What I would prefer seeing though is not removing nodes that have at > least two compatible strings, including one that is > "ethernet-phy-ieee802.3-c22", but those which have only one, like the > marvell ones that you patch, should have either an additional > "ethernet-phy-ieee802.3-c22", or none. So you are saying, if there is an "ethernet-phy-ieee802.3-c22" or an "ethernet-phy-ieee802.3-45" you can also have any other random junk, which we are going to ignore, since we have no way to verify, hence we have to assume it is broken, yet we need to be backwards compatible with it. Did you notice: + { .compatible = "marvell,88e1310", }, + { .compatible = "marvell,88E1510", }, No consistency with the 'e'. We would just be encouraging people to add more inconsistent stuff. Andrew
On January 26, 2016 5:57:55 PM PST, Andrew Lunn <andrew@lunn.ch> wrote: >> Well, we do now, since that is one of the results used by >> of_mdiobus_child_is_phy(), > >It uses it, but do not rely on it, since for backwards compatibility, >we cannot assume it is there. > >You can never change an optional parameter to a mandatory parameter in >DT. To do so breaks backwards compatibility. > >> What I would prefer seeing though is not removing nodes that have at >> least two compatible strings, including one that is >> "ethernet-phy-ieee802.3-c22", but those which have only one, like the >> marvell ones that you patch, should have either an additional >> "ethernet-phy-ieee802.3-c22", or none. > >So you are saying, if there is an "ethernet-phy-ieee802.3-c22" or an >"ethernet-phy-ieee802.3-45" you can also have any other random junk, >which we are going to ignore, since we have no way to verify, hence we >have to assume it is broken, yet we need to be backwards compatible >with it. > >Did you notice: > >+ { .compatible = "marvell,88e1310", }, >+ { .compatible = "marvell,88E1510", }, > >No consistency with the 'e'. We would just be encouraging people to >add more inconsistent stuff. You are right, I am convinced now. If somebody needs to know the exact PHY ID a priori, then you are also likely to use the more descriptive compatible string including that ID. So: Acked-by: Florian Fainelli <f.fainelli@gmail.com>
On Tue, Jan 26, 2016 at 4:11 PM, Andrew Lunn <andrew@lunn.ch> wrote: > PHY devices may only list clause 22, 45, and their PHY identifier > values as compatible values. No other compatible strings are allowed. > Make this clear in the documentation, and remove examples where > make/model compatible strings are listed. > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> I'm not sure I agree with the disallowing here. It's common practice to use a specific compatible to describe the actual hardware used, in case it's needed in the future for some driver to distinguish behavior, etc. So while it should be required to include the clause compats, having a more specific one in there should be acceptable. -Olof
On Wed, Jan 27, 2016 at 08:41:56AM -0800, Olof Johansson wrote: > On Tue, Jan 26, 2016 at 4:11 PM, Andrew Lunn <andrew@lunn.ch> wrote: > > PHY devices may only list clause 22, 45, and their PHY identifier > > values as compatible values. No other compatible strings are allowed. > > Make this clear in the documentation, and remove examples where > > make/model compatible strings are listed. > > > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > > I'm not sure I agree with the disallowing here. It's common practice > to use a specific compatible to describe the actual hardware used, in > case it's needed in the future for some driver to distinguish > behavior, etc. > > So while it should be required to include the clause compats, having a > more specific one in there should be acceptable. Hi Olof Matching PHY devices to drivers has never used to compatible string, other than the "ethernet-phy-XXXX.YYYY" string. The PHY has two registers containing a manufacture id, device id and revision, registers 2 and 3. These are the XXXX and YYYY. The core code reads these values, or uses the values from the ethernet-phy-XXXX.YYYY, and uses them to find a driver which supports these values. A make/model string is less specific than ethernet-phy-XXXX.YYYY. I will reword the changelog to make it clear that "ethernet-phy-XXXX.YYYY" is allowed. Thanks Andrew
On Wed, Jan 27, 2016 at 9:11 AM, Andrew Lunn <andrew@lunn.ch> wrote: > On Wed, Jan 27, 2016 at 08:41:56AM -0800, Olof Johansson wrote: >> On Tue, Jan 26, 2016 at 4:11 PM, Andrew Lunn <andrew@lunn.ch> wrote: >> > PHY devices may only list clause 22, 45, and their PHY identifier >> > values as compatible values. No other compatible strings are allowed. >> > Make this clear in the documentation, and remove examples where >> > make/model compatible strings are listed. >> > >> > Signed-off-by: Andrew Lunn <andrew@lunn.ch> >> >> I'm not sure I agree with the disallowing here. It's common practice >> to use a specific compatible to describe the actual hardware used, in >> case it's needed in the future for some driver to distinguish >> behavior, etc. >> >> So while it should be required to include the clause compats, having a >> more specific one in there should be acceptable. > > Hi Olof > > Matching PHY devices to drivers has never used to compatible string, > other than the "ethernet-phy-XXXX.YYYY" string. The PHY has two > registers containing a manufacture id, device id and revision, > registers 2 and 3. These are the XXXX and YYYY. The core code reads > these values, or uses the values from the ethernet-phy-XXXX.YYYY, and > uses them to find a driver which supports these values. > > A make/model string is less specific than ethernet-phy-XXXX.YYYY. > > I will reword the changelog to make it clear that > "ethernet-phy-XXXX.YYYY" is allowed. Only case I can see the need for a make/model string is if there's a need to add model-specific properties since you'd need a compatible then (or make those properties shared between all phy bindings). Anyway, we've never had a huge issue with this on other probable busses, so we should be fine with the above. With the clarification I'm OK with this change. -Olof
> Only case I can see the need for a make/model string is if there's a > need to add model-specific properties since you'd need a compatible > then (or make those properties shared between all phy bindings). Documentation/devicetree/bindings/net/micrel-ksz90x1.txt It adds optional properties to the phy node. > Anyway, we've never had a huge issue with this on other probable > busses, so we should be fine with the above. With the clarification > I'm OK with this change. Great. Thanks Andrew
diff --git a/Documentation/devicetree/bindings/net/brcm,bcmgenet.txt b/Documentation/devicetree/bindings/net/brcm,bcmgenet.txt index 451fef26b4df..10587bdadbbe 100644 --- a/Documentation/devicetree/bindings/net/brcm,bcmgenet.txt +++ b/Documentation/devicetree/bindings/net/brcm,bcmgenet.txt @@ -68,7 +68,7 @@ ethernet@f0b60000 { phy1: ethernet-phy@1 { max-speed = <1000>; reg = <0x1>; - compatible = "brcm,28nm-gphy", "ethernet-phy-ieee802.3-c22"; + compatible = "ethernet-phy-ieee802.3-c22"; }; }; }; @@ -115,7 +115,7 @@ ethernet@f0ba0000 { phy0: ethernet-phy@0 { max-speed = <1000>; reg = <0x0>; - compatible = "brcm,bcm53125", "ethernet-phy-ieee802.3-c22"; + compatible = "ethernet-phy-ieee802.3-c22"; }; }; }; diff --git a/Documentation/devicetree/bindings/net/mdio-mux-gpio.txt b/Documentation/devicetree/bindings/net/mdio-mux-gpio.txt index 79384113c2b0..694987d3c17a 100644 --- a/Documentation/devicetree/bindings/net/mdio-mux-gpio.txt +++ b/Documentation/devicetree/bindings/net/mdio-mux-gpio.txt @@ -38,7 +38,6 @@ Example : phy11: ethernet-phy@1 { reg = <1>; - compatible = "marvell,88e1149r"; marvell,reg-init = <3 0x10 0 0x5777>, <3 0x11 0 0x00aa>, <3 0x12 0 0x4105>, @@ -48,7 +47,6 @@ Example : }; phy12: ethernet-phy@2 { reg = <2>; - compatible = "marvell,88e1149r"; marvell,reg-init = <3 0x10 0 0x5777>, <3 0x11 0 0x00aa>, <3 0x12 0 0x4105>, @@ -58,7 +56,6 @@ Example : }; phy13: ethernet-phy@3 { reg = <3>; - compatible = "marvell,88e1149r"; marvell,reg-init = <3 0x10 0 0x5777>, <3 0x11 0 0x00aa>, <3 0x12 0 0x4105>, @@ -68,7 +65,6 @@ Example : }; phy14: ethernet-phy@4 { reg = <4>; - compatible = "marvell,88e1149r"; marvell,reg-init = <3 0x10 0 0x5777>, <3 0x11 0 0x00aa>, <3 0x12 0 0x4105>, @@ -85,7 +81,6 @@ Example : phy21: ethernet-phy@1 { reg = <1>; - compatible = "marvell,88e1149r"; marvell,reg-init = <3 0x10 0 0x5777>, <3 0x11 0 0x00aa>, <3 0x12 0 0x4105>, @@ -95,7 +90,6 @@ Example : }; phy22: ethernet-phy@2 { reg = <2>; - compatible = "marvell,88e1149r"; marvell,reg-init = <3 0x10 0 0x5777>, <3 0x11 0 0x00aa>, <3 0x12 0 0x4105>, @@ -105,7 +99,6 @@ Example : }; phy23: ethernet-phy@3 { reg = <3>; - compatible = "marvell,88e1149r"; marvell,reg-init = <3 0x10 0 0x5777>, <3 0x11 0 0x00aa>, <3 0x12 0 0x4105>, @@ -115,7 +108,6 @@ Example : }; phy24: ethernet-phy@4 { reg = <4>; - compatible = "marvell,88e1149r"; marvell,reg-init = <3 0x10 0 0x5777>, <3 0x11 0 0x00aa>, <3 0x12 0 0x4105>, diff --git a/Documentation/devicetree/bindings/net/mdio-mux.txt b/Documentation/devicetree/bindings/net/mdio-mux.txt index f65606f8d632..491f5bd55203 100644 --- a/Documentation/devicetree/bindings/net/mdio-mux.txt +++ b/Documentation/devicetree/bindings/net/mdio-mux.txt @@ -47,7 +47,6 @@ Example : phy11: ethernet-phy@1 { reg = <1>; - compatible = "marvell,88e1149r"; marvell,reg-init = <3 0x10 0 0x5777>, <3 0x11 0 0x00aa>, <3 0x12 0 0x4105>, @@ -57,7 +56,6 @@ Example : }; phy12: ethernet-phy@2 { reg = <2>; - compatible = "marvell,88e1149r"; marvell,reg-init = <3 0x10 0 0x5777>, <3 0x11 0 0x00aa>, <3 0x12 0 0x4105>, @@ -67,7 +65,6 @@ Example : }; phy13: ethernet-phy@3 { reg = <3>; - compatible = "marvell,88e1149r"; marvell,reg-init = <3 0x10 0 0x5777>, <3 0x11 0 0x00aa>, <3 0x12 0 0x4105>, @@ -77,7 +74,6 @@ Example : }; phy14: ethernet-phy@4 { reg = <4>; - compatible = "marvell,88e1149r"; marvell,reg-init = <3 0x10 0 0x5777>, <3 0x11 0 0x00aa>, <3 0x12 0 0x4105>, @@ -94,7 +90,6 @@ Example : phy21: ethernet-phy@1 { reg = <1>; - compatible = "marvell,88e1149r"; marvell,reg-init = <3 0x10 0 0x5777>, <3 0x11 0 0x00aa>, <3 0x12 0 0x4105>, @@ -104,7 +99,6 @@ Example : }; phy22: ethernet-phy@2 { reg = <2>; - compatible = "marvell,88e1149r"; marvell,reg-init = <3 0x10 0 0x5777>, <3 0x11 0 0x00aa>, <3 0x12 0 0x4105>, @@ -114,7 +108,6 @@ Example : }; phy23: ethernet-phy@3 { reg = <3>; - compatible = "marvell,88e1149r"; marvell,reg-init = <3 0x10 0 0x5777>, <3 0x11 0 0x00aa>, <3 0x12 0 0x4105>, @@ -124,7 +117,6 @@ Example : }; phy24: ethernet-phy@4 { reg = <4>; - compatible = "marvell,88e1149r"; marvell,reg-init = <3 0x10 0 0x5777>, <3 0x11 0 0x00aa>, <3 0x12 0 0x4105>, diff --git a/Documentation/devicetree/bindings/net/phy.txt b/Documentation/devicetree/bindings/net/phy.txt index 525e1658f2da..bc1c3c8bf8fa 100644 --- a/Documentation/devicetree/bindings/net/phy.txt +++ b/Documentation/devicetree/bindings/net/phy.txt @@ -17,8 +17,7 @@ Optional Properties: "ethernet-phy-ieee802.3-c22" or "ethernet-phy-ieee802.3-c45" for PHYs that implement IEEE802.3 clause 22 or IEEE802.3 clause 45 specifications. If neither of these are specified, the default is to - assume clause 22. The compatible list may also contain other - elements. + assume clause 22. If the phy's identifier is known then the list may contain an entry of the form: "ethernet-phy-idAAAA.BBBB" where @@ -28,6 +27,9 @@ Optional Properties: 4 hex digits. This is the chip vendor OUI bits 19:24, followed by 10 bits of a vendor specific ID. + The compatible list should not contain other values than those + listed here. + - max-speed: Maximum PHY supported speed (10, 100, 1000...) - broken-turn-around: If set, indicates the PHY device does not correctly
PHY devices may only list clause 22, 45, and their PHY identifier values as compatible values. No other compatible strings are allowed. Make this clear in the documentation, and remove examples where make/model compatible strings are listed. Signed-off-by: Andrew Lunn <andrew@lunn.ch> --- Documentation/devicetree/bindings/net/brcm,bcmgenet.txt | 4 ++-- Documentation/devicetree/bindings/net/mdio-mux-gpio.txt | 8 -------- Documentation/devicetree/bindings/net/mdio-mux.txt | 8 -------- Documentation/devicetree/bindings/net/phy.txt | 6 ++++-- 4 files changed, 6 insertions(+), 20 deletions(-)