[PATCH 0/3] net: phy: initialize PHYs via device tree properties

[PATCH 0/3] net: phy: initialize PHYs via device tree properties

[Michael Walle]

I was trying to configure the Atheros PHY for my board. There are fixups
all over the place, for example to enable the 125MHz clock output in almost
any i.MX architecture. Instead of adding another fixup in architecture
specific code, try to provide a generic way to init the PHY registers.

This patch series tries to pick up the "broadcom,reg-init" and
"marvell,reg-init" device tree properties idea and make it a more generic
"reg-init" which is handled by phy_device instead of a particular phy
driver.

Michael Walle (3):
  dt-bindings: net: phy: Add reg-init property
  net: phy: export __phy_{read|write}_page
  net: phy: Use device tree properties to initialize any PHYs

 .../devicetree/bindings/net/ethernet-phy.yaml | 31 ++++++
 MAINTAINERS                                   |  1 +
 drivers/net/phy/phy-core.c                    | 24 ++++-
 drivers/net/phy/phy_device.c                  | 97 ++++++++++++++++++-
 include/dt-bindings/net/phy.h                 | 18 ++++
 include/linux/phy.h                           |  2 +
 6 files changed, 170 insertions(+), 3 deletions(-)
 create mode 100644 include/dt-bindings/net/phy.h

Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Heiner Kallweit <hkallweit1@gmail.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>

-- 
2.20.1

[PATCH 1/3] dt-bindings: net: phy: Add reg-init property

[Michael Walle]

Describe the reg-init property to configure PHY registers.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 .../devicetree/bindings/net/ethernet-phy.yaml | 31 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 include/dt-bindings/net/phy.h                 | 18 +++++++++++
 3 files changed, 50 insertions(+)
 create mode 100644 include/dt-bindings/net/phy.h

diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
index f70f18ff821f..d2dda1f33119 100644
--- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml
+++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
@@ -153,6 +153,28 @@ properties:
       Delay after the reset was deasserted in microseconds. If
       this property is missing the delay will be skipped.
 
+  reg-init:
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/uint32-matrix
+        items:
+          items:
+            - description:
+                Set this to zero to write clause-22 register.
+                Set the page ORed with PHY_REG_PAGE to write to
+                a paged register. Set to devad ORed with
+                PHY_REG_C45 to write a clause-45 register.
+            - description:
+                The PHY register.
+            - description:
+                Mask, if non-zero, ANDed with existing register
+                value.
+            - description:
+                Value, ORed with the masked value and written to
+                the register.
+    description:
+      A list of <page_or_devad reg mask value> tuples to configure
+      the PHY registers at startup.
+
 required:
   - reg
 
@@ -173,5 +195,14 @@ examples:
             reset-gpios = <&gpio1 4 1>;
             reset-assert-us = <1000>;
             reset-deassert-us = <2000>;
+
+            reg-init =
+                /* Fix RX and TX clock transition timing */
+                <(PHY_REG_PAGE | 2) 0x15 0xffcf 0>, /* Reg 2,21 Clear bits 4, 5 */
+                /* Adjust LED drive. */
+                <(PHY_REG_PAGE | 3) 0x11 0 0x442a>, /* Reg 3,17 <- 0442a */
+                /* irq, blink-activity, blink-link */
+                <(PHY_REG_PAGE | 3) 0x10 0 0x0242>; /* Reg 3,16 <- 0x0242 */
+
         };
     };
diff --git a/MAINTAINERS b/MAINTAINERS
index a69e6db80c79..493ea5e13c2c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6147,6 +6147,7 @@ F:	Documentation/networking/phy.rst
 F:	drivers/net/phy/
 F:	drivers/of/of_mdio.c
 F:	drivers/of/of_net.c
+F:	include/dt-bindings/net/phy.h
 F:	include/linux/*mdio*.h
 F:	include/linux/of_net.h
 F:	include/linux/phy.h
diff --git a/include/dt-bindings/net/phy.h b/include/dt-bindings/net/phy.h
new file mode 100644
index 000000000000..b37853144719
--- /dev/null
+++ b/include/dt-bindings/net/phy.h
@@ -0,0 +1,18 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Device Tree constants for a generic network PHY
+ *
+ * Author: Michael Walle <michael@walle.cc>
+ *
+ */
+
+#ifndef _DT_BINDINGS_NET_PHY_H
+#define _DT_BINDINGS_NET_PHY_H
+
+/* PHY write selection bits */
+
+#define PHY_REG_C22	(0)
+#define PHY_REG_PAGE	(1 << 31)
+#define PHY_REG_C45	(1 << 30)
+
+#endif /* _DT_BINDINGS_NET_PHY_H */
-- 
2.20.1

[PATCH 2/3] net: phy: export __phy_{read|write}_page

[Michael Walle]

Also check if the op is actually available. Otherwise return -ENOTSUPP.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/net/phy/phy-core.c | 24 ++++++++++++++++++++++--
 include/linux/phy.h        |  2 ++
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
index 9412669b579c..70f93e405e91 100644
--- a/drivers/net/phy/phy-core.c
+++ b/drivers/net/phy/phy-core.c
@@ -687,15 +687,35 @@ int phy_modify_mmd(struct phy_device *phydev, int devad, u32 regnum,
 }
 EXPORT_SYMBOL_GPL(phy_modify_mmd);
 
-static int __phy_read_page(struct phy_device *phydev)
+/**
+ * __phy_read_page - get currently selected page
+ * @phydev: the phy_device struct
+ *
+ * Get the current PHY page. On error, returns a negative errno, otherwise
+ * returns the selected page number.
+ */
+int __phy_read_page(struct phy_device *phydev)
 {
+	if (!phydev->drv->read_page)
+		return -ENOTSUPP;
 	return phydev->drv->read_page(phydev);
 }
+EXPORT_SYMBOL_GPL(__phy_read_page);
 
-static int __phy_write_page(struct phy_device *phydev, int page)
+/**
+ * __phy_write_page - set the current page
+ * @phydev: the phy_device struct
+ * @page: desired page
+ *
+ * Set the current PHY page. On error, returns a negative errno.
+ */
+int __phy_write_page(struct phy_device *phydev, int page)
 {
+	if (!phydev->drv->write_page)
+		return -ENOTSUPP;
 	return phydev->drv->write_page(phydev, page);
 }
+EXPORT_SYMBOL_GPL(__phy_write_page);
 
 /**
  * phy_save_page() - take the bus lock and save the current page
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 9a0e981df502..70eca3cb25ff 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -797,6 +797,8 @@ int __phy_modify_mmd(struct phy_device *phydev, int devad, u32 regnum,
 		     u16 mask, u16 set);
 int phy_modify_mmd(struct phy_device *phydev, int devad, u32 regnum,
 		   u16 mask, u16 set);
+int __phy_read_page(struct phy_device *phydev);
+int __phy_write_page(struct phy_device *phydev, int page);
 
 /**
  * __phy_set_bits - Convenience function for setting bits in a PHY register
-- 
2.20.1

[PATCH 3/3] net: phy: Use device tree properties to initialize any PHYs

[Michael Walle]

Some PHYs drivers, like the marvell and the broadcom one, are able to
initialize PHY registers via device tree properties. This patch adds a
more generic property which applies to any PHY. It supports clause-22,
clause-45 and paged PHY writes.

Hopefully, some board maintainers will pick this up and switch to this
instead of adding more phy_fixups in architecture specific code,
although it is board specific. For example have a look at
arch/arm/mach-imx/ for phy_register_fixup.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/net/phy/phy_device.c | 97 +++++++++++++++++++++++++++++++++++-
 1 file changed, 96 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 9d2bbb13293e..3c4cbaf72c27 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -30,6 +30,9 @@
 #include <linux/mdio.h>
 #include <linux/io.h>
 #include <linux/uaccess.h>
+#include <linux/of.h>
+
+#include <dt-bindings/net/phy.h>
 
 MODULE_DESCRIPTION("PHY library");
 MODULE_AUTHOR("Andy Fleming");
@@ -1064,6 +1067,95 @@ static int phy_poll_reset(struct phy_device *phydev)
 	return 0;
 }
 
+/**
+ * phy_of_reg_init - Set and/or override configuration registers.
+ * @phydev: target phy_device struct
+ *
+ * Description: Set and/or override some configuration registers based
+ *   on the reg-init property stored in the of_node for the phydev.
+ *
+ *   reg-init = <dev reg mask value>,...;
+ *   There may be one or more sets of <dev reg mask value>:
+ */
+static int phy_of_reg_init(struct phy_device *phydev)
+{
+	struct device_node *node = phydev->mdio.dev.of_node;
+	int oldpage = -1, savedpage = -1;
+	const __be32 *paddr;
+	int len, i;
+	int ret;
+
+	if (!IS_ENABLED(CONFIG_OF_MDIO))
+		return 0;
+
+	if (!node)
+		return 0;
+
+	paddr = of_get_property(node, "reg-init", &len);
+	if (!paddr || len < (4 * sizeof(*paddr)))
+		return 0;
+
+	mutex_lock(&phydev->mdio.bus->mdio_lock);
+
+	savedpage = -1;
+	len /= sizeof(*paddr);
+	for (i = 0; i < len - 3; i += 4) {
+		u32 dev = be32_to_cpup(paddr + i);
+		u16 reg = be32_to_cpup(paddr + i + 1);
+		u16 mask = be32_to_cpup(paddr + i + 2);
+		u16 val_bits = be32_to_cpup(paddr + i + 3);
+		int page = dev & 0xffff;
+		int devad = dev & 0x1f;
+		int val;
+
+		if (dev & PHY_REG_PAGE) {
+			if (savedpage < 0) {
+				savedpage = __phy_read_page(phydev);
+				if (savedpage < 0) {
+					ret = savedpage;
+					goto err;
+				}
+				oldpage = savedpage;
+			}
+			if (oldpage != page) {
+				ret = __phy_write_page(phydev, page);
+				if (ret < 0)
+					goto err;
+				oldpage = page;
+			}
+		}
+
+		val = 0;
+		if (mask) {
+			if (dev & PHY_REG_C45)
+				val = __phy_read_mmd(phydev, devad, reg);
+			else
+				val = __phy_read(phydev, reg);
+			if (val < 0) {
+				ret = val;
+				goto err;
+			}
+			val &= mask;
+		}
+		val |= val_bits;
+
+		if (dev & PHY_REG_C45)
+			ret = __phy_write_mmd(phydev, devad, reg, val);
+		else
+			ret = __phy_write(phydev, reg, val);
+		if (ret < 0)
+			goto err;
+	}
+
+err:
+	if (savedpage >= 0)
+		__phy_write_page(phydev, savedpage);
+
+	mutex_unlock(&phydev->mdio.bus->mdio_lock);
+
+	return ret;
+}
+
 int phy_init_hw(struct phy_device *phydev)
 {
 	int ret = 0;
@@ -1087,7 +1179,10 @@ int phy_init_hw(struct phy_device *phydev)
 	if (phydev->drv->config_init)
 		ret = phydev->drv->config_init(phydev);
 
-	return ret;
+	if (ret < 0)
+		return ret;
+
+	return phy_of_reg_init(phydev);
 }
 EXPORT_SYMBOL(phy_init_hw);
 
-- 
2.20.1

Re: [PATCH 0/3] net: phy: initialize PHYs via device tree properties

[Florian Fainelli]

On 10/29/19 10:48 AM, Michael Walle wrote:
> I was trying to configure the Atheros PHY for my board. There are fixups
> all over the place, for example to enable the 125MHz clock output in almost
> any i.MX architecture. Instead of adding another fixup in architecture
> specific code, try to provide a generic way to init the PHY registers.
> 
> This patch series tries to pick up the "broadcom,reg-init" and
> "marvell,reg-init" device tree properties idea and make it a more generic
> "reg-init" which is handled by phy_device instead of a particular phy
> driver.

These two examples are actually quite bad and were symptomatic of a few
things at the time:

- rush to get a specific feature/device supported without thinking about
the big picture
- lack of appropriate review on the Device Tree bindings

Fortunately, the last item is now not happening anymore.

The problem with letting that approach go through is that the Device
Tree can now hold a configuration policy which is passed through as-is
from DT to the PHY device, this is bad on so many different levels,
starting with abstraction.

If all you need is to enable a particular clock, introduce device
specific properties that describe the hardware, and make the necessary
change to the local driver that needs to act on those. You can always
define a more generic scope property if you see a recurring pattern.

So just to be clear on the current approach: NACK.

> 
> Michael Walle (3):
>   dt-bindings: net: phy: Add reg-init property
>   net: phy: export __phy_{read|write}_page
>   net: phy: Use device tree properties to initialize any PHYs
> 
>  .../devicetree/bindings/net/ethernet-phy.yaml | 31 ++++++
>  MAINTAINERS                                   |  1 +
>  drivers/net/phy/phy-core.c                    | 24 ++++-
>  drivers/net/phy/phy_device.c                  | 97 ++++++++++++++++++-
>  include/dt-bindings/net/phy.h                 | 18 ++++
>  include/linux/phy.h                           |  2 +
>  6 files changed, 170 insertions(+), 3 deletions(-)
>  create mode 100644 include/dt-bindings/net/phy.h
> 
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: Heiner Kallweit <hkallweit1@gmail.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> 


-- 
Florian

Re: [PATCH 0/3] net: phy: initialize PHYs via device tree properties

[Andrew Lunn]

> So just to be clear on the current approach: NACK.

Agreed.

And the Marvell one has only been used to set LEDs, as far as i
know. I would definitely push back on using it for anything else.

      Andrew

Re: [PATCH 0/3] net: phy: initialize PHYs via device tree properties

[Michael Walle]

Am 29. Oktober 2019 18:59:07 MEZ schrieb Florian Fainelli <f.fainelli@gmail.com>:
>On 10/29/19 10:48 AM, Michael Walle wrote:
>> I was trying to configure the Atheros PHY for my board. There are
>fixups
>> all over the place, for example to enable the 125MHz clock output in
>almost
>> any i.MX architecture. Instead of adding another fixup in
>architecture
>> specific code, try to provide a generic way to init the PHY
>registers.
>> 
>> This patch series tries to pick up the "broadcom,reg-init" and
>> "marvell,reg-init" device tree properties idea and make it a more
>generic
>> "reg-init" which is handled by phy_device instead of a particular phy
>> driver.
>
>These two examples are actually quite bad and were symptomatic of a few
>things at the time:
>
>- rush to get a specific feature/device supported without thinking
>about
>the big picture
>- lack of appropriate review on the Device Tree bindings
>
>Fortunately, the last item is now not happening anymore.
>
>The problem with letting that approach go through is that the Device
>Tree can now hold a configuration policy which is passed through as-is
>from DT to the PHY device, this is bad on so many different levels,
>starting with abstraction.

I see.

>If all you need is to enable a particular clock, introduce device
>specific properties that describe the hardware, and make the necessary
>change to the local driver that needs to act on those. You can always
>define a more generic scope property if you see a recurring pattern.

Could you have a quick look at the following patch I made for u-boot, which adds a binding for the Atheros PHY. If that is the right direction. Yeah, I should have made it first to Linux to get some feedback on the binding :p

https://patchwork.ozlabs.org/patch/1184516/

I'd then prepare another patch for Linux based on your suggestions. 

-michael 

>
>So just to be clear on the current approach: NACK.
>
>> 
>> Michael Walle (3):
>>   dt-bindings: net: phy: Add reg-init property
>>   net: phy: export __phy_{read|write}_page
>>   net: phy: Use device tree properties to initialize any PHYs
>> 
>>  .../devicetree/bindings/net/ethernet-phy.yaml | 31 ++++++
>>  MAINTAINERS                                   |  1 +
>>  drivers/net/phy/phy-core.c                    | 24 ++++-
>>  drivers/net/phy/phy_device.c                  | 97
>++++++++++++++++++-
>>  include/dt-bindings/net/phy.h                 | 18 ++++
>>  include/linux/phy.h                           |  2 +
>>  6 files changed, 170 insertions(+), 3 deletions(-)
>>  create mode 100644 include/dt-bindings/net/phy.h
>> 
>> Cc: Andrew Lunn <andrew@lunn.ch>
>> Cc: Florian Fainelli <f.fainelli@gmail.com>
>> Cc: Heiner Kallweit <hkallweit1@gmail.com>
>> Cc: "David S. Miller" <davem@davemloft.net>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> 

Hi Florian,

Re: [PATCH 0/3] net: phy: initialize PHYs via device tree properties

[Florian Fainelli]

On 10/29/19 1:54 PM, Michael Walle wrote:
> Am 29. Oktober 2019 18:59:07 MEZ schrieb Florian Fainelli <f.fainelli@gmail.com>:
>> On 10/29/19 10:48 AM, Michael Walle wrote:
>>> I was trying to configure the Atheros PHY for my board. There are
>> fixups
>>> all over the place, for example to enable the 125MHz clock output in
>> almost
>>> any i.MX architecture. Instead of adding another fixup in
>> architecture
>>> specific code, try to provide a generic way to init the PHY
>> registers.
>>>
>>> This patch series tries to pick up the "broadcom,reg-init" and
>>> "marvell,reg-init" device tree properties idea and make it a more
>> generic
>>> "reg-init" which is handled by phy_device instead of a particular phy
>>> driver.
>>
>> These two examples are actually quite bad and were symptomatic of a few
>> things at the time:
>>
>> - rush to get a specific feature/device supported without thinking
>> about
>> the big picture
>> - lack of appropriate review on the Device Tree bindings
>>
>> Fortunately, the last item is now not happening anymore.
>>
>> The problem with letting that approach go through is that the Device
>> Tree can now hold a configuration policy which is passed through as-is
>>from DT to the PHY device, this is bad on so many different levels,
>> starting with abstraction.
> 
> I see.
> 
>> If all you need is to enable a particular clock, introduce device
>> specific properties that describe the hardware, and make the necessary
>> change to the local driver that needs to act on those. You can always
>> define a more generic scope property if you see a recurring pattern.
> 
> Could you have a quick look at the following patch I made for u-boot, which adds a binding for the Atheros PHY. If that is the right direction. Yeah, I should have made it first to Linux to get some feedback on the binding :p
> 
> https://patchwork.ozlabs.org/patch/1184516/
> 
> I'd then prepare another patch for Linux based on your suggestions. 

This looks like the right direction IMHO.
-- 
Florian

Re: [PATCH 0/3] net: phy: initialize PHYs via device tree properties

[Michael Walle]

Am 2019-10-29 21:59, schrieb Florian Fainelli:
>>> If all you need is to enable a particular clock, introduce device
>>> specific properties that describe the hardware, and make the 
>>> necessary
>>> change to the local driver that needs to act on those. You can always
>>> define a more generic scope property if you see a recurring pattern.
>> 
>> Could you have a quick look at the following patch I made for u-boot, 
>> which adds a binding for the Atheros PHY. If that is the right 
>> direction. Yeah, I should have made it first to Linux to get some 
>> feedback on the binding :p
>> 
>> https://patchwork.ozlabs.org/patch/1184516/
>> 
>> I'd then prepare another patch for Linux based on your suggestions.
> 
> This looks like the right direction IMHO.

Ok, one question though: There is a clock output, but it just supports 
four frequencies. Should there be a property like 
"atheros,clk-out-frequency = <25000000>" which can take an arbitrary 
number or should it be something like "atheros,clk-out-frequency = 
<AT803X_CLK_OUT_25_MHZ>" (the ti dp83867 uses the latter).

-michael