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 14:02, Guillaume Nault wrote:
> On Thu, Nov 14, 2024 at 12:12:47PM +0200, Roger Quadros wrote:
>> On 14/11/2024 11:41, Roger Quadros wrote:
>>> On 14/11/2024 02:16, Guillaume Nault wrote:
>>>> 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?
> 
> Looks generally good to me. A few comments inline though.
> 
>> 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
> 
> Maybe also add a link to
> https://datatracker.ietf.org/doc/html/rfc8622#section-11
> which defines the LE PHB (Low Effort) and updates RFC 8325 accordingly.
> 
>> 	 */
>> 	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 */
> 
> Until case 32 (CS4) you've kept the order of RFC 8325, table 1.
> It'd make life easier for reviewers if you could keep this order
> here. That is, moving CS4 after AF43 and CS3 after AF33.
> 
>> 			pri = 4;
>> 			break;
>> 		case 17:	/* AF21 */
> 
> AF21 is 18, not 17.
> 
>> 		case 20:	/* AF22 */
>> 		case 22:	/* AF23 */
>> 			pri = 3;
>> 			break;
>> 		case 8:		/* CS1 */
> 
> Let's be complete and add the case for LE (RFC 8622), which also
> maps to 1.

All comments are valid. I will fix and send v4 for this series.

> 
>> 			pri = 1;
>> 			break;

For sake of completeness I will mention CS2, AF11, AF12, AF13
here that can fallback to default case.

>> 		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);
>> }
>>
>>>

-- 
cheers,
-roger