Re: [PATCH net-next 2/2] net: ethernet: ti: am65-cpsw: enable DSCP to priority map for RX
From: Siddharth Vadapalli <s-vadapalli@ti.com>
Date: 2024-11-08 12:30:46
Also in:
linux-omap, lkml
On Tue, Nov 05, 2024 at 04:18:11PM +0200, Roger Quadros wrote: Hello Roger,
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
+#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.