Re: [RFC PATCH 0/2] Reset PHY in fec_resume if it got powered down

[Philippe Schenker <philippe.schenker@toradex.com>]

On Tue, 2021-12-07 at 01:58 +0000, Joakim Zhang wrote:
> 
> Hi Philippe,
> 
> > -----Original Message-----
> > From: Philippe Schenker <philippe.schenker@toradex.com>
> > Sent: 2021年12月6日 18:13
> > To: netdev@vger.kernel.org; Joakim Zhang <qiangqing.zhang@nxp.com>;
> > Fabio Estevam <festevam@gmail.com>; Fugang Duan
> > <fugang.duan@nxp.com>; David S . Miller <davem@davemloft.net>;
> > Russell
> > King <linux@armlinux.org.uk>; Andrew Lunn <andrew@lunn.ch>; Jakub
> > Kicinski <kuba@kernel.org>
> > Cc: Philippe Schenker <philippe.schenker@toradex.com>;
> > linux-kernel@vger.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
> > 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%2Fsourc
> > 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.

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?

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
>