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

[Andy Duan <fugang.duan@nxp.com>]

From: Manfred Schlaegl <manfred.schlaegl@ginzinger.com> Sent: Monday, October 24, 2016 10:43 PM
> To: Andy Duan <fugang.duan@nxp.com>
> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] net: fec: hard phy reset on open
> 
> On 2016-10-24 16:03, Andy Duan wrote:
> > From: manfred.schlaegl@gmx.at <manfred.schlaegl@gmx.at>  Sent:
> Monday,
> > October 24, 2016 5:26 PM
> >> To: Andy Duan <fugang.duan@nxp.com>
> >> Cc: netdev@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.schlaegl@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?
> 
I check some different vendor phy, hard reset assert after clock stable.
But I still don't ensure all phys are this action. 

> > 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).
> 
I suggest to do phy hard reset in phy driver like:
drivers/net/phy/spi_ks8995.c:

and Uwe Kleine-König's patch "phy: add support for a reset-gpio specification" (I don't know why the patch is reverted now.)
	
Regards,
Andy
> >
> > 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
>