>
-----------------------------
Eaton Industries Manufacturing GmbH ~ Registered place of business: Route de la
Longeraie 7, 1110, Morges, Switzerland
-----------------------------
-----Original Message-----
> From: Jörg Willmann <j...@clnt.de>
> Sent: Tuesday, May 19, 2020 10:41 AM
> To: Badel, Laurent <laurentba...@eaton.com>
> Cc: Heiner Kallweit <hkallwe...@gmail.com>; fugang.d...@nxp.com;
> netdev@vger.kernel.org; and...@lunn.ch; f.faine...@gmail.com;
> li...@armlinux.org.uk; richard.leit...@skidata.com;
> da...@davemloft.net; alexander.le...@microsoft.com;
> gre...@linuxfoundation.org; Quette, Arnaud <arnaudque...@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 <hkallwe...@gmail.com>
> >> Sent: Wednesday, April 29, 2020 7:06 PM
> >> To: Badel, Laurent <laurentba...@eaton.com>; fugang.d...@nxp.com;
> >> netdev@vger.kernel.org; and...@lunn.ch; f.faine...@gmail.com;
> >> li...@armlinux.org.uk; richard.leit...@skidata.com;
> >> da...@davemloft.net; alexander.le...@microsoft.com;
> >> gre...@linuxfoundation.org
> >> Cc: Quette, Arnaud <arnaudque...@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 <laurentba...@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;
> >>>
> >
> >
