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

[Guillaume Nault <gnault@redhat.com>]

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.

> 			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
>