[PATCH net-next v2 1/1] net: phy: dp83867: retrigger SGMII AN when link change

[PATCH net-next v2 1/1] net: phy: dp83867: retrigger SGMII AN when link change

[Tan Tee Min]

There is a limitation in TI DP83867 PHY device where SGMII AN is only
triggered once after the device is booted up. Even after the PHY TPI is
down and up again, SGMII AN is not triggered and hence no new in-band
message from PHY to MAC side SGMII.

This could cause an issue during power up, when PHY is up prior to MAC.
At this condition, once MAC side SGMII is up, MAC side SGMII wouldn`t
receive new in-band message from TI PHY with correct link status, speed
and duplex info.

As suggested by TI, implemented a SW solution here to retrigger SGMII
Auto-Neg whenever there is a link change.

v2: Add Fixes tag in commit message.

Fixes: 2a10154abcb7 ("net: phy: dp83867: Add TI dp83867 phy")
Cc: <stable@vger.kernel.org> # 5.4.x
Signed-off-by: Sit, Michael Wei Hong <michael.wei.hong.sit@intel.com>
Reviewed-by: Voon Weifeng <weifeng.voon@intel.com>
Signed-off-by: Tan Tee Min <tee.min.tan@linux.intel.com>
---
 drivers/net/phy/dp83867.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
index 8561f2d4443b..13dafe7a29bd 100644
--- a/drivers/net/phy/dp83867.c
+++ b/drivers/net/phy/dp83867.c
@@ -137,6 +137,7 @@
 #define DP83867_DOWNSHIFT_2_COUNT	2
 #define DP83867_DOWNSHIFT_4_COUNT	4
 #define DP83867_DOWNSHIFT_8_COUNT	8
+#define DP83867_SGMII_AUTONEG_EN	BIT(7)
 
 /* CFG3 bits */
 #define DP83867_CFG3_INT_OE			BIT(7)
@@ -855,6 +856,32 @@ static int dp83867_phy_reset(struct phy_device *phydev)
 			 DP83867_PHYCR_FORCE_LINK_GOOD, 0);
 }
 
+static void dp83867_link_change_notify(struct phy_device *phydev)
+{
+	/* There is a limitation in DP83867 PHY device where SGMII AN is
+	 * only triggered once after the device is booted up. Even after the
+	 * PHY TPI is down and up again, SGMII AN is not triggered and
+	 * hence no new in-band message from PHY to MAC side SGMII.
+	 * This could cause an issue during power up, when PHY is up prior
+	 * to MAC. At this condition, once MAC side SGMII is up, MAC side
+	 * SGMII wouldn`t receive new in-band message from TI PHY with
+	 * correct link status, speed and duplex info.
+	 * Thus, implemented a SW solution here to retrigger SGMII Auto-Neg
+	 * whenever there is a link change.
+	 */
+	if (phydev->interface == PHY_INTERFACE_MODE_SGMII) {
+		int val = 0;
+
+		val = phy_clear_bits(phydev, DP83867_CFG2,
+				     DP83867_SGMII_AUTONEG_EN);
+		if (val < 0)
+			return;
+
+		phy_set_bits(phydev, DP83867_CFG2,
+			     DP83867_SGMII_AUTONEG_EN);
+	}
+}
+
 static struct phy_driver dp83867_driver[] = {
 	{
 		.phy_id		= DP83867_PHY_ID,
@@ -879,6 +906,8 @@ static struct phy_driver dp83867_driver[] = {
 
 		.suspend	= genphy_suspend,
 		.resume		= genphy_resume,
+
+		.link_change_notify = dp83867_link_change_notify,
 	},
 };
 module_phy_driver(dp83867_driver);
-- 
2.25.1

Re: [PATCH net-next v2 1/1] net: phy: dp83867: retrigger SGMII AN when link change

[Andrew Lunn]

On Thu, May 26, 2022 at 05:03:47PM +0800, Tan Tee Min wrote:
> There is a limitation in TI DP83867 PHY device where SGMII AN is only
> triggered once after the device is booted up. Even after the PHY TPI is
> down and up again, SGMII AN is not triggered and hence no new in-band
> message from PHY to MAC side SGMII.
> 
> This could cause an issue during power up, when PHY is up prior to MAC.
> At this condition, once MAC side SGMII is up, MAC side SGMII wouldn`t
> receive new in-band message from TI PHY with correct link status, speed
> and duplex info.
> 
> As suggested by TI, implemented a SW solution here to retrigger SGMII
> Auto-Neg whenever there is a link change.

Is there a bit in the PHY which reports host side link? There is no
point triggering an AN if there is already link.

      Andrew

Re: [PATCH net-next v2 1/1] net: phy: dp83867: retrigger SGMII AN when link change

[Tan Tee Min]

On Thu, May 26, 2022 at 02:32:14PM +0200, Andrew Lunn wrote:
> On Thu, May 26, 2022 at 05:03:47PM +0800, Tan Tee Min wrote:
> > This could cause an issue during power up, when PHY is up prior to MAC.
> > At this condition, once MAC side SGMII is up, MAC side SGMII wouldn`t
> > receive new in-band message from TI PHY with correct link status, speed
> > and duplex info.
> > 
> > As suggested by TI, implemented a SW solution here to retrigger SGMII
> > Auto-Neg whenever there is a link change.
> 
> Is there a bit in the PHY which reports host side link? There is no
> point triggering an AN if there is already link.
> 
>       Andrew

Thanks for your comment.

There is no register bit in TI PHY which reports the SGMII AN link status.
But, there is a bit that only reports the SGMII AN completion status.

In this case, the PHY side SGMII AN has been already completed prior to MAC is up.
So, once MAC side SGMII is up, MAC side SGMII wouldn`t receive any new
in-band message from TI PHY.

Thanks,
Tee Min

Re: [PATCH net-next v2 1/1] net: phy: dp83867: retrigger SGMII AN when link change

[Andrew Lunn]

On Fri, May 27, 2022 at 09:47:09AM +0800, Tan Tee Min wrote:
> On Thu, May 26, 2022 at 02:32:14PM +0200, Andrew Lunn wrote:
> > On Thu, May 26, 2022 at 05:03:47PM +0800, Tan Tee Min wrote:
> > > This could cause an issue during power up, when PHY is up prior to MAC.
> > > At this condition, once MAC side SGMII is up, MAC side SGMII wouldn`t
> > > receive new in-band message from TI PHY with correct link status, speed
> > > and duplex info.
> > > 
> > > As suggested by TI, implemented a SW solution here to retrigger SGMII
> > > Auto-Neg whenever there is a link change.
> > 
> > Is there a bit in the PHY which reports host side link? There is no
> > point triggering an AN if there is already link.
> > 
> >       Andrew
> 
> Thanks for your comment.
> 
> There is no register bit in TI PHY which reports the SGMII AN link status.
> But, there is a bit that only reports the SGMII AN completion status.
> 
> In this case, the PHY side SGMII AN has been already completed prior to MAC is up.
> So, once MAC side SGMII is up, MAC side SGMII wouldn`t receive any new
> in-band message from TI PHY.

That does not make any sense for how i understand how this should
work.

Say the bootloader brings the MAC up, the SERDES gets sync and AN is
performed between the MAC and the PHY.

Linux takes over, downs the MAC and so the SERDES link is lost. The
PHY should notice this. Later Linux configures the MAC up, the SERDES
link should establish and AN should be performed.

Are you saying that the SERDES link is established, and stays
established, even when the MAC is down?

What is the structure of the host? Does it have a MAC block and a
SERDES block? It could be, the SERDES block is running independent of
the MAC block, and the link is established all the time, even when the
MAC is down. What you are missing is the MAC asking the SERDES block
for the results of the AN when the MAC comes up. So this is actually
an Ethernet driver bug, and you are working around it in the PHY
driver.

Are there registers in the MAC for the SERDES? Can you read the SERDES
link and AN state?

I have seen some MAC/SERDES combinations where you have to manually
move the AN results from the SERDES into the MAC. So could be, your
host will do it automatically is the MAC is up, but it won't do it if
the MAC is down when SERDES AN completes.

I just want to fully understand the issue, because if this is just a
workaround in the PHY, and you change the PHY, you are going to need
the same workaround in the next PHY driver.

    Andrew

Re: [PATCH net-next v2 1/1] net: phy: dp83867: retrigger SGMII AN when link change

[Tan Tee Min]

On Fri, May 27, 2022 at 02:43:04PM +0200, Andrew Lunn wrote:
> On Fri, May 27, 2022 at 09:47:09AM +0800, Tan Tee Min wrote:
> > On Thu, May 26, 2022 at 02:32:14PM +0200, Andrew Lunn wrote:
> > > On Thu, May 26, 2022 at 05:03:47PM +0800, Tan Tee Min wrote:
> > > > This could cause an issue during power up, when PHY is up prior to MAC.
> > > > At this condition, once MAC side SGMII is up, MAC side SGMII wouldn`t
> > > > receive new in-band message from TI PHY with correct link status, speed
> > > > and duplex info.
> > > > 
> > > > As suggested by TI, implemented a SW solution here to retrigger SGMII
> > > > Auto-Neg whenever there is a link change.
> > > 
> > > Is there a bit in the PHY which reports host side link? There is no
> > > point triggering an AN if there is already link.
> > > 
> > >       Andrew
> > 
> > Thanks for your comment.
> > 
> > There is no register bit in TI PHY which reports the SGMII AN link status.
> > But, there is a bit that only reports the SGMII AN completion status.
> > 
> > In this case, the PHY side SGMII AN has been already completed prior to MAC is up.
> > So, once MAC side SGMII is up, MAC side SGMII wouldn`t receive any new
> > in-band message from TI PHY.
> 
> That does not make any sense for how i understand how this should
> work.
> 
> Say the bootloader brings the MAC up, the SERDES gets sync and AN is
> performed between the MAC and the PHY.
> 
> Linux takes over, downs the MAC and so the SERDES link is lost. The
> PHY should notice this. Later Linux configures the MAC up, the SERDES
> link should establish and AN should be performed.
> 
> Are you saying that the SERDES link is established, and stays
> established, even when the MAC is down?
> 
> What is the structure of the host? Does it have a MAC block and a
> SERDES block? It could be, the SERDES block is running independent of
> the MAC block, and the link is established all the time, even when the
> MAC is down. What you are missing is the MAC asking the SERDES block
> for the results of the AN when the MAC comes up. So this is actually
> an Ethernet driver bug, and you are working around it in the PHY
> driver.
> 
> Are there registers in the MAC for the SERDES? Can you read the SERDES
> link and AN state?
> 
> I have seen some MAC/SERDES combinations where you have to manually
> move the AN results from the SERDES into the MAC. So could be, your
> host will do it automatically is the MAC is up, but it won't do it if
> the MAC is down when SERDES AN completes.
> 
> I just want to fully understand the issue, because if this is just a
> workaround in the PHY, and you change the PHY, you are going to need
> the same workaround in the next PHY driver.
> 
>     Andrew

Below is the HW structure for Intel mGbE controller with external PHY.
The SERDES is located in the PHY IF in the diagram below and the EQoS
MAC uses pcs-xpcs driver for SGMII interface.

    <-----------------GBE Controller---------->|<---External PHY chip--->
    +----------+         +----+            +---+           +------------+
    |   EQoS   | <-GMII->| DW | < ------ > |PHY| <-SGMII-> |External PHY|
    |   MAC    |         |xPCS|            |IF |           |(TI DP83867)|
    +----------+         +----+            +---+           +------------+
           ^               ^                 ^                ^
           |               |                 |                |
           +---------------------MDIO-------------------------+

There are registers in the DW XPCS to read the SGMII AN status and
it's showing the SGMII AN has not completed and link status is down.
But TI PHY is showing SGMII AN is completed and the copper link is
established.

FYI, the current pcs-xpcs driver is configuring C37 SGMII as MAC-side
SGMII, so it's expecting to receive AN Tx Config from PHY about the
link state change after C28 AN is completed between PHY and Link Partner.
Here is the pcs-xpcs code for your reference:
https://elixir.bootlin.com/linux/latest/source/drivers/net/pcs/pcs-xpcs.c#L725

We faced a similar issue on MaxLinear GPY PHY in the past.
And, MaxLinear folks admitted the issue and implemented fixes/improvements
in the GPY PHY Firmware to overcome the SGMII AN issue.
Besides, they have also implemented this similar SW Workaround in their
PHY driver code to cater for the old Firmware.
Feel free to refer GPY driver code here:
https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/mxl-gpy.c#L222

Apart from TI and MaxLinear PHY, we've also tested the Marvell 88E2110 and
88E1512 PHY with the MAC/SERDES combination above, Marvell PHY is working
fine without any issue.

Thanks,
Tee Min

Re: [PATCH net-next v2 1/1] net: phy: dp83867: retrigger SGMII AN when link change

[Andrew Lunn]

> Below is the HW structure for Intel mGbE controller with external PHY.
> The SERDES is located in the PHY IF in the diagram below and the EQoS
> MAC uses pcs-xpcs driver for SGMII interface.
> 
>     <-----------------GBE Controller---------->|<---External PHY chip--->
>     +----------+         +----+            +---+           +------------+
>     |   EQoS   | <-GMII->| DW | < ------ > |PHY| <-SGMII-> |External PHY|
>     |   MAC    |         |xPCS|            |IF |           |(TI DP83867)|
>     +----------+         +----+            +---+           +------------+
>            ^               ^                 ^                ^
>            |               |                 |                |
>            +---------------------MDIO-------------------------+
> 
> There are registers in the DW XPCS to read the SGMII AN status and
> it's showing the SGMII AN has not completed and link status is down.
> But TI PHY is showing SGMII AN is completed and the copper link is
> established.
> 
> FYI, the current pcs-xpcs driver is configuring C37 SGMII as MAC-side
> SGMII, so it's expecting to receive AN Tx Config from PHY about the
> link state change after C28 AN is completed between PHY and Link Partner.
> Here is the pcs-xpcs code for your reference:
> https://elixir.bootlin.com/linux/latest/source/drivers/net/pcs/pcs-xpcs.c#L725
> 
> We faced a similar issue on MaxLinear GPY PHY in the past.
> And, MaxLinear folks admitted the issue and implemented fixes/improvements
> in the GPY PHY Firmware to overcome the SGMII AN issue.
> Besides, they have also implemented this similar SW Workaround in their
> PHY driver code to cater for the old Firmware.
> Feel free to refer GPY driver code here:
> https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/mxl-gpy.c#L222
> 
> Apart from TI and MaxLinear PHY, we've also tested the Marvell 88E2110 and
> 88E1512 PHY with the MAC/SERDES combination above, Marvell PHY is working
> fine without any issue.

Thanks for the additional details.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next v2 1/1] net: phy: dp83867: retrigger SGMII AN when link change

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Thu, 26 May 2022 17:03:47 +0800 you wrote:
> There is a limitation in TI DP83867 PHY device where SGMII AN is only
> triggered once after the device is booted up. Even after the PHY TPI is
> down and up again, SGMII AN is not triggered and hence no new in-band
> message from PHY to MAC side SGMII.
> 
> This could cause an issue during power up, when PHY is up prior to MAC.
> At this condition, once MAC side SGMII is up, MAC side SGMII wouldn`t
> receive new in-band message from TI PHY with correct link status, speed
> and duplex info.
> 
> [...]

Here is the summary with links:
  - [net-next,v2,1/1] net: phy: dp83867: retrigger SGMII AN when link change
    https://git.kernel.org/netdev/net/c/c76acfb7e19d

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html