[PATCH net] net: spacemit: Implement emac_set_pauseparam properly

[PATCH net] net: spacemit: Implement emac_set_pauseparam properly

[Vivian Wang]

emac_set_pauseparam (the set_pauseparam callback) didn't properly update
phydev->advertising. Fix it by changing it to call phy_set_asym_pause.

Also simplify/reorganize related code around this.

Fixes: bfec6d7f2001 ("net: spacemit: Add K1 Ethernet MAC")
Signed-off-by: Vivian Wang <wangruikang@iscas.ac.cn>
---
 drivers/net/ethernet/spacemit/k1_emac.c | 48 ++++++++++++++-------------------
 1 file changed, 20 insertions(+), 28 deletions(-)

diff --git a/drivers/net/ethernet/spacemit/k1_emac.c b/drivers/net/ethernet/spacemit/k1_emac.c
index e1c5faff3b71..61d62c0f028e 100644
--- a/drivers/net/ethernet/spacemit/k1_emac.c
+++ b/drivers/net/ethernet/spacemit/k1_emac.c
@@ -133,7 +133,6 @@ struct emac_priv {
 	u32 rx_delay;
 
 	bool flow_control_autoneg;
-	u8 flow_control;
 
 	/* Softirq-safe, hold while touching hardware statistics */
 	spinlock_t stats_lock;
@@ -1039,14 +1038,7 @@ static void emac_set_rx_fc(struct emac_priv *priv, bool enable)
 	emac_wr(priv, MAC_FC_CONTROL, val);
 }
 
-static void emac_set_fc(struct emac_priv *priv, u8 fc)
-{
-	emac_set_tx_fc(priv, fc & FLOW_CTRL_TX);
-	emac_set_rx_fc(priv, fc & FLOW_CTRL_RX);
-	priv->flow_control = fc;
-}
-
-static void emac_set_fc_autoneg(struct emac_priv *priv)
+static void emac_set_fc(struct emac_priv *priv)
 {
 	struct phy_device *phydev = priv->ndev->phydev;
 	u32 local_adv, remote_adv;
@@ -1056,17 +1048,18 @@ static void emac_set_fc_autoneg(struct emac_priv *priv)
 
 	remote_adv = 0;
 
-	if (phydev->pause)
+	/* Force settings in advertising if autoneg disabled */
+
+	if (!priv->flow_control_autoneg || phydev->pause)
 		remote_adv |= LPA_PAUSE_CAP;
 
-	if (phydev->asym_pause)
+	if (!priv->flow_control_autoneg || phydev->asym_pause)
 		remote_adv |= LPA_PAUSE_ASYM;
 
 	fc = mii_resolve_flowctrl_fdx(local_adv, remote_adv);
 
-	priv->flow_control_autoneg = true;
-
-	emac_set_fc(priv, fc);
+	emac_set_tx_fc(priv, fc & FLOW_CTRL_TX);
+	emac_set_rx_fc(priv, fc & FLOW_CTRL_RX);
 }
 
 /*
@@ -1429,31 +1422,28 @@ static void emac_get_pauseparam(struct net_device *dev,
 				struct ethtool_pauseparam *pause)
 {
 	struct emac_priv *priv = netdev_priv(dev);
+	u32 val = emac_rd(priv, MAC_FC_CONTROL);
 
 	pause->autoneg = priv->flow_control_autoneg;
-	pause->tx_pause = !!(priv->flow_control & FLOW_CTRL_TX);
-	pause->rx_pause = !!(priv->flow_control & FLOW_CTRL_RX);
+	pause->tx_pause = !!(val & MREGBIT_FC_GENERATION_ENABLE);
+	pause->rx_pause = !!(val & MREGBIT_FC_DECODE_ENABLE);
 }
 
 static int emac_set_pauseparam(struct net_device *dev,
 			       struct ethtool_pauseparam *pause)
 {
 	struct emac_priv *priv = netdev_priv(dev);
-	u8 fc = 0;
+	struct phy_device *phydev = dev->phydev;
 
-	priv->flow_control_autoneg = pause->autoneg;
+	if (!phydev)
+		return -ENODEV;
 
-	if (pause->autoneg) {
-		emac_set_fc_autoneg(priv);
-	} else {
-		if (pause->tx_pause)
-			fc |= FLOW_CTRL_TX;
+	if (!phy_validate_pause(phydev, pause))
+		return -EINVAL;
 
-		if (pause->rx_pause)
-			fc |= FLOW_CTRL_RX;
+	priv->flow_control_autoneg = pause->autoneg;
 
-		emac_set_fc(priv, fc);
-	}
+	phy_set_asym_pause(dev->phydev, pause->rx_pause, pause->tx_pause);
 
 	return 0;
 }
@@ -1632,7 +1622,7 @@ static void emac_adjust_link(struct net_device *dev)
 
 		emac_wr(priv, MAC_GLOBAL_CONTROL, ctrl);
 
-		emac_set_fc_autoneg(priv);
+		emac_set_fc(priv);
 	}
 
 	phy_print_status(phydev);
@@ -2010,6 +2000,8 @@ static int emac_probe(struct platform_device *pdev)
 	priv->pdev = pdev;
 	platform_set_drvdata(pdev, priv);
 
+	priv->flow_control_autoneg = true;
+
 	ret = emac_config_dt(pdev, priv);
 	if (ret < 0)
 		return dev_err_probe(dev, ret, "Configuration failed\n");

---
base-commit: cb6649f6217c0331b885cf787f1d175963e2a1d2
change-id: 20251030-k1-ethernet-fix-autoneg-ae2a92b3c2db

Best regards,
-- 
Vivian "dramforever" Wang

Re: [PATCH net] net: spacemit: Implement emac_set_pauseparam properly

[Michael Opdenacker]

On 10/30/25 15:31, Vivian Wang wrote:
> emac_set_pauseparam (the set_pauseparam callback) didn't properly update
> phydev->advertising. Fix it by changing it to call phy_set_asym_pause.
>
> Also simplify/reorganize related code around this.
>
> Fixes: bfec6d7f2001 ("net: spacemit: Add K1 Ethernet MAC")
> Signed-off-by: Vivian Wang <wangruikang@iscas.ac.cn>
> ---
>   drivers/net/ethernet/spacemit/k1_emac.c | 48 ++++++++++++++-------------------
>   1 file changed, 20 insertions(+), 28 deletions(-)


Tested on OrangePi RV2 through performance tests, on 
https://github.com/spacemit-com/linux/commits/for-next. No regressions 
found:

root@orangepi-rv2-mainline:~# iperf3 -c 172.24.0.1
Connecting to host 172.24.0.1, port 5201
[  5] local 172.24.0.2 port 49948 connected to 172.24.0.1 port 5201
[ ID] Interval           Transfer     Bitrate         Retr  Cwnd
[  5]   0.00-1.00   sec   113 MBytes   946 Mbits/sec    0    339 KBytes
[  5]   1.00-2.00   sec   112 MBytes   943 Mbits/sec    0    447 KBytes
[  5]   2.00-3.00   sec   113 MBytes   948 Mbits/sec    0    447 KBytes
[  5]   3.00-4.00   sec   112 MBytes   941 Mbits/sec    0    475 KBytes
[  5]   4.00-5.00   sec   112 MBytes   940 Mbits/sec    0    505 KBytes
[  5]   5.00-6.00   sec   112 MBytes   944 Mbits/sec    0    567 KBytes
[  5]   6.00-7.00   sec   113 MBytes   949 Mbits/sec    0    600 KBytes
[  5]   7.00-8.00   sec   112 MBytes   939 Mbits/sec    0    600 KBytes
[  5]   8.00-9.00   sec   112 MBytes   936 Mbits/sec    0    600 KBytes
[  5]   9.00-10.01  sec   113 MBytes   940 Mbits/sec    0    600 KBytes
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval           Transfer     Bitrate         Retr
[  5]   0.00-10.01  sec  1.10 GBytes   943 Mbits/sec    0   sender
[  5]   0.00-10.02  sec  1.10 GBytes   940 Mbits/sec     receiver

iperf Done.
root@orangepi-rv2-mainline:~# iperf3 -s
-----------------------------------------------------------
Server listening on 5201 (test #1)
-----------------------------------------------------------
Accepted connection from 172.24.0.1, port 47834
[  5] local 172.24.0.2 port 5201 connected to 172.24.0.1 port 47840
[ ID] Interval           Transfer     Bitrate
[  5]   0.00-1.00   sec   112 MBytes   934 Mbits/sec
[  5]   1.00-2.00   sec   112 MBytes   941 Mbits/sec
[  5]   2.00-3.00   sec   112 MBytes   942 Mbits/sec
[  5]   3.00-4.00   sec   112 MBytes   942 Mbits/sec
[  5]   4.00-5.00   sec   112 MBytes   942 Mbits/sec
[  5]   5.00-6.00   sec   112 MBytes   942 Mbits/sec
[  5]   6.00-7.00   sec   112 MBytes   942 Mbits/sec
[  5]   7.00-8.00   sec   112 MBytes   942 Mbits/sec
[  5]   8.00-9.00   sec   112 MBytes   941 Mbits/sec
[  5]   9.00-10.00  sec   112 MBytes   942 Mbits/sec
[  5]  10.00-10.01  sec   640 KBytes  1.04 Gbits/sec
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval           Transfer     Bitrate
[  5]   0.00-10.01  sec  1.10 GBytes   941 Mbits/sec     receiver
-----------------------------------------------------------
Server listening on 5201 (test #2)
-----------------------------------------------------------

Tested-by: Michael Opdenacker <michael.opdenacker@rootcommit.com>
Thanks!
Michael.

-- 
Michael Opdenacker
Root Commit
Yocto Project and OpenEmbedded Training course - Learn by doing:
https://rootcommit.com/training/yocto/

Re: [PATCH net] net: spacemit: Implement emac_set_pauseparam properly

[Andrew Lunn]

On Thu, Oct 30, 2025 at 10:31:44PM +0800, Vivian Wang wrote:
> emac_set_pauseparam (the set_pauseparam callback) didn't properly update
> phydev->advertising. Fix it by changing it to call phy_set_asym_pause.

This patch is doing a lot more than that.

Please break this patch up into smaller parts.

One obvious part you can break out is emac_get_pauseparam() reading
from hardware rather that state variables.

>  static int emac_set_pauseparam(struct net_device *dev,
>  			       struct ethtool_pauseparam *pause)
>  {
>  	struct emac_priv *priv = netdev_priv(dev);
> -	u8 fc = 0;
> +	struct phy_device *phydev = dev->phydev;
>  
> -	priv->flow_control_autoneg = pause->autoneg;
> +	if (!phydev)
> +		return -ENODEV;

I'm not sure that is the correct condition. emac_up() will fail if it
cannot find the PHY. What you need to be testing here is if the
interface is admin down, and so is not connected to the PHY. If so,
-ENETDOWN would be more appropriate.

> -	if (pause->autoneg) {
> -		emac_set_fc_autoneg(priv);
> -	} else {
> -		if (pause->tx_pause)
> -			fc |= FLOW_CTRL_TX;
> +	if (!phy_validate_pause(phydev, pause))
> +		return -EINVAL;
>  
> -		if (pause->rx_pause)
> -			fc |= FLOW_CTRL_RX;
> +	priv->flow_control_autoneg = pause->autoneg;
>  
> -		emac_set_fc(priv, fc);
> -	}
> +	phy_set_asym_pause(dev->phydev, pause->rx_pause, pause->tx_pause);

It is hard to read what this patch is doing, but there are 3 use cases.

1) general autoneg for link speed etc, and pause autoneg
2) general autoneg for link speed etc, and forced pause
3) forced link speed etc, and forced pause.

I don't see all these being handled. It gets much easier to get this
right if you make use of phylink, since phylink handles all the
business logic for you.

	Andrew

Re: [PATCH net] net: spacemit: Implement emac_set_pauseparam properly

[Vivian Wang]

On 10/31/25 05:32, Andrew Lunn wrote:
>> [...]
>>
>> -		emac_set_fc(priv, fc);
>> -	}
>> +	phy_set_asym_pause(dev->phydev, pause->rx_pause, pause->tx_pause);
> It is hard to read what this patch is doing, but there are 3 use cases.

Yeah, I guess the patch doesn't look great. I'll reorganize it in the
next version to make it clearer what the new implementation is and also
fix it up per your other comments.

> 1) general autoneg for link speed etc, and pause autoneg
> 2) general autoneg for link speed etc, and forced pause
> 3) forced link speed etc, and forced pause.

Thanks for the tip on the different cases. However, there's one bit I
don't really understand: Isn't this set_pauseparam thing only for
setting pause autoneg / force?

AFAICT from my testing, forcing link speed and duplex from ethtool still
works, so I'm not sure what I'm missing here.

> I don't see all these being handled. It gets much easier to get this
> right if you make use of phylink, since phylink handles all the
> business logic for you.

I'll look into calling phylink next version. Thanks.

Vivian "dramforever" Wang

Re: [PATCH net] net: spacemit: Implement emac_set_pauseparam properly

[Andrew Lunn]

On Fri, Oct 31, 2025 at 03:22:56PM +0800, Vivian Wang wrote:
> 
> On 10/31/25 05:32, Andrew Lunn wrote:
> >> [...]
> >>
> >> -		emac_set_fc(priv, fc);
> >> -	}
> >> +	phy_set_asym_pause(dev->phydev, pause->rx_pause, pause->tx_pause);
> > It is hard to read what this patch is doing, but there are 3 use cases.
> 
> Yeah, I guess the patch doesn't look great. I'll reorganize it in the
> next version to make it clearer what the new implementation is and also
> fix it up per your other comments.
> 
> > 1) general autoneg for link speed etc, and pause autoneg
> > 2) general autoneg for link speed etc, and forced pause
> > 3) forced link speed etc, and forced pause.
> 
> Thanks for the tip on the different cases. However, there's one bit I
> don't really understand: Isn't this set_pauseparam thing only for
> setting pause autoneg / force?

Nope. You need to think about how it interacts with generic autoneg.

       ethtool -A|--pause devname [autoneg on|off] [rx on|off] [tx on|off]

       ethtool -s devname [speed N] [lanes N] [duplex half|full]
              [port tp|aui|bnc|mii] [mdix auto|on|off] [autoneg on|off]

These autoneg are different things. -s is about generic autoneg,
speed, duplex, etc. However pause can also be negotiated, or not,
using -A.

You can only autoneg pause if you are doing generic autoneg. So there
are three combinations you need to handle.

With pause autoneg off, you can set registers in the MAC immediately,
but you need to be careful not to overwrite the values when generic
autoneg completes and the adjust_link callback is called.

If you have pause autoneg on, you have to wait for the adjust_link
callback to be called with the results of the negotiation, including
pause.

phylink hides all this logic. There is a link_up callback, which tells
you how to program the hardware. You just do it, no logic needed.

	Andrew

Re: [PATCH net] net: spacemit: Implement emac_set_pauseparam properly

[Vivian Wang]

On 10/31/25 20:43, Andrew Lunn wrote:
> On Fri, Oct 31, 2025 at 03:22:56PM +0800, Vivian Wang wrote:
>> On 10/31/25 05:32, Andrew Lunn wrote:
>>>> [...]
>>>>
>>>> -		emac_set_fc(priv, fc);
>>>> -	}
>>>> +	phy_set_asym_pause(dev->phydev, pause->rx_pause, pause->tx_pause);
>>> It is hard to read what this patch is doing, but there are 3 use cases.
>> Yeah, I guess the patch doesn't look great. I'll reorganize it in the
>> next version to make it clearer what the new implementation is and also
>> fix it up per your other comments.
>>
>>> 1) general autoneg for link speed etc, and pause autoneg
>>> 2) general autoneg for link speed etc, and forced pause
>>> 3) forced link speed etc, and forced pause.
>> Thanks for the tip on the different cases. However, there's one bit I
>> don't really understand: Isn't this set_pauseparam thing only for
>> setting pause autoneg / force?
> Nope. You need to think about how it interacts with generic autoneg.
>
>        ethtool -A|--pause devname [autoneg on|off] [rx on|off] [tx on|off]
>
>        ethtool -s devname [speed N] [lanes N] [duplex half|full]
>               [port tp|aui|bnc|mii] [mdix auto|on|off] [autoneg on|off]
>
> These autoneg are different things. -s is about generic autoneg,
> speed, duplex, etc. However pause can also be negotiated, or not,
> using -A.
>
> You can only autoneg pause if you are doing generic autoneg. So there
> are three combinations you need to handle.

Oh, that is what I had missed. I hadn't understood this part before. Thanks.

> With pause autoneg off, you can set registers in the MAC immediately,
> but you need to be careful not to overwrite the values when generic
> autoneg completes and the adjust_link callback is called.
>
> If you have pause autoneg on, you have to wait for the adjust_link
> callback to be called with the results of the negotiation, including
> pause.
>
> phylink hides all this logic. There is a link_up callback, which tells
> you how to program the hardware. You just do it, no logic needed.

This makes sense. I'll look into using phylink.

Thanks,
Vivian "dramforever" Wang