All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] net: ethernet: fsl: don't en/disable refclk on open/close
@ 2017-10-22 13:11 Richard Leitner
  2017-10-22 18:31 ` Florian Fainelli
  0 siblings, 1 reply; 6+ messages in thread
From: Richard Leitner @ 2017-10-22 13:11 UTC (permalink / raw)
  To: fugang.duan; +Cc: netdev, linux-kernel, Richard Leitner, stable

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

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

Some PHYs (for example the LAN8710) doesn't allow turning the ethernet
ref clocks off and on again without reset (according to their datasheet).
Exactly this behaviour was introduced for power saving reasons by
commit e8fcfcd5684a ("net: fec: optimize the clock management to save...")
Therefore remove those en/disables of the refclk during probe, open and
close of the fec.

Generally speaking is this issue only be 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.MX6 SoC. The typical symptom of this problem is a "swinging" ethernet
link. Similar issues were experienced 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
thousand PORs of our board.

Fixes: e8fcfcd5684a ("net: fec: optimize the clock management to sa...")
Cc: stable@vger.kernel.org
Signed-off-by: Richard Leitner <richard.leitner@skidata.com>
---
 drivers/net/ethernet/freescale/fec_main.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 3dc2d771a222..8f696b53d8b8 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -2844,9 +2844,6 @@ fec_enet_open(struct net_device *ndev)
 		return ret;
 
 	pinctrl_pm_select_default_state(&fep->pdev->dev);
-	ret = fec_enet_clk_enable(ndev, true);
-	if (ret)
-		goto clk_enable;
 
 	/* I should reset the ring buffers here, but I don't yet know
 	 * a simple way to do that.
@@ -2879,8 +2876,6 @@ fec_enet_open(struct net_device *ndev)
 err_enet_mii_probe:
 	fec_enet_free_buffers(ndev);
 err_enet_alloc:
-	fec_enet_clk_enable(ndev, false);
-clk_enable:
 	pm_runtime_mark_last_busy(&fep->pdev->dev);
 	pm_runtime_put_autosuspend(&fep->pdev->dev);
 	pinctrl_pm_select_sleep_state(&fep->pdev->dev);
@@ -2907,7 +2902,6 @@ fec_enet_close(struct net_device *ndev)
 
 	fec_enet_update_ethtool_stats(ndev);
 
-	fec_enet_clk_enable(ndev, false);
 	pinctrl_pm_select_sleep_state(&fep->pdev->dev);
 	pm_runtime_mark_last_busy(&fep->pdev->dev);
 	pm_runtime_put_autosuspend(&fep->pdev->dev);
@@ -3495,7 +3489,6 @@ fec_probe(struct platform_device *pdev)
 
 	/* Carrier starts down, phylib will bring it up */
 	netif_carrier_off(ndev);
-	fec_enet_clk_enable(ndev, false);
 	pinctrl_pm_select_sleep_state(&pdev->dev);
 
 	ret = register_netdev(ndev);
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] net: ethernet: fsl: don't en/disable refclk on open/close
  2017-10-22 13:11 [PATCH] net: ethernet: fsl: don't en/disable refclk on open/close Richard Leitner
@ 2017-10-22 18:31 ` Florian Fainelli
  2017-10-22 22:30   ` Richard Leitner
  0 siblings, 1 reply; 6+ messages in thread
From: Florian Fainelli @ 2017-10-22 18:31 UTC (permalink / raw)
  To: Richard Leitner, fugang.duan
  Cc: netdev, linux-kernel, Richard Leitner, stable



On 10/22/2017 06:11 AM, Richard Leitner wrote:
> From: Richard Leitner <richard.leitner@skidata.com>
> 
> From: Richard Leitner <richard.leitner@skidata.com>
> 
> Some PHYs (for example the LAN8710) doesn't allow turning the ethernet
> ref clocks off and on again without reset (according to their datasheet).
> Exactly this behaviour was introduced for power saving reasons by
> commit e8fcfcd5684a ("net: fec: optimize the clock management to save...")

Please don't truncate the commit subject, put it in full length.

> Therefore remove those en/disables of the refclk during probe, open and
> close of the fec.
> 
> Generally speaking is this issue only be 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.MX6 SoC. The typical symptom of this problem is a "swinging" ethernet
> link. Similar issues were experienced 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
> thousand PORs of our board.

FWIW, this is a common problem on most Ethernet systems attached to a
PHY because of the clock dependencies between the MAC and PHY, in either
direction really. We recently stumbled across a similar problem on the
MAC side (using bcmgenet) where after shutting down the PHY in
ndo_stop() we would not get enough clock cycles for the MAC to cleanly
shutdown and upon subsequent ndo_open() we could get the MAC in an
invalid state.

> 
> Fixes: e8fcfcd5684a ("net: fec: optimize the clock management to sa...")

Likewise here, not truncation of the commit subject necessary.

> Cc: stable@vger.kernel.org

netdev patches are handled directly by David Miller, see netdev-FAQ for
details.

> Signed-off-by: Richard Leitner <richard.leitner@skidata.com>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 7 -------
>  1 file changed, 7 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index 3dc2d771a222..8f696b53d8b8 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -2844,9 +2844,6 @@ fec_enet_open(struct net_device *ndev)
>  		return ret;
>  
>  	pinctrl_pm_select_default_state(&fep->pdev->dev);
> -	ret = fec_enet_clk_enable(ndev, true);
> -	if (ret)
> -		goto clk_enable;
>  
>  	/* I should reset the ring buffers here, but I don't yet know
>  	 * a simple way to do that.
> @@ -2879,8 +2876,6 @@ fec_enet_open(struct net_device *ndev)
>  err_enet_mii_probe:
>  	fec_enet_free_buffers(ndev);
>  err_enet_alloc:
> -	fec_enet_clk_enable(ndev, false);
> -clk_enable:
>  	pm_runtime_mark_last_busy(&fep->pdev->dev);
>  	pm_runtime_put_autosuspend(&fep->pdev->dev);
>  	pinctrl_pm_select_sleep_state(&fep->pdev->dev);
> @@ -2907,7 +2902,6 @@ fec_enet_close(struct net_device *ndev)
>  
>  	fec_enet_update_ethtool_stats(ndev);
>  
> -	fec_enet_clk_enable(ndev, false);
>  	pinctrl_pm_select_sleep_state(&fep->pdev->dev);
>  	pm_runtime_mark_last_busy(&fep->pdev->dev);
>  	pm_runtime_put_autosuspend(&fep->pdev->dev);
> @@ -3495,7 +3489,6 @@ fec_probe(struct platform_device *pdev)
>  
>  	/* Carrier starts down, phylib will bring it up */
>  	netif_carrier_off(ndev);
> -	fec_enet_clk_enable(ndev, false);
>  	pinctrl_pm_select_sleep_state(&pdev->dev);
>  
>  	ret = register_netdev(ndev);
> 

-- 
Florian

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] net: ethernet: fsl: don't en/disable refclk on open/close
  2017-10-22 18:31 ` Florian Fainelli
@ 2017-10-22 22:30   ` Richard Leitner
  2017-10-22 22:48     ` Florian Fainelli
  0 siblings, 1 reply; 6+ messages in thread
From: Richard Leitner @ 2017-10-22 22:30 UTC (permalink / raw)
  To: Florian Fainelli, Richard Leitner, fugang.duan
  Cc: netdev, linux-kernel, Richard Leitner, stable


On 10/22/2017 08:31 PM, Florian Fainelli wrote:
> 
> On 10/22/2017 06:11 AM, Richard Leitner wrote:
>> From: Richard Leitner <richard.leitner@skidata.com>
>>
>> From: Richard Leitner <richard.leitner@skidata.com>
>>
>> Some PHYs (for example the LAN8710) doesn't allow turning the ethernet
>> ref clocks off and on again without reset (according to their datasheet).
>> Exactly this behaviour was introduced for power saving reasons by
>> commit e8fcfcd5684a ("net: fec: optimize the clock management to save...")
> 
> Please don't truncate the commit subject, put it in full length.

Ok. So I can then ignore the checkpatch.pl error complaining about too 
long lines?

> 
>> Therefore remove those en/disables of the refclk during probe, open and
>> close of the fec.
>>
>> Generally speaking is this issue only be 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.MX6 SoC. The typical symptom of this problem is a "swinging" ethernet
>> link. Similar issues were experienced 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
>> thousand PORs of our board.
> 
> FWIW, this is a common problem on most Ethernet systems attached to a
> PHY because of the clock dependencies between the MAC and PHY, in either
> direction really. We recently stumbled across a similar problem on the
> MAC side (using bcmgenet) where after shutting down the PHY in
> ndo_stop() we would not get enough clock cycles for the MAC to cleanly
> shutdown and upon subsequent ndo_open() we could get the MAC in an
> invalid state.

Hmm... So I'm not the only one struggling with this... Was there already 
discussion/ideas on how to solve this issue once and for all across the 
kernel?

Andrew Lunn suggested to make the PHY driver a clock driver and let it 
export the refclk... But IMHO that "feels" a bit strange. Due to the 
fact in our case the clock is generated by the SoC and the PHY is an 
external chip which only consumes it.

But back to this patch: Is it OK the way it fixes the issue?

> 
>>
>> Fixes: e8fcfcd5684a ("net: fec: optimize the clock management to sa...")
> 
> Likewise here, not truncation of the commit subject necessary.
> 
>> Cc: stable@vger.kernel.org
> 
> netdev patches are handled directly by David Miller, see netdev-FAQ for
> details.

Ok. So I shouldn't post them to stable@vger.kernel.org at all?

> 
>> Signed-off-by: Richard Leitner <richard.leitner@skidata.com>
>> ---
>>   drivers/net/ethernet/freescale/fec_main.c | 7 -------
>>   1 file changed, 7 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
>> index 3dc2d771a222..8f696b53d8b8 100644
>> --- a/drivers/net/ethernet/freescale/fec_main.c
>> +++ b/drivers/net/ethernet/freescale/fec_main.c
>> @@ -2844,9 +2844,6 @@ fec_enet_open(struct net_device *ndev)
>>   		return ret;
>>   
>>   	pinctrl_pm_select_default_state(&fep->pdev->dev);
>> -	ret = fec_enet_clk_enable(ndev, true);
>> -	if (ret)
>> -		goto clk_enable;
>>   
>>   	/* I should reset the ring buffers here, but I don't yet know
>>   	 * a simple way to do that.
>> @@ -2879,8 +2876,6 @@ fec_enet_open(struct net_device *ndev)
>>   err_enet_mii_probe:
>>   	fec_enet_free_buffers(ndev);
>>   err_enet_alloc:
>> -	fec_enet_clk_enable(ndev, false);
>> -clk_enable:
>>   	pm_runtime_mark_last_busy(&fep->pdev->dev);
>>   	pm_runtime_put_autosuspend(&fep->pdev->dev);
>>   	pinctrl_pm_select_sleep_state(&fep->pdev->dev);
>> @@ -2907,7 +2902,6 @@ fec_enet_close(struct net_device *ndev)
>>   
>>   	fec_enet_update_ethtool_stats(ndev);
>>   
>> -	fec_enet_clk_enable(ndev, false);
>>   	pinctrl_pm_select_sleep_state(&fep->pdev->dev);
>>   	pm_runtime_mark_last_busy(&fep->pdev->dev);
>>   	pm_runtime_put_autosuspend(&fep->pdev->dev);
>> @@ -3495,7 +3489,6 @@ fec_probe(struct platform_device *pdev)
>>   
>>   	/* Carrier starts down, phylib will bring it up */
>>   	netif_carrier_off(ndev);
>> -	fec_enet_clk_enable(ndev, false);
>>   	pinctrl_pm_select_sleep_state(&pdev->dev);
>>   
>>   	ret = register_netdev(ndev);
>>
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] net: ethernet: fsl: don't en/disable refclk on open/close
  2017-10-22 22:30   ` Richard Leitner
@ 2017-10-22 22:48     ` Florian Fainelli
  2017-10-22 23:01       ` Richard Leitner
  0 siblings, 1 reply; 6+ messages in thread
From: Florian Fainelli @ 2017-10-22 22:48 UTC (permalink / raw)
  To: Richard Leitner, Richard Leitner, fugang.duan
  Cc: netdev, linux-kernel, Richard Leitner, Andrew Lunn



On 10/22/2017 03:30 PM, Richard Leitner wrote:
> 
> On 10/22/2017 08:31 PM, Florian Fainelli wrote:
>>
>> On 10/22/2017 06:11 AM, Richard Leitner wrote:
>>> From: Richard Leitner <richard.leitner@skidata.com>
>>>
>>> From: Richard Leitner <richard.leitner@skidata.com>
>>>
>>> Some PHYs (for example the LAN8710) doesn't allow turning the ethernet
>>> ref clocks off and on again without reset (according to their
>>> datasheet).
>>> Exactly this behaviour was introduced for power saving reasons by
>>> commit e8fcfcd5684a ("net: fec: optimize the clock management to
>>> save...")
>>
>> Please don't truncate the commit subject, put it in full length.
> 
> Ok. So I can then ignore the checkpatch.pl error complaining about too
> long lines?

Yes absolutely it's just a tool, not the absolute reference.

> 
>>
>>> Therefore remove those en/disables of the refclk during probe, open and
>>> close of the fec.
>>>
>>> Generally speaking is this issue only be 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.MX6 SoC. The typical symptom of this problem is a "swinging" ethernet
>>> link. Similar issues were experienced 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
>>> thousand PORs of our board.
>>
>> FWIW, this is a common problem on most Ethernet systems attached to a
>> PHY because of the clock dependencies between the MAC and PHY, in either
>> direction really. We recently stumbled across a similar problem on the
>> MAC side (using bcmgenet) where after shutting down the PHY in
>> ndo_stop() we would not get enough clock cycles for the MAC to cleanly
>> shutdown and upon subsequent ndo_open() we could get the MAC in an
>> invalid state.
> 
> Hmm... So I'm not the only one struggling with this... Was there already
> discussion/ideas on how to solve this issue once and for all across the
> kernel?

Not really, there is not unfortunately a one size fits all solution.

> 
> Andrew Lunn suggested to make the PHY driver a clock driver and let it
> export the refclk... But IMHO that "feels" a bit strange. Due to the
> fact in our case the clock is generated by the SoC and the PHY is an
> external chip which only consumes it.

It depends which *MII mode you use. Unless you are using reverse MII,
the PHY supplies the RX clock and the MAC supplies the RX clock, so
solving this for all modes becomes quite tricky. Andrew's suggestion
makes sense, but considering that the MAC and PHY need to be connected
to each other, it may be tricky to request the PHY's clock before you
actually attached to the PHY...

> 
> But back to this patch: Is it OK the way it fixes the issue?

Fugang and Andrew probably know this hardware a lot better, I would have
to look at the code path a bit more to understand if an alternative
solution is possible. It sounds like your patch could create a power
consumption regression, so maybe add a check for the PHY ID that is
problematic by doing something like:

if (priv->phydev->drv && priv->phydev->drv->phy_id == XXXX_XXXX) where
XXXX_XXXX is the LAN870 PHY ID (obtained from MII register 2 & 3).

> 
>>
>>>
>>> Fixes: e8fcfcd5684a ("net: fec: optimize the clock management to sa...")
>>
>> Likewise here, not truncation of the commit subject necessary.
>>
>>> Cc: stable@vger.kernel.org
>>
>> netdev patches are handled directly by David Miller, see netdev-FAQ for
>> details.
> 
> Ok. So I shouldn't post them to stable@vger.kernel.org at all?

Nope.

> 
>>
>>> Signed-off-by: Richard Leitner <richard.leitner@skidata.com>
>>> ---
>>>   drivers/net/ethernet/freescale/fec_main.c | 7 -------
>>>   1 file changed, 7 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/freescale/fec_main.c
>>> b/drivers/net/ethernet/freescale/fec_main.c
>>> index 3dc2d771a222..8f696b53d8b8 100644
>>> --- a/drivers/net/ethernet/freescale/fec_main.c
>>> +++ b/drivers/net/ethernet/freescale/fec_main.c
>>> @@ -2844,9 +2844,6 @@ fec_enet_open(struct net_device *ndev)
>>>           return ret;
>>>         pinctrl_pm_select_default_state(&fep->pdev->dev);
>>> -    ret = fec_enet_clk_enable(ndev, true);
>>> -    if (ret)
>>> -        goto clk_enable;
>>>         /* I should reset the ring buffers here, but I don't yet know
>>>        * a simple way to do that.
>>> @@ -2879,8 +2876,6 @@ fec_enet_open(struct net_device *ndev)
>>>   err_enet_mii_probe:
>>>       fec_enet_free_buffers(ndev);
>>>   err_enet_alloc:
>>> -    fec_enet_clk_enable(ndev, false);
>>> -clk_enable:
>>>       pm_runtime_mark_last_busy(&fep->pdev->dev);
>>>       pm_runtime_put_autosuspend(&fep->pdev->dev);
>>>       pinctrl_pm_select_sleep_state(&fep->pdev->dev);
>>> @@ -2907,7 +2902,6 @@ fec_enet_close(struct net_device *ndev)
>>>         fec_enet_update_ethtool_stats(ndev);
>>>   -    fec_enet_clk_enable(ndev, false);
>>>       pinctrl_pm_select_sleep_state(&fep->pdev->dev);
>>>       pm_runtime_mark_last_busy(&fep->pdev->dev);
>>>       pm_runtime_put_autosuspend(&fep->pdev->dev);
>>> @@ -3495,7 +3489,6 @@ fec_probe(struct platform_device *pdev)
>>>         /* Carrier starts down, phylib will bring it up */
>>>       netif_carrier_off(ndev);
>>> -    fec_enet_clk_enable(ndev, false);
>>>       pinctrl_pm_select_sleep_state(&pdev->dev);
>>>         ret = register_netdev(ndev);
>>>
>>

-- 
Florian

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] net: ethernet: fsl: don't en/disable refclk on open/close
  2017-10-22 22:48     ` Florian Fainelli
@ 2017-10-22 23:01       ` Richard Leitner
  2017-10-26  8:28         ` Richard Leitner
  0 siblings, 1 reply; 6+ messages in thread
From: Richard Leitner @ 2017-10-22 23:01 UTC (permalink / raw)
  To: Florian Fainelli, Richard Leitner, fugang.duan
  Cc: netdev, linux-kernel, Richard Leitner, Andrew Lunn

On 10/23/2017 12:48 AM, Florian Fainelli wrote> On 10/22/2017 03:30 PM, 
Richard Leitner wrote:
>> On 10/22/2017 08:31 PM, Florian Fainelli wrote:
>>> On 10/22/2017 06:11 AM, Richard Leitner wrote:

...

>> Andrew Lunn suggested to make the PHY driver a clock driver and let it
>> export the refclk... But IMHO that "feels" a bit strange. Due to the
>> fact in our case the clock is generated by the SoC and the PHY is an
>> external chip which only consumes it.
> 
> It depends which *MII mode you use. Unless you are using reverse MII,
> the PHY supplies the RX clock and the MAC supplies the RX clock, so
> solving this for all modes becomes quite tricky. Andrew's suggestion
> makes sense, but considering that the MAC and PHY need to be connected
> to each other, it may be tricky to request the PHY's clock before you
> actually attached to the PHY...
>>
>> But back to this patch: Is it OK the way it fixes the issue?
> 
> Fugang and Andrew probably know this hardware a lot better, I would have
> to look at the code path a bit more to understand if an alternative
> solution is possible. It sounds like your patch could create a power
> consumption regression, so maybe add a check for the PHY ID that is
> problematic by doing something like:
> 
> if (priv->phydev->drv && priv->phydev->drv->phy_id == XXXX_XXXX) where
> XXXX_XXXX is the LAN870 PHY ID (obtained from MII register 2 & 3).

I already had a patch which does this check and triggers a reset of the 
PHY after the refclk was enabled (in fec_enet_clk_enable) in case the 
phy_id matches. This would IMHO avoid all power consumption 
regressions... It was something like:

switch (priv->phydev->drv->phy_id) {
case XXXX:
case XXXX:
	ret = fec_reset_phy(ndev);
	if (ret)
		goto failed_reset;
	if (ndev->phydev) {
		ret = phy_init_hw(ndev->phydev);
		if (ret)
			goto failed_reset;
	}
}

Would that be better?

Basically I'm fine with any kind of solution... It's up to you guys... 
You're the experts ;-)

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] net: ethernet: fsl: don't en/disable refclk on open/close
  2017-10-22 23:01       ` Richard Leitner
@ 2017-10-26  8:28         ` Richard Leitner
  0 siblings, 0 replies; 6+ messages in thread
From: Richard Leitner @ 2017-10-26  8:28 UTC (permalink / raw)
  To: Florian Fainelli, fugang.duan
  Cc: Richard Leitner, netdev, linux-kernel, Andrew Lunn, Richard Leitner


On 10/23/2017 01:01 AM, Richard Leitner wrote:
> On 10/23/2017 12:48 AM, Florian Fainelli wrote
>> On 10/22/2017 03:30 PM, Richard Leitner wrote:
>>> On 10/22/2017 08:31 PM, Florian Fainelli wrote:
>>>> On 10/22/2017 06:11 AM, Richard Leitner wrote:

...

>>> But back to this patch: Is it OK the way it fixes the issue?
>>
>> Fugang and Andrew probably know this hardware a lot better, I would have
>> to look at the code path a bit more to understand if an alternative
>> solution is possible. It sounds like your patch could create a power
>> consumption regression, so maybe add a check for the PHY ID that is
>> problematic by doing something like:
>>
>> if (priv->phydev->drv && priv->phydev->drv->phy_id == XXXX_XXXX) where
>> XXXX_XXXX is the LAN870 PHY ID (obtained from MII register 2 & 3).
> 
> I already had a patch which does this check and triggers a reset of the 
> PHY after the refclk was enabled (in fec_enet_clk_enable) in case the 
> phy_id matches. This would IMHO avoid all power consumption 
> regressions... It was something like:
> 
> switch (priv->phydev->drv->phy_id) {
> case XXXX:
> case XXXX:
>      ret = fec_reset_phy(ndev);
>      if (ret)
>          goto failed_reset;
>      if (ndev->phydev) {
>          ret = phy_init_hw(ndev->phydev);
>          if (ret)
>              goto failed_reset;
>      }
> }

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);
	if (ret)
		goto failed_reset;
	if (ndev->phydev) {
		ret = phy_init_hw(ndev->phydev);
		if (ret)
			goto failed_reset;
	}
}

This approach would IMHO also be easier and less prune to erros for 
porting to other MAC drivers (if needed).

So what would be the better approach in your opinion and how should I 
procede here?

kind regards,
Richard.L

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2017-10-26  8:38 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-22 13:11 [PATCH] net: ethernet: fsl: don't en/disable refclk on open/close Richard Leitner
2017-10-22 18:31 ` Florian Fainelli
2017-10-22 22:30   ` Richard Leitner
2017-10-22 22:48     ` Florian Fainelli
2017-10-22 23:01       ` Richard Leitner
2017-10-26  8:28         ` Richard Leitner

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.