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

From: Joakim Zhang <hidden>
Date: 2021-12-13 04:39:23
Also in: lkml

Hi Philippe,

-----Original Message-----
From: Philippe Schenker <redacted>
Sent: 2021年12月10日 21:51
To: festevam@gmail.com; linux@armlinux.org.uk; kuba@kernel.org;
netdev@vger.kernel.org; davem@davemloft.net; Joakim Zhang
[off-list ref]; andrew@lunn.ch
Cc: linux-kernel@vger.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:

quoted

Hi Philippe,

quoted

-----Original Message-----
From: Philippe Schenker <redacted>
Sent: 2021年12月6日 18:13
To: netdev@vger.kernel.org; Joakim Zhang

[off-list ref];

quoted

Fabio Estevam [off-list ref]; Fugang Duan
[off-list ref]; David S . Miller [off-list ref];
Russell King [off-list ref]; Andrew Lunn [off-list ref];
Jakub Kicinski [off-list ref]
Cc: Philippe Schenker <redacted>;
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%2F&amp;data=04%7C01%7Cqiangqing.zhang%40nxp.com%7Ca43416

6e62464

quoted

c4b69c408d9bbe420ef%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7
C637

quoted

747410747358279%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAi
LCJQIjoi

quoted

V2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=8u1EudB0
aDkC

quoted

K9tzrqVznAWY6mDgBIHqIQCyDWrsH4g%3D&amp;reserved=0

adex.com%2Fcgit%2Flinux-toradex.git%2Flog%2F%3Fh%3Dtoradex_5.4-2.3.x

quoted

-imx&amp;data=04%7C01%7Cqiangqing.zhang%40nxp.com%7Cf3c138ed9232

quoted

4a8d75e708d9b8a11b9a%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%

quoted

7C637743824364193423%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAw

quoted

MDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sda

quoted

ta=Bw%2BZdqhAjPXqKJFZCXp0mtId1x9mkX6f6MW2ky6U1ww%3D&amp;res

quoted

https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fso

quoted

e.codeaurora.org%2Fexternal%2Fimx%2Flinux-imx%2Flog%2F%3Fh%3Dimx_

quoted

5.4.70_2.3.0&amp;data=04%7C01%7Cqiangqing.zhang%40nxp.com%7Cf3c13

quoted

8ed92324a8d75e708d9b8a11b9a%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C

quoted

0%7C0%7C637743824364193423%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiM

quoted

C4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000

quoted

&amp;sdata=of9z9hfVhHakVScLxCdEo%2BXmd2B9Ad9X8Rry6GjEZE4%3D&a

quoted

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

quoted

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

++++++++++++++++----

quoted

---
 2 files changed, 73 insertions(+), 31 deletions(-)

--
2.34.0

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help