Message-ID: <20231005092943.q7no33k32thyo6y4@skbuf>
Date:   Thu, 5 Oct 2023 12:29:43 +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, horms@...nel.org, s-vadapalli@...com,
        srk@...com, vigneshr@...com, p-varis@...com,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 net-next 3/4] net: ethernet: ti: am65-cpsw-qos: Add
 Frame Preemption MAC Merge support

On Wed, Sep 27, 2023 at 10:27:40AM +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
> 
> Signed-off-by: Roger Quadros <rogerq@...nel.org>
> ---
>  drivers/net/ethernet/ti/am65-cpsw-ethtool.c | 150 ++++++++++++
>  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     | 240 ++++++++++++++++----
>  drivers/net/ethernet/ti/am65-cpsw-qos.h     | 104 +++++++++
>  5 files changed, 454 insertions(+), 47 deletions(-)
> 
> Changelog:
> v5:
> - No change
> 
> v4:
> - Rebase and include in the same series as mqprio support.
> 
> v3:
> - Rebase on top of v6.6-rc1 and mqprio support [1]
> - Support ethtool_ops :: get_mm_stats()
> - drop unused variables cmn_ctrl and verify_cnt
> - make am65_cpsw_iet_link_state_update() and
>   am65_cpsw_iet_change_preemptible_tcs() static
> 
> [1] https://lore.kernel.org/all/20230918075358.5878-1-rogerq@kernel.org/
> 
> v2:
> - Use proper control bits for PMAC enable (AM65_CPSW_PN_CTL_IET_PORT_EN)
>   and TX enable (AM65_CPSW_PN_IET_MAC_PENABLE)
> - Common IET Enable (AM65_CPSW_CTL_IET_EN) is set if any port has
>   AM65_CPSW_PN_CTL_IET_PORT_EN set.
> - Fix workaround for erratum i2208. i.e. Limit rx_min_frag_size to 124
> - Fix am65_cpsw_iet_get_verify_timeout_ms() to default to timeout for
>   1G link if link is inactive.
> - resize the RX FIFO based on pmac_enabled, not tx_enabled.
> 
> Test Procedure:
> 
> - 2 EVMs with AM65-CPSW network port connected to each other
> - Run iet-setup-mqprio.sh on both
> 
> #!/bin/sh
> #iet-setup-mqprio.sh
> 
> ifconfig eth0 down
> ifconfig eth1 down
> ethtool -L eth0 tx 4
> ethtool --set-mm eth0 pmac-enabled on tx-enabled on verify-enabled on verify-time 10 tx-min-frag-size 124
> ifconfig eth0 up
> sleep 10
> 
> tc qdisc add dev eth0 handle 100: root mqprio \
> num_tc 4 \
> map 0 1 2 3 \
> queues 1@0 1@1 1@2 1@3 \
> hw 1 \
> mode dcb \
> fp P P P E
> 
> tc -g class show dev eth0
> tc qdisc add dev eth0 clsact
> tc filter add dev eth0 egress protocol ip prio 1 u32 match ip dport 5002 0xffff action skbedit priority 2
> tc filter add dev eth0 egress protocol ip prio 1 u32 match ip dport 5003 0xffff action skbedit priority 3
> ip addr add 192.168.3.102/24 dev eth0 
> 
> - check that MAC merge verification has succeeded
> 
> ethtool --show-mm eth0
> 
>         MAC Merge layer state for eth0:
>         pMAC enabled: on
>         TX enabled: on
>         TX active: on
>         TX minimum fragment size: 124
>         RX minimum fragment size: 124
>         Verify enabled: on
>         Verify time: 10
>         Max verify time: 134
>         Verification status: SUCCEEDED
> 
> - On receiver EVM run 2 iperf instances
> 
> iperf3 -s -i30 -p5002&
> iperf3 -s -i30 -p5003&
> 
> - On sender EVM run 2 iperf instances
> 
> iperf3 -c 192.168.3.102 -u -b200M -l1472 -u -t5 -i30 -p5002&
> iperf3 -c 192.168.3.102 -u -b50M -l1472 -u -t5 -i30 -p5003&
> 
> - Check IET stats on sender. Look for MACMergeFragCountTx: increments
> 
> ethtool -I --show-mm eth0
> MAC Merge layer state for eth0:
> pMAC enabled: on
> TX enabled: on
> TX active: on
> TX minimum fragment size: 124
> RX minimum fragment size: 124
> Verify enabled: on
> Verify time: 10
> Max verify time: 134
> Verification status: SUCCEEDED
> Statistics:
>   MACMergeFrameAssErrorCount: 0
>   MACMergeFrameSmdErrorCount: 0
>   MACMergeFrameAssOkCount: 0
>   MACMergeFragCountRx: 0
>   MACMergeFragCountTx: 57824
>   MACMergeHoldCount: 0
> 
> - Check IET stats on receiver. Look for MACMergeFragCountRx: and
>   MACMergeFrameAssOkCount:
> 
> ethtool -I --show-mm eth0
> MAC Merge layer state for eth0:
> pMAC enabled: on
> TX enabled: on
> TX active: on
> TX minimum fragment size: 124
> RX minimum fragment size: 124
> Verify enabled: on
> Verify time: 10
> Max verify time: 134
> Verification status: SUCCEEDED
> Statistics:
>   MACMergeFrameAssErrorCount: 0
>   MACMergeFrameSmdErrorCount: 0
>   MACMergeFrameAssOkCount: 57018
>   MACMergeFragCountRx: 57824
>   MACMergeFragCountTx: 0
>   MACMergeHoldCount: 0

Nice of you to post commands, but could you also please clearly state
whether the implementation passes tools/testing/selftests/net/forwarding/ethtool_mm.sh?

> +	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;
> +
> +	/* enable/disable pre-emption based on link status */

For the benefit of grep, I would appreciate if it was spelled
"preemption" everywhere.

> +	am65_cpsw_iet_commit_preemptible_tcs(port);
> +
> +	mutex_unlock(&priv->mm_lock);
> +
> +	return 0;
> +}
> +
>  static int am65_cpsw_port_est_enabled(struct am65_cpsw_port *port)
>  {
>  	return port->qos.est_oper || port->qos.est_admin;
> @@ -602,6 +743,8 @@ static int am65_cpsw_setup_taprio(struct net_device *ndev, void *type_data)
>  	if (port->qos.link_speed == SPEED_UNKNOWN)
>  		return -ENOLINK;
>  
> +	am65_cpsw_iet_change_preemptible_tcs(port, taprio->mqprio.preemptible_tcs);
> +

Hmm, why just look at the preemptible traffic classes and not at
taprio's entire mqprio configuration? This bypasses the mapping between
Linux traffic classes and switch priorities that you've established in
am65_cpsw_setup_mqprio().

With the addition of the "mqprio" structure in tc_taprio_qopt_offload,
my intention was to facilitate calling am65_cpsw_setup_mqprio() from
am65_cpsw_setup_taprio().

>  	return am65_cpsw_set_taprio(ndev, type_data);
>  }

Powered by blists - more mailing lists