[PATCH net-next v10] net: ti: icssg-prueth: add TAPRIO offload support

[Vladimir Oltean]

It has been a long time since the last posting, everything has been
swapped out from my memory. Sorry if some comments are repeated.

On Fri, May 02, 2025 at 04:12:35PM +0530, MD Danish Anwar wrote:
> From: Roger Quadros <rogerq at ti.com>
> 
> The Time-Aware Shaper (TAS) is a key feature of the Enhanced Scheduled
> Traffic (EST) mechanism defined in IEEE 802.1Q-2018. This patch adds TAS
> support for the ICSSG driver by interacting with the ICSSG firmware to
> manage gate control lists, cycle times, and other TAS parameters.
> 
> The firmware maintains active and shadow lists. The driver updates the
> operating list using API `tas_update_oper_list()` which,
> - Updates firmware list pointers via `tas_update_fw_list_pointers`.
> - Writes gate masks, window end times, and clears unused entries in the
>   shadow list.
> - Updates gate close times and Max SDU values for each queue.
> - Triggers list changes using `tas_set_trigger_list_change`, which
>   - Computes cycle count (base-time % cycle-time) and extend (base-time %
>     cycle-time)

Please define the "cycle count" concept (local invention, not IEEE
standard). Also, cross-checking with the code, base-time % cycle-time is
incorrect here, that's not how you calculate it.

I'm afraid you also need to define the "extend" concept. It is not at
all clear what it does and how it does it. Does it have any relationship
with the CycleTimeExtension variables as documented by IEEE 802.1Q annex
Q.5 definitions?

A very compressed summary of the standard variable is this:
the CycleTimeExtension applies when:
- an Open schedule exists
- an Admin schedule is pending
- the AdminBaseTime is not an integer multiple of OperBaseTime + (N *
  OperCycleTime) - i.o.w. the admin schedule does not "line up" with the
  end of the oper schedule

The misalignment of the oper vs admin schedules might cause the very
last oper cycle to be truncated to an undesirably short value. The
OperCycleTimeExtension variable exists to prevent this, as such:

- If the length of the last oper cycle is < OperCycleTimeExtension,
  then this cycle does not execute at all. The gate states from the end
  of the next-to-last oper cycle remain in place (that cycle is extended)
  until the activation of the admin schedule at AdminBaseTime.

- If the length of the last oper cycle is >= OperCycleTimeExtension,
  this last cycle is left to execute until AdminBaseTime, and is
  potentially truncated during the switchover event (unless it perfectly
  lines up). Extension of the next-to-last oper cycle does not take
  place.

Is this the same functionality as the "extend" feature of the PRU
firmware - should I be reading the code and the commit message in this
key, in order to understand what it achieves?

>   - Writes cycle time, cycle count, and extend values to firmware memory.
>   - base-time being in past or base-time not being a multiple of
>     cycle-time is taken care by the firmware. Driver just writes these
>     variable for firmware and firmware takes care of the scheduling.

"base-time not being a multiple of cycle-time is taken care by the firmware":
To what extent is this true? You don't actually pass the base-time to
the firmware, so how would it know that it's not a multiple of cycle-time?

>   - If base-time is not a multiple of cycle-time, the value of extend
>     (base-time % cycle-time) is used by the firmware to extend the last
>     cycle.

I'm surprised to read this. Why does the firmware expect the base time
to be a multiple of the cycle time?

Also, I don't understand what the workaround achieves. If the "extend"
feature is similar to CycleTimeExtension, then it applies at the _end_
of the cycle. I.o.w. if you never change the cycle, it never applies.
How does that help address a problem which exists since the very first
cycle of the schedule (that it may be shifted relative to integer
multiples of the cycle time)?

And even assuming that a schedule change will take place - what's the
math that would suggest the "extend" feature does anything at all to
address the request to apply a phase-shifted schedule? The last cycle of
the oper schedule passes, the admin schedule becomes the new oper, and
then what? It still runs phase-aligned with its own cycle-time, but
misaligned with the user-provided base time, no?

The expectation is for all cycles to be shifted relative to N *
base-time, not just the first or last one. It doesn't "sound" like you
can achieve that using CycleTimeExtension (assuming that's what this
is), so better refuse those schedules which don't have the base-time you
need.

>   - Sets `config_change` and `config_pending` flags to notify firmware of
>     the new shadow list and its readiness for activation.
>   - Sends the `ICSSG_EMAC_PORT_TAS_TRIGGER` r30 command to ask firmware to
>     swap active and shadow lists.
> - Waits for the firmware to clear the `config_change` flag before
>   completing the update and returning successfully.
> 
> This implementation ensures seamless TAS functionality by offloading
> scheduling complexities to the firmware.
> 
> Signed-off-by: Roger Quadros <rogerq at ti.com>
> Signed-off-by: Vignesh Raghavendra <vigneshr at ti.com>
> Reviewed-by: Simon Horman <horms at kernel.org>
> Signed-off-by: MD Danish Anwar <danishanwar at ti.com>
> ---
> Cc: Vladimir Oltean <vladimir.oltean at nxp.com>
> v9 - v10:
> There has been significant changes since v9. I have tried to address all
> the comments given by Vladimir Oltean <vladimir.oltean at nxp.com> on v9
> *) Made the driver depend on NET_SCH_TAPRIO || NET_SCH_TAPRIO=n for TAS
> *) Used MACRO for max sdu size instead of magic number
> *) Kept `tas->state = state` outside of the switch case in `tas_set_state`
> *) Implemented TC_QUERY_CAPS case in `icssg_qos_ndo_setup_tc`
> *) Calling `tas_update_fw_list_pointers` only once in
>    `tas_update_oper_list` as the second call as unnecessary.
> *) Moved the check for TAS_MAX_CYCLE_TIME to beginning of
>    `emac_taprio_replace`
> *) Added `__packed` to structures in `icssg_qos.h`
> *) Modified implementation of `tas_set_trigger_list_change` to handle
>    cases where base-time isn't a multiple of cycle-time. For this a new
>    variable extend has to be calculated as base-time % cycle-time. This
>    variable is used by firmware to extend the last cycle.
> *) The API prueth_iep_gettime() and prueth_iep_settime() also needs to be
>    adjusted according to the cycle time extension. These changes are also
>    taken care in this patch.

Why? Given the explanation of CycleTimeExtension above, it makes no
sense to me why you would alter the gettime() and settime() values.

> 
> v9 https://lore.kernel.org/all/20240531044512.981587-3-danishanwar@ti.com/
> 
>  drivers/net/ethernet/ti/Kconfig               |   1 +
>  drivers/net/ethernet/ti/Makefile              |   2 +-
>  drivers/net/ethernet/ti/icssg/icssg_prueth.c  |   7 +
>  drivers/net/ethernet/ti/icssg/icssg_prueth.h  |   2 +
>  drivers/net/ethernet/ti/icssg/icssg_qos.c     | 310 ++++++++++++++++++
>  drivers/net/ethernet/ti/icssg/icssg_qos.h     | 112 +++++++
>  .../net/ethernet/ti/icssg/icssg_switch_map.h  |   6 +
>  7 files changed, 439 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/net/ethernet/ti/icssg/icssg_qos.c
>  create mode 100644 drivers/net/ethernet/ti/icssg/icssg_qos.h
> 
> +static int emac_taprio_replace(struct net_device *ndev,
> +			       struct tc_taprio_qopt_offload *taprio)
> +{
> +	struct prueth_emac *emac = netdev_priv(ndev);
> +	int ret;
> +
> +	if (taprio->cycle_time_extension) {
> +		NL_SET_ERR_MSG_MOD(taprio->extack, "Cycle time extension not supported");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (taprio->cycle_time > TAS_MAX_CYCLE_TIME) {
> +		NL_SET_ERR_MSG_FMT_MOD(taprio->extack, "cycle_time %llu is more than max supported cycle_time",
> +				       taprio->cycle_time);

It would be better to also print here TAS_MAX_CYCLE_TIME, like TAS_MIN_CYCLE_TIME below.
Also, looping back a user-supplied value (taprio->cycle_time) is IMO not needed.

> +		return -EINVAL;
> +	}
> +
> +	if (taprio->cycle_time < TAS_MIN_CYCLE_TIME) {
> +		NL_SET_ERR_MSG_FMT_MOD(taprio->extack, "cycle_time %llu is less than min supported cycle_time %d",
> +				       taprio->cycle_time, TAS_MIN_CYCLE_TIME);
> +		return -EINVAL;
> +	}
> +
> +	if (taprio->num_entries > TAS_MAX_CMD_LISTS) {
> +		NL_SET_ERR_MSG_FMT_MOD(taprio->extack, "num_entries %lu is more than max supported entries %d",
> +				       taprio->num_entries, TAS_MAX_CMD_LISTS);
> +		return -EINVAL;
> +	}
> +
> +	if (emac->qos.tas.taprio_admin)
> +		taprio_offload_free(emac->qos.tas.taprio_admin);
> +
> +	emac->qos.tas.taprio_admin = taprio_offload_get(taprio);
> +	ret = tas_update_oper_list(emac);
> +	if (ret)
> +		goto clear_taprio;
> +
> +	ret = tas_set_state(emac, TAS_STATE_ENABLE);
> +	if (ret)
> +		goto clear_taprio;
> +
> +	return 0;
> +
> +clear_taprio:
> +	emac->qos.tas.taprio_admin = NULL;
> +	taprio_offload_free(taprio);
> +
> +	return ret;
> +}