[v3] net: dsa: qca8k: enable port flow control

Commit Message

xiaofeis July 28, 2019, 12:57 a.m. UTC

Set phy device advertising to enable MAC flow control.

Signed-off-by: Xiaofei Shen <[e-mail]>
---
Changes since V2:
 drivers/net/dsa/qca8k.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Andrew Lunn July 28, 2019, 10:31 p.m. UTC | #1

On Sun, Jul 28, 2019 at 08:57:50AM +0800, xiaofeis wrote:
> Set phy device advertising to enable MAC flow control.

Hi Xiaofei.

This is half of the needed change for MAC flow control.

phy_support_asym_pause(phy) is used by the MAC to tell the PHY layer
that the MAC supports flow control. The PHY will then advertise
this. When auto-negotiation is completed, the PHY layer will call
qca8k_adjust_link() with the results. It could be that the peer does
not support flow control, or only supports symmetric flow control.  So
in that function, you need to program the MAC with the results of the
auto-neg. This is currently missing. You need to look at phydev->pause
and phydev->asym_pause to decide how to configure the MAC.

       Andrew

xiaofeis July 29, 2019, 5:01 a.m. UTC | #2

On 2019-07-29 06:31, Andrew Lunn wrote:
> On Sun, Jul 28, 2019 at 08:57:50AM +0800, xiaofeis wrote:
>> Set phy device advertising to enable MAC flow control.
> 
> Hi Xiaofei.
> 
> This is half of the needed change for MAC flow control.
> 
> phy_support_asym_pause(phy) is used by the MAC to tell the PHY layer
> that the MAC supports flow control. The PHY will then advertise
> this. When auto-negotiation is completed, the PHY layer will call
> qca8k_adjust_link() with the results. It could be that the peer does
> not support flow control, or only supports symmetric flow control.  So
> in that function, you need to program the MAC with the results of the
> auto-neg. This is currently missing. You need to look at phydev->pause
> and phydev->asym_pause to decide how to configure the MAC.
> 
>        Andrew
Hi Andrew

You are correct. With the change, the auto-negotiation result still 
depends on the peer.

But our qca8k HW can auto sync the pause status to MAC from phy with the 
auto-negotiated result.
So no need to set in qca8k_adjust_link, since there is one setting in 
qca8k_port_set_status: mask |= QCA8K_PORT_STATUS_LINK_AUTO;

This change's purpose is to keep enable advertise on our side.

Thanks
Xiaofeis

Andrew Lunn July 29, 2019, 1:23 p.m. UTC | #3

> But our qca8k HW can auto sync the pause status to MAC from phy with the
> auto-negotiated result.
> So no need to set in qca8k_adjust_link, since there is one setting in
> qca8k_port_set_status: mask |= QCA8K_PORT_STATUS_LINK_AUTO;

How does the auto-sync actually work? Does the MAC make MDIO reads to
the PHY? That is generally unsafe, since some PHYs support pages, and
the PHY driver might be using a different page while the MAC tries to
access the auto-neg results.

Do any of the ports support an external PHY? The auto-sync might not
work in that condition as well. Different register layout, c45 not
c22, etc.

The safest option is to explicitly set the MAC flow configuration
based on the values in phydev.

      Andrew

xiaofeis July 30, 2019, 3:01 a.m. UTC | #4

On 2019-07-29 21:23, Andrew Lunn wrote:
>> But our qca8k HW can auto sync the pause status to MAC from phy with 
>> the
>> auto-negotiated result.
>> So no need to set in qca8k_adjust_link, since there is one setting in
>> qca8k_port_set_status: mask |= QCA8K_PORT_STATUS_LINK_AUTO;
> 
> How does the auto-sync actually work? Does the MAC make MDIO reads to
> the PHY? That is generally unsafe, since some PHYs support pages, and
> the PHY driver might be using a different page while the MAC tries to
> access the auto-neg results.
> 
> Do any of the ports support an external PHY? The auto-sync might not
> work in that condition as well. Different register layout, c45 not
> c22, etc.
> 
> The safest option is to explicitly set the MAC flow configuration
> based on the values in phydev.
> 
Hi Andrew

To explicitly set the configuration is one option, but then we need 
disable QCA8K_PORT_STATUS_LINK_AUTO, and not only flow control,
speed/duplex all need to be set explicitly.

Here the qca8k driver is only support qca,qca8337/qca8334 switch family. 
The phy is internal phy. There is signals internaly
to sync the status from phy to mac directly which is HW specific 
implementation, it doesn't support to connect external phy.

Thanks
Xiaofeis

David Miller July 30, 2019, 10:08 p.m. UTC | #5

From: xiaofeis <[e-mail]>
Date: Sun, 28 Jul 2019 08:57:50 +0800

> Set phy device advertising to enable MAC flow control.
> 
> Signed-off-by: Xiaofei Shen <[e-mail]>

I've read the discussion over a few times and if internal PHY is the
only thing supported, and the specific setup this driver supports uses
internal signalling to sync the PHY and MAC settings I guess this is
OK although suboptimal.

So applied, thanks.

Thanks.

Patch

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 232e8cc..e429e92 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -2,7 +2,7 @@ 
 /*
  * Copyright (C) 2009 Felix Fietkau <[e-mail]>
  * Copyright (C) 2011-2012 Gabor Juhos <[e-mail]>
- * Copyright (c) 2015, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2015, 2019, The Linux Foundation. All rights reserved.
  * Copyright (c) 2016 John Crispin <[e-mail]>
  */
 
@@ -935,6 +935,8 @@ 
 	qca8k_port_set_status(priv, port, 1);
 	priv->port_sts[port].enabled = 1;
 
+	phy_support_asym_pause(phy);
+
 	return 0;
 }