[PATCH 0/2] gpio: Expose reset tolerance capability

[PATCH 0/2] gpio: Expose reset tolerance capability

[Andrew Jeffery]

Hello,

This short series enables GPIO state persistence across controller resets,
modifying gpiolib to expose control of the behaviour and providing an initial
implementation in the Aspeed GPIO controller.

This is a follow-up to the initial RFC series[1], which had some undesirable
features (and bugs).

>From the RFC, I've:

1. Dropped the idea of distinguishing between sleep and reset cases[2]
2. Changed the default behaviour from transitory to persistent (must explicitly
   request transitory behaviour for controllers supporting reset tolerance)[3]
3. Dropped the ability for userspace to configure whether or not the exported
   GPIO is persistent[4][5]

On point 2, whilst the patch modifying the sysfs interface ABI no-longer lives,
gpiolib-sysfs still requires modification to provide consistent behaviour with
the chardev interface in providing a default-persistent GPIO on export.

Regarding point 3, I figured this is something we can add later once there's
been some discussion on how much of a good idea it is.

As some of the capabilities in the RFC have been dropped, I've squashed the
gpiolib support into a single patch as it's not really that complex.

Hopefully that addresses the feedback on the RFC. Please review!

Andrew

[1] https://www.spinics.net/lists/devicetree/msg199559.html
[2] https://www.spinics.net/lists/devicetree/msg199566.html
[3] https://www.spinics.net/lists/devicetree/msg199574.html
[4] https://www.spinics.net/lists/devicetree/msg199568.html
[5] https://www.spinics.net/lists/devicetree/msg199569.html 

Andrew Jeffery (2):
  gpio: gpiolib: Expand sleep tolerance to include controller reset
  gpio: aspeed: Add support for reset tolerance

 drivers/gpio/gpio-aspeed.c              | 39 ++++++++++++++++++++--
 drivers/gpio/gpiolib-of.c               |  6 ++--
 drivers/gpio/gpiolib-sysfs.c            | 14 +++++---
 drivers/gpio/gpiolib.c                  | 58 ++++++++++++++++++++++++++++++---
 drivers/gpio/gpiolib.h                  |  2 +-
 include/dt-bindings/gpio/gpio.h         |  6 ++--
 include/linux/gpio/consumer.h           |  8 +++++
 include/linux/gpio/machine.h            |  4 +--
 include/linux/of_gpio.h                 |  2 +-
 include/linux/pinctrl/pinconf-generic.h |  2 ++
 10 files changed, 121 insertions(+), 20 deletions(-)

-- 
2.11.0

[PATCH 1/2] gpio: gpiolib: Expand sleep tolerance to include controller reset

[Andrew Jeffery]

Reset tolerance is added to gpiolib with the introduction of a new
pinconf parameter propagating the request to hardware. The existing
persistence support for sleep is augmented to include reset tolerance
if the GPIO driver provides it. Persistence continues to be enabled by
default; in-kernel consumers can opt out, but userspace (currently) does
not have a choice.

The *_SLEEP_MAY_LOSE_VALUE and *_SLEEP_MAINTAIN_VALUE symbols are
renamed, dropping the SLEEP prefix to reflect that the concept is no
longer sleep-specific.  I feel that renaming to just *_MAY_LOSE_VALUE
could initially be misinterpreted, so I've further changed the symbols
to *_TRANSITORY and *_PERSISTENT to address this.

The sysfs interface is modified only to keep consistency with the
chardev interface in enforcing persistence for userspace exports.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
I'm not wedded to the names 'transitory' and 'persistent', so feel free to
paint the bikeshed some other colour.

 drivers/gpio/gpiolib-of.c               |  6 ++--
 drivers/gpio/gpiolib-sysfs.c            | 14 +++++---
 drivers/gpio/gpiolib.c                  | 58 ++++++++++++++++++++++++++++++---
 drivers/gpio/gpiolib.h                  |  2 +-
 include/dt-bindings/gpio/gpio.h         |  6 ++--
 include/linux/gpio/consumer.h           |  8 +++++
 include/linux/gpio/machine.h            |  4 +--
 include/linux/of_gpio.h                 |  2 +-
 include/linux/pinctrl/pinconf-generic.h |  2 ++
 9 files changed, 84 insertions(+), 18 deletions(-)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index e0d59e61b52f..4a2b8d3397c7 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -153,8 +153,8 @@ struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
 			*flags |= GPIO_OPEN_SOURCE;
 	}
 
-	if (of_flags & OF_GPIO_SLEEP_MAY_LOSE_VALUE)
-		*flags |= GPIO_SLEEP_MAY_LOSE_VALUE;
+	if (of_flags & OF_GPIO_TRANSITORY)
+		*flags |= GPIO_TRANSITORY;
 
 	return desc;
 }
@@ -214,6 +214,8 @@ static struct gpio_desc *of_parse_own_gpio(struct device_node *np,
 
 	if (xlate_flags & OF_GPIO_ACTIVE_LOW)
 		*lflags |= GPIO_ACTIVE_LOW;
+	if (xlate_flags & OF_GPIO_TRANSITORY)
+		*lflags |= GPIO_TRANSITORY;
 
 	if (of_property_read_bool(np, "input"))
 		*dflags |= GPIOD_IN;
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 3f454eaf2101..0bd472ffb072 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -474,11 +474,15 @@ static ssize_t export_store(struct class *class,
 			status = -ENODEV;
 		goto done;
 	}
-	status = gpiod_export(desc, true);
-	if (status < 0)
-		gpiod_free(desc);
-	else
-		set_bit(FLAG_SYSFS, &desc->flags);
+
+	status = gpiod_set_transitory(desc, false);
+	if (!status) {
+		status = gpiod_export(desc, true);
+		if (status < 0)
+			gpiod_free(desc);
+		else
+			set_bit(FLAG_SYSFS, &desc->flags);
+	}
 
 done:
 	if (status)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 3827f0767101..fb29c81225f3 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -503,6 +503,10 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
 		if (lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE)
 			set_bit(FLAG_OPEN_SOURCE, &desc->flags);
 
+		ret = gpiod_set_transitory(desc, false);
+		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".
@@ -2424,6 +2428,46 @@ int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce)
 EXPORT_SYMBOL_GPL(gpiod_set_debounce);
 
 /**
+ * gpiod_set_transitory - Lose or retain GPIO state on suspend or reset
+ * @desc: descriptor of the GPIO for which to configure persistence
+ * @transitory: True to lose state on suspend or reset, false for persistence
+ *
+ * Returns:
+ * 0 on success, otherwise a negative error code.
+ */
+int gpiod_set_transitory(struct gpio_desc *desc, bool transitory)
+{
+	struct gpio_chip *chip;
+	unsigned long packed;
+	int gpio;
+	int rc;
+
+	/* Handle FLAG_TRANSITORY first for suspend case */
+	if (transitory)
+		set_bit(FLAG_TRANSITORY, &desc->flags);
+	else
+		clear_bit(FLAG_TRANSITORY, &desc->flags);
+
+	/* Configure reset persistence if the controller supports it */
+	chip = desc->gdev->chip;
+	if (!chip->set_config)
+		return 0;
+
+	packed = pinconf_to_config_packed(PIN_CONFIG_RESET_TOLERANT,
+					  !transitory);
+	gpio = gpio_chip_hwgpio(desc);
+	rc = chip->set_config(chip, gpio, packed);
+	if (rc == -ENOTSUPP) {
+		dev_dbg(&desc->gdev->dev, "Reset tolerance not supported for GPIO %d\n",
+				gpio);
+		return 0;
+	}
+
+	return rc;
+}
+EXPORT_SYMBOL_GPL(gpiod_set_transitory);
+
+/**
  * gpiod_is_active_low - test whether a GPIO is active-low or not
  * @desc: the gpio descriptor to test
  *
@@ -3010,8 +3054,7 @@ bool gpiochip_line_is_persistent(struct gpio_chip *chip, unsigned int offset)
 	if (offset >= chip->ngpio)
 		return false;
 
-	return !test_bit(FLAG_SLEEP_MAY_LOSE_VALUE,
-			 &chip->gpiodev->descs[offset].flags);
+	return !test_bit(FLAG_TRANSITORY, &chip->gpiodev->descs[offset].flags);
 }
 EXPORT_SYMBOL_GPL(gpiochip_line_is_persistent);
 
@@ -3435,8 +3478,10 @@ int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
 		set_bit(FLAG_OPEN_DRAIN, &desc->flags);
 	if (lflags & GPIO_OPEN_SOURCE)
 		set_bit(FLAG_OPEN_SOURCE, &desc->flags);
-	if (lflags & GPIO_SLEEP_MAY_LOSE_VALUE)
-		set_bit(FLAG_SLEEP_MAY_LOSE_VALUE, &desc->flags);
+
+	status = gpiod_set_transitory(desc, (lflags & GPIO_TRANSITORY));
+	if (status < 0)
+		return status;
 
 	/* No particular flag request, return here... */
 	if (!(dflags & GPIOD_FLAGS_BIT_DIR_SET)) {
@@ -3550,6 +3595,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 transitory = false;
 	int ret;
 
 	if (!fwnode)
@@ -3564,6 +3610,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;
+			transitory = flags & OF_GPIO_TRANSITORY;
 		}
 	} else if (is_acpi_node(fwnode)) {
 		struct acpi_gpio_info info;
@@ -3594,6 +3641,9 @@ struct gpio_desc *fwnode_get_named_gpiod(struct fwnode_handle *fwnode,
 			lflags |= GPIO_OPEN_SOURCE;
 	}
 
+	if (transitory)
+		lflags |= GPIO_TRANSITORY;
+
 	ret = gpiod_configure_flags(desc, propname, lflags, dflags);
 	if (ret < 0) {
 		gpiod_put(desc);
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index af48322839c3..736b189d1bb5 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -205,7 +205,7 @@ struct gpio_desc {
 #define FLAG_OPEN_SOURCE 8	/* Gpio is open source type */
 #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_TRANSITORY 12	/* GPIO may lose value in sleep or reset */
 
 	/* Connection label */
 	const char		*label;
diff --git a/include/dt-bindings/gpio/gpio.h b/include/dt-bindings/gpio/gpio.h
index 70de5b7a6c9b..43dc230d3921 100644
--- a/include/dt-bindings/gpio/gpio.h
+++ b/include/dt-bindings/gpio/gpio.h
@@ -28,8 +28,8 @@
 #define GPIO_OPEN_DRAIN (GPIO_SINGLE_ENDED | GPIO_LINE_OPEN_DRAIN)
 #define GPIO_OPEN_SOURCE (GPIO_SINGLE_ENDED | GPIO_LINE_OPEN_SOURCE)
 
-/* Bit 3 express GPIO suspend/resume persistence */
-#define GPIO_SLEEP_MAINTAIN_VALUE 0
-#define GPIO_SLEEP_MAY_LOSE_VALUE 8
+/* Bit 3 express GPIO suspend/resume and reset persistence */
+#define GPIO_PERSISTENT 0
+#define GPIO_TRANSITORY 8
 
 #endif
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index d4920ec1f1da..d77e88c96f38 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -132,6 +132,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_transitory(struct gpio_desc *desc, bool transitory);
 
 int gpiod_is_active_low(const struct gpio_desc *desc);
 int gpiod_cansleep(const struct gpio_desc *desc);
@@ -424,6 +425,13 @@ static inline int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce)
 	return -ENOSYS;
 }
 
+static inline int gpiod_set_transitory(struct gpio_desc *desc, bool transitory)
+{
+	/* 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/machine.h b/include/linux/gpio/machine.h
index 5e9f294c29eb..33a97ede6404 100644
--- a/include/linux/gpio/machine.h
+++ b/include/linux/gpio/machine.h
@@ -9,8 +9,8 @@ enum gpio_lookup_flags {
 	GPIO_ACTIVE_LOW = (1 << 0),
 	GPIO_OPEN_DRAIN = (1 << 1),
 	GPIO_OPEN_SOURCE = (1 << 2),
-	GPIO_SLEEP_MAINTAIN_VALUE = (0 << 3),
-	GPIO_SLEEP_MAY_LOSE_VALUE = (1 << 3),
+	GPIO_PERSISTENT = (0 << 3),
+	GPIO_TRANSITORY = (1 << 3),
 };
 
 /**
diff --git a/include/linux/of_gpio.h b/include/linux/of_gpio.h
index 1fe205582111..18a7f03e1182 100644
--- a/include/linux/of_gpio.h
+++ b/include/linux/of_gpio.h
@@ -31,7 +31,7 @@ enum of_gpio_flags {
 	OF_GPIO_ACTIVE_LOW = 0x1,
 	OF_GPIO_SINGLE_ENDED = 0x2,
 	OF_GPIO_OPEN_DRAIN = 0x4,
-	OF_GPIO_SLEEP_MAY_LOSE_VALUE = 0x8,
+	OF_GPIO_TRANSITORY = 0x8,
 };
 
 #ifdef CONFIG_OF_GPIO
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

Re: [PATCH 1/2] gpio: gpiolib: Expand sleep tolerance to include controller reset

[Charles Keepax]

On Wed, Oct 25, 2017 at 03:34:16PM +1030, Andrew Jeffery wrote:
> Reset tolerance is added to gpiolib with the introduction of a new
> pinconf parameter propagating the request to hardware. The existing
> persistence support for sleep is augmented to include reset tolerance
> if the GPIO driver provides it. Persistence continues to be enabled by
> default; in-kernel consumers can opt out, but userspace (currently) does
> not have a choice.
> 
> The *_SLEEP_MAY_LOSE_VALUE and *_SLEEP_MAINTAIN_VALUE symbols are
> renamed, dropping the SLEEP prefix to reflect that the concept is no
> longer sleep-specific.  I feel that renaming to just *_MAY_LOSE_VALUE
> could initially be misinterpreted, so I've further changed the symbols
> to *_TRANSITORY and *_PERSISTENT to address this.
> 
> The sysfs interface is modified only to keep consistency with the
> chardev interface in enforcing persistence for userspace exports.
> 
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
> I'm not wedded to the names 'transitory' and 'persistent', so feel free to
> paint the bikeshed some other colour.
> 

I am happy enough with the names.

>  drivers/gpio/gpiolib-of.c               |  6 ++--
>  drivers/gpio/gpiolib-sysfs.c            | 14 +++++---
>  drivers/gpio/gpiolib.c                  | 58 ++++++++++++++++++++++++++++++---
>  drivers/gpio/gpiolib.h                  |  2 +-
>  include/dt-bindings/gpio/gpio.h         |  6 ++--
>  include/linux/gpio/consumer.h           |  8 +++++
>  include/linux/gpio/machine.h            |  4 +--
>  include/linux/of_gpio.h                 |  2 +-
>  include/linux/pinctrl/pinconf-generic.h |  2 ++
>  9 files changed, 84 insertions(+), 18 deletions(-)

> @@ -2424,6 +2428,46 @@ int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce)
>  EXPORT_SYMBOL_GPL(gpiod_set_debounce);
>  
>  /**
> + * gpiod_set_transitory - Lose or retain GPIO state on suspend or reset
> + * @desc: descriptor of the GPIO for which to configure persistence
> + * @transitory: True to lose state on suspend or reset, false for persistence
> + *
> + * Returns:
> + * 0 on success, otherwise a negative error code.
> + */
> +int gpiod_set_transitory(struct gpio_desc *desc, bool transitory)
> +{
> +	struct gpio_chip *chip;
> +	unsigned long packed;
> +	int gpio;
> +	int rc;
> +
> +	/* Handle FLAG_TRANSITORY first for suspend case */
> +	if (transitory)
> +		set_bit(FLAG_TRANSITORY, &desc->flags);
> +	else
> +		clear_bit(FLAG_TRANSITORY, &desc->flags);
> +
> +	/* Configure reset persistence if the controller supports it */
> +	chip = desc->gdev->chip;
> +	if (!chip->set_config)
> +		return 0;
> +
> +	packed = pinconf_to_config_packed(PIN_CONFIG_RESET_TOLERANT,
> +					  !transitory);
> +	gpio = gpio_chip_hwgpio(desc);
> +	rc = chip->set_config(chip, gpio, packed);
> +	if (rc == -ENOTSUPP) {
> +		dev_dbg(&desc->gdev->dev, "Reset tolerance not supported for GPIO %d\n",
> +				gpio);
> +		return 0;
> +	}
> +
> +	return rc;

This means that if we have a set_config we are directly
equating PERSISTENT to RESET_TOLERANT, which seems wrong to
me. I might have a GPIO on a controller with pinconf that
doesn't have anything to do with RESET_TOLERANT. Should the
PIN_CONFIG_RESET_TOLERANT, really just be PIN_CONFIG_PERSISTENT?
And then its upto the driver what persistence means for that
chip?

Thanks,
Charles

Re: [PATCH 1/2] gpio: gpiolib: Expand sleep tolerance to include controller reset

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 6227 bytes --]

On Wed, 2017-10-25 at 09:32 +0100, Charles Keepax wrote:
> On Wed, Oct 25, 2017 at 03:34:16PM +1030, Andrew Jeffery wrote:
> Reset tolerance is added to gpiolib with the introduction of a new
> pinconf parameter propagating the request to hardware. The existing
> persistence support for sleep is augmented to include reset tolerance
> if the GPIO driver provides it. Persistence continues to be enabled by
> default; in-kernel consumers can opt out, but userspace (currently) does
> not have a choice.
> 
> The *_SLEEP_MAY_LOSE_VALUE and *_SLEEP_MAINTAIN_VALUE symbols are
> renamed, dropping the SLEEP prefix to reflect that the concept is no
> longer sleep-specific.  I feel that renaming to just *_MAY_LOSE_VALUE
> could initially be misinterpreted, so I've further changed the symbols
> to *_TRANSITORY and *_PERSISTENT to address this.
> 
> The sysfs interface is modified only to keep consistency with the
> chardev interface in enforcing persistence for userspace exports.
> 
> Signed-off-by: Andrew Jeffery <andrew-zrmu5oMJ5Fs@public.gmane.org>
> ---
> I'm not wedded to the names 'transitory' and 'persistent', so feel free to
> paint the bikeshed some other colour.
> 
> 
> I am happy enough with the names.
> 
>  drivers/gpio/gpiolib-of.c               |  6 ++--
>  drivers/gpio/gpiolib-sysfs.c            | 14 +++++---
>  drivers/gpio/gpiolib.c                  | 58 ++++++++++++++++++++++++++++++---
>  drivers/gpio/gpiolib.h                  |  2 +-
>  include/dt-bindings/gpio/gpio.h         |  6 ++--
>  include/linux/gpio/consumer.h           |  8 +++++
>  include/linux/gpio/machine.h            |  4 +--
>  include/linux/of_gpio.h                 |  2 +-
>  include/linux/pinctrl/pinconf-generic.h |  2 ++
>  9 files changed, 84 insertions(+), 18 deletions(-)
> @@ -2424,6 +2428,46 @@ int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce)
>  EXPORT_SYMBOL_GPL(gpiod_set_debounce);
>  
>  /**
> + * gpiod_set_transitory - Lose or retain GPIO state on suspend or reset
> + * @desc: descriptor of the GPIO for which to configure persistence
> + * @transitory: True to lose state on suspend or reset, false for persistence
> + *
> + * Returns:
> + * 0 on success, otherwise a negative error code.
> + */
> +int gpiod_set_transitory(struct gpio_desc *desc, bool transitory)
> +{
> +	struct gpio_chip *chip;
> +	unsigned long packed;
> +	int gpio;
> +	int rc;
> +
> +	/* Handle FLAG_TRANSITORY first for suspend case */
> +	if (transitory)
> +		set_bit(FLAG_TRANSITORY, &desc->flags);
> +	else
> +		clear_bit(FLAG_TRANSITORY, &desc->flags);
> +
> +	/* Configure reset persistence if the controller supports it */
> +	chip = desc->gdev->chip;
> +	if (!chip->set_config)
> +		return 0;
> +
> +	packed = pinconf_to_config_packed(PIN_CONFIG_RESET_TOLERANT,
> +					  !transitory);
> +	gpio = gpio_chip_hwgpio(desc);
> +	rc = chip->set_config(chip, gpio, packed);
> +	if (rc == -ENOTSUPP) {
> +		dev_dbg(&desc->gdev->dev, "Reset tolerance not supported for GPIO %d\n",
> +				gpio);
> +		return 0;
> +	}
> +
> +	return rc;
> 
> This means that if we have a set_config we are directly
> equating PERSISTENT to RESET_TOLERANT, which seems wrong to
> me. I might have a GPIO on a controller with pinconf that
> doesn't have anything to do with RESET_TOLERANT. Should the
> PIN_CONFIG_RESET_TOLERANT, really just be PIN_CONFIG_PERSISTENT?
> And then its upto the driver what persistence means for that
> chip?

Right, maybe I'm tying the design in a bit too closely with the Aspeed hardware
again. I'm coming out in favour of changing it based on my thoughts below.

However, I want to understand whether there are alternatives to reset tolerance
as a property. The obvious one is sleep persistence as implemented in the
Arizona driver, but it didn't take the set_config() route with its initial
implementation.

The set_config() approach was largely driven by the sysfs patch in
the RFC series, because I wanted a way to propagate the desired property
to hardware without affecting (or again calling through the handlers for) the
export state or direction. It seemed to come out neat enough in general so I
kept it.

Were you thinking of using set_config() to control the Arizona's behaviour and
do something other than the calls to gpiochip_line_is_persistent() in
arizona_gpio_direction_{in,out}() (as in, keep the state inside the device
driver rather than querying gpiolib where the state is currently stored)? That
would seem neat in that it gives us one method of setting both persistence
types (and in this case I should rename the pinconf parameter). With that we
could probably get rid of gpiochip_line_is_persistent(). A small downside is
some duplicated state (we probably still want to keep FLAG_TRANSITORY in
gpiolib).

Would it be reasonable for other drivers to implement sleep persistence in the
same manner as the Arizona driver currently does? If that's the case, I don't
think there is a third tolerance option in addition to sleep and reset, and so
we might not need to rename the pinconf parameter.

Finally, we could implement reset persistence in the same manner as the
Arizona's sleep persistence (with gpiochip_line_is_persistent()) given the
sysfs patch has been thrown away.

So to summarise, having rambled a bit, I see the situation as:

1. Rename the pinconf parameter, and patch the Arizona driver to use
   set_config() and hold sleep persistence state internally.
2. Drop the pinconf parameter and do something similar to the Arizona's sleep
   persistence for reset persistence in the Aspeed driver.
3. Keep things as proposed are and live with two approaches to persistence

Point 3 seems like the least desirable. I'm leaning towards 1. I could probably
live with 2. after experimenting with it and confirming it's workable for me.

If you think 1. is the way to go are you happy for me to send a patch to the
Arizona driver implementing the change?

Thanks for the feedback!

Andrew

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 1/2] gpio: gpiolib: Expand sleep tolerance to include controller reset

[Charles Keepax]

On Thu, Oct 26, 2017 at 11:10:53AM +1030, Andrew Jeffery wrote:
> On Wed, 2017-10-25 at 09:32 +0100, Charles Keepax wrote:
> > On Wed, Oct 25, 2017 at 03:34:16PM +1030, Andrew Jeffery wrote:
> > This means that if we have a set_config we are directly
> > equating PERSISTENT to RESET_TOLERANT, which seems wrong to
> > me. I might have a GPIO on a controller with pinconf that
> > doesn't have anything to do with RESET_TOLERANT. Should the
> > PIN_CONFIG_RESET_TOLERANT, really just be PIN_CONFIG_PERSISTENT?
> > And then its upto the driver what persistence means for that
> > chip?
> 
> Right, maybe I'm tying the design in a bit too closely with the Aspeed hardware
> again. I'm coming out in favour of changing it based on my thoughts below.
> 
> However, I want to understand whether there are alternatives to reset tolerance
> as a property. The obvious one is sleep persistence as implemented in the
> Arizona driver, but it didn't take the set_config() route with its initial
> implementation.
> 
> The set_config() approach was largely driven by the sysfs patch in
> the RFC series, because I wanted a way to propagate the desired property
> to hardware without affecting (or again calling through the handlers for) the
> export state or direction. It seemed to come out neat enough in general so I
> kept it.
> 
> Were you thinking of using set_config() to control the Arizona's behaviour and
> do something other than the calls to gpiochip_line_is_persistent() in
> arizona_gpio_direction_{in,out}() (as in, keep the state inside the device
> driver rather than querying gpiolib where the state is currently stored)? That
> would seem neat in that it gives us one method of setting both persistence
> types (and in this case I should rename the pinconf parameter). With that we
> could probably get rid of gpiochip_line_is_persistent(). A small downside is
> some duplicated state (we probably still want to keep FLAG_TRANSITORY in
> gpiolib).
> 
> Would it be reasonable for other drivers to implement sleep persistence in the
> same manner as the Arizona driver currently does? If that's the case, I don't
> think there is a third tolerance option in addition to sleep and reset, and so
> we might not need to rename the pinconf parameter.
> 
> Finally, we could implement reset persistence in the same manner as the
> Arizona's sleep persistence (with gpiochip_line_is_persistent()) given the
> sysfs patch has been thrown away.
> 
> So to summarise, having rambled a bit, I see the situation as:
> 
> 1. Rename the pinconf parameter, and patch the Arizona driver to use
>    set_config() and hold sleep persistence state internally.
> 2. Drop the pinconf parameter and do something similar to the Arizona's sleep
>    persistence for reset persistence in the Aspeed driver.
> 3. Keep things as proposed are and live with two approaches to persistence
> 
> Point 3 seems like the least desirable. I'm leaning towards 1. I could probably
> live with 2. after experimenting with it and confirming it's workable for me.
> 
> If you think 1. is the way to go are you happy for me to send a patch to the
> Arizona driver implementing the change?
> 

I guess the design was more playing to the fact that in the
Arizona case we arn't really setting any register bit for the
persisence, we are just not letting the CODEC power down whilst
the GPIO is in use. The config approach does make more sense in
the case where you are actually setting register bits.

I don't have any great objection to keeping the persistence state
in the Arizona GPIO driver, but it does seem to be duplicating
the state a little. Also an early version of the patch chain did
that and we decided it would better in the core.

I am not opposed to option 3 either. For example open-drain has
a similar setup where there is a pin config option for it and
also a GPIO function call to check for its presence on a specific
GPIO. Certainly my objection was just that we were equating the
very generic property to a very specific pinconf.

I think I would probably be keen to see what Linus's thoughts
are, but my feelings are really your patch is probably good if we
just rename the RESET_TOLERANT pinconf to something a little more
generic.

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

Re: [PATCH 1/2] gpio: gpiolib: Expand sleep tolerance to include controller reset

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 4658 bytes --]

On Thu, 2017-10-26 at 10:03 +0100, Charles Keepax wrote:
> On Thu, Oct 26, 2017 at 11:10:53AM +1030, Andrew Jeffery wrote:
> > On Wed, 2017-10-25 at 09:32 +0100, Charles Keepax wrote:
> > > On Wed, Oct 25, 2017 at 03:34:16PM +1030, Andrew Jeffery wrote:
> > > This means that if we have a set_config we are directly
> > > equating PERSISTENT to RESET_TOLERANT, which seems wrong to
> > > me. I might have a GPIO on a controller with pinconf that
> > > doesn't have anything to do with RESET_TOLERANT. Should the
> > > PIN_CONFIG_RESET_TOLERANT, really just be PIN_CONFIG_PERSISTENT?
> > > And then its upto the driver what persistence means for that
> > > chip?
> > 
> > Right, maybe I'm tying the design in a bit too closely with the Aspeed hardware
> > again. I'm coming out in favour of changing it based on my thoughts below.
> > 
> > However, I want to understand whether there are alternatives to reset tolerance
> > as a property. The obvious one is sleep persistence as implemented in the
> > Arizona driver, but it didn't take the set_config() route with its initial
> > implementation.
> > 
> > The set_config() approach was largely driven by the sysfs patch in
> > the RFC series, because I wanted a way to propagate the desired property
> > to hardware without affecting (or again calling through the handlers for) the
> > export state or direction. It seemed to come out neat enough in general so I
> > kept it.
> > 
> > Were you thinking of using set_config() to control the Arizona's behaviour and
> > do something other than the calls to gpiochip_line_is_persistent() in
> > arizona_gpio_direction_{in,out}() (as in, keep the state inside the device
> > driver rather than querying gpiolib where the state is currently stored)? That
> > would seem neat in that it gives us one method of setting both persistence
> > types (and in this case I should rename the pinconf parameter). With that we
> > could probably get rid of gpiochip_line_is_persistent(). A small downside is
> > some duplicated state (we probably still want to keep FLAG_TRANSITORY in
> > gpiolib).
> > 
> > Would it be reasonable for other drivers to implement sleep persistence in the
> > same manner as the Arizona driver currently does? If that's the case, I don't
> > think there is a third tolerance option in addition to sleep and reset, and so
> > we might not need to rename the pinconf parameter.
> > 
> > Finally, we could implement reset persistence in the same manner as the
> > Arizona's sleep persistence (with gpiochip_line_is_persistent()) given the
> > sysfs patch has been thrown away.
> > 
> > So to summarise, having rambled a bit, I see the situation as:
> > 
> > 1. Rename the pinconf parameter, and patch the Arizona driver to use
> >    set_config() and hold sleep persistence state internally.
> > 2. Drop the pinconf parameter and do something similar to the Arizona's sleep
> >    persistence for reset persistence in the Aspeed driver.
> > 3. Keep things as proposed are and live with two approaches to persistence
> > 
> > Point 3 seems like the least desirable. I'm leaning towards 1. I could probably
> > live with 2. after experimenting with it and confirming it's workable for me.
> > 
> > If you think 1. is the way to go are you happy for me to send a patch to the
> > Arizona driver implementing the change?
> > 
> 
> I guess the design was more playing to the fact that in the
> Arizona case we arn't really setting any register bit for the
> persisence, we are just not letting the CODEC power down whilst
> the GPIO is in use. The config approach does make more sense in
> the case where you are actually setting register bits.
> 
> I don't have any great objection to keeping the persistence state
> in the Arizona GPIO driver, but it does seem to be duplicating
> the state a little. Also an early version of the patch chain did
> that and we decided it would better in the core.
> 
> I am not opposed to option 3 either. For example open-drain has
> a similar setup where there is a pin config option for it and
> also a GPIO function call to check for its presence on a specific
> GPIO. Certainly my objection was just that we were equating the
> very generic property to a very specific pinconf.
> 
> I think I would probably be keen to see what Linus's thoughts
> are, but my feelings are really your patch is probably good if we
> just rename the RESET_TOLERANT pinconf to something a little more
> generic.

Okay, well in that case I'll do the rename and send out the patches, it
is simple enough. Thanks again for your feedback.

Andrew

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[PATCH 2/2] gpio: aspeed: Add support for reset tolerance

[Andrew Jeffery]

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 00dc1c020198..856498925538 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", "", "", "" },
 	},
 };
@@ -534,6 +543,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))
@@ -771,6 +804,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