drivers/net/ethernet/spacemit/k1_emac.c | 48 ++++++++++++++------------------- 1 file changed, 20 insertions(+), 28 deletions(-)
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" WangOn 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
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
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
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
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/
© 2016 - 2026 Red Hat, Inc.