Date:   Mon, 13 Dec 2021 04:39:15 +0000
From:   Joakim Zhang <qiangqing.zhang@....com>
To:     Philippe Schenker <philippe.schenker@...adex.com>,
        "festevam@...il.com" <festevam@...il.com>,
        "linux@...linux.org.uk" <linux@...linux.org.uk>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "andrew@...n.ch" <andrew@...n.ch>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [RFC PATCH 0/2] Reset PHY in fec_resume if it got powered down


Hi Philippe,

> -----Original Message-----
> From: Philippe Schenker <philippe.schenker@...adex.com>
> Sent: 2021年12月10日 21:51
> To: festevam@...il.com; linux@...linux.org.uk; kuba@...nel.org;
> netdev@...r.kernel.org; davem@...emloft.net; Joakim Zhang
> <qiangqing.zhang@....com>; andrew@...n.ch
> Cc: linux-kernel@...r.kernel.org
> Subject: Re: [RFC PATCH 0/2] Reset PHY in fec_resume if it got powered
> down
> 
> On Tue, 2021-12-07 at 01:58 +0000, Joakim Zhang wrote:
> >
> > Hi Philippe,
> >
> > > -----Original Message-----
> > > From: Philippe Schenker <philippe.schenker@...adex.com>
> > > Sent: 2021年12月6日 18:13
> > > To: netdev@...r.kernel.org; Joakim Zhang
> <qiangqing.zhang@....com>;
> > > Fabio Estevam <festevam@...il.com>; Fugang Duan
> > > <fugang.duan@....com>; David S . Miller <davem@...emloft.net>;
> > > Russell King <linux@...linux.org.uk>; Andrew Lunn <andrew@...n.ch>;
> > > Jakub Kicinski <kuba@...nel.org>
> > > Cc: Philippe Schenker <philippe.schenker@...adex.com>;
> > > linux-kernel@...r.kernel.org
> > > Subject: [RFC PATCH 0/2] Reset PHY in fec_resume if it got powered
> > > down
> > >
> > >
> > > If a hardware-design is able to control power to the Ethernet PHY
> > > and relying on software to do a reset, the PHY does no longer work
> > > after resuming from suspend, given the PHY does need a
> > > hardware-reset.
> > > The Freescale fec driver does currently control the reset-signal of
> > > a phy but does not issue a reset on resume.
> > >
> > > On Toradex Apalis iMX8 board we do have such a design where we also
> > > don't place the RC circuit to delay the reset-line by hardware.
> > > Hence we fully rely on software to do so.
> > > Since I didn't manage to get the needed parts of Apalis iMX8 working
> > > with mainline this patchset was only tested on the downstream kernel
> > > toradex_5.4-2.3.x-imx. [1] This kernel is based on NXP's release
> > > imx_5.4.70_2.3.0. [2] The affected code is still the same on
> > > mainline kernel, which would actually make me comfortable merging
> > > this patch, but due to this fact I'm sending this as RFC maybe
> > > someone else is able to test this code.
> > >
> > > This patchset aims to change the behavior by resetting the ethernet
> > > PHY in fec_resume. A short description of the patches can be found
> > > below, please find a detailed description in the commit-messages of
> > > the respective patches.
> > >
> > > [PATCH 2/2] net: fec: reset phy in resume if it was powered down
> > >
> > > This patch calls fec_reset_phy just after regulator enable in
> > > fec_resume, when the phy is resumed
> > >
> > > [PATCH 1/2] net: fec: make fec_reset_phy not only usable once
> > >
> > > This patch prepares the function fec_reset_phy to be called multiple
> > > times. It stores the data around the reset-gpio in fec_enet_private.
> > > This patch aims to do no functional changes.
> > >
> > > [1]
> > > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fgit
> > > .tor%2F&amp;data=04%7C01%7Cqiangqing.zhang%40nxp.com%7Ca43416
> 6e62464
> > >
> c4b69c408d9bbe420ef%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7
> C637
> > >
> 747410747358279%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAi
> LCJQIjoi
> > >
> V2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=8u1EudB0
> aDkC
> > > K9tzrqVznAWY6mDgBIHqIQCyDWrsH4g%3D&amp;reserved=0
> > >
> adex.com%2Fcgit%2Flinux-toradex.git%2Flog%2F%3Fh%3Dtoradex_5.4-2.3.x
> > >
> -imx&amp;data=04%7C01%7Cqiangqing.zhang%40nxp.com%7Cf3c138ed9232
> > >
> 4a8d75e708d9b8a11b9a%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%
> > >
> 7C637743824364193423%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAw
> > >
> MDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sda
> > >
> ta=Bw%2BZdqhAjPXqKJFZCXp0mtId1x9mkX6f6MW2ky6U1ww%3D&amp;res
> > > erved=0
> > > [2]
> > >
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fso
> > > urc
> > >
> e.codeaurora.org%2Fexternal%2Fimx%2Flinux-imx%2Flog%2F%3Fh%3Dimx_
> > >
> 5.4.70_2.3.0&amp;data=04%7C01%7Cqiangqing.zhang%40nxp.com%7Cf3c13
> > >
> 8ed92324a8d75e708d9b8a11b9a%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C
> > >
> 0%7C0%7C637743824364193423%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiM
> > >
> C4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000
> > >
> &amp;sdata=of9z9hfVhHakVScLxCdEo%2BXmd2B9Ad9X8Rry6GjEZE4%3D&a
> > > mp;reserved=0
> > >
> >
> > In fec driver, it has supported hardware reset for PHY when MAC resume
> > back,
> >
> > fec_resume() -> phy_init_hw() -> phy_device_reset() de-assert the
> > reset signal, you only need implement the properties which PHY core
> > provided.
> >
> > I think you should not use deprecated reset properties provided by fec
> > driver, instead the common reset properties provided by PHY core.
> >
> > Please check the dt-bindings for more details:
> > Documentation/devicetree/bindings/net/fsl,fec.yaml
> > Documentation/devicetree/bindings/net/ethernet-phy.yaml
> >
> > Best Regards,
> > Joakim Zhang
> 
> Hi Joakim and many thanks for that hint! I tried that out but unfortunately it
> still does not work due to phy_init_hw() only deasserting the reset. For that
> to work, for example a call of phy_device_reset(ndev->phydev, 1); in
> fec_suspend or in fec_resume before enabling the supply would work, in
> order to assert that reset.

Yes, you are right. It only de-assert the reset in phy_init_hw(), should not enough for you.

It seems that phy reset would not be performed during suspend/resume in the phylib.
AFAIK, a hardware reset automatically generated when power on for most PHYs, the PHY
you used may be special, and not sure why phylib has not taken reset into suspend/resume scenario，
only perform PHY suspend/resume.
 
> I see now two ways to go to fix our issue:
> 
> 1. Assert the phy-reset gpio in fec_suspend() or fec_resume()
> 
> 2. Add support for regulators in drivers/net/phy/phy-core.c and handle the
> phy-reset properly in there with assert-us and deassert-us delays.
> 
> As you probably have a much better overview: Do you see another possibility
> to handle phy-reset after resuming? Or which way shall I choose to go
> forward?

This should be a common issue for specific PHY, and I prefer to performing reset in phylib.

Could you please take a look at phy_reset_after_clk_enable()? Is it possible to add a function
like phy_reset_after_power_on() (adding extra macro, e.g. PHY_RST_AFTER_POWER_ON) for these PHYs?

Best Regards,
Joakim Zhang
> Thanks in advance for any advice
> Philippe
> 
> > > Philippe Schenker (2):
> > >   net: fec: make fec_reset_phy not only usable once
> > >   net: fec: reset phy in resume if it was powered down
> > >
> > >  drivers/net/ethernet/freescale/fec.h      |  6 ++
> > >  drivers/net/ethernet/freescale/fec_main.c | 98
> ++++++++++++++++----
> > > ---
> > >  2 files changed, 73 insertions(+), 31 deletions(-)
> > >
> > > --
> > > 2.34.0
> >

Powered by blists - more mailing lists