Re: [PATCH net-next 10/12] net: ethernet: Add helper for set_pauseparam for Asym Pause
On 09/03/18 12:58, Andrew Lunn wrote: >> 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? > > Hi Florian > > Given the number of changes i'm making, over a so many different > drivers which i cannot test, i wanted to try to keep the changes > KISS. That way we are more likely to spot errors during review. So i > would prefer to be done later, unless it actually makes review > simpler... That's fine, but you still need to keep the test for phydev->autoneg in xgene I believe. Since all of these drivers appear to be doing the same thing, that's why I suggested moving this into the helper directly. -- Florian
Re: [PATCH net-next 10/12] net: ethernet: Add helper for set_pauseparam for Asym Pause
> 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? Hi Florian Given the number of changes i'm making, over a so many different drivers which i cannot test, i wanted to try to keep the changes KISS. That way we are more likely to spot errors during review. So i would prefer to be done later, unless it actually makes review simpler... Andrew
Re: [PATCH net-next 10/12] net: ethernet: Add helper for set_pauseparam for Asym Pause
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
[PATCH net-next 10/12] net: ethernet: Add helper for set_pauseparam for Asym Pause
ethtool can be used to enable/disable pause. Add a helper to configure
the PHY when asym pause is supported.
Signed-off-by: Andrew Lunn
---
.../ethernet/apm/xgene/xgene_enet_ethtool.c | 26 ++---
drivers/net/ethernet/aurora/nb8800.c | 9 +---
drivers/net/ethernet/broadcom/tg3.c | 43 +--
drivers/net/ethernet/faraday/ftgmac100.c | 13 +
.../ethernet/freescale/dpaa/dpaa_ethtool.c| 26 ++---
.../net/ethernet/freescale/gianfar_ethtool.c | 53 +++
.../hisilicon/hns3/hns3pf/hclge_main.c| 8 +--
drivers/net/ethernet/socionext/sni_ave.c | 6 +--
drivers/net/phy/phy_device.c | 23
include/linux/phy.h | 1 +
10 files changed, 69 insertions(+), 139 deletions(-)
diff --git a/drivers/net/ethernet/apm/xgene/xgene_enet_ethtool.c
b/drivers/net/ethernet/apm/xgene/xgene_enet_ethtool.c
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);
-
- 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);
}
-
} else {
if (pp->autoneg)
return -EINVAL;
diff --git a/drivers/net/ethernet/aurora/nb8800.c
b/drivers/net/ethernet/aurora/nb8800.c
index c8d1f8fa4713..6f56276015a4 100644
--- a/drivers/net/ethernet/aurora/nb8800.c
+++ b/drivers/net/ethernet/aurora/nb8800.c
@@ -935,18 +935,11 @@ static void nb8800_pause_adv(struct net_device *dev)
{
struct nb8800_priv *priv = netdev_priv(dev);
struct phy_device *phydev = dev->phydev;
- u32 adv = 0;
if (!phydev)
return;
- if (priv->pause_rx)
- adv |= ADVERTISED_Pause | ADVERTISED_Asym_Pause;
- if (priv->pause_tx)
- adv ^= ADVERTISED_Asym_Pause;
-
- phydev->supported |= adv;
- phydev->advertising |= adv;
+ phy_set_asym_pause(phydev, priv->pause_rx, priv->pause_tx);
}
static int nb8800_open(struct net_device *dev)
diff --git a/drivers/net/ethernet/broadcom/tg3.c
b/drivers/net/ethernet/broadcom/tg3.c
index 9aa7955d5d31..b406b6e34a8c 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -12492,7 +12492,6 @@ static int tg3_set_pauseparam(struct net_device *dev,
struct ethtool_pauseparam
tg3_warn_mgmt_link_flap(tp);
if (tg3_flag(tp, USE_PHYLIB)) {
- u32 newadv;
struct phy_device *phydev;
phydev = mdiobus_get_phy(tp->mdio_bus, tp->phy_addr);
@@ -12503,20 +12502,16 @@ static int tg3_set_pauseparam(struct net_device *dev,
struct ethtool_pauseparam
return -EINVAL;
tp->link_config.flowctrl = 0;
+ phy_set_asym_pause(phydev, epause->rx_pause, epause->tx_pause);
if (epause->rx_pause) {
tp->link_config.flowctrl |= FLOW_CTRL_RX;
if (epause->tx_pause) {
tp->link_config.flowctrl |= FLOW_CTRL_TX;
- newadv = ADVERTISED_Pause;
- } else
- newadv = ADVERTISED_Pause |
-ADVERTISED_Asym_Pause;
+ }
}
