On 2016-10-24 16:03, Andy Duan wrote:
> From: manfred.schla...@gmx.at <manfred.schla...@gmx.at> Sent: Monday,
> October 24, 2016 5:26 PM
>> To: Andy Duan <fugang.d...@nxp.com>
>> Cc: net...@vger.kernel.org; linux-kernel@vger.kernel.org
>> Subject: [PATCH] net: fec: hard phy reset on open
>>
>> We have seen some problems with auto negotiation on i.MX6 using LAN8720,
>> after interface down/up.
>>
>> In our configuration, the ptp clock is used externally as reference clock for
>> the phy. Some phys (e.g. LAN8720) needs a stable clock while and after hard
>> reset.
>> Before this patch, the driver disabled the clock on close but did no hard
>> reset
>> on open, after enabling the clocks again.
>>
>> A solution that prevents disabling the clocks on close was considered, but
>> discarded because of bad power saving behavior.
>>
>> This patch saves the reset dt properties on probe and does a reset on every
>> open after clocks where enabled, to make sure the clock is stable while and
>> after hard reset.
>>
>> Tested on i.MX6 and i.MX28, both using LAN8720.
>>
>> Signed-off-by: Manfred Schlaegl <manfred.schla...@ginzinger.com>
>> ---
> This patch did hard reset to let phy stable.
> Firstly, you should do reset before clock enable.
I have to disagree here.
The phy demands(datasheet + tests) a stable clock at the time of (hard-)reset
and after this. Therefore the clock has to be enabled before the hard reset.
(This is exactly the reason for the patch.)
Generally: The sense of a reset is to defer the start of digital circuit until
the environment (power, clocks, ...) has stabilized.
Furthermore: Before this patch the hard reset was done in fec_probe. And here
also after the clocks were enabled.
Whats was your argument to do it the other way in this special case?
> Secondly, we suggest to do phy reset in phy driver, not MAC driver.
I was not sure, if you meant a soft-, or hard-reset here.
In case you are talking about soft reset:
Yes, the phy drivers perform a soft reset. Sadly a soft reset is not sufficient
in this case - The phy recovers only on a hard reset from lost clock.
(datasheet + tests)
In case you're talking about hard reset:
Intuitively I would also think, that the (hard-)reset should be handled by the
phy driver, but this is not reality in given implementations.
Documentation/devicetree/bindings/net/fsl-fec.txt says
- phy-reset-gpios : Should specify the gpio for phy reset
It is explicitly talked about phy-reset here. And the (hard-)reset was handled
by the fec driver also before this patch (once on probe).
>
> Regards,
> Andy
Thanks for your feedback!
Best regards,
Manfred
>
>> drivers/net/ethernet/freescale/fec.h | 4 ++
>> drivers/net/ethernet/freescale/fec_main.c | 77 +++++++++++++++++-------
>> -------
>> 2 files changed, 47 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/freescale/fec.h
>> b/drivers/net/ethernet/freescale/fec.h
>> index c865135..379e619 100644
>> --- a/drivers/net/ethernet/freescale/fec.h
>> +++ b/drivers/net/ethernet/freescale/fec.h
>> @@ -498,6 +498,10 @@ struct fec_enet_private {
>> struct clk *clk_enet_out;
>> struct clk *clk_ptp;
>>
>> + int phy_reset;
>> + bool phy_reset_active_high;
>> + int phy_reset_msec;
>> +
>> bool ptp_clk_on;
>> struct mutex ptp_clk_mutex;
>> unsigned int num_tx_queues;
>> diff --git a/drivers/net/ethernet/freescale/fec_main.c
>> b/drivers/net/ethernet/freescale/fec_main.c
>> index 48a033e..8cc1ec5 100644
>> --- a/drivers/net/ethernet/freescale/fec_main.c
>> +++ b/drivers/net/ethernet/freescale/fec_main.c
>> @@ -2802,6 +2802,22 @@ static int fec_enet_alloc_buffers(struct
>> net_device *ndev)
>> return 0;
>> }
>>
>> +static void fec_reset_phy(struct fec_enet_private *fep) {
>> + if (!gpio_is_valid(fep->phy_reset))
>> + return;
>> +
>> + gpio_set_value_cansleep(fep->phy_reset, !!fep-
>>> phy_reset_active_high);
>> +
>> + if (fep->phy_reset_msec > 20)
>> + msleep(fep->phy_reset_msec);
>> + else
>> + usleep_range(fep->phy_reset_msec * 1000,
>> + fep->phy_reset_msec * 1000 + 1000);
>> +
>> + gpio_set_value_cansleep(fep->phy_reset, !fep-
>>> phy_reset_active_high);
>> +}
>> +
>> static int
>> fec_enet_open(struct net_device *ndev)
>> {
>> @@ -2817,6 +2833,8 @@ fec_enet_open(struct net_device *ndev)
>> if (ret)
>> goto clk_enable;
>>
>> + fec_reset_phy(fep);
>> +
>> /* I should reset the ring buffers here, but I don't yet know
>> * a simple way to do that.
>> */
>> @@ -3183,52 +3201,39 @@ static int fec_enet_init(struct net_device *ndev)
>> return 0;
>> }
>>
>> -#ifdef CONFIG_OF
>> -static void fec_reset_phy(struct platform_device *pdev)
>> +static int
>> +fec_get_reset_phy(struct platform_device *pdev, int *msec, int
>> *phy_reset,
>> + bool *active_high)
>> {
>> - int err, phy_reset;
>> - bool active_high = false;
>> - int msec = 1;
>> + int err;
>> struct device_node *np = pdev->dev.of_node;
>>
>> - if (!np)
>> - return;
>> + if (!np || !of_device_is_available(np))
>> + return 0;
>>
>> - of_property_read_u32(np, "phy-reset-duration", &msec);
>> + of_property_read_u32(np, "phy-reset-duration", msec);
>> /* A sane reset duration should not be longer than 1s */
>> - if (msec > 1000)
>> - msec = 1;
>> + if (*msec > 1000)
>> + *msec = 1;
>>
>> - phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0);
>> - if (!gpio_is_valid(phy_reset))
>> - return;
>> + *phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0);
>> + if (!gpio_is_valid(*phy_reset))
>> + return 0;
>>
>> - active_high = of_property_read_bool(np, "phy-reset-active-high");
>> + *active_high = of_property_read_bool(np, "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");
>> + 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;
>> + return err;
>> }
>>
>> - if (msec > 20)
>> - msleep(msec);
>> - else
>> - usleep_range(msec * 1000, msec * 1000 + 1000);
>> -
>> - gpio_set_value_cansleep(phy_reset, !active_high);
>> -}
>> -#else /* CONFIG_OF */
>> -static void fec_reset_phy(struct platform_device *pdev) -{
>> - /*
>> - * In case of platform probe, the reset has been done
>> - * by machine code.
>> - */
>> + return 0;
>> }
>> -#endif /* CONFIG_OF */
>>
>> static void
>> fec_enet_get_queue_num(struct platform_device *pdev, int *num_tx, int
>> *num_rx) @@ -3409,7 +3414,10 @@ fec_probe(struct platform_device
>> *pdev)
>> pm_runtime_set_active(&pdev->dev);
>> pm_runtime_enable(&pdev->dev);
>>
>> - fec_reset_phy(pdev);
>> + ret = fec_get_reset_phy(pdev, &fep->phy_reset_msec, &fep-
>>> phy_reset,
>> + &fep->phy_reset_active_high);
>> + if (ret)
>> + goto failed_reset;
>>
>> if (fep->bufdesc_ex)
>> fec_ptp_init(pdev);
>> @@ -3467,6 +3475,7 @@ fec_probe(struct platform_device *pdev)
>> failed_mii_init:
>> failed_irq:
>> failed_init:
>> +failed_reset:
>> fec_ptp_stop(pdev);
>> if (fep->reg_phy)
>> regulator_disable(fep->reg_phy);
>> --
>> 2.1.4
