Re: [PATCH] net: fec: hard phy reset on open

[Manfred Schlaegl]

On 2016-10-24 16:03, Andy Duan wrote:
> From: manfred.schlaegl@xxxxxx <manfred.schlaegl@xxxxxx>  Sent: Monday, October 24, 2016 5:26 PM
>> To: Andy Duan <fugang.duan@xxxxxxx>
>> Cc: netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
>> 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.schlaegl@xxxxxxxxxxxxx>
>> ---

> 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