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

[Richard Leitner]

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;
> > mark.rutland@xxxxxxx
> > Cc: netdev@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-
> > kernel@xxxxxxxxxxxxxxx; dev@xxxxxxxxxx; Richard Leitner
> > <richard.leitner@xxxxxxxxxxx>
> > 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
> > enable.
> >
> > 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:
> > _______XX_______________________________________
> >
> > 	w/  patch eCLK:
> > ___||||||||____________________|||||||||||||||||
> > 	w/  patch nRST: ----__--------------------------__--------------
> > 	w/  patch CONF:
> > _______XX__________________________XX___________
> > 			  ^                           ^
> > 			  ^--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
> > forum:
> > 	https://community.nxp.com/thread/389902
> > 	https://community.nxp.com/message/309354
> > 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 ;-)

> Andy

Thanks & regards,
Richard.L