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

[Siddharth Vadapalli]

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