Re: [PATCH 07/14] dt-bindings: pinctrl: stm32: support IO synchronization parameters

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

On Fri, 2024-10-25 at 00:38 +0200, Linus Walleij wrote:
> Hi Antonio/Fabien,
> 
> thanks for your patch!
> 
> On Tue, Oct 22, 2024 at 5:59 PM Antonio Borneo
> <antonio.borneo@foss.st.com> wrote:
> 
> > From: Fabien Dessenne <fabien.dessenne@foss.st.com>
> > 
> > Support the following IO synchronization parameters:
> > - Delay (in ns)
> > - Delay path (input / output)
> > - Clock edge (single / double edge)
> > - Clock inversion
> > - Retiming
> > 
> > Signed-off-by: Fabien Dessenne <fabien.dessenne@foss.st.com>
> > Signed-off-by: Antonio Borneo <antonio.borneo@foss.st.com>
> (...)
> 
> I want to check if we already have some of these properties
> and if we don't, if they could and should be made generic,
> i.e. will we see more of them, also from other vendors?

Hi Linus,

Thanks for your review.

Apart for the generic property 'skew-delay' that you mentioned below, I cannot find other I can re-use here.

I'm preparing a V2 taking care of the observation from Krzysztof and you.

I will surely take 'skew-delay' in place of 'st,io-delay'.

> > +          st,io-delay-path:
> > +            description: |
> > +              IO synchronization delay path location
> > +              0: Delay switched into the output path
> > +              1: Delay switched into the input path
> > +            $ref: /schemas/types.yaml#/definitions/uint32
> > +            enum: [0, 1]
> 
> This looks related to the st,io-delay below so please keep those
> properties together.
> 
> Is this path identification really needed in practice, isn't it
> implicit from other pin config properties if the pin is used as
> input or output, and in that case where the delay applies?
> 
> Do you really have - in practice - pins that change between
> input and output and need different delays at runtime (i.e. not
> at startup)?
> 
> Otherwise I would say that just checking if the line is in input
> or output from other properties should be enough to configure
> this? input-enable, output-enable to name the obvious.

On STM32MP25x there is a 'skew-delay' HW block on each pin, but it's applied independently on each pin either only on the input direction OR only on the output direction.
There is no automatic way to switch it between input and output path. This property assigns the delay to one path.
The generic property 'skew-delay' does not considers this case.

While I could extend the pinctrl driver to include the info about direction, that is trivial for example for UART or SPI, it will fail for bidirectional pins like I2C's SDA; some use case could
require the skew-delay on SDA input path, other on the output path.
Also the idea of assigning the direction at startup (e.g. in the bootloader) is not feasible as the delay depends on the functionality that can change at runtime e.g. by loading modules.
I prefer having this "direction" path explicitly selected through a DT property.

The existing properties 'input-enable' and 'output-enable' are not specific for the skew-delay.
And I think it would be confusing having 'input-enable' or 'output-enable' associated with a bidirectional pins like I2C's SDA.

I propose to change it as, e.g.
  st,skew-delay-on-input:
    type: boolean
    description: |
      If this property is present, then skew-delay applies to input path only,
      otherwise it applies to output patch only.

Or, it could be a new generic property (keeping backward compatibility), e.g.:
  skew-delay-direction:
    enum [0, 1, 2]
    default: 0
    description: |
      0: skew-delay applies to both input and output path, or it switches automatically
         between the two direction
      1: skew-delay applies only to input path
      2: skew-delay applies only to output path

> > +          st,io-clk-edge:
> > +            description: |
> > +              IO synchronization clock edge
> > +              0: Data single-edge (changing on rising or falling clock edge)
> > +              1: Data double-edge (changing on both clock edges)
> > +            $ref: /schemas/types.yaml#/definitions/uint32
> > +            enum: [0, 1]
> 
> This looks like it should be made into a generic property,

I believe it is too specific to ST implementation.
I see already some 'retime' mentioned in old ST bindings bindings/pinctrl/pinctrl-st.txt and bindings/net/sti-dwmac.txt, but the control looks quite different; I don't plan to reuse them.

I will fuse in V2 this property together with the next two in a more meaningful one, partially acknowledging your proposal below.

> it seems to be about how the logic is used rather than something
> electronic but arguable fits in pin config.
> 
> Isn't this usually called DDR (double data rate) in tech speak?
> 
> What about a generic property "double-data-rate"?
> 
> > +          st,io-clk-type:
> > +            description: |
> > +              IO synchronization clock inversion
> > +              0: IO clocks not inverted. Data retimed to rising clock edge
> > +              1: IO clocks inverted. Data retimed to falling clock edge
> > +            $ref: /schemas/types.yaml#/definitions/uint32
> > +            enum: [0, 1]
> 
> Doesn't this require st,io-retime to be specified at the same time?
> 
> Then we should add some YAML magic (if we can) to make sure
> that happens.
> 
> > +          st,io-retime:
> > +            description: |
> > +              IO synchronization data retime
> > +              0: Data not synchronized or retimed on clock edges
> > +              1: Data retimed to either rising or falling clock edge
> > +            $ref: /schemas/types.yaml#/definitions/uint32
> > +            enum: [0, 1]
> 
> Can't these two be merged into one (generic) property:
> 
> io-retime
> 
> enum [0, 1, 2]
> 
> 0=none
> 1=rising retime
> 2=falling retime
> 
> Retiming seems like a very generic concept so I think it should
> be made into a generic property.
> 
> > +          st,io-delay:
> > +            description: |
> > +              IO synchronization delay applied to the input or output path
> > +              0: No delay
> > +              1: Delay 0.30 ns
> > +              2: Delay 0.50 ns
> > +              3: Delay 0.75 ns
> > +              4: Delay 1.00 ns
> > +              5: Delay 1.25 ns
> > +              6: Delay 1.50 ns
> > +              7: Delay 1.75 ns
> > +              8: Delay 2.00 ns
> > +              9: Delay 2.25 ns
> > +              10: Delay 2.50 ns
> > +              11: Delay 2.75 ns
> > +              12: Delay 3.00 ns
> > +              13: Delay 3.25 ns
> > +            $ref: /schemas/types.yaml#/definitions/uint32
> > +            minimum: 0
> > +            maximum: 13
> 
> This looks very similar to the existing "skew-delay" property:
> 
>   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.
> 
> can't we just use that?
> 
> Feel free to edit the text for it in
> Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml
> if that is too clock-specific.

I find that text already accurate; I don't plan to change it.
But adding a generic 'skew-delay-direction' could eventually require a mention in the description of 'skew-delay'.

Best Regards,
Antonio