Re: [RESEND PATCH net-next] amd-xgbe: Configure and retrieve 'tx-usecs' for Tx coalescing
From: Badole, Vishal
Date: Fri Jul 25 2025 - 04:59:50 EST
On 7/21/2025 3:05 AM, Vadim Fedorenko wrote:
On 20.07.2025 19:28, Badole, Vishal wrote:
On 7/19/2025 8:46 PM, Vadim Fedorenko wrote:
On 19.07.2025 08:26, Vishal Badole wrote:
Ethtool has advanced with additional configurable options, but the
current driver does not support tx-usecs configuration.
Add support to configure and retrieve 'tx-usecs' using ethtool, which
specifies the wait time before servicing an interrupt for Tx
coalescing.
Signed-off-by: Vishal Badole <Vishal.Badole@xxxxxxx>
Acked-by: Shyam Sundar S K <Shyam-sundar.S-k@xxxxxxx>
---
drivers/net/ethernet/amd/xgbe/xgbe-ethtool.c | 19 +++++++++++++++++--
drivers/net/ethernet/amd/xgbe/xgbe.h | 1 +
2 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-ethtool.c b/drivers/
net/ ethernet/amd/xgbe/xgbe-ethtool.c
index 12395428ffe1..362f8623433a 100644
--- a/drivers/net/ethernet/amd/xgbe/xgbe-ethtool.c
+++ b/drivers/net/ethernet/amd/xgbe/xgbe-ethtool.c
@@ -450,6 +450,7 @@ static int xgbe_get_coalesce(struct net_device
*netdev,
ec->rx_coalesce_usecs = pdata->rx_usecs;
ec->rx_max_coalesced_frames = pdata->rx_frames;
+ ec->tx_coalesce_usecs = pdata->tx_usecs;
ec->tx_max_coalesced_frames = pdata->tx_frames;
return 0;
@@ -463,7 +464,7 @@ static int xgbe_set_coalesce(struct net_device
*netdev,
struct xgbe_prv_data *pdata = netdev_priv(netdev);
struct xgbe_hw_if *hw_if = &pdata->hw_if;
unsigned int rx_frames, rx_riwt, rx_usecs;
- unsigned int tx_frames;
+ unsigned int tx_frames, tx_usecs;
rx_riwt = hw_if->usec_to_riwt(pdata, ec->rx_coalesce_usecs);
rx_usecs = ec->rx_coalesce_usecs;
@@ -485,9 +486,22 @@ static int xgbe_set_coalesce(struct net_device
*netdev,
return -EINVAL;
}
+ tx_usecs = ec->tx_coalesce_usecs;
tx_frames = ec->tx_max_coalesced_frames;
+ /* Check if both tx_usecs and tx_frames are set to 0
simultaneously */
+ if (!tx_usecs && !tx_frames) {
+ netdev_err(netdev,
+ "tx_usecs and tx_frames must not be 0 together\n");
+ return -EINVAL;
+ }
+
/* Check the bounds of values for Tx */
+ if (tx_usecs > XGMAC_MAX_COAL_TX_TICK) {
+ netdev_err(netdev, "tx-usecs is limited to %d usec\n",
+ XGMAC_MAX_COAL_TX_TICK);
+ return -EINVAL;
+ }
if (tx_frames > pdata->tx_desc_count) {
netdev_err(netdev, "tx-frames is limited to %d frames\n",
pdata->tx_desc_count);
@@ -499,6 +513,7 @@ static int xgbe_set_coalesce(struct net_device
*netdev,
pdata->rx_frames = rx_frames;
hw_if->config_rx_coalesce(pdata);
+ pdata->tx_usecs = tx_usecs;
pdata->tx_frames = tx_frames;
hw_if->config_tx_coalesce(pdata);
I'm not quite sure, but it looks like it never works.
config_tx_coalesce()
callback equals to xgbe_config_tx_coalesce() which is implemented as:
static int xgbe_config_tx_coalesce(struct xgbe_prv_data *pdata)
{
return 0;
}
How is it expected to change anything from HW side?
The code analysis reveals that pdata, a pointer to xgbe_prv_data, is
obtained via netdev_priv(netdev). The tx_usecs member of the
xgbe_prv_data structure is then updated with the user-specified value
through this pdata pointer. This updated tx_usecs value propagates
throughout the codebase wherever TX coalescing functionality is
referenced.
We have validated this behavior through log analysis and transmission
timestamps, confirming the parameter updates are taking effect.
Since this is a legacy driver implementation where
xgbe_config_tx_coalesce() currently lacks actual hardware
configuration logic for TX coalescing parameters, we plan to modernize
the xgbe driver and eliminate redundant code segments in future releases.
Effectively, when the user asks for the coalescing configuration, the
driver reports values which are not really HW-configured values. At the
same time
driver reports correct configuration even though the configuration is not
actually supported by the driver and it doesn't configure HW. This
sounds odd.
Why didn't you start with the actual implementation instead of doing this
useless copying of values?
Since the XGMAC controller does not provide hardware-level register
support for tx_usecs-based TX interrupt coalescing, the driver employs
an advanced timer-driven software implementation to achieve interrupt
batching and improve performance. The tx_usecs parameter is accessible
through the pdata structure pointer, allowing dynamic updates that
automatically influence the TX coalescing timer mechanism across the
driver implementation.
@@ -830,7 +845,7 @@ static int xgbe_set_channels(struct net_device
*netdev,
}
static const struct ethtool_ops xgbe_ethtool_ops = {
- .supported_coalesce_params = ETHTOOL_COALESCE_RX_USECS |
+ .supported_coalesce_params = ETHTOOL_COALESCE_USECS |
ETHTOOL_COALESCE_MAX_FRAMES,
.get_drvinfo = xgbe_get_drvinfo,
.get_msglevel = xgbe_get_msglevel,
diff --git a/drivers/net/ethernet/amd/xgbe/xgbe.h b/drivers/net/
ethernet/ amd/xgbe/xgbe.h
index 42fa4f84ff01..e330ae9ea685 100755
--- a/drivers/net/ethernet/amd/xgbe/xgbe.h
+++ b/drivers/net/ethernet/amd/xgbe/xgbe.h
@@ -272,6 +272,7 @@
/* Default coalescing parameters */
#define XGMAC_INIT_DMA_TX_USECS 1000
#define XGMAC_INIT_DMA_TX_FRAMES 25
+#define XGMAC_MAX_COAL_TX_TICK 100000
#define XGMAC_MAX_DMA_RIWT 0xff
#define XGMAC_INIT_DMA_RX_USECS 30