RE: [EXTERNAL] Re: [PATCH 2/2] Reset PHY in phy_init_hw() before interrupt configuration

["Badel, Laurent" <LaurentBadel@eaton.com>]



> 

-----------------------------
Eaton Industries Manufacturing GmbH ~ Registered place of business: Route de la Longeraie 7, 1110, Morges, Switzerland 

-----------------------------

-----Original Message-----
> From: Jörg Willmann <joe@clnt.de>
> Sent: Tuesday, May 19, 2020 10:41 AM
> To: Badel, Laurent <LaurentBadel@eaton.com>
> Cc: Heiner Kallweit <hkallweit1@gmail.com>; fugang.duan@nxp.com;
> netdev@vger.kernel.org; andrew@lunn.ch; f.fainelli@gmail.com;
> linux@armlinux.org.uk; richard.leitner@skidata.com;
> davem@davemloft.net; alexander.levin@microsoft.com;
> gregkh@linuxfoundation.org; Quette, Arnaud <ArnaudQuette@Eaton.com>
> Subject: RE: [EXTERNAL] Re: [PATCH 2/2] Reset PHY in phy_init_hw() before
> interrupt configuration
> 
> 
> 
> On Thu, 30 Apr 2020, Badel, Laurent wrote:
> 
> > -----Original Message-----
> >> From: Heiner Kallweit <hkallweit1@gmail.com>
> >> Sent: Wednesday, April 29, 2020 7:06 PM
> >> To: Badel, Laurent <LaurentBadel@eaton.com>; fugang.duan@nxp.com;
> >> netdev@vger.kernel.org; andrew@lunn.ch; f.fainelli@gmail.com;
> >> linux@armlinux.org.uk; richard.leitner@skidata.com;
> >> davem@davemloft.net; alexander.levin@microsoft.com;
> >> gregkh@linuxfoundation.org
> >> Cc: Quette, Arnaud <ArnaudQuette@Eaton.com>
> >> Subject: [EXTERNAL] Re: [PATCH 2/2] Reset PHY in phy_init_hw() before
> >> interrupt configuration
> >>
> >> On 29.04.2020 11:03, Badel, Laurent wrote:
> >>> Description: this patch adds a reset of the PHY in phy_init_hw()
> >>> for PHY drivers bearing the PHY_RST_AFTER_CLK_EN flag.
> >>>
> >>> Rationale: due to the PHY reset reverting the interrupt mask to
> >>> default, it is necessary to either perform the reset before PHY
> >>> configuration, or re-configure the PHY after reset. This patch
> >>> implements the former as it is simpler and more generic.
> >>>
> >>> Fixes: 1b0a83ac04e383e3bed21332962b90710fcf2828 ("net: fec: add
> >> phy_reset_after_clk_enable() support")
> >>> Signed-off-by: Laurent Badel <laurentbadel@eaton.com>
> >>>
> >>> ---
> >>>  drivers/net/phy/phy_device.c | 7 +++++--
> >>>  1 file changed, 5 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/net/phy/phy_device.c
> >>> b/drivers/net/phy/phy_device.c index 28e3c5c0e..2cc511364 100644
> >>> --- a/drivers/net/phy/phy_device.c
> >>> +++ b/drivers/net/phy/phy_device.c
> >>> @@ -1082,8 +1082,11 @@ int phy_init_hw(struct phy_device *phydev)
> {
> >>>  	int ret = 0;
> >>>
> >>> -	/* Deassert the reset signal */
> >>> -	phy_device_reset(phydev, 0);
> >>> +	/* Deassert the reset signal
> >>> +	 * If the PHY needs a reset, do it now
> >>> +	 */
> >>> +	if (!phy_reset_after_clk_enable(phydev))
> >>
> >> If reset is asserted when entering phy_init_hw(), then
> >> phy_reset_after_clk_enable() basically becomes a no-op.
> >> Still it should work as expected due to the reset signal being
> >> deasserted. It would be worth describing in the comment why the code
> >> still works in this case.
> >>
> >
> > Thank you for the comment, this is a very good point.
> > I will make sure to include some description when resubmitting.
> > I had previously tested this and what I saw was that the first time
> > you bring up the interface, the reset is not asserted so that
> > phy_reset_after_clk_enable() is effective.
> > The subsequent times the interface is brought up, the reset is already
> > asserted when entering phy_init_hw(), so that it becomes a no-op as
> > you correctly pointed out. However, that didn't cause any problem on
> > my board, presumably because in that case the clock is already running
> > when the PHY comes out of reset.
> > I will re-test this carefully against the 'net' tree, though, before
> > coming to conclusions.
> >
> I have two additional things to take into account:
> * phy_reset_after_clk_enable() shoulnd't be longer called that way since it is
> now misleading -> the phy is no longer reset after clock enable but during
> hw_init()
> * how about fec_resume()? I don't think hw_init() is called then and so
> phy_reset_after_clk_enable() will no longer be called.
> 

My apologies, in the midst of modifying and preparing a new patch for this issue
I now realize that I seem to have failed to reply to your comments. Thank you
very much for your time and efforts reviewing the patch; I have taken these into
account and indeed there also seems to be an issue with fec_resume that I have
now addressed.

> >>> +		phy_device_reset(phydev, 0);
> >>>
> >>>  	if (!phydev->drv)
> >>>  		return 0;
> >>>
> >
> >