[PATCH v2 0/2] leds: gpio: Allow retaining state on shutdown

[PATCH v2 0/2] leds: gpio: Allow retaining state on shutdown

[Andrew Jeffery]

Hello,

LEDs controlled by Baseboard Management Controllers (BMCs) are sometimes
required to retain their state across BMC resets. BMC resets may occur whilst
the host is alive, thus the chassis and host system remain powered up. In these
two patches I define an new devicetree property to describe the behaviour for
GPIO LEDs and add support to the leds-gpio driver.

Changes in v2:
* Expand BMC to Baseboard Management Controller at least once in each commit
  message.

v1: https://lkml.org/lkml/2017/8/25/44

Please review!

Cheers,

Andrew

Andrew Jeffery (2):
  dt-bindings: leds: gpio: Add optional retain-state-shutdown property
  leds: gpio: Allow LED to retain state at shutdown

 Documentation/devicetree/bindings/leds/leds-gpio.txt | 3 +++
 drivers/leds/leds-gpio.c                             | 7 ++++++-
 include/linux/leds.h                                 | 2 ++
 3 files changed, 11 insertions(+), 1 deletion(-)

-- 
2.11.0

[PATCH v2 1/2] dt-bindings: leds: gpio: Add optional retain-state-shutdown property

[Andrew Jeffery]

On Baseboard Management Controller (BMC) systems it's sometimes
necessary for a LED to retain its state across a BMC reset (which is
independent of the host system state). Add a devicetree property to
describe this behaviour. The property would typically be used in
conjunction with 'default-state = "keep"'.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 Documentation/devicetree/bindings/leds/leds-gpio.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/leds/leds-gpio.txt b/Documentation/devicetree/bindings/leds/leds-gpio.txt
index 76535ca37120..a48dda268f81 100644
--- a/Documentation/devicetree/bindings/leds/leds-gpio.txt
+++ b/Documentation/devicetree/bindings/leds/leds-gpio.txt
@@ -18,6 +18,9 @@ LED sub-node properties:
   see Documentation/devicetree/bindings/leds/common.txt
 - retain-state-suspended: (optional) The suspend state can be retained.Such
   as charge-led gpio.
+- retain-state-shutdown: (optional) Retain the state of the LED on shutdown.
+  Useful in BMC systems, for example when the BMC is rebooted while the host
+  remains up.
 - panic-indicator : (optional)
   see Documentation/devicetree/bindings/leds/common.txt
 
-- 
2.11.0

Re: [PATCH v2 1/2] dt-bindings: leds: gpio: Add optional retain-state-shutdown property

[Pavel Machek]

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

On Mon 2017-08-28 09:47:10, Andrew Jeffery wrote:
> On Baseboard Management Controller (BMC) systems it's sometimes
> necessary for a LED to retain its state across a BMC reset (which is
> independent of the host system state). Add a devicetree property to
> describe this behaviour. The property would typically be used in
> conjunction with 'default-state = "keep"'.
> 
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>

Acked-by: Pavel Machek <pavel@ucw.cz>

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v2 1/2] dt-bindings: leds: gpio: Add optional retain-state-shutdown property

[Brandon Wyman]

On Mon, Aug 28, 2017 at 5:19 AM, Pavel Machek <pavel@ucw.cz> wrote:
> On Mon 2017-08-28 09:47:10, Andrew Jeffery wrote:
>> On Baseboard Management Controller (BMC) systems it's sometimes
>> necessary for a LED to retain its state across a BMC reset (which is
>> independent of the host system state). Add a devicetree property to
>> describe this behaviour. The property would typically be used in
>> conjunction with 'default-state = "keep"'.
>>
>> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
>
> Acked-by: Pavel Machek <pavel@ucw.cz>
>
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Tested-by: Brandon Wyman <bjwyman@gmail.com>

Re: [PATCH v2 1/2] dt-bindings: leds: gpio: Add optional retain-state-shutdown property

[Jacek Anaszewski]

Hi Brandon,

On 08/28/2017 11:13 PM, Brandon Wyman wrote:
> On Mon, Aug 28, 2017 at 5:19 AM, Pavel Machek <pavel@ucw.cz> wrote:
>> On Mon 2017-08-28 09:47:10, Andrew Jeffery wrote:
>>> On Baseboard Management Controller (BMC) systems it's sometimes
>>> necessary for a LED to retain its state across a BMC reset (which is
>>> independent of the host system state). Add a devicetree property to
>>> describe this behaviour. The property would typically be used in
>>> conjunction with 'default-state = "keep"'.
>>>
>>> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
>>
>> Acked-by: Pavel Machek <pavel@ucw.cz>
>>
>> --
>> (english) http://www.livejournal.com/~pavelmachek
>> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
> 
> Tested-by: Brandon Wyman <bjwyman@gmail.com>

Added this tag to both patches (1/2 and 2/2), thanks.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v2 1/2] dt-bindings: leds: gpio: Add optional retain-state-shutdown property

[Rob Herring]

On Mon, Aug 28, 2017 at 09:47:10AM +0930, Andrew Jeffery wrote:
> On Baseboard Management Controller (BMC) systems it's sometimes
> necessary for a LED to retain its state across a BMC reset (which is
> independent of the host system state). Add a devicetree property to
> describe this behaviour. The property would typically be used in
> conjunction with 'default-state = "keep"'.

A bit quick on the applying of this...

I don't understand how this works. The BMC usecase is interesting but 
that's fairly irrelevant to the binding. How do you know the GPIO state 
thru a reset? You're doing a core reset, but not reseting the GPIO 
controller?

When you use 'default-state = "keep"' but not this property? Seems like 
the same thing to me. Presumably the bootloader would just distinguish 
between a warm and cold reset to decide whether to re-init the state.

Finally, if we do have this property, why is it GPIO specific. You could 
just as easily have a LED controller IC and want to do the same thing.

Rob

Re: [PATCH v2 1/2] dt-bindings: leds: gpio: Add optional retain-state-shutdown property

[Andrew Jeffery]

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

Hi Rob,

On Thu, 2017-08-31 at 16:28 -0500, Rob Herring wrote:
> On Mon, Aug 28, 2017 at 09:47:10AM +0930, Andrew Jeffery wrote:
> > On Baseboard Management Controller (BMC) systems it's sometimes
> > necessary for a LED to retain its state across a BMC reset (which is
> > independent of the host system state). Add a devicetree property to
> > describe this behaviour. The property would typically be used in
> > conjunction with 'default-state = "keep"'.
> 
> A bit quick on the applying of this...
> 
> I don't understand how this works. The BMC usecase is interesting but 
> that's fairly irrelevant to the binding. How do you know the GPIO state 
> thru a reset?

The answer to that is a property of the system design in my opinion.

> You're doing a core reset, but not reseting the GPIO 
> controller?

Well, in my specific case, the GPIO controller in question isn't on the SoC,
it's off-chip behind an I2C bus. Powering off a BMC doesn't necessarily mean
removing power from its peripherals, so in this case the their state is
maintained.

Alternatively, and again in my specific (Aspeed) case, the on-SoC GPIO
controller can be selectively reset with respect to the rest of the SoC, or
individual GPIO lines can be marked as reset-tolerant. This gives the same
capability and is what you've suggested above.

> 
> When you use 'default-state = "keep"' but not this property? Seems like  the
> same thing to me.

Is that not also true of retain-state-suspend?

The existing behaviour of leds-gpio was to turn the managed LEDs off in the
remove() path. To me this was unexpected behaviour, but it is what it is. My
first pass at the patch (before I sent it out) was to do as you suggest and
only turn it off in the absence of 'default-state = "keep"'.  However there
were quite a number of users of that configuration, and this could be an
unexpected change of behaviour. Given retain-state-suspend was already defined,
it seemed that the non-breaking way to get the behaviour I desired was to
introduce retain-state-shutdown.

Sure, this is dealing with problems in the driver and not describing hardware,
but at this point the behaviour is already in place and people might be relying
on it.

> Presumably the bootloader would just distinguish  between a
> warm and cold reset to decide whether to re-init the state.
> 
> Finally, if we do have this property, why is it GPIO specific. You could 
> just as easily have a LED controller IC and want to do the same thing.

I don't think we're precluded from making it more general even if it's
submitted as specific to leds-gpio? I don't have anything against making it
more generic. I was tossing up about doing so, but went for the specific case
first because we could make it more generic without problems. Going the other
way is harder.

Cheers,

Andrew

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

Re: [PATCH v2 1/2] dt-bindings: leds: gpio: Add optional retain-state-shutdown property

[Jacek Anaszewski]

Hi Andrew and Rob,

On 09/01/2017 01:02 AM, Andrew Jeffery wrote:
> Hi Rob,
> 
> On Thu, 2017-08-31 at 16:28 -0500, Rob Herring wrote:
>> On Mon, Aug 28, 2017 at 09:47:10AM +0930, Andrew Jeffery wrote:
>>> On Baseboard Management Controller (BMC) systems it's sometimes
>>> necessary for a LED to retain its state across a BMC reset (which is
>>> independent of the host system state). Add a devicetree property to
>>> describe this behaviour. The property would typically be used in
>>> conjunction with 'default-state = "keep"'.
>>  
>> A bit quick on the applying of this...
>>  
>> I don't understand how this works. The BMC usecase is interesting but 
>> that's fairly irrelevant to the binding. How do you know the GPIO state 
>> thru a reset?
> 
> The answer to that is a property of the system design in my opinion.
> 
>> You're doing a core reset, but not reseting the GPIO 
>> controller?
> 
> Well, in my specific case, the GPIO controller in question isn't on the SoC,
> it's off-chip behind an I2C bus. Powering off a BMC doesn't necessarily mean
> removing power from its peripherals, so in this case the their state is
> maintained.
> 
> Alternatively, and again in my specific (Aspeed) case, the on-SoC GPIO
> controller can be selectively reset with respect to the rest of the SoC, or
> individual GPIO lines can be marked as reset-tolerant. This gives the same
> capability and is what you've suggested above.
> 
>>  
>> When you use 'default-state = "keep"' but not this property? Seems like  the
>> same thing to me.
> 
> Is that not also true of retain-state-suspend?
> 
> The existing behaviour of leds-gpio was to turn the managed LEDs off in the
> remove() path. To me this was unexpected behaviour, but it is what it is. My
> first pass at the patch (before I sent it out) was to do as you suggest and
> only turn it off in the absence of 'default-state = "keep"'.  However there
> were quite a number of users of that configuration, and this could be an
> unexpected change of behaviour. Given retain-state-suspend was already defined,
> it seemed that the non-breaking way to get the behaviour I desired was to
> introduce retain-state-shutdown.
> 
> Sure, this is dealing with problems in the driver and not describing hardware,
> but at this point the behaviour is already in place and people might be relying
> on it.
> 
>> Presumably the bootloader would just distinguish  between a
>> warm and cold reset to decide whether to re-init the state.
>>  
>> Finally, if we do have this property, why is it GPIO specific. You could 
>> just as easily have a LED controller IC and want to do the same thing.
> 
> I don't think we're precluded from making it more general even if it's
> submitted as specific to leds-gpio? I don't have anything against making it
> more generic. I was tossing up about doing so, but went for the specific case
> first because we could make it more generic without problems. Going the other
> way is harder.

This explanation looks reasonable. I'll wait two more days before
sending LED 4.14 updates upstream.

It would be nice to have clear Rob's statement about this patch set -
either ack or NACK.

-- 
Best regards,
Jacek Anaszewski

[PATCH v2 2/2] leds: gpio: Allow LED to retain state at shutdown

[Andrew Jeffery]

In some systems, such as Baseboard Management Controllers (BMCs), we
want to retain the state of LEDs across a reboot of the BMC (whilst the
host remains up). Implement support for the retain-state-shutdown
devicetree property in leds-gpio.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
Acked-by: Pavel Machek <pavel@ucw.cz>
---
 drivers/leds/leds-gpio.c | 7 ++++++-
 include/linux/leds.h     | 2 ++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
index e753ba93ba1e..764c31301f90 100644
--- a/drivers/leds/leds-gpio.c
+++ b/drivers/leds/leds-gpio.c
@@ -134,6 +134,8 @@ static int create_gpio_led(const struct gpio_led *template,
 		led_dat->cdev.flags |= LED_CORE_SUSPENDRESUME;
 	if (template->panic_indicator)
 		led_dat->cdev.flags |= LED_PANIC_INDICATOR;
+	if (template->retain_state_shutdown)
+		led_dat->cdev.flags |= LED_RETAIN_AT_SHUTDOWN;
 
 	ret = gpiod_direction_output(led_dat->gpiod, state);
 	if (ret < 0)
@@ -205,6 +207,8 @@ static struct gpio_leds_priv *gpio_leds_create(struct platform_device *pdev)
 
 		if (fwnode_property_present(child, "retain-state-suspended"))
 			led.retain_state_suspended = 1;
+		if (fwnode_property_present(child, "retain-state-shutdown"))
+			led.retain_state_shutdown = 1;
 		if (fwnode_property_present(child, "panic-indicator"))
 			led.panic_indicator = 1;
 
@@ -267,7 +271,8 @@ static void gpio_led_shutdown(struct platform_device *pdev)
 	for (i = 0; i < priv->num_leds; i++) {
 		struct gpio_led_data *led = &priv->leds[i];
 
-		gpio_led_set(&led->cdev, LED_OFF);
+		if (!(led->cdev.flags & LED_RETAIN_AT_SHUTDOWN))
+			gpio_led_set(&led->cdev, LED_OFF);
 	}
 }
 
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 64c56d454f7d..bf6db4fe895b 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -49,6 +49,7 @@ struct led_classdev {
 #define LED_HW_PLUGGABLE	(1 << 19)
 #define LED_PANIC_INDICATOR	(1 << 20)
 #define LED_BRIGHT_HW_CHANGED	(1 << 21)
+#define LED_RETAIN_AT_SHUTDOWN	(1 << 22)
 
 	/* set_brightness_work / blink_timer flags, atomic, private. */
 	unsigned long		work_flags;
@@ -392,6 +393,7 @@ struct gpio_led {
 	unsigned	retain_state_suspended : 1;
 	unsigned	panic_indicator : 1;
 	unsigned	default_state : 2;
+	unsigned	retain_state_shutdown : 1;
 	/* default_state should be one of LEDS_GPIO_DEFSTATE_(ON|OFF|KEEP) */
 	struct gpio_desc *gpiod;
 };
-- 
2.11.0

Re: [PATCH v2 0/2] leds: gpio: Allow retaining state on shutdown

[Jacek Anaszewski]

Hi Andrew,

Thanks for the update.

On 08/28/2017 02:17 AM, Andrew Jeffery wrote:
> Hello,
> 
> LEDs controlled by Baseboard Management Controllers (BMCs) are sometimes
> required to retain their state across BMC resets. BMC resets may occur whilst
> the host is alive, thus the chassis and host system remain powered up. In these
> two patches I define an new devicetree property to describe the behaviour for
> GPIO LEDs and add support to the leds-gpio driver.
> 
> Changes in v2:
> * Expand BMC to Baseboard Management Controller at least once in each commit
>   message.
> 
> v1: https://lkml.org/lkml/2017/8/25/44
> 
> Please review!
> 
> Cheers,
> 
> Andrew
> 
> Andrew Jeffery (2):
>   dt-bindings: leds: gpio: Add optional retain-state-shutdown property
>   leds: gpio: Allow LED to retain state at shutdown
> 
>  Documentation/devicetree/bindings/leds/leds-gpio.txt | 3 +++
>  drivers/leds/leds-gpio.c                             | 7 ++++++-
>  include/linux/leds.h                                 | 2 ++
>  3 files changed, 11 insertions(+), 1 deletion(-)
> 

Patchset applied to the for-next branch of linux-leds.git, thanks.

-- 
Best regards,
Jacek Anaszewski