RE: [EXTERNAL] Re: [PATCH 2/2] Reset PHY in phy_init_hw() before interrupt configuration

["Jörg Willmann" <joe@clnt.de>]

[-- Attachment #1: Type: text/plain, Size: 3274 bytes --]



On Thu, 30 Apr 2020, Badel, Laurent wrote:

> -----Original Message-----
>> From: Heiner Kallweit <hkallweit1@gmail.com>
>> Sent: Wednesday, April 29, 2020 7:06 PM
>> To: Badel, Laurent <LaurentBadel@eaton.com>; fugang.duan@nxp.com;
>> netdev@vger.kernel.org; andrew@lunn.ch; f.fainelli@gmail.com;
>> linux@armlinux.org.uk; richard.leitner@skidata.com;
>> davem@davemloft.net; alexander.levin@microsoft.com;
>> gregkh@linuxfoundation.org
>> Cc: Quette, Arnaud <ArnaudQuette@Eaton.com>
>> Subject: [EXTERNAL] Re: [PATCH 2/2] Reset PHY in phy_init_hw() before
>> interrupt configuration
>>
>> On 29.04.2020 11:03, Badel, Laurent wrote:
>>> Description: this patch adds a reset of the PHY in phy_init_hw()
>>> for PHY drivers bearing the PHY_RST_AFTER_CLK_EN flag.
>>>
>>> Rationale: due to the PHY reset reverting the interrupt mask to default,
>>> it is necessary to either perform the reset before PHY configuration,
>>> or re-configure the PHY after reset. This patch implements the former
>>> as it is simpler and more generic.
>>>
>>> Fixes: 1b0a83ac04e383e3bed21332962b90710fcf2828 ("net: fec: add
>> phy_reset_after_clk_enable() support")
>>> Signed-off-by: Laurent Badel <laurentbadel@eaton.com>
>>>
>>> ---
>>>  drivers/net/phy/phy_device.c | 7 +++++--
>>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>>> index 28e3c5c0e..2cc511364 100644
>>> --- a/drivers/net/phy/phy_device.c
>>> +++ b/drivers/net/phy/phy_device.c
>>> @@ -1082,8 +1082,11 @@ int phy_init_hw(struct phy_device *phydev)
>>>  {
>>>  	int ret = 0;
>>>
>>> -	/* Deassert the reset signal */
>>> -	phy_device_reset(phydev, 0);
>>> +	/* Deassert the reset signal
>>> +	 * If the PHY needs a reset, do it now
>>> +	 */
>>> +	if (!phy_reset_after_clk_enable(phydev))
>>
>> If reset is asserted when entering phy_init_hw(), then
>> phy_reset_after_clk_enable() basically becomes a no-op.
>> Still it should work as expected due to the reset signal being
>> deasserted. It would be worth describing in the comment
>> why the code still works in this case.
>>
>
> Thank you for the comment, this is a very good point.
> I will make sure to include some description when resubmitting.
> I had previously tested this and what I saw was that the first
> time you bring up the interface, the reset is not asserted so
> that phy_reset_after_clk_enable() is effective.
> The subsequent times the interface is brought up, the reset
> is already asserted when entering phy_init_hw(), so that
> it becomes a no-op as you correctly pointed out. However,
> that didn't cause any problem on my board, presumably because
> in that case the clock is already running when the PHY comes
> out of reset.
> I will re-test this carefully against the 'net' tree, though,
> before coming to conclusions.
>
I have two additional things to take into account:
* phy_reset_after_clk_enable() shoulnd't be longer called that way since 
it is now misleading -> the phy is no longer reset after clock enable but 
during hw_init()
* how about fec_resume()? I don't think hw_init() is called then and so 
phy_reset_after_clk_enable() will no longer be called.

>>> +		phy_device_reset(phydev, 0);
>>>
>>>  	if (!phydev->drv)
>>>  		return 0;
>>>
>
>