[PATCH net-next v3 0/4] net: fec: fix refclk enable for SMSC LAN8710/20

[PATCH net-next v3 0/4] net: fec: fix refclk enable for SMSC LAN8710/20

[Richard Leitner]

From: Richard Leitner <richard.leitner@skidata.com>

This patch series fixes the use of the SMSC LAN8710/20 with a Freescale ETH
when the refclk is generated by the FSL.

This patch depends on the "phylib: Add device reset GPIO support" patch
submitted by Geert Uytterhoeven/Sergei Shtylyov, see:
	https://patchwork.kernel.org/patch/10090149/

Changes v3:
	- use phylib to hard-reset the PHY
	- implement reset delays in phylib
	- add new phylib API & flag (PHY_RST_AFTER_CLK_EN) to determine if a PHY
	  is affected

Changes v2:
	- simplify and fix fec_reset_phy function to support multiple calls
	- include: linux: phy: harmonize phy_id{,_mask} type
	- reset the phy instead of not turning the clock on and off
	  (which would have caused a power consumption regression)

Richard Leitner (4):
  phylib: Add device reset delay support
  phylib: add reset after clk enable support
  net: phy: smsc: LAN8710/20: add PHY_RST_AFTER_CLK_EN flag
  net: fec: add phy_reset_after_clk_enable() support

 Documentation/devicetree/bindings/net/phy.txt | 10 ++++++++++
 drivers/net/ethernet/freescale/fec_main.c     |  7 +++++++
 drivers/net/phy/mdio_device.c                 | 13 +++++++++++--
 drivers/net/phy/phy_device.c                  | 24 ++++++++++++++++++++++++
 drivers/net/phy/smsc.c                        |  2 +-
 drivers/of/of_mdio.c                          |  8 ++++++++
 include/linux/mdio.h                          |  2 ++
 include/linux/phy.h                           |  2 ++
 8 files changed, 65 insertions(+), 3 deletions(-)

-- 
2.11.0

[PATCH net-next v3 1/4] phylib: Add device reset delay support

[Richard Leitner]

From: Richard Leitner <richard.leitner@skidata.com>

Some PHYs need a minimum time after the reset gpio was asserted and/or
deasserted. To ensure we meet these timing requirements add two new
optional devicetree parameters for the phy: reset-delay-us and
reset-post-delay-us.

This patch depends on the "phylib: Add device reset GPIO support" patch
submitted by Geert Uytterhoeven/Sergei Shtylyov, see:
	https://patchwork.kernel.org/patch/10090149/

Signed-off-by: Richard Leitner <richard.leitner@skidata.com>
---
 Documentation/devicetree/bindings/net/phy.txt | 10 ++++++++++
 drivers/net/phy/mdio_device.c                 | 13 +++++++++++--
 drivers/of/of_mdio.c                          |  8 ++++++++
 include/linux/mdio.h                          |  2 ++
 4 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/phy.txt b/Documentation/devicetree/bindings/net/phy.txt
index c05479f5ac7c..72860ce7f610 100644
--- a/Documentation/devicetree/bindings/net/phy.txt
+++ b/Documentation/devicetree/bindings/net/phy.txt
@@ -55,6 +55,12 @@ Optional Properties:
 
 - reset-gpios: The GPIO phandle and specifier for the PHY reset signal.
 
+- reset-delay-us: Delay after the reset was asserted in microseconds.
+  If this property is missing the delay will be skipped.
+
+- reset-post-delay-us: Delay after the reset was deasserted in microseconds.
+  If this property is missing the delay will be skipped.
+
 Example:
 
 ethernet-phy@0 {
@@ -62,4 +68,8 @@ ethernet-phy@0 {
 	interrupt-parent = <&PIC>;
 	interrupts = <35 IRQ_TYPE_EDGE_RISING>;
 	reg = <0>;
+
+	reset-gpios = <&gpio1 4 GPIO_ACTIVE_LOW>;
+	reset-delay-us = <1000>;
+	reset-post-delay-us = <2000>;
 };
diff --git a/drivers/net/phy/mdio_device.c b/drivers/net/phy/mdio_device.c
index 75d97dd9fb28..ca3ff43f8ee8 100644
--- a/drivers/net/phy/mdio_device.c
+++ b/drivers/net/phy/mdio_device.c
@@ -24,6 +24,7 @@
 #include <linux/slab.h>
 #include <linux/string.h>
 #include <linux/unistd.h>
+#include <linux/delay.h>
 
 void mdio_device_free(struct mdio_device *mdiodev)
 {
@@ -118,8 +119,16 @@ EXPORT_SYMBOL(mdio_device_remove);
 
 void mdio_device_reset(struct mdio_device *mdiodev, int value)
 {
-	if (mdiodev->reset)
-		gpiod_set_value(mdiodev->reset, value);
+	if (!mdiodev->reset)
+		return;
+
+	gpiod_set_value(mdiodev->reset, value);
+
+	if (value && mdiodev->reset_delay)
+		usleep_range(mdiodev->reset_delay, mdiodev->reset_delay + 100);
+	else if (!value && mdiodev->reset_post_delay)
+		usleep_range(mdiodev->reset_post_delay,
+			     mdiodev->reset_post_delay + 100);
 }
 EXPORT_SYMBOL(mdio_device_reset);
 
diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index 98258583abb0..fb56486dfaa0 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -77,6 +77,14 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio,
 	if (of_property_read_bool(child, "broken-turn-around"))
 		mdio->phy_ignore_ta_mask |= 1 << addr;
 
+	if (of_property_read_u32(child, "reset-delay-us",
+				 &phy->mdio.reset_delay))
+		phy->mdio.reset_delay = 0;
+
+	if (of_property_read_u32(child, "reset-post-delay-us",
+				 &phy->mdio.reset_post_delay))
+		phy->mdio.reset_post_delay = 0;
+
 	/* Associate the OF node with the device structure so it
 	 * can be looked up later */
 	of_node_get(child);
diff --git a/include/linux/mdio.h b/include/linux/mdio.h
index 92d4e55ffe67..e37c21d8eb19 100644
--- a/include/linux/mdio.h
+++ b/include/linux/mdio.h
@@ -41,6 +41,8 @@ struct mdio_device {
 	int addr;
 	int flags;
 	struct gpio_desc *reset;
+	unsigned int reset_delay;
+	unsigned int reset_post_delay;
 };
 #define to_mdio_device(d) container_of(d, struct mdio_device, dev)
 
-- 
2.11.0

[PATCH net-next v3 2/4] phylib: add reset after clk enable support

[Richard Leitner]

From: Richard Leitner <richard.leitner@skidata.com>

Some PHYs need the refclk to be a continuous clock. Therefore they don't
allow turning it off and on again during operation. Nonetheless such a
clock switching is performed by some ETH drivers (namely FEC [1]) for
power saving reasons. An example for an affected PHY is the
SMSC/Microchip LAN8720 in "REF_CLK In Mode".

In order to provide a uniform method to overcome this problem this patch
adds a new phy_driver flag (PHY_RST_AFTER_CLK_EN) and corresponding
function phy_reset_after_clk_enable() to the phylib. These should be
used to trigger reset of the PHY after the refclk is switched on again.

This patch depends on the "phylib: Add device reset GPIO support" patch
submitted by Geert Uytterhoeven/Sergei Shtylyov [2].

[1] commit e8fcfcd5684a ("net: fec: optimize the clock management to save power")
[2] https://patchwork.kernel.org/patch/10090149/

Signed-off-by: Richard Leitner <richard.leitner@skidata.com>
---
 drivers/net/phy/phy_device.c | 24 ++++++++++++++++++++++++
 include/linux/phy.h          |  2 ++
 2 files changed, 26 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 1de5e242b8b4..462c17ed87b8 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1218,6 +1218,30 @@ int phy_loopback(struct phy_device *phydev, bool enable)
 }
 EXPORT_SYMBOL(phy_loopback);
 
+/**
+ * phy_reset_after_clk_enable - perform a PHY reset if needed
+ * @phydev: target phy_device struct
+ *
+ * Description: Some PHYs are known to need a reset after their refclk was
+ *   enabled. This function evaluates the flags and perform the reset if it's
+ *   needed. Returns < 0 on error, 0 if the phy wasn't reset and 1 if the phy
+ *   was reset.
+ */
+int phy_reset_after_clk_enable(struct phy_device *phydev)
+{
+	if (!phydev || !phydev->drv)
+		return -ENODEV;
+
+	if (phydev->drv->flags & PHY_RST_AFTER_CLK_EN) {
+		phy_device_reset(phydev, 1);
+		phy_device_reset(phydev, 0);
+		return 1;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(phy_reset_after_clk_enable);
+
 /* Generic PHY support and helper functions */
 
 /**
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 2bcbe894eb10..5c05fc73af70 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -59,6 +59,7 @@
 
 #define PHY_HAS_INTERRUPT	0x00000001
 #define PHY_IS_INTERNAL		0x00000002
+#define PHY_RST_AFTER_CLK_EN	0x00000004
 #define MDIO_DEVICE_IS_PHY	0x80000000
 
 /* Interface Mode definitions */
@@ -839,6 +840,7 @@ int phy_aneg_done(struct phy_device *phydev);
 
 int phy_stop_interrupts(struct phy_device *phydev);
 int phy_restart_aneg(struct phy_device *phydev);
+int phy_reset_after_clk_enable(struct phy_device *phydev);
 
 static inline void phy_device_reset(struct phy_device *phydev, int value)
 {
-- 
2.11.0

[PATCH net-next v3 3/4] net: phy: smsc: LAN8710/20: add PHY_RST_AFTER_CLK_EN flag

[Richard Leitner]

From: Richard Leitner <richard.leitner@skidata.com>

The Microchip/SMSC LAN8710/LAN8720 PHYs need (according to their
datasheet [1]) a continuous REF_CLK when configured to "REF_CLK In Mode".
Therefore set the PHY_RST_AFTER_CLK_EN flag for those PHYs to let the
ETH driver reset them after the REF_CLK is enabled.

[1] http://ww1.microchip.com/downloads/en/DeviceDoc/00002165B.pdf

Signed-off-by: Richard Leitner <richard.leitner@skidata.com>
---
 drivers/net/phy/smsc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
index a1961ba87e2b..be399d645224 100644
--- a/drivers/net/phy/smsc.c
+++ b/drivers/net/phy/smsc.c
@@ -312,7 +312,7 @@ static struct phy_driver smsc_phy_driver[] = {
 	.name		= "SMSC LAN8710/LAN8720",
 
 	.features	= PHY_BASIC_FEATURES,
-	.flags		= PHY_HAS_INTERRUPT,
+	.flags		= PHY_HAS_INTERRUPT | PHY_RST_AFTER_CLK_EN,
 
 	.probe		= smsc_phy_probe,
 
-- 
2.11.0

[PATCH net-next v3 4/4] net: fec: add phy_reset_after_clk_enable() support

[Richard Leitner]

From: Richard Leitner <richard.leitner@skidata.com>

Some PHYs (for example the SMSC LAN8710/LAN8720) doesn't allow turning
the refclk on and off again during operation (according to their
datasheet). Nonetheless exactly this behaviour was introduced for power
saving reasons by commit e8fcfcd5684a ("net: fec: optimize the clock management to save power").
Therefore add support for the phy_reset_after_clk_enable function from
phylib to mitigate this issue.

Generally speaking this issue is only relevant if the ref clk for the
PHY is generated by the SoC and therefore the PHY is configured to
"REF_CLK In Mode". In our specific case (PCB) this problem does occur at
about every 10th to 50th POR of an LAN8710 connected to an i.MX6SOLO
SoC. The typical symptom of this problem is a "swinging" ethernet link.
Similar issues were reported by users of the NXP forum:
	https://community.nxp.com/thread/389902
	https://community.nxp.com/message/309354
With this patch applied the issue didn't occur for at least a few
hundret PORs of our board.

Fixes: e8fcfcd5684a ("net: fec: optimize the clock management to save power")
Signed-off-by: Richard Leitner <richard.leitner@skidata.com>
---
 drivers/net/ethernet/freescale/fec_main.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 610573855213..8c3d0fb7db20 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1862,6 +1862,8 @@ static int fec_enet_clk_enable(struct net_device *ndev, bool enable)
 		ret = clk_prepare_enable(fep->clk_ref);
 		if (ret)
 			goto failed_clk_ref;
+
+		phy_reset_after_clk_enable(ndev->phydev);
 	} else {
 		clk_disable_unprepare(fep->clk_ahb);
 		clk_disable_unprepare(fep->clk_enet_out);
@@ -2860,6 +2862,11 @@ fec_enet_open(struct net_device *ndev)
 	if (ret)
 		goto err_enet_mii_probe;
 
+	/* reset phy if needed here, due to the fact this is the first time we
+	 * have the net_device to phy_driver link
+	 */
+	phy_reset_after_clk_enable(ndev->phydev);
+
 	if (fep->quirks & FEC_QUIRK_ERR006687)
 		imx6q_cpuidle_fec_irqs_used();
 
-- 
2.11.0

Re: [PATCH net-next v3 1/4] phylib: Add device reset delay support

[Geert Uytterhoeven]

Hi Richard,

On Tue, Dec 5, 2017 at 2:25 PM, Richard Leitner <dev-M/VWbR8SM2SsTnJN9+BGXg@public.gmane.org> wrote:
> From: Richard Leitner <richard.leitner-WcANXNA0UjBBDgjK7y7TUQ@public.gmane.org>
>
> Some PHYs need a minimum time after the reset gpio was asserted and/or
> deasserted. To ensure we meet these timing requirements add two new
> optional devicetree parameters for the phy: reset-delay-us and
> reset-post-delay-us.

Thanks for your patch!

> This patch depends on the "phylib: Add device reset GPIO support" patch
> submitted by Geert Uytterhoeven/Sergei Shtylyov, see:
>         https://patchwork.kernel.org/patch/10090149/

The above paragraph belongs under the "---" line below, as it is not intended
to be preserved in the eternal git history.

> Signed-off-by: Richard Leitner <richard.leitner-WcANXNA0UjBBDgjK7y7TUQ@public.gmane.org>

Reviewed-by: Geert Uytterhoeven <geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>

Although I have a few suggestions below:

> --- a/drivers/net/phy/mdio_device.c
> +++ b/drivers/net/phy/mdio_device.c
> @@ -118,8 +119,16 @@ EXPORT_SYMBOL(mdio_device_remove);
>
>  void mdio_device_reset(struct mdio_device *mdiodev, int value)
>  {
> -       if (mdiodev->reset)
> -               gpiod_set_value(mdiodev->reset, value);
> +       if (!mdiodev->reset)
> +               return;
> +
> +       gpiod_set_value(mdiodev->reset, value);
> +
> +       if (value && mdiodev->reset_delay)
> +               usleep_range(mdiodev->reset_delay, mdiodev->reset_delay + 100);
> +       else if (!value && mdiodev->reset_post_delay)
> +               usleep_range(mdiodev->reset_post_delay,
> +                            mdiodev->reset_post_delay + 100);

I think this can be written simpler using e.g.:

        unsigned int delay;

        ...
        delay = value ? mdiodev->reset_delay : mdiodev->reset_post_delay;
        if (delay)
                usleep_range(delay, delay + 100);

Perhaps the range extension should be relative, e.g.
"delay + min(delay / 10, 100)"?

> --- a/drivers/of/of_mdio.c
> +++ b/drivers/of/of_mdio.c
> @@ -77,6 +77,14 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio,
>         if (of_property_read_bool(child, "broken-turn-around"))
>                 mdio->phy_ignore_ta_mask |= 1 << addr;
>
> +       if (of_property_read_u32(child, "reset-delay-us",
> +                                &phy->mdio.reset_delay))
> +               phy->mdio.reset_delay = 0;
> +
> +       if (of_property_read_u32(child, "reset-post-delay-us",
> +                                &phy->mdio.reset_post_delay))
> +               phy->mdio.reset_post_delay = 0;

If of_property_read_u32() fails, it doesn't write to its output parameter.
As the structure should be zeroed during allocation, you can just write:

        of_property_read_u32(child, "reset-delay-us", &phy->mdio.reset_delay);
        of_property_read_u32(child, "reset-post-delay-us",
                             &phy->mdio.reset_post_delay);

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
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-next v3 1/4] phylib: Add device reset delay support

[Richard Leitner]

Hi Geert,

On 12/05/2017 02:54 PM, Geert Uytterhoeven wrote:
> Hi Richard,
> 
> On Tue, Dec 5, 2017 at 2:25 PM, Richard Leitner <dev-M/VWbR8SM2SsTnJN9+BGXg@public.gmane.org> wrote:
>> From: Richard Leitner <richard.leitner-WcANXNA0UjBBDgjK7y7TUQ@public.gmane.org>
>>
>> Some PHYs need a minimum time after the reset gpio was asserted and/or
>> deasserted. To ensure we meet these timing requirements add two new
>> optional devicetree parameters for the phy: reset-delay-us and
>> reset-post-delay-us.
> 
> Thanks for your patch!
> 
>> This patch depends on the "phylib: Add device reset GPIO support" patch
>> submitted by Geert Uytterhoeven/Sergei Shtylyov, see:
>>         https://patchwork.kernel.org/patch/10090149/
> 
> The above paragraph belongs under the "---" line below, as it is not intended
> to be preserved in the eternal git history.

Ok. Thanks. That makes sense. I'll take it into account for v4.

> 
>> Signed-off-by: Richard Leitner <richard.leitner-WcANXNA0UjBBDgjK7y7TUQ@public.gmane.org>
> 
> Reviewed-by: Geert Uytterhoeven <geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>
> 
> Although I have a few suggestions below:

Thank you for your review and suggestions (they make the code look way
more neater). I'll adapt v4 accordingly.

> 
>> --- a/drivers/net/phy/mdio_device.c
>> +++ b/drivers/net/phy/mdio_device.c
>> @@ -118,8 +119,16 @@ EXPORT_SYMBOL(mdio_device_remove);
>>
>>  void mdio_device_reset(struct mdio_device *mdiodev, int value)
>>  {
>> -       if (mdiodev->reset)
>> -               gpiod_set_value(mdiodev->reset, value);
>> +       if (!mdiodev->reset)
>> +               return;
>> +
>> +       gpiod_set_value(mdiodev->reset, value);
>> +
>> +       if (value && mdiodev->reset_delay)
>> +               usleep_range(mdiodev->reset_delay, mdiodev->reset_delay + 100);
>> +       else if (!value && mdiodev->reset_post_delay)
>> +               usleep_range(mdiodev->reset_post_delay,
>> +                            mdiodev->reset_post_delay + 100);
> 
> I think this can be written simpler using e.g.:
> 
>         unsigned int delay;
> 
>         ...
>         delay = value ? mdiodev->reset_delay : mdiodev->reset_post_delay;
>         if (delay)
>                 usleep_range(delay, delay + 100);
> 
> Perhaps the range extension should be relative, e.g.
> "delay + min(delay / 10, 100)"?
> 
>> --- a/drivers/of/of_mdio.c
>> +++ b/drivers/of/of_mdio.c
>> @@ -77,6 +77,14 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio,
>>         if (of_property_read_bool(child, "broken-turn-around"))
>>                 mdio->phy_ignore_ta_mask |= 1 << addr;
>>
>> +       if (of_property_read_u32(child, "reset-delay-us",
>> +                                &phy->mdio.reset_delay))
>> +               phy->mdio.reset_delay = 0;
>> +
>> +       if (of_property_read_u32(child, "reset-post-delay-us",
>> +                                &phy->mdio.reset_post_delay))
>> +               phy->mdio.reset_post_delay = 0;
> 
> If of_property_read_u32() fails, it doesn't write to its output parameter.
> As the structure should be zeroed during allocation, you can just write:
> 
>         of_property_read_u32(child, "reset-delay-us", &phy->mdio.reset_delay);
>         of_property_read_u32(child, "reset-post-delay-us",
>                              &phy->mdio.reset_post_delay);
--
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-next v3 1/4] phylib: Add device reset delay support

[Andrew Lunn]

Hi Richard

> +++ b/drivers/of/of_mdio.c
> @@ -77,6 +77,14 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio,
>  	if (of_property_read_bool(child, "broken-turn-around"))
>  		mdio->phy_ignore_ta_mask |= 1 << addr;
>  
> +	if (of_property_read_u32(child, "reset-delay-us",
> +				 &phy->mdio.reset_delay))
> +		phy->mdio.reset_delay = 0;
> +
> +	if (of_property_read_u32(child, "reset-post-delay-us",
> +				 &phy->mdio.reset_post_delay))
> +		phy->mdio.reset_post_delay = 0;

of_property_read_u32() should not change the variable you pass to it,
if it does not find the property. So you can change this to:

	phy->mdio.reset_delay = 0;
	phy->mdio.reset_post_delay = 0;

	of_property_read_u32(child, "reset-delay-us",
			     &phy->mdio.reset_delay);

	of_property_read_u32(child, "reset-post-delay-us",
			     &phy->mdio.reset_post_delay);


     Andrew

Re: [PATCH net-next v3 2/4] phylib: add reset after clk enable support

[Andrew Lunn]

On Tue, Dec 05, 2017 at 02:25:58PM +0100, Richard Leitner wrote:
> From: Richard Leitner <richard.leitner@skidata.com>
> 
> Some PHYs need the refclk to be a continuous clock. Therefore they don't
> allow turning it off and on again during operation. Nonetheless such a
> clock switching is performed by some ETH drivers (namely FEC [1]) for
> power saving reasons. An example for an affected PHY is the
> SMSC/Microchip LAN8720 in "REF_CLK In Mode".
> 
> In order to provide a uniform method to overcome this problem this patch
> adds a new phy_driver flag (PHY_RST_AFTER_CLK_EN) and corresponding
> function phy_reset_after_clk_enable() to the phylib. These should be
> used to trigger reset of the PHY after the refclk is switched on again.
> 
> This patch depends on the "phylib: Add device reset GPIO support" patch
> submitted by Geert Uytterhoeven/Sergei Shtylyov [2].
> 
> [1] commit e8fcfcd5684a ("net: fec: optimize the clock management to save power")
> [2] https://patchwork.kernel.org/patch/10090149/
> 
> Signed-off-by: Richard Leitner <richard.leitner@skidata.com>

Hi Richard

Same comment about moving text below the ---

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

    Andrew

Re: [PATCH net-next v3 3/4] net: phy: smsc: LAN8710/20: add PHY_RST_AFTER_CLK_EN flag

[Andrew Lunn]

On Tue, Dec 05, 2017 at 02:25:59PM +0100, Richard Leitner wrote:
> From: Richard Leitner <richard.leitner-WcANXNA0UjBBDgjK7y7TUQ@public.gmane.org>
> 
> The Microchip/SMSC LAN8710/LAN8720 PHYs need (according to their
> datasheet [1]) a continuous REF_CLK when configured to "REF_CLK In Mode".
> Therefore set the PHY_RST_AFTER_CLK_EN flag for those PHYs to let the
> ETH driver reset them after the REF_CLK is enabled.
> 
> [1] http://ww1.microchip.com/downloads/en/DeviceDoc/00002165B.pdf
> 
> Signed-off-by: Richard Leitner <richard.leitner-WcANXNA0UjBBDgjK7y7TUQ@public.gmane.org>

Reviewed-by: Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org>

    Andrew
--
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-next v3 1/4] phylib: Add device reset delay support

[Richard Leitner]

Hi Andrew,

On 12/05/2017 06:28 PM, Andrew Lunn wrote:
> Hi Richard
> 
>> +++ b/drivers/of/of_mdio.c
>> @@ -77,6 +77,14 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio,
>>   	if (of_property_read_bool(child, "broken-turn-around"))
>>   		mdio->phy_ignore_ta_mask |= 1 << addr;
>>   
>> +	if (of_property_read_u32(child, "reset-delay-us",
>> +				 &phy->mdio.reset_delay))
>> +		phy->mdio.reset_delay = 0;
>> +
>> +	if (of_property_read_u32(child, "reset-post-delay-us",
>> +				 &phy->mdio.reset_post_delay))
>> +		phy->mdio.reset_post_delay = 0;
> 
> of_property_read_u32() should not change the variable you pass to it,
> if it does not find the property. So you can change this to:
> 
> 	phy->mdio.reset_delay = 0;
> 	phy->mdio.reset_post_delay = 0;
> 
> 	of_property_read_u32(child, "reset-delay-us",
> 			     &phy->mdio.reset_delay);
> 
> 	of_property_read_u32(child, "reset-post-delay-us",
> 			     &phy->mdio.reset_post_delay);

Geert already pointed this out, but he said it's possible to omit also 
the zeroing of the variables.

 > On 12/05/2017 02:54 PM, Geert Uytterhoeven wrote:
 >> If of_property_read_u32() fails, it doesn't write to its output
 >> parameter.
 >> As the structure should be zeroed during allocation, you can just
 >> write:
 >>
 >> of_property_read_u32(child, "reset-delay-us",&phy->mdio.reset_delay);
 >> of_property_read_u32(child, "reset-post-delay-us",
 >>                      &phy->mdio.reset_post_delay);

If that's ok I'll take the shorter (Geerts) suggestion for v4.

Nonetheless thanks for your quick feedback!

regards;Richard.L

Re: [PATCH net-next v3 2/4] phylib: add reset after clk enable support

[Richard Leitner]

Hi Andrew,

On 12/05/2017 06:34 PM, Andrew Lunn wrote:
> On Tue, Dec 05, 2017 at 02:25:58PM +0100, Richard Leitner wrote:
>> From: Richard Leitner <richard.leitner-WcANXNA0UjBBDgjK7y7TUQ@public.gmane.org>
>>
>> Some PHYs need the refclk to be a continuous clock. Therefore they don't
>> allow turning it off and on again during operation. Nonetheless such a
>> clock switching is performed by some ETH drivers (namely FEC [1]) for
>> power saving reasons. An example for an affected PHY is the
>> SMSC/Microchip LAN8720 in "REF_CLK In Mode".
>>
>> In order to provide a uniform method to overcome this problem this patch
>> adds a new phy_driver flag (PHY_RST_AFTER_CLK_EN) and corresponding
>> function phy_reset_after_clk_enable() to the phylib. These should be
>> used to trigger reset of the PHY after the refclk is switched on again.
>>
>> This patch depends on the "phylib: Add device reset GPIO support" patch
>> submitted by Geert Uytterhoeven/Sergei Shtylyov [2].
>>
>> [1] commit e8fcfcd5684a ("net: fec: optimize the clock management to save power")
>> [2] https://patchwork.kernel.org/patch/10090149/
>>
>> Signed-off-by: Richard Leitner <richard.leitner-WcANXNA0UjBBDgjK7y7TUQ@public.gmane.org>
> 
> Hi Richard
> 
> Same comment about moving text below the ---

Ok. Thanks for your feedback and review.

> 
> Reviewed-by: Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org>
> 

regards;Richard.L

>      Andrew
> 
--
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-next v3 4/4] net: fec: add phy_reset_after_clk_enable() support

[Andy Duan]

From: Richard Leitner <dev-M/VWbR8SM2SsTnJN9+BGXg@public.gmane.org> Sent: Tuesday, December 05, 2017 9:26 PM
>Some PHYs (for example the SMSC LAN8710/LAN8720) doesn't allow turning
>the refclk on and off again during operation (according to their datasheet).
>Nonetheless exactly this behaviour was introduced for power saving reasons
>by commit e8fcfcd5684a ("net: fec: optimize the clock management to save
>power").
>Therefore add support for the phy_reset_after_clk_enable function from
>phylib to mitigate this issue.
>
>Generally speaking this issue is only relevant if the ref clk for the PHY is
>generated by the SoC and therefore the PHY is configured to "REF_CLK In
>Mode". In our specific case (PCB) this problem does occur at about every 10th
>to 50th POR of an LAN8710 connected to an i.MX6SOLO SoC. The typical
>symptom of this problem is a "swinging" ethernet link.
>Similar issues were reported by users of the NXP forum:
>	https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F
>%2Fcommunity.nxp.com%2Fthread%2F389902&data=02%7C01%7Cfugang.du
>an%40nxp.com%7C7f9fee272fc44662c2a108d53be3d1ee%7C686ea1d3bc2b4c6
>fa92cd99c5c301635%7C0%7C0%7C636480772022331090&sdata=7RdUsoWVWu
>o1nM5zKwLt7%2F6U3dxgDJtBDGlQCUWC6IM%3D&reserved=0
>	https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F
>%2Fcommunity.nxp.com%2Fmessage%2F309354&data=02%7C01%7Cfugang.d
>uan%40nxp.com%7C7f9fee272fc44662c2a108d53be3d1ee%7C686ea1d3bc2b4
>c6fa92cd99c5c301635%7C0%7C0%7C636480772022331090&sdata=D56KilGWD3
>kLABxc0yOI%2B44Y%2FhLfrGtdAvupCEyvI%2BI%3D&reserved=0
>With this patch applied the issue didn't occur for at least a few hundret PORs
>of our board.
>
>Fixes: e8fcfcd5684a ("net: fec: optimize the clock management to save
>power")
>Signed-off-by: Richard Leitner <richard.leitner-WcANXNA0UjBBDgjK7y7TUQ@public.gmane.org>
>---
> drivers/net/ethernet/freescale/fec_main.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
>diff --git a/drivers/net/ethernet/freescale/fec_main.c
>b/drivers/net/ethernet/freescale/fec_main.c
>index 610573855213..8c3d0fb7db20 100644
>--- a/drivers/net/ethernet/freescale/fec_main.c
>+++ b/drivers/net/ethernet/freescale/fec_main.c
>@@ -1862,6 +1862,8 @@ static int fec_enet_clk_enable(struct net_device
>*ndev, bool enable)
> 		ret = clk_prepare_enable(fep->clk_ref);
> 		if (ret)
> 			goto failed_clk_ref;
>+
>+		phy_reset_after_clk_enable(ndev->phydev);
> 	} else {
> 		clk_disable_unprepare(fep->clk_ahb);
> 		clk_disable_unprepare(fep->clk_enet_out);
>@@ -2860,6 +2862,11 @@ fec_enet_open(struct net_device *ndev)
> 	if (ret)
> 		goto err_enet_mii_probe;
>
>+	/* reset phy if needed here, due to the fact this is the first time we
>+	 * have the net_device to phy_driver link
>+	 */
>+	phy_reset_after_clk_enable(ndev->phydev);
>+

The patch series look better.
But why does it need to reset phy here since phy already is hard reset after clock enable.


> 	if (fep->quirks & FEC_QUIRK_ERR006687)
> 		imx6q_cpuidle_fec_irqs_used();
>
>--
>2.11.0
--
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-next v3 4/4] net: fec: add phy_reset_after_clk_enable() support

[Richard Leitner]

Hi Andy,

On 12/06/2017 02:50 AM, Andy Duan wrote:
> From: Richard Leitner <dev-M/VWbR8SM2SsTnJN9+BGXg@public.gmane.org> Sent: Tuesday, December 05, 2017 9:26 PM
>> Some PHYs (for example the SMSC LAN8710/LAN8720) doesn't allow turning
>> the refclk on and off again during operation (according to their datasheet).
>> Nonetheless exactly this behaviour was introduced for power saving reasons
>> by commit e8fcfcd5684a ("net: fec: optimize the clock management to save
>> power").
>> Therefore add support for the phy_reset_after_clk_enable function from
>> phylib to mitigate this issue.

...

>> diff --git a/drivers/net/ethernet/freescale/fec_main.c
>> b/drivers/net/ethernet/freescale/fec_main.c
>> index 610573855213..8c3d0fb7db20 100644
>> --- a/drivers/net/ethernet/freescale/fec_main.c
>> +++ b/drivers/net/ethernet/freescale/fec_main.c
>> @@ -1862,6 +1862,8 @@ static int fec_enet_clk_enable(struct net_device
>> *ndev, bool enable)
>> 		ret = clk_prepare_enable(fep->clk_ref);
>> 		if (ret)
>> 			goto failed_clk_ref;
>> +
>> +		phy_reset_after_clk_enable(ndev->phydev);
>> 	} else {
>> 		clk_disable_unprepare(fep->clk_ahb);
>> 		clk_disable_unprepare(fep->clk_enet_out);
>> @@ -2860,6 +2862,11 @@ fec_enet_open(struct net_device *ndev)
>> 	if (ret)
>> 		goto err_enet_mii_probe;
>>
>> +	/* reset phy if needed here, due to the fact this is the first time we
>> +	 * have the net_device to phy_driver link
>> +	 */
>> +	phy_reset_after_clk_enable(ndev->phydev);
>> +
> 
> The patch series look better.
> But why does it need to reset phy here since phy already is hard reset after clock enable.

The problem here is that in fec_enet_open() the fec_enet_clk_enable()
call is done before the phy is probed. Therefore (as mentioned in the
comment) there's no link from the net_device (ndev) to the phy_driver
(which holds the flags).

Any suggestions for a better solution are highly appreciated here! Thanks!

regards;Richard.L
--
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-next v3 4/4] net: fec: add phy_reset_after_clk_enable() support

[Andy Duan]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2834 bytes --]

From: Richard Leitner <richard.leitner@skidata.com> Sent: Wednesday, December 06, 2017 4:12 PM
>To: Andy Duan <fugang.duan@nxp.com>; Richard Leitner <dev@g0hl1n.net>;
>robh+dt@kernel.org; mark.rutland@arm.com; andrew@lunn.ch;
>f.fainelli@gmail.com; frowand.list@gmail.com
>Cc: davem@davemloft.net; geert+renesas@glider.be;
>sergei.shtylyov@cogentembedded.com; baruch@tkos.co.il; david.wu@rock-
>chips.com; lukma@denx.de; netdev@vger.kernel.org;
>devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
>Subject: Re: [PATCH net-next v3 4/4] net: fec: add
>phy_reset_after_clk_enable() support
>
>Hi Andy,
>
>On 12/06/2017 02:50 AM, Andy Duan wrote:
>> From: Richard Leitner <dev@g0hl1n.net> Sent: Tuesday, December 05,
>> 2017 9:26 PM
>>> Some PHYs (for example the SMSC LAN8710/LAN8720) doesn't allow
>>> turning the refclk on and off again during operation (according to their
>datasheet).
>>> Nonetheless exactly this behaviour was introduced for power saving
>>> reasons by commit e8fcfcd5684a ("net: fec: optimize the clock
>>> management to save power").
>>> Therefore add support for the phy_reset_after_clk_enable function
>>> from phylib to mitigate this issue.
>
>...
>
>>> diff --git a/drivers/net/ethernet/freescale/fec_main.c
>>> b/drivers/net/ethernet/freescale/fec_main.c
>>> index 610573855213..8c3d0fb7db20 100644
>>> --- a/drivers/net/ethernet/freescale/fec_main.c
>>> +++ b/drivers/net/ethernet/freescale/fec_main.c
>>> @@ -1862,6 +1862,8 @@ static int fec_enet_clk_enable(struct
>>> net_device *ndev, bool enable)
>>> 		ret = clk_prepare_enable(fep->clk_ref);
>>> 		if (ret)
>>> 			goto failed_clk_ref;
>>> +
>>> +		phy_reset_after_clk_enable(ndev->phydev);
>>> 	} else {
>>> 		clk_disable_unprepare(fep->clk_ahb);
>>> 		clk_disable_unprepare(fep->clk_enet_out);
>>> @@ -2860,6 +2862,11 @@ fec_enet_open(struct net_device *ndev)
>>> 	if (ret)
>>> 		goto err_enet_mii_probe;
>>>
>>> +	/* reset phy if needed here, due to the fact this is the first time we
>>> +	 * have the net_device to phy_driver link
>>> +	 */
>>> +	phy_reset_after_clk_enable(ndev->phydev);
>>> +
>>
>> The patch series look better.
>> But why does it need to reset phy here since phy already is hard reset after
>clock enable.
>
>The problem here is that in fec_enet_open() the fec_enet_clk_enable() call is
>done before the phy is probed. Therefore (as mentioned in the
>comment) there's no link from the net_device (ndev) to the phy_driver
>(which holds the flags).
>
>Any suggestions for a better solution are highly appreciated here! Thanks!
>
>regards;Richard.L

Okay, I see.

For the patch: Acked-by: Fugang Duan <fugang.duan@nxp.com>
N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·zøœzÚÞz)í…æèw*\x1fjg¬±¨\x1e¶‰šŽŠÝ¢j.ïÛ°\½½MŽúgjÌæa×\x02››–' ™©Þ¢¸\f¢·¦j:+v‰¨ŠwèjØm¶Ÿÿ¾\a«‘êçzZ+ƒùšŽŠÝ¢j"ú!¶i