From mboxrd@z Thu Jan  1 00:00:00 1970
From: Stefan Roese <sr@denx.de>
Date: Thu, 2 Jun 2016 07:42:28 +0200
Subject: [U-Boot] [PATCH] net: phy: marvell: Fix up reset ordering
In-Reply-To: <574FC4CC.8090201@xilinx.com>
References: <1464798164-11830-1-git-send-email-nathan@nathanrossi.com>
 <574FC4CC.8090201@xilinx.com>
Message-ID: <574FC744.3090706@denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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