| Message ID | 20240513-gemini-ethernet-fix-tso-v3-5-b442540cc140@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ethernet: cortina: TSO and pause param | expand |
On Mon, May 13, 2024 at 03:38:52PM +0200, Linus Walleij wrote: > The Cortina Gemini ethernet can very well set up TX or RX > pausing, so add this functionality to the driver in a > .set_pauseparam() callback. > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > drivers/net/ethernet/cortina/gemini.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c > index 85a9777083ba..4ae25a064407 100644 > --- a/drivers/net/ethernet/cortina/gemini.c > +++ b/drivers/net/ethernet/cortina/gemini.c > @@ -2146,6 +2146,20 @@ static void gmac_get_pauseparam(struct net_device *netdev, > pparam->autoneg = true; > } > > +static int gmac_set_pauseparam(struct net_device *netdev, > + struct ethtool_pauseparam *pparam) > +{ > + struct phy_device *phydev = netdev->phydev; > + > + if (!pparam->autoneg) > + return -EOPNOTSUPP; > + > + gmac_set_flow_control(netdev, pparam->tx_pause, pparam->rx_pause); It is not obvious to my why you need this call here? Andrew
On Mon, May 13, 2024 at 7:22 PM Andrew Lunn <andrew@lunn.ch> wrote: > On Mon, May 13, 2024 at 03:38:52PM +0200, Linus Walleij wrote: > > The Cortina Gemini ethernet can very well set up TX or RX > > pausing, so add this functionality to the driver in a > > .set_pauseparam() callback. > > > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > > --- > > drivers/net/ethernet/cortina/gemini.c | 15 +++++++++++++++ > > 1 file changed, 15 insertions(+) > > > > diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c > > index 85a9777083ba..4ae25a064407 100644 > > --- a/drivers/net/ethernet/cortina/gemini.c > > +++ b/drivers/net/ethernet/cortina/gemini.c > > @@ -2146,6 +2146,20 @@ static void gmac_get_pauseparam(struct net_device *netdev, > > pparam->autoneg = true; > > } > > > > +static int gmac_set_pauseparam(struct net_device *netdev, > > + struct ethtool_pauseparam *pparam) > > +{ > > + struct phy_device *phydev = netdev->phydev; > > + > > + if (!pparam->autoneg) > > + return -EOPNOTSUPP; > > + > > + gmac_set_flow_control(netdev, pparam->tx_pause, pparam->rx_pause); > > It is not obvious to my why you need this call here? I don't know if I don't understand the flow of code here... The Datasheet says that these registers shall be programmed to match what is set up in the PHY. Yours, Linus Walleij
On Tue, May 14, 2024 at 10:55:18AM +0200, Linus Walleij wrote: > On Mon, May 13, 2024 at 7:22 PM Andrew Lunn <andrew@lunn.ch> wrote: > > On Mon, May 13, 2024 at 03:38:52PM +0200, Linus Walleij wrote: > > > The Cortina Gemini ethernet can very well set up TX or RX > > > pausing, so add this functionality to the driver in a > > > .set_pauseparam() callback. > > > > > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > > > --- > > > drivers/net/ethernet/cortina/gemini.c | 15 +++++++++++++++ > > > 1 file changed, 15 insertions(+) > > > > > > diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c > > > index 85a9777083ba..4ae25a064407 100644 > > > --- a/drivers/net/ethernet/cortina/gemini.c > > > +++ b/drivers/net/ethernet/cortina/gemini.c > > > @@ -2146,6 +2146,20 @@ static void gmac_get_pauseparam(struct net_device *netdev, > > > pparam->autoneg = true; > > > } > > > > > > +static int gmac_set_pauseparam(struct net_device *netdev, > > > + struct ethtool_pauseparam *pparam) > > > +{ > > > + struct phy_device *phydev = netdev->phydev; > > > + > > > + if (!pparam->autoneg) > > > + return -EOPNOTSUPP; > > > + > > > + gmac_set_flow_control(netdev, pparam->tx_pause, pparam->rx_pause); > > > > It is not obvious to my why you need this call here? > > I don't know if I don't understand the flow of code here... > > The Datasheet says that these registers shall be programmed to > match what is set up in the PHY. I expect the registers should match what the PHY has negotiated, not what it is advertising. So i would expect to see gmac_set_flow_control() only in the adjust link callback once negotiation has completed. Andrew
On Tue, May 14, 2024 at 4:32 PM Andrew Lunn <andrew@lunn.ch> wrote: > On Tue, May 14, 2024 at 10:55:18AM +0200, Linus Walleij wrote: > > I don't know if I don't understand the flow of code here... > > > > The Datasheet says that these registers shall be programmed to > > match what is set up in the PHY. > > I expect the registers should match what the PHY has negotiated, not > what it is advertising. So i would expect to see > gmac_set_flow_control() only in the adjust link callback once > negotiation has completed. I get it, you are right of course. I'll send a new iteration dropping this setting after net-next opens again in two weeks. (also split off these three patches separately.) Yours, Linus Walleij
diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c index 85a9777083ba..4ae25a064407 100644 --- a/drivers/net/ethernet/cortina/gemini.c +++ b/drivers/net/ethernet/cortina/gemini.c @@ -2146,6 +2146,20 @@ static void gmac_get_pauseparam(struct net_device *netdev, pparam->autoneg = true; } +static int gmac_set_pauseparam(struct net_device *netdev, + struct ethtool_pauseparam *pparam) +{ + struct phy_device *phydev = netdev->phydev; + + if (!pparam->autoneg) + return -EOPNOTSUPP; + + gmac_set_flow_control(netdev, pparam->tx_pause, pparam->rx_pause); + phy_set_asym_pause(phydev, pparam->rx_pause, pparam->tx_pause); + + return 0; +} + static void gmac_get_ringparam(struct net_device *netdev, struct ethtool_ringparam *rp, struct kernel_ethtool_ringparam *kernel_rp, @@ -2266,6 +2280,7 @@ static const struct ethtool_ops gmac_351x_ethtool_ops = { .set_link_ksettings = gmac_set_ksettings, .nway_reset = gmac_nway_reset, .get_pauseparam = gmac_get_pauseparam, + .set_pauseparam = gmac_set_pauseparam, .get_ringparam = gmac_get_ringparam, .set_ringparam = gmac_set_ringparam, .get_coalesce = gmac_get_coalesce,
The Cortina Gemini ethernet can very well set up TX or RX pausing, so add this functionality to the driver in a .set_pauseparam() callback. Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- drivers/net/ethernet/cortina/gemini.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)