| Message ID | 20170828001711.19662-2-andrew@aj.id.au |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | leds: gpio: Allow retaining state on shutdown | expand |
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>
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> -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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.
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
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(+)