Need help with mdiobus_register and phy

Need help with mdiobus_register and phy

[Timur Tabi]

I need help with a program I've discovered with my driver 
(ethernet/qualcomm/emac/) that occurs in the following process:

1. Load the driver
1a. Driver calls mdiobus_register (see emac_phy_config), which calls 
mdiobus_scan(), which calls get_phy_device()
1b. get_phy_device() performs MDIO bus operations, which means the bus 
and the external phy need to be active.
2. Bring up the interface
3. Bring down the interface.  This calls phy_disconnect() which calls 
phy_detach() which calls phy_suspend().  This puts the external phy to 
sleep.
4. Unload the driver
5. Re-load the driver.
5a. Driver calls mdiobus_register again, but this time the call to 
get_phy_device() fails because the external phy is asleep and does not 
respond to MDIO transactions.

I don't know what my driver is doing wrong.  I'm concerned that 
mdiobus_register() is causing MDIO transactions to occur before the rest 
of the EMAC has a chance to initialize.  I've examined many other 
drivers, and I think I'm initializing things in the proper order.

The call to mdiobus_register() occurs before I know what phydev is. 
Since I need the phydev in order to talk to the phy driver, I don't see 
how I can "wake up" the external phy before calling mdiobus_register().

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: Need help with mdiobus_register and phy

[Andrew Lunn]

On Thu, Oct 13, 2016 at 06:15:39PM -0500, Timur Tabi wrote:
> I need help with a program I've discovered with my driver
> (ethernet/qualcomm/emac/) that occurs in the following process:
> 
> 1. Load the driver
> 1a. Driver calls mdiobus_register (see emac_phy_config), which calls
> mdiobus_scan(), which calls get_phy_device()
> 1b. get_phy_device() performs MDIO bus operations, which means the
> bus and the external phy need to be active.
> 2. Bring up the interface
> 3. Bring down the interface.  This calls phy_disconnect() which
> calls phy_detach() which calls phy_suspend().  This puts the
> external phy to sleep.
> 4. Unload the driver
> 5. Re-load the driver.
> 5a. Driver calls mdiobus_register again, but this time the call to
> get_phy_device() fails because the external phy is asleep and does
> not respond to MDIO transactions.

Hi Timur

Normally, a sleeping PHY does respond to MDIO. Otherwise, how do you
wake it?

So i assume this phy has some other means to wake it. What is this
means?

      Andrew

Re: Need help with mdiobus_register and phy

[Timur Tabi]

Andrew Lunn wrote:
> Normally, a sleeping PHY does respond to MDIO. Otherwise, how do you
> wake it?
>
> So i assume this phy has some other means to wake it. What is this
> means?

I'm guessing that someone has to call phy_resume() before/during the 
call to mdiobus_register, but I don't see how that's possible.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

Re: Need help with mdiobus_register and phy

[Andrew Lunn]

On Fri, Oct 14, 2016 at 06:38:47AM -0500, Timur Tabi wrote:
> Andrew Lunn wrote:
> >Normally, a sleeping PHY does respond to MDIO. Otherwise, how do you
> >wake it?
> >
> >So i assume this phy has some other means to wake it. What is this
> >means?
> 
> I'm guessing that someone has to call phy_resume() before/during the
> call to mdiobus_register, but I don't see how that's possible.

Please can you tell us what PHY which is, and how it is put to sleep
and woken up.

If the PHY cannot be woken up using MDIO, then maybe you need to look
at the mdio bus reset call?

   Andrew

Re: Need help with mdiobus_register and phy

[Timur Tabi]

Andrew Lunn wrote:
> Please can you tell us what PHY which is, and how it is put to sleep
> and woken up.

It's the at803x driver.

http://lxr.free-electrons.com/source/drivers/net/phy/at803x.c

It goes to sleep in its at803x_suspend() function, which is called by 
phy_suspend().

There is a corresponding at803x_resume().  The problem is that this is 
not called by mdiobus_register().  I'm guessing that mdiobus_register() 
assumes that the phy is awake.

It seems like a catch-22.  mdiobus_register() assumes that the phy is 
awake, but you can't wake up the phy until after you call 
mdiobus_register().

> If the PHY cannot be woken up using MDIO, then maybe you need to look
> at the mdio bus reset call?

I looked at that, but it won't work because there is no phydev when the 
reset function is called:

http://lxr.free-electrons.com/source/drivers/net/phy/mdio_bus.c#L328

It's the same catch-22.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

Re: Need help with mdiobus_register and phy

[Andrew Lunn]

> It's the at803x driver.

The at803x_resume() just does normal MDIO transactions. Which suggests
the MDIO bus side of the device is still away. Or at least, the
MII_BMCR register is.

So are you seeing that the reads to MII_PHYSID1 and MII_PHYSID2 return
0xffff, when called from get_phy_id()?

	Andrew

Re: Need help with mdiobus_register and phy

[Timur Tabi]

Andrew Lunn wrote:
> So are you seeing that the reads to MII_PHYSID1 and MII_PHYSID2 return
> 0xffff, when called from get_phy_id()?

Yes.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

Re: Need help with mdiobus_register and phy

[Andrew Lunn]

On Fri, Oct 14, 2016 at 07:49:56AM -0500, Timur Tabi wrote:
> Andrew Lunn wrote:
> >So are you seeing that the reads to MII_PHYSID1 and MII_PHYSID2 return
> >0xffff, when called from get_phy_id()?

Have you tried using the ethernet-phy-id device tree property? It
looks like that will allow you to skip get_phy_device and just create
the phy device. You can then bring the phy out of sleep in the probe
function?

	Andrew

Re: Need help with mdiobus_register and phy

[Timur Tabi]

Andrew Lunn wrote:
> Have you tried using the ethernet-phy-id device tree property? It
> looks like that will allow you to skip get_phy_device and just create
> the phy device. You can then bring the phy out of sleep in the probe
> function?

The problem I'm experiencing is with ACPI, so I can't use any of the 
fancy of_ apis like of_get_phy_id().  But I'll look into it.

Is it possible that at803x_suspend() is too aggressive?  That's it's 
effectively disabling the phy?  While the phy is suspended, should it 
still respond to MII_PHYSID1 and MII_PHYSID2 requests?

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

Re: Need help with mdiobus_register and phy

[Andrew Lunn]

On Fri, Oct 14, 2016 at 08:03:18AM -0500, Timur Tabi wrote:
> Andrew Lunn wrote:
> >Have you tried using the ethernet-phy-id device tree property? It
> >looks like that will allow you to skip get_phy_device and just create
> >the phy device. You can then bring the phy out of sleep in the probe
> >function?
> 
> The problem I'm experiencing is with ACPI, so I can't use any of the
> fancy of_ apis like of_get_phy_id().  But I'll look into it.
> 
> Is it possible that at803x_suspend() is too aggressive?  That's it's
> effectively disabling the phy?  While the phy is suspended, should
> it still respond to MII_PHYSID1 and MII_PHYSID2 requests?

That is a basic assumption of the code. If you cannot read the IDs how
are you supposed to know what device it is, and what quirks you need
to work around its broken features...

Does the datasheet say anything about this?

I would say for this device, suspend() is too aggressive.

  Andrew

Re: Need help with mdiobus_register and phy

[Timur Tabi]

Andrew Lunn wrote:
> Does the datasheet say anything about this?
>
> I would say for this device, suspend() is too aggressive.

I'll have to find the datasheet.  Let me do some research and get back 
to you.  Thanks for your help so far.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

Re: Need help with mdiobus_register and phy

[Timur Tabi]

Andrew Lunn wrote:
> That is a basic assumption of the code. If you cannot read the IDs how
> are you supposed to know what device it is, and what quirks you need
> to work around its broken features...
>
> Does the datasheet say anything about this?
>
> I would say for this device, suspend() is too aggressive.

This change in my driver makes the problem go away (I'm not sure if it's 
a "fix"):

@@ -992,7 +992,7 @@ int emac_mac_up(struct emac_adapter *adpt)
         emac_mac_rx_descs_refill(adpt, &adpt->rx_q);

         ret = phy_connect_direct(netdev, adpt->phydev, emac_adjust_link,
-                                PHY_INTERFACE_MODE_SGMII);
+                                PHY_INTERFACE_MODE_NA);

With the interface not set as SGMII, the following code in 
at803x_suspend() is not executed:

/* also power-down SGMII interface */
ccr = phy_read(phydev, AT803X_REG_CHIP_CONFIG);
phy_write(phydev, AT803X_REG_CHIP_CONFIG, ccr & ~AT803X_BT_BX_REG_SEL);
phy_write(phydev, MII_BMCR, phy_read(phydev, MII_BMCR) | BMCR_PDOWN);
phy_write(phydev, AT803X_REG_CHIP_CONFIG, ccr | AT803X_BT_BX_REG_SEL);

I don't see any other driver issue BMCR_PDOWN in their functions.  I 
added some printks for the PHYSID1 and PHYSID2 registers before and 
after BMCR_PDOWN:

at803x_suspend:235 MII_PHYSID1=004d MII_PHYSID2=d074
at803x_suspend:242 MII_PHYSID1=ffff MII_PHYSID2=ffff

So after calling BMCR_PDOWN, the PHYSID1 and PHYSID2 registers are no 
longer readable.  Is that expected?

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: Need help with mdiobus_register and phy

[Andrew Lunn]

On Fri, Oct 14, 2016 at 11:57:29AM -0500, Timur Tabi wrote:
> Andrew Lunn wrote:
> >That is a basic assumption of the code. If you cannot read the IDs how
> >are you supposed to know what device it is, and what quirks you need
> >to work around its broken features...
> >
> >Does the datasheet say anything about this?
> >
> >I would say for this device, suspend() is too aggressive.
> 
> This change in my driver makes the problem go away (I'm not sure if
> it's a "fix"):
> 
> @@ -992,7 +992,7 @@ int emac_mac_up(struct emac_adapter *adpt)
>         emac_mac_rx_descs_refill(adpt, &adpt->rx_q);
> 
>         ret = phy_connect_direct(netdev, adpt->phydev, emac_adjust_link,
> -                                PHY_INTERFACE_MODE_SGMII);
> +                                PHY_INTERFACE_MODE_NA);

It is normal to get the phy-mode from device tree. I've no idea what
ACPI is supposed to do. Setting it to PHY_INTERFACE_MODE_NA means you
assume the boot loader has correctly setup the hardware. You ACPI
firmware might of done this, but there is no guarantee a device tree
base bootloader has. So i would prefer not changing this.
 
> With the interface not set as SGMII, the following code in
> at803x_suspend() is not executed:
> 
> /* also power-down SGMII interface */
> ccr = phy_read(phydev, AT803X_REG_CHIP_CONFIG);
> phy_write(phydev, AT803X_REG_CHIP_CONFIG, ccr & ~AT803X_BT_BX_REG_SEL);
> phy_write(phydev, MII_BMCR, phy_read(phydev, MII_BMCR) | BMCR_PDOWN);
> phy_write(phydev, AT803X_REG_CHIP_CONFIG, ccr | AT803X_BT_BX_REG_SEL);
> 
> I don't see any other driver issue BMCR_PDOWN in their functions.  I
> added some printks for the PHYSID1 and PHYSID2 registers before and
> after BMCR_PDOWN:
> 
> at803x_suspend:235 MII_PHYSID1=004d MII_PHYSID2=d074
> at803x_suspend:242 MII_PHYSID1=ffff MII_PHYSID2=ffff
> 
> So after calling BMCR_PDOWN, the PHYSID1 and PHYSID2 registers are
> no longer readable.  Is that expected?

You are making two changes here. Is it the SGMII power down which is
causing the id registers to return 0xffff, or the BMCR_PDOWN.

The generic suspend code sets the PDOWN bit, so it is assuming the PHY
will respond afterwards.

     Andrew

Re: Need help with mdiobus_register and phy

[Timur Tabi]

Andrew Lunn wrote:

> It is normal to get the phy-mode from device tree. I've no idea what
> ACPI is supposed to do. Setting it to PHY_INTERFACE_MODE_NA means you
> assume the boot loader has correctly setup the hardware. You ACPI
> firmware might of done this, but there is no guarantee a device tree
> base bootloader has. So i would prefer not changing this.

Fair enough.  I don't think it's specified anywhere what firmware is 
supposed to do.

What about specifying PHY_INTERFACE_MODE_NA on ACPI systems, but using 
the phy-mode property on device tree systems?  That doesn't sound like a 
great idea.

>> >I don't see any other driver issue BMCR_PDOWN in their functions.  I
>> >added some printks for the PHYSID1 and PHYSID2 registers before and
>> >after BMCR_PDOWN:
>> >
>> >at803x_suspend:235 MII_PHYSID1=004d MII_PHYSID2=d074
>> >at803x_suspend:242 MII_PHYSID1=ffff MII_PHYSID2=ffff
>> >
>> >So after calling BMCR_PDOWN, the PHYSID1 and PHYSID2 registers are
>> >no longer readable.  Is that expected?
> You are making two changes here. Is it the SGMII power down which is
> causing the id registers to return 0xffff, or the BMCR_PDOWN.
>
> The generic suspend code sets the PDOWN bit, so it is assuming the PHY
> will respond afterwards.

Ok, it took me a while to figure this out.  The driver does three writes:

phy_write(phydev, AT803X_REG_CHIP_CONFIG, ccr & ~AT803X_BT_BX_REG_SEL);
phy_write(phydev, MII_BMCR, phy_read(phydev, MII_BMCR) | BMCR_PDOWN);
phy_write(phydev, AT803X_REG_CHIP_CONFIG, ccr | AT803X_BT_BX_REG_SEL);

The first clears the AT803X_BT_BX_REG_SEL bit.  According to the 
datasheet, that changes the register set from copper to fiber mode. 
BMCR_PDOWN in fiber mode shuts off the SerDes bus.  That's not true in 
copper mode.

Then after shutting down SerDes, it switches back to copper mode.

I also noticed the at803x_suspend already sends BMCR_PDOWN in copper 
mode earlier in the function.

So the question remains: should drivers shut down the SerDes bus when 
they suspend?  In a sense, I'm wondering if we should revert

	at803x: fix suspend/resume for SGMII link

However, the changelog for that patch makes it sound like it's a 
necessary fix.

So I'm torn.  With the SerDes connection disabled, the driver no longer 
responds to ID register reads.  That seems like something that would be 
broken on device tree as well, but I don't understand why no one noticed 
it before.

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: Need help with mdiobus_register and phy

[Florian Fainelli]

On October 14, 2016 7:25:14 PM CEST, Andrew Lunn <andrew@lunn.ch> wrote:

>> So after calling BMCR_PDOWN, the PHYSID1 and PHYSID2 registers are
>> no longer readable.  Is that expected?
>
>You are making two changes here. Is it the SGMII power down which is
>causing the id registers to return 0xffff, or the BMCR_PDOWN.

I would be curious to know about that as well.

>
>The generic suspend code sets the PDOWN bit, so it is assuming the PHY
>will respond afterwards.

After reading the spec again, it does not appear to me that a PHY with PDOWN set is guaranteed or even required to respond to other register reads such as MII_PHYID1/2, in which case we may have to implement a MDIO bus reset routine which clears PDOWN for all PHYs that we detect(ed), or as Andrew suggested, utilize the matching by compatible string with the PHY OUI in it.

-- 
Florian

Re: Need help with mdiobus_register and phy

[Timur Tabi]

Florian Fainelli wrote:
> After reading the spec again, it does not appear to me that a PHY
> with PDOWN set is guaranteed or even required to respond to other
> register reads such as MII_PHYID1/2, in which case we may have to
> implement a MDIO bus reset routine which clears PDOWN for all PHYs
> that we detect(ed), or as Andrew suggested, utilize the matching by
> compatible string with the PHY OUI in it.

The 8031 does respond normally when PDOWN is set.  However, the ID 
registers are not available when the SerDes bus is also powered down. 
I'll call this PDOWN+.  This is a special power-down sequence that the 
at803x driver does on suspend.  See my other email for details.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

Re: Need help with mdiobus_register and phy

[Andrew Lunn]

On Sat, Oct 15, 2016 at 09:39:12AM -0500, Timur Tabi wrote:
> Florian Fainelli wrote:
> >After reading the spec again, it does not appear to me that a PHY
> >with PDOWN set is guaranteed or even required to respond to other
> >register reads such as MII_PHYID1/2, in which case we may have to
> >implement a MDIO bus reset routine which clears PDOWN for all PHYs
> >that we detect(ed), or as Andrew suggested, utilize the matching by
> >compatible string with the PHY OUI in it.
> 
> The 8031 does respond normally when PDOWN is set.  However, the ID
> registers are not available when the SerDes bus is also powered
> down. I'll call this PDOWN+.  This is a special power-down sequence
> that the at803x driver does on suspend.  See my other email for
> details.

So we appear to have two ways to go:

1) Take the SerDes power down out of the suspend code for the at803x.

2) Assume MII_PHYID1/2 registers are not guaranteed to be available
when the PHY is powered down. So get_phy_id should first read
MII_BMCR. If it gets 0xffff, assume there is no PHY there. If the
PDOWN bit is set, power up the PHY. Then reading the ID registers.

      Andrew

Re: Need help with mdiobus_register and phy

[Timur Tabi]

Andrew Lunn wrote:
> 1) Take the SerDes power down out of the suspend code for the at803x.
>
> 2) Assume MII_PHYID1/2 registers are not guaranteed to be available
> when the PHY is powered down. So get_phy_id should first read
> MII_BMCR. If it gets 0xffff, assume there is no PHY there. If the
> PDOWN bit is set, power up the PHY. Then reading the ID registers.

Before we take approach #1, I'd like to hear from the developer of that 
patch, Zefir.  According to him, that patch is necessary to fix a bug. 
I don't know if that bug exists only on his system, though.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

Re: Need help with mdiobus_register and phy

[Zefir Kurtisi]

On 10/15/2016 08:28 PM, Timur Tabi wrote:
> Andrew Lunn wrote:
>> 1) Take the SerDes power down out of the suspend code for the at803x.
>>
>> 2) Assume MII_PHYID1/2 registers are not guaranteed to be available
>> when the PHY is powered down. So get_phy_id should first read
>> MII_BMCR. If it gets 0xffff, assume there is no PHY there. If the
>> PDOWN bit is set, power up the PHY. Then reading the ID registers.
> 
> Before we take approach #1, I'd like to hear from the developer of that patch,
> Zefir.  According to him, that patch is necessary to fix a bug. I don't know if
> that bug exists only on his system, though.
> 

Hi,

the problem we observed was that in SGMII mode after a suspend/resume cycle the
PHY link states get out of sync (e.g. gianfar reports link up, at803x down) and
power cycling the SerDes was a reliable way to get them re-synchronized again.

This obviously worked for some time after March 2016 and must have stopped only
recently (maybe additional PHY suspends were added).

Anyway, since the SGMII reset is required, instead of reverting the patch in full
I suggest to move the SGMII power down from at803x_suspend() and do a SerDes power
cycle in at803x_resume(). Could you please test if the patch below fixes the problem?


Thanks,
Zefir
---



Subject: [PATCH] at803x: don't power down SerDes on suspend

Powering down SerDes renders registers inaccessible
and with that re-initializing a suspended PHY fails.

Instead of powering down SerDes at suspend(), leave
it as is and power-cycle it on resume().

Signed-off-by: Zefir Kurtisi <zefir.kurtisi@neratec.com>
---
 drivers/net/phy/at803x.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index f279a89..282ffbb 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -225,16 +225,6 @@ static int at803x_suspend(struct phy_device *phydev)

 	phy_write(phydev, MII_BMCR, value);

-	if (phydev->interface != PHY_INTERFACE_MODE_SGMII)
-		goto done;
-
-	/* also power-down SGMII interface */
-	ccr = phy_read(phydev, AT803X_REG_CHIP_CONFIG);
-	phy_write(phydev, AT803X_REG_CHIP_CONFIG, ccr & ~AT803X_BT_BX_REG_SEL);
-	phy_write(phydev, MII_BMCR, phy_read(phydev, MII_BMCR) | BMCR_PDOWN);
-	phy_write(phydev, AT803X_REG_CHIP_CONFIG, ccr | AT803X_BT_BX_REG_SEL);
-
-done:
 	mutex_unlock(&phydev->lock);

 	return 0;
@@ -254,9 +244,11 @@ static int at803x_resume(struct phy_device *phydev)
 	if (phydev->interface != PHY_INTERFACE_MODE_SGMII)
 		goto done;

-	/* also power-up SGMII interface */
+	/* reset SGMII interface for re-synchronization */
 	ccr = phy_read(phydev, AT803X_REG_CHIP_CONFIG);
 	phy_write(phydev, AT803X_REG_CHIP_CONFIG, ccr & ~AT803X_BT_BX_REG_SEL);
+	phy_write(phydev, MII_BMCR, phy_read(phydev, MII_BMCR) | BMCR_PDOWN);
+
 	value = phy_read(phydev, MII_BMCR) & ~(BMCR_PDOWN | BMCR_ISOLATE);
 	phy_write(phydev, MII_BMCR, value);
 	phy_write(phydev, AT803X_REG_CHIP_CONFIG, ccr | AT803X_BT_BX_REG_SEL);
-- 
2.7.4

Re: Need help with mdiobus_register and phy

[Timur Tabi]

Zefir Kurtisi wrote:
> Anyway, since the SGMII reset is required, instead of reverting the patch in full
> I suggest to move the SGMII power down from at803x_suspend() and do a SerDes power
> cycle in at803x_resume(). Could you please test if the patch below fixes the problem?

I have never seen the original problem that you noticed.  When I use the 
generic phy driver instead of the at803x driver, everything works great 
for me.  Perhaps the problem that you noticed only occurs with the 
Gianfar NIC?

Anyway, I tested the change you suggested, and it fixes the problem for 
me.  I moved the power-down code to before the power-up code.  But like 
I said, since I never experienced the original problem, I don't know if 
that works for you.

I suggest you make the changes in the driver yourself and test it, and 
then I will test whether that patch also works for me.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

Re: Need help with mdiobus_register and phy

[Zefir Kurtisi]

On 10/17/2016 10:53 PM, Timur Tabi wrote:
> Zefir Kurtisi wrote:
>> Anyway, since the SGMII reset is required, instead of reverting the patch in full
>> I suggest to move the SGMII power down from at803x_suspend() and do a SerDes power
>> cycle in at803x_resume(). Could you please test if the patch below fixes the
>> problem?
> 
> I have never seen the original problem that you noticed.  When I use the generic
> phy driver instead of the at803x driver, everything works great for me.  Perhaps
> the problem that you noticed only occurs with the Gianfar NIC?
> 
You mean it works for you in SGMII mode without glitches? Then it might in fact be
an unfortunate rare or unique combination that we chose.

> Anyway, I tested the change you suggested, and it fixes the problem for me.  I
> moved the power-down code to before the power-up code.  But like I said, since I
> never experienced the original problem, I don't know if that works for you.
> 
> I suggest you make the changes in the driver yourself and test it, and then I will
> test whether that patch also works for me.
> 

I'd need some time to get that tested, and since our device might be the only one
requiring the fix, I am fine with keeping the patch private. Feel free to revert
it, if required, I'll provide a revised one.


Thanks
Zefir

Re: Need help with mdiobus_register and phy

[Timur Tabi]

Zefir Kurtisi wrote:

>> I have never seen the original problem that you noticed.  When I use the generic
>> phy driver instead of the at803x driver, everything works great for me.  Perhaps
>> the problem that you noticed only occurs with the Gianfar NIC?
>>
> You mean it works for you in SGMII mode without glitches?

That is correct.

> Then it might in fact be
> an unfortunate rare or unique combination that we chose.

That is what I expect.

> I'd need some time to get that tested, and since our device might be the only one
> requiring the fix, I am fine with keeping the patch private. Feel free to revert
> it, if required, I'll provide a revised one.

I'm in no hurry to get this fixed.  Do you need a couple weeks to run 
some tests?  It would be good to know for sure that your fix is needed 
only on your platform.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

Re: Need help with mdiobus_register and phy

[Zefir Kurtisi]

On 10/18/2016 02:40 PM, Timur Tabi wrote:
> Zefir Kurtisi wrote:
> 
>>> I have never seen the original problem that you noticed.  When I use the generic
>>> phy driver instead of the at803x driver, everything works great for me.  Perhaps
>>> the problem that you noticed only occurs with the Gianfar NIC?
>>>
>> You mean it works for you in SGMII mode without glitches?
> 
> That is correct.
> 
>> Then it might in fact be
>> an unfortunate rare or unique combination that we chose.
> 
> That is what I expect.
> 
>> I'd need some time to get that tested, and since our device might be the only one
>> requiring the fix, I am fine with keeping the patch private. Feel free to revert
>> it, if required, I'll provide a revised one.
> 
> I'm in no hurry to get this fixed.  Do you need a couple weeks to run some tests? 
> It would be good to know for sure that your fix is needed only on your platform.
> 
Ok then, if you can wait some days, I'll prepare and provide you a more detailed
failure report to allow you testing if the issue happens with other NICs.

Re: Need help with mdiobus_register and phy

[Timur Tabi]

Zefir Kurtisi wrote:
> Ok then, if you can wait some days, I'll prepare and provide you a more detailed
> failure report to allow you testing if the issue happens with other NICs.

That sounds great.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

Re: Need help with mdiobus_register and phy

[Zefir Kurtisi]

On 10/19/2016 02:16 PM, Timur Tabi wrote:
> Zefir Kurtisi wrote:
>> Ok then, if you can wait some days, I'll prepare and provide you a more detailed
>> failure report to allow you testing if the issue happens with other NICs.
> 
> That sounds great.
> 

I am now able again to reproduce the issue reliably, let's see if it is helpful
enough to check if this a problem unique to our setup.

As described earlier, the HW is as follows:
(copper)--[at8031]--(SGMII)--[eTSEC/mpc85xx]

We are using OpenWRT/LEDE as SW platform, therefore I can't do the tests on the
latest kernel but provide a reference to inspect the problem. The kernel used is
4.4.21 with the relevant drivers (ath803x and gianfar_driver) being either clean
or patched with OWRT/LEDE modifications (issue happens in either case).


The process for detecting the failure is rather trivial: continuously put the
Ethernet interface down and up and try to get some data through until it fails.
The script I used to run on the device (over serial) is:
#----------------------------------------------------------
HOST=192.168.1.100
TIMEOUT=20

while true; do
	ifconfig eth0 down
	ifconfig eth0 up

        ping $HOST -c 1 -w $TIMEOUT || {
                echo "Ping failed after $TIMEOUT seconds"
                exit 1
        }
done
#----------------------------------------------------------
In my setup, it takes less than 5 minutes for the script to exit. Once running
into this state, the relevant registers read from at803x are as follows:
* copper status              (0x01): 0x796d
* fiber  status              (0x01): 0x6149
* copper phy specific status (0x11): 0xbc10
* fiber  phy specific status (0x11): 0x8100
* copper/fiber status        (0x1b): 0x020e

In essence
* the copper link is up with autonegotiation completed
  (switch/peer link indication is also up), but
* the SGMII link is down
  (bits LINK and MR_AN_COMPLETE in PHY specific status fiber cleared)
* although SGMII is synchronized (SYNC_STATUS bit in same register set)

The SGMII link does not recover from this state thereafter, the only reliable way
to get it up again I found is to do a power down/up cycle at the fiber/SGMII side,
which is what the patch I contributed does.


Now the interesting (and new for me) part is: if I remove the at803x driver from
the system and use the generic phy instead, the problem does not happen (or at
least not while running for one full day). I don't see any significant differences
between what at803x does above genphy (gpio based HW reset is not run for 8031 in
at803x_link_change_notify()). Using the genphy is not an option for us, since we
have the PHY also attached to fiber and need dedicated handling.


Hope this helps for the beginning, let me know if you need more information.


Cheers,
Zefir

Re: Need help with mdiobus_register and phy

[Zefir Kurtisi]

On 10/20/2016 02:55 PM, Zefir Kurtisi wrote:
> [...]
> 
> Now the interesting (and new for me) part is: if I remove the at803x driver from
> the system and use the generic phy instead, the problem does not happen (or at
> least not while running for one full day). [...]
> 
Update: the failure also happened with genphy after running for another day.

Re: Need help with mdiobus_register and phy

[Timur Tabi]

Zefir Kurtisi wrote:
>> >Now the interesting (and new for me) part is: if I remove the at803x driver from
>> >the system and use the generic phy instead, the problem does not happen (or at
>> >least not while running for one full day). [...]
>> >
> Update: the failure also happened with genphy after running for another day.

I haven't had a chance to try to reproduce this failure, but to me this 
sounds like you have a problem with gianfar or your board, and not the 
at803x driver.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.