[PATCH net 0/2] Part 2 of v4.5-rc1 phylib regression

[PATCH net 0/2] Part 2 of v4.5-rc1 phylib regression

[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

[PATCH net 1/2] of: of_mdio: Add a whitelist of PHY compatibilities.

[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

[PATCH net 2/2] DT: phy.txt: Clarify expected compatible values

[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

Re: [PATCH net 2/2] DT: phy.txt: Clarify expected compatible values

[Florian Fainelli]

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

Re: [PATCH net 2/2] DT: phy.txt: Clarify expected compatible values

[Andrew Lunn]

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

Re: [PATCH net 2/2] DT: phy.txt: Clarify expected compatible values

[Florian Fainelli]

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

Re: [PATCH net 2/2] DT: phy.txt: Clarify expected compatible values

[Andrew Lunn]

> 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

Re: [PATCH net 1/2] of: of_mdio: Add a whitelist of PHY compatibilities.

[Aaro Koskinen]

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
> 

Re: [PATCH net 1/2] of: of_mdio: Add a whitelist of PHY compatibilities.

[Sergei Shtylyov]

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

Re: [PATCH net 1/2] of: of_mdio: Add a whitelist of PHY compatibilities.

[Andrew Lunn]

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

Re: [PATCH net 2/2] DT: phy.txt: Clarify expected compatible values

[Florian Fainelli]

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

Re: [PATCH net 2/2] DT: phy.txt: Clarify expected compatible values

[Olof Johansson]

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

Re: [PATCH net 2/2] DT: phy.txt: Clarify expected compatible values

[Andrew Lunn]

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

Re: [PATCH net 2/2] DT: phy.txt: Clarify expected compatible values

[Olof Johansson]

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

Re: [PATCH net 2/2] DT: phy.txt: Clarify expected compatible values

[Andrew Lunn]

> 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