[RFC,0/2] Reset PHY in fec_resume if it got powered down

Message

Philippe Schenker Dec. 6, 2021, 10:13 a.m. UTC

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] http://git.toradex.com/cgit/linux-toradex.git/log/?h=toradex_5.4-2.3.x-imx
[2] https://source.codeaurora.org/external/imx/linux-imx/log/?h=imx_5.4.70_2.3.0


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(-)

Comments

Joakim Zhang Dec. 7, 2021, 1:58 a.m. UTC | #1

Hi Philippe,

> -----Original Message-----
> From: Philippe Schenker <[e-mail]>
> Sent: 2021年12月6日 18:13
> To: [e-mail]; Joakim Zhang <[e-mail]>;
> Fabio Estevam <[e-mail]>; Fugang Duan
> <[e-mail]>; David S . Miller <[e-mail]>; Russell
> King <[e-mail]>; Andrew Lunn <[e-mail]>; Jakub
> Kicinski <[e-mail]>
> Cc: Philippe Schenker <[e-mail]>;
> [e-mail]
> 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
> 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

Philippe Schenker Dec. 10, 2021, 1:51 p.m. UTC | #2

On Tue, 2021-12-07 at 01:58 +0000, Joakim Zhang wrote:
> 
> Hi Philippe,
> 
> > -----Original Message-----
> > From: Philippe Schenker <[e-mail]>
> > Sent: 2021年12月6日 18:13
> > To: [e-mail]; Joakim Zhang <[e-mail]>;
> > Fabio Estevam <[e-mail]>; Fugang Duan
> > <[e-mail]>; David S . Miller <[e-mail]>;
> > Russell
> > King <[e-mail]>; Andrew Lunn <[e-mail]>; Jakub
> > Kicinski <[e-mail]>
> > Cc: Philippe Schenker <[e-mail]>;
> > [e-mail]
> > 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
>

Joakim Zhang Dec. 13, 2021, 4:39 a.m. UTC | #3

Hi Philippe,

> -----Original Message-----
> From: Philippe Schenker <[e-mail]>
> Sent: 2021年12月10日 21:51
> To: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; Joakim Zhang
> <[e-mail]>; [e-mail]
> Cc: [e-mail]
> 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 <[e-mail]>
> > > Sent: 2021年12月6日 18:13
> > > To: [e-mail]; Joakim Zhang
> <[e-mail]>;
> > > Fabio Estevam <[e-mail]>; Fugang Duan
> > > <[e-mail]>; David S . Miller <[e-mail]>;
> > > Russell King <[e-mail]>; Andrew Lunn <[e-mail]>;
> > > Jakub Kicinski <[e-mail]>
> > > Cc: Philippe Schenker <[e-mail]>;
> > > [e-mail]
> > > 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
> >