[U-Boot] [PATCH] net: phy: marvell: Fix up reset ordering

[Michal Simek]

On 2.6.2016 07:42, Stefan Roese wrote:
> Hi Michal,
> 
> On 02.06.2016 07:31, Michal Simek wrote:
>> On 1.6.2016 18:22, Nathan Rossi wrote:
>>> Commit a058052c "net: phy: do not read configuration register on reset",
>>> changes the behaviour of the phy_reset function such that the state of
>>> the BMCR register is not preserved during reset.
>>>
>>> Reorder the phy_reset and genphy_config_aneg calls for some of the
>>> marvell phy drivers so that auto-negotiation occurs after reset.
>>>
>>> Signed-off-by: Nathan Rossi <nathan at nathanrossi.com>
>>> Cc: Joe Hershberger <joe.hershberger at ni.com>
>>> Cc: Michal Simek <michal.simek at xilinx.com>
>>> Cc: Stefan Roese <sr at denx.de>
>>> ---
>>>   drivers/net/phy/marvell.c | 11 ++++++-----
>>>   1 file changed, 6 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
>>> index d2e68d4..40284a5 100644
>>> --- a/drivers/net/phy/marvell.c
>>> +++ b/drivers/net/phy/marvell.c
>>> @@ -414,10 +414,10 @@ static int m88e1145_config(struct phy_device
>>> *phydev)
>>>               MIIM_M88E1145_RGMII_TX_DELAY;
>>>       phy_write(phydev, MDIO_DEVAD_NONE, MIIM_88E1145_PHY_EXT_CR, reg);
>>>
>>> -    genphy_config_aneg(phydev);
>>> -
>>>       phy_reset(phydev);
>>>
>>> +    genphy_config_aneg(phydev);
>>> +
>>
>> As you see from my patch
>> 1b008fdb06848c7c84e7c1a4a9b2b76239550555
>> you should return value from genphy_config_aneg() which should be fixed
>> everywhere.
>>
>> Also as you see above you do some writes to the phy and the question is
>> if you should run phy_reset here.
>> Based on my patch above and investigating I found that phy_reset is
>> called before this function is called and not sure if phy should be
>> reset twice.
> 
> Some changes to the PHY registers need a soft-reset to occur
> before these changes to be effective. Not sure if this is the case
> here though.
> 
> But I share your thoughts about this phy_reset() being called now
> in some of the xxx_config() functions and not in others. Your patch
> mentions that phy_reset() is already called in phy_connect_dev(),
> which not all ethernet drivers do right now AFAICT.
> 
> We should perhaps take a look at the Linux Marvell PHY driver to
> check how it is done there.

That was also the part of the reason that I have fixed just one
particular phy which I have access to. I expect that some phy can
require this reset and some of them not.
But definitely this should be properly tested.

Thanks,
Michal