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

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

[Roger Quadros]

Configure DSCP to Priority mapping registers so that IP precedence
field (top 3 bits of DSCP) map it to one of the 8 priority queues
for RX traffic.
    
Also update Priority to Thread maping to be compliant with
IEEE802.1Q-2014. Priority Code Point (PCP) 2 is higher priority than
PCP 0 (Best Effort). PCP 1 (Background) is lower priority than
PCP 0 (Best Effort).

Signed-off-by: Roger Quadros <rogerq@kernel.org>
---
Changes in v3:
- Added Reviewed-by tag to patch 1
- Added macros for DSCP PRI field size and DSCP PRI per register
- Drop unnecessary readl() in am65_cpsw_port_set_dscp_map()
- Link to v2: https://lore.kernel.org/r/20241107-am65-cpsw-multi-rx-dscp-v2-0-9e9cd1920035@kernel.org

Changes in v2:
- Updated references to more recent standard IEEE802.1Q-2014.
- Dropped reference to web link which might change in the future.
- Typo fix in commit log.
- Link to v1: https://lore.kernel.org/r/20241105-am65-cpsw-multi-rx-dscp-v1-0-38db85333c88@kernel.org

---
Roger Quadros (2):
      net: ethernet: ti: am65-cpsw: update pri_thread_map as per IEEE802.1Q-2014
      net: ethernet: ti: am65-cpsw: enable DSCP to priority map for RX

 drivers/net/ethernet/ti/am65-cpsw-nuss.c | 54 ++++++++++++++++++++++++++++++++
 drivers/net/ethernet/ti/cpsw_ale.c       | 36 ++++++++++++---------
 2 files changed, 76 insertions(+), 14 deletions(-)
---
base-commit: 42f7652d3eb527d03665b09edac47f85fb600924
change-id: 20241101-am65-cpsw-multi-rx-dscp-000b2c4af6d0

Best regards,
-- 
Roger Quadros <rogerq@kernel.org>

[PATCH net-next v3 1/2] net: ethernet: ti: am65-cpsw: update pri_thread_map as per IEEE802.1Q-2014

[Roger Quadros]

IEEE802.1Q-2014 supersedes IEEE802.1D-2004. Now Priority Code Point (PCP)
2 is no longer at a lower priority than PCP 0. PCP 1 (Background) is still
at a lower priority than PCP 0 (Best Effort).

Reference:
IEEE802.1Q-2014, Standard for Local and metropolitan area networks
  Table I-2 - Traffic type acronyms
  Table I-3 - Defining traffic types

Signed-off-by: Roger Quadros <rogerq@kernel.org>
Reviewed-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---
 drivers/net/ethernet/ti/cpsw_ale.c | 36 ++++++++++++++++++++++--------------
 1 file changed, 22 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw_ale.c b/drivers/net/ethernet/ti/cpsw_ale.c
index 8d02d2b21429..9f79056b3f48 100644
--- a/drivers/net/ethernet/ti/cpsw_ale.c
+++ b/drivers/net/ethernet/ti/cpsw_ale.c
@@ -1692,26 +1692,34 @@ static void cpsw_ale_policer_reset(struct cpsw_ale *ale)
 void cpsw_ale_classifier_setup_default(struct cpsw_ale *ale, int num_rx_ch)
 {
 	int pri, idx;
-	/* IEEE802.1D-2004, Standard for Local and metropolitan area networks
-	 *    Table G-2 - Traffic type acronyms
-	 *    Table G-3 - Defining traffic types
-	 * User priority values 1 and 2 effectively communicate a lower
-	 * priority than 0. In the below table 0 is assigned to higher priority
-	 * thread than 1 and 2 wherever possible.
-	 * The below table maps which thread the user priority needs to be
+
+	/* Reference:
+	 * IEEE802.1Q-2014, Standard for Local and metropolitan area networks
+	 *    Table I-2 - Traffic type acronyms
+	 *    Table I-3 - Defining traffic types
+	 * Section I.4 Traffic types and priority values, states:
+	 * "0 is thus used both for default priority and for Best Effort, and
+	 *  Background is associated with a priority value of 1. This means
+	 * that the value 1 effectively communicates a lower priority than 0."
+	 *
+	 * In the table below, Priority Code Point (PCP) 0 is assigned
+	 * to a higher priority thread than PCP 1 wherever possible.
+	 * The table maps which thread the PCP traffic needs to be
 	 * sent to for a given number of threads (RX channels). Upper threads
 	 * have higher priority.
 	 * e.g. if number of threads is 8 then user priority 0 will map to
-	 * pri_thread_map[8-1][0] i.e. thread 2
+	 * pri_thread_map[8-1][0] i.e. thread 1
 	 */
-	int pri_thread_map[8][8] = {	{ 0, 0, 0, 0, 0, 0, 0, 0, },
+
+	int pri_thread_map[8][8] = {   /* BK,BE,EE,CA,VI,VO,IC,NC */
+					{ 0, 0, 0, 0, 0, 0, 0, 0, },
 					{ 0, 0, 0, 0, 1, 1, 1, 1, },
 					{ 0, 0, 0, 0, 1, 1, 2, 2, },
-					{ 1, 0, 0, 1, 2, 2, 3, 3, },
-					{ 1, 0, 0, 1, 2, 3, 4, 4, },
-					{ 1, 0, 0, 2, 3, 4, 5, 5, },
-					{ 1, 0, 0, 2, 3, 4, 5, 6, },
-					{ 2, 0, 1, 3, 4, 5, 6, 7, } };
+					{ 0, 0, 1, 1, 2, 2, 3, 3, },
+					{ 0, 0, 1, 1, 2, 2, 3, 4, },
+					{ 1, 0, 2, 2, 3, 3, 4, 5, },
+					{ 1, 0, 2, 3, 4, 4, 5, 6, },
+					{ 1, 0, 2, 3, 4, 5, 6, 7 } };
 
 	cpsw_ale_policer_reset(ale);
 

-- 
2.34.1

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

[Roger Quadros]

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 | 54 ++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
index 0520e9f4bea7..fab35e6aac7f 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
+#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,53 @@ 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)
+#define AM65_CPSW_DSCP_PRI_PER_REG	8
+#define AM65_CPSW_DSCP_PRI_SIZE		4	/* in bits */
+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;
+
+	if (pri > AM65_CPSW_PRI_MAX)
+		return -EINVAL;
+
+	/* 32-bit register offset to this dscp */
+	reg_ofs = (dscp / AM65_CPSW_DSCP_PRI_PER_REG) * 4;
+	/* bit field offset to this dscp */
+	bit_ofs = AM65_CPSW_DSCP_PRI_SIZE * (dscp % AM65_CPSW_DSCP_PRI_PER_REG);
+
+	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);
+
+	return 0;
+}
+
+static void am65_cpsw_port_enable_dscp_map(struct am65_cpsw_port *slave)
+{
+	int dscp, pri;
+	u32 val;
+
+	/* Map IP Precedence field to Priority */
+	for (dscp = 0; dscp <= AM65_CPSW_DSCP_MAX; dscp++) {
+		pri = dscp >> 3; /* Extract IP Precedence */
+		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);
+}
+
 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

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

[Siddharth Vadapalli]

On Sat, Nov 09, 2024 at 01:00:08PM +0200, Roger Quadros wrote:
> 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>

Reviewed-by: Siddharth Vadapalli <s-vadapalli@ti.com>

Regards,
Siddharth.

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

[Simon Horman]

+ Ido and Guilliame

On Sat, Nov 09, 2024 at 01:00:06PM +0200, Roger Quadros wrote:
> Configure DSCP to Priority mapping registers so that IP precedence
> field (top 3 bits of DSCP) map it to one of the 8 priority queues
> for RX traffic.
>     
> Also update Priority to Thread maping to be compliant with
> IEEE802.1Q-2014. Priority Code Point (PCP) 2 is higher priority than
> PCP 0 (Best Effort). PCP 1 (Background) is lower priority than
> PCP 0 (Best Effort).
> 
> Signed-off-by: Roger Quadros <rogerq@kernel.org>

Hi Ido and Guilliame,

I am wondering if you could find time to review this series.

> ---
> Changes in v3:
> - Added Reviewed-by tag to patch 1
> - Added macros for DSCP PRI field size and DSCP PRI per register
> - Drop unnecessary readl() in am65_cpsw_port_set_dscp_map()
> - Link to v2: https://lore.kernel.org/r/20241107-am65-cpsw-multi-rx-dscp-v2-0-9e9cd1920035@kernel.org
> 
> Changes in v2:
> - Updated references to more recent standard IEEE802.1Q-2014.
> - Dropped reference to web link which might change in the future.
> - Typo fix in commit log.
> - Link to v1: https://lore.kernel.org/r/20241105-am65-cpsw-multi-rx-dscp-v1-0-38db85333c88@kernel.org
> 
> ---
> Roger Quadros (2):
>       net: ethernet: ti: am65-cpsw: update pri_thread_map as per IEEE802.1Q-2014
>       net: ethernet: ti: am65-cpsw: enable DSCP to priority map for RX
> 
>  drivers/net/ethernet/ti/am65-cpsw-nuss.c | 54 ++++++++++++++++++++++++++++++++
>  drivers/net/ethernet/ti/cpsw_ale.c       | 36 ++++++++++++---------
>  2 files changed, 76 insertions(+), 14 deletions(-)
> ---
> base-commit: 42f7652d3eb527d03665b09edac47f85fb600924
> change-id: 20241101-am65-cpsw-multi-rx-dscp-000b2c4af6d0
> 
> Best regards,
> -- 
> Roger Quadros <rogerq@kernel.org>
> 

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

[Guillaume Nault]

On Sat, Nov 09, 2024 at 01:00:08PM +0200, Roger Quadros wrote:
> 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 | 54 ++++++++++++++++++++++++++++++++
>  1 file changed, 54 insertions(+)
> 
> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> index 0520e9f4bea7..fab35e6aac7f 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
> +#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,53 @@ 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)
> +#define AM65_CPSW_DSCP_PRI_PER_REG	8
> +#define AM65_CPSW_DSCP_PRI_SIZE		4	/* in bits */
> +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;
> +
> +	if (pri > AM65_CPSW_PRI_MAX)
> +		return -EINVAL;
> +
> +	/* 32-bit register offset to this dscp */
> +	reg_ofs = (dscp / AM65_CPSW_DSCP_PRI_PER_REG) * 4;
> +	/* bit field offset to this dscp */
> +	bit_ofs = AM65_CPSW_DSCP_PRI_SIZE * (dscp % AM65_CPSW_DSCP_PRI_PER_REG);
> +
> +	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);
> +
> +	return 0;
> +}
> +
> +static void am65_cpsw_port_enable_dscp_map(struct am65_cpsw_port *slave)
> +{
> +	int dscp, pri;
> +	u32 val;
> +
> +	/* Map IP Precedence field to Priority */
> +	for (dscp = 0; dscp <= AM65_CPSW_DSCP_MAX; dscp++) {
> +		pri = dscp >> 3; /* Extract IP Precedence */
> +		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);
> +}

It seems that this hardware is capable of mapping all possible DSCP
values. Then why restricting the mapping to the 3 high order bits only?
According to RFC 8325 section 2.3, this seem to be a common practice,
which this RFC considers a problem:
https://datatracker.ietf.org/doc/html/rfc8325#section-2.3

I know this RFC is about 802.11, not 802.1p, but as far as I know, the
user priority (UP) are the same for both, so that shouldn't make a
difference.

So what about following the IETF mapping found in section 4.3?
https://datatracker.ietf.org/doc/html/rfc8325#section-4.3

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

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

[Guillaume Nault]

On Tue, Nov 12, 2024 at 02:08:33PM +0000, Simon Horman wrote:
> + Ido and Guilliame
> 
> On Sat, Nov 09, 2024 at 01:00:06PM +0200, Roger Quadros wrote:
> > Configure DSCP to Priority mapping registers so that IP precedence
> > field (top 3 bits of DSCP) map it to one of the 8 priority queues
> > for RX traffic.
> >     
> > Also update Priority to Thread maping to be compliant with
> > IEEE802.1Q-2014. Priority Code Point (PCP) 2 is higher priority than
> > PCP 0 (Best Effort). PCP 1 (Background) is lower priority than
> > PCP 0 (Best Effort).
> > 
> > Signed-off-by: Roger Quadros <rogerq@kernel.org>
> 
> Hi Ido and Guilliame,
> 
> I am wondering if you could find time to review this series.

I don't have the IEEE802.1Q-2014 spec at hand, so I focused on
patch 2/2.

> > ---
> > Changes in v3:
> > - Added Reviewed-by tag to patch 1
> > - Added macros for DSCP PRI field size and DSCP PRI per register
> > - Drop unnecessary readl() in am65_cpsw_port_set_dscp_map()
> > - Link to v2: https://lore.kernel.org/r/20241107-am65-cpsw-multi-rx-dscp-v2-0-9e9cd1920035@kernel.org
> > 
> > Changes in v2:
> > - Updated references to more recent standard IEEE802.1Q-2014.
> > - Dropped reference to web link which might change in the future.
> > - Typo fix in commit log.
> > - Link to v1: https://lore.kernel.org/r/20241105-am65-cpsw-multi-rx-dscp-v1-0-38db85333c88@kernel.org
> > 
> > ---
> > Roger Quadros (2):
> >       net: ethernet: ti: am65-cpsw: update pri_thread_map as per IEEE802.1Q-2014
> >       net: ethernet: ti: am65-cpsw: enable DSCP to priority map for RX
> > 
> >  drivers/net/ethernet/ti/am65-cpsw-nuss.c | 54 ++++++++++++++++++++++++++++++++
> >  drivers/net/ethernet/ti/cpsw_ale.c       | 36 ++++++++++++---------
> >  2 files changed, 76 insertions(+), 14 deletions(-)
> > ---
> > base-commit: 42f7652d3eb527d03665b09edac47f85fb600924
> > change-id: 20241101-am65-cpsw-multi-rx-dscp-000b2c4af6d0
> > 
> > Best regards,
> > -- 
> > Roger Quadros <rogerq@kernel.org>
> > 
> 

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

[Roger Quadros]

Hi Guillaume,

On 14/11/2024 02:19, Guillaume Nault wrote:
> On Tue, Nov 12, 2024 at 02:08:33PM +0000, Simon Horman wrote:
>> + Ido and Guilliame
>>
>> On Sat, Nov 09, 2024 at 01:00:06PM +0200, Roger Quadros wrote:
>>> Configure DSCP to Priority mapping registers so that IP precedence
>>> field (top 3 bits of DSCP) map it to one of the 8 priority queues
>>> for RX traffic.
>>>     
>>> Also update Priority to Thread maping to be compliant with
>>> IEEE802.1Q-2014. Priority Code Point (PCP) 2 is higher priority than
>>> PCP 0 (Best Effort). PCP 1 (Background) is lower priority than
>>> PCP 0 (Best Effort).
>>>
>>> Signed-off-by: Roger Quadros <rogerq@kernel.org>
>>
>> Hi Ido and Guilliame,
>>
>> I am wondering if you could find time to review this series.
> 
> I don't have the IEEE802.1Q-2014 spec at hand, so I focused on
> patch 2/2.

You can look at an older spec along with this page
https://en.wikipedia.org/wiki/IEEE_P802.1p#Priority_levels

> 
>>> ---
>>> Changes in v3:
>>> - Added Reviewed-by tag to patch 1
>>> - Added macros for DSCP PRI field size and DSCP PRI per register
>>> - Drop unnecessary readl() in am65_cpsw_port_set_dscp_map()
>>> - Link to v2: https://lore.kernel.org/r/20241107-am65-cpsw-multi-rx-dscp-v2-0-9e9cd1920035@kernel.org
>>>
>>> Changes in v2:
>>> - Updated references to more recent standard IEEE802.1Q-2014.
>>> - Dropped reference to web link which might change in the future.
>>> - Typo fix in commit log.
>>> - Link to v1: https://lore.kernel.org/r/20241105-am65-cpsw-multi-rx-dscp-v1-0-38db85333c88@kernel.org
>>>
>>> ---
>>> Roger Quadros (2):
>>>       net: ethernet: ti: am65-cpsw: update pri_thread_map as per IEEE802.1Q-2014
>>>       net: ethernet: ti: am65-cpsw: enable DSCP to priority map for RX
>>>
>>>  drivers/net/ethernet/ti/am65-cpsw-nuss.c | 54 ++++++++++++++++++++++++++++++++
>>>  drivers/net/ethernet/ti/cpsw_ale.c       | 36 ++++++++++++---------
>>>  2 files changed, 76 insertions(+), 14 deletions(-)
>>> ---
>>> base-commit: 42f7652d3eb527d03665b09edac47f85fb600924
>>> change-id: 20241101-am65-cpsw-multi-rx-dscp-000b2c4af6d0
>>>
>>> Best regards,
>>> -- 
>>> Roger Quadros <rogerq@kernel.org>
>>>
>>
> 

-- 
cheers,
-roger

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

[Roger Quadros]

On 14/11/2024 02:16, Guillaume Nault wrote:
> On Sat, Nov 09, 2024 at 01:00:08PM +0200, Roger Quadros wrote:
>> 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 | 54 ++++++++++++++++++++++++++++++++
>>  1 file changed, 54 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>> index 0520e9f4bea7..fab35e6aac7f 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
>> +#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,53 @@ 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)
>> +#define AM65_CPSW_DSCP_PRI_PER_REG	8
>> +#define AM65_CPSW_DSCP_PRI_SIZE		4	/* in bits */
>> +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;
>> +
>> +	if (pri > AM65_CPSW_PRI_MAX)
>> +		return -EINVAL;
>> +
>> +	/* 32-bit register offset to this dscp */
>> +	reg_ofs = (dscp / AM65_CPSW_DSCP_PRI_PER_REG) * 4;
>> +	/* bit field offset to this dscp */
>> +	bit_ofs = AM65_CPSW_DSCP_PRI_SIZE * (dscp % AM65_CPSW_DSCP_PRI_PER_REG);
>> +
>> +	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);
>> +
>> +	return 0;
>> +}
>> +
>> +static void am65_cpsw_port_enable_dscp_map(struct am65_cpsw_port *slave)
>> +{
>> +	int dscp, pri;
>> +	u32 val;
>> +
>> +	/* Map IP Precedence field to Priority */
>> +	for (dscp = 0; dscp <= AM65_CPSW_DSCP_MAX; dscp++) {
>> +		pri = dscp >> 3; /* Extract IP Precedence */
>> +		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);
>> +}
> 
> It seems that this hardware is capable of mapping all possible DSCP
yes.

> values. Then why restricting the mapping to the 3 high order bits only?

Currently, the 64 DSCP values are mapped to 8 User Priorities (UP) based
on just the Class Selector Codepoint field (first 3 bits of DSCP).

But now looking at rfc8325#section-4.3.
"Note: All unused codepoints are RECOMMENDED to be mapped to UP 0"

So what this patch does doesn't look like a good idea.

> According to RFC 8325 section 2.3, this seem to be a common practice,
> which this RFC considers a problem:
> https://datatracker.ietf.org/doc/html/rfc8325#section-2.3

Good to know about this.

> 
> I know this RFC is about 802.11, not 802.1p, but as far as I know, the
> user priority (UP) are the same for both, so that shouldn't make a
> difference.
> 
> 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.

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

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

[Roger Quadros]

On 14/11/2024 11:41, Roger Quadros wrote:
> 
> 
> On 14/11/2024 02:16, Guillaume Nault wrote:
>> On Sat, Nov 09, 2024 at 01:00:08PM +0200, Roger Quadros wrote:
>>> 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 | 54 ++++++++++++++++++++++++++++++++
>>>  1 file changed, 54 insertions(+)
>>>
>>> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>>> index 0520e9f4bea7..fab35e6aac7f 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
>>> +#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,53 @@ 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)
>>> +#define AM65_CPSW_DSCP_PRI_PER_REG	8
>>> +#define AM65_CPSW_DSCP_PRI_SIZE		4	/* in bits */
>>> +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;
>>> +
>>> +	if (pri > AM65_CPSW_PRI_MAX)
>>> +		return -EINVAL;
>>> +
>>> +	/* 32-bit register offset to this dscp */
>>> +	reg_ofs = (dscp / AM65_CPSW_DSCP_PRI_PER_REG) * 4;
>>> +	/* bit field offset to this dscp */
>>> +	bit_ofs = AM65_CPSW_DSCP_PRI_SIZE * (dscp % AM65_CPSW_DSCP_PRI_PER_REG);
>>> +
>>> +	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);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static void am65_cpsw_port_enable_dscp_map(struct am65_cpsw_port *slave)
>>> +{
>>> +	int dscp, pri;
>>> +	u32 val;
>>> +
>>> +	/* Map IP Precedence field to Priority */
>>> +	for (dscp = 0; dscp <= AM65_CPSW_DSCP_MAX; dscp++) {
>>> +		pri = dscp >> 3; /* Extract IP Precedence */
>>> +		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);
>>> +}
>>
>> It seems that this hardware is capable of mapping all possible DSCP
> yes.
> 
>> values. Then why restricting the mapping to the 3 high order bits only?
> 
> Currently, the 64 DSCP values are mapped to 8 User Priorities (UP) based
> on just the Class Selector Codepoint field (first 3 bits of DSCP).
> 
> But now looking at rfc8325#section-4.3.
> "Note: All unused codepoints are RECOMMENDED to be mapped to UP 0"
> 
> So what this patch does doesn't look like a good idea.
> 
>> According to RFC 8325 section 2.3, this seem to be a common practice,
>> which this RFC considers a problem:
>> https://datatracker.ietf.org/doc/html/rfc8325#section-2.3
> 
> Good to know about this.
> 
>>
>> I know this RFC is about 802.11, not 802.1p, but as far as I know, the
>> user priority (UP) are the same for both, so that shouldn't make a
>> difference.
>>
>> 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?

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
	 */
	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 */
			pri = 4;
			break;
		case 17:	/* AF21 */
		case 20:	/* AF22 */
		case 22:	/* AF23 */
			pri = 3;
			break;
		case 8:		/* CS1 */
			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

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

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 11:41:06AM +0200, 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.
> 
> Is there any mechanism/API for network administrator to change this
> default mapping in the network drivers?

I'm not aware of any (appart from manual update with tc), but I could
have missed something. It'd probably make sense to have such a
mechanism though.

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

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

[Roger Quadros]

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

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
>