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

[Nathan Rossi]

On Thu, Jun 2, 2016 at 3:59 PM, Michal Simek <michal.simek at xilinx.com> wrote:
> 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.

The m88e131(8/0) in linux does not reset during config.
The m88e1145 in linux does not reset during config.
The m88e1149 in linux does reset during config.

Also there are differences in what is configured between linux and
u-boot, so it is a bit hard to determine if the reset is required
based on the registers being configured.

>
> 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.
>

I am only using the 88e1318 (same as 88e1310 but 1.8V variant). In the
configuration it is used on the board I am testing the phy reset does
not appear to be needed, behaviour is consistent with and or without
the reset (given the config_aneg being after the reset). With the
config_aneg being before the reset, it causes the phy to disable
autoneg.

I will send a separate patch that only applies to that device,
removing the phy reset and making the return value that of
genphy_config_aneg.

Regards,
Nathan