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@xxxxxxxxxx>
> ---
> 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.
