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

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

[Nathan Rossi]

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@nathanrossi.com>
Cc: Joe Hershberger <joe.hershberger@ni.com>
Cc: Michal Simek <michal.simek@xilinx.com>
Cc: Stefan Roese <sr@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);
+
 	return 0;
 }
 
@@ -443,10 +443,10 @@ static int m88e1149_config(struct phy_device *phydev)
 	phy_write(phydev, MDIO_DEVAD_NONE, 0x1e, 0x0);
 	phy_write(phydev, MDIO_DEVAD_NONE, 0x1e, 0x100);
 
-	genphy_config_aneg(phydev);
-
 	phy_reset(phydev);
 
+	genphy_config_aneg(phydev);
+
 	return 0;
 }
 
@@ -476,9 +476,10 @@ static int m88e1310_config(struct phy_device *phydev)
 	/* Ensure to return to page 0 */
 	phy_write(phydev, MDIO_DEVAD_NONE, MIIM_88E1310_PHY_PAGE, 0x0000);
 
-	genphy_config_aneg(phydev);
 	phy_reset(phydev);
 
+	genphy_config_aneg(phydev);
+
 	return 0;
 }
 
-- 
2.8.1

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

[Michal Simek]

Hi Nathan,

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@nathanrossi.com>
> Cc: Joe Hershberger <joe.hershberger@ni.com>
> Cc: Michal Simek <michal.simek@xilinx.com>
> Cc: Stefan Roese <sr@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.

Thanks,
Michal

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

[Stefan Roese]

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@nathanrossi.com>
>> Cc: Joe Hershberger <joe.hershberger@ni.com>
>> Cc: Michal Simek <michal.simek@xilinx.com>
>> Cc: Stefan Roese <sr@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.

Thanks,
Stefan

[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@nathanrossi.com>
>>> Cc: Joe Hershberger <joe.hershberger@ni.com>
>>> Cc: Michal Simek <michal.simek@xilinx.com>
>>> Cc: Stefan Roese <sr@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

[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@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@nathanrossi.com>
>>>> Cc: Joe Hershberger <joe.hershberger@ni.com>
>>>> Cc: Michal Simek <michal.simek@xilinx.com>
>>>> Cc: Stefan Roese <sr@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