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

[Guillaume Nault]

On Thu, Nov 14, 2024 at 02:47:07PM +0200, Roger Quadros wrote:
> 
> 
> 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.

Yes, very nice.

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