* [RFC PATCH 1/5] gpio: gpiolib: Add core support for maintaining GPIO values on reset
2017-10-20 3:37 [RFC PATCH 0/5] gpio: Expose reset tolerance capability Andrew Jeffery
@ 2017-10-20 3:37 ` Andrew Jeffery
2017-10-20 7:17 ` Linus Walleij
2017-10-20 3:37 ` [RFC PATCH 2/5] gpio: gpiolib: Add OF " Andrew Jeffery
` (3 subsequent siblings)
4 siblings, 1 reply; 22+ messages in thread
From: Andrew Jeffery @ 2017-10-20 3:37 UTC (permalink / raw)
To: linux-gpio
Cc: Andrew Jeffery, linus.walleij, corbet, joel, ryan_chen, robh+dt,
frowand.list, ckeepax, ldewangan, linux-doc, linux-kernel,
patches, devicetree, openbmc, linux-aspeed
GPIO state reset tolerance is implemented in gpiolib through the
addition of a new pinconf parameter. With that, some renaming of helpers
is done to clarify the scope of the already existing
gpiochip_line_is_persistent(), as it's now ambiguous as to whether that
means on suspend, reset or both. This in-turn impacts gpio-arizona, but
not in any complicated way.
This change lays the groundwork for implementing reset tolerance support
in all of the external interfaces that can influence GPIOs.
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
drivers/gpio/gpio-arizona.c | 4 +--
drivers/gpio/gpiolib.c | 55 +++++++++++++++++++++++++++++++--
drivers/gpio/gpiolib.h | 1 +
include/linux/gpio/consumer.h | 9 ++++++
include/linux/gpio/driver.h | 5 ++-
include/linux/gpio/machine.h | 2 ++
include/linux/pinctrl/pinconf-generic.h | 2 ++
7 files changed, 73 insertions(+), 5 deletions(-)
diff --git a/drivers/gpio/gpio-arizona.c b/drivers/gpio/gpio-arizona.c
index d4e6ba0301bc..d3fe23569811 100644
--- a/drivers/gpio/gpio-arizona.c
+++ b/drivers/gpio/gpio-arizona.c
@@ -33,7 +33,7 @@ static int arizona_gpio_direction_in(struct gpio_chip *chip, unsigned offset)
{
struct arizona_gpio *arizona_gpio = gpiochip_get_data(chip);
struct arizona *arizona = arizona_gpio->arizona;
- bool persistent = gpiochip_line_is_persistent(chip, offset);
+ bool persistent = gpiochip_line_is_persistent_suspend(chip, offset);
bool change;
int ret;
@@ -99,7 +99,7 @@ static int arizona_gpio_direction_out(struct gpio_chip *chip,
{
struct arizona_gpio *arizona_gpio = gpiochip_get_data(chip);
struct arizona *arizona = arizona_gpio->arizona;
- bool persistent = gpiochip_line_is_persistent(chip, offset);
+ bool persistent = gpiochip_line_is_persistent_suspend(chip, offset);
unsigned int val;
int ret;
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index a56b29fd8bb1..d9dc7e588699 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2414,6 +2414,40 @@ int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce)
EXPORT_SYMBOL_GPL(gpiod_set_debounce);
/**
+ * gpiod_set_reset_tolerant - Hold state across controller reset
+ * @desc: descriptor of the GPIO for which to set debounce time
+ * @tolerant: True to hold state across a controller reset, false otherwise
+ *
+ * Returns:
+ * 0 on success, %-ENOTSUPP if the controller doesn't support setting the
+ * reset tolerance or less than zero on other failures.
+ */
+int gpiod_set_reset_tolerant(struct gpio_desc *desc, bool tolerant)
+{
+ struct gpio_chip *chip;
+ unsigned long packed;
+ int rc;
+
+ chip = desc->gdev->chip;
+ if (!chip->set_config)
+ return -ENOTSUPP;
+
+ packed = pinconf_to_config_packed(PIN_CONFIG_RESET_TOLERANT, tolerant);
+
+ rc = chip->set_config(chip, gpio_chip_hwgpio(desc), packed);
+ if (rc < 0)
+ return rc;
+
+ if (tolerant)
+ set_bit(FLAG_RESET_TOLERANT, &desc->flags);
+ else
+ clear_bit(FLAG_RESET_TOLERANT, &desc->flags);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(gpiod_set_reset_tolerant);
+
+/**
* gpiod_is_active_low - test whether a GPIO is active-low or not
* @desc: the gpio descriptor to test
*
@@ -2885,7 +2919,8 @@ bool gpiochip_line_is_open_source(struct gpio_chip *chip, unsigned int offset)
}
EXPORT_SYMBOL_GPL(gpiochip_line_is_open_source);
-bool gpiochip_line_is_persistent(struct gpio_chip *chip, unsigned int offset)
+bool gpiochip_line_is_persistent_suspend(struct gpio_chip *chip,
+ unsigned int offset)
{
if (offset >= chip->ngpio)
return false;
@@ -2893,7 +2928,18 @@ bool gpiochip_line_is_persistent(struct gpio_chip *chip, unsigned int offset)
return !test_bit(FLAG_SLEEP_MAY_LOSE_VALUE,
&chip->gpiodev->descs[offset].flags);
}
-EXPORT_SYMBOL_GPL(gpiochip_line_is_persistent);
+EXPORT_SYMBOL_GPL(gpiochip_line_is_persistent_suspend);
+
+bool gpiochip_line_is_persistent_reset(struct gpio_chip *chip,
+ unsigned int offset)
+{
+ if (offset >= chip->ngpio)
+ return false;
+
+ return test_bit(FLAG_RESET_TOLERANT,
+ &chip->gpiodev->descs[offset].flags);
+}
+EXPORT_SYMBOL_GPL(gpiochip_line_is_persistent_reset);
/**
* gpiod_get_raw_value_cansleep() - return a gpio's raw value
@@ -3271,6 +3317,11 @@ int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
if (lflags & GPIO_SLEEP_MAY_LOSE_VALUE)
set_bit(FLAG_SLEEP_MAY_LOSE_VALUE, &desc->flags);
+ status = gpiod_set_reset_tolerant(desc,
+ !!(lflags & GPIO_RESET_TOLERANT));
+ if (status < 0)
+ return status;
+
/* No particular flag request, return here... */
if (!(dflags & GPIOD_FLAGS_BIT_DIR_SET)) {
pr_debug("no flags found for %s\n", con_id);
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 799208fc189a..4ce36f14c1ad 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -202,6 +202,7 @@ struct gpio_desc {
#define FLAG_USED_AS_IRQ 9 /* GPIO is connected to an IRQ */
#define FLAG_IS_HOGGED 11 /* GPIO is hogged */
#define FLAG_SLEEP_MAY_LOSE_VALUE 12 /* GPIO may lose value in sleep */
+#define FLAG_RESET_TOLERANT 13 /* GPIO to maintain state at shutdown */
/* Connection label */
const char *label;
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index 8f702fcbe485..ee5e125c8fec 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -121,6 +121,7 @@ void gpiod_set_raw_array_value_cansleep(unsigned int array_size,
int *value_array);
int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce);
+int gpiod_set_reset_tolerant(struct gpio_desc *desc, bool tolerant);
int gpiod_is_active_low(const struct gpio_desc *desc);
int gpiod_cansleep(const struct gpio_desc *desc);
@@ -381,6 +382,14 @@ static inline int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce)
return -ENOSYS;
}
+static inline int gpiod_set_reset_tolerant(struct gpio_desc *desc,
+ bool tolerant)
+{
+ /* GPIO can never have been requested */
+ WARN_ON(1);
+ return -ENOSYS;
+}
+
static inline int gpiod_is_active_low(const struct gpio_desc *desc)
{
/* GPIO can never have been requested */
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index c97f8325e8bf..6e9d32bdab55 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -233,7 +233,10 @@ bool gpiochip_line_is_open_drain(struct gpio_chip *chip, unsigned int offset);
bool gpiochip_line_is_open_source(struct gpio_chip *chip, unsigned int offset);
/* Sleep persistence inquiry for drivers */
-bool gpiochip_line_is_persistent(struct gpio_chip *chip, unsigned int offset);
+bool gpiochip_line_is_persistent_suspend(struct gpio_chip *chip,
+ unsigned int offset);
+bool gpiochip_line_is_persistent_reset(struct gpio_chip *chip,
+ unsigned int offset);
/* get driver data */
void *gpiochip_get_data(struct gpio_chip *chip);
diff --git a/include/linux/gpio/machine.h b/include/linux/gpio/machine.h
index 5e9f294c29eb..11b7209c7ab9 100644
--- a/include/linux/gpio/machine.h
+++ b/include/linux/gpio/machine.h
@@ -11,6 +11,8 @@ enum gpio_lookup_flags {
GPIO_OPEN_SOURCE = (1 << 2),
GPIO_SLEEP_MAINTAIN_VALUE = (0 << 3),
GPIO_SLEEP_MAY_LOSE_VALUE = (1 << 3),
+ GPIO_RESET_INTOLERANT = (0 << 4),
+ GPIO_RESET_TOLERANT = (1 << 4),
};
/**
diff --git a/include/linux/pinctrl/pinconf-generic.h b/include/linux/pinctrl/pinconf-generic.h
index 5d8bc7f21c2a..487cc863cb36 100644
--- a/include/linux/pinctrl/pinconf-generic.h
+++ b/include/linux/pinctrl/pinconf-generic.h
@@ -90,6 +90,7 @@
* @PIN_CONFIG_SLEW_RATE: if the pin can select slew rate, the argument to
* this parameter (on a custom format) tells the driver which alternative
* slew rate to use.
+ * @PIN_CONFIG_RESET_TOLERANT: retains the pin state across a system reset
* @PIN_CONFIG_END: this is the last enumerator for pin configurations, if
* you need to pass in custom configurations to the pin controller, use
* PIN_CONFIG_END+1 as the base offset.
@@ -117,6 +118,7 @@ enum pin_config_param {
PIN_CONFIG_POWER_SOURCE,
PIN_CONFIG_SLEEP_HARDWARE_STATE,
PIN_CONFIG_SLEW_RATE,
+ PIN_CONFIG_RESET_TOLERANT,
PIN_CONFIG_END = 0x7F,
PIN_CONFIG_MAX = 0xFF,
};
--
2.11.0
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [RFC PATCH 1/5] gpio: gpiolib: Add core support for maintaining GPIO values on reset
2017-10-20 3:37 ` [RFC PATCH 1/5] gpio: gpiolib: Add core support for maintaining GPIO values on reset Andrew Jeffery
@ 2017-10-20 7:17 ` Linus Walleij
2017-10-20 7:43 ` Linus Walleij
2017-10-20 8:24 ` Andrew Jeffery
0 siblings, 2 replies; 22+ messages in thread
From: Linus Walleij @ 2017-10-20 7:17 UTC (permalink / raw)
To: Andrew Jeffery
Cc: linux-gpio@vger.kernel.org, Jonathan Corbet, Joel Stanley,
Ryan Chen, Rob Herring, Frank Rowand, Charles Keepax,
Laxman Dewangan, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org, patches, devicetree@vger.kernel.org,
OpenBMC Maillist, linux-aspeed
On Fri, Oct 20, 2017 at 5:37 AM, Andrew Jeffery <andrew@aj.id.au> wrote:
> GPIO state reset tolerance is implemented in gpiolib through the
> addition of a new pinconf parameter. With that, some renaming of helpers
> is done to clarify the scope of the already existing
> gpiochip_line_is_persistent(), as it's now ambiguous as to whether that
> means on suspend, reset or both.
Isn't it most reasonable to say persistance covers both cases, reset
and/or sleep? This seems a bit like overdefined.
So can we say that is this flag is set, the hardware and driver should
do its best to preserve the value across any system disruptions.
We can change the wording of course, patches welcome for that.
But do we really need to distinguish the cases of disruption and
whether we cover up for them or not?
I would say we can deal with that the day we have a system with
two register bits (or similar) where you can select to preserve across
sleep, reset, one or the other, AND there is also a usecase such that
a user wants to preserve the value across reset but not suspend or
vice versa.
I suspect that will not happen.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC PATCH 1/5] gpio: gpiolib: Add core support for maintaining GPIO values on reset
2017-10-20 7:17 ` Linus Walleij
@ 2017-10-20 7:43 ` Linus Walleij
2017-10-20 8:32 ` Andrew Jeffery
2017-10-20 8:24 ` Andrew Jeffery
1 sibling, 1 reply; 22+ messages in thread
From: Linus Walleij @ 2017-10-20 7:43 UTC (permalink / raw)
To: Andrew Jeffery, Bartosz Golaszewski, Michael Welling
Cc: linux-gpio@vger.kernel.org, Jonathan Corbet, Joel Stanley,
Ryan Chen, Rob Herring, Frank Rowand, Charles Keepax,
Laxman Dewangan, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org, patches, devicetree@vger.kernel.org,
OpenBMC Maillist, linux-aspeed
On Fri, Oct 20, 2017 at 9:17 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Fri, Oct 20, 2017 at 5:37 AM, Andrew Jeffery <andrew@aj.id.au> wrote:
>
>> GPIO state reset tolerance is implemented in gpiolib through the
>> addition of a new pinconf parameter. With that, some renaming of helpers
>> is done to clarify the scope of the already existing
>> gpiochip_line_is_persistent(), as it's now ambiguous as to whether that
>> means on suspend, reset or both.
>
> Isn't it most reasonable to say persistance covers both cases, reset
> and/or sleep? This seems a bit like overdefined.
I should also add: right now persistance is defined in negative terms,
you can supply the flag "may lose value", which means the subsystem
by default, and driver by default, will try to keep values persistent across
sleep.
Then it is possible to opt in for not doing so. (Usually to save power I
think.)
I think that especially for userspace use cases, saving power should
not really be the concern, but correct me if I'm wrong. I am thinking
of a box with a DC plug wired up to a factory line here.
What we have in the Arizona driver is an opt-in where the DT can
say "don't preserve the value this line during system sleep" i.e. "lay lose
value" and we can extend that flag to mean "don't preserve this line
during reset either" but by default assume that we should.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC PATCH 1/5] gpio: gpiolib: Add core support for maintaining GPIO values on reset
2017-10-20 7:43 ` Linus Walleij
@ 2017-10-20 8:32 ` Andrew Jeffery
2017-10-25 8:11 ` Charles Keepax
0 siblings, 1 reply; 22+ messages in thread
From: Andrew Jeffery @ 2017-10-20 8:32 UTC (permalink / raw)
To: Linus Walleij, Bartosz Golaszewski, Michael Welling
Cc: linux-gpio@vger.kernel.org, Jonathan Corbet, Joel Stanley,
Ryan Chen, Rob Herring, Frank Rowand, Charles Keepax,
Laxman Dewangan, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org, patches, devicetree@vger.kernel.org,
OpenBMC Maillist, linux-aspeed
[-- Attachment #1: Type: text/plain, Size: 2088 bytes --]
On Fri, 2017-10-20 at 09:43 +0200, Linus Walleij wrote:
> On Fri, Oct 20, 2017 at 9:17 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> > > > On Fri, Oct 20, 2017 at 5:37 AM, Andrew Jeffery <andrew@aj.id.au> wrote:
> >
> > > GPIO state reset tolerance is implemented in gpiolib through the
> > > addition of a new pinconf parameter. With that, some renaming of helpers
> > > is done to clarify the scope of the already existing
> > > gpiochip_line_is_persistent(), as it's now ambiguous as to whether that
> > > means on suspend, reset or both.
> >
> > Isn't it most reasonable to say persistance covers both cases, reset
> > and/or sleep? This seems a bit like overdefined.
>
> I should also add: right now persistance is defined in negative terms,
> you can supply the flag "may lose value", which means the subsystem
> by default, and driver by default, will try to keep values persistent across
> sleep.
>
> Then it is possible to opt in for not doing so. (Usually to save power I
> think.)
>
> I think that especially for userspace use cases, saving power should
> not really be the concern, but correct me if I'm wrong. I am thinking
> of a box with a DC plug wired up to a factory line here.
>
> What we have in the Arizona driver is an opt-in where the DT can
> say "don't preserve the value this line during system sleep" i.e. "lay lose
> value" and we can extend that flag to mean "don't preserve this line
> during reset either" but by default assume that we should.
Yeah, the preserve polarity was another thing I debated given the
current example with the Arizona driver. Not preserving is the default
for the Aspeed hardware, so that ended up influencing my choice. Not
that implementation details should necessarily influence interface
design, but it was at least more than a coin toss.
I don't have anything specific against preserving by default, just my
gut instinct and the hardware went the other way. As long as we expose
the option to opt out, which the additions for the Arizona already do.
Cheers,
Andrew
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC PATCH 1/5] gpio: gpiolib: Add core support for maintaining GPIO values on reset
2017-10-20 8:32 ` Andrew Jeffery
@ 2017-10-25 8:11 ` Charles Keepax
2017-10-26 0:00 ` Andrew Jeffery
0 siblings, 1 reply; 22+ messages in thread
From: Charles Keepax @ 2017-10-25 8:11 UTC (permalink / raw)
To: Andrew Jeffery
Cc: Linus Walleij, Bartosz Golaszewski, Michael Welling,
linux-gpio@vger.kernel.org, Jonathan Corbet, Joel Stanley,
Ryan Chen, Rob Herring, Frank Rowand, Charles Keepax,
Laxman Dewangan, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org, patches, devicetree@vger.kernel.org,
OpenBMC Maillist, linux-aspeed
On Fri, Oct 20, 2017 at 07:02:27PM +1030, Andrew Jeffery wrote:
> On Fri, 2017-10-20 at 09:43 +0200, Linus Walleij wrote:
> > On Fri, Oct 20, 2017 at 9:17 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> > > > > On Fri, Oct 20, 2017 at 5:37 AM, Andrew Jeffery <andrew@aj.id.au> wrote:
> > >
> > > > GPIO state reset tolerance is implemented in gpiolib through the
> > > > addition of a new pinconf parameter. With that, some renaming of helpers
> > > > is done to clarify the scope of the already existing
> > > > gpiochip_line_is_persistent(), as it's now ambiguous as to whether that
> > > > means on suspend, reset or both.
> > >
> > > Isn't it most reasonable to say persistance covers both cases, reset
> > > and/or sleep? This seems a bit like overdefined.
> >
Seems reasonable to me to just expand the existing stuff to cover
reset as well, I don't think that should cause any issues for the
Arizona stuff.
> > I should also add: right now persistance is defined in negative terms,
> > you can supply the flag "may lose value", which means the subsystem
> > by default, and driver by default, will try to keep values persistent across
> > sleep.
> >
> > Then it is possible to opt in for not doing so. (Usually to save power I
> > think.)
> >
> > I think that especially for userspace use cases, saving power should
> > not really be the concern, but correct me if I'm wrong. I am thinking
> > of a box with a DC plug wired up to a factory line here.
> >
> > What we have in the Arizona driver is an opt-in where the DT can
> > say "don't preserve the value this line during system sleep" i.e. "lay lose
> > value" and we can extend that flag to mean "don't preserve this line
> > during reset either" but by default assume that we should.
>
> Yeah, the preserve polarity was another thing I debated given the
> current example with the Arizona driver. Not preserving is the default
> for the Aspeed hardware, so that ended up influencing my choice. Not
> that implementation details should necessarily influence interface
> design, but it was at least more than a coin toss.
>
The way I think we ended up looking at this was when a user
requests a GPIO the least surprising thing for them is the value
maintains until they change it again. So it made sense to make
the allowing the value to be dropped the opt in side.
> I don't have anything specific against preserving by default, just my
> gut instinct and the hardware went the other way. As long as we expose
> the option to opt out, which the additions for the Arizona already do.
>
> Cheers,
>
> Andrew
Thanks,
Charles
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC PATCH 1/5] gpio: gpiolib: Add core support for maintaining GPIO values on reset
2017-10-25 8:11 ` Charles Keepax
@ 2017-10-26 0:00 ` Andrew Jeffery
0 siblings, 0 replies; 22+ messages in thread
From: Andrew Jeffery @ 2017-10-26 0:00 UTC (permalink / raw)
To: Charles Keepax
Cc: Linus Walleij, Bartosz Golaszewski, Michael Welling,
linux-gpio@vger.kernel.org, Jonathan Corbet, Joel Stanley,
Ryan Chen, Rob Herring, Frank Rowand, Charles Keepax,
Laxman Dewangan, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org, patches, devicetree@vger.kernel.org,
OpenBMC Maillist, linux-aspeed
[-- Attachment #1: Type: text/plain, Size: 1103 bytes --]
On Wed, 2017-10-25 at 09:11 +0100, Charles Keepax wrote:
> On Fri, Oct 20, 2017 at 07:02:27PM +1030, Andrew Jeffery wrote:
> > On Fri, 2017-10-20 at 09:43 +0200, Linus Walleij wrote:
> > > On Fri, Oct 20, 2017 at 9:17 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> > > > > > On Fri, Oct 20, 2017 at 5:37 AM, Andrew Jeffery <andrew@aj.id.au> wrote:
> > > > > GPIO state reset tolerance is implemented in gpiolib through the
> > > > > addition of a new pinconf parameter. With that, some renaming of helpers
> > > > > is done to clarify the scope of the already existing
> > > > > gpiochip_line_is_persistent(), as it's now ambiguous as to whether that
> > > > > means on suspend, reset or both.
> > > >
> > > > Isn't it most reasonable to say persistance covers both cases, reset
> > > > and/or sleep? This seems a bit like overdefined.
>
> Seems reasonable to me to just expand the existing stuff to cover
> reset as well, I don't think that should cause any issues for the
> Arizona stuff.
Great. I addressed this in the non-RFC series.
Thanks for the feedback.
Andrew
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC PATCH 1/5] gpio: gpiolib: Add core support for maintaining GPIO values on reset
2017-10-20 7:17 ` Linus Walleij
2017-10-20 7:43 ` Linus Walleij
@ 2017-10-20 8:24 ` Andrew Jeffery
1 sibling, 0 replies; 22+ messages in thread
From: Andrew Jeffery @ 2017-10-20 8:24 UTC (permalink / raw)
To: Linus Walleij
Cc: linux-gpio@vger.kernel.org, Jonathan Corbet, Joel Stanley,
Ryan Chen, Rob Herring, Frank Rowand, Charles Keepax,
Laxman Dewangan, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org, patches, devicetree@vger.kernel.org,
OpenBMC Maillist, linux-aspeed
[-- Attachment #1: Type: text/plain, Size: 1550 bytes --]
On Fri, 2017-10-20 at 09:17 +0200, Linus Walleij wrote:
> > On Fri, Oct 20, 2017 at 5:37 AM, Andrew Jeffery <andrew@aj.id.au> wrote:
>
> > GPIO state reset tolerance is implemented in gpiolib through the
> > addition of a new pinconf parameter. With that, some renaming of helpers
> > is done to clarify the scope of the already existing
> > gpiochip_line_is_persistent(), as it's now ambiguous as to whether that
> > means on suspend, reset or both.
>
> Isn't it most reasonable to say persistance covers both cases, reset
> and/or sleep? This seems a bit like overdefined.
I definitely had some internal debate about that. I erred on the side of
avoiding potential change in expectations for the arizona. If you consider that
overdefined then I'm happy to go the other way.
>
> So can we say that is this flag is set, the hardware and driver should
> do its best to preserve the value across any system disruptions.
>
> We can change the wording of course, patches welcome for that.
Yep.
>
> But do we really need to distinguish the cases of disruption and
> whether we cover up for them or not?
>
> I would say we can deal with that the day we have a system with
> two register bits (or similar) where you can select to preserve across
> sleep, reset, one or the other, AND there is also a usecase such that
> a user wants to preserve the value across reset but not suspend or
> vice versa.
>
> I suspect that will not happen.
A very reasonable approach.
Cheers for the feedback.
Andrew
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* [RFC PATCH 2/5] gpio: gpiolib: Add OF support for maintaining GPIO values on reset
2017-10-20 3:37 [RFC PATCH 0/5] gpio: Expose reset tolerance capability Andrew Jeffery
2017-10-20 3:37 ` [RFC PATCH 1/5] gpio: gpiolib: Add core support for maintaining GPIO values on reset Andrew Jeffery
@ 2017-10-20 3:37 ` Andrew Jeffery
[not found] ` <20171020033727.21557-3-andrew-zrmu5oMJ5Fs@public.gmane.org>
2017-10-20 3:37 ` [RFC PATCH 3/5] gpio: gpiolib: Add chardev " Andrew Jeffery
` (2 subsequent siblings)
4 siblings, 1 reply; 22+ messages in thread
From: Andrew Jeffery @ 2017-10-20 3:37 UTC (permalink / raw)
To: linux-gpio
Cc: Andrew Jeffery, linus.walleij, corbet, joel, ryan_chen, robh+dt,
frowand.list, ckeepax, ldewangan, linux-doc, linux-kernel,
patches, devicetree, openbmc, linux-aspeed
Add flags and the associated flag mappings between interfaces to enable
GPIO reset tolerance to be specified via devicetree.
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
drivers/gpio/gpiolib-of.c | 2 ++
drivers/gpio/gpiolib.c | 5 +++++
include/dt-bindings/gpio/gpio.h | 4 ++++
include/linux/of_gpio.h | 1 +
4 files changed, 12 insertions(+)
diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index e0d59e61b52f..4a268ba52998 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -155,6 +155,8 @@ struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
if (of_flags & OF_GPIO_SLEEP_MAY_LOSE_VALUE)
*flags |= GPIO_SLEEP_MAY_LOSE_VALUE;
+ if (of_flags & OF_GPIO_RESET_TOLERANT)
+ *flags |= GPIO_RESET_TOLERANT;
return desc;
}
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index d9dc7e588699..6b4c5df10e84 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -3434,6 +3434,7 @@ struct gpio_desc *fwnode_get_named_gpiod(struct fwnode_handle *fwnode,
bool active_low = false;
bool single_ended = false;
bool open_drain = false;
+ bool reset_tolerant = false;
int ret;
if (!fwnode)
@@ -3448,6 +3449,7 @@ struct gpio_desc *fwnode_get_named_gpiod(struct fwnode_handle *fwnode,
active_low = flags & OF_GPIO_ACTIVE_LOW;
single_ended = flags & OF_GPIO_SINGLE_ENDED;
open_drain = flags & OF_GPIO_OPEN_DRAIN;
+ reset_tolerant = flags & OF_GPIO_RESET_TOLERANT;
}
} else if (is_acpi_node(fwnode)) {
struct acpi_gpio_info info;
@@ -3478,6 +3480,9 @@ struct gpio_desc *fwnode_get_named_gpiod(struct fwnode_handle *fwnode,
lflags |= GPIO_OPEN_SOURCE;
}
+ if (reset_tolerant)
+ lflags |= GPIO_RESET_TOLERANT;
+
ret = gpiod_configure_flags(desc, propname, lflags, dflags);
if (ret < 0) {
gpiod_put(desc);
diff --git a/include/dt-bindings/gpio/gpio.h b/include/dt-bindings/gpio/gpio.h
index 70de5b7a6c9b..01c75d9e308e 100644
--- a/include/dt-bindings/gpio/gpio.h
+++ b/include/dt-bindings/gpio/gpio.h
@@ -32,4 +32,8 @@
#define GPIO_SLEEP_MAINTAIN_VALUE 0
#define GPIO_SLEEP_MAY_LOSE_VALUE 8
+/* Bit 4 express GPIO persistence on reset */
+#define GPIO_RESET_INTOLERANT 0
+#define GPIO_RESET_TOLERANT 16
+
#endif
diff --git a/include/linux/of_gpio.h b/include/linux/of_gpio.h
index 1fe205582111..9b34737706a7 100644
--- a/include/linux/of_gpio.h
+++ b/include/linux/of_gpio.h
@@ -32,6 +32,7 @@ enum of_gpio_flags {
OF_GPIO_SINGLE_ENDED = 0x2,
OF_GPIO_OPEN_DRAIN = 0x4,
OF_GPIO_SLEEP_MAY_LOSE_VALUE = 0x8,
+ OF_GPIO_RESET_TOLERANT = 0x16,
};
#ifdef CONFIG_OF_GPIO
--
2.11.0
^ permalink raw reply related [flat|nested] 22+ messages in thread* [RFC PATCH 3/5] gpio: gpiolib: Add chardev support for maintaining GPIO values on reset
2017-10-20 3:37 [RFC PATCH 0/5] gpio: Expose reset tolerance capability Andrew Jeffery
2017-10-20 3:37 ` [RFC PATCH 1/5] gpio: gpiolib: Add core support for maintaining GPIO values on reset Andrew Jeffery
2017-10-20 3:37 ` [RFC PATCH 2/5] gpio: gpiolib: Add OF " Andrew Jeffery
@ 2017-10-20 3:37 ` Andrew Jeffery
2017-10-20 7:27 ` Linus Walleij
2017-10-20 3:37 ` [RFC PATCH 4/5] gpio: gpiolib: Add sysfs " Andrew Jeffery
2017-10-20 3:37 ` [RFC PATCH 5/5] gpio: aspeed: Add support for reset tolerance Andrew Jeffery
4 siblings, 1 reply; 22+ messages in thread
From: Andrew Jeffery @ 2017-10-20 3:37 UTC (permalink / raw)
To: linux-gpio
Cc: Andrew Jeffery, linus.walleij, corbet, joel, ryan_chen, robh+dt,
frowand.list, ckeepax, ldewangan, linux-doc, linux-kernel,
patches, devicetree, openbmc, linux-aspeed
Similar to devicetree support, add flags and mappings to expose reset
tolerance configuration through the chardev interface.
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
drivers/gpio/gpiolib.c | 14 +++++++++++++-
include/uapi/linux/gpio.h | 11 ++++++-----
2 files changed, 19 insertions(+), 6 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 6b4c5df10e84..442ee5ceee08 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -357,7 +357,8 @@ struct linehandle_state {
GPIOHANDLE_REQUEST_OUTPUT | \
GPIOHANDLE_REQUEST_ACTIVE_LOW | \
GPIOHANDLE_REQUEST_OPEN_DRAIN | \
- GPIOHANDLE_REQUEST_OPEN_SOURCE)
+ GPIOHANDLE_REQUEST_OPEN_SOURCE | \
+ GPIOHANDLE_REQUEST_RESET_TOLERANT)
static long linehandle_ioctl(struct file *filep, unsigned int cmd,
unsigned long arg)
@@ -498,6 +499,17 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
set_bit(FLAG_OPEN_SOURCE, &desc->flags);
/*
+ * Unconditionally configure reset tolerance, as it's possible
+ * that the tolerance flag itself becomes tolerant to resets.
+ * Thus it could remain set from a previous environment, but
+ * the current environment may not expect it so.
+ */
+ ret = gpiod_set_reset_tolerant(desc,
+ !!(lflags & GPIOHANDLE_REQUEST_RESET_TOLERANT));
+ if (ret < 0)
+ goto out_free_descs;
+
+ /*
* Lines have to be requested explicitly for input
* or output, else the line will be treated "as is".
*/
diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h
index 333d3544c964..1b1ce1af8653 100644
--- a/include/uapi/linux/gpio.h
+++ b/include/uapi/linux/gpio.h
@@ -56,11 +56,12 @@ struct gpioline_info {
#define GPIOHANDLES_MAX 64
/* Linerequest flags */
-#define GPIOHANDLE_REQUEST_INPUT (1UL << 0)
-#define GPIOHANDLE_REQUEST_OUTPUT (1UL << 1)
-#define GPIOHANDLE_REQUEST_ACTIVE_LOW (1UL << 2)
-#define GPIOHANDLE_REQUEST_OPEN_DRAIN (1UL << 3)
-#define GPIOHANDLE_REQUEST_OPEN_SOURCE (1UL << 4)
+#define GPIOHANDLE_REQUEST_INPUT (1UL << 0)
+#define GPIOHANDLE_REQUEST_OUTPUT (1UL << 1)
+#define GPIOHANDLE_REQUEST_ACTIVE_LOW (1UL << 2)
+#define GPIOHANDLE_REQUEST_OPEN_DRAIN (1UL << 3)
+#define GPIOHANDLE_REQUEST_OPEN_SOURCE (1UL << 4)
+#define GPIOHANDLE_REQUEST_RESET_TOLERANT (1UL << 5)
/**
* struct gpiohandle_request - Information about a GPIO handle request
--
2.11.0
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [RFC PATCH 3/5] gpio: gpiolib: Add chardev support for maintaining GPIO values on reset
2017-10-20 3:37 ` [RFC PATCH 3/5] gpio: gpiolib: Add chardev " Andrew Jeffery
@ 2017-10-20 7:27 ` Linus Walleij
2017-10-20 9:02 ` Andrew Jeffery
0 siblings, 1 reply; 22+ messages in thread
From: Linus Walleij @ 2017-10-20 7:27 UTC (permalink / raw)
To: Andrew Jeffery, Bartosz Golaszewski, Michael Welling
Cc: linux-gpio@vger.kernel.org, Jonathan Corbet, Joel Stanley,
Ryan Chen, Rob Herring, Frank Rowand, Charles Keepax,
Laxman Dewangan, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org, patches, devicetree@vger.kernel.org,
OpenBMC Maillist, linux-aspeed
I paged Bartosz and Michael on this, they are experts on the use cases for
the character device and their opinions are likely more valuable than mine.
On Fri, Oct 20, 2017 at 5:37 AM, Andrew Jeffery <andrew@aj.id.au> wrote:
> Similar to devicetree support, add flags and mappings to expose reset
> tolerance configuration through the chardev interface.
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
(...)
> + * Unconditionally configure reset tolerance, as it's possible
> + * that the tolerance flag itself becomes tolerant to resets.
> + * Thus it could remain set from a previous environment, but
> + * the current environment may not expect it so.
> + */
> + ret = gpiod_set_reset_tolerant(desc,
> + !!(lflags & GPIOHANDLE_REQUEST_RESET_TOLERANT));
> + if (ret < 0)
> + goto out_free_descs;
First, as noted in the first patch, IMO we should just go for persistance,
i.e. you want to flag to the system to keep the line persistent in any case,
no matter if the system goes to sleep or resets.
So the usecase is going to be a control system or similar, a makerspace
project, an industrial product of some kind, driving GPIO from userspace.
I don't see it as helpful to give userspace control over whether the line
is persistent or not. It is more reasonable to assume persistance for
userspace use cases, don't you think? Whether the system goes to sleep
or the gpiochip resets should not make a door suddenly close or the
lights in the christmas tree go out, right? I think if the gpiochip supports
persistance of any kind, we should try to use it and not have userspace
provide flags for that.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC PATCH 3/5] gpio: gpiolib: Add chardev support for maintaining GPIO values on reset
2017-10-20 7:27 ` Linus Walleij
@ 2017-10-20 9:02 ` Andrew Jeffery
2017-10-25 8:14 ` Charles Keepax
0 siblings, 1 reply; 22+ messages in thread
From: Andrew Jeffery @ 2017-10-20 9:02 UTC (permalink / raw)
To: Linus Walleij, Bartosz Golaszewski, Michael Welling
Cc: linux-gpio@vger.kernel.org, Jonathan Corbet, Joel Stanley,
Ryan Chen, Rob Herring, Frank Rowand, Charles Keepax,
Laxman Dewangan, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org, patches, devicetree@vger.kernel.org,
OpenBMC Maillist, linux-aspeed
[-- Attachment #1: Type: text/plain, Size: 2408 bytes --]
On Fri, 2017-10-20 at 09:27 +0200, Linus Walleij wrote:
> I paged Bartosz and Michael on this, they are experts on the use cases for
> the character device and their opinions are likely more valuable than mine.
>
> > On Fri, Oct 20, 2017 at 5:37 AM, Andrew Jeffery <andrew@aj.id.au> wrote:
> > Similar to devicetree support, add flags and mappings to expose reset
> > tolerance configuration through the chardev interface.
> >
> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
>
> (...)
>
> > + * Unconditionally configure reset tolerance, as it's possible
> > + * that the tolerance flag itself becomes tolerant to resets.
> > + * Thus it could remain set from a previous environment, but
> > + * the current environment may not expect it so.
> > + */
> > + ret = gpiod_set_reset_tolerant(desc,
> > + !!(lflags & GPIOHANDLE_REQUEST_RESET_TOLERANT));
> > + if (ret < 0)
> > + goto out_free_descs;
>
> First, as noted in the first patch, IMO we should just go for persistance,
> i.e. you want to flag to the system to keep the line persistent in any case,
> no matter if the system goes to sleep or resets.
>
> So the usecase is going to be a control system or similar, a makerspace
> project, an industrial product of some kind, driving GPIO from userspace.
>
> I don't see it as helpful to give userspace control over whether the line
> is persistent or not. It is more reasonable to assume persistance for
> userspace use cases, don't you think? Whether the system goes to sleep
> or the gpiochip resets should not make a door suddenly close or the
> lights in the christmas tree go out, right? I think if the gpiochip supports
> persistance of any kind, we should try to use it and not have userspace
> provide flags for that.
Right. I guess the counter argument to your examples is if the gpio is
controlling any active process that we don't want to continue if we've
lost the capacity to monitor some other inputs (some kind of dead-man's
switch). But maybe the argument is that should be implemented in the
kernel anyway?
Andrew
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC PATCH 3/5] gpio: gpiolib: Add chardev support for maintaining GPIO values on reset
2017-10-20 9:02 ` Andrew Jeffery
@ 2017-10-25 8:14 ` Charles Keepax
2017-10-26 0:05 ` Andrew Jeffery
0 siblings, 1 reply; 22+ messages in thread
From: Charles Keepax @ 2017-10-25 8:14 UTC (permalink / raw)
To: Andrew Jeffery
Cc: Linus Walleij, Bartosz Golaszewski, Michael Welling,
linux-gpio@vger.kernel.org, Jonathan Corbet, Joel Stanley,
Ryan Chen, Rob Herring, Frank Rowand, Charles Keepax,
Laxman Dewangan, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org, patches, devicetree@vger.kernel.org,
OpenBMC Maillist, linux-aspeed
On Fri, Oct 20, 2017 at 07:32:53PM +1030, Andrew Jeffery wrote:
> On Fri, 2017-10-20 at 09:27 +0200, Linus Walleij wrote:
> > I don't see it as helpful to give userspace control over whether the line
> > is persistent or not. It is more reasonable to assume persistance for
> > userspace use cases, don't you think? Whether the system goes to sleep
> > or the gpiochip resets should not make a door suddenly close or the
> > lights in the christmas tree go out, right? I think if the gpiochip supports
> > persistance of any kind, we should try to use it and not have userspace
> > provide flags for that.
>
> Right. I guess the counter argument to your examples is if the gpio is
> controlling any active process that we don't want to continue if we've
> lost the capacity to monitor some other inputs (some kind of dead-man's
> switch). But maybe the argument is that should be implemented in the
> kernel anyway?
>
To me it certainly feels like decisions like this should live in
the kernel, your talking about things that could cause very weird
hardware behaviour if set wrong, so it makes sense to me to have
that responsibility guarded in the kernel.
Thanks,
Charles
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC PATCH 3/5] gpio: gpiolib: Add chardev support for maintaining GPIO values on reset
2017-10-25 8:14 ` Charles Keepax
@ 2017-10-26 0:05 ` Andrew Jeffery
[not found] ` <1508976339.13477.5.camel-zrmu5oMJ5Fs@public.gmane.org>
2017-10-31 9:59 ` Linus Walleij
0 siblings, 2 replies; 22+ messages in thread
From: Andrew Jeffery @ 2017-10-26 0:05 UTC (permalink / raw)
To: Charles Keepax
Cc: Linus Walleij, Bartosz Golaszewski, Michael Welling,
linux-gpio@vger.kernel.org, Jonathan Corbet, Joel Stanley,
Ryan Chen, Rob Herring, Frank Rowand, Charles Keepax,
Laxman Dewangan, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org, patches, devicetree@vger.kernel.org,
OpenBMC Maillist, linux-aspeed
[-- Attachment #1: Type: text/plain, Size: 1832 bytes --]
On Wed, 2017-10-25 at 09:14 +0100, Charles Keepax wrote:
> On Fri, Oct 20, 2017 at 07:32:53PM +1030, Andrew Jeffery wrote:
> > On Fri, 2017-10-20 at 09:27 +0200, Linus Walleij wrote:
> > > I don't see it as helpful to give userspace control over whether the line
> > > is persistent or not. It is more reasonable to assume persistance for
> > > userspace use cases, don't you think? Whether the system goes to sleep
> > > or the gpiochip resets should not make a door suddenly close or the
> > > lights in the christmas tree go out, right? I think if the gpiochip supports
> > > persistance of any kind, we should try to use it and not have userspace
> > > provide flags for that.
> >
> > Right. I guess the counter argument to your examples is if the gpio is
> > controlling any active process that we don't want to continue if we've
> > lost the capacity to monitor some other inputs (some kind of dead-man's
> > switch). But maybe the argument is that should be implemented in the
> > kernel anyway?
> >
>
> To me it certainly feels like decisions like this should live in
> the kernel, your talking about things that could cause very weird
> hardware behaviour if set wrong, so it makes sense to me to have
> that responsibility guarded in the kernel.
I feel that taking this argument to its logical conclusion leads to
never exporting any GPIOs to userspace and doing everything in the
kernel. If userspace has exported the GPIO and is managing its state,
then it can *already* cause very weird hardware behaviour if set wrong.
The fact that userspace is controlling the GPIO state and not the
kernel already says that the kernel doesn't know how to manage it, so
why not expose the option for userspace to set the persistence, given
that it should know what it's doing?
Cheers,
Andrew
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread[parent not found: <1508976339.13477.5.camel-zrmu5oMJ5Fs@public.gmane.org>]
* Re: [RFC PATCH 3/5] gpio: gpiolib: Add chardev support for maintaining GPIO values on reset
[not found] ` <1508976339.13477.5.camel-zrmu5oMJ5Fs@public.gmane.org>
@ 2017-10-26 9:10 ` Charles Keepax
0 siblings, 0 replies; 22+ messages in thread
From: Charles Keepax @ 2017-10-26 9:10 UTC (permalink / raw)
To: Andrew Jeffery
Cc: Linus Walleij, Bartosz Golaszewski, Michael Welling,
linux-gpio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Jonathan Corbet, Joel Stanley, Ryan Chen, Rob Herring,
Frank Rowand, Charles Keepax, Laxman Dewangan,
linux-doc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
patches-yzvPICuk2AA4QjBA90+/kJqQE7yCjDx5,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
OpenBMC Maillist, linux-aspeed-uLR06cmDAlY/bJ5BZ2RsiQ
On Thu, Oct 26, 2017 at 10:35:39AM +1030, Andrew Jeffery wrote:
> On Wed, 2017-10-25 at 09:14 +0100, Charles Keepax wrote:
> > On Fri, Oct 20, 2017 at 07:32:53PM +1030, Andrew Jeffery wrote:
> > > On Fri, 2017-10-20 at 09:27 +0200, Linus Walleij wrote:
> > > > I don't see it as helpful to give userspace control over whether the line
> > > > is persistent or not. It is more reasonable to assume persistance for
> > > > userspace use cases, don't you think? Whether the system goes to sleep
> > > > or the gpiochip resets should not make a door suddenly close or the
> > > > lights in the christmas tree go out, right? I think if the gpiochip supports
> > > > persistance of any kind, we should try to use it and not have userspace
> > > > provide flags for that.
> > >
> > > Right. I guess the counter argument to your examples is if the gpio is
> > > controlling any active process that we don't want to continue if we've
> > > lost the capacity to monitor some other inputs (some kind of dead-man's
> > > switch). But maybe the argument is that should be implemented in the
> > > kernel anyway?
> > >
> >
> > To me it certainly feels like decisions like this should live in
> > the kernel, your talking about things that could cause very weird
> > hardware behaviour if set wrong, so it makes sense to me to have
> > that responsibility guarded in the kernel.
>
> I feel that taking this argument to its logical conclusion leads to
> never exporting any GPIOs to userspace and doing everything in the
> kernel. If userspace has exported the GPIO and is managing its state,
> then it can *already* cause very weird hardware behaviour if set wrong.
> The fact that userspace is controlling the GPIO state and not the
> kernel already says that the kernel doesn't know how to manage it, so
> why not expose the option for userspace to set the persistence, given
> that it should know what it's doing?
Admittedly yes, I guess it really comes down to use-cases. There
are fairly strong use-cases to control GPIOs from user-space
that justify the risks. The use-cases for being able to set
non-persistent GPIOs from user-space seem less clear to me, but
if they exist I certainly don't have any objection.
Thanks,
Charles
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC PATCH 3/5] gpio: gpiolib: Add chardev support for maintaining GPIO values on reset
2017-10-26 0:05 ` Andrew Jeffery
[not found] ` <1508976339.13477.5.camel-zrmu5oMJ5Fs@public.gmane.org>
@ 2017-10-31 9:59 ` Linus Walleij
1 sibling, 0 replies; 22+ messages in thread
From: Linus Walleij @ 2017-10-31 9:59 UTC (permalink / raw)
To: Andrew Jeffery
Cc: Charles Keepax, Bartosz Golaszewski, Michael Welling,
linux-gpio@vger.kernel.org, Jonathan Corbet, Joel Stanley,
Ryan Chen, Rob Herring, Frank Rowand, Charles Keepax,
Laxman Dewangan, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org, patches, devicetree@vger.kernel.org,
OpenBMC Maillist, linux-aspeed
On Thu, Oct 26, 2017 at 2:05 AM, Andrew Jeffery <andrew@aj.id.au> wrote:
> I feel that taking this argument to its logical conclusion leads to
> never exporting any GPIOs to userspace and doing everything in the
> kernel.
That is very much how I feel about things anyways.
In a recent presentation:
https://dflund.se/~triad/papers/GPIO-for-Engineers-and-Makers.pdf
I had the following text:
The Rules of Linux Userspace GPIO
1. You do not access GPIOs from userspace
2. YOU DO NOT ACCESS GPIOS FROM USERSPACE
3. Read Documentation/gpio/drivers-on-gpio.txt
4. Use the character device
> If userspace has exported the GPIO and is managing its state,
> then it can *already* cause very weird hardware behaviour if set wrong.
> The fact that userspace is controlling the GPIO state and not the
> kernel already says that the kernel doesn't know how to manage it, so
> why not expose the option for userspace to set the persistence, given
> that it should know what it's doing?
People do need to access GPIOs from userspace for things
like one-off makerspace projects, relays on factory lines,
fire alarms, door openers etc etc.
One-off projects is fine, the user likely has an idea about the
whole system that is comprehensive. They use the random
raspberry Pi (etc) development board for this. OK.
When we are talking about adding GPIO in mass-market goods
such as phones and tablets and laptops userspace GPIO
access become more and more dubious.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 22+ messages in thread
* [RFC PATCH 4/5] gpio: gpiolib: Add sysfs support for maintaining GPIO values on reset
2017-10-20 3:37 [RFC PATCH 0/5] gpio: Expose reset tolerance capability Andrew Jeffery
` (2 preceding siblings ...)
2017-10-20 3:37 ` [RFC PATCH 3/5] gpio: gpiolib: Add chardev " Andrew Jeffery
@ 2017-10-20 3:37 ` Andrew Jeffery
[not found] ` <20171020033727.21557-5-andrew-zrmu5oMJ5Fs@public.gmane.org>
2017-10-20 3:37 ` [RFC PATCH 5/5] gpio: aspeed: Add support for reset tolerance Andrew Jeffery
4 siblings, 1 reply; 22+ messages in thread
From: Andrew Jeffery @ 2017-10-20 3:37 UTC (permalink / raw)
To: linux-gpio
Cc: Andrew Jeffery, linus.walleij, corbet, joel, ryan_chen, robh+dt,
frowand.list, ckeepax, ldewangan, linux-doc, linux-kernel,
patches, devicetree, openbmc, linux-aspeed
Expose a new 'maintain' sysfs attribute to control both suspend and
reset tolerance.
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
Documentation/gpio/sysfs.txt | 9 +++++
drivers/gpio/gpiolib-sysfs.c | 88 ++++++++++++++++++++++++++++++++++++++++++--
2 files changed, 93 insertions(+), 4 deletions(-)
diff --git a/Documentation/gpio/sysfs.txt b/Documentation/gpio/sysfs.txt
index aeab01aa4d00..f447f0746884 100644
--- a/Documentation/gpio/sysfs.txt
+++ b/Documentation/gpio/sysfs.txt
@@ -96,6 +96,15 @@ and have the following read/write attributes:
for "rising" and "falling" edges will follow this
setting.
+ "maintain" ... displays and controls whether the state of the GPIO is
+ maintained or lost on suspend or reset. The valid values take
+ the following meanings:
+
+ 0: Do not maintain state on either suspend or reset
+ 1: Maintain state for suspend only
+ 2: Maintain state for reset only
+ 3: Maintain state for both suspend and reset
+
GPIO controllers have paths like /sys/class/gpio/gpiochip42/ (for the
controller implementing GPIOs starting at #42) and have the following
read-only attributes:
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 3f454eaf2101..bfa186e73e26 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -289,6 +289,74 @@ static ssize_t edge_store(struct device *dev,
}
static DEVICE_ATTR_RW(edge);
+#define GPIOLIB_SYSFS_MAINTAIN_SUSPEND BIT(0)
+#define GPIOLIB_SYSFS_MAINTAIN_RESET BIT(1)
+#define GPIOLIB_SYSFS_MAINTAIN_ALL GENMASK(1, 0)
+static ssize_t maintain_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct gpiod_data *data = dev_get_drvdata(dev);
+ ssize_t status = 0;
+ int val = 0;
+
+ mutex_lock(&data->mutex);
+
+ if (!test_bit(FLAG_SLEEP_MAY_LOSE_VALUE, &data->desc->flags))
+ val |= GPIOLIB_SYSFS_MAINTAIN_SUSPEND;
+
+ if (test_bit(FLAG_RESET_TOLERANT, &data->desc->flags))
+ val |= GPIOLIB_SYSFS_MAINTAIN_RESET;
+
+ status = sprintf(buf, "%d\n", val);
+
+ mutex_unlock(&data->mutex);
+
+ return status;
+}
+
+static ssize_t maintain_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf,
+ size_t size)
+{
+ struct gpiod_data *data = dev_get_drvdata(dev);
+ struct gpio_chip *chip;
+ ssize_t status;
+ long provided;
+
+ mutex_lock(&data->mutex);
+
+ chip = data->desc->gdev->chip;
+
+ if (!chip->set_config)
+ return -ENOTSUPP;
+
+ status = kstrtol(buf, 0, &provided);
+ if (status < 0)
+ goto out;
+
+ if (provided & ~GPIOLIB_SYSFS_MAINTAIN_ALL) {
+ status = -EINVAL;
+ goto out;
+ }
+
+ if (!(provided & GPIOLIB_SYSFS_MAINTAIN_SUSPEND))
+ set_bit(FLAG_SLEEP_MAY_LOSE_VALUE, &data->desc->flags);
+ else
+ clear_bit(FLAG_SLEEP_MAY_LOSE_VALUE,
+ &data->desc->flags);
+
+ /* Configure reset tolerance */
+ status = gpiod_set_reset_tolerant(data->desc,
+ !!(provided & GPIOLIB_SYSFS_MAINTAIN_RESET));
+out:
+ mutex_unlock(&data->mutex);
+
+ return status ? : size;
+
+}
+static DEVICE_ATTR_RW(maintain);
+
/* Caller holds gpiod-data mutex. */
static int gpio_sysfs_set_active_low(struct device *dev, int value)
{
@@ -378,6 +446,7 @@ static struct attribute *gpio_attrs[] = {
&dev_attr_edge.attr,
&dev_attr_value.attr,
&dev_attr_active_low.attr,
+ &dev_attr_maintain.attr,
NULL,
};
@@ -474,11 +543,22 @@ static ssize_t export_store(struct class *class,
status = -ENODEV;
goto done;
}
- status = gpiod_export(desc, true);
- if (status < 0)
+
+ /*
+ * If userspace is requesting the GPIO via sysfs, make them explicitly
+ * configure reset tolerance each time by unconditionally disabling it
+ * here, as the export and configuration steps are not atomic.
+ */
+ status = gpiod_set_reset_tolerant(desc, false);
+ if (status < 0) {
gpiod_free(desc);
- else
- set_bit(FLAG_SYSFS, &desc->flags);
+ } else {
+ status = gpiod_export(desc, true);
+ if (status < 0)
+ gpiod_free(desc);
+ else
+ set_bit(FLAG_SYSFS, &desc->flags);
+ }
done:
if (status)
--
2.11.0
^ permalink raw reply related [flat|nested] 22+ messages in thread* [RFC PATCH 5/5] gpio: aspeed: Add support for reset tolerance
2017-10-20 3:37 [RFC PATCH 0/5] gpio: Expose reset tolerance capability Andrew Jeffery
` (3 preceding siblings ...)
2017-10-20 3:37 ` [RFC PATCH 4/5] gpio: gpiolib: Add sysfs " Andrew Jeffery
@ 2017-10-20 3:37 ` Andrew Jeffery
4 siblings, 0 replies; 22+ messages in thread
From: Andrew Jeffery @ 2017-10-20 3:37 UTC (permalink / raw)
To: linux-gpio
Cc: Andrew Jeffery, linus.walleij, corbet, joel, ryan_chen, robh+dt,
frowand.list, ckeepax, ldewangan, linux-doc, linux-kernel,
patches, devicetree, openbmc, linux-aspeed
Use the new pinconf parameter for reset tolerance to expose the
associated capability of the Aspeed GPIO controller.
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
drivers/gpio/gpio-aspeed.c | 39 +++++++++++++++++++++++++++++++++++++--
1 file changed, 37 insertions(+), 2 deletions(-)
diff --git a/drivers/gpio/gpio-aspeed.c b/drivers/gpio/gpio-aspeed.c
index bfc53995064a..0492cd917178 100644
--- a/drivers/gpio/gpio-aspeed.c
+++ b/drivers/gpio/gpio-aspeed.c
@@ -60,6 +60,7 @@ struct aspeed_gpio_bank {
uint16_t val_regs;
uint16_t irq_regs;
uint16_t debounce_regs;
+ uint16_t tolerance_regs;
const char names[4][3];
};
@@ -70,48 +71,56 @@ static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
.val_regs = 0x0000,
.irq_regs = 0x0008,
.debounce_regs = 0x0040,
+ .tolerance_regs = 0x001c,
.names = { "A", "B", "C", "D" },
},
{
.val_regs = 0x0020,
.irq_regs = 0x0028,
.debounce_regs = 0x0048,
+ .tolerance_regs = 0x003c,
.names = { "E", "F", "G", "H" },
},
{
.val_regs = 0x0070,
.irq_regs = 0x0098,
.debounce_regs = 0x00b0,
+ .tolerance_regs = 0x00ac,
.names = { "I", "J", "K", "L" },
},
{
.val_regs = 0x0078,
.irq_regs = 0x00e8,
.debounce_regs = 0x0100,
+ .tolerance_regs = 0x00fc,
.names = { "M", "N", "O", "P" },
},
{
.val_regs = 0x0080,
.irq_regs = 0x0118,
.debounce_regs = 0x0130,
+ .tolerance_regs = 0x012c,
.names = { "Q", "R", "S", "T" },
},
{
.val_regs = 0x0088,
.irq_regs = 0x0148,
.debounce_regs = 0x0160,
+ .tolerance_regs = 0x015c,
.names = { "U", "V", "W", "X" },
},
{
.val_regs = 0x01E0,
.irq_regs = 0x0178,
.debounce_regs = 0x0190,
+ .tolerance_regs = 0x018c,
.names = { "Y", "Z", "AA", "AB" },
},
{
- .val_regs = 0x01E8,
- .irq_regs = 0x01A8,
+ .val_regs = 0x01e8,
+ .irq_regs = 0x01a8,
.debounce_regs = 0x01c0,
+ .tolerance_regs = 0x01bc,
.names = { "AC", "", "", "" },
},
};
@@ -531,6 +540,30 @@ static int aspeed_gpio_setup_irqs(struct aspeed_gpio *gpio,
return 0;
}
+static int aspeed_gpio_reset_tolerance(struct gpio_chip *chip,
+ unsigned int offset, bool enable)
+{
+ struct aspeed_gpio *gpio = gpiochip_get_data(chip);
+ const struct aspeed_gpio_bank *bank;
+ unsigned long flags;
+ u32 val;
+
+ bank = to_bank(offset);
+
+ spin_lock_irqsave(&gpio->lock, flags);
+ val = readl(gpio->base + bank->tolerance_regs);
+
+ if (enable)
+ val |= GPIO_BIT(offset);
+ else
+ val &= ~GPIO_BIT(offset);
+
+ writel(val, gpio->base + bank->tolerance_regs);
+ spin_unlock_irqrestore(&gpio->lock, flags);
+
+ return 0;
+}
+
static int aspeed_gpio_request(struct gpio_chip *chip, unsigned int offset)
{
if (!have_gpio(gpiochip_get_data(chip), offset))
@@ -768,6 +801,8 @@ static int aspeed_gpio_set_config(struct gpio_chip *chip, unsigned int offset,
param == PIN_CONFIG_DRIVE_OPEN_SOURCE)
/* Return -ENOTSUPP to trigger emulation, as per datasheet */
return -ENOTSUPP;
+ else if (param == PIN_CONFIG_RESET_TOLERANT)
+ return aspeed_gpio_reset_tolerance(chip, offset, arg);
return -ENOTSUPP;
}
--
2.11.0
^ permalink raw reply related [flat|nested] 22+ messages in thread