This adds a new TODO item for gpiolib and can also be used to start
a discussion about the need for it and implementation details.
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
drivers/gpio/TODO | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
diff --git a/drivers/gpio/TODO b/drivers/gpio/TODO
index f87ff3fa8a53..6ab39c5cec9d 100644
--- a/drivers/gpio/TODO
+++ b/drivers/gpio/TODO
@@ -197,3 +197,25 @@ A small number of drivers have been converted (pl061, tegra186, msm,
amd, apple), and can be used as examples of how to proceed with this
conversion. Note that drivers using the generic irqchip framework
cannot be converted yet, but watch this space!
+
+Safe-state of GPIOs
+
+During 2022 Linux Plumbers Conference's GPIO & pinctrl BOF it's been discussed
+that we don't have any middle ground between hogging GPIO lines and letting the
+user (either in-kernel or user-space) control them. Either the lines are forever
+reserved as hogs or their state is undefined unless requested.
+
+Currently the behavior of GPIOs that were not requested or were released is
+largely driver dependent (the provider driver decides whether the line's state
+is reverted to some predefined value or left as-is). This can be problematic
+as the output state of a line can damage physical hardware.
+
+This item is about proposing a solution, most likely in the form of a new device
+property called "safe-state" that would define the safe states of specific lines
+(e.g. output-high) but not block the line from being requested by users who
+could then modify that default state. Once released the GPIO core would then
+put the line back into the "safe-state".
+
+While this could potentially be seen as defining policy in the device-tree, it's
+not anymore a policy than properties such as "regulator-always-on" or gpio-hogs
+themselves are.
--
2.34.1
Hi Bartosz,
On Wed, Sep 14, 2022 at 4:11 PM Bartosz Golaszewski <[email protected]> wrote:
> This adds a new TODO item for gpiolib and can also be used to start
> a discussion about the need for it and implementation details.
>
> Signed-off-by: Bartosz Golaszewski <[email protected]>
> --- a/drivers/gpio/TODO
> +++ b/drivers/gpio/TODO
> +This item is about proposing a solution, most likely in the form of a new device
> +property called "safe-state" that would define the safe states of specific lines
> +(e.g. output-high) but not block the line from being requested by users who
> +could then modify that default state. Once released the GPIO core would then
> +put the line back into the "safe-state".
#bikeshedding
If this state is the "safe" state, would that imply that any other state is
"unsafe"? I guess not, as the idea is that a knowledgeable driver can
still change it (else a hog would be sufficient).
Hence I think "idle-state" would reflect this better. Any other thoughts?
Thanks!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Wed, Sep 14, 2022 at 5:20 PM Geert Uytterhoeven <[email protected]> wrote:
>
> Hi Bartosz,
>
> On Wed, Sep 14, 2022 at 4:11 PM Bartosz Golaszewski <[email protected]> wrote:
> > This adds a new TODO item for gpiolib and can also be used to start
> > a discussion about the need for it and implementation details.
> >
> > Signed-off-by: Bartosz Golaszewski <[email protected]>
>
> > --- a/drivers/gpio/TODO
> > +++ b/drivers/gpio/TODO
>
> > +This item is about proposing a solution, most likely in the form of a new device
> > +property called "safe-state" that would define the safe states of specific lines
> > +(e.g. output-high) but not block the line from being requested by users who
> > +could then modify that default state. Once released the GPIO core would then
> > +put the line back into the "safe-state".
>
> #bikeshedding
>
> If this state is the "safe" state, would that imply that any other state is
> "unsafe"? I guess not, as the idea is that a knowledgeable driver can
> still change it (else a hog would be sufficient).
> Hence I think "idle-state" would reflect this better. Any other thoughts?
>
No, you're right, you even mentioned the name "idle-state" during the
BoF session.
Bart
On Wed, Sep 14, 2022 at 6:21 PM Kent Gibson <[email protected]> wrote:
>
> On Wed, Sep 14, 2022 at 05:11:45PM +0200, Bartosz Golaszewski wrote:
> > This adds a new TODO item for gpiolib and can also be used to start
> > a discussion about the need for it and implementation details.
> >
> > Signed-off-by: Bartosz Golaszewski <[email protected]>
> > ---
> > drivers/gpio/TODO | 22 ++++++++++++++++++++++
> > 1 file changed, 22 insertions(+)
> >
> > diff --git a/drivers/gpio/TODO b/drivers/gpio/TODO
> > index f87ff3fa8a53..6ab39c5cec9d 100644
> > --- a/drivers/gpio/TODO
> > +++ b/drivers/gpio/TODO
> > @@ -197,3 +197,25 @@ A small number of drivers have been converted (pl061, tegra186, msm,
> > amd, apple), and can be used as examples of how to proceed with this
> > conversion. Note that drivers using the generic irqchip framework
> > cannot be converted yet, but watch this space!
> > +
> > +Safe-state of GPIOs
> > +
> > +During 2022 Linux Plumbers Conference's GPIO & pinctrl BOF it's been discussed
> > +that we don't have any middle ground between hogging GPIO lines and letting the
> > +user (either in-kernel or user-space) control them. Either the lines are forever
> > +reserved as hogs or their state is undefined unless requested.
> > +
> > +Currently the behavior of GPIOs that were not requested or were released is
> > +largely driver dependent (the provider driver decides whether the line's state
> > +is reverted to some predefined value or left as-is). This can be problematic
> > +as the output state of a line can damage physical hardware.
> > +
> > +This item is about proposing a solution, most likely in the form of a new device
> > +property called "safe-state" that would define the safe states of specific lines
> > +(e.g. output-high) but not block the line from being requested by users who
> > +could then modify that default state. Once released the GPIO core would then
> > +put the line back into the "safe-state".
> > +
>
> Geert suggests idle-state, rather than safe-state, but you call it
> the "default state" here as well - pick one.
>
idle-state it is then.
> So this idle-state would be another attribute on a line that the user
> could configure via the GPIO uAPI, and so replicate the "set and forget"
> sysfs behavior that we are currently missing, and which seems to be the
> biggest sticking point for a transition away from sysfs?
>
No, this should only be defined on the device tree or in ACPI. As the
HW policy of a device. I don't think we should allow user-space to
override this behavior.
> For backward compatibility the default idle-state, i.e. the value the
> idle-state would take if not explicitly set, would map to existing
> behaviour, so let the driver decide?
>
> What happens when gpiolib frees the line? Isn't the driver still able
> to do what it likes to the line at that point, no matter what GPIO core
> has set it to previously? e.g. gpio_sim_free() restores the line to its
> own internal pull value.
>
This "idle-state" property wouldn't be mandatory and normally would
only be defined for a limited set of lines. I'd say we just override
whatever the driver does in free() (most drivers don't implement it
BTW) and do what the property says we should.
Bart
On Wed, Sep 14, 2022 at 05:11:45PM +0200, Bartosz Golaszewski wrote:
> This adds a new TODO item for gpiolib and can also be used to start
> a discussion about the need for it and implementation details.
>
> Signed-off-by: Bartosz Golaszewski <[email protected]>
> ---
> drivers/gpio/TODO | 22 ++++++++++++++++++++++
> 1 file changed, 22 insertions(+)
>
> diff --git a/drivers/gpio/TODO b/drivers/gpio/TODO
> index f87ff3fa8a53..6ab39c5cec9d 100644
> --- a/drivers/gpio/TODO
> +++ b/drivers/gpio/TODO
> @@ -197,3 +197,25 @@ A small number of drivers have been converted (pl061, tegra186, msm,
> amd, apple), and can be used as examples of how to proceed with this
> conversion. Note that drivers using the generic irqchip framework
> cannot be converted yet, but watch this space!
> +
> +Safe-state of GPIOs
> +
> +During 2022 Linux Plumbers Conference's GPIO & pinctrl BOF it's been discussed
> +that we don't have any middle ground between hogging GPIO lines and letting the
> +user (either in-kernel or user-space) control them. Either the lines are forever
> +reserved as hogs or their state is undefined unless requested.
> +
> +Currently the behavior of GPIOs that were not requested or were released is
> +largely driver dependent (the provider driver decides whether the line's state
> +is reverted to some predefined value or left as-is). This can be problematic
> +as the output state of a line can damage physical hardware.
> +
> +This item is about proposing a solution, most likely in the form of a new device
> +property called "safe-state" that would define the safe states of specific lines
> +(e.g. output-high) but not block the line from being requested by users who
> +could then modify that default state. Once released the GPIO core would then
> +put the line back into the "safe-state".
> +
Geert suggests idle-state, rather than safe-state, but you call it
the "default state" here as well - pick one.
So this idle-state would be another attribute on a line that the user
could configure via the GPIO uAPI, and so replicate the "set and forget"
sysfs behavior that we are currently missing, and which seems to be the
biggest sticking point for a transition away from sysfs?
For backward compatibility the default idle-state, i.e. the value the
idle-state would take if not explicitly set, would map to existing
behaviour, so let the driver decide?
What happens when gpiolib frees the line? Isn't the driver still able
to do what it likes to the line at that point, no matter what GPIO core
has set it to previously? e.g. gpio_sim_free() restores the line to its
own internal pull value.
Cheers,
Kent.
On Wed, Sep 14, 2022 at 06:25:21PM +0200, Bartosz Golaszewski wrote:
> On Wed, Sep 14, 2022 at 6:21 PM Kent Gibson <[email protected]> wrote:
> >
> > On Wed, Sep 14, 2022 at 05:11:45PM +0200, Bartosz Golaszewski wrote:
> > > This adds a new TODO item for gpiolib and can also be used to start
> > > a discussion about the need for it and implementation details.
> > >
> > > Signed-off-by: Bartosz Golaszewski <[email protected]>
> > > ---
> > > drivers/gpio/TODO | 22 ++++++++++++++++++++++
> > > 1 file changed, 22 insertions(+)
> > >
> > > diff --git a/drivers/gpio/TODO b/drivers/gpio/TODO
> > > index f87ff3fa8a53..6ab39c5cec9d 100644
> > > --- a/drivers/gpio/TODO
> > > +++ b/drivers/gpio/TODO
> > > @@ -197,3 +197,25 @@ A small number of drivers have been converted (pl061, tegra186, msm,
> > > amd, apple), and can be used as examples of how to proceed with this
> > > conversion. Note that drivers using the generic irqchip framework
> > > cannot be converted yet, but watch this space!
> > > +
> > > +Safe-state of GPIOs
> > > +
> > > +During 2022 Linux Plumbers Conference's GPIO & pinctrl BOF it's been discussed
> > > +that we don't have any middle ground between hogging GPIO lines and letting the
> > > +user (either in-kernel or user-space) control them. Either the lines are forever
> > > +reserved as hogs or their state is undefined unless requested.
> > > +
> > > +Currently the behavior of GPIOs that were not requested or were released is
> > > +largely driver dependent (the provider driver decides whether the line's state
> > > +is reverted to some predefined value or left as-is). This can be problematic
> > > +as the output state of a line can damage physical hardware.
> > > +
> > > +This item is about proposing a solution, most likely in the form of a new device
> > > +property called "safe-state" that would define the safe states of specific lines
> > > +(e.g. output-high) but not block the line from being requested by users who
> > > +could then modify that default state. Once released the GPIO core would then
> > > +put the line back into the "safe-state".
> > > +
> >
> > Geert suggests idle-state, rather than safe-state, but you call it
> > the "default state" here as well - pick one.
> >
>
> idle-state it is then.
>
> > So this idle-state would be another attribute on a line that the user
> > could configure via the GPIO uAPI, and so replicate the "set and forget"
> > sysfs behavior that we are currently missing, and which seems to be the
> > biggest sticking point for a transition away from sysfs?
> >
>
> No, this should only be defined on the device tree or in ACPI. As the
> HW policy of a device. I don't think we should allow user-space to
> override this behavior.
>
Oh, ok - from the item I got the impression you did want to be able to
control it from user-space.
> > For backward compatibility the default idle-state, i.e. the value the
> > idle-state would take if not explicitly set, would map to existing
> > behaviour, so let the driver decide?
> >
> > What happens when gpiolib frees the line? Isn't the driver still able
> > to do what it likes to the line at that point, no matter what GPIO core
> > has set it to previously? e.g. gpio_sim_free() restores the line to its
> > own internal pull value.
> >
>
> This "idle-state" property wouldn't be mandatory and normally would
> only be defined for a limited set of lines. I'd say we just override
> whatever the driver does in free() (most drivers don't implement it
> BTW) and do what the property says we should.
>
Not sure what "override" involves.
You call the driver to set the value after calling the free()?
Cheers,
Kent.
> You call the driver to set the value after calling the free()?
>On Wed, Sep 14, 2022 at 6:42 PM Kent Gibson <[email protected]> wrote:
>
> On Wed, Sep 14, 2022 at 06:25:21PM +0200, Bartosz Golaszewski wrote:
> > On Wed, Sep 14, 2022 at 6:21 PM Kent Gibson <[email protected]> wrote:
> > >
> > > On Wed, Sep 14, 2022 at 05:11:45PM +0200, Bartosz Golaszewski wrote:
> > > > This adds a new TODO item for gpiolib and can also be used to start
> > > > a discussion about the need for it and implementation details.
> > > >
> > > > Signed-off-by: Bartosz Golaszewski <[email protected]>
> > > > ---
> > > > drivers/gpio/TODO | 22 ++++++++++++++++++++++
> > > > 1 file changed, 22 insertions(+)
> > > >
> > > > diff --git a/drivers/gpio/TODO b/drivers/gpio/TODO
> > > > index f87ff3fa8a53..6ab39c5cec9d 100644
> > > > --- a/drivers/gpio/TODO
> > > > +++ b/drivers/gpio/TODO
> > > > @@ -197,3 +197,25 @@ A small number of drivers have been converted (pl061, tegra186, msm,
> > > > amd, apple), and can be used as examples of how to proceed with this
> > > > conversion. Note that drivers using the generic irqchip framework
> > > > cannot be converted yet, but watch this space!
> > > > +
> > > > +Safe-state of GPIOs
> > > > +
> > > > +During 2022 Linux Plumbers Conference's GPIO & pinctrl BOF it's been discussed
> > > > +that we don't have any middle ground between hogging GPIO lines and letting the
> > > > +user (either in-kernel or user-space) control them. Either the lines are forever
> > > > +reserved as hogs or their state is undefined unless requested.
> > > > +
> > > > +Currently the behavior of GPIOs that were not requested or were released is
> > > > +largely driver dependent (the provider driver decides whether the line's state
> > > > +is reverted to some predefined value or left as-is). This can be problematic
> > > > +as the output state of a line can damage physical hardware.
> > > > +
> > > > +This item is about proposing a solution, most likely in the form of a new device
> > > > +property called "safe-state" that would define the safe states of specific lines
> > > > +(e.g. output-high) but not block the line from being requested by users who
> > > > +could then modify that default state. Once released the GPIO core would then
> > > > +put the line back into the "safe-state".
> > > > +
> > >
> > > Geert suggests idle-state, rather than safe-state, but you call it
> > > the "default state" here as well - pick one.
> > >
> >
> > idle-state it is then.
> >
> > > So this idle-state would be another attribute on a line that the user
> > > could configure via the GPIO uAPI, and so replicate the "set and forget"
> > > sysfs behavior that we are currently missing, and which seems to be the
> > > biggest sticking point for a transition away from sysfs?
> > >
> >
> > No, this should only be defined on the device tree or in ACPI. As the
> > HW policy of a device. I don't think we should allow user-space to
> > override this behavior.
> >
>
> Oh, ok - from the item I got the impression you did want to be able to
> control it from user-space.
>
Oh god no, then it would be sysfs all over again.
> > > For backward compatibility the default idle-state, i.e. the value the
> > > idle-state would take if not explicitly set, would map to existing
> > > behaviour, so let the driver decide?
> > >
> > > What happens when gpiolib frees the line? Isn't the driver still able
> > > to do what it likes to the line at that point, no matter what GPIO core
> > > has set it to previously? e.g. gpio_sim_free() restores the line to its
> > > own internal pull value.
> > >
> >
> > This "idle-state" property wouldn't be mandatory and normally would
> > only be defined for a limited set of lines. I'd say we just override
> > whatever the driver does in free() (most drivers don't implement it
> > BTW) and do what the property says we should.
> >
>
> Not sure what "override" involves.
> You call the driver to set the value after calling the free()?
>
Yes. We call the driver's free() callback and it does something in it
(potentially set some predefined state). We see that this line has the
idle-state property defined so we call into the driver and set the
value as defined by idle-state.
Bartosz
On Wed, Sep 14, 2022 at 5:11 PM Bartosz Golaszewski <[email protected]> wrote:
> This adds a new TODO item for gpiolib and can also be used to start
> a discussion about the need for it and implementation details.
>
> Signed-off-by: Bartosz Golaszewski <[email protected]>
Acked-by: Linus Walleij <[email protected]>
Several approaches have been tried in the past. Consider adding
these lore links:
"[PATCH 3/4] gpio: dt-bindings: Declare gpio-hog optional for GPIO
subnodes" (2016)
https://lore.kernel.org/linux-devicetree/[email protected]/
"[PATCH] gpio: 74x164: add lines-initial-states property" (2018)
https://lore.kernel.org/lkml/[email protected]/"
"[RFC] Initial state for GPIOs" (2019)
https://lore.kernel.org/linux-gpio/[email protected]/
Just so we don't have to constantly look them up for referral.
Yours,
Linus Walleij
On Thu, Sep 15, 2022 at 10:59 AM Linus Walleij <[email protected]> wrote:
>
> On Wed, Sep 14, 2022 at 5:11 PM Bartosz Golaszewski <[email protected]> wrote:
>
> > This adds a new TODO item for gpiolib and can also be used to start
> > a discussion about the need for it and implementation details.
> >
> > Signed-off-by: Bartosz Golaszewski <[email protected]>
>
> Acked-by: Linus Walleij <[email protected]>
>
> Several approaches have been tried in the past. Consider adding
> these lore links:
>
> "[PATCH 3/4] gpio: dt-bindings: Declare gpio-hog optional for GPIO
> subnodes" (2016)
> https://lore.kernel.org/linux-devicetree/[email protected]/
> "[PATCH] gpio: 74x164: add lines-initial-states property" (2018)
> https://lore.kernel.org/lkml/[email protected]/"
> "[RFC] Initial state for GPIOs" (2019)
> https://lore.kernel.org/linux-gpio/[email protected]/
>
> Just so we don't have to constantly look them up for referral.
>
Of course we were not the first ones to think about it...
I'll read through it later. Was there any particular reason why this
never made its way into the kernel?
Bart
On Fri, Sep 16, 2022 at 9:11 AM Bartosz Golaszewski <[email protected]> wrote:
> Of course we were not the first ones to think about it...
:D
> I'll read through it later. Was there any particular reason why this
> never made its way into the kernel?
Inability to drive through a DT binding that was acceptable for the
DT binding maintainers.
A good idea to get it passed I think would be to ask Rob (with some
examples) how he thinks it should look and finalize the bindings
before coding.
Yours,
Linus Walleij
On Fri, Sep 16, 2022 at 8:12 AM Linus Walleij <[email protected]> wrote:
>
> On Fri, Sep 16, 2022 at 9:11 AM Bartosz Golaszewski <[email protected]> wrote:
>
> > Of course we were not the first ones to think about it...
>
> :D
>
> > I'll read through it later. Was there any particular reason why this
> > never made its way into the kernel?
>
> Inability to drive through a DT binding that was acceptable for the
> DT binding maintainers.
AFAICT, this came up briefly in 2015, then 2017, then 2019. (You're a
year late this time.) A nice regular pattern to not get something
upstream...
> A good idea to get it passed I think would be to ask Rob (with some
> examples) how he thinks it should look and finalize the bindings
> before coding.
My issue was more that by the time the kernel or even bootloader runs,
quite a bit of time has passed given all the firmware that runs
nowadays. Doesn't a safe state need to be set as early as possible?
Like probably before anything using DT?
A node per GPIO could end up being a lot of nodes and I can certainly
see folks just initializing every GPIO. That would be a lot of bloat.
As I see it we need 4 bits per line: direction, state(high/low), pull
up/down/none.
Finally, don't non-GPIO pins need the same thing? You don't want a
default output driving what needs to be an input. Of course, a good
h/w designer wouldn't design such a thing.
Rob
On Fri, Sep 16, 2022 at 3:47 PM Rob Herring <[email protected]> wrote:
> > Inability to drive through a DT binding that was acceptable for the
> > DT binding maintainers.
>
> AFAICT, this came up briefly in 2015, then 2017, then 2019. (You're a
> year late this time.) A nice regular pattern to not get something
> upstream...
:D
> > A good idea to get it passed I think would be to ask Rob (with some
> > examples) how he thinks it should look and finalize the bindings
> > before coding.
>
> My issue was more that by the time the kernel or even bootloader runs,
> quite a bit of time has passed given all the firmware that runs
> nowadays. Doesn't a safe state need to be set as early as possible?
> Like probably before anything using DT?
So that is less related to the bindings and more related to the
implementation, right?
I think this has always been seen as "default state at the point
when the operating system boots" but possibly we should just add
a flag or something to the bindings to say how early-ish the
particular default safe state config applies?
> A node per GPIO could end up being a lot of nodes and I can certainly
> see folks just initializing every GPIO. That would be a lot of bloat.
> As I see it we need 4 bits per line: direction, state(high/low), pull
> up/down/none.
I don't know exactly which use cases people have here, but yeah
definitely biasing for example, so a certain line can go out of reset
thanks to getting pulled up.
I imagined something like a ngpios long array:
/* Initial states */
gpio-init-states = <GPIO_OUT_LOW, GPIO_OUT_HIGH,
GPIO_NO_CHANGE, GPIO_NO_CHANGE, GPIO_OUT_HIGH ...>;
these defines does not exist in include/dt-bindings/gpio/gpio.h,
it's just my idea of it.
> Finally, don't non-GPIO pins need the same thing? You don't want a
> default output driving what needs to be an input.
The pin control subsystem has something like this, in the "default"
and "init" states the pins are set to a default mux/pin config.
So for SoC GPIOs in many cases this problem is already solved
by using pin control states and hogs, as pin control is often used
as a back-end for GPIO. e.g. all Qualcomm platforms TLMM.
GPIO lines however can also be on a I2C or USB expander or
something where pin control is not applicable at all, so this would be
for initializing lines on those controllers.
Yours,
Linus Walleij