On Thu, Nov 14, 2024 at 02:47:07PM +0200, Roger Quadros wrote:

On 14/11/2024 14:02, Guillaume Nault wrote:

quoted

On Thu, Nov 14, 2024 at 12:12:47PM +0200, Roger Quadros wrote:

quoted

On 14/11/2024 11:41, Roger Quadros wrote:

quoted

On 14/11/2024 02:16, Guillaume Nault wrote:

quoted

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.

quoted

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.

quoted

	 */
	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.

quoted

			pri = 4;
			break;
		case 17:	/* AF21 */

AF21 is 18, not 17.

quoted

		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.

quoted

quoted

			pri = 1;
			break;

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

Yes, very nice.

quoted

quoted

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

quoted

-- 
cheers,
-roger