[PATCH] net/dt: Add support for overriding phy configuration from device tree

[PATCH] net/dt: Add support for overriding phy configuration from device tree

[Matthew Garrett]

Some hardware may be broken in interesting and board-specific ways, such
that various bits of functionality don't work. This patch provides a
mechanism for overriding mii registers during init based on the contents of
the device tree data, allowing board-specific fixups without having to
pollute generic code.

Signed-off-by: Matthew Garrett <matthew.garrett-05XSO3Yj/JvQT0dZR+AlfA@public.gmane.org>
---
 Documentation/devicetree/bindings/net/phy.txt |  13 +++
 drivers/net/phy/phy_device.c                  |  29 +++++-
 drivers/of/of_net.c                           | 124 ++++++++++++++++++++++++++
 include/linux/of_net.h                        |  12 +++
 4 files changed, 177 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/net/phy.txt b/Documentation/devicetree/bindings/net/phy.txt
index 7cd18fb..552a5e0 100644
--- a/Documentation/devicetree/bindings/net/phy.txt
+++ b/Documentation/devicetree/bindings/net/phy.txt
@@ -23,6 +23,19 @@ Optional Properties:
   assume clause 22. The compatible list may also contain other
   elements.
 
+The following properties may be added to either the phy node or the parent
+ethernet device:
+
+- phy-mii-advertise-10half: Whether to advertise half-duplex 10MBit
+- phy-mii-advertise-10full: Whether to advertise full-duplex 10MBit
+- phy-mii-advertise-100half: Whether to advertise half-duplex 100MBit
+- phy-mii-advertise-100full: Whether to advertise full-duplex 100MBit
+- phy-mii-advertise-100base4: Whether to advertise 100base4
+- phy-mii-advertise-1000half: Whether to advertise half-duplex 1000MBit
+- phy-mii-advertise-1000full: Whether to advertise full-duplex 1000MBit
+- phy-mii-as-master: Configure phy to act as master/slave
+- phy-mii-manual-master: Enable/disable manual master/slave configuration
+
 Example:
 
 ethernet-phy@0 {
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index d6447b3..91793bc 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -33,6 +33,7 @@
 #include <linux/mii.h>
 #include <linux/ethtool.h>
 #include <linux/phy.h>
+#include <linux/of_net.h>
 
 #include <asm/io.h>
 #include <asm/irq.h>
@@ -497,6 +498,28 @@ void phy_disconnect(struct phy_device *phydev)
 }
 EXPORT_SYMBOL(phy_disconnect);
 
+int phy_override_from_of(struct phy_device *phydev)
+{
+	int reg, regval;
+	u16 val, mask;
+
+	/* Check for phy register overrides from OF */
+	for (reg = 0; reg < 16; reg++) {
+		if (!of_get_mii_register(phydev, reg, &val, &mask)) {
+			if (!mask)
+				continue;
+			regval = phy_read(phydev, reg);
+			if (regval < 0)
+				continue;
+			regval &= ~mask;
+			regval |= val;
+			phy_write(phydev, reg, regval);
+		}
+	}
+
+	return 0;
+}
+
 int phy_init_hw(struct phy_device *phydev)
 {
 	int ret;
@@ -508,7 +531,11 @@ int phy_init_hw(struct phy_device *phydev)
 	if (ret < 0)
 		return ret;
 
-	return phydev->drv->config_init(phydev);
+	ret = phydev->drv->config_init(phydev);
+	if (ret < 0)
+		return ret;
+
+	return phy_override_from_of(phydev);
 }
 
 /**
diff --git a/drivers/of/of_net.c b/drivers/of/of_net.c
index 8f9be2e..4545608 100644
--- a/drivers/of/of_net.c
+++ b/drivers/of/of_net.c
@@ -93,3 +93,127 @@ const void *of_get_mac_address(struct device_node *np)
 	return NULL;
 }
 EXPORT_SYMBOL(of_get_mac_address);
+
+/**
+ * Provide phy register overrides from the device tree. Some hardware may
+ * be broken in interesting and board-specific ways, so we want a mechanism
+ * for the board data to provide overrides for default values. This should be
+ * called during phy init.
+ */
+int of_get_mii_register(struct phy_device *phydev, int reg, u16 *val,
+			u16 *mask)
+{
+	u32 tmp;
+	struct device *dev = &phydev->dev;
+	struct device_node *np = dev->of_node;
+
+	*val = 0;
+	*mask = 0;
+
+	if (!np && dev->parent->of_node)
+		np = dev->parent->of_node;
+
+	if (!np)
+		return 0;
+
+	switch (reg) {
+	case MII_ADVERTISE:
+		if (!of_property_read_u32(np, "phy-mii-advertise-10half",
+					   &tmp)) {
+			if (tmp) {
+				*val |= ADVERTISE_10HALF;
+				phydev->advertising |= SUPPORTED_10baseT_Half;
+			} else {
+				phydev->advertising &=
+					~(SUPPORTED_10baseT_Half);
+			}
+
+			*mask |= ADVERTISE_10HALF;
+		}
+		if (!of_property_read_u32(np, "phy-mii-advertise-10full",
+					   &tmp)) {
+			if (tmp) {
+				*val |= ADVERTISE_10FULL;
+				phydev->advertising |= SUPPORTED_10baseT_Full;
+			} else {
+				phydev->advertising &=
+					~(SUPPORTED_10baseT_Full);
+			}
+
+			*mask |= ADVERTISE_10FULL;
+		}
+		if (!of_property_read_u32(np, "phy-mii-advertise-100half",
+					   &tmp)) {
+			if (tmp) {
+				*val |= ADVERTISE_100HALF;
+				phydev->advertising |= SUPPORTED_100baseT_Half;
+			} else {
+				phydev->advertising &=
+					~(SUPPORTED_100baseT_Half);
+			}
+
+			*mask |= ADVERTISE_100HALF;
+		}
+		if (!of_property_read_u32(np, "phy-mii-advertise-100full",
+					   &tmp)) {
+			if (tmp) {
+				*val |= ADVERTISE_100FULL;
+				phydev->advertising |= SUPPORTED_100baseT_Full;
+			} else {
+				phydev->advertising &=
+					~(SUPPORTED_100baseT_Full);
+			}
+
+			*mask |= ADVERTISE_100FULL;
+		}
+		if (!of_property_read_u32(np, "phy-mii-advertise-100base4",
+					   &tmp)) {
+			if (tmp)
+				*val |= ADVERTISE_100BASE4;
+			*mask |= ADVERTISE_100BASE4;
+		}
+		break;
+	case MII_CTRL1000:
+		if (!of_property_read_u32(np, "phy-mii-advertise-1000full",
+					   &tmp)) {
+			if (tmp) {
+				*val |= ADVERTISE_1000FULL;
+				phydev->advertising |= SUPPORTED_1000baseT_Full;
+			} else {
+				phydev->advertising &=
+					~(SUPPORTED_1000baseT_Full);
+			}
+
+			*mask |= ADVERTISE_1000FULL;
+		}
+		if (!of_property_read_u32(np, "phy-mii-advertise-1000half",
+					   &tmp)) {
+			if (tmp) {
+				*val |= ADVERTISE_1000HALF;
+				phydev->advertising |= SUPPORTED_1000baseT_Half;
+			} else {
+				phydev->advertising &=
+					~(SUPPORTED_1000baseT_Half);
+			}
+
+			*mask |= ADVERTISE_1000HALF;
+		}
+		if (!of_property_read_u32(np, "phy-mii-as-master",
+					   &tmp)) {
+			if (tmp)
+				*val |= CTL1000_AS_MASTER;
+			*mask |= CTL1000_AS_MASTER;
+		}
+		if (!of_property_read_u32(np, "phy-mii-manual-master",
+					   &tmp)) {
+			if (tmp)
+				*val |= CTL1000_ENABLE_MASTER;
+			*mask |= CTL1000_ENABLE_MASTER;
+		}
+		break;
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
+EXPORT_SYMBOL(of_get_mii_register);
diff --git a/include/linux/of_net.h b/include/linux/of_net.h
index 34597c8..2e478bc 100644
--- a/include/linux/of_net.h
+++ b/include/linux/of_net.h
@@ -7,10 +7,14 @@
 #ifndef __LINUX_OF_NET_H
 #define __LINUX_OF_NET_H
 
+#include <linux/phy.h>
+
 #ifdef CONFIG_OF_NET
 #include <linux/of.h>
 extern int of_get_phy_mode(struct device_node *np);
 extern const void *of_get_mac_address(struct device_node *np);
+extern int of_get_mii_register(struct phy_device *np, int reg, u16 *val,
+			       u16 *mask);
 #else
 static inline int of_get_phy_mode(struct device_node *np)
 {
@@ -21,6 +25,14 @@ static inline const void *of_get_mac_address(struct device_node *np)
 {
 	return NULL;
 }
+static inline int of_get_mii_register(struct phy_device *np, int reg, u16 *val,
+				      u16 *mask)
+{
+	*val = 0;
+	*mask = 0;
+
+	return -EINVAL;
+}
 #endif
 
 #endif /* __LINUX_OF_NET_H */
-- 
1.8.4.2

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] net/dt: Add support for overriding phy configuration from device tree

[Gerhard Sittig]

On Wed, Jan 15, 2014 at 16:38 -0500, Matthew Garrett wrote:
> 
> Some hardware may be broken in interesting and board-specific ways, such
> that various bits of functionality don't work. This patch provides a
> mechanism for overriding mii registers during init based on the contents of
> the device tree data, allowing board-specific fixups without having to
> pollute generic code.
> 
> Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>
> ---
>  Documentation/devicetree/bindings/net/phy.txt |  13 +++
>  drivers/net/phy/phy_device.c                  |  29 +++++-
>  drivers/of/of_net.c                           | 124 ++++++++++++++++++++++++++
>  include/linux/of_net.h                        |  12 +++
>  4 files changed, 177 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/phy.txt b/Documentation/devicetree/bindings/net/phy.txt
> index 7cd18fb..552a5e0 100644
> --- a/Documentation/devicetree/bindings/net/phy.txt
> +++ b/Documentation/devicetree/bindings/net/phy.txt
> @@ -23,6 +23,19 @@ Optional Properties:
>    assume clause 22. The compatible list may also contain other
>    elements.
>  
> +The following properties may be added to either the phy node or the parent
> +ethernet device:
> +
> +- phy-mii-advertise-10half: Whether to advertise half-duplex 10MBit
> +- phy-mii-advertise-10full: Whether to advertise full-duplex 10MBit
> +- phy-mii-advertise-100half: Whether to advertise half-duplex 100MBit
> +- phy-mii-advertise-100full: Whether to advertise full-duplex 100MBit
> +- phy-mii-advertise-100base4: Whether to advertise 100base4
> +- phy-mii-advertise-1000half: Whether to advertise half-duplex 1000MBit
> +- phy-mii-advertise-1000full: Whether to advertise full-duplex 1000MBit
> +- phy-mii-as-master: Configure phy to act as master/slave
> +- phy-mii-manual-master: Enable/disable manual master/slave configuration
> +
>  Example:
>  
>  ethernet-phy@0 {

These properties are booleans, and optional?  Does it mean that
you cannot _disable_ broken features?  Or does it mean that you
_must_ specify the non-broken features and thus break backwards
compatibility?  Or are these properties not boolean (they are not
used in the example either, unfortunately), and the binding text
would need an update for clarity?  What am I missing?


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office@denx.de

Re: [PATCH] net/dt: Add support for overriding phy configuration from device tree

[Matthew Garrett]

On Thu, 2014-01-16 at 14:59 +0100, Gerhard Sittig wrote:

> These properties are booleans, and optional?  Does it mean that
> you cannot _disable_ broken features?  Or does it mean that you
> _must_ specify the non-broken features and thus break backwards
> compatibility?  Or are these properties not boolean (they are not
> used in the example either, unfortunately), and the binding text
> would need an update for clarity?  What am I missing?

They're not booleans. I'll update the text to make that clear.

-- 
Matthew Garrett <matthew.garrett@nebula.com>

[PATCH V2] net/dt: Add support for overriding phy configuration from device tree

[Matthew Garrett]

Some hardware may be broken in interesting and board-specific ways, such
that various bits of functionality don't work. This patch provides a
mechanism for overriding mii registers during init based on the contents of
the device tree data, allowing board-specific fixups without having to
pollute generic code.

Signed-off-by: Matthew Garrett <matthew.garrett-05XSO3Yj/JvQT0dZR+AlfA@public.gmane.org>
---
 Documentation/devicetree/bindings/net/phy.txt |  21 +++++
 drivers/net/phy/phy_device.c                  |  29 +++++-
 drivers/of/of_net.c                           | 124 ++++++++++++++++++++++++++
 include/linux/of_net.h                        |  12 +++
 4 files changed, 185 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/net/phy.txt b/Documentation/devicetree/bindings/net/phy.txt
index 7cd18fb..fc50f02 100644
--- a/Documentation/devicetree/bindings/net/phy.txt
+++ b/Documentation/devicetree/bindings/net/phy.txt
@@ -23,6 +23,21 @@ Optional Properties:
   assume clause 22. The compatible list may also contain other
   elements.
 
+The following properties may be added to either the phy node or the parent
+ethernet device. If not present, the hardware defaults will be used.
+
+- phy-mii-advertise-10half: 1 to advertise half-duplex 10MBit, 0 to disable
+- phy-mii-advertise-10full: 1 to advertise full-duplex 10MBit, 0 to disable
+- phy-mii-advertise-100half: 1 to advertise half-duplex 100MBit, 0 to disable
+- phy-mii-advertise-100full: 1 to advertise full-duplex 100MBit, 0 to disable
+- phy-mii-advertise-100base4: 1 to advertise 100base4, 0 to disable
+- phy-mii-advertise-1000half: 1 to advertise half-duplex 1000MBit, 0 to disable
+- phy-mii-advertise-1000full: 1 to advertise full-duplex 1000MBit, 0 to disable
+- phy-mii-manual-master: 1 to enable manual master/slave configuration, 0
+  to disable manual master/slave configuration
+- phy-mii-as-master: 1 to configure phy to act as master, 0 to configure phy
+  to act as slave. Ignored if manual master/slave configuration is not enabled.
+
 Example:
 
 ethernet-phy@0 {
@@ -32,4 +47,10 @@ ethernet-phy@0 {
 	interrupts = <35 1>;
 	reg = <0>;
 	device_type = "ethernet-phy";
+	// Disable advertising of full duplex 1000MBit
+	phy-mii-advertise-1000full = <0>;
+	// Force manual phy master/slave configuration
+	phy-mii-manual-master = <1>;
+	// Forcibly configure phy as slave
+	phy-mii-as-master = <0>;
 };
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index d6447b3..91793bc 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -33,6 +33,7 @@
 #include <linux/mii.h>
 #include <linux/ethtool.h>
 #include <linux/phy.h>
+#include <linux/of_net.h>
 
 #include <asm/io.h>
 #include <asm/irq.h>
@@ -497,6 +498,28 @@ void phy_disconnect(struct phy_device *phydev)
 }
 EXPORT_SYMBOL(phy_disconnect);
 
+int phy_override_from_of(struct phy_device *phydev)
+{
+	int reg, regval;
+	u16 val, mask;
+
+	/* Check for phy register overrides from OF */
+	for (reg = 0; reg < 16; reg++) {
+		if (!of_get_mii_register(phydev, reg, &val, &mask)) {
+			if (!mask)
+				continue;
+			regval = phy_read(phydev, reg);
+			if (regval < 0)
+				continue;
+			regval &= ~mask;
+			regval |= val;
+			phy_write(phydev, reg, regval);
+		}
+	}
+
+	return 0;
+}
+
 int phy_init_hw(struct phy_device *phydev)
 {
 	int ret;
@@ -508,7 +531,11 @@ int phy_init_hw(struct phy_device *phydev)
 	if (ret < 0)
 		return ret;
 
-	return phydev->drv->config_init(phydev);
+	ret = phydev->drv->config_init(phydev);
+	if (ret < 0)
+		return ret;
+
+	return phy_override_from_of(phydev);
 }
 
 /**
diff --git a/drivers/of/of_net.c b/drivers/of/of_net.c
index 8f9be2e..4545608 100644
--- a/drivers/of/of_net.c
+++ b/drivers/of/of_net.c
@@ -93,3 +93,127 @@ const void *of_get_mac_address(struct device_node *np)
 	return NULL;
 }
 EXPORT_SYMBOL(of_get_mac_address);
+
+/**
+ * Provide phy register overrides from the device tree. Some hardware may
+ * be broken in interesting and board-specific ways, so we want a mechanism
+ * for the board data to provide overrides for default values. This should be
+ * called during phy init.
+ */
+int of_get_mii_register(struct phy_device *phydev, int reg, u16 *val,
+			u16 *mask)
+{
+	u32 tmp;
+	struct device *dev = &phydev->dev;
+	struct device_node *np = dev->of_node;
+
+	*val = 0;
+	*mask = 0;
+
+	if (!np && dev->parent->of_node)
+		np = dev->parent->of_node;
+
+	if (!np)
+		return 0;
+
+	switch (reg) {
+	case MII_ADVERTISE:
+		if (!of_property_read_u32(np, "phy-mii-advertise-10half",
+					   &tmp)) {
+			if (tmp) {
+				*val |= ADVERTISE_10HALF;
+				phydev->advertising |= SUPPORTED_10baseT_Half;
+			} else {
+				phydev->advertising &=
+					~(SUPPORTED_10baseT_Half);
+			}
+
+			*mask |= ADVERTISE_10HALF;
+		}
+		if (!of_property_read_u32(np, "phy-mii-advertise-10full",
+					   &tmp)) {
+			if (tmp) {
+				*val |= ADVERTISE_10FULL;
+				phydev->advertising |= SUPPORTED_10baseT_Full;
+			} else {
+				phydev->advertising &=
+					~(SUPPORTED_10baseT_Full);
+			}
+
+			*mask |= ADVERTISE_10FULL;
+		}
+		if (!of_property_read_u32(np, "phy-mii-advertise-100half",
+					   &tmp)) {
+			if (tmp) {
+				*val |= ADVERTISE_100HALF;
+				phydev->advertising |= SUPPORTED_100baseT_Half;
+			} else {
+				phydev->advertising &=
+					~(SUPPORTED_100baseT_Half);
+			}
+
+			*mask |= ADVERTISE_100HALF;
+		}
+		if (!of_property_read_u32(np, "phy-mii-advertise-100full",
+					   &tmp)) {
+			if (tmp) {
+				*val |= ADVERTISE_100FULL;
+				phydev->advertising |= SUPPORTED_100baseT_Full;
+			} else {
+				phydev->advertising &=
+					~(SUPPORTED_100baseT_Full);
+			}
+
+			*mask |= ADVERTISE_100FULL;
+		}
+		if (!of_property_read_u32(np, "phy-mii-advertise-100base4",
+					   &tmp)) {
+			if (tmp)
+				*val |= ADVERTISE_100BASE4;
+			*mask |= ADVERTISE_100BASE4;
+		}
+		break;
+	case MII_CTRL1000:
+		if (!of_property_read_u32(np, "phy-mii-advertise-1000full",
+					   &tmp)) {
+			if (tmp) {
+				*val |= ADVERTISE_1000FULL;
+				phydev->advertising |= SUPPORTED_1000baseT_Full;
+			} else {
+				phydev->advertising &=
+					~(SUPPORTED_1000baseT_Full);
+			}
+
+			*mask |= ADVERTISE_1000FULL;
+		}
+		if (!of_property_read_u32(np, "phy-mii-advertise-1000half",
+					   &tmp)) {
+			if (tmp) {
+				*val |= ADVERTISE_1000HALF;
+				phydev->advertising |= SUPPORTED_1000baseT_Half;
+			} else {
+				phydev->advertising &=
+					~(SUPPORTED_1000baseT_Half);
+			}
+
+			*mask |= ADVERTISE_1000HALF;
+		}
+		if (!of_property_read_u32(np, "phy-mii-as-master",
+					   &tmp)) {
+			if (tmp)
+				*val |= CTL1000_AS_MASTER;
+			*mask |= CTL1000_AS_MASTER;
+		}
+		if (!of_property_read_u32(np, "phy-mii-manual-master",
+					   &tmp)) {
+			if (tmp)
+				*val |= CTL1000_ENABLE_MASTER;
+			*mask |= CTL1000_ENABLE_MASTER;
+		}
+		break;
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
+EXPORT_SYMBOL(of_get_mii_register);
diff --git a/include/linux/of_net.h b/include/linux/of_net.h
index 34597c8..2e478bc 100644
--- a/include/linux/of_net.h
+++ b/include/linux/of_net.h
@@ -7,10 +7,14 @@
 #ifndef __LINUX_OF_NET_H
 #define __LINUX_OF_NET_H
 
+#include <linux/phy.h>
+
 #ifdef CONFIG_OF_NET
 #include <linux/of.h>
 extern int of_get_phy_mode(struct device_node *np);
 extern const void *of_get_mac_address(struct device_node *np);
+extern int of_get_mii_register(struct phy_device *np, int reg, u16 *val,
+			       u16 *mask);
 #else
 static inline int of_get_phy_mode(struct device_node *np)
 {
@@ -21,6 +25,14 @@ static inline const void *of_get_mac_address(struct device_node *np)
 {
 	return NULL;
 }
+static inline int of_get_mii_register(struct phy_device *np, int reg, u16 *val,
+				      u16 *mask)
+{
+	*val = 0;
+	*mask = 0;
+
+	return -EINVAL;
+}
 #endif
 
 #endif /* __LINUX_OF_NET_H */
-- 
1.8.4.2

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2] net/dt: Add support for overriding phy configuration from device tree

[Arnd Bergmann]

On Thursday 16 January 2014 09:47:11 Matthew Garrett wrote:
> +               if (!of_property_read_u32(np, "phy-mii-advertise-10half",
> +                                          &tmp)) {
> +                       if (tmp) {
> +                               *val |= ADVERTISE_10HALF;
> +                               phydev->advertising |= SUPPORTED_10baseT_Half;
> +                       } else {
> +                               phydev->advertising &=
> +                                       ~(SUPPORTED_10baseT_Half);
> +                       }
> +
> +                       *mask |= ADVERTISE_10HALF;
> +               }
> +               if (!of_property_read_u32(np, "phy-mii-advertise-10full",
> +                                          &tmp)) {
> +                       if (tmp) {
> +                               *val |= ADVERTISE_10FULL;
> +                               phydev->advertising |= SUPPORTED_10baseT_Full;
> +                       } else {
> +                               phydev->advertising &=
> +                                       ~(SUPPORTED_10baseT_Full);
> +                       }
> +
> +                       *mask |= ADVERTISE_10FULL;
> +               }
> 

Just a style suggestion: rather than almost duplicating the same 12 lines
of code for each property, I think it can be split out into a helper
function.

	Arnd

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V3] net/dt: Add support for overriding phy configuration from device tree

[Matthew Garrett]

Some hardware may be broken in interesting and board-specific ways, such
that various bits of functionality don't work. This patch provides a
mechanism for overriding mii registers during init based on the contents of
the device tree data, allowing board-specific fixups without having to
pollute generic code.

Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>
---

V3: Break the main function out into some helper functions and store the
values in some structs.

 Documentation/devicetree/bindings/net/phy.txt | 21 +++++++
 drivers/net/phy/phy_device.c                  | 29 ++++++++-
 drivers/of/of_net.c                           | 87 +++++++++++++++++++++++++++
 include/linux/of_net.h                        | 12 ++++
 4 files changed, 148 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/net/phy.txt b/Documentation/devicetree/bindings/net/phy.txt
index 7cd18fb..fc50f02 100644
--- a/Documentation/devicetree/bindings/net/phy.txt
+++ b/Documentation/devicetree/bindings/net/phy.txt
@@ -23,6 +23,21 @@ Optional Properties:
   assume clause 22. The compatible list may also contain other
   elements.
 
+The following properties may be added to either the phy node or the parent
+ethernet device. If not present, the hardware defaults will be used.
+
+- phy-mii-advertise-10half: 1 to advertise half-duplex 10MBit, 0 to disable
+- phy-mii-advertise-10full: 1 to advertise full-duplex 10MBit, 0 to disable
+- phy-mii-advertise-100half: 1 to advertise half-duplex 100MBit, 0 to disable
+- phy-mii-advertise-100full: 1 to advertise full-duplex 100MBit, 0 to disable
+- phy-mii-advertise-100base4: 1 to advertise 100base4, 0 to disable
+- phy-mii-advertise-1000half: 1 to advertise half-duplex 1000MBit, 0 to disable
+- phy-mii-advertise-1000full: 1 to advertise full-duplex 1000MBit, 0 to disable
+- phy-mii-manual-master: 1 to enable manual master/slave configuration, 0
+  to disable manual master/slave configuration
+- phy-mii-as-master: 1 to configure phy to act as master, 0 to configure phy
+  to act as slave. Ignored if manual master/slave configuration is not enabled.
+
 Example:
 
 ethernet-phy@0 {
@@ -32,4 +47,10 @@ ethernet-phy@0 {
 	interrupts = <35 1>;
 	reg = <0>;
 	device_type = "ethernet-phy";
+	// Disable advertising of full duplex 1000MBit
+	phy-mii-advertise-1000full = <0>;
+	// Force manual phy master/slave configuration
+	phy-mii-manual-master = <1>;
+	// Forcibly configure phy as slave
+	phy-mii-as-master = <0>;
 };
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index d6447b3..91793bc 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -33,6 +33,7 @@
 #include <linux/mii.h>
 #include <linux/ethtool.h>
 #include <linux/phy.h>
+#include <linux/of_net.h>
 
 #include <asm/io.h>
 #include <asm/irq.h>
@@ -497,6 +498,28 @@ void phy_disconnect(struct phy_device *phydev)
 }
 EXPORT_SYMBOL(phy_disconnect);
 
+int phy_override_from_of(struct phy_device *phydev)
+{
+	int reg, regval;
+	u16 val, mask;
+
+	/* Check for phy register overrides from OF */
+	for (reg = 0; reg < 16; reg++) {
+		if (!of_get_mii_register(phydev, reg, &val, &mask)) {
+			if (!mask)
+				continue;
+			regval = phy_read(phydev, reg);
+			if (regval < 0)
+				continue;
+			regval &= ~mask;
+			regval |= val;
+			phy_write(phydev, reg, regval);
+		}
+	}
+
+	return 0;
+}
+
 int phy_init_hw(struct phy_device *phydev)
 {
 	int ret;
@@ -508,7 +531,11 @@ int phy_init_hw(struct phy_device *phydev)
 	if (ret < 0)
 		return ret;
 
-	return phydev->drv->config_init(phydev);
+	ret = phydev->drv->config_init(phydev);
+	if (ret < 0)
+		return ret;
+
+	return phy_override_from_of(phydev);
 }
 
 /**
diff --git a/drivers/of/of_net.c b/drivers/of/of_net.c
index 8f9be2e..75751b7 100644
--- a/drivers/of/of_net.c
+++ b/drivers/of/of_net.c
@@ -93,3 +93,90 @@ const void *of_get_mac_address(struct device_node *np)
 	return NULL;
 }
 EXPORT_SYMBOL(of_get_mac_address);
+
+struct mii_override {
+	char *prop;
+	u32 supported;
+	u16 value;
+};
+
+static struct mii_override mii_advertise_override[] = {
+	{ "phy-mii-advertise-10half", SUPPORTED_10baseT_Half,
+	  ADVERTISE_10HALF },
+	{ "phy-mii-advertise-10full", SUPPORTED_10baseT_Full,
+	  ADVERTISE_10FULL },
+	{ "phy-mii-advertise-100half", SUPPORTED_100baseT_Half,
+	  ADVERTISE_100HALF },
+	{ "phy-mii-advertise-100full", SUPPORTED_100baseT_Full,
+	  ADVERTISE_100FULL },
+	{ "phy-mii-advertise-100base4", 0, ADVERTISE_100BASE4 },
+	{ NULL },
+};
+
+static struct mii_override mii_gigabit_override[] = {
+	{ "phy-mii-advertise-1000half", SUPPORTED_1000baseT_Half,
+	  ADVERTISE_1000HALF },
+	{ "phy-mii-advertise-1000full", SUPPORTED_1000baseT_Full,
+	  ADVERTISE_1000FULL },
+	{ "phy-mii-as-master", 0, CTL1000_AS_MASTER },
+	{ "phy-mii-manual-master", 0, CTL1000_ENABLE_MASTER },
+	{ NULL },
+};
+
+static void mii_handle_override(struct mii_override *override_list,
+				struct phy_device *phydev, u16 *val, u16 *mask)
+{
+	struct device *dev = &phydev->dev;
+	struct device_node *np = dev->of_node;
+	struct mii_override *override;
+	u32 tmp;
+
+	if (!np && dev->parent->of_node)
+		np = dev->parent->of_node;
+
+	if (!np)
+		return;
+
+	for (override = &override_list[0]; override->prop != NULL; override++) {
+		if (!of_property_read_u32(np, override->prop, &tmp)) {
+			if (tmp) {
+				*val |= override->value;
+				phydev->advertising |= override->supported;
+			} else {
+				phydev->advertising &= ~(override->supported);
+			}
+
+			*mask |= override->value;
+		}
+	}
+
+	return;
+}
+
+/**
+ * Provide phy register overrides from the device tree. Some hardware may
+ * be broken in interesting and board-specific ways, so we want a mechanism
+ * for the board data to provide overrides for default values. This should be
+ * called during phy init.
+ */
+int of_get_mii_register(struct phy_device *phydev, int reg, u16 *val,
+			u16 *mask)
+{
+	*val = 0;
+	*mask = 0;
+
+	switch (reg) {
+	case MII_ADVERTISE:
+		mii_handle_override(mii_advertise_override, phydev, val, mask);
+		break;
+
+	case MII_CTRL1000:
+		mii_handle_override(mii_gigabit_override, phydev, val, mask);
+		break;
+
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
+EXPORT_SYMBOL(of_get_mii_register);
diff --git a/include/linux/of_net.h b/include/linux/of_net.h
index 34597c8..2e478bc 100644
--- a/include/linux/of_net.h
+++ b/include/linux/of_net.h
@@ -7,10 +7,14 @@
 #ifndef __LINUX_OF_NET_H
 #define __LINUX_OF_NET_H
 
+#include <linux/phy.h>
+
 #ifdef CONFIG_OF_NET
 #include <linux/of.h>
 extern int of_get_phy_mode(struct device_node *np);
 extern const void *of_get_mac_address(struct device_node *np);
+extern int of_get_mii_register(struct phy_device *np, int reg, u16 *val,
+			       u16 *mask);
 #else
 static inline int of_get_phy_mode(struct device_node *np)
 {
@@ -21,6 +25,14 @@ static inline const void *of_get_mac_address(struct device_node *np)
 {
 	return NULL;
 }
+static inline int of_get_mii_register(struct phy_device *np, int reg, u16 *val,
+				      u16 *mask)
+{
+	*val = 0;
+	*mask = 0;
+
+	return -EINVAL;
+}
 #endif
 
 #endif /* __LINUX_OF_NET_H */
-- 
1.8.4.2

Re: [PATCH V3] net/dt: Add support for overriding phy configuration from device tree

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 2093 bytes --]

On Fri, 2014-01-17 at 17:57 -0500, Matthew Garrett wrote:
> Some hardware may be broken in interesting and board-specific ways, such
> that various bits of functionality don't work. This patch provides a
> mechanism for overriding mii registers during init based on the contents of
> the device tree data, allowing board-specific fixups without having to
> pollute generic code.
[...]
> --- a/Documentation/devicetree/bindings/net/phy.txt
> +++ b/Documentation/devicetree/bindings/net/phy.txt
> @@ -23,6 +23,21 @@ Optional Properties:
>    assume clause 22. The compatible list may also contain other
>    elements.
>  
> +The following properties may be added to either the phy node or the parent
> +ethernet device. If not present, the hardware defaults will be used.
> +
> +- phy-mii-advertise-10half: 1 to advertise half-duplex 10MBit, 0 to disable
> +- phy-mii-advertise-10full: 1 to advertise full-duplex 10MBit, 0 to disable
> +- phy-mii-advertise-100half: 1 to advertise half-duplex 100MBit, 0 to disable
> +- phy-mii-advertise-100full: 1 to advertise full-duplex 100MBit, 0 to disable
> +- phy-mii-advertise-100base4: 1 to advertise 100base4, 0 to disable
> +- phy-mii-advertise-1000half: 1 to advertise half-duplex 1000MBit, 0 to disable
> +- phy-mii-advertise-1000full: 1 to advertise full-duplex 1000MBit, 0 to disable

Are these really all needed?  Apparently there is a standard 'max-speed'
property on Ethernet devices already, which I think is probably
sufficient to express the actual restrictions of some boards.

> +- phy-mii-manual-master: 1 to enable manual master/slave configuration, 0
> +  to disable manual master/slave configuration
[...]

Although the standard calls this 'manual', if it's set in the DT it
won't really be a manual setting.  The description should probably
clarify that.

I think the name should include 'master-slave' or 'clock-role' rather
than just 'master', as currently it suggests only the master role can be
forced.

Ben.

-- 
Ben Hutchings
friends: People who know you well, but like you anyway.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH V3] net/dt: Add support for overriding phy configuration from device tree

[Florian Fainelli]

2014-01-19 Ben Hutchings <ben-/+tVBieCtBitmTQ+vhA3Yw@public.gmane.org>:
> On Fri, 2014-01-17 at 17:57 -0500, Matthew Garrett wrote:
>> Some hardware may be broken in interesting and board-specific ways, such
>> that various bits of functionality don't work. This patch provides a
>> mechanism for overriding mii registers during init based on the contents of
>> the device tree data, allowing board-specific fixups without having to
>> pollute generic code.
> [...]
>> --- a/Documentation/devicetree/bindings/net/phy.txt
>> +++ b/Documentation/devicetree/bindings/net/phy.txt
>> @@ -23,6 +23,21 @@ Optional Properties:
>>    assume clause 22. The compatible list may also contain other
>>    elements.
>>
>> +The following properties may be added to either the phy node or the parent
>> +ethernet device. If not present, the hardware defaults will be used.
>> +
>> +- phy-mii-advertise-10half: 1 to advertise half-duplex 10MBit, 0 to disable
>> +- phy-mii-advertise-10full: 1 to advertise full-duplex 10MBit, 0 to disable
>> +- phy-mii-advertise-100half: 1 to advertise half-duplex 100MBit, 0 to disable
>> +- phy-mii-advertise-100full: 1 to advertise full-duplex 100MBit, 0 to disable
>> +- phy-mii-advertise-100base4: 1 to advertise 100base4, 0 to disable
>> +- phy-mii-advertise-1000half: 1 to advertise half-duplex 1000MBit, 0 to disable
>> +- phy-mii-advertise-1000full: 1 to advertise full-duplex 1000MBit, 0 to disable
>
> Are these really all needed?  Apparently there is a standard 'max-speed'
> property on Ethernet devices already, which I think is probably
> sufficient to express the actual restrictions of some boards.

This is what I think as well. Maybe there is the need for something
more fine-grained, although I really doubt it.

>
>> +- phy-mii-manual-master: 1 to enable manual master/slave configuration, 0
>> +  to disable manual master/slave configuration
> [...]
>
> Although the standard calls this 'manual', if it's set in the DT it
> won't really be a manual setting.  The description should probably
> clarify that.
>
> I think the name should include 'master-slave' or 'clock-role' rather
> than just 'master', as currently it suggests only the master role can be
> forced.

Right, and this would match what 802.3-2008, Section 22.2.4.3.7 mentions.

>
> Ben.
>
> --
> Ben Hutchings
> friends: People who know you well, but like you anyway.



-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V3] net/dt: Add support for overriding phy configuration from device tree

[Grant Likely]

On Fri, 17 Jan 2014 17:57:39 -0500, Matthew Garrett <matthew.garrett@nebula.com> wrote:
> Some hardware may be broken in interesting and board-specific ways, such
> that various bits of functionality don't work. This patch provides a
> mechanism for overriding mii registers during init based on the contents of
> the device tree data, allowing board-specific fixups without having to
> pollute generic code.
> 
> Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>

I've got no problems in principle with the feature. I think it is a good
change. The binding itself feels too verbose to me. As an alternative,
how about the following:

	phy-mii-advertise = "10half", "!10full", "!100base4"

So it is a list of features. Prepending a '!' makes it disabled. And we
could add something like "nodefault" to clear the probed settings
entirely before listing the modes we want.

g.

> ---
> 
> V3: Break the main function out into some helper functions and store the
> values in some structs.
> 
>  Documentation/devicetree/bindings/net/phy.txt | 21 +++++++
>  drivers/net/phy/phy_device.c                  | 29 ++++++++-
>  drivers/of/of_net.c                           | 87 +++++++++++++++++++++++++++
>  include/linux/of_net.h                        | 12 ++++
>  4 files changed, 148 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/phy.txt b/Documentation/devicetree/bindings/net/phy.txt
> index 7cd18fb..fc50f02 100644
> --- a/Documentation/devicetree/bindings/net/phy.txt
> +++ b/Documentation/devicetree/bindings/net/phy.txt
> @@ -23,6 +23,21 @@ Optional Properties:
>    assume clause 22. The compatible list may also contain other
>    elements.
>  
> +The following properties may be added to either the phy node or the parent
> +ethernet device. If not present, the hardware defaults will be used.
> +
> +- phy-mii-advertise-10half: 1 to advertise half-duplex 10MBit, 0 to disable
> +- phy-mii-advertise-10full: 1 to advertise full-duplex 10MBit, 0 to disable
> +- phy-mii-advertise-100half: 1 to advertise half-duplex 100MBit, 0 to disable
> +- phy-mii-advertise-100full: 1 to advertise full-duplex 100MBit, 0 to disable
> +- phy-mii-advertise-100base4: 1 to advertise 100base4, 0 to disable
> +- phy-mii-advertise-1000half: 1 to advertise half-duplex 1000MBit, 0 to disable
> +- phy-mii-advertise-1000full: 1 to advertise full-duplex 1000MBit, 0 to disable
> +- phy-mii-manual-master: 1 to enable manual master/slave configuration, 0
> +  to disable manual master/slave configuration
> +- phy-mii-as-master: 1 to configure phy to act as master, 0 to configure phy
> +  to act as slave. Ignored if manual master/slave configuration is not enabled.
> +
>  Example:
>  
>  ethernet-phy@0 {
> @@ -32,4 +47,10 @@ ethernet-phy@0 {
>  	interrupts = <35 1>;
>  	reg = <0>;
>  	device_type = "ethernet-phy";
> +	// Disable advertising of full duplex 1000MBit
> +	phy-mii-advertise-1000full = <0>;
> +	// Force manual phy master/slave configuration
> +	phy-mii-manual-master = <1>;
> +	// Forcibly configure phy as slave
> +	phy-mii-as-master = <0>;
>  };
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index d6447b3..91793bc 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -33,6 +33,7 @@
>  #include <linux/mii.h>
>  #include <linux/ethtool.h>
>  #include <linux/phy.h>
> +#include <linux/of_net.h>
>  
>  #include <asm/io.h>
>  #include <asm/irq.h>
> @@ -497,6 +498,28 @@ void phy_disconnect(struct phy_device *phydev)
>  }
>  EXPORT_SYMBOL(phy_disconnect);
>  
> +int phy_override_from_of(struct phy_device *phydev)
> +{
> +	int reg, regval;
> +	u16 val, mask;
> +
> +	/* Check for phy register overrides from OF */
> +	for (reg = 0; reg < 16; reg++) {
> +		if (!of_get_mii_register(phydev, reg, &val, &mask)) {
> +			if (!mask)
> +				continue;
> +			regval = phy_read(phydev, reg);
> +			if (regval < 0)
> +				continue;
> +			regval &= ~mask;
> +			regval |= val;
> +			phy_write(phydev, reg, regval);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  int phy_init_hw(struct phy_device *phydev)
>  {
>  	int ret;
> @@ -508,7 +531,11 @@ int phy_init_hw(struct phy_device *phydev)
>  	if (ret < 0)
>  		return ret;
>  
> -	return phydev->drv->config_init(phydev);
> +	ret = phydev->drv->config_init(phydev);
> +	if (ret < 0)
> +		return ret;
> +
> +	return phy_override_from_of(phydev);
>  }
>  
>  /**
> diff --git a/drivers/of/of_net.c b/drivers/of/of_net.c
> index 8f9be2e..75751b7 100644
> --- a/drivers/of/of_net.c
> +++ b/drivers/of/of_net.c
> @@ -93,3 +93,90 @@ const void *of_get_mac_address(struct device_node *np)
>  	return NULL;
>  }
>  EXPORT_SYMBOL(of_get_mac_address);
> +
> +struct mii_override {
> +	char *prop;
> +	u32 supported;
> +	u16 value;
> +};
> +
> +static struct mii_override mii_advertise_override[] = {
> +	{ "phy-mii-advertise-10half", SUPPORTED_10baseT_Half,
> +	  ADVERTISE_10HALF },
> +	{ "phy-mii-advertise-10full", SUPPORTED_10baseT_Full,
> +	  ADVERTISE_10FULL },
> +	{ "phy-mii-advertise-100half", SUPPORTED_100baseT_Half,
> +	  ADVERTISE_100HALF },
> +	{ "phy-mii-advertise-100full", SUPPORTED_100baseT_Full,
> +	  ADVERTISE_100FULL },
> +	{ "phy-mii-advertise-100base4", 0, ADVERTISE_100BASE4 },
> +	{ NULL },
> +};
> +
> +static struct mii_override mii_gigabit_override[] = {
> +	{ "phy-mii-advertise-1000half", SUPPORTED_1000baseT_Half,
> +	  ADVERTISE_1000HALF },
> +	{ "phy-mii-advertise-1000full", SUPPORTED_1000baseT_Full,
> +	  ADVERTISE_1000FULL },
> +	{ "phy-mii-as-master", 0, CTL1000_AS_MASTER },
> +	{ "phy-mii-manual-master", 0, CTL1000_ENABLE_MASTER },
> +	{ NULL },
> +};
> +
> +static void mii_handle_override(struct mii_override *override_list,
> +				struct phy_device *phydev, u16 *val, u16 *mask)
> +{
> +	struct device *dev = &phydev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct mii_override *override;
> +	u32 tmp;
> +
> +	if (!np && dev->parent->of_node)
> +		np = dev->parent->of_node;
> +
> +	if (!np)
> +		return;
> +
> +	for (override = &override_list[0]; override->prop != NULL; override++) {
> +		if (!of_property_read_u32(np, override->prop, &tmp)) {
> +			if (tmp) {
> +				*val |= override->value;
> +				phydev->advertising |= override->supported;
> +			} else {
> +				phydev->advertising &= ~(override->supported);
> +			}
> +
> +			*mask |= override->value;
> +		}
> +	}
> +
> +	return;
> +}
> +
> +/**
> + * Provide phy register overrides from the device tree. Some hardware may
> + * be broken in interesting and board-specific ways, so we want a mechanism
> + * for the board data to provide overrides for default values. This should be
> + * called during phy init.
> + */
> +int of_get_mii_register(struct phy_device *phydev, int reg, u16 *val,
> +			u16 *mask)
> +{
> +	*val = 0;
> +	*mask = 0;
> +
> +	switch (reg) {
> +	case MII_ADVERTISE:
> +		mii_handle_override(mii_advertise_override, phydev, val, mask);
> +		break;
> +
> +	case MII_CTRL1000:
> +		mii_handle_override(mii_gigabit_override, phydev, val, mask);
> +		break;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +EXPORT_SYMBOL(of_get_mii_register);
> diff --git a/include/linux/of_net.h b/include/linux/of_net.h
> index 34597c8..2e478bc 100644
> --- a/include/linux/of_net.h
> +++ b/include/linux/of_net.h
> @@ -7,10 +7,14 @@
>  #ifndef __LINUX_OF_NET_H
>  #define __LINUX_OF_NET_H
>  
> +#include <linux/phy.h>
> +
>  #ifdef CONFIG_OF_NET
>  #include <linux/of.h>
>  extern int of_get_phy_mode(struct device_node *np);
>  extern const void *of_get_mac_address(struct device_node *np);
> +extern int of_get_mii_register(struct phy_device *np, int reg, u16 *val,
> +			       u16 *mask);
>  #else
>  static inline int of_get_phy_mode(struct device_node *np)
>  {
> @@ -21,6 +25,14 @@ static inline const void *of_get_mac_address(struct device_node *np)
>  {
>  	return NULL;
>  }
> +static inline int of_get_mii_register(struct phy_device *np, int reg, u16 *val,
> +				      u16 *mask)
> +{
> +	*val = 0;
> +	*mask = 0;
> +
> +	return -EINVAL;
> +}
>  #endif
>  
>  #endif /* __LINUX_OF_NET_H */
> -- 
> 1.8.4.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH V3] net/dt: Add support for overriding phy configuration from device tree

[Florian Fainelli]

2014-01-17 Matthew Garrett <matthew.garrett@nebula.com>:
> Some hardware may be broken in interesting and board-specific ways, such
> that various bits of functionality don't work. This patch provides a
> mechanism for overriding mii registers during init based on the contents of
> the device tree data, allowing board-specific fixups without having to
> pollute generic code.

It would be good to explain exactly how your hardware is broken
exactly. I really do not think that such a fine-grained setting where
you could disable, e.g: 100BaseT_Full, but allow 100BaseT_Half to
remain usable makes that much sense. In general, Gigabit might be
badly broken, but 100 and 10Mbits/sec should work fine. How about the
MASTER-SLAVE bit, is overriding it really required?

Is not a PHY fixup registered for a specific OUI the solution you are
looking for? I am also concerned that this creates PHY troubleshooting
issues much harder to debug than before as we may have no idea about
how much information has been put in Device Tree to override that.

Finally, how about making this more general just like the BCM87xx PHY
driver, which is supplied value/reg pairs directly? There are 16
common MII registers, and 16 others for vendor specific registers,
this is just covering for about 2% of the possible changes.

>
> Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>
> ---
>
> V3: Break the main function out into some helper functions and store the
> values in some structs.
>
>  Documentation/devicetree/bindings/net/phy.txt | 21 +++++++
>  drivers/net/phy/phy_device.c                  | 29 ++++++++-
>  drivers/of/of_net.c                           | 87 +++++++++++++++++++++++++++
>  include/linux/of_net.h                        | 12 ++++
>  4 files changed, 148 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/net/phy.txt b/Documentation/devicetree/bindings/net/phy.txt
> index 7cd18fb..fc50f02 100644
> --- a/Documentation/devicetree/bindings/net/phy.txt
> +++ b/Documentation/devicetree/bindings/net/phy.txt
> @@ -23,6 +23,21 @@ Optional Properties:
>    assume clause 22. The compatible list may also contain other
>    elements.
>
> +The following properties may be added to either the phy node or the parent
> +ethernet device. If not present, the hardware defaults will be used.
> +
> +- phy-mii-advertise-10half: 1 to advertise half-duplex 10MBit, 0 to disable
> +- phy-mii-advertise-10full: 1 to advertise full-duplex 10MBit, 0 to disable
> +- phy-mii-advertise-100half: 1 to advertise half-duplex 100MBit, 0 to disable
> +- phy-mii-advertise-100full: 1 to advertise full-duplex 100MBit, 0 to disable
> +- phy-mii-advertise-100base4: 1 to advertise 100base4, 0 to disable
> +- phy-mii-advertise-1000half: 1 to advertise half-duplex 1000MBit, 0 to disable
> +- phy-mii-advertise-1000full: 1 to advertise full-duplex 1000MBit, 0 to disable
> +- phy-mii-manual-master: 1 to enable manual master/slave configuration, 0
> +  to disable manual master/slave configuration
> +- phy-mii-as-master: 1 to configure phy to act as master, 0 to configure phy
> +  to act as slave. Ignored if manual master/slave configuration is not enabled.
> +
>  Example:
>
>  ethernet-phy@0 {
> @@ -32,4 +47,10 @@ ethernet-phy@0 {
>         interrupts = <35 1>;
>         reg = <0>;
>         device_type = "ethernet-phy";
> +       // Disable advertising of full duplex 1000MBit
> +       phy-mii-advertise-1000full = <0>;
> +       // Force manual phy master/slave configuration
> +       phy-mii-manual-master = <1>;
> +       // Forcibly configure phy as slave
> +       phy-mii-as-master = <0>;
>  };
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index d6447b3..91793bc 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -33,6 +33,7 @@
>  #include <linux/mii.h>
>  #include <linux/ethtool.h>
>  #include <linux/phy.h>
> +#include <linux/of_net.h>
>
>  #include <asm/io.h>
>  #include <asm/irq.h>
> @@ -497,6 +498,28 @@ void phy_disconnect(struct phy_device *phydev)
>  }
>  EXPORT_SYMBOL(phy_disconnect);
>
> +int phy_override_from_of(struct phy_device *phydev)
> +{
> +       int reg, regval;
> +       u16 val, mask;
> +
> +       /* Check for phy register overrides from OF */
> +       for (reg = 0; reg < 16; reg++) {
> +               if (!of_get_mii_register(phydev, reg, &val, &mask)) {
> +                       if (!mask)
> +                               continue;
> +                       regval = phy_read(phydev, reg);
> +                       if (regval < 0)
> +                               continue;
> +                       regval &= ~mask;
> +                       regval |= val;
> +                       phy_write(phydev, reg, regval);
> +               }
> +       }
> +
> +       return 0;
> +}
> +
>  int phy_init_hw(struct phy_device *phydev)
>  {
>         int ret;
> @@ -508,7 +531,11 @@ int phy_init_hw(struct phy_device *phydev)
>         if (ret < 0)
>                 return ret;
>
> -       return phydev->drv->config_init(phydev);
> +       ret = phydev->drv->config_init(phydev);
> +       if (ret < 0)
> +               return ret;
> +
> +       return phy_override_from_of(phydev);
>  }
>
>  /**
> diff --git a/drivers/of/of_net.c b/drivers/of/of_net.c
> index 8f9be2e..75751b7 100644
> --- a/drivers/of/of_net.c
> +++ b/drivers/of/of_net.c
> @@ -93,3 +93,90 @@ const void *of_get_mac_address(struct device_node *np)
>         return NULL;
>  }
>  EXPORT_SYMBOL(of_get_mac_address);
> +
> +struct mii_override {
> +       char *prop;
> +       u32 supported;
> +       u16 value;
> +};
> +
> +static struct mii_override mii_advertise_override[] = {
> +       { "phy-mii-advertise-10half", SUPPORTED_10baseT_Half,
> +         ADVERTISE_10HALF },
> +       { "phy-mii-advertise-10full", SUPPORTED_10baseT_Full,
> +         ADVERTISE_10FULL },
> +       { "phy-mii-advertise-100half", SUPPORTED_100baseT_Half,
> +         ADVERTISE_100HALF },
> +       { "phy-mii-advertise-100full", SUPPORTED_100baseT_Full,
> +         ADVERTISE_100FULL },
> +       { "phy-mii-advertise-100base4", 0, ADVERTISE_100BASE4 },
> +       { NULL },
> +};
> +
> +static struct mii_override mii_gigabit_override[] = {
> +       { "phy-mii-advertise-1000half", SUPPORTED_1000baseT_Half,
> +         ADVERTISE_1000HALF },
> +       { "phy-mii-advertise-1000full", SUPPORTED_1000baseT_Full,
> +         ADVERTISE_1000FULL },
> +       { "phy-mii-as-master", 0, CTL1000_AS_MASTER },
> +       { "phy-mii-manual-master", 0, CTL1000_ENABLE_MASTER },
> +       { NULL },
> +};
> +
> +static void mii_handle_override(struct mii_override *override_list,
> +                               struct phy_device *phydev, u16 *val, u16 *mask)
> +{
> +       struct device *dev = &phydev->dev;
> +       struct device_node *np = dev->of_node;
> +       struct mii_override *override;
> +       u32 tmp;
> +
> +       if (!np && dev->parent->of_node)
> +               np = dev->parent->of_node;
> +
> +       if (!np)
> +               return;
> +
> +       for (override = &override_list[0]; override->prop != NULL; override++) {
> +               if (!of_property_read_u32(np, override->prop, &tmp)) {
> +                       if (tmp) {
> +                               *val |= override->value;
> +                               phydev->advertising |= override->supported;
> +                       } else {
> +                               phydev->advertising &= ~(override->supported);
> +                       }
> +
> +                       *mask |= override->value;
> +               }
> +       }
> +
> +       return;
> +}
> +
> +/**
> + * Provide phy register overrides from the device tree. Some hardware may
> + * be broken in interesting and board-specific ways, so we want a mechanism
> + * for the board data to provide overrides for default values. This should be
> + * called during phy init.
> + */
> +int of_get_mii_register(struct phy_device *phydev, int reg, u16 *val,
> +                       u16 *mask)
> +{
> +       *val = 0;
> +       *mask = 0;
> +
> +       switch (reg) {
> +       case MII_ADVERTISE:
> +               mii_handle_override(mii_advertise_override, phydev, val, mask);
> +               break;
> +
> +       case MII_CTRL1000:
> +               mii_handle_override(mii_gigabit_override, phydev, val, mask);
> +               break;
> +
> +       default:
> +               return -EINVAL;
> +       }
> +       return 0;
> +}
> +EXPORT_SYMBOL(of_get_mii_register);
> diff --git a/include/linux/of_net.h b/include/linux/of_net.h
> index 34597c8..2e478bc 100644
> --- a/include/linux/of_net.h
> +++ b/include/linux/of_net.h
> @@ -7,10 +7,14 @@
>  #ifndef __LINUX_OF_NET_H
>  #define __LINUX_OF_NET_H
>
> +#include <linux/phy.h>
> +
>  #ifdef CONFIG_OF_NET
>  #include <linux/of.h>
>  extern int of_get_phy_mode(struct device_node *np);
>  extern const void *of_get_mac_address(struct device_node *np);
> +extern int of_get_mii_register(struct phy_device *np, int reg, u16 *val,
> +                              u16 *mask);
>  #else
>  static inline int of_get_phy_mode(struct device_node *np)
>  {
> @@ -21,6 +25,14 @@ static inline const void *of_get_mac_address(struct device_node *np)
>  {
>         return NULL;
>  }
> +static inline int of_get_mii_register(struct phy_device *np, int reg, u16 *val,
> +                                     u16 *mask)
> +{
> +       *val = 0;
> +       *mask = 0;
> +
> +       return -EINVAL;
> +}
>  #endif
>
>  #endif /* __LINUX_OF_NET_H */
> --
> 1.8.4.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/



-- 
Florian

Re: [PATCH V3] net/dt: Add support for overriding phy configuration from device tree

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1232 bytes --]

On Tue, 2014-02-04 at 12:39 -0800, Florian Fainelli wrote:
> 2014-01-17 Matthew Garrett <matthew.garrett@nebula.com>:
> > Some hardware may be broken in interesting and board-specific ways, such
> > that various bits of functionality don't work. This patch provides a
> > mechanism for overriding mii registers during init based on the contents of
> > the device tree data, allowing board-specific fixups without having to
> > pollute generic code.
> 
> It would be good to explain exactly how your hardware is broken
> exactly. I really do not think that such a fine-grained setting where
> you could disable, e.g: 100BaseT_Full, but allow 100BaseT_Half to
> remain usable makes that much sense. In general, Gigabit might be
> badly broken, but 100 and 10Mbits/sec should work fine. How about the
> MASTER-SLAVE bit, is overriding it really required?

Yes, it is entirely possible that one or other of the clock modes
(locally generated vs recovered) is not reliable.

> Is not a PHY fixup registered for a specific OUI the solution you are
> looking for?
[...]

The fault is in the board, not the PHY.

Ben.

-- 
Ben Hutchings
One of the nice things about standards is that there are so many of them.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH V3] net/dt: Add support for overriding phy configuration from device tree

[Florian Fainelli]

2014-02-04 Ben Hutchings <ben@decadent.org.uk>:
> On Tue, 2014-02-04 at 12:39 -0800, Florian Fainelli wrote:
>> 2014-01-17 Matthew Garrett <matthew.garrett@nebula.com>:
>> > Some hardware may be broken in interesting and board-specific ways, such
>> > that various bits of functionality don't work. This patch provides a
>> > mechanism for overriding mii registers during init based on the contents of
>> > the device tree data, allowing board-specific fixups without having to
>> > pollute generic code.
>>
>> It would be good to explain exactly how your hardware is broken
>> exactly. I really do not think that such a fine-grained setting where
>> you could disable, e.g: 100BaseT_Full, but allow 100BaseT_Half to
>> remain usable makes that much sense. In general, Gigabit might be
>> badly broken, but 100 and 10Mbits/sec should work fine. How about the
>> MASTER-SLAVE bit, is overriding it really required?
>
> Yes, it is entirely possible that one or other of the clock modes
> (locally generated vs recovered) is not reliable.

That one is not covered in the existing Ethernet PHY binding, okay for
handling it.

>
>> Is not a PHY fixup registered for a specific OUI the solution you are
>> looking for?
> [...]
>
> The fault is in the board, not the PHY.

What kind of fault at the board level are we talking about? Lack of
specific twisted pair wiring to the RJ-45 jack? Out of spec RXC/TXC on
a (R)GMII path? If the latter, this is going to be via vendor-specific
MII registers, and should be a good enough reason for registering a
PHY fixup. What about pad control, and Ethernet MACs specicif register
affecting the internal delays and such?
-- 
Florian

Re: [PATCH V3] net/dt: Add support for overriding phy configuration from device tree

[Grant Likely]

On Tue, 4 Feb 2014 12:39:41 -0800, Florian Fainelli <f.fainelli@gmail.com> wrote:
> 2014-01-17 Matthew Garrett <matthew.garrett@nebula.com>:
> > Some hardware may be broken in interesting and board-specific ways, such
> > that various bits of functionality don't work. This patch provides a
> > mechanism for overriding mii registers during init based on the contents of
> > the device tree data, allowing board-specific fixups without having to
> > pollute generic code.
> 
> It would be good to explain exactly how your hardware is broken
> exactly. I really do not think that such a fine-grained setting where
> you could disable, e.g: 100BaseT_Full, but allow 100BaseT_Half to
> remain usable makes that much sense. In general, Gigabit might be
> badly broken, but 100 and 10Mbits/sec should work fine. How about the
> MASTER-SLAVE bit, is overriding it really required?
> 
> Is not a PHY fixup registered for a specific OUI the solution you are
> looking for? I am also concerned that this creates PHY troubleshooting
> issues much harder to debug than before as we may have no idea about
> how much information has been put in Device Tree to override that.
> 
> Finally, how about making this more general just like the BCM87xx PHY
> driver, which is supplied value/reg pairs directly? There are 16
> common MII registers, and 16 others for vendor specific registers,
> this is just covering for about 2% of the possible changes.

I would be fine with that too.

g.

RE: [PATCH V3] net/dt: Add support for overriding phy configuration from device tree

[David Laight]

From: Florian Fainelli
> It would be good to explain exactly how your hardware is broken
> exactly. I really do not think that such a fine-grained setting where
> you could disable, e.g: 100BaseT_Full, but allow 100BaseT_Half to
> remain usable makes that much sense. In general, Gigabit might be
> badly broken, but 100 and 10Mbits/sec should work fine. How about the
> MASTER-SLAVE bit, is overriding it really required?

There are plenty of systems out there where you'd want to disable
either HDX or FDX modes.
The MAC unit has to know whether the PHY is in HDX or FDX in order
to work properly. Many do not need to know the speed - since the
PHY is responsible for the tx/rx fifo clock.
Getting the negotiated speed out of the PHY can be difficult, while
the ANAR can easily be set.
Unfortunately it is usually impossible to disable the 'fall-back'
10M HDX.

	David

Re: [PATCH V3] net/dt: Add support for overriding phy configuration from device tree

[Florian Fainelli]

2014-02-05 David Laight <David.Laight-JxhZ9S5GRejQT0dZR+AlfA@public.gmane.org>:
> From: Florian Fainelli
>> It would be good to explain exactly how your hardware is broken
>> exactly. I really do not think that such a fine-grained setting where
>> you could disable, e.g: 100BaseT_Full, but allow 100BaseT_Half to
>> remain usable makes that much sense. In general, Gigabit might be
>> badly broken, but 100 and 10Mbits/sec should work fine. How about the
>> MASTER-SLAVE bit, is overriding it really required?
>
> There are plenty of systems out there where you'd want to disable
> either HDX or FDX modes.
> The MAC unit has to know whether the PHY is in HDX or FDX in order
> to work properly. Many do not need to know the speed - since the
> PHY is responsible for the tx/rx fifo clock.
> Getting the negotiated speed out of the PHY can be difficult, while
> the ANAR can easily be set.
> Unfortunately it is usually impossible to disable the 'fall-back'
> 10M HDX.

The problem that I have with that approach in general is that:

- it bloats the code for a set of properties that are going to be used
by hopefully a few percentage of the actual Device Trees out there
- it puts no limits on what is acceptable/best-practice to be put in
terms of configuration in the Device Tree, how about the 16x16 other
register values out there which are standardized?
- a PHY fixup should be registered based on the top-level compatible
property for a given board where the specific PHY on a specific board
is known to be broken
- make things incredibly harder to debug than they are today

I do acknowledge the need to have a solution to these problems, but
this seems to duplicate existing mechanisms available (e.g: PHY
fixups) without leveraging information that should be properly flagged
in the Device Tree (board model, root-node compatible string etc...)
to allow software to take corrective measures.
-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V3] net/dt: Add support for overriding phy configuration from device tree

[Gerlando Falauto]

Hi,

I'm currently trying to fix an issue for which this patch provides a 
partial solution, so apologies in advance for jumping into the 
discussion for my own purposes...

On 02/04/2014 09:39 PM, Florian Fainelli wrote:> 2014-01-17 Matthew 
Garrett <matthew.garrett@nebula.com>:
 >> Some hardware may be broken in interesting and board-specific ways, such
 >> that various bits of functionality don't work. This patch provides a
 >> mechanism for overriding mii registers during init based on the 
contents of
 >> the device tree data, allowing board-specific fixups without having to
 >> pollute generic code.
 >
 > It would be good to explain exactly how your hardware is broken
 > exactly. I really do not think that such a fine-grained setting where
 > you could disable, e.g: 100BaseT_Full, but allow 100BaseT_Half to
 > remain usable makes that much sense. In general, Gigabit might be
 > badly broken, but 100 and 10Mbits/sec should work fine. How about the
 > MASTER-SLAVE bit, is overriding it really required?
 >
 > Is not a PHY fixup registered for a specific OUI the solution you are
 > looking for? I am also concerned that this creates PHY troubleshooting
 > issues much harder to debug than before as we may have no idea about
 > how much information has been put in Device Tree to override that.
 >
 > Finally, how about making this more general just like the BCM87xx PHY
 > driver, which is supplied value/reg pairs directly? There are 16
 > common MII registers, and 16 others for vendor specific registers,
 > this is just covering for about 2% of the possible changes.

Good point. That would easily help me with my current issue, which 
requires autoneg to be disabled to begin with (by clearing BMCR_ANENABLE 
from register 0).
This would not however fix it entirely (I tried a quick hardwired 
implementation), as the whole PHY machinery would not take that into 
account and would re-enable autoneg anyway.
I also tried changing the patch so that phydev->support gets updated 
(instead of phydev->advertising):

 >> +               if (!of_property_read_u32(np, override->prop, &tmp)) {
 >> +                       if (tmp) {
 >> +                               *val |= override->value;
 >> +                               phydev->advertising |= 
override->supported;
 >> +                       } else {
 >> +                               phydev->advertising &= 
~(override->supported);
 >> +                       }
 >> +
 >> +                       *mask |= override->value;

What I find weird is that the only way phydev->autoneg could ever be set 
to disabled is from here (phy.c):

static void phy_sanitize_settings(struct phy_device *phydev)
{
	u32 features = phydev->supported;
	int idx;

	/* Sanitize settings based on PHY capabilities */
	if ((features & SUPPORTED_Autoneg) == 0)
		phydev->autoneg = AUTONEG_DISABLE;

which is in turn only called when phydev->autoneg is set to 
AUTONEG_DISABLE to begin with:

int phy_start_aneg(struct phy_device *phydev)
{
	int err;

	mutex_lock(&phydev->lock);

	if (AUTONEG_DISABLE == phydev->autoneg)
		phy_sanitize_settings(phydev);

So could someone please help me figure out what I'm missing here?

Thanks!
Gerlando

Re: [PATCH V3] net/dt: Add support for overriding phy configuration from device tree

[Florian Fainelli]

Hi Gerlando,

Le lundi 10 février 2014, 17:14:59 Gerlando Falauto a écrit :
> Hi,
> 
> I'm currently trying to fix an issue for which this patch provides a
> partial solution, so apologies in advance for jumping into the
> discussion for my own purposes...
> 
> On 02/04/2014 09:39 PM, Florian Fainelli wrote:> 2014-01-17 Matthew
> 
> Garrett <matthew.garrett-05XSO3Yj/JvQT0dZR+AlfA@public.gmane.org>:
>  >> Some hardware may be broken in interesting and board-specific ways, such
>  >> that various bits of functionality don't work. This patch provides a
>  >> mechanism for overriding mii registers during init based on the
> 
> contents of
> 
>  >> the device tree data, allowing board-specific fixups without having to
>  >> pollute generic code.
>  > 
>  > It would be good to explain exactly how your hardware is broken
>  > exactly. I really do not think that such a fine-grained setting where
>  > you could disable, e.g: 100BaseT_Full, but allow 100BaseT_Half to
>  > remain usable makes that much sense. In general, Gigabit might be
>  > badly broken, but 100 and 10Mbits/sec should work fine. How about the
>  > MASTER-SLAVE bit, is overriding it really required?
>  > 
>  > Is not a PHY fixup registered for a specific OUI the solution you are
>  > looking for? I am also concerned that this creates PHY troubleshooting
>  > issues much harder to debug than before as we may have no idea about
>  > how much information has been put in Device Tree to override that.
>  > 
>  > Finally, how about making this more general just like the BCM87xx PHY
>  > driver, which is supplied value/reg pairs directly? There are 16
>  > common MII registers, and 16 others for vendor specific registers,
>  > this is just covering for about 2% of the possible changes.
> 
> Good point. That would easily help me with my current issue, which
> requires autoneg to be disabled to begin with (by clearing BMCR_ANENABLE
> from register 0).

Is there a point in time (e.g: after some specific initial configuration has 
been made) where BMCR_ANENABLE can be used?

> This would not however fix it entirely (I tried a quick hardwired
> implementation), as the whole PHY machinery would not take that into
> account and would re-enable autoneg anyway.
> I also tried changing the patch so that phydev->support gets updated

There are multiple things that you could try doing here:

- override the PHY state machine in your read_status callback to make sure 
that you always set phydev->autoneg set to AUTONEG_ENABLE

- clear the SUPPORTED_Autoneg bits from phydev->supported right after PHY 
registration and before the call to phy_start()

- set the PHY_HAS_MAGICANEG bit in your PHY driver flag

> 
> (instead of phydev->advertising):
>  >> +               if (!of_property_read_u32(np, override->prop, &tmp)) {
>  >> +                       if (tmp) {
>  >> +                               *val |= override->value;
>  >> +                               phydev->advertising |=
> 
> override->supported;
> 
>  >> +                       } else {
>  >> +                               phydev->advertising &=
> 
> ~(override->supported);
> 
>  >> +                       }
>  >> +
>  >> +                       *mask |= override->value;
> 
> What I find weird is that the only way phydev->autoneg could ever be set
> to disabled is from here (phy.c):
> 
> static void phy_sanitize_settings(struct phy_device *phydev)
> {
> 	u32 features = phydev->supported;
> 	int idx;
> 
> 	/* Sanitize settings based on PHY capabilities */
> 	if ((features & SUPPORTED_Autoneg) == 0)
> 		phydev->autoneg = AUTONEG_DISABLE;
> 
> which is in turn only called when phydev->autoneg is set to
> AUTONEG_DISABLE to begin with:
> 
> int phy_start_aneg(struct phy_device *phydev)
> {
> 	int err;
> 
> 	mutex_lock(&phydev->lock);
> 
> 	if (AUTONEG_DISABLE == phydev->autoneg)
> 		phy_sanitize_settings(phydev);
> 
> So could someone please help me figure out what I'm missing here?

At first glance it looks like the PHY driver should be reading the phydev-
>autoneg value when the PHY driver config_aneg() callback is called to be 
allowed to set the forced speed and settings.

The way phy_sanitize_settings() is coded does not make it return a mask of 
features, but only the forced supported speed and duplex. Then when the link 
is forced but we are having some issues getting a link status, libphy tries 
lower speeds and this function is used again to provide the next speed/duplex 
pair to try.
-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V3] net/dt: Add support for overriding phy configuration from device tree

[Gerlando Falauto]

Hi Florian,

first of all, thank you for your answer.

On 02/10/2014 06:09 PM, Florian Fainelli wrote:
> Hi Gerlando,
>
> Le lundi 10 février 2014, 17:14:59 Gerlando Falauto a écrit :
>> Hi,
>>
>> I'm currently trying to fix an issue for which this patch provides a
>> partial solution, so apologies in advance for jumping into the
>> discussion for my own purposes...
>>
>> On 02/04/2014 09:39 PM, Florian Fainelli wrote:> 2014-01-17 Matthew
>>
>> Garrett <matthew.garrett@nebula.com>:
>>   >> Some hardware may be broken in interesting and board-specific ways, such
>>   >> that various bits of functionality don't work. This patch provides a
>>   >> mechanism for overriding mii registers during init based on the
>>
>> contents of
>>
>>   >> the device tree data, allowing board-specific fixups without having to
>>   >> pollute generic code.
>>   >
>>   > It would be good to explain exactly how your hardware is broken
>>   > exactly. I really do not think that such a fine-grained setting where
>>   > you could disable, e.g: 100BaseT_Full, but allow 100BaseT_Half to
>>   > remain usable makes that much sense. In general, Gigabit might be
>>   > badly broken, but 100 and 10Mbits/sec should work fine. How about the
>>   > MASTER-SLAVE bit, is overriding it really required?
>>   >
>>   > Is not a PHY fixup registered for a specific OUI the solution you are
>>   > looking for? I am also concerned that this creates PHY troubleshooting
>>   > issues much harder to debug than before as we may have no idea about
>>   > how much information has been put in Device Tree to override that.
>>   >
>>   > Finally, how about making this more general just like the BCM87xx PHY
>>   > driver, which is supplied value/reg pairs directly? There are 16
>>   > common MII registers, and 16 others for vendor specific registers,
>>   > this is just covering for about 2% of the possible changes.
>>
>> Good point. That would easily help me with my current issue, which
>> requires autoneg to be disabled to begin with (by clearing BMCR_ANENABLE
>> from register 0).
>
> Is there a point in time (e.g: after some specific initial configuration has
> been made) where BMCR_ANENABLE can be used?

What do you mean? In my case, for some HW-related reason (due to the PHY 
counterpart I guess) autoneg needs to be disabled.
This is currently done by the bootloader code (which clears the bit).
What I'm looking for is some way for the kernel to either reinforce this 
setting, or just take that into account and skip autoneg.
On top of that, there's a HW errata about that particular PHY, which 
requires certain operations to be performed on the PHY as a workaround 
*WHEN AUTONEG IS DISABLED*. That I'd implement on a PHY-specif driver.

>> This would not however fix it entirely (I tried a quick hardwired
>> implementation), as the whole PHY machinery would not take that into
>> account and would re-enable autoneg anyway.
>> I also tried changing the patch so that phydev->support gets updated
>
> There are multiple things that you could try doing here:
>
> - override the PHY state machine in your read_status callback to make sure
> that you always set phydev->autoneg set to AUTONEG_ENABLE

[you mean AUTONEG_DISABLE, right?]
Uhm, but I don't want to implement a driver for that PHY that always 
disables autoneg. I only want to disable autoneg for that particular 
board. I figure I might register a fixup for that board, but that kindof 
makes everything more complicated and less clear. Plus, what should be 
the criterion to determine whether we're running on that particular 
hardware?

> - clear the SUPPORTED_Autoneg bits from phydev->supported right after PHY
> registration and before the call to phy_start()

I actually tried clearing it by tweaking the patch on this thread, but 
the end result is that it does not produce any effect (see further 
comments below). Only thing that seems to play a role here is explictly 
setting phydev->autoneg = AUTONEG_DISABLE.

> - set the PHY_HAS_MAGICANEG bit in your PHY driver flag

Again, this seems to play no role whatsoever here:

			} else if (0 == phydev->link_timeout--) {
				needs_aneg = 1;
				/* If we have the magic_aneg bit,
				 * we try again */
				if (phydev->drv->flags & PHY_HAS_MAGICANEG)
					break;
			}
			break;
		case PHY_NOLINK:

This code might have made sense when it was written in 2006 -- back 
then, the break statement was skipping some fallback code. But now it 
seems to do nothing.

>
>>
>> (instead of phydev->advertising):
>>   >> +               if (!of_property_read_u32(np, override->prop, &tmp)) {
>>   >> +                       if (tmp) {
>>   >> +                               *val |= override->value;
>>   >> +                               phydev->advertising |=
>>
>> override->supported;
>>
>>   >> +                       } else {
>>   >> +                               phydev->advertising &=
>>
>> ~(override->supported);
>>
>>   >> +                       }
>>   >> +
>>   >> +                       *mask |= override->value;
>>
>> What I find weird is that the only way phydev->autoneg could ever be set
>> to disabled is from here (phy.c):
>>
>> static void phy_sanitize_settings(struct phy_device *phydev)
>> {
>> 	u32 features = phydev->supported;
>> 	int idx;
>>
>> 	/* Sanitize settings based on PHY capabilities */
>> 	if ((features & SUPPORTED_Autoneg) == 0)
>> 		phydev->autoneg = AUTONEG_DISABLE;
>>
>> which is in turn only called when phydev->autoneg is set to
>> AUTONEG_DISABLE to begin with:
>>
>> int phy_start_aneg(struct phy_device *phydev)
>> {
>> 	int err;
>>
>> 	mutex_lock(&phydev->lock);
>>
>> 	if (AUTONEG_DISABLE == phydev->autoneg)
>> 		phy_sanitize_settings(phydev);
>>
>> So could someone please help me figure out what I'm missing here?
>
> At first glance it looks like the PHY driver should be reading the phydev-
>> autoneg value when the PHY driver config_aneg() callback is called to be
> allowed to set the forced speed and settings.
>
> The way phy_sanitize_settings() is coded does not make it return a mask of
> features, but only the forced supported speed and duplex. Then when the link
> is forced but we are having some issues getting a link status, libphy tries
> lower speeds and this function is used again to provide the next speed/duplex
> pair to try.
>

What I was trying to say is that phy_sanitize_settings() is only called 
when phydev->autoneg == AUTONEG_DISABLE, and in turn it's the only 
generic function setting phydev->autoneg = AUTONEG_DISABLE.
So perhaps the condition should read:

- 	if (AUTONEG_DISABLE == phydev->autoneg)
+ 	if ((features & SUPPORTED_Autoneg) == 0)
  		phy_sanitize_settings(phydev);

Or else, some other parts of the generic code should take care of 
setting it to AUTONEG_DISABLE, depending on whether the feature is 
supported or not.
What I found weird is explicitly setting a value (phydev->autoneg = 
AUTONEG_DISABLE), from a static function which is only called when that 
condition is already true.

BTW, I feel like disabling autoneg from the start has never been a use 
case before, am I right?

Thanks!
Gerlando

Re: [PATCH V3] net/dt: Add support for overriding phy configuration from device tree

[Florian Fainelli]

Hi Gerlando,

2014-02-11 1:09 GMT-08:00 Gerlando Falauto <gerlando.falauto-SkAbAL50j+6IwRZHo2/mJg@public.gmane.orgm>:
> Hi Florian,
>
> first of all, thank you for your answer.
>
>
> On 02/10/2014 06:09 PM, Florian Fainelli wrote:
>>
>> Hi Gerlando,
>>
>> Le lundi 10 février 2014, 17:14:59 Gerlando Falauto a écrit :
>>>
>>> Hi,
>>>
>>> I'm currently trying to fix an issue for which this patch provides a
>>> partial solution, so apologies in advance for jumping into the
>>> discussion for my own purposes...
>>>
>>> On 02/04/2014 09:39 PM, Florian Fainelli wrote:> 2014-01-17 Matthew
>>>
>>> Garrett <matthew.garrett-05XSO3Yj/JvQT0dZR+AlfA@public.gmane.org>:
>>>   >> Some hardware may be broken in interesting and board-specific ways,
>>> such
>>>   >> that various bits of functionality don't work. This patch provides a
>>>   >> mechanism for overriding mii registers during init based on the
>>>
>>> contents of
>>>
>>>   >> the device tree data, allowing board-specific fixups without having
>>> to
>>>   >> pollute generic code.
>>>   >
>>>   > It would be good to explain exactly how your hardware is broken
>>>   > exactly. I really do not think that such a fine-grained setting where
>>>   > you could disable, e.g: 100BaseT_Full, but allow 100BaseT_Half to
>>>   > remain usable makes that much sense. In general, Gigabit might be
>>>   > badly broken, but 100 and 10Mbits/sec should work fine. How about the
>>>   > MASTER-SLAVE bit, is overriding it really required?
>>>   >
>>>   > Is not a PHY fixup registered for a specific OUI the solution you are
>>>   > looking for? I am also concerned that this creates PHY
>>> troubleshooting
>>>   > issues much harder to debug than before as we may have no idea about
>>>   > how much information has been put in Device Tree to override that.
>>>   >
>>>   > Finally, how about making this more general just like the BCM87xx PHY
>>>   > driver, which is supplied value/reg pairs directly? There are 16
>>>   > common MII registers, and 16 others for vendor specific registers,
>>>   > this is just covering for about 2% of the possible changes.
>>>
>>> Good point. That would easily help me with my current issue, which
>>> requires autoneg to be disabled to begin with (by clearing BMCR_ANENABLE
>>> from register 0).
>>
>>
>> Is there a point in time (e.g: after some specific initial configuration
>> has
>> been made) where BMCR_ANENABLE can be used?
>
>
> What do you mean? In my case, for some HW-related reason (due to the PHY
> counterpart I guess) autoneg needs to be disabled.
> This is currently done by the bootloader code (which clears the bit).
> What I'm looking for is some way for the kernel to either reinforce this
> setting, or just take that into account and skip autoneg.
> On top of that, there's a HW errata about that particular PHY, which
> requires certain operations to be performed on the PHY as a workaround *WHEN
> AUTONEG IS DISABLED*. That I'd implement on a PHY-specif driver.

Ok.

>
>
>>> This would not however fix it entirely (I tried a quick hardwired
>>> implementation), as the whole PHY machinery would not take that into
>>> account and would re-enable autoneg anyway.
>>> I also tried changing the patch so that phydev->support gets updated
>>
>>
>> There are multiple things that you could try doing here:
>>
>> - override the PHY state machine in your read_status callback to make sure
>> that you always set phydev->autoneg set to AUTONEG_ENABLE
>
>
> [you mean AUTONEG_DISABLE, right?]

Right, I fat fingered here.

> Uhm, but I don't want to implement a driver for that PHY that always
> disables autoneg. I only want to disable autoneg for that particular board.
> I figure I might register a fixup for that board, but that kindof makes
> everything more complicated and less clear. Plus, what should be the
> criterion to determine whether we're running on that particular hardware?

of_machine_is_compatible() plus reading the specific PHY OUI should
provide you with with an unique machine + PHY tuple. If your machine
name is too generic.

>
>
>> - clear the SUPPORTED_Autoneg bits from phydev->supported right after PHY
>> registration and before the call to phy_start()
>
>
> I actually tried clearing it by tweaking the patch on this thread, but the
> end result is that it does not produce any effect (see further comments
> below). Only thing that seems to play a role here is explictly setting
> phydev->autoneg = AUTONEG_DISABLE.
>
>
>> - set the PHY_HAS_MAGICANEG bit in your PHY driver flag
>
>
> Again, this seems to play no role whatsoever here:
>
>                         } else if (0 == phydev->link_timeout--) {
>                                 needs_aneg = 1;
>                                 /* If we have the magic_aneg bit,
>                                  * we try again */
>                                 if (phydev->drv->flags & PHY_HAS_MAGICANEG)
>                                         break;
>                         }
>                         break;
>                 case PHY_NOLINK:
>
> This code might have made sense when it was written in 2006 -- back then,
> the break statement was skipping some fallback code. But now it seems to do
> nothing.
>
>
>>
>>>
>>> (instead of phydev->advertising):
>>>   >> +               if (!of_property_read_u32(np, override->prop, &tmp))
>>> {
>>>   >> +                       if (tmp) {
>>>   >> +                               *val |= override->value;
>>>   >> +                               phydev->advertising |=
>>>
>>> override->supported;
>>>
>>>   >> +                       } else {
>>>   >> +                               phydev->advertising &=
>>>
>>> ~(override->supported);
>>>
>>>   >> +                       }
>>>   >> +
>>>   >> +                       *mask |= override->value;
>>>
>>> What I find weird is that the only way phydev->autoneg could ever be set
>>> to disabled is from here (phy.c):
>>>
>>> static void phy_sanitize_settings(struct phy_device *phydev)
>>> {
>>>         u32 features = phydev->supported;
>>>         int idx;
>>>
>>>         /* Sanitize settings based on PHY capabilities */
>>>         if ((features & SUPPORTED_Autoneg) == 0)
>>>                 phydev->autoneg = AUTONEG_DISABLE;
>>>
>>> which is in turn only called when phydev->autoneg is set to
>>> AUTONEG_DISABLE to begin with:
>>>
>>> int phy_start_aneg(struct phy_device *phydev)
>>> {
>>>         int err;
>>>
>>>         mutex_lock(&phydev->lock);
>>>
>>>         if (AUTONEG_DISABLE == phydev->autoneg)
>>>                 phy_sanitize_settings(phydev);
>>>
>>> So could someone please help me figure out what I'm missing here?
>>
>>
>> At first glance it looks like the PHY driver should be reading the phydev-
>>>
>>> autoneg value when the PHY driver config_aneg() callback is called to be
>>
>> allowed to set the forced speed and settings.
>>
>> The way phy_sanitize_settings() is coded does not make it return a mask of
>> features, but only the forced supported speed and duplex. Then when the
>> link
>> is forced but we are having some issues getting a link status, libphy
>> tries
>> lower speeds and this function is used again to provide the next
>> speed/duplex
>> pair to try.
>>
>
> What I was trying to say is that phy_sanitize_settings() is only called when
> phydev->autoneg == AUTONEG_DISABLE, and in turn it's the only generic
> function setting phydev->autoneg = AUTONEG_DISABLE.
> So perhaps the condition should read:
>
> -       if (AUTONEG_DISABLE == phydev->autoneg)
> +       if ((features & SUPPORTED_Autoneg) == 0)
>                 phy_sanitize_settings(phydev);
>
> Or else, some other parts of the generic code should take care of setting it
> to AUTONEG_DISABLE, depending on whether the feature is supported or not.
> What I found weird is explicitly setting a value (phydev->autoneg =
> AUTONEG_DISABLE), from a static function which is only called when that
> condition is already true.

I do not think that this change is correct either, let me cook a patch
for you to allow disabling autoneg from the start.

>
> BTW, I feel like disabling autoneg from the start has never been a use case
> before, am I right?

Not really no, and that is because most hardware does not need quirks
to work correctly.

>
> Thanks!
> Gerlando



-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V3] net/dt: Add support for overriding phy configuration from device tree

[Gerlando Falauto]

Hi Florian,

On 02/11/2014 06:43 PM, Florian Fainelli wrote:
> Hi Gerlando,
>
> 2014-02-11 1:09 GMT-08:00 Gerlando Falauto <gerlando.falauto@keymile.com>:
>> Hi Florian,
>>
>> first of all, thank you for your answer.
>>
>>
>> On 02/10/2014 06:09 PM, Florian Fainelli wrote:
>>>
>>> Hi Gerlando,
>>>
>>> Le lundi 10 février 2014, 17:14:59 Gerlando Falauto a écrit :
>>>>
>>>> Hi,
>>>>
>>>> I'm currently trying to fix an issue for which this patch provides a
>>>> partial solution, so apologies in advance for jumping into the
>>>> discussion for my own purposes...
>>>>
>>>> On 02/04/2014 09:39 PM, Florian Fainelli wrote:> 2014-01-17 Matthew
>>>>
>>>> Garrett <matthew.garrett@nebula.com>:
>>>>    >> Some hardware may be broken in interesting and board-specific ways,
>>>> such
>>>>    >> that various bits of functionality don't work. This patch provides a
>>>>    >> mechanism for overriding mii registers during init based on the
>>>>
>>>> contents of
>>>>
>>>>    >> the device tree data, allowing board-specific fixups without having
>>>> to
>>>>    >> pollute generic code.
>>>>    >
>>>>    > It would be good to explain exactly how your hardware is broken
>>>>    > exactly. I really do not think that such a fine-grained setting where
>>>>    > you could disable, e.g: 100BaseT_Full, but allow 100BaseT_Half to
>>>>    > remain usable makes that much sense. In general, Gigabit might be
>>>>    > badly broken, but 100 and 10Mbits/sec should work fine. How about the
>>>>    > MASTER-SLAVE bit, is overriding it really required?
>>>>    >
>>>>    > Is not a PHY fixup registered for a specific OUI the solution you are
>>>>    > looking for? I am also concerned that this creates PHY
>>>> troubleshooting
>>>>    > issues much harder to debug than before as we may have no idea about
>>>>    > how much information has been put in Device Tree to override that.
>>>>    >
>>>>    > Finally, how about making this more general just like the BCM87xx PHY
>>>>    > driver, which is supplied value/reg pairs directly? There are 16
>>>>    > common MII registers, and 16 others for vendor specific registers,
>>>>    > this is just covering for about 2% of the possible changes.
>>>>
>>>> Good point. That would easily help me with my current issue, which
>>>> requires autoneg to be disabled to begin with (by clearing BMCR_ANENABLE
>>>> from register 0).
>>>
>>>
>>> Is there a point in time (e.g: after some specific initial configuration
>>> has
>>> been made) where BMCR_ANENABLE can be used?
>>
>>
>> What do you mean? In my case, for some HW-related reason (due to the PHY
>> counterpart I guess) autoneg needs to be disabled.
>> This is currently done by the bootloader code (which clears the bit).
>> What I'm looking for is some way for the kernel to either reinforce this
>> setting, or just take that into account and skip autoneg.
>> On top of that, there's a HW errata about that particular PHY, which
>> requires certain operations to be performed on the PHY as a workaround *WHEN
>> AUTONEG IS DISABLED*. That I'd implement on a PHY-specif driver.
>
> Ok.
>
>>
>>
>>>> This would not however fix it entirely (I tried a quick hardwired
>>>> implementation), as the whole PHY machinery would not take that into
>>>> account and would re-enable autoneg anyway.
>>>> I also tried changing the patch so that phydev->support gets updated
>>>
>>>
>>> There are multiple things that you could try doing here:
>>>
>>> - override the PHY state machine in your read_status callback to make sure
>>> that you always set phydev->autoneg set to AUTONEG_ENABLE
>>
>>
>> [you mean AUTONEG_DISABLE, right?]
>
> Right, I fat fingered here.
>
>> Uhm, but I don't want to implement a driver for that PHY that always
>> disables autoneg. I only want to disable autoneg for that particular board.
>> I figure I might register a fixup for that board, but that kindof makes
>> everything more complicated and less clear. Plus, what should be the
>> criterion to determine whether we're running on that particular hardware?
>
> of_machine_is_compatible() plus reading the specific PHY OUI should
> provide you with with an unique machine + PHY tuple. If your machine
> name is too generic.

Uhm, actually, my machine name ("model") is specific, but the compatible 
string is indeed generic so this would mean adding an extra string 
there. Not that it's a big issue, but it just seems too complicated and 
hard to follow. After all, we wanted device tree in the first place to 
get rid of board-sepcific files. To me, filtering by machine name looks 
like a big step backwards, especially if it's all about a "pretty 
standard feature" like disabling autoneg.

>
>>
>>
>>> - clear the SUPPORTED_Autoneg bits from phydev->supported right after PHY
>>> registration and before the call to phy_start()
>>
>>
>> I actually tried clearing it by tweaking the patch on this thread, but the
>> end result is that it does not produce any effect (see further comments
>> below). Only thing that seems to play a role here is explictly setting
>> phydev->autoneg = AUTONEG_DISABLE.
>>
>>
>>> - set the PHY_HAS_MAGICANEG bit in your PHY driver flag
>>
>>
>> Again, this seems to play no role whatsoever here:
>>
>>                          } else if (0 == phydev->link_timeout--) {
>>                                  needs_aneg = 1;
>>                                  /* If we have the magic_aneg bit,
>>                                   * we try again */
>>                                  if (phydev->drv->flags & PHY_HAS_MAGICANEG)
>>                                          break;
>>                          }
>>                          break;
>>                  case PHY_NOLINK:
>>
>> This code might have made sense when it was written in 2006 -- back then,
>> the break statement was skipping some fallback code. But now it seems to do
>> nothing.
>>
>>
>>>
>>>>
>>>> (instead of phydev->advertising):
>>>>    >> +               if (!of_property_read_u32(np, override->prop, &tmp))
>>>> {
>>>>    >> +                       if (tmp) {
>>>>    >> +                               *val |= override->value;
>>>>    >> +                               phydev->advertising |=
>>>>
>>>> override->supported;
>>>>
>>>>    >> +                       } else {
>>>>    >> +                               phydev->advertising &=
>>>>
>>>> ~(override->supported);
>>>>
>>>>    >> +                       }
>>>>    >> +
>>>>    >> +                       *mask |= override->value;
>>>>
>>>> What I find weird is that the only way phydev->autoneg could ever be set
>>>> to disabled is from here (phy.c):
>>>>
>>>> static void phy_sanitize_settings(struct phy_device *phydev)
>>>> {
>>>>          u32 features = phydev->supported;
>>>>          int idx;
>>>>
>>>>          /* Sanitize settings based on PHY capabilities */
>>>>          if ((features & SUPPORTED_Autoneg) == 0)
>>>>                  phydev->autoneg = AUTONEG_DISABLE;
>>>>
>>>> which is in turn only called when phydev->autoneg is set to
>>>> AUTONEG_DISABLE to begin with:
>>>>
>>>> int phy_start_aneg(struct phy_device *phydev)
>>>> {
>>>>          int err;
>>>>
>>>>          mutex_lock(&phydev->lock);
>>>>
>>>>          if (AUTONEG_DISABLE == phydev->autoneg)
>>>>                  phy_sanitize_settings(phydev);
>>>>
>>>> So could someone please help me figure out what I'm missing here?
>>>
>>>
>>> At first glance it looks like the PHY driver should be reading the phydev-
>>>>
>>>> autoneg value when the PHY driver config_aneg() callback is called to be
>>>
>>> allowed to set the forced speed and settings.
>>>
>>> The way phy_sanitize_settings() is coded does not make it return a mask of
>>> features, but only the forced supported speed and duplex. Then when the
>>> link
>>> is forced but we are having some issues getting a link status, libphy
>>> tries
>>> lower speeds and this function is used again to provide the next
>>> speed/duplex
>>> pair to try.
>>>
>>
>> What I was trying to say is that phy_sanitize_settings() is only called when
>> phydev->autoneg == AUTONEG_DISABLE, and in turn it's the only generic
>> function setting phydev->autoneg = AUTONEG_DISABLE.
>> So perhaps the condition should read:
>>
>> -       if (AUTONEG_DISABLE == phydev->autoneg)
>> +       if ((features & SUPPORTED_Autoneg) == 0)
>>                  phy_sanitize_settings(phydev);
>>
>> Or else, some other parts of the generic code should take care of setting it
>> to AUTONEG_DISABLE, depending on whether the feature is supported or not.
>> What I found weird is explicitly setting a value (phydev->autoneg =
>> AUTONEG_DISABLE), from a static function which is only called when that
>> condition is already true.
>
> I do not think that this change is correct either, let me cook a patch
> for you to allow disabling autoneg from the start.

Oh, OK, that would be great, thank you!
FWIW, I've already spent quite some time trying to overcome this -- my 
understanding is that you somehow need to set phydev->autoneg to 
AUTONEG_DISABLE at a very early stage (and that could of course be done 
as a consequence of SUPPORTED_Autoneg being unset), otherwise the whole 
software phy state machine and speed-matching algorithms will get confused.

>>
>> BTW, I feel like disabling autoneg from the start has never been a use case
>> before, am I right?
>
> Not really no, and that is because most hardware does not need quirks
> to work correctly.

To be honest with you, I'm not long experienced on MII/PHY, but I've 
already seen two completely unrelated cases where autoneg needs to be 
disabled in order for the hardware to work correctly. Of course I'm only 
talking about in-board connections (e.g. not PHYs connected to an RJ-45 
jack), still...
In this particular hardware configuration, not only does autoneg need to 
be disabled in the first place (otherwise link won't work at all), but 
the phy HW is also buggy so that when autoneg is disabled, it may still 
occasionally not work (like 0.1% of the times).

Thank you so much!
Gerlando

Re: [PATCH V3] net/dt: Add support for overriding phy configuration from device tree

[Gerlando Falauto]

Hi Matthew,

On 01/17/2014 11:57 PM, Matthew Garrett wrote:
> Some hardware may be broken in interesting and board-specific ways, such
> that various bits of functionality don't work. This patch provides a
> mechanism for overriding mii registers during init based on the contents of
> the device tree data, allowing board-specific fixups without having to
> pollute generic code.

[six months later]
I assume nothing ever came out of this patch... is that right?

Thank you,
Gerlando

>
> Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>
> ---
>
> V3: Break the main function out into some helper functions and store the
> values in some structs.
>
>   Documentation/devicetree/bindings/net/phy.txt | 21 +++++++
>   drivers/net/phy/phy_device.c                  | 29 ++++++++-
>   drivers/of/of_net.c                           | 87 +++++++++++++++++++++++++++
>   include/linux/of_net.h                        | 12 ++++
>   4 files changed, 148 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/net/phy.txt b/Documentation/devicetree/bindings/net/phy.txt
> index 7cd18fb..fc50f02 100644
> --- a/Documentation/devicetree/bindings/net/phy.txt
> +++ b/Documentation/devicetree/bindings/net/phy.txt
> @@ -23,6 +23,21 @@ Optional Properties:
>     assume clause 22. The compatible list may also contain other
>     elements.
>
> +The following properties may be added to either the phy node or the parent
> +ethernet device. If not present, the hardware defaults will be used.
> +
> +- phy-mii-advertise-10half: 1 to advertise half-duplex 10MBit, 0 to disable
> +- phy-mii-advertise-10full: 1 to advertise full-duplex 10MBit, 0 to disable
> +- phy-mii-advertise-100half: 1 to advertise half-duplex 100MBit, 0 to disable
> +- phy-mii-advertise-100full: 1 to advertise full-duplex 100MBit, 0 to disable
> +- phy-mii-advertise-100base4: 1 to advertise 100base4, 0 to disable
> +- phy-mii-advertise-1000half: 1 to advertise half-duplex 1000MBit, 0 to disable
> +- phy-mii-advertise-1000full: 1 to advertise full-duplex 1000MBit, 0 to disable
> +- phy-mii-manual-master: 1 to enable manual master/slave configuration, 0
> +  to disable manual master/slave configuration
> +- phy-mii-as-master: 1 to configure phy to act as master, 0 to configure phy
> +  to act as slave. Ignored if manual master/slave configuration is not enabled.
> +
>   Example:
>
>   ethernet-phy@0 {
> @@ -32,4 +47,10 @@ ethernet-phy@0 {
>   	interrupts = <35 1>;
>   	reg = <0>;
>   	device_type = "ethernet-phy";
> +	// Disable advertising of full duplex 1000MBit
> +	phy-mii-advertise-1000full = <0>;
> +	// Force manual phy master/slave configuration
> +	phy-mii-manual-master = <1>;
> +	// Forcibly configure phy as slave
> +	phy-mii-as-master = <0>;
>   };
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index d6447b3..91793bc 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -33,6 +33,7 @@
>   #include <linux/mii.h>
>   #include <linux/ethtool.h>
>   #include <linux/phy.h>
> +#include <linux/of_net.h>
>
>   #include <asm/io.h>
>   #include <asm/irq.h>
> @@ -497,6 +498,28 @@ void phy_disconnect(struct phy_device *phydev)
>   }
>   EXPORT_SYMBOL(phy_disconnect);
>
> +int phy_override_from_of(struct phy_device *phydev)
> +{
> +	int reg, regval;
> +	u16 val, mask;
> +
> +	/* Check for phy register overrides from OF */
> +	for (reg = 0; reg < 16; reg++) {
> +		if (!of_get_mii_register(phydev, reg, &val, &mask)) {
> +			if (!mask)
> +				continue;
> +			regval = phy_read(phydev, reg);
> +			if (regval < 0)
> +				continue;
> +			regval &= ~mask;
> +			regval |= val;
> +			phy_write(phydev, reg, regval);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>   int phy_init_hw(struct phy_device *phydev)
>   {
>   	int ret;
> @@ -508,7 +531,11 @@ int phy_init_hw(struct phy_device *phydev)
>   	if (ret < 0)
>   		return ret;
>
> -	return phydev->drv->config_init(phydev);
> +	ret = phydev->drv->config_init(phydev);
> +	if (ret < 0)
> +		return ret;
> +
> +	return phy_override_from_of(phydev);
>   }
>
>   /**
> diff --git a/drivers/of/of_net.c b/drivers/of/of_net.c
> index 8f9be2e..75751b7 100644
> --- a/drivers/of/of_net.c
> +++ b/drivers/of/of_net.c
> @@ -93,3 +93,90 @@ const void *of_get_mac_address(struct device_node *np)
>   	return NULL;
>   }
>   EXPORT_SYMBOL(of_get_mac_address);
> +
> +struct mii_override {
> +	char *prop;
> +	u32 supported;
> +	u16 value;
> +};
> +
> +static struct mii_override mii_advertise_override[] = {
> +	{ "phy-mii-advertise-10half", SUPPORTED_10baseT_Half,
> +	  ADVERTISE_10HALF },
> +	{ "phy-mii-advertise-10full", SUPPORTED_10baseT_Full,
> +	  ADVERTISE_10FULL },
> +	{ "phy-mii-advertise-100half", SUPPORTED_100baseT_Half,
> +	  ADVERTISE_100HALF },
> +	{ "phy-mii-advertise-100full", SUPPORTED_100baseT_Full,
> +	  ADVERTISE_100FULL },
> +	{ "phy-mii-advertise-100base4", 0, ADVERTISE_100BASE4 },
> +	{ NULL },
> +};
> +
> +static struct mii_override mii_gigabit_override[] = {
> +	{ "phy-mii-advertise-1000half", SUPPORTED_1000baseT_Half,
> +	  ADVERTISE_1000HALF },
> +	{ "phy-mii-advertise-1000full", SUPPORTED_1000baseT_Full,
> +	  ADVERTISE_1000FULL },
> +	{ "phy-mii-as-master", 0, CTL1000_AS_MASTER },
> +	{ "phy-mii-manual-master", 0, CTL1000_ENABLE_MASTER },
> +	{ NULL },
> +};
> +
> +static void mii_handle_override(struct mii_override *override_list,
> +				struct phy_device *phydev, u16 *val, u16 *mask)
> +{
> +	struct device *dev = &phydev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct mii_override *override;
> +	u32 tmp;
> +
> +	if (!np && dev->parent->of_node)
> +		np = dev->parent->of_node;
> +
> +	if (!np)
> +		return;
> +
> +	for (override = &override_list[0]; override->prop != NULL; override++) {
> +		if (!of_property_read_u32(np, override->prop, &tmp)) {
> +			if (tmp) {
> +				*val |= override->value;
> +				phydev->advertising |= override->supported;
> +			} else {
> +				phydev->advertising &= ~(override->supported);
> +			}
> +
> +			*mask |= override->value;
> +		}
> +	}
> +
> +	return;
> +}
> +
> +/**
> + * Provide phy register overrides from the device tree. Some hardware may
> + * be broken in interesting and board-specific ways, so we want a mechanism
> + * for the board data to provide overrides for default values. This should be
> + * called during phy init.
> + */
> +int of_get_mii_register(struct phy_device *phydev, int reg, u16 *val,
> +			u16 *mask)
> +{
> +	*val = 0;
> +	*mask = 0;
> +
> +	switch (reg) {
> +	case MII_ADVERTISE:
> +		mii_handle_override(mii_advertise_override, phydev, val, mask);
> +		break;
> +
> +	case MII_CTRL1000:
> +		mii_handle_override(mii_gigabit_override, phydev, val, mask);
> +		break;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +EXPORT_SYMBOL(of_get_mii_register);
> diff --git a/include/linux/of_net.h b/include/linux/of_net.h
> index 34597c8..2e478bc 100644
> --- a/include/linux/of_net.h
> +++ b/include/linux/of_net.h
> @@ -7,10 +7,14 @@
>   #ifndef __LINUX_OF_NET_H
>   #define __LINUX_OF_NET_H
>
> +#include <linux/phy.h>
> +
>   #ifdef CONFIG_OF_NET
>   #include <linux/of.h>
>   extern int of_get_phy_mode(struct device_node *np);
>   extern const void *of_get_mac_address(struct device_node *np);
> +extern int of_get_mii_register(struct phy_device *np, int reg, u16 *val,
> +			       u16 *mask);
>   #else
>   static inline int of_get_phy_mode(struct device_node *np)
>   {
> @@ -21,6 +25,14 @@ static inline const void *of_get_mac_address(struct device_node *np)
>   {
>   	return NULL;
>   }
> +static inline int of_get_mii_register(struct phy_device *np, int reg, u16 *val,
> +				      u16 *mask)
> +{
> +	*val = 0;
> +	*mask = 0;
> +
> +	return -EINVAL;
> +}
>   #endif
>
>   #endif /* __LINUX_OF_NET_H */
>

Re: [PATCH V3] net/dt: Add support for overriding phy configuration from device tree

[Florian Fainelli]

2014-07-10 5:37 GMT-07:00 Gerlando Falauto <gerlando.falauto-SkAbAL50j+5BDgjK7y7TUQ@public.gmane.org>:
> Hi Matthew,
>
>
> On 01/17/2014 11:57 PM, Matthew Garrett wrote:
>>
>> Some hardware may be broken in interesting and board-specific ways, such
>> that various bits of functionality don't work. This patch provides a
>> mechanism for overriding mii registers during init based on the contents
>> of
>> the device tree data, allowing board-specific fixups without having to
>> pollute generic code.
>
>
> [six months later]
> I assume nothing ever came out of this patch... is that right?

I have not seen any update, and if it does get implemented at some
point, we'll want separate Device Tree properties which control
invididual or logical groups of bits within a standard MII registers,
not a placeholder for register addresses = value pairs.

>
> Thank you,
> Gerlando
>
>
>>
>> Signed-off-by: Matthew Garrett <matthew.garrett-05XSO3Yj/JvQT0dZR+AlfA@public.gmane.org>
>> ---
>>
>> V3: Break the main function out into some helper functions and store the
>> values in some structs.
>>
>>   Documentation/devicetree/bindings/net/phy.txt | 21 +++++++
>>   drivers/net/phy/phy_device.c                  | 29 ++++++++-
>>   drivers/of/of_net.c                           | 87
>> +++++++++++++++++++++++++++
>>   include/linux/of_net.h                        | 12 ++++
>>   4 files changed, 148 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/net/phy.txt
>> b/Documentation/devicetree/bindings/net/phy.txt
>> index 7cd18fb..fc50f02 100644
>> --- a/Documentation/devicetree/bindings/net/phy.txt
>> +++ b/Documentation/devicetree/bindings/net/phy.txt
>> @@ -23,6 +23,21 @@ Optional Properties:
>>     assume clause 22. The compatible list may also contain other
>>     elements.
>>
>> +The following properties may be added to either the phy node or the
>> parent
>> +ethernet device. If not present, the hardware defaults will be used.
>> +
>> +- phy-mii-advertise-10half: 1 to advertise half-duplex 10MBit, 0 to
>> disable
>> +- phy-mii-advertise-10full: 1 to advertise full-duplex 10MBit, 0 to
>> disable
>> +- phy-mii-advertise-100half: 1 to advertise half-duplex 100MBit, 0 to
>> disable
>> +- phy-mii-advertise-100full: 1 to advertise full-duplex 100MBit, 0 to
>> disable
>> +- phy-mii-advertise-100base4: 1 to advertise 100base4, 0 to disable
>> +- phy-mii-advertise-1000half: 1 to advertise half-duplex 1000MBit, 0 to
>> disable
>> +- phy-mii-advertise-1000full: 1 to advertise full-duplex 1000MBit, 0 to
>> disable
>> +- phy-mii-manual-master: 1 to enable manual master/slave configuration, 0
>> +  to disable manual master/slave configuration
>> +- phy-mii-as-master: 1 to configure phy to act as master, 0 to configure
>> phy
>> +  to act as slave. Ignored if manual master/slave configuration is not
>> enabled.
>> +
>>   Example:
>>
>>   ethernet-phy@0 {
>> @@ -32,4 +47,10 @@ ethernet-phy@0 {
>>         interrupts = <35 1>;
>>         reg = <0>;
>>         device_type = "ethernet-phy";
>> +       // Disable advertising of full duplex 1000MBit
>> +       phy-mii-advertise-1000full = <0>;
>> +       // Force manual phy master/slave configuration
>> +       phy-mii-manual-master = <1>;
>> +       // Forcibly configure phy as slave
>> +       phy-mii-as-master = <0>;
>>   };
>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>> index d6447b3..91793bc 100644
>> --- a/drivers/net/phy/phy_device.c
>> +++ b/drivers/net/phy/phy_device.c
>> @@ -33,6 +33,7 @@
>>   #include <linux/mii.h>
>>   #include <linux/ethtool.h>
>>   #include <linux/phy.h>
>> +#include <linux/of_net.h>
>>
>>   #include <asm/io.h>
>>   #include <asm/irq.h>
>> @@ -497,6 +498,28 @@ void phy_disconnect(struct phy_device *phydev)
>>   }
>>   EXPORT_SYMBOL(phy_disconnect);
>>
>> +int phy_override_from_of(struct phy_device *phydev)
>> +{
>> +       int reg, regval;
>> +       u16 val, mask;
>> +
>> +       /* Check for phy register overrides from OF */
>> +       for (reg = 0; reg < 16; reg++) {
>> +               if (!of_get_mii_register(phydev, reg, &val, &mask)) {
>> +                       if (!mask)
>> +                               continue;
>> +                       regval = phy_read(phydev, reg);
>> +                       if (regval < 0)
>> +                               continue;
>> +                       regval &= ~mask;
>> +                       regval |= val;
>> +                       phy_write(phydev, reg, regval);
>> +               }
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>>   int phy_init_hw(struct phy_device *phydev)
>>   {
>>         int ret;
>> @@ -508,7 +531,11 @@ int phy_init_hw(struct phy_device *phydev)
>>         if (ret < 0)
>>                 return ret;
>>
>> -       return phydev->drv->config_init(phydev);
>> +       ret = phydev->drv->config_init(phydev);
>> +       if (ret < 0)
>> +               return ret;
>> +
>> +       return phy_override_from_of(phydev);
>>   }
>>
>>   /**
>> diff --git a/drivers/of/of_net.c b/drivers/of/of_net.c
>> index 8f9be2e..75751b7 100644
>> --- a/drivers/of/of_net.c
>> +++ b/drivers/of/of_net.c
>> @@ -93,3 +93,90 @@ const void *of_get_mac_address(struct device_node *np)
>>         return NULL;
>>   }
>>   EXPORT_SYMBOL(of_get_mac_address);
>> +
>> +struct mii_override {
>> +       char *prop;
>> +       u32 supported;
>> +       u16 value;
>> +};
>> +
>> +static struct mii_override mii_advertise_override[] = {
>> +       { "phy-mii-advertise-10half", SUPPORTED_10baseT_Half,
>> +         ADVERTISE_10HALF },
>> +       { "phy-mii-advertise-10full", SUPPORTED_10baseT_Full,
>> +         ADVERTISE_10FULL },
>> +       { "phy-mii-advertise-100half", SUPPORTED_100baseT_Half,
>> +         ADVERTISE_100HALF },
>> +       { "phy-mii-advertise-100full", SUPPORTED_100baseT_Full,
>> +         ADVERTISE_100FULL },
>> +       { "phy-mii-advertise-100base4", 0, ADVERTISE_100BASE4 },
>> +       { NULL },
>> +};
>> +
>> +static struct mii_override mii_gigabit_override[] = {
>> +       { "phy-mii-advertise-1000half", SUPPORTED_1000baseT_Half,
>> +         ADVERTISE_1000HALF },
>> +       { "phy-mii-advertise-1000full", SUPPORTED_1000baseT_Full,
>> +         ADVERTISE_1000FULL },
>> +       { "phy-mii-as-master", 0, CTL1000_AS_MASTER },
>> +       { "phy-mii-manual-master", 0, CTL1000_ENABLE_MASTER },
>> +       { NULL },
>> +};
>> +
>> +static void mii_handle_override(struct mii_override *override_list,
>> +                               struct phy_device *phydev, u16 *val, u16
>> *mask)
>> +{
>> +       struct device *dev = &phydev->dev;
>> +       struct device_node *np = dev->of_node;
>> +       struct mii_override *override;
>> +       u32 tmp;
>> +
>> +       if (!np && dev->parent->of_node)
>> +               np = dev->parent->of_node;
>> +
>> +       if (!np)
>> +               return;
>> +
>> +       for (override = &override_list[0]; override->prop != NULL;
>> override++) {
>> +               if (!of_property_read_u32(np, override->prop, &tmp)) {
>> +                       if (tmp) {
>> +                               *val |= override->value;
>> +                               phydev->advertising |=
>> override->supported;
>> +                       } else {
>> +                               phydev->advertising &=
>> ~(override->supported);
>> +                       }
>> +
>> +                       *mask |= override->value;
>> +               }
>> +       }
>> +
>> +       return;
>> +}
>> +
>> +/**
>> + * Provide phy register overrides from the device tree. Some hardware may
>> + * be broken in interesting and board-specific ways, so we want a
>> mechanism
>> + * for the board data to provide overrides for default values. This
>> should be
>> + * called during phy init.
>> + */
>> +int of_get_mii_register(struct phy_device *phydev, int reg, u16 *val,
>> +                       u16 *mask)
>> +{
>> +       *val = 0;
>> +       *mask = 0;
>> +
>> +       switch (reg) {
>> +       case MII_ADVERTISE:
>> +               mii_handle_override(mii_advertise_override, phydev, val,
>> mask);
>> +               break;
>> +
>> +       case MII_CTRL1000:
>> +               mii_handle_override(mii_gigabit_override, phydev, val,
>> mask);
>> +               break;
>> +
>> +       default:
>> +               return -EINVAL;
>> +       }
>> +       return 0;
>> +}
>> +EXPORT_SYMBOL(of_get_mii_register);
>> diff --git a/include/linux/of_net.h b/include/linux/of_net.h
>> index 34597c8..2e478bc 100644
>> --- a/include/linux/of_net.h
>> +++ b/include/linux/of_net.h
>> @@ -7,10 +7,14 @@
>>   #ifndef __LINUX_OF_NET_H
>>   #define __LINUX_OF_NET_H
>>
>> +#include <linux/phy.h>
>> +
>>   #ifdef CONFIG_OF_NET
>>   #include <linux/of.h>
>>   extern int of_get_phy_mode(struct device_node *np);
>>   extern const void *of_get_mac_address(struct device_node *np);
>> +extern int of_get_mii_register(struct phy_device *np, int reg, u16 *val,
>> +                              u16 *mask);
>>   #else
>>   static inline int of_get_phy_mode(struct device_node *np)
>>   {
>> @@ -21,6 +25,14 @@ static inline const void *of_get_mac_address(struct
>> device_node *np)
>>   {
>>         return NULL;
>>   }
>> +static inline int of_get_mii_register(struct phy_device *np, int reg, u16
>> *val,
>> +                                     u16 *mask)
>> +{
>> +       *val = 0;
>> +       *mask = 0;
>> +
>> +       return -EINVAL;
>> +}
>>   #endif
>>
>>   #endif /* __LINUX_OF_NET_H */
>>
>



-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html