Re: [net-next] net: ethernet: rtsn: Add support for Renesas Ethernet-TSN

["Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>]

Hi Andrew,

Thanks for your thorough review, much appreciated.

I agree with all suggestions except one and will fix those for v2.

On 2024-04-16 00:55:12 +0200, Andrew Lunn wrote:

<snip>

> > +static void rtsn_set_delay_mode(struct rtsn_private *priv)
> > +{
> > +	struct device_node *np = priv->ndev->dev.parent->of_node;
> > +	u32 delay;
> > +	u32 val;
> > +
> > +	val = 0;
> > +
> > +	/* Valid values are 0 and 1800, according to DT bindings */
> 
> The bindings should not matter. It is what the hardware supports. The
> bindings should match the hardware, since it is hard to modify the
> hardware to make it match the binding.

I agree the comment could be improved. It should likely point to the 
datasheet instead. See below for why.

> 
> > +	if (!of_property_read_u32(np, "rx-internal-delay-ps", &delay))
> > +		if (delay)
> > +			val |= GPOUT_RDM;
> > +
> > +	/* Valid values are 0 and 2000, according to DT bindings */
> > +	if (!of_property_read_u32(np, "tx-internal-delay-ps", &delay))
> > +		if (delay)
> > +			val |= GPOUT_TDM;
> > +
> > +	rtsn_write(priv, GPOUT, val);
> 
> So you seem to be using it as bool?

Yes.

> That is wrong. It is a number of pico seconds!

The issue is that the hardware only supports no delay or a fixed delay 
that can depend on electric properties of the board. The datasheets 
states that the typical Rx delay is 1800 ps while the typical Tx delay 
is 2000 ps. The hardware register implementation for this is a single 
bit for each delay, on or off.

To model this in the bindings after some discussions [1] the standard 
property was picked over a vendor specific bool variant of it. Here in 
the driver I tried to document that the binding will enforce the value 
to either be 0 or {1800,2000}, but that for the hardware it should be 
treated as a on/off switch.

<snip>

1. https://lore.kernel.org/linux-renesas-soc/ZVzbigCtv2q_2-Bx@oden.dyn.berto.se/

-- 
Kind Regards,
Niklas Söderlund