| Message ID | 20241022155658.1647350-8-antonio.borneo@foss.st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pinctrl: stm32: Add new features and support for more SoC | expand |
On Tue, Oct 22, 2024 at 05:56:51PM +0200, Antonio Borneo 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 Why? What is missing for existing hardware support? > > Signed-off-by: Fabien Dessenne <fabien.dessenne@foss.st.com> > Signed-off-by: Antonio Borneo <antonio.borneo@foss.st.com> > --- > .../bindings/pinctrl/st,stm32-pinctrl.yaml | 48 +++++++++++++++++++ > 1 file changed, 48 insertions(+) > > diff --git a/Documentation/devicetree/bindings/pinctrl/st,stm32-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/st,stm32-pinctrl.yaml > index 5d17d6487ae9c..9a7ecfea6eb5b 100644 > --- a/Documentation/devicetree/bindings/pinctrl/st,stm32-pinctrl.yaml > +++ b/Documentation/devicetree/bindings/pinctrl/st,stm32-pinctrl.yaml > @@ -207,6 +207,54 @@ patternProperties: > 3: High speed > $ref: /schemas/types.yaml#/definitions/uint32 > enum: [0, 1, 2, 3] > + 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] Why enum? Why not bool? What is the "synchronization delay"? Why this is needed per board? > + 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] All the same questions. > + 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] OK, so if not bool why this cannot be a readable string? > + 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] Missing blank lines everywhere between properties. > + st,io-delay: Use proper unit suffix. Or is there no such? Best regards, Krzysztof
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? > + 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. > + 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, 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. Yours, Linus Walleij
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
Hi Antonio, some responses below! On Thu, Oct 31, 2024 at 2:44 PM Antonio Borneo <antonio.borneo@foss.st.com> 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
diff --git a/Documentation/devicetree/bindings/pinctrl/st,stm32-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/st,stm32-pinctrl.yaml index 5d17d6487ae9c..9a7ecfea6eb5b 100644 --- a/Documentation/devicetree/bindings/pinctrl/st,stm32-pinctrl.yaml +++ b/Documentation/devicetree/bindings/pinctrl/st,stm32-pinctrl.yaml @@ -207,6 +207,54 @@ patternProperties: 3: High speed $ref: /schemas/types.yaml#/definitions/uint32 enum: [0, 1, 2, 3] + 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] + 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] + 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] + 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] + 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 required: - pinmux