| Message ID | 20221212103525.231298-1-alexander.stein@ew.tq-group.com |
|---|---|
| Headers | show |
| Series | gpiolib: ramp-up delay support | expand |
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. > 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 ? > 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. > 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(+)
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(+)
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 Rob, On Tue, Dec 13, 2022 at 08:20:57AM -0600, Rob Herring wrote: > On Mon, Dec 12, 2022 at 4:35 AM Alexander Stein 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. My main requirement is to handle these properties in a central place, without having to patch individual drivers, so this would work for me. > 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.