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 | 50 ++++++++++++++++++++++++++++++++
1 file changed, 50 insertions(+)
diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
index 0520e9f4bea7..65fbf6727e02 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,49 @@ 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)
+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;
+
+ reg_ofs = (dscp / 8) * 4; /* reg offset to this dscp */
+ bit_ofs = 4 * (dscp % 8); /* bit offset to this dscp */
+ 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);
+ val = readl(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 +970,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.1On Tue, Nov 05, 2024 at 04:18:11PM +0200, Roger Quadros wrote:
Hello Roger,
> 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 | 50 ++++++++++++++++++++++++++++++++
> 1 file changed, 50 insertions(+)
>
> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> index 0520e9f4bea7..65fbf6727e02 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
nitpick: indentation needs to be fixed here to align with the macros
below.
> +#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,49 @@ 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)
> +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;
am65_cpsw_port_set_dscp_map() seems to be invoked by
am65_cpsw_port_enable_dscp_map() below, where the above check is guaranteed
to be satisfied. Is the check added for future-proofing this function?
> +
> + if (pri > AM65_CPSW_PRI_MAX)
> + return -EINVAL;
> +
> + reg_ofs = (dscp / 8) * 4; /* reg offset to this dscp */
> + bit_ofs = 4 * (dscp % 8); /* bit offset to this dscp */
Maybe a macro can be used for the "4" since it is not clear what it
corresponds to. Or maybe two macros can be used for "reg_ofs" and
"bit_ofs".
> + 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);
> + val = readl(slave->port_base + AM65_CPSW_PORTN_REG_DSCP_MAP + reg_ofs);
The above readback seems to be just to flush the writel(). A comment of
the form:
/* flush */
might help, considering that other drivers do the same. Also, assigning
the returned value to "val" might not be required unless it is intended to
be checked.
[...]
Regards,
Siddharth.
Hi Siddharth,
On 08/11/2024 14:30, Siddharth Vadapalli wrote:
> On Tue, Nov 05, 2024 at 04:18:11PM +0200, Roger Quadros wrote:
>
> Hello Roger,
>
>> 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 | 50 ++++++++++++++++++++++++++++++++
>> 1 file changed, 50 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>> index 0520e9f4bea7..65fbf6727e02 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
>
> nitpick: indentation needs to be fixed here to align with the macros
> below.
It is fine in the code and in my editor in this reply email.
>
>> +#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,49 @@ 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)
>> +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;
>
> am65_cpsw_port_set_dscp_map() seems to be invoked by
> am65_cpsw_port_enable_dscp_map() below, where the above check is guaranteed
> to be satisfied. Is the check added for future-proofing this function?
>
Right, future callers can't be guaranteed to do the check so I'd prefer
to have the check here.
>> +
>> + if (pri > AM65_CPSW_PRI_MAX)
>> + return -EINVAL;
>> +
>> + reg_ofs = (dscp / 8) * 4; /* reg offset to this dscp */
>> + bit_ofs = 4 * (dscp % 8); /* bit offset to this dscp */
>
> Maybe a macro can be used for the "4" since it is not clear what it
First 4 was for 4 bytes per register. Not sure if we need a macro for this.
The comment already mentions register offset and we know each register is
32-bits wide.
We could add a macro for the 8 though
#define AM65_CPSW_DSCP_PRI_PER_REG 8
The second 4 is actually 4 bits per DSCP field. I could add a macro for this.
#define AM65_CPSW_DSCP_PRI_FIELD_WIDTH 4
> corresponds to. Or maybe two macros can be used for "reg_ofs" and
> "bit_ofs".
>
>> + 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);
>> + val = readl(slave->port_base + AM65_CPSW_PORTN_REG_DSCP_MAP + reg_ofs);
>
> The above readback seems to be just to flush the writel(). A comment of
> the form:
> /* flush */
> might help, considering that other drivers do the same. Also, assigning
> the returned value to "val" might not be required unless it is intended to
> be checked.
This was actually left over debug code. I'll drop the readl.
--
cheers,
-roger
On Fri, Nov 08, 2024 at 02:55:18PM +0200, Roger Quadros wrote: > Hi Siddharth, > > On 08/11/2024 14:30, Siddharth Vadapalli wrote: [...] > >> +#define AM65_CPSW_PORTN_REG_CTL 0x004 > > > > nitpick: indentation needs to be fixed here to align with the macros > > below. > > It is fine in the code and in my editor in this reply email. That's strange. But it appears the same to me as seen at: https://lore.kernel.org/r/20241105-am65-cpsw-multi-rx-dscp-v1-2-38db85333c88@kernel.org/ where the indentation looks incorrect. [...] > > >> + > >> + if (dscp > AM65_CPSW_DSCP_MAX) > >> + return -EINVAL; > > > > am65_cpsw_port_set_dscp_map() seems to be invoked by > > am65_cpsw_port_enable_dscp_map() below, where the above check is guaranteed > > to be satisfied. Is the check added for future-proofing this function? > > > > Right, future callers can't be guaranteed to do the check so I'd prefer > to have the check here. Thank you for the confirmation. > > >> + > >> + if (pri > AM65_CPSW_PRI_MAX) > >> + return -EINVAL; > >> + > >> + reg_ofs = (dscp / 8) * 4; /* reg offset to this dscp */ > >> + bit_ofs = 4 * (dscp % 8); /* bit offset to this dscp */ > > > > Maybe a macro can be used for the "4" since it is not clear what it > > First 4 was for 4 bytes per register. Not sure if we need a macro for this. > The comment already mentions register offset and we know each register is > 32-bits wide. > > We could add a macro for the 8 though > #define AM65_CPSW_DSCP_PRI_PER_REG 8 > > The second 4 is actually 4 bits per DSCP field. I could add a macro for this. > #define AM65_CPSW_DSCP_PRI_FIELD_WIDTH 4 This looks good to me, but I am fine either way, in case you prefer to drop the macros. > > > > corresponds to. Or maybe two macros can be used for "reg_ofs" and > > "bit_ofs". > > > >> + 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); > >> + val = readl(slave->port_base + AM65_CPSW_PORTN_REG_DSCP_MAP + reg_ofs); > > > > The above readback seems to be just to flush the writel(). A comment of > > the form: > > /* flush */ > > might help, considering that other drivers do the same. Also, assigning > > the returned value to "val" might not be required unless it is intended to > > be checked. > > This was actually left over debug code. I'll drop the readl. Ok. Regards, Siddharth.
On 08/11/2024 16:42, Siddharth Vadapalli wrote: > On Fri, Nov 08, 2024 at 02:55:18PM +0200, Roger Quadros wrote: >> Hi Siddharth, >> >> On 08/11/2024 14:30, Siddharth Vadapalli wrote: > > [...] > >>>> +#define AM65_CPSW_PORTN_REG_CTL 0x004 >>> >>> nitpick: indentation needs to be fixed here to align with the macros >>> below. >> >> It is fine in the code and in my editor in this reply email. > > That's strange. But it appears the same to me as seen at: > https://lore.kernel.org/r/20241105-am65-cpsw-multi-rx-dscp-v1-2-38db85333c88@kernel.org/ > where the indentation looks incorrect. It is probably editor specific. There are in fact 3 tab spaces to align it with the number. Can you please apply the patch and see if it is OK in the code? -- cheers, -roger
On Sat, Nov 09, 2024 at 12:31:24PM +0200, Roger Quadros wrote: > > > On 08/11/2024 16:42, Siddharth Vadapalli wrote: > > On Fri, Nov 08, 2024 at 02:55:18PM +0200, Roger Quadros wrote: > >> Hi Siddharth, > >> > >> On 08/11/2024 14:30, Siddharth Vadapalli wrote: > > > > [...] > > > >>>> +#define AM65_CPSW_PORTN_REG_CTL 0x004 > >>> > >>> nitpick: indentation needs to be fixed here to align with the macros > >>> below. > >> > >> It is fine in the code and in my editor in this reply email. > > > > That's strange. But it appears the same to me as seen at: > > https://lore.kernel.org/r/20241105-am65-cpsw-multi-rx-dscp-v1-2-38db85333c88@kernel.org/ > > where the indentation looks incorrect. > > It is probably editor specific. There are in fact 3 tab spaces to align it > with the number. > > Can you please apply the patch and see if it is OK in the code? I still see the indentation being off, but maybe it is just me seeing this incorrectly. Regards, Siddharth.
© 2016 - 2024 Red Hat, Inc.