Re: [PATCH net-next 2/2] net: ethernet: ti: am65-cpsw: enable DSCP to priority map for RX

On Fri, Nov 08, 2024 at 02:55:18PM +0200, Roger Quadros wrote:

Hi Siddharth,

On 08/11/2024 14:30, Siddharth Vadapalli wrote:

[...]

quoted

quoted

+#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/ (local)
where the indentation looks incorrect.

[...]

quoted

quoted

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

quoted

quoted

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

quoted

corresponds to. Or maybe two macros can be used for "reg_ofs" and
"bit_ofs".

quoted

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