| Message ID | 1375871056-10420-2-git-send-email-claudiu.manoil@freescale.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, 2013-08-07 at 13:24 +0300, Claudiu Manoil wrote: > Allow Rx/Tx pause frame configuration via ethtool. > The gfar devices feature link autonegotioation by default. So the MAC configuration bits are actually copied to the PHY autoneg basic page, and then the PHY autoneg result is automatically used by the MAC? This is of course possible to do in hardware, but... since this MAC is not smart enough to ignore pause settings when running in half-duplex mode, I seriously doubt it is doing all this by itself. > The device is being configured with the new pause frame > parameters if the link is up, depending on link duplex (no > pause frames for half-duplex links), or during link autoneg > (see adjust_link()). [...] > --- a/drivers/net/ethernet/freescale/gianfar_ethtool.c > +++ b/drivers/net/ethernet/freescale/gianfar_ethtool.c > @@ -535,6 +535,34 @@ static int gfar_sringparam(struct net_device *dev, > return err; > } > > +static void gfar_gpauseparam(struct net_device *dev, > + struct ethtool_pauseparam *pause) > +{ > + struct gfar_private *priv = netdev_priv(dev); > + > + pause->autoneg = AUTONEG_ENABLE; > + if (priv->rx_pause) > + pause->rx_pause = 1; > + if (priv->tx_pause) > + pause->tx_pause = 1; > +} > + > +static int gfar_spauseparam(struct net_device *dev, > + struct ethtool_pauseparam *pause) > +{ > + struct gfar_private *priv = netdev_priv(dev); > + struct phy_device *phydev = priv->phydev; You need to reject an unsupported setting of pause->autoneg here. Ben. > + priv->rx_pause = !!pause->rx_pause; > + priv->tx_pause = !!pause->tx_pause; > + > + /* update h/w settings, if link is up */ > + if (phydev && phydev->link) > + gfar_configure_pause(priv, !!phydev->duplex); > + > + return 0; > +} > + > int gfar_set_features(struct net_device *dev, netdev_features_t features) > { > struct gfar_private *priv = netdev_priv(dev); [...]
Hi Ben, On 8/7/2013 10:12 PM, Ben Hutchings wrote: > On Wed, 2013-08-07 at 13:24 +0300, Claudiu Manoil wrote: >> Allow Rx/Tx pause frame configuration via ethtool. >> The gfar devices feature link autonegotioation by default. > > So the MAC configuration bits are actually copied to the PHY autoneg > basic page, and then the PHY autoneg result is automatically used by the > MAC? > > This is of course possible to do in hardware, but... since this MAC is > not smart enough to ignore pause settings when running in half-duplex > mode, I seriously doubt it is doing all this by itself. > I just wanted to say actually that the pause->autoneg parameter is not needed by the gianfar driver, but I didn't know what to do with it in get_pauseparam(), apparently pause->autoneg needs a value (or can simply ignore this param?). I don't see what autonegotiation has to do with enabling/disabling pause frame generation in this case. My understanding is that link autonegotiation is taken care somewhere else, by the phy state machine. Each time this happens, the gianfar driver gets notified via the adjust_link() hook that it implements and makes the necessary configs in the mac registers. Besides, autoneg info is already being displayed by ethtool (see print below). So I don't understand the use of pause->autoneg. What should I do with it? Thanks, Claudiu
On Thu, 2013-08-08 at 20:10 +0300, Claudiu Manoil wrote: > Hi Ben, > > On 8/7/2013 10:12 PM, Ben Hutchings wrote: > > On Wed, 2013-08-07 at 13:24 +0300, Claudiu Manoil wrote: > >> Allow Rx/Tx pause frame configuration via ethtool. > >> The gfar devices feature link autonegotioation by default. > > > > So the MAC configuration bits are actually copied to the PHY autoneg > > basic page, and then the PHY autoneg result is automatically used by the > > MAC? > > > > This is of course possible to do in hardware, but... since this MAC is > > not smart enough to ignore pause settings when running in half-duplex > > mode, I seriously doubt it is doing all this by itself. > > > > I just wanted to say actually that the pause->autoneg parameter is not > needed by the gianfar driver, but I didn't know what to do with it in > get_pauseparam(), apparently pause->autoneg needs a value (or can > simply ignore this param?). > > I don't see what autonegotiation has to do with enabling/disabling > pause frame generation in this case. My understanding is that link > autonegotiation is taken care somewhere else, by the phy state machine. > Each time this happens, the gianfar driver gets notified via the > adjust_link() hook that it implements and makes the necessary configs > in the mac registers. That's what it *should* be doing. But it's not; it's using priv->rx_pause and priv->tx_pause, not phydev->pause and phydev->asym_pause. I.e. pause autoneg is really always *disabled*. > Besides, autoneg info is already being displayed by ethtool (see > print below). > So I don't understand the use of pause->autoneg. What should I do with > it? [...] Pause frame generation may be forced even though autonegotiation is enabled. I don't think this is a good idea but many drivers do support it. And gianfar does appear to allow autonegotiation to be disabled, anyway, so in any case you have to support both forced and autonegotiated cases. Ben.
I have been working on the same issue and came up with a similar
modification (that additionally implements LFC if supported)
I have attached the two patches created:
* gianfar: implement flow control handling
which looks quite similar to Claudiu's patches
* gianfar: add support for LFC (Lossless Flow Control)
which is more or less just extracted from some patch I found on the internet
linux-2.6.29.6-mpc8308e_rdb.patch
The patches I have taken from a custom 3.4 based kernel so they may not
apply cleanly but maybe bits of it can be used to generate fully supported
solution.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks, First patch seems to include the complete implementation of ethtool -A, fixing the pause issue for half-duplex links as well, and it also implements pause autoneg. (nice) I'm going to apply an test this code and re-spin the resulting patch(es). Are you the author of this patch? (gianfar: implement flow control handling) Claudiu On 8/9/2013 11:26 AM, Lutz Jaenicke wrote: > I have been working on the same issue and came up with a similar > modification (that additionally implements LFC if supported) > I have attached the two patches created: > * gianfar: implement flow control handling > which looks quite similar to Claudiu's patches > * gianfar: add support for LFC (Lossless Flow Control) > which is more or less just extracted from some patch I found on the internet > linux-2.6.29.6-mpc8308e_rdb.patch > The patches I have taken from a custom 3.4 based kernel so they may not > apply cleanly but maybe bits of it can be used to generate fully supported > solution. > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 09, 2013 at 01:12:15PM +0300, Claudiu Manoil wrote: > Thanks, > > First patch seems to include the complete implementation of ethtool -A, > fixing the pause issue for half-duplex links as well, and it also > implements pause autoneg. (nice) > I'm going to apply an test this code and re-spin the resulting > patch(es). > Are you the author of this patch? > (gianfar: implement flow control handling) Yes. Best regards, Lutz
diff --git a/drivers/net/ethernet/freescale/gianfar.h b/drivers/net/ethernet/freescale/gianfar.h index e6a03f4..aaac843 100644 --- a/drivers/net/ethernet/freescale/gianfar.h +++ b/drivers/net/ethernet/freescale/gianfar.h @@ -1186,6 +1186,7 @@ void gfar_init_sysfs(struct net_device *dev); int gfar_set_features(struct net_device *dev, netdev_features_t features); extern void gfar_check_rx_parser_mode(struct gfar_private *priv); extern void gfar_vlan_mode(struct net_device *dev, netdev_features_t features); +void gfar_configure_pause(struct gfar_private *priv, bool en); extern const struct ethtool_ops gfar_ethtool_ops; diff --git a/drivers/net/ethernet/freescale/gianfar_ethtool.c b/drivers/net/ethernet/freescale/gianfar_ethtool.c index 21cd881..6cf89c1 100644 --- a/drivers/net/ethernet/freescale/gianfar_ethtool.c +++ b/drivers/net/ethernet/freescale/gianfar_ethtool.c @@ -535,6 +535,34 @@ static int gfar_sringparam(struct net_device *dev, return err; } +static void gfar_gpauseparam(struct net_device *dev, + struct ethtool_pauseparam *pause) +{ + struct gfar_private *priv = netdev_priv(dev); + + pause->autoneg = AUTONEG_ENABLE; + if (priv->rx_pause) + pause->rx_pause = 1; + if (priv->tx_pause) + pause->tx_pause = 1; +} + +static int gfar_spauseparam(struct net_device *dev, + struct ethtool_pauseparam *pause) +{ + struct gfar_private *priv = netdev_priv(dev); + struct phy_device *phydev = priv->phydev; + + priv->rx_pause = !!pause->rx_pause; + priv->tx_pause = !!pause->tx_pause; + + /* update h/w settings, if link is up */ + if (phydev && phydev->link) + gfar_configure_pause(priv, !!phydev->duplex); + + return 0; +} + int gfar_set_features(struct net_device *dev, netdev_features_t features) { struct gfar_private *priv = netdev_priv(dev); @@ -1806,6 +1834,8 @@ const struct ethtool_ops gfar_ethtool_ops = { .set_coalesce = gfar_scoalesce, .get_ringparam = gfar_gringparam, .set_ringparam = gfar_sringparam, + .get_pauseparam = gfar_gpauseparam, + .set_pauseparam = gfar_spauseparam, .get_strings = gfar_gstrings, .get_sset_count = gfar_sset_count, .get_ethtool_stats = gfar_fill_stats,
Allow Rx/Tx pause frame configuration via ethtool. The gfar devices feature link autonegotioation by default. The device is being configured with the new pause frame parameters if the link is up, depending on link duplex (no pause frames for half-duplex links), or during link autoneg (see adjust_link()). Signed-off-by: Claudiu Manoil <claudiu.manoil@freescale.com> --- drivers/net/ethernet/freescale/gianfar.h | 1 + drivers/net/ethernet/freescale/gianfar_ethtool.c | 30 ++++++++++++++++++++++++ 2 files changed, 31 insertions(+)