| Message ID | 20221212103525.231298-1-alexander.stein@ew.tq-group.com |
|---|---|
| Headers | show |
| Series | gpiolib: ramp-up delay support | expand |
Hello Laurent, thanks for your fast comments. Am Montag, 12. Dezember 2022, 13:40:50 CET schrieb Laurent Pinchart: > Hi Alexander, > > On Mon, Dec 12, 2022 at 11:35:22AM +0100, Alexander Stein wrote: > > Hi all, > > > > this series is an RFC for a general approach to solve the issue at [1]. > > While > I'm impressed by how fast you came up with a solution :-) > > > a device specific property works as well, a more generic approach is > > preferred. In short: When enabling a GPIO the actual ramp-up time might > > be (much) bigger than what software usually assume, in my case >100ms. > > Adding a delay to each driver is cumbersome. > > Instead the (optional) ramp-up delay is added to each gpio_desc. The > > delays can be specified per gpio-controller, similar to > > 'gpio-line-names'. Actually the parsing code is almost a 1:1 copy of > > devprop_gpiochip_set_names(). > While I like consistency, I wonder if it wouldn't be better in this case > to use a list of <gpio-number delay> cells in gpio-ramp-up-delays-us. In > typical use cases, very few GPIOs will need a delay, and a GPIO > controller could support a very large number of GPIOs, which would make > your current proposal cumbersome. That's a good idea. I would even go a step further to specify both ramp-up and ramp-down in one cell, e.g. <gpio-number ramp-up ramp-down>. This way a second property is not needed. > > Due to > > (temporary) memory allocation, I opted for a separate function, there is > > code duplication, but handling both properties in a single function > > seemed too tedious, let alone the to be added ramp-down delays. > > > > This feature could also be added as a callback in gpio_chip, but the > > callbacks have to be added to each driver then. I would prefer a single > > one-fits-all implementation and another indirection in the GPIO call > > chain. > > Agreed. > > > Laurent suggest to add a GPIO delay node in DT. IMHO this increased the DT > > complexity unnecessarily. But comments are welcome. > > It's an alternative approach that could be considered if this one is > rejected, but I have a preference for your solution. > > > The following 3 patches are a proof-of-concept on my platform, consisting > > of: Patch 1 is the proposed bindings and straight forward. > > Patch 2 is the current implementation > > Patch 3 is an actual usage example for specifying the delays > > > > TODO: > > 1. Adding ramp-down delays (Just the inverse copy of ramp-up delay) > > 2. Should these delays take active low flags into account? > > How so ? Given the name ramp-up (& ramp-down) I would assume they affect the voltage low -> high change (resp. high -> low), not just gpiod_set_value(..., 1). > > 3. How to deal with setting multiple GPIOs at once? > > > > I skipped 1. for now, because this is just a copy with ramp-up being > > replaced with ramp-down. > > > > I'm not that well versed in gpiolib code, so I'm not sure if I got all > > placed where GPIOs are set. So patch 2 might be incomplete. > > > > For now I skipped setting multiple GPIOs at once completely, so to get > > some > > feedback on this approach. A possible solution is to check for the bigest > > delay in the set and use that for all afterwards. But I'm not sure about > > the overhead in this case. > > I assume you're talking about the gpiod_set_array_value() API. That > sounds OK as an initial implementation, a caller of that function needs > to be prepared for the GPIOs being set in a random order due to hardware > delays, so it shouldn't break the API contract. I would however state > this explicitly in the function documentation. Okay, that seems sensible. Will do it. Best regards, Alexander > > I hope there is some feedback. While thinking about this issue appears to > > be more widespread than I expected. > > > > Best regards, > > Alexander > > > > [1] > > https://lore.kernel.org/all/20221209083339.3780776-1-alexander.stein@ew.t > > q-group.com/> > > Alexander Stein (3): > > dt-bindings: gpio: Add optional ramp-up delay property > > gpiolib: Add support for optional ramp-up delays > > arm64: dts: mba8mx: Add GPIO ramp-up delays > > > > .../devicetree/bindings/gpio/gpio.txt | 22 +++++ > > arch/arm64/boot/dts/freescale/mba8mx.dtsi | 5 ++ > > drivers/gpio/gpiolib.c | 80 +++++++++++++++++++ > > drivers/gpio/gpiolib.h | 3 + > > 4 files changed, 110 insertions(+)
On Mon, Dec 12, 2022 at 11:35 AM Alexander Stein <alexander.stein@ew.tq-group.com> wrote: > This adds a ramp-up delay (in us) for adding a delay after enabling > GPIO. This is necessary if the ramp-up time is increased by some external > components. Usually this is quite fast, but certain combinations can > increase this to grater than 100ms. > > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> (...) > +gpio-controller@00000000 { > + compatible = "foo"; > + reg = <0x00000000 0x1000>; > + gpio-controller; > + #gpio-cells = <2>; > + gpio-ramp-up-delays-us = <0>, <0>, <0>, <0>, > + <0>, <0>, <120000>, <0>, > + <0>, <0>, <0>, <0>, > + <0>, <0>, <0>, <0>; Why would this be different per-gpio-line? If this should be on the GPIO controller, this should be the ramp-up for the GPIO controller output itself, not for the random electronics that may or may not be attached to the line. Otherwise the ramp-up should certainly be on the consumer side. And that seems very much like what is going on here. Consider a gpio-regulator: Documentation/devicetree/bindings/regulator/fixed-regulator.yamlproperties: compatible: enum: - regulator-fixed - regulator-fixed-clock - regulator-fixed-domain regulator-name: true gpio: description: gpio to use for enable control maxItems: 1 (...) startup-delay-us: description: startup time in microseconds off-on-delay-us: description: off delay time in microseconds There is one consumer, and if you add ramp-up and ramp-down delays to the GPIO lines like this you have just created two ways of doing the same thing. When there is a ramp-up for a regulator now the used can choose to put it on the regulator or on the gpio. This is clearly ambiguous so NAK to this approach. IMO the property goes on the consumer due to precedence. [Other context] > Laurent suggest to add a GPIO delay node in DT. IMHO this increased the DT > complexity unnecessarily. But comments are welcome. If the consumer subsystem don't want it, I guess this is where you would have to go in and add more DT descriptions for the electronics on the board, which I understand is a bit frustrating, and it is hard to find the right trade-off. It makes me think about the classical problem "how long is the coast of Britain?" by Benoit Mandelbrot: https://en.wikipedia.org/wiki/How_Long_Is_the_Coast_of_Britain%3F_Statistical_Self-Similarity_and_Fractional_Dimension The DT maintainers will have the final word on it I guess. Yours, Linus Walleij
Hi Alexander, On Tue, Dec 13, 2022 at 08:49:06AM +0100, Alexander Stein wrote: > Am Montag, 12. Dezember 2022, 13:40:50 CET schrieb Laurent Pinchart: > > On Mon, Dec 12, 2022 at 11:35:22AM +0100, Alexander Stein wrote: > > > Hi all, > > > > > > this series is an RFC for a general approach to solve the issue at [1]. > > > While > > > > I'm impressed by how fast you came up with a solution :-) > > > > > a device specific property works as well, a more generic approach is > > > preferred. In short: When enabling a GPIO the actual ramp-up time might > > > be (much) bigger than what software usually assume, in my case >100ms. > > > Adding a delay to each driver is cumbersome. > > > Instead the (optional) ramp-up delay is added to each gpio_desc. The > > > delays can be specified per gpio-controller, similar to > > > 'gpio-line-names'. Actually the parsing code is almost a 1:1 copy of > > > devprop_gpiochip_set_names(). > > > > While I like consistency, I wonder if it wouldn't be better in this case > > to use a list of <gpio-number delay> cells in gpio-ramp-up-delays-us. In > > typical use cases, very few GPIOs will need a delay, and a GPIO > > controller could support a very large number of GPIOs, which would make > > your current proposal cumbersome. > > That's a good idea. I would even go a step further to specify both ramp-up and > ramp-down in one cell, e.g. <gpio-number ramp-up ramp-down>. This way a second > property is not needed. > > > > Due to > > > (temporary) memory allocation, I opted for a separate function, there is > > > code duplication, but handling both properties in a single function > > > seemed too tedious, let alone the to be added ramp-down delays. > > > > > > This feature could also be added as a callback in gpio_chip, but the > > > callbacks have to be added to each driver then. I would prefer a single > > > one-fits-all implementation and another indirection in the GPIO call > > > chain. > > > > Agreed. > > > > > Laurent suggest to add a GPIO delay node in DT. IMHO this increased the DT > > > complexity unnecessarily. But comments are welcome. > > > > It's an alternative approach that could be considered if this one is > > rejected, but I have a preference for your solution. > > > > > The following 3 patches are a proof-of-concept on my platform, consisting > > > of: Patch 1 is the proposed bindings and straight forward. > > > Patch 2 is the current implementation > > > Patch 3 is an actual usage example for specifying the delays > > > > > > TODO: > > > 1. Adding ramp-down delays (Just the inverse copy of ramp-up delay) > > > 2. Should these delays take active low flags into account? > > > > How so ? > > Given the name ramp-up (& ramp-down) I would assume they affect the voltage > low -> high change (resp. high -> low), not just gpiod_set_value(..., 1). Good point. They should indeed. > > > 3. How to deal with setting multiple GPIOs at once? > > > > > > I skipped 1. for now, because this is just a copy with ramp-up being > > > replaced with ramp-down. > > > > > > I'm not that well versed in gpiolib code, so I'm not sure if I got all > > > placed where GPIOs are set. So patch 2 might be incomplete. > > > > > > For now I skipped setting multiple GPIOs at once completely, so to get some > > > feedback on this approach. A possible solution is to check for the bigest > > > delay in the set and use that for all afterwards. But I'm not sure about > > > the overhead in this case. > > > > I assume you're talking about the gpiod_set_array_value() API. That > > sounds OK as an initial implementation, a caller of that function needs > > to be prepared for the GPIOs being set in a random order due to hardware > > delays, so it shouldn't break the API contract. I would however state > > this explicitly in the function documentation. > > Okay, that seems sensible. Will do it. > > > > I hope there is some feedback. While thinking about this issue appears to > > > be more widespread than I expected. > > > > > > Best regards, > > > Alexander > > > > > > [1] https://lore.kernel.org/all/20221209083339.3780776-1-alexander.stein@ew.tq-group.com/ > > > > > > Alexander Stein (3): > > > dt-bindings: gpio: Add optional ramp-up delay property > > > gpiolib: Add support for optional ramp-up delays > > > arm64: dts: mba8mx: Add GPIO ramp-up delays > > > > > > .../devicetree/bindings/gpio/gpio.txt | 22 +++++ > > > arch/arm64/boot/dts/freescale/mba8mx.dtsi | 5 ++ > > > drivers/gpio/gpiolib.c | 80 +++++++++++++++++++ > > > drivers/gpio/gpiolib.h | 3 + > > > 4 files changed, 110 insertions(+)
On Mon, Dec 12, 2022 at 4:35 AM Alexander Stein <alexander.stein@ew.tq-group.com> wrote: > > Hi all, > > this series is an RFC for a general approach to solve the issue at [1]. While > a device specific property works as well, a more generic approach is preferred. > In short: When enabling a GPIO the actual ramp-up time might be (much) bigger > than what software usually assume, in my case >100ms. Adding a delay to each > driver is cumbersome. At least for DT, I think this belongs (if at all) in the consumers, rather than a producer property. The options there are 'foo-gpios-ramp-us' for 'foo-gpios' or add some delay bits to GPIO flags. We already have some of the former for various 'generic' power sequencing related delays. Of course, there's no real pattern to them as they all get added as we go without much foresight. In this case even, there are 4 possible delays: pre and post ramp up and down. > Instead the (optional) ramp-up delay is added to each gpio_desc. The delays can > be specified per gpio-controller, similar to 'gpio-line-names'. Actually the > parsing code is almost a 1:1 copy of devprop_gpiochip_set_names(). Due to > (temporary) memory allocation, I opted for a separate function, there is code > duplication, but handling both properties in a single function seemed too > tedious, let alone the to be added ramp-down delays. > > This feature could also be added as a callback in gpio_chip, but the callbacks > have to be added to each driver then. I would prefer a single one-fits-all > implementation and another indirection in the GPIO call chain. > > Laurent suggest to add a GPIO delay node in DT. IMHO this increased the DT > complexity unnecessarily. But comments are welcome. > > The following 3 patches are a proof-of-concept on my platform, consisting of: > Patch 1 is the proposed bindings and straight forward. > Patch 2 is the current implementation > Patch 3 is an actual usage example for specifying the delays > > TODO: > 1. Adding ramp-down delays (Just the inverse copy of ramp-up delay) > 2. Should these delays take active low flags into account? > 3. How to deal with setting multiple GPIOs at once? > > I skipped 1. for now, because this is just a copy with ramp-up being replaced > with ramp-down. > > I'm not that well versed in gpiolib code, so I'm not sure if I got all placed > where GPIOs are set. So patch 2 might be incomplete. > > For now I skipped setting multiple GPIOs at once completely, so to get some > feedback on this approach. A possible solution is to check for the bigest delay > in the set and use that for all afterwards. But I'm not sure about the overhead > in this case. > > I hope there is some feedback. While thinking about this issue appears to be > more widespread than I expected. Many/most GPIO controllers can read the actual state of an output (IIRC, i.MX ctrlr can). Perhaps that capability could be used to delay until the state of the signal matches the set state. And you'd probably want to measure how long that took and then add some more time based on it. This of course gets into the electricals of at what levels a low or high state will register. If you can't read the state, then you would be stuck with some maximum timeout. Rob
Hi Laurent, Am Dienstag, 20. Dezember 2022, 00:01:23 CET schrieb Laurent Pinchart: > On Thu, Dec 15, 2022 at 11:56:57AM +0100, Linus Walleij wrote: > > Hi Laurent, > > > > thanks for the detailed brief! > > > > On Tue, Dec 13, 2022 at 12:45 PM Laurent Pinchart > > > > <laurent.pinchart@ideasonboard.com> wrote: > > > The circuit we're looking at is > > > > > > +----------+ +-----------+ > > > > > > | SoC | | VCC | > > > | > > > | | | _ | > > > | | | > > > | | | | | R | > > > | | | | > > > | | | |_| | > > > | > > > | [IOx|-----+-----|EN]--+ | > > > | > > > | | | | SN65DSI83 | > > > > > > +----------+ --- C +-----------+ > > > > > > --- > > > > > > - > > > > > > GND > > > > > > The IOx pin is an open-drain output, the board has a 470nF capacitor to > > > ground, and the SN65DSI83 has an internal pull-up off 200kΩ. This gives > > > an RC time constant of 94ms, far from being negligible. > > > > > > The delay is caused by the combination of the open-drain nature of the > > > output (an intrinsic property of the GPIO controller), the pull-up > > > resistor (an intrinsic property of the SN65DSI83) and the capacitor on > > > the line (a property of the board). DT is notoriously bad at modelling > > > this kind of setup. > > > > Yeah :/ > > > > It's not like we don't model discrete electronics, we do that a lot, > > but as you say, it is really hard to know where to draw the line > > in cases like this. > > > > > The alternative I proposed, adding a "GPIO delay" DT node to model this, > > > would also offer a centralized solution to the problem, but with > > > additional complexity both at probe time and runtime. > > > > I have a slight preference for this, as it will be very explicit in the > > device tree and we can just put all the code inside its own file and > > depend on GPIO_OF so other HW description systems do not > > need to include it. > > > > At the same time it feels a bit overengineered, so maybe just adding > > this delay as in the patch with some strings attached like comments > > and docs is yet the best. It feels like we need some more input to > > reach consensus. > > > > > The regulator delays model the intrinsic delays when enabling or > > > disabling a regulator, and they should stay. They address a different > > > problem. > > > > OK right. But someone not knowing exactly what they are doing > > will end up abusing the delay property on the delay line > > also for this delay. The risk of that is lesser with a separate > > delay box. > > That may be true, but I think we can also try to catch abuses in > reviews. I would be a bit sad if we made life more difficult (and less > efficient at runtime too) for legitimate users just because we are > worried about abuses. What is a legitimate user for you? Given the example in v2 of this series it's clear that this feature is an opt-in, both for the DT node as well as for specifying a delay. Another benefit of using a dedicated driver: It also automatically handles things like setting multiple GPIOs at once. > Another thing I've been thinking about is that we may not always want to > wait for the GPIO delay. Some consumers may not care when the GPIO line > reaches the desired state as long as it eventually does, or maybe they > need to perform multiple operations (such as enabling/disabling > regulators and/or clocks) and only need a synchronization point for a > group of operations. All that would be pretty hard to handle, and maybe > it's a problem we'll look at only when needed (and hopefully never). If you don't care about rising time, do not use gpio-delay for that GPIO, or just don't specify a ramp-up delay in the gpio-cells, aka setting to 0. The more complex synchronisation example you mentioned probably needs a similar dedicated driver for grouping those resources. Best regards, Alexander
On Tue, Jan 03, 2023 at 12:56:38PM +0100, Alexander Stein wrote: > Am Dienstag, 20. Dezember 2022, 00:01:23 CET schrieb Laurent Pinchart: > > On Thu, Dec 15, 2022 at 11:56:57AM +0100, Linus Walleij wrote: > > > On Tue, Dec 13, 2022 at 12:45 PM Laurent Pinchart wrote: > > > > The circuit we're looking at is > > > > > > > > +----------+ +-----------+ > > > > | SoC | | VCC | > > > > | | | _ | > > > > | | | | | R | > > > > | | | |_| | > > > > | | | | | > > > > | [IOx|-----+-----|EN]--+ | > > > > | | | | SN65DSI83 | > > > > +----------+ --- C +-----------+ > > > > --- > > > > - > > > > GND > > > > > > > > The IOx pin is an open-drain output, the board has a 470nF capacitor to > > > > ground, and the SN65DSI83 has an internal pull-up off 200kΩ. This gives > > > > an RC time constant of 94ms, far from being negligible. > > > > > > > > The delay is caused by the combination of the open-drain nature of the > > > > output (an intrinsic property of the GPIO controller), the pull-up > > > > resistor (an intrinsic property of the SN65DSI83) and the capacitor on > > > > the line (a property of the board). DT is notoriously bad at modelling > > > > this kind of setup. > > > > > > Yeah :/ > > > > > > It's not like we don't model discrete electronics, we do that a lot, > > > but as you say, it is really hard to know where to draw the line > > > in cases like this. > > > > > > > The alternative I proposed, adding a "GPIO delay" DT node to model this, > > > > would also offer a centralized solution to the problem, but with > > > > additional complexity both at probe time and runtime. > > > > > > I have a slight preference for this, as it will be very explicit in the > > > device tree and we can just put all the code inside its own file and > > > depend on GPIO_OF so other HW description systems do not > > > need to include it. > > > > > > At the same time it feels a bit overengineered, so maybe just adding > > > this delay as in the patch with some strings attached like comments > > > and docs is yet the best. It feels like we need some more input to > > > reach consensus. > > > > > > > The regulator delays model the intrinsic delays when enabling or > > > > disabling a regulator, and they should stay. They address a different > > > > problem. > > > > > > OK right. But someone not knowing exactly what they are doing > > > will end up abusing the delay property on the delay line > > > also for this delay. The risk of that is lesser with a separate > > > delay box. > > > > That may be true, but I think we can also try to catch abuses in > > reviews. I would be a bit sad if we made life more difficult (and less > > efficient at runtime too) for legitimate users just because we are > > worried about abuses. > > What is a legitimate user for you? Given the example in v2 of this series it's > clear that this feature is an opt-in, both for the DT node as well as for > specifying a delay. > Another benefit of using a dedicated driver: It also automatically handles > things like setting multiple GPIOs at once. Your use case is totally legitimate I think. An illegitimate case would be someone modelling the internal enable delay of a regulator controlled by a GPIO as a GPIO delay instead of using the regulator enable delay DT property defined by the regulators bindings. > > Another thing I've been thinking about is that we may not always want to > > wait for the GPIO delay. Some consumers may not care when the GPIO line > > reaches the desired state as long as it eventually does, or maybe they > > need to perform multiple operations (such as enabling/disabling > > regulators and/or clocks) and only need a synchronization point for a > > group of operations. All that would be pretty hard to handle, and maybe > > it's a problem we'll look at only when needed (and hopefully never). > > If you don't care about rising time, do not use gpio-delay for that GPIO, or > just don't specify a ramp-up delay in the gpio-cells, aka setting to 0. > The more complex synchronisation example you mentioned probably needs a > similar dedicated driver for grouping those resources. You're right that in cases where there is a single consumer, and the consumer is well known, and the fact that it doesn't care about rising time is an intrinsic property of that consumer, then DT should simply not specify any delay. For more complex cases, I'd say it's likely overkill to try and design a solution now without real use cases.