From mboxrd@z Thu Jan 1 00:00:00 1970 From: Florian Fainelli Subject: Re: [PATCH net-next 10/12] net: ethernet: Add helper for set_pauseparam for Asym Pause Date: Mon, 3 Sep 2018 10:49:20 -0700 Message-ID: References: <1535908001-18593-1-git-send-email-andrew@lunn.ch> <1535908001-18593-11-git-send-email-andrew@lunn.ch> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: netdev , maxime.chevallier@bootlin.com To: Andrew Lunn , David Miller Return-path: Received: from mail-pl1-f195.google.com ([209.85.214.195]:38598 "EHLO mail-pl1-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727414AbeICWKp (ORCPT ); Mon, 3 Sep 2018 18:10:45 -0400 Received: by mail-pl1-f195.google.com with SMTP id u11-v6so468442plq.5 for ; Mon, 03 Sep 2018 10:49:32 -0700 (PDT) In-Reply-To: <1535908001-18593-11-git-send-email-andrew@lunn.ch> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: 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