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

[Roger Quadros <rogerq@kernel.org>]

Hi Siddharth,

On 08/11/2024 14:30, Siddharth Vadapalli wrote:
> On Tue, Nov 05, 2024 at 04:18:11PM +0200, Roger Quadros wrote:
> 
> Hello Roger,
> 
>> 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 | 50 ++++++++++++++++++++++++++++++++
>>  1 file changed, 50 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>> index 0520e9f4bea7..65fbf6727e02 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
> 
> nitpick: indentation needs to be fixed here to align with the macros
> below.

It is fine in the code and in my editor in this reply email.

> 
>> +#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,49 @@ 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)
>> +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;
> 
> am65_cpsw_port_set_dscp_map() seems to be invoked by
> am65_cpsw_port_enable_dscp_map() below, where the above check is guaranteed
> to be satisfied. Is the check added for future-proofing this function?
> 

Right, future callers can't be guaranteed to do the check so I'd prefer
to have the check here.

>> +
>> +	if (pri > AM65_CPSW_PRI_MAX)
>> +		return -EINVAL;
>> +
>> +	reg_ofs = (dscp / 8) * 4;	/* reg offset to this dscp */
>> +	bit_ofs = 4 * (dscp % 8);	/* bit offset to this dscp */
> 
> Maybe a macro can be used for the "4" since it is not clear what it

First 4 was for 4 bytes per register. Not sure if we need a macro for this.
The comment already mentions register offset and we know each register is
32-bits wide.

We could add a macro for the 8 though
#define AM65_CPSW_DSCP_PRI_PER_REG	8

The second 4 is actually 4 bits per DSCP field. I could add a macro for this.
#define AM65_CPSW_DSCP_PRI_FIELD_WIDTH	4


> corresponds to. Or maybe two macros can be used for "reg_ofs" and
> "bit_ofs".
> 
>> +	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);
>> +	val = readl(slave->port_base + AM65_CPSW_PORTN_REG_DSCP_MAP + reg_ofs);
> 
> The above readback seems to be just to flush the writel(). A comment of
> the form:
> /* flush */
> might help, considering that other drivers do the same. Also, assigning
> the returned value to "val" might not be required unless it is intended to
> be checked.

This was actually left over debug code. I'll drop the readl.

-- 
cheers,
-roger