Re: [PATCH 2/2] net: ethernet: fsl: add phy reset after clk enable option

From: Richard Leitner
Date: Fri Jul 07 2017 - 02:00:58 EST

On 07/07/2017 07:30 AM, Andy Duan wrote:
From: Richard Leitner <richard.leitner@xxxxxxxxxxx> Sent: Thursday, July 06, 2017 9:06 PM
To: Andy Duan <fugang.duan@xxxxxxx>; robh+dt@xxxxxxxxxx;
Cc: netdev@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-
kernel@xxxxxxxxxxxxxxx; dev@xxxxxxxxxx; Richard Leitner
Subject: [PATCH 2/2] net: ethernet: fsl: add phy reset after clk enable option

Some PHYs (for example the LAN8710) doesn't allow turning the clocks off and
on again without reset (according to their datasheet). Exactly this behaviour
was introduced for power saving reasons by commit e8fcfcd5684a
("net: fec: optimize the clock management to save power") Therefore add a
devictree option to perform a PHY reset and configuration after every clock

For a better understanding here's a outline of the time response of the clock
and reset lines before and after this patch:

v--fec_probe() v--fec_enet_open()
v v
w/o patch eCLK:
w/o patch nRST: ----__------------------------------------------
w/o patch CONF:

w/ patch eCLK:
w/ patch nRST: ----__--------------------------__--------------
w/ patch CONF:
^ ^
^--fec_probe() ^--fec_enet_open()

In our case this problem does occur at about every 10th to 50th POR of an
LAN8710 connected to an i.MX6 SoC. The typical symptom of this problem is a
"swinging" ethernet link. Similar issues were experienced by users of the NXP
With this patch applied the issue didn't occur for at least a few hundred PORs
of our board.

Fixes: e8fcfcd5684a ("net: fec: optimize the clock management to sa...")
Signed-off-by: Richard Leitner <richard.leitner@xxxxxxxxxxx>


*ndev, bool enable)
ret = clk_prepare_enable(fep->clk_ref);
if (ret)
goto failed_clk_ref;
+ /* reset the phy after clk is enabled if it's configured */
+ if (fep->phy_reset_after_clk_enable) {
+ ret = fec_reset_phy(ndev);
+ if (ret)
+ goto failed_reset;
+ if (ndev->phydev) {
+ ret = phy_init_hw(ndev->phydev);
+ if (ret)
+ goto failed_reset;
+ }
+ }

Since it is common issue so long as using the PHY, can you move it into smsc phy driver like in .smsc_phy_reset() function ?
And get the reset pin from phy dts node.

During my first glance at this problem I had the same "feeling" that it should go into smsc.c. Therefore I've tried that already, but I haven't found a suitable "place" for that. My basic problem is: Where do I know (from smsc.c view) when the enetrefclk was disabled/enabled again without changing fec_main.c?

Some more points that come into my mind:
- The smsc_phy_reset function is registered as "soft_reset". Would it be OK to use nRST in it?
- Would it be OK to call the phy_init_hw function from within the smsc_phy_reset?
- IMHO I'd have to move the reset gpio binding inside the phy node then. Isn't that a pretty big change doing that for all PHYs/FECs? Would it be "worth" it?

Sorry for these many (maybe noobish) questions, but I'm pretty new to the kernels fec/phy stuff ;-)


Thanks & regards,