[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Fri, 7 Jul 2017 07:50:47 +0200
From: Richard Leitner <richard.leitner@...data.com>
To: Andy Duan <fugang.duan@....com>,
"robh+dt@...nel.org" <robh+dt@...nel.org>,
"mark.rutland@....com" <mark.rutland@....com>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"dev@...l1n.net" <dev@...l1n.net>, Andrew Lunn <andrew@...n.ch>,
Richard Leitner <richard.leitner@...data.com>
Subject: Re: [PATCH 2/2] net: ethernet: fsl: add phy reset after clk enable
option
On 07/07/2017 07:30 AM, Andy Duan wrote:
> From: Richard Leitner <richard.leitner@...data.com> Sent: Thursday, July 06, 2017 9:06 PM
>> To: Andy Duan <fugang.duan@....com>; robh+dt@...nel.org;
>> mark.rutland@....com
>> Cc: netdev@...r.kernel.org; devicetree@...r.kernel.org; linux-
>> kernel@...r.kernel.org; dev@...l1n.net; Richard Leitner
>> <richard.leitner@...data.com>
>> 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@...data.com>
...
>> *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
Powered by blists - more mailing lists