[PATCH v2 0/4 net-next] Improve MDIO Ethernet PHY reset

[PATCH v2 0/4 net-next] Improve MDIO Ethernet PHY reset

[Bruno Thomsen]

This patch series is a result of trying to upstream a new device
tree for a TQMa7D based board[1][2]. Initial this DTS used some
deprecated PHY reset properties on the FEC device; NXP Ethernet
MAC also known as Freescale Fast Ethernet Controller.

When switching from FEC properties[3]:
"phy-reset-gpios"
"phy-reset-duration"
"phy-reset-post-delay"

To MDIO PHY properties[4]:
"reset-gpios"
"reset-assert-us"
"reset-deassert-us"

The result was that no Ethernet PHY device was detected on boot.

This issue could be worked around by disabling PHY type ID auto-
detection by using "ethernet-phy-id0022.1560" as compatible
string and not "ethernet-phy-ieee802.3-c22".

Upstreaming a DTS with this workaround was not accepted, so I
digged into the MDIO reset flow and found that it had a few
missing parts compared to the deprecated FEC reset function.
After some more testing and logic analyzer traces it was
revealed that the failed PHY communication was due to missing
initial device reset.

I was suggested[5] in a earlier mail thread to use MDIO bus
reset as that was performed before auto-detection, but current
device tree binding was limited to reset assert in usec.
Microchip/Micrel Ethernet PHYs recommended reset circuit[8],
figure 7-12, is a little "slow" after reset deassert as that
is left to a RC circuit with a tau of ~100ms; using a 10k PU
resistor together with a 10uF decoupling capacitor. The diode
in serie of the reset signal converts the GPIO push-pull output
into a open-drain output. So a post reset delay in the range
of 500-1000ms is needed, depending on component tolerances
and general hardware design margins.

In the first version of this patch series[6] I reused the
"reset-delay-us" property for reset deassert in usec as that
would cause 50/50% duty-cycle, but that would always apply.
The solution in this patch series is to add a new MDIO bus
property, so post reset delay is optional and configured
separately.

MDIO bus properties[7]:
"reset-delay-us"
"reset-post-delay-us" (new)

I have not marked this with "Fixes:" as no single commit is the
cause and historically this code has only supported MDIO devices
that need reset after auto-detection. The patch series also uses
a new flexible sleep helper function that was introduced in
5.8-rc1, so the driver uses the optimal sleep function depending
on value loaded from device tree.

Future work in this area could add new properties on the MDIO
device, so reset points are configurable, e.g. no reset,
before/after auto-detection or both.

[1] https://lore.kernel.org/linux-devicetree/20200629114927.17379-2-bruno.thomsen@gmail.com/
[2] https://lore.kernel.org/linux-devicetree/20200716172611.5349-2-bruno.thomsen@gmail.com/
[3] https://elixir.bootlin.com/linux/v5.7.8/source/Documentation/devicetree/bindings/net/fsl-fec.txt#L44
[4] https://elixir.bootlin.com/linux/v5.8-rc4/source/Documentation/devicetree/bindings/net/mdio.yaml#L78
[5] https://lore.kernel.org/netdev/CAOMZO5DtYDomD8FDCZDwYCSr2AwNT81Ay4==aDxXyBxtyvPiJA@mail.gmail.com/
[6] https://lore.kernel.org/netdev/20200728090203.17313-1-bruno.thomsen@gmail.com/
[7] https://elixir.bootlin.com/linux/v5.8-rc4/source/Documentation/devicetree/bindings/net/mdio.yaml#L36
[8] http://ww1.microchip.com/downloads/en/DeviceDoc/00002202C.pdf

Bruno Thomsen (4):
  dt-bindings: net: mdio: add reset-post-delay-us property
  net: mdiobus: use flexible sleeping for reset-delay-us
  net: mdiobus: add reset-post-delay-us handling
  net: mdio device: use flexible sleeping in reset function

 Documentation/devicetree/bindings/net/mdio.yaml | 7 +++++++
 drivers/net/phy/mdio_bus.c                      | 4 +++-
 drivers/net/phy/mdio_device.c                   | 2 +-
 drivers/of/of_mdio.c                            | 2 ++
 include/linux/phy.h                             | 2 ++
 5 files changed, 15 insertions(+), 2 deletions(-)


base-commit: 490ed0b908d371cd9ab63fc142213e5d02d810ee
-- 
2.26.2

[PATCH v2 1/4 net-next] dt-bindings: net: mdio: add reset-post-delay-us property

[Bruno Thomsen]

Add "reset-post-delay-us" parameter to MDIO bus properties,
so it's possible to add a delay after reset deassert.
This is optional in case external hardware slows down
release of the reset signal.

Signed-off-by: Bruno Thomsen <bruno.thomsen@gmail.com>
---
 Documentation/devicetree/bindings/net/mdio.yaml | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/mdio.yaml b/Documentation/devicetree/bindings/net/mdio.yaml
index d6a3bf8550eb..26afb556dfae 100644
--- a/Documentation/devicetree/bindings/net/mdio.yaml
+++ b/Documentation/devicetree/bindings/net/mdio.yaml
@@ -39,6 +39,13 @@ properties:
       and must therefore be appropriately determined based on all devices
       requirements (maximum value of all per-device RESET pulse widths).
 
+  reset-post-delay-us:
+    description:
+      Delay after reset deassert in microseconds. It applies to all MDIO
+      devices and it's determined by how fast all devices are ready for
+      communication. This delay happens just before e.g. Ethernet PHY
+      type ID auto detection.
+
   clock-frequency:
     description:
       Desired MDIO bus clock frequency in Hz. Values greater than IEEE 802.3
-- 
2.26.2

Re: [PATCH v2 1/4 net-next] dt-bindings: net: mdio: add reset-post-delay-us property

[Andrew Lunn]

On Thu, Jul 30, 2020 at 09:57:46PM +0200, Bruno Thomsen wrote:
> Add "reset-post-delay-us" parameter to MDIO bus properties,
> so it's possible to add a delay after reset deassert.
> This is optional in case external hardware slows down
> release of the reset signal.
> 
> Signed-off-by: Bruno Thomsen <bruno.thomsen@gmail.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v2 1/4 net-next] dt-bindings: net: mdio: add reset-post-delay-us property

[Florian Fainelli]

On 7/30/2020 12:57 PM, Bruno Thomsen wrote:
> Add "reset-post-delay-us" parameter to MDIO bus properties,
> so it's possible to add a delay after reset deassert.
> This is optional in case external hardware slows down
> release of the reset signal.
> 
> Signed-off-by: Bruno Thomsen <bruno.thomsen@gmail.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

> ---
>  Documentation/devicetree/bindings/net/mdio.yaml | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/mdio.yaml b/Documentation/devicetree/bindings/net/mdio.yaml
> index d6a3bf8550eb..26afb556dfae 100644
> --- a/Documentation/devicetree/bindings/net/mdio.yaml
> +++ b/Documentation/devicetree/bindings/net/mdio.yaml
> @@ -39,6 +39,13 @@ properties:
>        and must therefore be appropriately determined based on all devices
>        requirements (maximum value of all per-device RESET pulse widths).
>  
> +  reset-post-delay-us:
> +    description:
> +      Delay after reset deassert in microseconds. It applies to all MDIO
> +      devices and it's determined by how fast all devices are ready for

Uber nit: it is should be spelled out, but that does not warrant a resend.
-- 
Florian

[PATCH v2 2/4 net-next] net: mdiobus: use flexible sleeping for reset-delay-us

[Bruno Thomsen]

MDIO bus reset pulse width is created by using udelay()
and that function might not be optimal depending on
device tree value. By switching to the new fsleep() helper
the correct delay function is called depending on
delay length, e.g. udelay(), usleep_range() or msleep().

Signed-off-by: Bruno Thomsen <bruno.thomsen@gmail.com>
---
 drivers/net/phy/mdio_bus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 46b33701ad4b..5df3782b05b4 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -554,7 +554,7 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)
 		bus->reset_gpiod = gpiod;
 
 		gpiod_set_value_cansleep(gpiod, 1);
-		udelay(bus->reset_delay_us);
+		fsleep(bus->reset_delay_us);
 		gpiod_set_value_cansleep(gpiod, 0);
 	}
 
-- 
2.26.2

Re: [PATCH v2 2/4 net-next] net: mdiobus: use flexible sleeping for reset-delay-us

[Andrew Lunn]

On Thu, Jul 30, 2020 at 09:57:47PM +0200, Bruno Thomsen wrote:
> MDIO bus reset pulse width is created by using udelay()
> and that function might not be optimal depending on
> device tree value. By switching to the new fsleep() helper
> the correct delay function is called depending on
> delay length, e.g. udelay(), usleep_range() or msleep().
> 
> Signed-off-by: Bruno Thomsen <bruno.thomsen@gmail.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v2 2/4 net-next] net: mdiobus: use flexible sleeping for reset-delay-us

[Florian Fainelli]

On 7/30/2020 12:57 PM, Bruno Thomsen wrote:
> MDIO bus reset pulse width is created by using udelay()
> and that function might not be optimal depending on
> device tree value. By switching to the new fsleep() helper
> the correct delay function is called depending on
> delay length, e.g. udelay(), usleep_range() or msleep().
> 
> Signed-off-by: Bruno Thomsen <bruno.thomsen@gmail.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

[PATCH v2 3/4 net-next] net: mdiobus: add reset-post-delay-us handling

[Bruno Thomsen]

Load new "reset-post-delay-us" value from MDIO properties,
and if configured to a greater then zero delay do a
flexible sleeping delay after MDIO bus reset deassert.
This allows devices to exit reset state before start
bus communication.

Signed-off-by: Bruno Thomsen <bruno.thomsen@gmail.com>
---
 drivers/net/phy/mdio_bus.c | 2 ++
 drivers/of/of_mdio.c       | 2 ++
 include/linux/phy.h        | 2 ++
 3 files changed, 6 insertions(+)

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 5df3782b05b4..0af20faad69d 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -556,6 +556,8 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)
 		gpiod_set_value_cansleep(gpiod, 1);
 		fsleep(bus->reset_delay_us);
 		gpiod_set_value_cansleep(gpiod, 0);
+		if (bus->reset_post_delay_us > 0)
+			fsleep(bus->reset_post_delay_us);
 	}
 
 	if (bus->reset) {
diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index eb84507de28a..cb32d7ef4938 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -268,6 +268,8 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
 	/* Get bus level PHY reset GPIO details */
 	mdio->reset_delay_us = DEFAULT_GPIO_RESET_DELAY;
 	of_property_read_u32(np, "reset-delay-us", &mdio->reset_delay_us);
+	mdio->reset_post_delay_us = 0;
+	of_property_read_u32(np, "reset-post-delay-us", &mdio->reset_post_delay_us);
 
 	/* Register the MDIO bus */
 	rc = mdiobus_register(mdio);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 0403eb799913..3a09d2bf69ea 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -293,6 +293,8 @@ struct mii_bus {
 
 	/* GPIO reset pulse width in microseconds */
 	int reset_delay_us;
+	/* GPIO reset deassert delay in microseconds */
+	int reset_post_delay_us;
 	/* RESET GPIO descriptor pointer */
 	struct gpio_desc *reset_gpiod;
 
-- 
2.26.2

Re: [PATCH v2 3/4 net-next] net: mdiobus: add reset-post-delay-us handling

[Andrew Lunn]

On Thu, Jul 30, 2020 at 09:57:48PM +0200, Bruno Thomsen wrote:
> Load new "reset-post-delay-us" value from MDIO properties,
> and if configured to a greater then zero delay do a
> flexible sleeping delay after MDIO bus reset deassert.
> This allows devices to exit reset state before start
> bus communication.
> 
> Signed-off-by: Bruno Thomsen <bruno.thomsen@gmail.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v2 3/4 net-next] net: mdiobus: add reset-post-delay-us handling

[Florian Fainelli]

On 7/30/2020 12:57 PM, Bruno Thomsen wrote:
> Load new "reset-post-delay-us" value from MDIO properties,
> and if configured to a greater then zero delay do a
> flexible sleeping delay after MDIO bus reset deassert.
> This allows devices to exit reset state before start
> bus communication.
> 
> Signed-off-by: Bruno Thomsen <bruno.thomsen@gmail.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

[PATCH v2 4/4 net-next] net: mdio device: use flexible sleeping in reset function

[Bruno Thomsen]

MDIO device reset assert and deassert length was created by
usleep_range() but that does not ensure optimal handling of
all the different values from device tree properties.
By switching to the new flexible sleeping helper function,
fsleep(), the correct delay function is called depending on
delay length, e.g. udelay(), usleep_range() or msleep().

Signed-off-by: Bruno Thomsen <bruno.thomsen@gmail.com>
---
 drivers/net/phy/mdio_device.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/mdio_device.c b/drivers/net/phy/mdio_device.c
index 0f625a1b1644..0837319a52d7 100644
--- a/drivers/net/phy/mdio_device.c
+++ b/drivers/net/phy/mdio_device.c
@@ -132,7 +132,7 @@ void mdio_device_reset(struct mdio_device *mdiodev, int value)
 
 	d = value ? mdiodev->reset_assert_delay : mdiodev->reset_deassert_delay;
 	if (d)
-		usleep_range(d, d + max_t(unsigned int, d / 10, 100));
+		fsleep(d);
 }
 EXPORT_SYMBOL(mdio_device_reset);
 
-- 
2.26.2

Re: [PATCH v2 4/4 net-next] net: mdio device: use flexible sleeping in reset function

[Andrew Lunn]

On Thu, Jul 30, 2020 at 09:57:49PM +0200, Bruno Thomsen wrote:
> MDIO device reset assert and deassert length was created by
> usleep_range() but that does not ensure optimal handling of
> all the different values from device tree properties.
> By switching to the new flexible sleeping helper function,
> fsleep(), the correct delay function is called depending on
> delay length, e.g. udelay(), usleep_range() or msleep().
> 
> Signed-off-by: Bruno Thomsen <bruno.thomsen@gmail.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v2 4/4 net-next] net: mdio device: use flexible sleeping in reset function

[Florian Fainelli]

On 7/30/2020 12:57 PM, Bruno Thomsen wrote:
> MDIO device reset assert and deassert length was created by
> usleep_range() but that does not ensure optimal handling of
> all the different values from device tree properties.
> By switching to the new flexible sleeping helper function,
> fsleep(), the correct delay function is called depending on
> delay length, e.g. udelay(), usleep_range() or msleep().
> 
> Signed-off-by: Bruno Thomsen <bruno.thomsen@gmail.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH v2 0/4 net-next] Improve MDIO Ethernet PHY reset

[David Miller]

From: Bruno Thomsen <bruno.thomsen@gmail.com>
Date: Thu, 30 Jul 2020 21:57:45 +0200

> This patch series is a result of trying to upstream a new device
> tree for a TQMa7D based board[1][2]. Initial this DTS used some
> deprecated PHY reset properties on the FEC device; NXP Ethernet
> MAC also known as Freescale Fast Ethernet Controller.
> 
> When switching from FEC properties[3]:
> "phy-reset-gpios"
> "phy-reset-duration"
> "phy-reset-post-delay"
> 
> To MDIO PHY properties[4]:
> "reset-gpios"
> "reset-assert-us"
> "reset-deassert-us"
> 
> The result was that no Ethernet PHY device was detected on boot.
> 
> This issue could be worked around by disabling PHY type ID auto-
> detection by using "ethernet-phy-id0022.1560" as compatible
> string and not "ethernet-phy-ieee802.3-c22".
> 
> Upstreaming a DTS with this workaround was not accepted, so I
> digged into the MDIO reset flow and found that it had a few
> missing parts compared to the deprecated FEC reset function.
> After some more testing and logic analyzer traces it was
> revealed that the failed PHY communication was due to missing
> initial device reset.
 ...

Series applied, thank you.