| Message ID | 20221212103525.231298-2-alexander.stein@ew.tq-group.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpiolib: ramp-up delay support | expand |
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 Linus, On Tue, Dec 13, 2022 at 10:08:19AM +0100, Linus Walleij wrote: > On Mon, Dec 12, 2022 at 11:35 AM Alexander Stein 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. The goal of this series is to cover the delay of the RC filter on the signal between the GPIO controller and the GPIO consumer. > Otherwise the ramp-up should certainly be on the consumer side. And that > seems very much like what is going on here. 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. Note that we would have a similar issue with a GPIO controller that would drive the signal actively and a consumer without an internal pull-up if the board had an RC filter on the line. Alexander started with a patch that added a ti,enable-delay-us property to the SN65DSI83 DT binding. I don't think that's a good idea, as the issue is far from being specific to the SN65DSI83, patching all GPIO consumers as needed to support signal propagation delays doesn't scale. Patching every GPIO controller driver would be equally bad. I've thus proposed modelling this with a standard property on the GPIO provider side, handled by gpiolib, to have a centralized solution. 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. > 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. The regulator delays model the intrinsic delays when enabling or disabling a regulator, and they should stay. They address a different problem. > 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.
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. Yours, Linus Walleij
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. 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).
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.
diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt b/Documentation/devicetree/bindings/gpio/gpio.txt index 5663e71b751fc..b87669dce9a61 100644 --- a/Documentation/devicetree/bindings/gpio/gpio.txt +++ b/Documentation/devicetree/bindings/gpio/gpio.txt @@ -182,6 +182,28 @@ gpio-controller@00000000 { "poweroff", "reset"; } +Optionally, a GPIO controller may have a "gpio-ramp-up-delays-us" property. +This is an array of u32 defining the ramp-up delays of the GPIO lines +going out of the GPIO controller. This might be necessary if external +components (e.g. capacitors, resistors) increase the ramp up time notably. +The delay are assigned starting from line offset 0 from +left to right from the passed array. An incomplete array (where the number +of passed delays are less than ngpios) will still be used up until the last +provided valid line index. Setting to will not add any delay. + +Example: + +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>; +} + The GPIO chip may contain GPIO hog definitions. GPIO hogging is a mechanism providing automatic GPIO request and configuration as part of the gpio-controller's driver probe function.
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> --- .../devicetree/bindings/gpio/gpio.txt | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+)