Re: [PATCH net-next v3 2/2] net: ethernet: ti: am65-cpsw: enable DSCP to priority map for RX

[Roger Quadros <rogerq@kernel.org>]

On 14/11/2024 11:41, Roger Quadros wrote:
> 
> 
> On 14/11/2024 02:16, Guillaume Nault wrote:
>> On Sat, Nov 09, 2024 at 01:00:08PM +0200, Roger Quadros wrote:
>>> AM65 CPSW hardware can map the 6-bit DSCP/TOS field to
>>> appropriate priority queue via DSCP to Priority mapping registers
>>> (CPSW_PN_RX_PRI_MAP_REG).
>>>
>>> We use the upper 3 bits of the DSCP field that indicate IP Precedence
>>> to map traffic to 8 priority queues.
>>>
>>> Signed-off-by: Roger Quadros <rogerq@kernel.org>
>>> ---
>>>  drivers/net/ethernet/ti/am65-cpsw-nuss.c | 54 ++++++++++++++++++++++++++++++++
>>>  1 file changed, 54 insertions(+)
>>>
>>> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>>> index 0520e9f4bea7..fab35e6aac7f 100644
>>> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>>> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>>> @@ -71,6 +71,8 @@
>>>  #define AM65_CPSW_PORT_REG_RX_PRI_MAP		0x020
>>>  #define AM65_CPSW_PORT_REG_RX_MAXLEN		0x024
>>>  
>>> +#define AM65_CPSW_PORTN_REG_CTL			0x004
>>> +#define AM65_CPSW_PORTN_REG_DSCP_MAP		0x120
>>>  #define AM65_CPSW_PORTN_REG_SA_L		0x308
>>>  #define AM65_CPSW_PORTN_REG_SA_H		0x30c
>>>  #define AM65_CPSW_PORTN_REG_TS_CTL              0x310
>>> @@ -94,6 +96,10 @@
>>>  /* AM65_CPSW_PORT_REG_PRI_CTL */
>>>  #define AM65_CPSW_PORT_REG_PRI_CTL_RX_PTYPE_RROBIN	BIT(8)
>>>  
>>> +/* AM65_CPSW_PN_REG_CTL */
>>> +#define AM65_CPSW_PN_REG_CTL_DSCP_IPV4_EN	BIT(1)
>>> +#define AM65_CPSW_PN_REG_CTL_DSCP_IPV6_EN	BIT(2)
>>> +
>>>  /* AM65_CPSW_PN_TS_CTL register fields */
>>>  #define AM65_CPSW_PN_TS_CTL_TX_ANX_F_EN		BIT(4)
>>>  #define AM65_CPSW_PN_TS_CTL_TX_VLAN_LT1_EN	BIT(5)
>>> @@ -176,6 +182,53 @@ static void am65_cpsw_port_set_sl_mac(struct am65_cpsw_port *slave,
>>>  	writel(mac_lo, slave->port_base + AM65_CPSW_PORTN_REG_SA_L);
>>>  }
>>>  
>>> +#define AM65_CPSW_DSCP_MAX	GENMASK(5, 0)
>>> +#define AM65_CPSW_PRI_MAX	GENMASK(2, 0)
>>> +#define AM65_CPSW_DSCP_PRI_PER_REG	8
>>> +#define AM65_CPSW_DSCP_PRI_SIZE		4	/* in bits */
>>> +static int am65_cpsw_port_set_dscp_map(struct am65_cpsw_port *slave, u8 dscp, u8 pri)
>>> +{
>>> +	int reg_ofs;
>>> +	int bit_ofs;
>>> +	u32 val;
>>> +
>>> +	if (dscp > AM65_CPSW_DSCP_MAX)
>>> +		return -EINVAL;
>>> +
>>> +	if (pri > AM65_CPSW_PRI_MAX)
>>> +		return -EINVAL;
>>> +
>>> +	/* 32-bit register offset to this dscp */
>>> +	reg_ofs = (dscp / AM65_CPSW_DSCP_PRI_PER_REG) * 4;
>>> +	/* bit field offset to this dscp */
>>> +	bit_ofs = AM65_CPSW_DSCP_PRI_SIZE * (dscp % AM65_CPSW_DSCP_PRI_PER_REG);
>>> +
>>> +	val = readl(slave->port_base + AM65_CPSW_PORTN_REG_DSCP_MAP + reg_ofs);
>>> +	val &= ~(AM65_CPSW_PRI_MAX << bit_ofs);	/* clear */
>>> +	val |= pri << bit_ofs;			/* set */
>>> +	writel(val, slave->port_base + AM65_CPSW_PORTN_REG_DSCP_MAP + reg_ofs);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static void am65_cpsw_port_enable_dscp_map(struct am65_cpsw_port *slave)
>>> +{
>>> +	int dscp, pri;
>>> +	u32 val;
>>> +
>>> +	/* Map IP Precedence field to Priority */
>>> +	for (dscp = 0; dscp <= AM65_CPSW_DSCP_MAX; dscp++) {
>>> +		pri = dscp >> 3; /* Extract IP Precedence */
>>> +		am65_cpsw_port_set_dscp_map(slave, dscp, pri);
>>> +	}
>>> +
>>> +	/* enable port IPV4 and IPV6 DSCP for this port */
>>> +	val = readl(slave->port_base + AM65_CPSW_PORTN_REG_CTL);
>>> +	val |= AM65_CPSW_PN_REG_CTL_DSCP_IPV4_EN |
>>> +		AM65_CPSW_PN_REG_CTL_DSCP_IPV6_EN;
>>> +	writel(val, slave->port_base + AM65_CPSW_PORTN_REG_CTL);
>>> +}
>>
>> It seems that this hardware is capable of mapping all possible DSCP
> yes.
> 
>> values. Then why restricting the mapping to the 3 high order bits only?
> 
> Currently, the 64 DSCP values are mapped to 8 User Priorities (UP) based
> on just the Class Selector Codepoint field (first 3 bits of DSCP).
> 
> But now looking at rfc8325#section-4.3.
> "Note: All unused codepoints are RECOMMENDED to be mapped to UP 0"
> 
> So what this patch does doesn't look like a good idea.
> 
>> According to RFC 8325 section 2.3, this seem to be a common practice,
>> which this RFC considers a problem:
>> https://datatracker.ietf.org/doc/html/rfc8325#section-2.3
> 
> Good to know about this.
> 
>>
>> I know this RFC is about 802.11, not 802.1p, but as far as I know, the
>> user priority (UP) are the same for both, so that shouldn't make a
>> difference.
>>
>> So what about following the IETF mapping found in section 4.3?
>> https://datatracker.ietf.org/doc/html/rfc8325#section-4.3
> 
> Thanks for this tip.
> I will update this patch to have the default DSCP to UP mapping as per
> above link and map all unused DSCP to UP 0.

How does the below code look in this regard?

static void am65_cpsw_port_enable_dscp_map(struct am65_cpsw_port *slave)
{
	int dscp, pri;
	u32 val;

	/* Default DSCP to User Priority mapping as per:
	 * https://datatracker.ietf.org/doc/html/rfc8325#section-4.3
	 */
	for (dscp = 0; dscp <= AM65_CPSW_DSCP_MAX; dscp++) {
		switch (dscp) {
		case 56:	/* CS7 */
		case 48:	/* CS6 */
			pri = 7;
			break;
		case 46:	/* EF */
		case 44:	/* VA */
			pri = 6;
			break;
		case 40:	/* CS5 */
			pri = 5;
			break;
		case 32:	/* CS4 */
		case 34:	/* AF41 */
		case 36:	/* AF42 */
		case 38:	/* AF43 */
		case 24:	/* CS3 */
		case 26:	/* AF31 */
		case 28:	/* AF32 */
		case 30:	/* AF33 */
			pri = 4;
			break;
		case 17:	/* AF21 */
		case 20:	/* AF22 */
		case 22:	/* AF23 */
			pri = 3;
			break;
		case 8:		/* CS1 */
			pri = 1;
			break;
		default:
			pri = 0;
			break;
		}

		am65_cpsw_port_set_dscp_map(slave, dscp, pri);
	}

	/* enable port IPV4 and IPV6 DSCP for this port */
	val = readl(slave->port_base + AM65_CPSW_PORTN_REG_CTL);
	val |= AM65_CPSW_PN_REG_CTL_DSCP_IPV4_EN |
		AM65_CPSW_PN_REG_CTL_DSCP_IPV6_EN;
	writel(val, slave->port_base + AM65_CPSW_PORTN_REG_CTL);
}

> 
> Is there any mechanism/API for network administrator to change this
> default mapping in the network drivers?
> 
>>
>>>  static void am65_cpsw_sl_ctl_reset(struct am65_cpsw_port *port)
>>>  {
>>>  	cpsw_sl_reset(port->slave.mac_sl, 100);
>>> @@ -921,6 +974,7 @@ static int am65_cpsw_nuss_ndo_slave_open(struct net_device *ndev)
>>>  	common->usage_count++;
>>>  
>>>  	am65_cpsw_port_set_sl_mac(port, ndev->dev_addr);
>>> +	am65_cpsw_port_enable_dscp_map(port);
>>>  
>>>  	if (common->is_emac_mode)
>>>  		am65_cpsw_init_port_emac_ale(port);
>>>
>>> -- 
>>> 2.34.1
>>>
>>>
>>
> 

-- 
cheers,
-roger