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