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

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

Hi Antonio,

some responses below!

On Thu, Oct 31, 2024 at 2:44 PM Antonio Borneo
<antonio.borneo@xxxxxxxxxxx> wrote:

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

I like this property the most. Can we go with the generic
skew-delay-direction?

Also state in the existing skew-delay property that if
skew-delay-direction is not
present then it is assumed that the property applies to all
directions of a pin.

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

Hmmmm. Let's see. I know that e.g. MMC has similar properties
and if similar things appear in other bindings (not necessarily
pinctrl bindings) then that should also be taken into account.

And in MMC this is called DDR.

Yours,
Linus Walleij