Re: [PATCH net-next 10/12] net: ethernet: Add helper for set_pauseparam for Asym Pause

[Florian Fainelli]

On 9/2/2018 10:06 AM, Andrew Lunn wrote:
ethtool can be used to enable/disable pause. Add a helper to configure
the PHY when asym pause is supported.

Don't you want to go one step further and incorporate the logic that 
xgenet, tg3, gianfar and others have? That is: look at the currently 
advertised parameters, determine what changed, and re-start 
auto-negotiation as a result of it being enabled and something changed?

Could be done as a follow-up patch I suppose, although see below:

[snip]
index 4f50f11718f4..dfe03afd00b0 100644
--- a/drivers/net/ethernet/apm/xgene/xgene_enet_ethtool.c
+++ b/drivers/net/ethernet/apm/xgene/xgene_enet_ethtool.c
@@ -306,7 +306,6 @@ static int xgene_set_pauseparam(struct net_device *ndev,
  {
        struct xgene_enet_pdata *pdata = netdev_priv(ndev);
        struct phy_device *phydev = ndev->phydev;
-       u32 oldadv, newadv;
  
  	if (phy_interface_mode_is_rgmii(pdata->phy_mode) ||
            pdata->phy_mode == PHY_INTERFACE_MODE_SGMII) {
@@ -322,29 +321,12 @@ static int xgene_set_pauseparam(struct net_device *ndev,
                pdata->tx_pause = pp->tx_pause;
                pdata->rx_pause = pp->rx_pause;
  
-		oldadv = phydev->advertising;
-               newadv = oldadv & ~(ADVERTISED_Pause | ADVERTISED_Asym_Pause);
+               phy_set_asym_pause(phydev, pp->rx_pause,  pp->tx_pause);
  
-		if (pp->rx_pause)
-                       newadv |= ADVERTISED_Pause | ADVERTISED_Asym_Pause;
-
-               if (pp->tx_pause)
-                       newadv ^= ADVERTISED_Asym_Pause;
-
-               if (oldadv ^ newadv) {
-                       phydev->advertising = newadv;
-
-                       if (phydev->autoneg)
-                               return phy_start_aneg(phydev);
-

You have remove the part that checks for phydev->autoneg here, was that 
intentional?

-                       if (!pp->autoneg) {
-                               pdata->mac_ops->flowctl_tx(pdata,
-                                                          pdata->tx_pause);
-                               pdata->mac_ops->flowctl_rx(pdata,
-                                                          pdata->rx_pause);
-                       }
+               if (!pp->autoneg) {
+                       pdata->mac_ops->flowctl_tx(pdata, pdata->tx_pause);
+                       pdata->mac_ops->flowctl_rx(pdata, pdata->rx_pause);
                }


--
Florian