[PATCH v2 0/3] net: ethernet: fec: fix refclk enable for SMSC LAN8710/20

[PATCH v2 0/3] net: ethernet: 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.

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 (3):
  net: ethernet: freescale: simplify fec_reset_phy
  include: linux: phy: harmonize phy_id{,_mask} type
  net: ethernet: fec: fix refclk enable for SMSC LAN8710/20

 drivers/net/ethernet/freescale/fec.h      |   4 +
 drivers/net/ethernet/freescale/fec_main.c | 125 ++++++++++++++++++++----------
 include/linux/phy.h                       |   2 +-
 3 files changed, 88 insertions(+), 43 deletions(-)

-- 
2.11.0

[PATCH v2 1/3] net: ethernet: freescale: simplify fec_reset_phy

[Richard Leitner]

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

The fec_reset_phy function allowed only one execution during probeing.
To make it more usable move the dt parsing and gpio allocation to the
probe function. The parameters of the phy reset are added to the
fec_enet_private struct. As a result the fec_reset_phy function may be
called anytime after probe.

One checkpatch.pl warning (too long line) is ignored. This is due to the
fact a string (dt property name) otherwise needs to be split over
multiple lines, which is counterproductive for the readability.

Signed-off-by: Richard Leitner <richard.leitner@skidata.com>
---
 drivers/net/ethernet/freescale/fec.h      |  4 ++
 drivers/net/ethernet/freescale/fec_main.c | 88 ++++++++++++++++---------------
 2 files changed, 50 insertions(+), 42 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index 5385074b3b7d..401c4eabf08a 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -539,6 +539,10 @@ struct fec_enet_private {
 	int	pause_flag;
 	int	wol_flag;
 	u32	quirks;
+	int	phy_reset;
+	int	phy_reset_duration;
+	int	phy_reset_post_delay;
+	bool	phy_reset_active_high;
 
 	struct	napi_struct napi;
 	int	csum_flags;
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 610573855213..06a7caca0cee 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -3212,62 +3212,36 @@ static int fec_enet_init(struct net_device *ndev)
 }
 
 #ifdef CONFIG_OF
-static int fec_reset_phy(struct platform_device *pdev)
+static int fec_reset_phy(struct net_device *ndev)
 {
-	int err, phy_reset;
-	bool active_high = false;
-	int msec = 1, phy_post_delay = 0;
-	struct device_node *np = pdev->dev.of_node;
-
-	if (!np)
-		return 0;
-
-	err = of_property_read_u32(np, "phy-reset-duration", &msec);
-	/* A sane reset duration should not be longer than 1s */
-	if (!err && msec > 1000)
-		msec = 1;
+	struct fec_enet_private *fep = netdev_priv(ndev);
 
-	phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0);
-	if (phy_reset == -EPROBE_DEFER)
-		return phy_reset;
-	else if (!gpio_is_valid(phy_reset))
+	if (!fep->phy_reset)
 		return 0;
 
-	err = of_property_read_u32(np, "phy-reset-post-delay", &phy_post_delay);
-	/* valid reset duration should be less than 1s */
-	if (!err && phy_post_delay > 1000)
-		return -EINVAL;
-
-	active_high = of_property_read_bool(np, "phy-reset-active-high");
+	gpio_set_value_cansleep(fep->phy_reset, fep->phy_reset_active_high);
 
-	err = devm_gpio_request_one(&pdev->dev, phy_reset,
-			active_high ? GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
-			"phy-reset");
-	if (err) {
-		dev_err(&pdev->dev, "failed to get phy-reset-gpios: %d\n", err);
-		return err;
-	}
-
-	if (msec > 20)
-		msleep(msec);
+	if (fep->phy_reset_duration > 20)
+		msleep(fep->phy_reset_duration);
 	else
-		usleep_range(msec * 1000, msec * 1000 + 1000);
+		usleep_range(fep->phy_reset_duration * 1000,
+			     fep->phy_reset_duration * 1000 + 1000);
 
-	gpio_set_value_cansleep(phy_reset, !active_high);
+	gpio_set_value_cansleep(fep->phy_reset, !fep->phy_reset_active_high);
 
-	if (!phy_post_delay)
+	if (!fep->phy_reset_post_delay)
 		return 0;
 
-	if (phy_post_delay > 20)
-		msleep(phy_post_delay);
+	if (fep->phy_reset_post_delay > 20)
+		msleep(fep->phy_reset_post_delay);
 	else
-		usleep_range(phy_post_delay * 1000,
-			     phy_post_delay * 1000 + 1000);
+		usleep_range(fep->phy_reset_post_delay * 1000,
+			     fep->phy_reset_post_delay * 1000 + 1000);
 
 	return 0;
 }
 #else /* CONFIG_OF */
-static int fec_reset_phy(struct platform_device *pdev)
+static int fec_reset_phy(struct net_device *ndev)
 {
 	/*
 	 * In case of platform probe, the reset has been done
@@ -3400,6 +3374,36 @@ fec_probe(struct platform_device *pdev)
 	}
 	fep->phy_node = phy_node;
 
+	fep->phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0);
+	if (gpio_is_valid(fep->phy_reset)) {
+		ret = of_property_read_u32(np, "phy-reset-duration",
+					   &fep->phy_reset_duration);
+		/* A sane reset duration should not be longer than 1s */
+		if (!ret && fep->phy_reset_post_delay > 1000)
+			fep->phy_reset_post_delay = 1;
+
+		ret = of_property_read_u32(np, "phy-reset-post-delay",
+					   &fep->phy_reset_post_delay);
+		/* valid post reset delay should be less than 1s */
+		if (!ret && fep->phy_reset_post_delay > 1000)
+			fep->phy_reset_post_delay = 1;
+
+		fep->phy_reset_active_high = of_property_read_bool(np, "phy-reset-active-high");
+
+		ret = devm_gpio_request_one(&pdev->dev, fep->phy_reset,
+					    fep->phy_reset_active_high ?
+					    GPIOF_OUT_INIT_HIGH :
+					    GPIOF_OUT_INIT_LOW,
+					    "phy-reset");
+		if (ret) {
+			dev_err(&pdev->dev, "failed to get reset-gpios: %d\n",
+				ret);
+			goto failed_phy;
+		}
+	} else {
+		fep->phy_reset = 0;
+	}
+
 	ret = of_get_phy_mode(pdev->dev.of_node);
 	if (ret < 0) {
 		pdata = dev_get_platdata(&pdev->dev);
@@ -3472,7 +3476,7 @@ fec_probe(struct platform_device *pdev)
 	pm_runtime_set_active(&pdev->dev);
 	pm_runtime_enable(&pdev->dev);
 
-	ret = fec_reset_phy(pdev);
+	ret = fec_reset_phy(ndev);
 	if (ret)
 		goto failed_reset;
 
-- 
2.11.0

RE: [PATCH v2 1/3] net: ethernet: freescale: simplify fec_reset_phy

[Andy Duan]

From: Richard Leitner <dev@g0hl1n.net> Sent: Monday, November 20, 2017 4:34 PM
>The fec_reset_phy function allowed only one execution during probeing.
>To make it more usable move the dt parsing and gpio allocation to the probe
>function. The parameters of the phy reset are added to the fec_enet_private
>struct. As a result the fec_reset_phy function may be called anytime after
>probe.
>
>One checkpatch.pl warning (too long line) is ignored. This is due to the fact a
>string (dt property name) otherwise needs to be split over multiple lines,
>which is counterproductive for the readability.
>
>Signed-off-by: Richard Leitner <richard.leitner@skidata.com>
>---

It is better to convert to gpio descriptor, and use dts gpio flag as the gpio polarity instead of extra phy_reset_active_high variable.

Regards,
Andy

> drivers/net/ethernet/freescale/fec.h      |  4 ++
> drivers/net/ethernet/freescale/fec_main.c | 88 ++++++++++++++++----------
>-----
> 2 files changed, 50 insertions(+), 42 deletions(-)
>
>diff --git a/drivers/net/ethernet/freescale/fec.h
>b/drivers/net/ethernet/freescale/fec.h
>index 5385074b3b7d..401c4eabf08a 100644
>--- a/drivers/net/ethernet/freescale/fec.h
>+++ b/drivers/net/ethernet/freescale/fec.h
>@@ -539,6 +539,10 @@ struct fec_enet_private {
> 	int	pause_flag;
> 	int	wol_flag;
> 	u32	quirks;
>+	int	phy_reset;
>+	int	phy_reset_duration;
>+	int	phy_reset_post_delay;
>+	bool	phy_reset_active_high;
>
> 	struct	napi_struct napi;
> 	int	csum_flags;
>diff --git a/drivers/net/ethernet/freescale/fec_main.c
>b/drivers/net/ethernet/freescale/fec_main.c
>index 610573855213..06a7caca0cee 100644
>--- a/drivers/net/ethernet/freescale/fec_main.c
>+++ b/drivers/net/ethernet/freescale/fec_main.c
>@@ -3212,62 +3212,36 @@ static int fec_enet_init(struct net_device *ndev)  }
>
> #ifdef CONFIG_OF
>-static int fec_reset_phy(struct platform_device *pdev)
>+static int fec_reset_phy(struct net_device *ndev)
> {
>-	int err, phy_reset;
>-	bool active_high = false;
>-	int msec = 1, phy_post_delay = 0;
>-	struct device_node *np = pdev->dev.of_node;
>-
>-	if (!np)
>-		return 0;
>-
>-	err = of_property_read_u32(np, "phy-reset-duration", &msec);
>-	/* A sane reset duration should not be longer than 1s */
>-	if (!err && msec > 1000)
>-		msec = 1;
>+	struct fec_enet_private *fep = netdev_priv(ndev);
>
>-	phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0);
>-	if (phy_reset == -EPROBE_DEFER)
>-		return phy_reset;
>-	else if (!gpio_is_valid(phy_reset))
>+	if (!fep->phy_reset)
> 		return 0;
>
>-	err = of_property_read_u32(np, "phy-reset-post-delay",
>&phy_post_delay);
>-	/* valid reset duration should be less than 1s */
>-	if (!err && phy_post_delay > 1000)
>-		return -EINVAL;
>-
>-	active_high = of_property_read_bool(np, "phy-reset-active-high");
>+	gpio_set_value_cansleep(fep->phy_reset, fep-
>>phy_reset_active_high);
>
>-	err = devm_gpio_request_one(&pdev->dev, phy_reset,
>-			active_high ? GPIOF_OUT_INIT_HIGH :
>GPIOF_OUT_INIT_LOW,
>-			"phy-reset");
>-	if (err) {
>-		dev_err(&pdev->dev, "failed to get phy-reset-gpios: %d\n",
>err);
>-		return err;
>-	}
>-
>-	if (msec > 20)
>-		msleep(msec);
>+	if (fep->phy_reset_duration > 20)
>+		msleep(fep->phy_reset_duration);
> 	else
>-		usleep_range(msec * 1000, msec * 1000 + 1000);
>+		usleep_range(fep->phy_reset_duration * 1000,
>+			     fep->phy_reset_duration * 1000 + 1000);
>
>-	gpio_set_value_cansleep(phy_reset, !active_high);
>+	gpio_set_value_cansleep(fep->phy_reset, !fep-
>>phy_reset_active_high);
>
>-	if (!phy_post_delay)
>+	if (!fep->phy_reset_post_delay)
> 		return 0;
>
>-	if (phy_post_delay > 20)
>-		msleep(phy_post_delay);
>+	if (fep->phy_reset_post_delay > 20)
>+		msleep(fep->phy_reset_post_delay);
> 	else
>-		usleep_range(phy_post_delay * 1000,
>-			     phy_post_delay * 1000 + 1000);
>+		usleep_range(fep->phy_reset_post_delay * 1000,
>+			     fep->phy_reset_post_delay * 1000 + 1000);
>
> 	return 0;
> }
> #else /* CONFIG_OF */
>-static int fec_reset_phy(struct platform_device *pdev)
>+static int fec_reset_phy(struct net_device *ndev)
> {
> 	/*
> 	 * In case of platform probe, the reset has been done @@ -3400,6
>+3374,36 @@ fec_probe(struct platform_device *pdev)
> 	}
> 	fep->phy_node = phy_node;
>
>+	fep->phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0);
>+	if (gpio_is_valid(fep->phy_reset)) {
>+		ret = of_property_read_u32(np, "phy-reset-duration",
>+					   &fep->phy_reset_duration);
>+		/* A sane reset duration should not be longer than 1s */
>+		if (!ret && fep->phy_reset_post_delay > 1000)
>+			fep->phy_reset_post_delay = 1;
>+
>+		ret = of_property_read_u32(np, "phy-reset-post-delay",
>+					   &fep->phy_reset_post_delay);
>+		/* valid post reset delay should be less than 1s */
>+		if (!ret && fep->phy_reset_post_delay > 1000)
>+			fep->phy_reset_post_delay = 1;
>+
>+		fep->phy_reset_active_high = of_property_read_bool(np,
>+"phy-reset-active-high");
>+
>+		ret = devm_gpio_request_one(&pdev->dev, fep->phy_reset,
>+					    fep->phy_reset_active_high ?
>+					    GPIOF_OUT_INIT_HIGH :
>+					    GPIOF_OUT_INIT_LOW,
>+					    "phy-reset");
>+		if (ret) {
>+			dev_err(&pdev->dev, "failed to get reset-
>gpios: %d\n",
>+				ret);
>+			goto failed_phy;
>+		}
>+	} else {
>+		fep->phy_reset = 0;
>+	}
>+
> 	ret = of_get_phy_mode(pdev->dev.of_node);
> 	if (ret < 0) {
> 		pdata = dev_get_platdata(&pdev->dev); @@ -3472,7 +3476,7
>@@ fec_probe(struct platform_device *pdev)
> 	pm_runtime_set_active(&pdev->dev);
> 	pm_runtime_enable(&pdev->dev);
>
>-	ret = fec_reset_phy(pdev);
>+	ret = fec_reset_phy(ndev);
> 	if (ret)
> 		goto failed_reset;
>
>--
>2.11.0

[PATCH v2 2/3] include: linux: phy: harmonize phy_id{,_mask} type

[Richard Leitner]

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

Previously phy_id was u32 and phy_id_mask was unsigned int. As the
phy_id_mask defines the important bits of the phy_id (and is therefore the
same size) these two variables should be the same datatype.

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

diff --git a/include/linux/phy.h b/include/linux/phy.h
index dc82a07cb4fd..e00fd9ce3bce 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -509,7 +509,7 @@ struct phy_driver {
 	struct mdio_driver_common mdiodrv;
 	u32 phy_id;
 	char *name;
-	unsigned int phy_id_mask;
+	u32 phy_id_mask;
 	u32 features;
 	u32 flags;
 	const void *driver_data;
-- 
2.11.0

[PATCH v2 3/3] net: ethernet: fec: fix refclk enable for SMSC LAN8710/20

[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 after enabling the refclk we detect if an affected PHY is
attached. If so reset and initialize it again.

For a better understanding here's a outline of the time response of the
clock and reset lines before and after this patch:

			  v--fec_probe()              v--fec_enet_open()
			  v                           v
	w/o patch eCLK: ___||||||||____________________|||||||||||||||||
	w/o patch nRST: ----__------------------------------------------
	w/o patch CONF: _______XX_______________________________________

	w/  patch eCLK: ___||||||||____________________|||||||||||||||||
	w/  patch nRST: ----__-----------------------------__-----------
	w/  patch CONF: _______XX_____________________________XX________
			  ^                           ^
			  ^--fec_probe()              ^--fec_enet_open()

Generally speaking this issue is only relevant if the ref clk for the
PHY is generated by the SoC. In our specific case (PCB) this problem
does occur at about every 10th to 50th POR of an LAN8710 connected to an
i.MX6DL 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 | 37 +++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 06a7caca0cee..52ec9b29a70e 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -68,6 +68,7 @@
 
 static void set_multicast_list(struct net_device *ndev);
 static void fec_enet_itr_coal_init(struct net_device *ndev);
+static int fec_reset_phy(struct net_device *ndev);
 
 #define DRIVER_NAME	"fec"
 
@@ -1833,6 +1834,32 @@ static int fec_enet_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
 	return ret;
 }
 
+static int fec_enet_clk_ref_enable_reset_phy_quirk(struct net_device *ndev)
+{
+	struct phy_device *phy_dev = ndev->phydev;
+	u32 real_phy_id;
+	int ret;
+
+	/* some PHYs need a reset after the refclk was enabled, so we
+	 * reset them here
+	 */
+	if (!phy_dev)
+		return 0;
+	if (!phy_dev->drv)
+		return 0;
+	real_phy_id = phy_dev->drv->phy_id & phy_dev->drv->phy_id_mask;
+	switch (real_phy_id) {
+	case 0x0007c0f0: /* SMSC LAN8710/LAN8720 */
+		ret = fec_reset_phy(ndev);
+		if (ret)
+			return ret;
+		ret = phy_init_hw(phy_dev);
+		if (ret)
+			return ret;
+	}
+	return 0;
+}
+
 static int fec_enet_clk_enable(struct net_device *ndev, bool enable)
 {
 	struct fec_enet_private *fep = netdev_priv(ndev);
@@ -1862,6 +1889,10 @@ 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;
+
+		ret = fec_enet_clk_ref_enable_reset_phy_quirk(ndev);
+		if (ret)
+			netdev_warn(ndev, "Resetting PHY failed, connection may be unstable\n");
 	} else {
 		clk_disable_unprepare(fep->clk_ahb);
 		clk_disable_unprepare(fep->clk_enet_out);
@@ -2860,11 +2891,17 @@ fec_enet_open(struct net_device *ndev)
 	if (ret)
 		goto err_enet_mii_probe;
 
+	/* as the PHY is connected now, trigger the reset quirk again */
+	ret = fec_enet_clk_ref_enable_reset_phy_quirk(ndev);
+	if (ret)
+		netdev_warn(ndev, "Resetting PHY failed, connection may be unstable\n");
+
 	if (fep->quirks & FEC_QUIRK_ERR006687)
 		imx6q_cpuidle_fec_irqs_used();
 
 	napi_enable(&fep->napi);
 	phy_start(ndev->phydev);
+
 	netif_tx_start_all_queues(ndev);
 
 	device_set_wakeup_enable(&ndev->dev, fep->wol_flag &
-- 
2.11.0

RE: [PATCH v2 3/3] net: ethernet: fec: fix refclk enable for SMSC LAN8710/20

[Andy Duan]

From: Richard Leitner <dev@g0hl1n.net> Sent: Monday, November 20, 2017 4:34 PM
>To: f.fainelli@gmail.com; Andy Duan <fugang.duan@nxp.com>;
>andrew@lunn.ch
>Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
>richard.leitner@skidata.com
>Subject: [PATCH v2 3/3] net: ethernet: fec: fix refclk enable for SMSC
>LAN8710/20
>
>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 after enabling the refclk we detect if an affected PHY is attached. If
>so reset and initialize it again.
>
>For a better understanding here's a outline of the time response of the clock
>and reset lines before and after this patch:
>
>			  v--fec_probe()              v--fec_enet_open()
>			  v                           v
>	w/o patch eCLK:
>___||||||||____________________|||||||||||||||||
>	w/o patch nRST: ----__------------------------------------------
>	w/o patch CONF:
>_______XX_______________________________________
>
>	w/  patch eCLK:
>___||||||||____________________|||||||||||||||||
>	w/  patch nRST: ----__-----------------------------__-----------
>	w/  patch CONF:
>_______XX_____________________________XX________
>			  ^                           ^
>			  ^--fec_probe()              ^--fec_enet_open()
>
>Generally speaking this issue is only relevant if the ref clk for the PHY is
>generated by the SoC. In our specific case (PCB) this problem does occur at
>about every 10th to 50th POR of an LAN8710 connected to an i.MX6DL 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%7C507c7aafbd4744f81ebf08d52ff1acff%7C686ea1d3bc2b4c6f
>a92cd99c5c301635%7C0%7C0%7C636467637399145483&sdata=RNXVGpPrlrcyL
>0SoQl8%2BI0k8Oc8BM0Iwykd1O%2Bjmvcc%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%7C507c7aafbd4744f81ebf08d52ff1acff%7C686ea1d3bc2b4c6
>fa92cd99c5c301635%7C0%7C0%7C636467637399145483&sdata=pjeJEZGuBpb9
>uCMKGr70qa%2FmsNoak6v3nCID2vbNAeg%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@skidata.com>
>---
> drivers/net/ethernet/freescale/fec_main.c | 37
>+++++++++++++++++++++++++++++++
> 1 file changed, 37 insertions(+)
>
>diff --git a/drivers/net/ethernet/freescale/fec_main.c
>b/drivers/net/ethernet/freescale/fec_main.c
>index 06a7caca0cee..52ec9b29a70e 100644
>--- a/drivers/net/ethernet/freescale/fec_main.c
>+++ b/drivers/net/ethernet/freescale/fec_main.c
>@@ -68,6 +68,7 @@
>
> static void set_multicast_list(struct net_device *ndev);  static void
>fec_enet_itr_coal_init(struct net_device *ndev);
>+static int fec_reset_phy(struct net_device *ndev);
>
> #define DRIVER_NAME	"fec"
>
>@@ -1833,6 +1834,32 @@ static int fec_enet_mdio_write(struct mii_bus *bus,
>int mii_id, int regnum,
> 	return ret;
> }
>
>+static int fec_enet_clk_ref_enable_reset_phy_quirk(struct net_device
>+*ndev) {
>+	struct phy_device *phy_dev = ndev->phydev;
>+	u32 real_phy_id;
>+	int ret;
>+
>+	/* some PHYs need a reset after the refclk was enabled, so we
>+	 * reset them here
>+	 */
>+	if (!phy_dev)
>+		return 0;
>+	if (!phy_dev->drv)
>+		return 0;
>+	real_phy_id = phy_dev->drv->phy_id & phy_dev->drv->phy_id_mask;
>+	switch (real_phy_id) {
>+	case 0x0007c0f0: /* SMSC LAN8710/LAN8720 */

Don't hard code here...
I believe there have many other phys also do such operation, hardcode is unacceptable...

And these code can be put into phy_device.c as common interface.

>+		ret = fec_reset_phy(ndev);
>+		if (ret)
>+			return ret;
>+		ret = phy_init_hw(phy_dev);
>+		if (ret)
>+			return ret;
>+	}
>+	return 0;
>+}
>+
> static int fec_enet_clk_enable(struct net_device *ndev, bool enable)  {
> 	struct fec_enet_private *fep = netdev_priv(ndev); @@ -1862,6
>+1889,10 @@ 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;
>+
>+		ret = fec_enet_clk_ref_enable_reset_phy_quirk(ndev);
>+		if (ret)
>+			netdev_warn(ndev, "Resetting PHY failed, connection
>may be
>+unstable\n");
> 	} else {
> 		clk_disable_unprepare(fep->clk_ahb);
> 		clk_disable_unprepare(fep->clk_enet_out);
>@@ -2860,11 +2891,17 @@ fec_enet_open(struct net_device *ndev)
> 	if (ret)
> 		goto err_enet_mii_probe;
>
>+	/* as the PHY is connected now, trigger the reset quirk again */
>+	ret = fec_enet_clk_ref_enable_reset_phy_quirk(ndev);
>+	if (ret)
>+		netdev_warn(ndev, "Resetting PHY failed, connection may be
>+unstable\n");
>+
> 	if (fep->quirks & FEC_QUIRK_ERR006687)
> 		imx6q_cpuidle_fec_irqs_used();
>
> 	napi_enable(&fep->napi);
> 	phy_start(ndev->phydev);
>+

No need blank line here...
> 	netif_tx_start_all_queues(ndev);
>
> 	device_set_wakeup_enable(&ndev->dev, fep->wol_flag &
>--
>2.11.0

Re: [PATCH v2 3/3] net: ethernet: fec: fix refclk enable for SMSC LAN8710/20

[Richard Leitner]

On 11/20/2017 10:47 AM, Andy Duan wrote:
> From: Richard Leitner <dev@g0hl1n.net> Sent: Monday, November 20, 2017 4:34 PM
>> To: f.fainelli@gmail.com; Andy Duan <fugang.duan@nxp.com>;
>> andrew@lunn.ch
>> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
>> richard.leitner@skidata.com
>> Subject: [PATCH v2 3/3] net: ethernet: fec: fix refclk enable for SMSC
>> LAN8710/20
>>
>> 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 after enabling the refclk we detect if an affected PHY is attached. If
>> so reset and initialize it again.

...

>> +static int fec_enet_clk_ref_enable_reset_phy_quirk(struct net_device
>> +*ndev) {
>> +	struct phy_device *phy_dev = ndev->phydev;
>> +	u32 real_phy_id;
>> +	int ret;
>> +
>> +	/* some PHYs need a reset after the refclk was enabled, so we
>> +	 * reset them here
>> +	 */
>> +	if (!phy_dev)
>> +		return 0;
>> +	if (!phy_dev->drv)
>> +		return 0;
>> +	real_phy_id = phy_dev->drv->phy_id & phy_dev->drv->phy_id_mask;
>> +	switch (real_phy_id) {
>> +	case 0x0007c0f0: /* SMSC LAN8710/LAN8720 */
> 
> Don't hard code here...
> I believe there have many other phys also do such operation, hardcode is unacceptable...
> 
> And these code can be put into phy_device.c as common interface.

Ok. Thank you for the feedback.
So it would be fine to hardcode the affected phy_id's in a common
function in phy_device.c?


Another possible solution that came to my mind is to add a flag called
something like "PHY_RST_AFTER_CLK_EN" to the flags variable in struct
phy_driver. This flag could then be set in the smsc PHY driver for
affected PHYs.

Then instead of comparing the phy_id in the MAC driver this flag could
be checked:

if (phydev->drv->flags & PHY_RST_AFTER_CLK_EN) {
    ret = fec_reset_phy(ndev);
    ...
}

Would checking the flag be OK in fec_main.c?

What would be the "better" approach?

> 
>> +		ret = fec_reset_phy(ndev);
>> +		if (ret)
>> +			return ret;
>> +		ret = phy_init_hw(phy_dev);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +	return 0;
>> +}
>> +
>> static int fec_enet_clk_enable(struct net_device *ndev, bool enable)  {
>> 	struct fec_enet_private *fep = netdev_priv(ndev); @@ -1862,6
>> +1889,10 @@ 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;
>> +
>> +		ret = fec_enet_clk_ref_enable_reset_phy_quirk(ndev);
>> +		if (ret)
>> +			netdev_warn(ndev, "Resetting PHY failed, connection
>> may be
>> +unstable\n");
>> 	} else {
>> 		clk_disable_unprepare(fep->clk_ahb);
>> 		clk_disable_unprepare(fep->clk_enet_out);
>> @@ -2860,11 +2891,17 @@ fec_enet_open(struct net_device *ndev)
>> 	if (ret)
>> 		goto err_enet_mii_probe;
>>
>> +	/* as the PHY is connected now, trigger the reset quirk again */
>> +	ret = fec_enet_clk_ref_enable_reset_phy_quirk(ndev);
>> +	if (ret)
>> +		netdev_warn(ndev, "Resetting PHY failed, connection may be
>> +unstable\n");
>> +
>> 	if (fep->quirks & FEC_QUIRK_ERR006687)
>> 		imx6q_cpuidle_fec_irqs_used();
>>
>> 	napi_enable(&fep->napi);
>> 	phy_start(ndev->phydev);
>> +
> 
> No need blank line here...
>> 	netif_tx_start_all_queues(ndev);
>>
>> 	device_set_wakeup_enable(&ndev->dev, fep->wol_flag &
>> --
>> 2.11.0

RE: [PATCH v2 3/3] net: ethernet: fec: fix refclk enable for SMSC LAN8710/20

[Andy Duan]

From: Richard Leitner <richard.leitner@skidata.com> Sent: Monday, November 20, 2017 5:57 PM
>To: Andy Duan <fugang.duan@nxp.com>; f.fainelli@gmail.com;
>andrew@lunn.ch
>Cc: Richard Leitner <dev@g0hl1n.net>; netdev@vger.kernel.org; linux-
>kernel@vger.kernel.org
>Subject: Re: [PATCH v2 3/3] net: ethernet: fec: fix refclk enable for SMSC
>LAN8710/20
>
>
>On 11/20/2017 10:47 AM, Andy Duan wrote:
>> From: Richard Leitner <dev@g0hl1n.net> Sent: Monday, November 20, 2017
>> 4:34 PM
>>> To: f.fainelli@gmail.com; Andy Duan <fugang.duan@nxp.com>;
>>> andrew@lunn.ch
>>> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
>>> richard.leitner@skidata.com
>>> Subject: [PATCH v2 3/3] net: ethernet: fec: fix refclk enable for
>>> SMSC
>>> LAN8710/20
>>>
>>> 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 after enabling the refclk we detect if an affected PHY is
>>> attached. If so reset and initialize it again.
>
>...
>
>>> +static int fec_enet_clk_ref_enable_reset_phy_quirk(struct net_device
>>> +*ndev) {
>>> +	struct phy_device *phy_dev = ndev->phydev;
>>> +	u32 real_phy_id;
>>> +	int ret;
>>> +
>>> +	/* some PHYs need a reset after the refclk was enabled, so we
>>> +	 * reset them here
>>> +	 */
>>> +	if (!phy_dev)
>>> +		return 0;
>>> +	if (!phy_dev->drv)
>>> +		return 0;
>>> +	real_phy_id = phy_dev->drv->phy_id & phy_dev->drv->phy_id_mask;
>>> +	switch (real_phy_id) {
>>> +	case 0x0007c0f0: /* SMSC LAN8710/LAN8720 */
>>
>> Don't hard code here...
>> I believe there have many other phys also do such operation, hardcode is
>unacceptable...
>>
>> And these code can be put into phy_device.c as common interface.
>
>Ok. Thank you for the feedback.
>So it would be fine to hardcode the affected phy_id's in a common function in
>phy_device.c?
>
>
>Another possible solution that came to my mind is to add a flag called
>something like "PHY_RST_AFTER_CLK_EN" to the flags variable in struct
>phy_driver. This flag could then be set in the smsc PHY driver for affected
>PHYs.
>
>Then instead of comparing the phy_id in the MAC driver this flag could be
>checked:
>
>if (phydev->drv->flags & PHY_RST_AFTER_CLK_EN) {
>    ret = fec_reset_phy(ndev);
>    ...
>}
>
>Would checking the flag be OK in fec_main.c?

Yes, it is better than previous solution.
But add new common API in phy_device.c is much better like: 
1. add a flag called "PHY_RST_AFTER_CLK_EN" to the flags variable in struct phy_driver,  all phy driver that need reset can set the flag.
2. add new common api interface phy_reset_after_clk_enable() in phy_device.c driver
3. add reset gpio descriptor for common phy device driver. 
4. then any mac driver can directly call the common interface .phy_reset_after_clk_enable().

That is only my suggestion, maybe there have better idea.
Thanks.

>
>What would be the "better" approach?
>
>>
>>> +		ret = fec_reset_phy(ndev);
>>> +		if (ret)
>>> +			return ret;
>>> +		ret = phy_init_hw(phy_dev);
>>> +		if (ret)
>>> +			return ret;
>>> +	}
>>> +	return 0;
>>> +}
>>> +
>>> static int fec_enet_clk_enable(struct net_device *ndev, bool enable)  {
>>> 	struct fec_enet_private *fep = netdev_priv(ndev); @@ -1862,6
>>> +1889,10 @@ 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;
>>> +
>>> +		ret = fec_enet_clk_ref_enable_reset_phy_quirk(ndev);
>>> +		if (ret)
>>> +			netdev_warn(ndev, "Resetting PHY failed, connection
>>> may be
>>> +unstable\n");
>>> 	} else {
>>> 		clk_disable_unprepare(fep->clk_ahb);
>>> 		clk_disable_unprepare(fep->clk_enet_out);
>>> @@ -2860,11 +2891,17 @@ fec_enet_open(struct net_device *ndev)
>>> 	if (ret)
>>> 		goto err_enet_mii_probe;
>>>
>>> +	/* as the PHY is connected now, trigger the reset quirk again */
>>> +	ret = fec_enet_clk_ref_enable_reset_phy_quirk(ndev);
>>> +	if (ret)
>>> +		netdev_warn(ndev, "Resetting PHY failed, connection may be
>>> +unstable\n");
>>> +
>>> 	if (fep->quirks & FEC_QUIRK_ERR006687)
>>> 		imx6q_cpuidle_fec_irqs_used();
>>>
>>> 	napi_enable(&fep->napi);
>>> 	phy_start(ndev->phydev);
>>> +
>>
>> No need blank line here...
>>> 	netif_tx_start_all_queues(ndev);
>>>
>>> 	device_set_wakeup_enable(&ndev->dev, fep->wol_flag &
>>> --
>>> 2.11.0

Re: [PATCH v2 3/3] net: ethernet: fec: fix refclk enable for SMSC LAN8710/20

[Richard Leitner]

On 11/20/2017 11:35 AM, Andy Duan wrote:
> From: Richard Leitner <richard.leitner@skidata.com> Sent: Monday, November 20, 2017 5:57 PM
>> To: Andy Duan <fugang.duan@nxp.com>; f.fainelli@gmail.com;
>> andrew@lunn.ch
>> Cc: Richard Leitner <dev@g0hl1n.net>; netdev@vger.kernel.org; linux-
>> kernel@vger.kernel.org
>> Subject: Re: [PATCH v2 3/3] net: ethernet: fec: fix refclk enable for SMSC
>> LAN8710/20
>>
>>
>> On 11/20/2017 10:47 AM, Andy Duan wrote:
>>> From: Richard Leitner <dev@g0hl1n.net> Sent: Monday, November 20, 2017
>>> 4:34 PM
>>>> To: f.fainelli@gmail.com; Andy Duan <fugang.duan@nxp.com>;
>>>> andrew@lunn.ch
>>>> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
>>>> richard.leitner@skidata.com
>>>> Subject: [PATCH v2 3/3] net: ethernet: fec: fix refclk enable for
>>>> SMSC
>>>> LAN8710/20
>>>>
>>>> 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 after enabling the refclk we detect if an affected PHY is
>>>> attached. If so reset and initialize it again.
>> ...
>>
>>>> +static int fec_enet_clk_ref_enable_reset_phy_quirk(struct net_device
>>>> +*ndev) {
>>>> +	struct phy_device *phy_dev = ndev->phydev;
>>>> +	u32 real_phy_id;
>>>> +	int ret;
>>>> +
>>>> +	/* some PHYs need a reset after the refclk was enabled, so we
>>>> +	 * reset them here
>>>> +	 */
>>>> +	if (!phy_dev)
>>>> +		return 0;
>>>> +	if (!phy_dev->drv)
>>>> +		return 0;
>>>> +	real_phy_id = phy_dev->drv->phy_id & phy_dev->drv->phy_id_mask;
>>>> +	switch (real_phy_id) {
>>>> +	case 0x0007c0f0: /* SMSC LAN8710/LAN8720 */
>>> Don't hard code here...
>>> I believe there have many other phys also do such operation, hardcode is
>> unacceptable...
>>> And these code can be put into phy_device.c as common interface.
>> Ok. Thank you for the feedback.
>> So it would be fine to hardcode the affected phy_id's in a common function in
>> phy_device.c?
>>
>>
>> Another possible solution that came to my mind is to add a flag called
>> something like "PHY_RST_AFTER_CLK_EN" to the flags variable in struct
>> phy_driver. This flag could then be set in the smsc PHY driver for affected
>> PHYs.
>>
>> Then instead of comparing the phy_id in the MAC driver this flag could be
>> checked:
>>
>> if (phydev->drv->flags & PHY_RST_AFTER_CLK_EN) {
>>    ret = fec_reset_phy(ndev);
>>    ...
>> }
>>
>> Would checking the flag be OK in fec_main.c?
> Yes, it is better than previous solution.
> But add new common API in phy_device.c is much better like: 
> 1. add a flag called "PHY_RST_AFTER_CLK_EN" to the flags variable in struct phy_driver,  all phy driver that need reset can set the flag.

OK.

> 2. add new common api interface phy_reset_after_clk_enable() in phy_device.c driver

OK. But see below...

> 3. add reset gpio descriptor for common phy device driver. 

... if I understood it correctly the patch called "Teach phylib
hard-resetting devices" by Geert and Sergei is exactly doing this:
	https://patchwork.ozlabs.org/cover/828503/
	https://lkml.org/lkml/2017/10/20/166

So I'll implement the phy_reset_after_clk_enable function atop of this
patch-set and add a note that my patch-series depends on it. Would that
be OK?

> 4. then any mac driver can directly call the common interface .phy_reset_after_clk_enable().

Sounds reasonable :-)

> 
> That is only my suggestion, maybe there have better idea.
> Thanks.
> 

Thanks for your quick feedback.

regards
Richard.L

Re: [PATCH v2 3/3] net: ethernet: fec: fix refclk enable for SMSC LAN8710/20

[Geert Uytterhoeven]

Hi Richard,

On Mon, Nov 20, 2017 at 1:55 PM, Richard Leitner
<richard.leitner@skidata.com> wrote:
> On 11/20/2017 11:35 AM, Andy Duan wrote:
>> 3. add reset gpio descriptor for common phy device driver.
>
> ... if I understood it correctly the patch called "Teach phylib
> hard-resetting devices" by Geert and Sergei is exactly doing this:
>         https://patchwork.ozlabs.org/cover/828503/
>         https://lkml.org/lkml/2017/10/20/166
>
> So I'll implement the phy_reset_after_clk_enable function atop of this
> patch-set and add a note that my patch-series depends on it. Would that
> be OK?

I will update and respin that patch series after the merge window has closed.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.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

Re: [PATCH v2 3/3] net: ethernet: fec: fix refclk enable for SMSC LAN8710/20

[Richard Leitner]

On 11/20/2017 02:13 PM, Geert Uytterhoeven wrote:
> Hi Richard,
> 
> On Mon, Nov 20, 2017 at 1:55 PM, Richard Leitner
> <richard.leitner@skidata.com> wrote:
>> On 11/20/2017 11:35 AM, Andy Duan wrote:
>>> 3. add reset gpio descriptor for common phy device driver.
>>
>> ... if I understood it correctly the patch called "Teach phylib
>> hard-resetting devices" by Geert and Sergei is exactly doing this:
>>         https://patchwork.ozlabs.org/cover/828503/
>>         https://lkml.org/lkml/2017/10/20/166
>>
>> So I'll implement the phy_reset_after_clk_enable function atop of this
>> patch-set and add a note that my patch-series depends on it. Would that
>> be OK?
> 
> I will update and respin that patch series after the merge window has closed.

Ok. Thank you for the quick response an this information.

For the Freescale Fast Ethernet Controller (FEC) there are currently (in
addition to the reset gpio) two additional optional dt properties for
the reset:
 - phy-reset-duration : Reset duration in milliseconds.
 - phy-reset-post-delay : Post reset delay in milliseconds.

IMHO it would make sense to include them also in the phylib
implementation. What do you think about it? Should I include it in my
patch-series?

kind regards;
Richard.L

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.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
> 

Re: [PATCH v2 3/3] net: ethernet: fec: fix refclk enable for SMSC LAN8710/20

[Geert Uytterhoeven]

Hi Richard,

On Mon, Nov 20, 2017 at 2:21 PM, Richard Leitner
<richard.leitner@skidata.com> wrote:
> On 11/20/2017 02:13 PM, Geert Uytterhoeven wrote:
>> On Mon, Nov 20, 2017 at 1:55 PM, Richard Leitner
>> <richard.leitner@skidata.com> wrote:
>>> On 11/20/2017 11:35 AM, Andy Duan wrote:
>>>> 3. add reset gpio descriptor for common phy device driver.
>>>
>>> ... if I understood it correctly the patch called "Teach phylib
>>> hard-resetting devices" by Geert and Sergei is exactly doing this:
>>>         https://patchwork.ozlabs.org/cover/828503/
>>>         https://lkml.org/lkml/2017/10/20/166
>>>
>>> So I'll implement the phy_reset_after_clk_enable function atop of this
>>> patch-set and add a note that my patch-series depends on it. Would that
>>> be OK?
>>
>> I will update and respin that patch series after the merge window has closed.
>
> Ok. Thank you for the quick response an this information.
>
> For the Freescale Fast Ethernet Controller (FEC) there are currently (in
> addition to the reset gpio) two additional optional dt properties for
> the reset:
>  - phy-reset-duration : Reset duration in milliseconds.
>  - phy-reset-post-delay : Post reset delay in milliseconds.
>
> IMHO it would make sense to include them also in the phylib
> implementation. What do you think about it? Should I include it in my
> patch-series?

Sure, you can always extend phylib, building on top of our simple reset
implementation.

BTW, I think phy-reset-{duration,post-delay} should be moved to the
phy node as well, dropping the "phy-" prefix in the process.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.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

RE: [PATCH v2 3/3] net: ethernet: fec: fix refclk enable for SMSC LAN8710/20

[Andy Duan]

From: Richard Leitner <richard.leitner@skidata.com> Sent: Monday, November 20, 2017 8:55 PM
>On 11/20/2017 11:35 AM, Andy Duan wrote:
>> From: Richard Leitner <richard.leitner@skidata.com> Sent: Monday,
>> November 20, 2017 5:57 PM
>>> To: Andy Duan <fugang.duan@nxp.com>; f.fainelli@gmail.com;
>>> andrew@lunn.ch
>>> Cc: Richard Leitner <dev@g0hl1n.net>; netdev@vger.kernel.org; linux-
>>> kernel@vger.kernel.org
>>> Subject: Re: [PATCH v2 3/3] net: ethernet: fec: fix refclk enable for
>>> SMSC
>>> LAN8710/20
>>>
>>>
>>> On 11/20/2017 10:47 AM, Andy Duan wrote:
>>>> From: Richard Leitner <dev@g0hl1n.net> Sent: Monday, November 20,
>>>> 2017
>>>> 4:34 PM
>>>>> To: f.fainelli@gmail.com; Andy Duan <fugang.duan@nxp.com>;
>>>>> andrew@lunn.ch
>>>>> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
>>>>> richard.leitner@skidata.com
>>>>> Subject: [PATCH v2 3/3] net: ethernet: fec: fix refclk enable for
>>>>> SMSC
>>>>> LAN8710/20
>>>>>
>>>>> 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 after enabling the refclk we detect if an affected PHY is
>>>>> attached. If so reset and initialize it again.
>>> ...
>>>
>>>>> +static int fec_enet_clk_ref_enable_reset_phy_quirk(struct
>>>>> +net_device
>>>>> +*ndev) {
>>>>> +	struct phy_device *phy_dev = ndev->phydev;
>>>>> +	u32 real_phy_id;
>>>>> +	int ret;
>>>>> +
>>>>> +	/* some PHYs need a reset after the refclk was enabled, so we
>>>>> +	 * reset them here
>>>>> +	 */
>>>>> +	if (!phy_dev)
>>>>> +		return 0;
>>>>> +	if (!phy_dev->drv)
>>>>> +		return 0;
>>>>> +	real_phy_id = phy_dev->drv->phy_id & phy_dev->drv->phy_id_mask;
>>>>> +	switch (real_phy_id) {
>>>>> +	case 0x0007c0f0: /* SMSC LAN8710/LAN8720 */
>>>> Don't hard code here...
>>>> I believe there have many other phys also do such operation,
>>>> hardcode is
>>> unacceptable...
>>>> And these code can be put into phy_device.c as common interface.
>>> Ok. Thank you for the feedback.
>>> So it would be fine to hardcode the affected phy_id's in a common
>>> function in phy_device.c?
>>>
>>>
>>> Another possible solution that came to my mind is to add a flag
>>> called something like "PHY_RST_AFTER_CLK_EN" to the flags variable in
>>> struct phy_driver. This flag could then be set in the smsc PHY driver
>>> for affected PHYs.
>>>
>>> Then instead of comparing the phy_id in the MAC driver this flag
>>> could be
>>> checked:
>>>
>>> if (phydev->drv->flags & PHY_RST_AFTER_CLK_EN) {
>>>    ret = fec_reset_phy(ndev);
>>>    ...
>>> }
>>>
>>> Would checking the flag be OK in fec_main.c?
>> Yes, it is better than previous solution.
>> But add new common API in phy_device.c is much better like:
>> 1. add a flag called "PHY_RST_AFTER_CLK_EN" to the flags variable in struct
>phy_driver,  all phy driver that need reset can set the flag.
>
>OK.
>
>> 2. add new common api interface phy_reset_after_clk_enable() in
>> phy_device.c driver
>
>OK. But see below...
>
>> 3. add reset gpio descriptor for common phy device driver.
>
>... if I understood it correctly the patch called "Teach phylib hard-resetting
>devices" by Geert and Sergei is exactly doing this:
>	https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F
>%2Fpatchwork.ozlabs.org%2Fcover%2F828503%2F&data=02%7C01%7Cfugang
>.duan%40nxp.com%7C087010d8b4784e3c88c608d53015f2ad%7C686ea1d3bc2
>b4c6fa92cd99c5c301635%7C0%7C0%7C636467793180108636&sdata=yY9thX8Q
>CCVteoF5vvUoAYYxGH0gg4wOUq7TQKtkiok%3D&reserved=0
>	https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F
>%2Flkml.org%2Flkml%2F2017%2F10%2F20%2F166&data=02%7C01%7Cfugang.
>duan%40nxp.com%7C087010d8b4784e3c88c608d53015f2ad%7C686ea1d3bc2b
>4c6fa92cd99c5c301635%7C0%7C0%7C636467793180108636&sdata=rxV12dum1
>VmbWLWvSACDuZevFSFbUoWr9AiUtVSsV6w%3D&reserved=0
>
>So I'll implement the phy_reset_after_clk_enable function atop of this patch-
>set and add a note that my patch-series depends on it. Would that be OK?
>
Yes, it is the best solution.

>> 4. then any mac driver can directly call the common
>interface .phy_reset_after_clk_enable().
>
>Sounds reasonable :-)
>
>>
>> That is only my suggestion, maybe there have better idea.
>> Thanks.
>>
>
>Thanks for your quick feedback.
>
>regards
>Richard.L