* [PATCH net 0/2] Part 2 of v4.5-rc1 phylib regression @ 2016-01-27 0:11 Andrew Lunn 2016-01-27 0:11 ` [PATCH net 1/2] of: of_mdio: Add a whitelist of PHY compatibilities Andrew Lunn 2016-01-27 0:11 ` [PATCH net 2/2] DT: phy.txt: Clarify expected compatible values Andrew Lunn 0 siblings, 2 replies; 15+ messages in thread From: Andrew Lunn @ 2016-01-27 0:11 UTC (permalink / raw) To: David Miller; +Cc: netdev, Florian Fainelli, aaro.koskinen, olof, Andrew Lunn White list PHY compatible values which indicate PHYs. Issue a warning when one is encountered. Update the documentation to make it clear what is expected in the compatible string. Andrew Lunn (2): of: of_mdio: Add a whitelist of PHY compatibilities. DT: phy.txt: Clarify expected compatible values .../devicetree/bindings/net/brcm,bcmgenet.txt | 4 ++-- .../devicetree/bindings/net/mdio-mux-gpio.txt | 8 ------- Documentation/devicetree/bindings/net/mdio-mux.txt | 8 ------- Documentation/devicetree/bindings/net/phy.txt | 6 +++-- drivers/of/of_mdio.c | 27 ++++++++++++++++++++++ 5 files changed, 33 insertions(+), 20 deletions(-) -- 2.7.0 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH net 1/2] of: of_mdio: Add a whitelist of PHY compatibilities. 2016-01-27 0:11 [PATCH net 0/2] Part 2 of v4.5-rc1 phylib regression Andrew Lunn @ 2016-01-27 0:11 ` Andrew Lunn 2016-01-27 11:17 ` Aaro Koskinen 2016-01-27 13:51 ` Sergei Shtylyov 2016-01-27 0:11 ` [PATCH net 2/2] DT: phy.txt: Clarify expected compatible values Andrew Lunn 1 sibling, 2 replies; 15+ messages in thread From: Andrew Lunn @ 2016-01-27 0:11 UTC (permalink / raw) To: David Miller; +Cc: netdev, Florian Fainelli, aaro.koskinen, olof, Andrew Lunn Some phy nodes list a compatible value indicating the PHY make/model. This is never used to match the device to the driver. However it does confuse the code to separate a PHY from a generic MDIO device like a switch. Generic MDIO devices must have a compatible value, PHYs can list clause 22 or 45, but nothing else. Issue a warning if we find a compatible value known on the whitelist, and say it is a PHY. Fixes: a9049e0c513c ("mdio: Add support for mdio drivers.") Reported-by: Aaro Koskinen <aaro.koskinen@nokia.com> Reported-by: Olof Johansson <olof@lixom.net> Signed-off-by: Andrew Lunn <andrew@lunn.ch> --- drivers/of/of_mdio.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c index b5aa004a24b6..26c245041493 100644 --- a/drivers/of/of_mdio.c +++ b/drivers/of/of_mdio.c @@ -143,11 +143,31 @@ int of_mdio_parse_addr(struct device *dev, const struct device_node *np) } EXPORT_SYMBOL(of_mdio_parse_addr); +/* The following is a list of PHY compatible strings which appear in + * some DTBs. The compatible string is never matched against a PHY + * driver, so is pointless. We only expect devices which are not PHYs + * to have a compatible string, so they can be matched to an MDIO + * driver. Encourage users to upgrade there DT blobs to remove these. + */ +static const struct of_device_id whitelist_phys[] = { + { .compatible = "brcm,40nm-ephy" }, + { .compatible = "marvell,88E1111", }, + { .compatible = "marvell,88e1116", }, + { .compatible = "marvell,88e1118", }, + { .compatible = "marvell,88e1149r", }, + { .compatible = "marvell,88e1310", }, + { .compatible = "marvell,88E1510", }, + { .compatible = "marvell,88E1514", }, + { .compatible = "moxa,moxart-rtl8201cp", }, + {} +}; + /* * Return true if the child node is for a phy. It must either: * o Compatible string of "ethernet-phy-idX.X" * o Compatible string of "ethernet-phy-ieee802.3-c45" * o Compatible string of "ethernet-phy-ieee802.3-c22" + * o In the white list above (and issue a warning) * o No compatibility string * * A device which is not a phy is expected to have a compatible string @@ -166,6 +186,13 @@ static bool of_mdiobus_child_is_phy(struct device_node *child) if (of_device_is_compatible(child, "ethernet-phy-ieee802.3-c22")) return true; + if (of_match_node(whitelist_phys, child)) { + pr_warn(FW_WARN + "%s: Whitelisted compatible string. Please remove\n", + child->full_name); + return true; + } + if (!of_find_property(child, "compatible", NULL)) return true; -- 2.7.0 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH net 1/2] of: of_mdio: Add a whitelist of PHY compatibilities. 2016-01-27 0:11 ` [PATCH net 1/2] of: of_mdio: Add a whitelist of PHY compatibilities Andrew Lunn @ 2016-01-27 11:17 ` Aaro Koskinen 2016-01-27 13:51 ` Sergei Shtylyov 1 sibling, 0 replies; 15+ messages in thread From: Aaro Koskinen @ 2016-01-27 11:17 UTC (permalink / raw) To: Andrew Lunn; +Cc: David Miller, netdev, Florian Fainelli, olof Hi, On Wed, Jan 27, 2016 at 01:11:38AM +0100, Andrew Lunn wrote: > Some phy nodes list a compatible value indicating the PHY make/model. > This is never used to match the device to the driver. However it does > confuse the code to separate a PHY from a generic MDIO device like a > switch. Generic MDIO devices must have a compatible value, PHYs can > list clause 22 or 45, but nothing else. > > Issue a warning if we find a compatible value known on the whitelist, > and say it is a PHY. > > Fixes: a9049e0c513c ("mdio: Add support for mdio drivers.") > Reported-by: Aaro Koskinen <aaro.koskinen@nokia.com> > Reported-by: Olof Johansson <olof@lixom.net> > Signed-off-by: Andrew Lunn <andrew@lunn.ch> Tested-by: Aaro Koskinen <aaro.koskinen@nokia.com> A. > --- > drivers/of/of_mdio.c | 27 +++++++++++++++++++++++++++ > 1 file changed, 27 insertions(+) > > diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c > index b5aa004a24b6..26c245041493 100644 > --- a/drivers/of/of_mdio.c > +++ b/drivers/of/of_mdio.c > @@ -143,11 +143,31 @@ int of_mdio_parse_addr(struct device *dev, const struct device_node *np) > } > EXPORT_SYMBOL(of_mdio_parse_addr); > > +/* The following is a list of PHY compatible strings which appear in > + * some DTBs. The compatible string is never matched against a PHY > + * driver, so is pointless. We only expect devices which are not PHYs > + * to have a compatible string, so they can be matched to an MDIO > + * driver. Encourage users to upgrade there DT blobs to remove these. > + */ > +static const struct of_device_id whitelist_phys[] = { > + { .compatible = "brcm,40nm-ephy" }, > + { .compatible = "marvell,88E1111", }, > + { .compatible = "marvell,88e1116", }, > + { .compatible = "marvell,88e1118", }, > + { .compatible = "marvell,88e1149r", }, > + { .compatible = "marvell,88e1310", }, > + { .compatible = "marvell,88E1510", }, > + { .compatible = "marvell,88E1514", }, > + { .compatible = "moxa,moxart-rtl8201cp", }, > + {} > +}; > + > /* > * Return true if the child node is for a phy. It must either: > * o Compatible string of "ethernet-phy-idX.X" > * o Compatible string of "ethernet-phy-ieee802.3-c45" > * o Compatible string of "ethernet-phy-ieee802.3-c22" > + * o In the white list above (and issue a warning) > * o No compatibility string > * > * A device which is not a phy is expected to have a compatible string > @@ -166,6 +186,13 @@ static bool of_mdiobus_child_is_phy(struct device_node *child) > if (of_device_is_compatible(child, "ethernet-phy-ieee802.3-c22")) > return true; > > + if (of_match_node(whitelist_phys, child)) { > + pr_warn(FW_WARN > + "%s: Whitelisted compatible string. Please remove\n", > + child->full_name); > + return true; > + } > + > if (!of_find_property(child, "compatible", NULL)) > return true; > > -- > 2.7.0 > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH net 1/2] of: of_mdio: Add a whitelist of PHY compatibilities. 2016-01-27 0:11 ` [PATCH net 1/2] of: of_mdio: Add a whitelist of PHY compatibilities Andrew Lunn 2016-01-27 11:17 ` Aaro Koskinen @ 2016-01-27 13:51 ` Sergei Shtylyov 2016-01-27 14:03 ` Andrew Lunn 1 sibling, 1 reply; 15+ messages in thread From: Sergei Shtylyov @ 2016-01-27 13:51 UTC (permalink / raw) To: Andrew Lunn, David Miller; +Cc: netdev, Florian Fainelli, aaro.koskinen, olof Hello. On 01/27/2016 03:11 AM, Andrew Lunn wrote: > Some phy nodes list a compatible value indicating the PHY make/model. > This is never used to match the device to the driver. However it does > confuse the code to separate a PHY from a generic MDIO device like a > switch. Generic MDIO devices must have a compatible value, PHYs can > list clause 22 or 45, but nothing else. > > Issue a warning if we find a compatible value known on the whitelist, My spell-checker trips on "whitelist"... Perhaps a space/hyphen needed? > and say it is a PHY. > > Fixes: a9049e0c513c ("mdio: Add support for mdio drivers.") > Reported-by: Aaro Koskinen <aaro.koskinen@nokia.com> > Reported-by: Olof Johansson <olof@lixom.net> > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > --- > drivers/of/of_mdio.c | 27 +++++++++++++++++++++++++++ > 1 file changed, 27 insertions(+) > > diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c > index b5aa004a24b6..26c245041493 100644 > --- a/drivers/of/of_mdio.c > +++ b/drivers/of/of_mdio.c > @@ -143,11 +143,31 @@ int of_mdio_parse_addr(struct device *dev, const struct device_node *np) > } > EXPORT_SYMBOL(of_mdio_parse_addr); > > +/* The following is a list of PHY compatible strings which appear in > + * some DTBs. The compatible string is never matched against a PHY > + * driver, so is pointless. We only expect devices which are not PHYs > + * to have a compatible string, so they can be matched to an MDIO > + * driver. Encourage users to upgrade there DT blobs to remove these. s/there/their/. [...] > @@ -166,6 +186,13 @@ static bool of_mdiobus_child_is_phy(struct device_node *child) > if (of_device_is_compatible(child, "ethernet-phy-ieee802.3-c22")) > return true; > > + if (of_match_node(whitelist_phys, child)) { > + pr_warn(FW_WARN > + "%s: Whitelisted compatible string. Please remove\n", White-listed? [...] MBR, Sergei ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH net 1/2] of: of_mdio: Add a whitelist of PHY compatibilities. 2016-01-27 13:51 ` Sergei Shtylyov @ 2016-01-27 14:03 ` Andrew Lunn 0 siblings, 0 replies; 15+ messages in thread From: Andrew Lunn @ 2016-01-27 14:03 UTC (permalink / raw) To: Sergei Shtylyov Cc: David Miller, netdev, Florian Fainelli, aaro.koskinen, olof On Wed, Jan 27, 2016 at 04:51:38PM +0300, Sergei Shtylyov wrote: > Hello. > > On 01/27/2016 03:11 AM, Andrew Lunn wrote: > > >Some phy nodes list a compatible value indicating the PHY make/model. > >This is never used to match the device to the driver. However it does > >confuse the code to separate a PHY from a generic MDIO device like a > >switch. Generic MDIO devices must have a compatible value, PHYs can > >list clause 22 or 45, but nothing else. > > > >Issue a warning if we find a compatible value known on the whitelist, > > My spell-checker trips on "whitelist"... Perhaps a space/hyphen needed? $ dict whitelist 1 definition found >From The Jargon File (version 4.4.7, 29 Dec 2003) [jargon]: whitelist n. The opposite of a blacklist. That is, instead of being an explicit list of people who are banned, it's an explicit list of people who are to be admitted. Hackers use this especially of lists of email addresses that are explicitly enabled to get past strict anti-spam filters. > >+/* The following is a list of PHY compatible strings which appear in > >+ * some DTBs. The compatible string is never matched against a PHY > >+ * driver, so is pointless. We only expect devices which are not PHYs > >+ * to have a compatible string, so they can be matched to an MDIO > >+ * driver. Encourage users to upgrade there DT blobs to remove these. > > s/there/their/. I will fix this. Andrew ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH net 2/2] DT: phy.txt: Clarify expected compatible values 2016-01-27 0:11 [PATCH net 0/2] Part 2 of v4.5-rc1 phylib regression Andrew Lunn 2016-01-27 0:11 ` [PATCH net 1/2] of: of_mdio: Add a whitelist of PHY compatibilities Andrew Lunn @ 2016-01-27 0:11 ` Andrew Lunn 2016-01-27 0:33 ` Florian Fainelli 2016-01-27 16:41 ` Olof Johansson 1 sibling, 2 replies; 15+ messages in thread From: Andrew Lunn @ 2016-01-27 0:11 UTC (permalink / raw) To: David Miller; +Cc: netdev, Florian Fainelli, aaro.koskinen, olof, Andrew Lunn 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(-) 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 -- 2.7.0 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH net 2/2] DT: phy.txt: Clarify expected compatible values 2016-01-27 0:11 ` [PATCH net 2/2] DT: phy.txt: Clarify expected compatible values Andrew Lunn @ 2016-01-27 0:33 ` Florian Fainelli 2016-01-27 1:06 ` Andrew Lunn 2016-01-27 16:41 ` Olof Johansson 1 sibling, 1 reply; 15+ messages in thread From: Florian Fainelli @ 2016-01-27 0:33 UTC (permalink / raw) To: Andrew Lunn, David Miller; +Cc: netdev, aaro.koskinen, olof 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? -- Florian ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH net 2/2] DT: phy.txt: Clarify expected compatible values 2016-01-27 0:33 ` Florian Fainelli @ 2016-01-27 1:06 ` Andrew Lunn 2016-01-27 1:25 ` Florian Fainelli 0 siblings, 1 reply; 15+ messages in thread From: Andrew Lunn @ 2016-01-27 1:06 UTC (permalink / raw) To: Florian Fainelli; +Cc: David Miller, netdev, aaro.koskinen, olof 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH net 2/2] DT: phy.txt: Clarify expected compatible values 2016-01-27 1:06 ` Andrew Lunn @ 2016-01-27 1:25 ` Florian Fainelli 2016-01-27 1:57 ` Andrew Lunn 0 siblings, 1 reply; 15+ messages in thread From: Florian Fainelli @ 2016-01-27 1:25 UTC (permalink / raw) To: Andrew Lunn; +Cc: David Miller, netdev, aaro.koskinen, olof 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? -- Florian ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH net 2/2] DT: phy.txt: Clarify expected compatible values 2016-01-27 1:25 ` Florian Fainelli @ 2016-01-27 1:57 ` Andrew Lunn 2016-01-27 16:31 ` Florian Fainelli 0 siblings, 1 reply; 15+ messages in thread From: Andrew Lunn @ 2016-01-27 1:57 UTC (permalink / raw) To: Florian Fainelli; +Cc: David Miller, netdev, aaro.koskinen, olof > 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH net 2/2] DT: phy.txt: Clarify expected compatible values 2016-01-27 1:57 ` Andrew Lunn @ 2016-01-27 16:31 ` Florian Fainelli 0 siblings, 0 replies; 15+ messages in thread From: Florian Fainelli @ 2016-01-27 16:31 UTC (permalink / raw) To: Andrew Lunn; +Cc: David Miller, netdev, aaro.koskinen, olof 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> -- Florian ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH net 2/2] DT: phy.txt: Clarify expected compatible values 2016-01-27 0:11 ` [PATCH net 2/2] DT: phy.txt: Clarify expected compatible values Andrew Lunn 2016-01-27 0:33 ` Florian Fainelli @ 2016-01-27 16:41 ` Olof Johansson 2016-01-27 17:11 ` Andrew Lunn 1 sibling, 1 reply; 15+ messages in thread From: Olof Johansson @ 2016-01-27 16:41 UTC (permalink / raw) To: Andrew Lunn; +Cc: David Miller, netdev, Florian Fainelli, Aaro Koskinen 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH net 2/2] DT: phy.txt: Clarify expected compatible values 2016-01-27 16:41 ` Olof Johansson @ 2016-01-27 17:11 ` Andrew Lunn 2016-01-27 17:32 ` Olof Johansson 0 siblings, 1 reply; 15+ messages in thread From: Andrew Lunn @ 2016-01-27 17:11 UTC (permalink / raw) To: Olof Johansson; +Cc: David Miller, netdev, Florian Fainelli, Aaro Koskinen 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH net 2/2] DT: phy.txt: Clarify expected compatible values 2016-01-27 17:11 ` Andrew Lunn @ 2016-01-27 17:32 ` Olof Johansson 2016-01-27 17:36 ` Andrew Lunn 0 siblings, 1 reply; 15+ messages in thread From: Olof Johansson @ 2016-01-27 17:32 UTC (permalink / raw) To: Andrew Lunn; +Cc: David Miller, netdev, Florian Fainelli, Aaro Koskinen 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH net 2/2] DT: phy.txt: Clarify expected compatible values 2016-01-27 17:32 ` Olof Johansson @ 2016-01-27 17:36 ` Andrew Lunn 0 siblings, 0 replies; 15+ messages in thread From: Andrew Lunn @ 2016-01-27 17:36 UTC (permalink / raw) To: Olof Johansson; +Cc: David Miller, netdev, Florian Fainelli, Aaro Koskinen > 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2016-01-27 17:36 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-01-27 0:11 [PATCH net 0/2] Part 2 of v4.5-rc1 phylib regression Andrew Lunn 2016-01-27 0:11 ` [PATCH net 1/2] of: of_mdio: Add a whitelist of PHY compatibilities Andrew Lunn 2016-01-27 11:17 ` Aaro Koskinen 2016-01-27 13:51 ` Sergei Shtylyov 2016-01-27 14:03 ` Andrew Lunn 2016-01-27 0:11 ` [PATCH net 2/2] DT: phy.txt: Clarify expected compatible values Andrew Lunn 2016-01-27 0:33 ` Florian Fainelli 2016-01-27 1:06 ` Andrew Lunn 2016-01-27 1:25 ` Florian Fainelli 2016-01-27 1:57 ` Andrew Lunn 2016-01-27 16:31 ` Florian Fainelli 2016-01-27 16:41 ` Olof Johansson 2016-01-27 17:11 ` Andrew Lunn 2016-01-27 17:32 ` Olof Johansson 2016-01-27 17:36 ` Andrew Lunn
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.