| Message ID | 20221214095342.937303-1-alexander.stein@ew.tq-group.com |
|---|---|
| Headers | show |
| Series | gpio: Add gpio-delay support | expand |
On 14/12/2022 10:53, Alexander Stein wrote: > This adds bindings for a GPIO enable/disable delay driver. > > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> > --- > .../devicetree/bindings/gpio/gpio-delay.yaml | 75 +++++++++++++++++++ > 1 file changed, 75 insertions(+) > create mode 100644 Documentation/devicetree/bindings/gpio/gpio-delay.yaml > > diff --git a/Documentation/devicetree/bindings/gpio/gpio-delay.yaml b/Documentation/devicetree/bindings/gpio/gpio-delay.yaml > new file mode 100644 > index 000000000000..20871356e9b5 > --- /dev/null > +++ b/Documentation/devicetree/bindings/gpio/gpio-delay.yaml > @@ -0,0 +1,75 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/gpio/gpio-delay.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: GPIO delay controller > + > +maintainers: > + - Alexander Stein <linux@ew.tq-group.com> > + > +description: | > + This binding describes an electrical setup where setting an GPIO output > + is delayed by some external setup, e.g. RC curcuit. > + > + +----------+ +-----------+ > + | | VCC_B | | > + | | | | | > + | | VCC_A _ | | > + | GPIO | | | R | Consumer | > + |controller| ___ |_| | | > + | | | | | | | > + | [IOx|-------| |--+-----|-----+ | > + | | |___| | | input | > + | | | | | > + +----------+ --- C +-----------+ > + --- > + | > + - > + GND > + > + If the input on the consumer is controlled by an open-drain signal If IOx is open-drain, what is the VCC_A on the diagram? I think it wasn't present in original Laurent's diagram. > + attached to an RC curcuit the ramp-up delay is not under control > + of the GPIO controller. > + > +properties: > + compatible: > + const: gpio-delay > + > + "#gpio-cells": > + description: | > + Specifies the pin, ramp-up and ramp-down delays. The > + delays are specified in microseconds. > + const: 3 > + > + input-gpios: > + description: Array of GPIOs which output signal change is delayed maxItems: 32 or some other reasonable value > + > + gpio-controller: true > + > + gpio-line-names: true and then the same maxItems. > + > +required: > + - compatible > + - "#gpio-cells" > + - gpio-controller > + - input-gpios > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/gpio/gpio.h> > + > + enable_delay: enable-delay { > + compatible = "gpio-delay"; I am not sure whether the naming is the most accurate - it represents desired behavior (so the delay in rising signal), not actual hardware (RC filter), but maybe that's a bit more generic. Anyway look fine for me. > + #gpio-cells = <3>; > + gpio-controller; > + input-gpios = <&gpio0 3 GPIO_ACTIVE_LOW>, > + <&gpio3 1 GPIO_ACTIVE_HIGH>; > + }; > + > + consumer { > + enable-gpios = <&enable_delay 0 130000 30000>; > + }; Best regards, Krzysztof
Hi Krzysztof, Am Donnerstag, 15. Dezember 2022, 10:11:47 CET schrieb Krzysztof Kozlowski: > On 14/12/2022 10:53, Alexander Stein wrote: > > This adds bindings for a GPIO enable/disable delay driver. > > > > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> > > --- > > > > .../devicetree/bindings/gpio/gpio-delay.yaml | 75 +++++++++++++++++++ > > 1 file changed, 75 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/gpio/gpio-delay.yaml > > > > diff --git a/Documentation/devicetree/bindings/gpio/gpio-delay.yaml > > b/Documentation/devicetree/bindings/gpio/gpio-delay.yaml new file mode > > 100644 > > index 000000000000..20871356e9b5 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/gpio/gpio-delay.yaml > > @@ -0,0 +1,75 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/gpio/gpio-delay.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: GPIO delay controller > > + > > +maintainers: > > + - Alexander Stein <linux@ew.tq-group.com> > > + > > +description: | > > + This binding describes an electrical setup where setting an GPIO output > > + is delayed by some external setup, e.g. RC curcuit. > > + > > + +----------+ +-----------+ > > + | | VCC_B | | > > + | | | | | > > + | | VCC_A _ | | > > + | GPIO | | | R | Consumer | > > + |controller| ___ |_| | | > > + | | | | | | | > > + | [IOx|-------| |--+-----|-----+ | > > + | | |___| | | input | > > + | | | | | > > + +----------+ --- C +-----------+ > > + --- > > + | > > + - > > + GND > > + > > + If the input on the consumer is controlled by an open-drain signal > > If IOx is open-drain, what is the VCC_A on the diagram? I think it > wasn't present in original Laurent's diagram. I have to admit my artistic skills are lacking :( I wanted to highlight that the actual GPIO output IOx is not (necessarily) the open-drain. This is somewhat important, because this can not be solved by just reconfiguring the GPIO to push-pull. Instead there is a buffer (small box in the middle) which (in this case) converts from VCC_A on the left side connected to SoC to VCC_B on the right connected to consumer using an open-drain. So this delay is induced passively by external circuits the SoC can not do anything about. Best regards, Alexander > > + attached to an RC curcuit the ramp-up delay is not under control > > + of the GPIO controller. > > + > > +properties: > > + compatible: > > + const: gpio-delay > > + > > + "#gpio-cells": > > + description: | > > + Specifies the pin, ramp-up and ramp-down delays. The > > + delays are specified in microseconds. > > + const: 3 > > + > > + input-gpios: > > + description: Array of GPIOs which output signal change is delayed > > maxItems: 32 or some other reasonable value Okay. Apparently there is no limit within gpiolib, so I was not limiting the amount unnecessarily. > > + > > + gpio-controller: true > > + > > + gpio-line-names: true > > and then the same maxItems. Sure, will adjust as well. > > + > > +required: > > + - compatible > > + - "#gpio-cells" > > + - gpio-controller > > + - input-gpios > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + #include <dt-bindings/gpio/gpio.h> > > + > > + enable_delay: enable-delay { > > + compatible = "gpio-delay"; > > I am not sure whether the naming is the most accurate - it represents > desired behavior (so the delay in rising signal), not actual hardware > (RC filter), but maybe that's a bit more generic. > > Anyway look fine for me. IMHO delay fits pretty well, because it's the behaviour. I'm no hardware developer, but I assume that there are more possibilities than just RC filter which might require this delay. Best regards, Alexander > > + #gpio-cells = <3>; > > + gpio-controller; > > + input-gpios = <&gpio0 3 GPIO_ACTIVE_LOW>, > > + <&gpio3 1 GPIO_ACTIVE_HIGH>; > > + }; > > + > > + consumer { > > + enable-gpios = <&enable_delay 0 130000 30000>; > > + }; > > Best regards, > Krzysztof
On Wed, Dec 14, 2022 at 10:53 AM Alexander Stein <alexander.stein@ew.tq-group.com> wrote: > This adds bindings for a GPIO enable/disable delay driver. > > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> > + input-gpios: > + description: Array of GPIOs which output signal change is delayed I would just call this "gpios", it's not like we're ever going to add any more specified GPIOs to this hardware, ever. Yours, Linus Walleij
On Wed, Dec 14, 2022 at 10:53 AM Alexander Stein <alexander.stein@ew.tq-group.com> wrote: > thanks for the feedback I've received. This is the reworked RFC for > adressing a platform specific ramp-up/ramp-down delay on GPIO outputs. > Now the delays are neither specified as gpio-controller nor > consumer-specific properties. > > v2 is a different approach than v1 in that it adds a new driver which will > simply forward setting the GPIO output of specified GPIOs in OF node. > The ramp-up/ramp-down delay can now be actually defined on consumer side, > see Patch 1 or 3 for examples. I really like this approach, it looks better than I imagined. > Thanks a lot to the existing gpio-latch driver where I could learn/copy > from a lot for creating this driver! Yeah that one is really neat, and also kind of sets a standard for how we should do these things. This patch set overall: Acked-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Thu, Dec 15, 2022 at 7:16 AM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Wed, Dec 14, 2022 at 10:53 AM Alexander Stein > <alexander.stein@ew.tq-group.com> wrote: > > > thanks for the feedback I've received. This is the reworked RFC for > > adressing a platform specific ramp-up/ramp-down delay on GPIO outputs. > > Now the delays are neither specified as gpio-controller nor > > consumer-specific properties. > > > > v2 is a different approach than v1 in that it adds a new driver which will > > simply forward setting the GPIO output of specified GPIOs in OF node. > > The ramp-up/ramp-down delay can now be actually defined on consumer side, > > see Patch 1 or 3 for examples. > > I really like this approach, it looks better than I imagined. It seems over-engineered to me. So far no comments on my 3 suggestions either... One is to just use some GPIO flag bits. Say 4-bits of GPIO flags encoded as power of 2 ramp delay. We have to pick the units. For example, 100us*2^N, which gives you 200us-3.2s of delay. Anything less is short enough to just hard code in a driver. Rob
On Thu, Dec 15, 2022 at 12:21:33PM -0600, Rob Herring wrote: > On Thu, Dec 15, 2022 at 7:16 AM Linus Walleij wrote: > > On Wed, Dec 14, 2022 at 10:53 AM Alexander Stein wrote: > > > > > thanks for the feedback I've received. This is the reworked RFC for > > > adressing a platform specific ramp-up/ramp-down delay on GPIO outputs. > > > Now the delays are neither specified as gpio-controller nor > > > consumer-specific properties. > > > > > > v2 is a different approach than v1 in that it adds a new driver which will > > > simply forward setting the GPIO output of specified GPIOs in OF node. > > > The ramp-up/ramp-down delay can now be actually defined on consumer side, > > > see Patch 1 or 3 for examples. > > > > I really like this approach, it looks better than I imagined. > > It seems over-engineered to me. So far no comments on my 3 suggestions either... I like the idea of handling this on the consumer's side, possibly with standard foo-gpios-ramp-{up,down}-delay-us (name to be bikeshedded) properties as you mentioned in the review of v1. > One is to just use some GPIO flag bits. Say 4-bits of GPIO flags > encoded as power of 2 ramp delay. We have to pick the units. For > example, 100us*2^N, which gives you 200us-3.2s of delay. This could probably work too. > Anything less is short enough to just hard code in a driver. In which driver though ? The whole point is that we should avoid handling this in particular drivers.
On Thu, Dec 15, 2022 at 3:26 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > On Thu, Dec 15, 2022 at 12:21:33PM -0600, Rob Herring wrote: > > On Thu, Dec 15, 2022 at 7:16 AM Linus Walleij wrote: > > > On Wed, Dec 14, 2022 at 10:53 AM Alexander Stein wrote: > > > > > > > thanks for the feedback I've received. This is the reworked RFC for > > > > adressing a platform specific ramp-up/ramp-down delay on GPIO outputs. > > > > Now the delays are neither specified as gpio-controller nor > > > > consumer-specific properties. > > > > > > > > v2 is a different approach than v1 in that it adds a new driver which will > > > > simply forward setting the GPIO output of specified GPIOs in OF node. > > > > The ramp-up/ramp-down delay can now be actually defined on consumer side, > > > > see Patch 1 or 3 for examples. > > > > > > I really like this approach, it looks better than I imagined. > > > > It seems over-engineered to me. So far no comments on my 3 suggestions either... > > I like the idea of handling this on the consumer's side, possibly with > standard foo-gpios-ramp-{up,down}-delay-us (name to be bikeshedded) > properties as you mentioned in the review of v1. > > > One is to just use some GPIO flag bits. Say 4-bits of GPIO flags > > encoded as power of 2 ramp delay. We have to pick the units. For > > example, 100us*2^N, which gives you 200us-3.2s of delay. > > This could probably work too. > > > Anything less is short enough to just hard code in a driver. > > In which driver though ? The whole point is that we should avoid > handling this in particular drivers. Okay, make the range 100us-1.63s and the minimum delay is 100us. Or 50us-819ms? What's a small enough minimum that no one will care about the extra delay? One thing we don't want is DT authors putting a device's delay needs in here. Then we'll get coupling to the OS implementation or double delays. Something like this should be clear: #define GPIO_THIS_IS_ONLY_THE_SIGNAL_RC_RAMP_TIME_100us ;) Rob
Hi all, thanks for your comments. Am Donnerstag, 15. Dezember 2022, 23:44:19 CET schrieb Rob Herring: > On Thu, Dec 15, 2022 at 3:26 PM Laurent Pinchart > > <laurent.pinchart@ideasonboard.com> wrote: > > On Thu, Dec 15, 2022 at 12:21:33PM -0600, Rob Herring wrote: > > > On Thu, Dec 15, 2022 at 7:16 AM Linus Walleij wrote: > > > > On Wed, Dec 14, 2022 at 10:53 AM Alexander Stein wrote: > > > > > thanks for the feedback I've received. This is the reworked RFC for > > > > > adressing a platform specific ramp-up/ramp-down delay on GPIO > > > > > outputs. > > > > > Now the delays are neither specified as gpio-controller nor > > > > > consumer-specific properties. > > > > > > > > > > v2 is a different approach than v1 in that it adds a new driver > > > > > which will > > > > > simply forward setting the GPIO output of specified GPIOs in OF > > > > > node. > > > > > The ramp-up/ramp-down delay can now be actually defined on consumer > > > > > side, > > > > > see Patch 1 or 3 for examples. > > > > > > > > I really like this approach, it looks better than I imagined. > > > > > > It seems over-engineered to me. So far no comments on my 3 suggestions > > > either...> > > I like the idea of handling this on the consumer's side, possibly with > > standard foo-gpios-ramp-{up,down}-delay-us (name to be bikeshedded) > > properties as you mentioned in the review of v1. Rob mentioned 4 possible delays: pre and post ramp up and down. Is there a need for a pre ramp delay, ever? If there is need to wait until a GPIO can be switched this seems highly device specific to me. Also reading back the requested output level on the GPIO is not possible in every case. Looking at the example in Patch 1 you can only read back the state of VCC_A, but the actual delay happens on VCC_B. It might seem over-engineered, but I'm getting more and more inclined to this v2 approach. Having an explicit delay node, its obvious there is some dedicated circuit inducing this delay. But it is not caused by the GPIO controller nor by the consumer (LVDS Bridge in this case), but something passive in between. Considering the hypothetical case there is a configurable IC instead, inducing this delay as well. The DT setup would look similar, but having a "regular" device instead of "gpio-delay" virtual device. > > > One is to just use some GPIO flag bits. Say 4-bits of GPIO flags > > > encoded as power of 2 ramp delay. We have to pick the units. For > > > example, 100us*2^N, which gives you 200us-3.2s of delay. > > > > This could probably work too. > > > > > Anything less is short enough to just hard code in a driver. > > > > In which driver though ? The whole point is that we should avoid > > handling this in particular drivers. > > Okay, make the range 100us-1.63s and the minimum delay is 100us. Or > 50us-819ms? What's a small enough minimum that no one will care about > the extra delay? Is there a definite answer to this at all? Realtime (RT_PREMPT) people might have a different answer to these ranges. But I'm not really fond of using a bitmask in GPIO flags. > One thing we don't want is DT authors putting a device's delay needs > in here. Then we'll get coupling to the OS implementation or double > delays. Can you actually avoid that? There is no difference of behavior in software if you have a) waiting/locking/... time once device is enabled, with an immediate ramp up b) ramp up time until device is enabled, but it can be used immediately In both cases you enable the GPIO and you have to wait for some specific time. But the reasoning for waiting are different. You can "solve" both cases on two ways: 1. device specific, configurable/hard-coded enable delays 2. general GPIO switch delays (this series) I'm not sure if a property 'foo-gpios-ramp-us' on the consumer side is prone to hide the fact this delay is not actually related to the consumer. Maybe it's even better to specify the delay in the "gpio-delay" consumer node. Resulting in an example like this: gpio_delay: gpio-delay { compatible = "gpio-delay"; #gpio-cells = <1>; gpio-controller; gpios = <&gpio0 3 GPIO_ACTIVE_LOW>, <&gpio3 1 GPIO_ACTIVE_HIGH>; gpios-ramp-us = <56000 0>, <130000 30000>; }; consumer { enable-gpios = <&gpio_delay 0>; }; Best regards, Alexander > Something like this should be clear: > > #define GPIO_THIS_IS_ONLY_THE_SIGNAL_RC_RAMP_TIME_100us > > ;) > > Rob