| Message ID | 20241105-am65-cpsw-multi-rx-dscp-v1-2-38db85333c88@kernel.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | net: ethernet: ti: am65-cpsw: enable DSCP to priority map for RX | expand |
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. > +#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.
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?
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.
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);
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(+)