Message-ID: <20230729000127.fqwwp2j6rzyw6xth@skbuf>
Date:   Sat, 29 Jul 2023 03:01:27 +0300
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Roger Quadros <rogerq@...nel.org>
Cc:     davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
        pabeni@...hat.com, s-vadapalli@...com, srk@...com,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH] net: ethernet: ti: am65-cpsw-qos: Add Frame
 Preemption MAC Merge support

Hi Roger,

On Tue, Jul 25, 2023 at 10:23:38AM +0300, Roger Quadros wrote:
> Add driver support for viewing / changing the MAC Merge sublayer
> parameters and seeing the verification state machine's current state
> via ethtool.
> 
> As hardware does not support interrupt notification for verification
> events we resort to polling on link up. On link up we try a couple of
> times for verification success and if unsuccessful then give up.
> 
> The Frame Preemption feature is described in the Technical Reference
> Manual [1] in section:
> 	12.3.1.4.6.7 Intersperced Express Traffic (IET – P802.3br/D2.0)
> 
> Due to Silicon Errata i2208 [2] we set limit min IET fragment size to 124.
> 
> [1] AM62x TRM - https://www.ti.com/lit/ug/spruiv7a/spruiv7a.pdf
> [2] AM62x Silicon Errata - https://www.ti.com/lit/er/sprz487c/sprz487c.pdf

Thanks for the documentation.

> 
> Signed-off-by: Roger Quadros <rogerq@...nel.org>
> ---
> 
> Hi,
> 
> This is RFC because I've still not got Verification to work

What are you testing with? There's a selftest at tools/testing/selftests/net/forwarding/ethtool_mm.sh.

> and I'm still clueless on how to set the preemptible mask to set the
> preemtible queues. The driver doesn't yet support MQPRIO offloading.

I'm not sure what it is that you say you're clueless about. The user
space tooling for setting the preemptible traffic classes? You can take
the "fp" line of arguments from the mqprio command in the selftest
above, and transplant it to a taprio command you already have.

To avoid any confusion, please make sure that your iproute2 version has
commit e848ef0ad5d0 ("tc/taprio: fix parsing of "fp" option when it
doesn't appear last") applied.

> 
> Please let me know if overall approach is OK. Thanks.
> 
> cheers,
> -roger
> 
>  drivers/net/ethernet/ti/am65-cpsw-ethtool.c | 168 ++++++++++++++++
>  drivers/net/ethernet/ti/am65-cpsw-nuss.c    |   2 +
>  drivers/net/ethernet/ti/am65-cpsw-nuss.h    |   5 +
>  drivers/net/ethernet/ti/am65-cpsw-qos.c     | 212 ++++++++++++++++----
>  drivers/net/ethernet/ti/am65-cpsw-qos.h     |  90 +++++++++
>  5 files changed, 440 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ti/am65-cpsw-ethtool.c b/drivers/net/ethernet/ti/am65-cpsw-ethtool.c
> index c51e2af91f69..fd2ba29ebc0b 100644
> --- a/drivers/net/ethernet/ti/am65-cpsw-ethtool.c
> +++ b/drivers/net/ethernet/ti/am65-cpsw-ethtool.c
> @@ -11,6 +11,7 @@
>  #include <linux/pm_runtime.h>
>  
>  #include "am65-cpsw-nuss.h"
> +#include "am65-cpsw-qos.h"
>  #include "cpsw_ale.h"
>  #include "am65-cpts.h"
>  
> @@ -715,6 +716,171 @@ static int am65_cpsw_set_ethtool_priv_flags(struct net_device *ndev, u32 flags)
>  	return 0;
>  }
>  
> +/* enable common IET only if at least 1 port has pre-emptible queues. disable otherwise */
> +static void am65_cpsw_iet_enable(struct am65_cpsw_common *common)
> +{
> +	u32 common_enable = 0;
> +	u32 val;
> +	int i;
> +
> +	for (i = 0; i < common->port_num; i++)
> +		common_enable |= common->ports[i].qos.iet.preemptible_tcs;
> +
> +	val = readl(common->cpsw_base + AM65_CPSW_REG_CTL);
> +
> +	if (common_enable)
> +		val |= AM65_CPSW_CTL_IET_EN;
> +	else
> +		val &= ~AM65_CPSW_CTL_IET_EN;
> +
> +	writel(val, common->cpsw_base + AM65_CPSW_REG_CTL);
> +	common->iet_enabled = common_enable;
> +}

I see that IET_ENABLE is global to the switch. I wonder what it controls
exactly, since there's also a port-level IET_PORT_EN below.

Our interpretations (likely speculative both) seem to differ on how this
should be set. Let me just say a few words about the UAPI
(Documentation/networking/ethtool-netlink.rst and "man ethtool" would
hold the info).

Verification should succeed regardless of whether there is any preemptible
TC configured. The pmac-enabled knob (ETHTOOL_A_MM_PMAC_ENABLED) is
intended to control, among other things, whether the port responds to
verification frames from the link partner. IOW need pmac-enabled=true on
a port, before verify-enabled=true on the link partner can work. Also,
in the UAPI, tx-enabled=true requires pmac-enabled=true within the same
station.

The semantically closest hardware bit for pmac-enabled seems to be
IET_PORT_EN, and tx-enabled seems to correspond to MAC_PENABLE.

Whereas IET_ENABLE seems to be a global override for all, both RX and TX.

If I'm correct, you could implicitly set/unset IET_ENABLE depending on
whether IET_PORT_EN is set on any port, and in turn, set IET_PORT_EN
based on pmac-enabled.

> +
> +static void am65_cpsw_iet_port_enable(struct am65_cpsw_port *port, bool enable)
> +{
> +	u32 val;
> +
> +	val = readl(port->port_base + AM65_CPSW_PN_REG_CTL);
> +	if (enable)
> +		val |= AM65_CPSW_PN_CTL_IET_PORT_EN;
> +	else
> +		val &= ~AM65_CPSW_PN_CTL_IET_PORT_EN;
> +
> +	writel(val, port->port_base + AM65_CPSW_PN_REG_CTL);
> +}

Not too much documentation on what this does. If it's about being able
to process preemptible traffic on this port in general, then you are not
correct in hooking it to tx_enabled.

> +
> +static void am65_cpsw_iet_mac_penable(struct am65_cpsw_port *port, bool enable)
> +{
> +	u32 val;
> +
> +	val = readl(port->port_base + AM65_CPSW_PN_REG_IET_CTRL);
> +	if (enable)
> +		val |= AM65_CPSW_PN_IET_MAC_PENABLE;
> +	else
> +		val &= ~AM65_CPSW_PN_IET_MAC_PENABLE;
> +
> +	writel(val, port->port_base + AM65_CPSW_PN_REG_IET_CTRL);
> +}

At least the documentation seems to be clear here that hooking this to
tx_enabled seems to be the right thing.

> +
> +static int am65_cpsw_get_mm(struct net_device *ndev, struct ethtool_mm_state *state)
> +{
> +	struct am65_cpsw_port *port = am65_ndev_to_port(ndev);
> +	struct am65_cpsw_common *common = am65_ndev_to_common(ndev);
> +	struct am65_cpsw_ndev_priv *priv = netdev_priv(ndev);
> +	u32 port_ctrl, cmn_ctrl, iet_ctrl, iet_status, verify_cnt;
> +	u32 add_frag_size;
> +
> +	mutex_lock(&priv->mm_lock);
> +
> +	iet_ctrl = readl(port->port_base + AM65_CPSW_PN_REG_IET_CTRL);
> +	cmn_ctrl = readl(common->cpsw_base + AM65_CPSW_REG_CTL);
> +	port_ctrl = readl(port->port_base + AM65_CPSW_PN_REG_CTL);
> +
> +	state->pmac_enabled = !!(iet_ctrl & AM65_CPSW_PN_IET_MAC_PENABLE);
> +
> +	iet_status = readl(port->port_base + AM65_CPSW_PN_REG_IET_STATUS);
> +
> +	if (iet_ctrl & AM65_CPSW_PN_IET_MAC_DISABLEVERIFY)
> +		state->verify_status = ETHTOOL_MM_VERIFY_STATUS_DISABLED;
> +	else if (iet_status & AM65_CPSW_PN_MAC_VERIFIED)
> +		state->verify_status = ETHTOOL_MM_VERIFY_STATUS_SUCCEEDED;
> +	else if (iet_status & AM65_CPSW_PN_MAC_VERIFY_FAIL)
> +		state->verify_status = ETHTOOL_MM_VERIFY_STATUS_FAILED;
> +	else
> +		state->verify_status = ETHTOOL_MM_VERIFY_STATUS_UNKNOWN;
> +
> +	add_frag_size = AM65_CPSW_PN_IET_MAC_GET_ADDFRAGSIZE(iet_ctrl);
> +	state->tx_min_frag_size = ethtool_mm_frag_size_add_to_min(add_frag_size);
> +	state->rx_min_frag_size = state->tx_min_frag_size;

This is most likely wrong. You don't want rx_min_frag_size to follow tx_min_frag_size,
because tx_min_frag_size is settable (by ethtool, openlldp etc) and rx_min_frag_size
is not (it's an indication of what is the minimum fragment size that the port can receive).

In fact, the erratum i2208 you've linked to seems to be an RX issue, not
a TX one. So the correct workaround seems to be to set rx_min_frag_size
to 124, not to limit tx_min_frag_size as you've done?

> +
> +	state->tx_enabled = !!(port_ctrl & AM65_CPSW_PN_CTL_IET_PORT_EN);
> +
> +	/* FPE active if common IET enabled and verification success or disabled (forced) */
> +	state->tx_active = state->tx_enabled && !!(cmn_ctrl & AM65_CPSW_CTL_IET_EN) &&
> +			   (state->verify_status == ETHTOOL_MM_VERIFY_STATUS_SUCCEEDED ||
> +			    state->verify_status == ETHTOOL_MM_VERIFY_STATUS_DISABLED);
> +	state->verify_enabled = !(iet_ctrl & AM65_CPSW_PN_IET_MAC_DISABLEVERIFY);
> +
> +	verify_cnt = AM65_CPSW_PN_MAC_GET_VERIFY_CNT(readl(port->port_base +
> +							   AM65_CPSW_PN_REG_IET_VERIFY));
> +	state->verify_time = port->qos.iet.verify_time_ms;
> +	state->max_verify_time = am65_cpsw_iet_get_verify_timeout_ms(AM65_CPSW_PN_MAC_VERIFY_CNT_MASK,
> +								     SPEED_1000);

I guess this should return the verify_timeout_ms for the current speed,
with a fallback for SPEED_1000 if there's no link. Otherwise it may be
wrong for other speeds and openlldp may fail trying to set it to the max.

> +
> +	mutex_unlock(&priv->mm_lock);
> +
> +	return 0;
> +}
> +
> +static int am65_cpsw_set_mm(struct net_device *ndev, struct ethtool_mm_cfg *cfg,
> +			    struct netlink_ext_ack *extack)
> +{
> +	struct am65_cpsw_port *port = am65_ndev_to_port(ndev);
> +	struct am65_cpsw_ndev_priv *priv = netdev_priv(ndev);
> +	struct am65_cpsw_iet *iet = &port->qos.iet;
> +	u32 val, add_frag_size;
> +	int err;
> +
> +	/* Errata i2208: min fragment size cannot be less than 124 */
> +	if (cfg->tx_min_frag_size < 124) {
> +		netdev_err(ndev, "tx_min_fragment_size cannot be less than 124\n");

nitpick: use the extack (if the workaround is correct at all, see above)

> +		return -EINVAL;
> +	}
> +
> +	err = ethtool_mm_frag_size_min_to_add(cfg->tx_min_frag_size, &add_frag_size, extack);
> +	if (err)
> +		return err;
> +
> +	mutex_lock(&priv->mm_lock);
> +
> +	if (cfg->tx_enabled) {
> +		/* For IET, Change MAX_BLKS */
> +		if (!iet->original_max_blks)
> +			iet->original_max_blks = readl(port->port_base + AM65_CPSW_PN_REG_MAX_BLKS);
> +
> +		writel(AM65_CPSW_PN_TX_RX_MAX_BLKS_IET,
> +		       port->port_base + AM65_CPSW_PN_REG_MAX_BLKS);
> +	} else {
> +		/* restore MAX_BLKS to default */
> +		if (iet->original_max_blks) {
> +			writel(iet->original_max_blks,
> +			       port->port_base + AM65_CPSW_PN_REG_MAX_BLKS);
> +		}
> +	}

According to the documentation, it seems necessary to resize the RX FIFO
based on pmac_enabled, not tx_enabled.

> +
> +	am65_cpsw_iet_port_enable(port, cfg->tx_enabled);
> +	am65_cpsw_iet_mac_penable(port, cfg->tx_enabled);
> +
> +	val = readl(port->port_base + AM65_CPSW_PN_REG_IET_CTRL);
> +	if (cfg->verify_enabled) {
> +		val &= ~AM65_CPSW_PN_IET_MAC_DISABLEVERIFY;
> +		/* Reset Verify state machine. Verification won't start here.
> +		 * Verification will be done once link-up.
> +		 */
> +		val |= AM65_CPSW_PN_IET_MAC_LINKFAIL;
> +	} else {
> +		val |= AM65_CPSW_PN_IET_MAC_DISABLEVERIFY;
> +	}
> +
> +	val &= ~AM65_CPSW_PN_IET_MAC_MAC_ADDFRAGSIZE_MASK;
> +	val |= AM65_CPSW_PN_IET_MAC_SET_ADDFRAGSIZE(add_frag_size);
> +	writel(val, port->port_base + AM65_CPSW_PN_REG_IET_CTRL);
> +
> +	/* verify_timeout_count can only be set at valid link */
> +	port->qos.iet.verify_time_ms = cfg->verify_time;
> +
> +	/* iet common enable/disable */
> +	am65_cpsw_iet_enable(port->common);
> +
> +	/* enable/disable pre-emption based on link status */
> +	am65_cpsw_iet_commit_preemptible_tcs(port);
> +
> +	mutex_unlock(&priv->mm_lock);
> +
> +	return 0;
> +}
> +
>  const struct ethtool_ops am65_cpsw_ethtool_ops_slave = {
>  	.begin			= am65_cpsw_ethtool_op_begin,
>  	.complete		= am65_cpsw_ethtool_op_complete,
> @@ -743,4 +909,6 @@ const struct ethtool_ops am65_cpsw_ethtool_ops_slave = {
>  	.get_eee		= am65_cpsw_get_eee,
>  	.set_eee		= am65_cpsw_set_eee,
>  	.nway_reset		= am65_cpsw_nway_reset,
> +	.get_mm			= am65_cpsw_get_mm,
> +	.set_mm			= am65_cpsw_set_mm,
>  };
> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> index bebcfd5e6b57..b0e2d6773543 100644
> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> @@ -2160,6 +2160,8 @@ am65_cpsw_nuss_init_port_ndev(struct am65_cpsw_common *common, u32 port_idx)
>  	ndev_priv = netdev_priv(port->ndev);
>  	ndev_priv->port = port;
>  	ndev_priv->msg_enable = AM65_CPSW_DEBUG;
> +	mutex_init(&ndev_priv->mm_lock);
> +	port->qos.link_speed = SPEED_UNKNOWN;
>  	SET_NETDEV_DEV(port->ndev, dev);
>  
>  	eth_hw_addr_set(port->ndev, port->slave.mac_addr);
> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.h b/drivers/net/ethernet/ti/am65-cpsw-nuss.h
> index bf40c88fbd9b..ede3a7457e9c 100644
> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.h
> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.h
> @@ -145,6 +145,7 @@ struct am65_cpsw_common {
>  	bool			pf_p0_rx_ptype_rrobin;
>  	struct am65_cpts	*cpts;
>  	int			est_enabled;
> +	int			iet_enabled;
>  
>  	bool		is_emac_mode;
>  	u16			br_members;
> @@ -170,6 +171,10 @@ struct am65_cpsw_ndev_priv {
>  	struct am65_cpsw_port	*port;
>  	struct am65_cpsw_ndev_stats __percpu *stats;
>  	bool offload_fwd_mark;
> +	/* Serialize access to MAC Merge state between ethtool requests
> +	 * and link state updates
> +	 */
> +	struct mutex		mm_lock;
>  };
>  
>  #define am65_ndev_to_priv(ndev) \
> diff --git a/drivers/net/ethernet/ti/am65-cpsw-qos.c b/drivers/net/ethernet/ti/am65-cpsw-qos.c
> index 3a908db6e5b2..ae30d5c79be8 100644
> --- a/drivers/net/ethernet/ti/am65-cpsw-qos.c
> +++ b/drivers/net/ethernet/ti/am65-cpsw-qos.c
> @@ -4,9 +4,11 @@
>   *
>   * quality of service module includes:
>   * Enhanced Scheduler Traffic (EST - P802.1Qbv/D2.2)
> + * Interspersed Express Traffic (IET - P802.3br/D2.0)
>   */
>  
>  #include <linux/pm_runtime.h>
> +#include <linux/units.h>
>  #include <linux/time.h>
>  #include <net/pkt_cls.h>
>  
> @@ -15,47 +17,176 @@
>  #include "am65-cpts.h"
>  #include "cpsw_ale.h"
>  
> -#define AM65_CPSW_REG_CTL			0x004
> -#define AM65_CPSW_PN_REG_CTL			0x004
> -#define AM65_CPSW_PN_REG_FIFO_STATUS		0x050
> -#define AM65_CPSW_PN_REG_EST_CTL		0x060
> -#define AM65_CPSW_PN_REG_PRI_CIR(pri)		(0x140 + 4 * (pri))
> -
> -/* AM65_CPSW_REG_CTL register fields */
> -#define AM65_CPSW_CTL_EST_EN			BIT(18)
> -
> -/* AM65_CPSW_PN_REG_CTL register fields */
> -#define AM65_CPSW_PN_CTL_EST_PORT_EN		BIT(17)
> -
> -/* AM65_CPSW_PN_REG_EST_CTL register fields */
> -#define AM65_CPSW_PN_EST_ONEBUF			BIT(0)
> -#define AM65_CPSW_PN_EST_BUFSEL			BIT(1)
> -#define AM65_CPSW_PN_EST_TS_EN			BIT(2)
> -#define AM65_CPSW_PN_EST_TS_FIRST		BIT(3)
> -#define AM65_CPSW_PN_EST_ONEPRI			BIT(4)
> -#define AM65_CPSW_PN_EST_TS_PRI_MSK		GENMASK(7, 5)
> -
> -/* AM65_CPSW_PN_REG_FIFO_STATUS register fields */
> -#define AM65_CPSW_PN_FST_TX_PRI_ACTIVE_MSK	GENMASK(7, 0)
> -#define AM65_CPSW_PN_FST_TX_E_MAC_ALLOW_MSK	GENMASK(15, 8)
> -#define AM65_CPSW_PN_FST_EST_CNT_ERR		BIT(16)
> -#define AM65_CPSW_PN_FST_EST_ADD_ERR		BIT(17)
> -#define AM65_CPSW_PN_FST_EST_BUFACT		BIT(18)
> -
> -/* EST FETCH COMMAND RAM */
> -#define AM65_CPSW_FETCH_RAM_CMD_NUM		0x80
> -#define AM65_CPSW_FETCH_CNT_MSK			GENMASK(21, 8)
> -#define AM65_CPSW_FETCH_CNT_MAX			(AM65_CPSW_FETCH_CNT_MSK >> 8)
> -#define AM65_CPSW_FETCH_CNT_OFFSET		8
> -#define AM65_CPSW_FETCH_ALLOW_MSK		GENMASK(7, 0)
> -#define AM65_CPSW_FETCH_ALLOW_MAX		AM65_CPSW_FETCH_ALLOW_MSK
> -
>  enum timer_act {
>  	TACT_PROG,		/* need program timer */
>  	TACT_NEED_STOP,		/* need stop first */
>  	TACT_SKIP_PROG,		/* just buffer can be updated */
>  };
>  
> +/* IET */
> +static int am65_cpsw_iet_set_verify_timeout_count(struct am65_cpsw_port *port)
> +{
> +	int link_speed = port->qos.link_speed;
> +	int verify_time_ms = port->qos.iet.verify_time_ms;

nitpick: gratuitous reverse xmas tree rule breakage

> +	u32 val;
> +
> +	if (link_speed == SPEED_UNKNOWN) {
> +		netdev_err(port->ndev, "%s called without active link\n", __func__);
> +		return -ENODEV;
> +	}

I guess you can use a simple WARN_ON() for code paths like this where it
is obvious that it can't trigger.

> +
> +	/* The number of wireside clocks contained in the verify
> +	 * timeout counter. The default is 0x1312d0
> +	 * (10ms at 125Mhz in 1G mode).
> +	 */
> +	val = 125 * HZ_PER_MHZ;	/* assuming 125MHz wireside clock */
> +
> +	val /= MILLIHZ_PER_HZ;		/* count per ms timeout */
> +	val *= verify_time_ms;		/* count for timeout ms */
> +	if (link_speed < SPEED_1000)
> +		val <<= 1;	/* FIXME: Is this correct? */

What indication do you have that you should do this shift?

> +
> +	if (val > AM65_CPSW_PN_MAC_VERIFY_CNT_MASK)
> +		return -EINVAL;
> +
> +	writel(val, port->port_base + AM65_CPSW_PN_REG_IET_VERIFY);
> +
> +	return 0;
> +}
> +
> +unsigned int am65_cpsw_iet_get_verify_timeout_ms(u32 count, int link_speed)
> +{
> +	unsigned int timeout_ms;
> +	u32 val = 125 * HZ_PER_MHZ;	/* assuming 125MHz wireside clock */
> +
> +	if (link_speed == SPEED_UNKNOWN)
> +		return -EINVAL;
> +
> +	val /= MILLIHZ_PER_HZ;		/* count per ms timeout */
> +
> +	timeout_ms = count / val;
> +
> +	if (link_speed < SPEED_1000)
> +		timeout_ms >>= 1;	/* FIXME: Is this correct? */
> +
> +	return timeout_ms;
> +}
> +
> +static int am65_cpsw_iet_verify_wait(struct am65_cpsw_port *port)
> +{
> +	u32 ctrl, status;
> +	int try;
> +
> +	ctrl = readl(port->port_base + AM65_CPSW_PN_REG_IET_CTRL);
> +	/* Clear MAC_PENABLE */
> +	ctrl &= ~AM65_CPSW_PN_IET_MAC_PENABLE;
> +	writel(ctrl, port->port_base + AM65_CPSW_PN_REG_IET_CTRL);
> +
> +	try = 20;
> +	do {
> +		/* Set MAC_PENABLE and Clear MAC_LINKFAIL bit to start Verify. */
> +		ctrl = readl(port->port_base + AM65_CPSW_PN_REG_IET_CTRL);
> +		ctrl &= ~AM65_CPSW_PN_IET_MAC_LINKFAIL;
> +		ctrl |= AM65_CPSW_PN_IET_MAC_PENABLE;
> +		writel(ctrl, port->port_base + AM65_CPSW_PN_REG_IET_CTRL);
> +
> +		msleep(port->qos.iet.verify_time_ms);
> +
> +		status = readl(port->port_base + AM65_CPSW_PN_REG_IET_STATUS);
> +		if (status & AM65_CPSW_PN_MAC_VERIFIED)
> +			return 0;
> +
> +		if (status & AM65_CPSW_PN_MAC_VERIFY_FAIL) {
> +			netdev_dbg(port->ndev,
> +				   "MM MAC verify failed, trying again");

nitpick: one of the Ms in MM stands for MAC already

> +			/* Reset the verify state machine by writing 1
> +			 * to LINKFAIL and 0 to MAC_PENABLE
> +			 */
> +			ctrl = readl(port->port_base + AM65_CPSW_PN_REG_IET_CTRL);
> +			ctrl &= ~AM65_CPSW_PN_IET_MAC_PENABLE;
> +			ctrl |= AM65_CPSW_PN_IET_MAC_LINKFAIL;
> +			writel(ctrl, port->port_base + AM65_CPSW_PN_REG_IET_CTRL);
> +			continue;
> +		}
> +
> +		if (status & AM65_CPSW_PN_MAC_RESPOND_ERR) {
> +			netdev_err(port->ndev, "MM MAC respond error");
> +			return -ENODEV;
> +		}
> +
> +		if (status & AM65_CPSW_PN_MAC_VERIFY_ERR) {
> +			netdev_err(port->ndev, "MM MAC verify error");
> +			return -ENODEV;
> +		}
> +	} while (try-- > 0);
> +
> +	netdev_info(port->ndev, "MM MAC verify timeout");
> +	return -ETIMEDOUT;

What error messages are you getting here? Timeout or an actual response
from hardware?

> +}
> +
> +static void am65_cpsw_iet_set_preempt_mask(struct am65_cpsw_port *port, u8 preemptible_tcs)
> +{
> +	u32 val;
> +
> +	val = readl(port->port_base + AM65_CPSW_PN_REG_IET_CTRL);
> +	val &= ~AM65_CPSW_PN_IET_MAC_PREMPT_MASK;
> +	val |= AM65_CPSW_PN_IET_MAC_SET_PREEMPT(preemptible_tcs);
> +	writel(val, port->port_base + AM65_CPSW_PN_REG_IET_CTRL);
> +}
> +
> +/* CPSW does not have an IRQ to notify changes to the MAC Merge TX status
> + * (active/inactive), but the preemptible traffic classes should only be
> + * committed to hardware once TX is active. Resort to polling.
> + */
> +void am65_cpsw_iet_commit_preemptible_tcs(struct am65_cpsw_port *port)
> +{
> +	u8 preemptible_tcs = 0;
> +	u32 val;
> +	int err;
> +
> +	if (port->qos.link_speed == SPEED_UNKNOWN)
> +		goto out;
> +
> +	val = readl(port->port_base + AM65_CPSW_PN_REG_CTL);
> +	if (!(val & AM65_CPSW_PN_CTL_IET_PORT_EN))
> +		goto out;
> +
> +	/* update verify count */
> +	err = am65_cpsw_iet_set_verify_timeout_count(port);
> +	if (err) {
> +		netdev_err(port->ndev, "couldn't set verify count: %d\n", err);
> +		return;
> +	}
> +
> +	val = readl(port->port_base + AM65_CPSW_PN_REG_IET_CTRL);
> +	if (!(val & AM65_CPSW_PN_IET_MAC_DISABLEVERIFY)) {
> +		err = am65_cpsw_iet_verify_wait(port);
> +		if (err)
> +			goto out;
> +	}
> +
> +	preemptible_tcs = port->qos.iet.preemptible_tcs;
> +out:
> +	am65_cpsw_iet_set_preempt_mask(port, preemptible_tcs);
> +}
> +
> +void am65_cpsw_iet_change_preemptible_tcs(struct am65_cpsw_port *port, u8 preemptible_tcs)
> +{
> +	port->qos.iet.preemptible_tcs = preemptible_tcs;
> +	am65_cpsw_iet_commit_preemptible_tcs(port);
> +}
> +
> +void am65_cpsw_iet_link_state_update(struct net_device *ndev)
> +{
> +	struct am65_cpsw_ndev_priv *priv = am65_ndev_to_priv(ndev);
> +	struct am65_cpsw_port *port = am65_ndev_to_port(ndev);
> +
> +	mutex_lock(&priv->mm_lock);
> +	am65_cpsw_iet_commit_preemptible_tcs(port);
> +	mutex_unlock(&priv->mm_lock);
> +}
> +
> +/* EST */
>  static int am65_cpsw_port_est_enabled(struct am65_cpsw_port *port)
>  {
>  	return port->qos.est_oper || port->qos.est_admin;
> @@ -541,7 +672,6 @@ static void am65_cpsw_est_link_up(struct net_device *ndev, int link_speed)
>  	ktime_t cur_time;
>  	s64 delta;
>  
> -	port->qos.link_speed = link_speed;
>  	if (!am65_cpsw_port_est_enabled(port))
>  		return;
>  
> @@ -565,10 +695,13 @@ static int am65_cpsw_setup_taprio(struct net_device *ndev, void *type_data)
>  {
>  	struct am65_cpsw_port *port = am65_ndev_to_port(ndev);
>  	struct am65_cpsw_common *common = port->common;
> +	struct tc_taprio_qopt_offload *taprio = type_data;
>  
>  	if (!IS_ENABLED(CONFIG_TI_AM65_CPSW_TAS))
>  		return -ENODEV;
>  
> +	am65_cpsw_iet_change_preemptible_tcs(port, taprio->mqprio.preemptible_tcs);
> +
>  	if (!netif_running(ndev)) {
>  		dev_err(&ndev->dev, "interface is down, link speed unknown\n");
>  		return -ENETDOWN;
> @@ -801,6 +934,9 @@ void am65_cpsw_qos_link_up(struct net_device *ndev, int link_speed)
>  {
>  	struct am65_cpsw_port *port = am65_ndev_to_port(ndev);
>  
> +	port->qos.link_speed = link_speed;
> +	am65_cpsw_iet_link_state_update(ndev);
> +
>  	if (!IS_ENABLED(CONFIG_TI_AM65_CPSW_TAS))
>  		return;
>  
> @@ -812,13 +948,15 @@ void am65_cpsw_qos_link_down(struct net_device *ndev)
>  {
>  	struct am65_cpsw_port *port = am65_ndev_to_port(ndev);
>  
> +	port->qos.link_speed = SPEED_UNKNOWN;
> +	am65_cpsw_iet_link_state_update(ndev);
> +
>  	if (!IS_ENABLED(CONFIG_TI_AM65_CPSW_TAS))
>  		return;
>  
>  	if (!port->qos.link_down_time)
>  		port->qos.link_down_time = ktime_get();
>  
> -	port->qos.link_speed = SPEED_UNKNOWN;
>  }
>  
>  static u32
> diff --git a/drivers/net/ethernet/ti/am65-cpsw-qos.h b/drivers/net/ethernet/ti/am65-cpsw-qos.h
> index 0cc2a3b3d7f9..2cfb18e6587d 100644
> --- a/drivers/net/ethernet/ti/am65-cpsw-qos.h
> +++ b/drivers/net/ethernet/ti/am65-cpsw-qos.h
> @@ -9,6 +9,7 @@
>  #include <net/pkt_sched.h>
>  
>  struct am65_cpsw_common;
> +struct am65_cpsw_port;
>  
>  struct am65_cpsw_est {
>  	int buf;
> @@ -16,6 +17,13 @@ struct am65_cpsw_est {
>  	struct tc_taprio_qopt_offload taprio;
>  };
>  
> +struct am65_cpsw_iet {
> +	struct ethtool_mm_cfg *cfg;

Leftover? This pointer is stale outside of am65_cpsw_set_mm().

> +	u8 preemptible_tcs;
> +	u32 original_max_blks;
> +	int verify_time_ms;
> +};

Powered by blists - more mailing lists