Re: [PATCH v3 02/10] dt-bindings: pincfg-node: Add properties 'skew-delay-{in,out}put'

[Antonio Borneo <antonio.borneo@foss.st.com>]

On Tue, 2025-10-14 at 20:39 +0100, Conor Dooley wrote:
> On Tue, Oct 14, 2025 at 09:33:14PM +0200, Linus Walleij wrote:
> > On Tue, Oct 14, 2025 at 8:04 PM Conor Dooley <conor@kernel.org> wrote:
> > > On Tue, Oct 14, 2025 at 04:04:43PM +0200, Antonio Borneo wrote:
> > 
> > > > +  skew-delay-input:
> > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > > +    description:
> > > > +      this affects the expected clock skew on input pins.
> > > > +      Typically indicates how many double-inverters are used to
> > > > +      delay the signal.
> > > 
> > > This property seems to be temporal, I would expect to see a unit of time
> > > mentioned here, otherwise it'll totally inconsistent in use between
> > > devices, and also a standard unit suffix in the property name.
> > > pw-bot: changes-requested
> > 
> > Don't blame the messenger, the existing property skew-delay
> > says:
> > 
> >   skew-delay:
> >     $ref: /schemas/types.yaml#/definitions/uint32
> >     description:
> >       this affects the expected clock skew on input pins
> >       and the delay before latching a value to an output
> >       pin. Typically indicates how many double-inverters are
> >       used to delay the signal.
> > 
> > This in turn comes from the original
> > Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> > document, which in turn comes from this commit:
> > 
> > commit e0e1e39de490a2d9b8a173363ccf2415ddada871
> > Author: Linus Walleij <linus.walleij@linaro.org>
> > Date:   Sat Oct 28 15:37:17 2017 +0200
> > 
> >     pinctrl: Add skew-delay pin config and bindings
> > 
> >     Some pin controllers (such as the Gemini) can control the
> >     expected clock skew and output delay on certain pins with a
> >     sub-nanosecond granularity. This is typically done by shunting
> >     in a number of double inverters in front of or behind the pin.
> >     Make it possible to configure this with a generic binding.
> > 
> >     Cc: devicetree@vger.kernel.org
> >     Acked-by: Rob Herring <robh@kernel.org>
> >     Acked-by: Hans Ulli Kroll <ulli.kroll@googlemail.com>
> >     Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> > 
> > So by legacy skew-delay is a custom format, usually the number of
> > (double) inverters.
> 
> Yeah, I actually noticed this after sending the mail. But as you say
> below, the new properties can be done with a unit etc

Quite an interesting discussion, here.

This skew delay is "implemented" inside the device through a set of
unitary delay cells (double inverters), where the value of the delay
of each cell can heavily depend on silicon process, temperature,
voltage, ...

When used to compensate skew "inside" the device, we can assume that
the skew of the signal to be compensated is also affected by process,
temperature, voltage, almost as the delay of each delay cell.
In this case it could be fine reporting the skew-delay as number of
delay cells because we don't really care about the value of delay in
time units.

But when used to compensate delay on the board, the value expressed
as time units is way more appropriate because it's what we get by the
board design.
And, this is the main use case in this series.

> 
> > 
> > I don't recall the reason for this way of defining things, but one reason
> > could be that the skew-delay incurred by two inverters is very
> > dependent on the production node of the silicon, and can be
> > nanoseconds or picoseconds, these days mostly picoseconds.
> > Example: Documentation/devicetree/bindings/net/adi,adin.yaml
> > 
> > Antonio, what do you say? Do you have the actual skew picosecond
> > values for the different settings so we could define this as
> > 
> > skew-delay-input-ps:
> > skew-delay-output-ps:
> > 
> > and translate it to the right register values in the driver?
> 
> The patch for the specific binding does have values in units of seconds
> assigned to each register value, so I think this should be doable.

While in this series I just kept the new properties uniform to
'skew-delay', I see no issue on using the '*-ps' version.
I will send a new version of the series.

What about the existing 'skew-delay'? Should it become deprecated in
favor of a new 'skew-delay-ps' ?

Thanks for the review!
Antonio

> 
> > 
> > If we have the proper data this could be a good time to add this
> > ISO unit to these two props.
> > 
> > Yours,
> > Linus Walleij