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

[Andrew Lunn <andrew@lunn.ch>]

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