| Message ID | 1459166834-26015-6-git-send-email-contact@paulk.fr |
|---|---|
| State | Accepted |
| Commit | 523849a0887c2d400d379eb0bce5ec74fc7a4dcd |
| Delegated to: | Tom Rini |
| Headers | show |
On Mon, Mar 28, 2016 at 02:07:13PM +0200, Paul Kocialkowski wrote: > With the previous implementation, rebooting without registering a recognized > reboot mode would end up with U-Boot checking for a valid power-on reason, which > might result in the device turning off (e.g. with no USB cable attached and no > buttons pressed). > > Since this approach is not viable (breaks reboot in most cases), the validity of > the reboot reason is checked (in turn, by checking that a warm reset happened, > as there is no magic) to detect a reboot and the 'o' char is recognized to > indicate that power-off is required. Still, that might be overridden by the > detection of usual power-on reasons, on purpose. > > Signed-off-by: Paul Kocialkowski <contact@paulk.fr> Reviewed-by: Tom Rini <trini@konsulko.com> ... but since Sniper and KC1 are doing the same thing, and other OMAP devices that are also Android devices will also be in the same camp, can we perhaps include some of the above information in a comment, make android_omap_reboot_mode (or something along those lines) in arch/arm/cpu/armv7/omap-common/something-appropriate.c ?
Le lundi 28 mars 2016 à 10:06 -0400, Tom Rini a écrit : > On Mon, Mar 28, 2016 at 02:07:13PM +0200, Paul Kocialkowski wrote: > > With the previous implementation, rebooting without registering a recognized > > reboot mode would end up with U-Boot checking for a valid power-on reason, > > which > > might result in the device turning off (e.g. with no USB cable attached and > > no > > buttons pressed). > > > > Since this approach is not viable (breaks reboot in most cases), the > > validity of > > the reboot reason is checked (in turn, by checking that a warm reset > > happened, > > as there is no magic) to detect a reboot and the 'o' char is recognized to > > indicate that power-off is required. Still, that might be overridden by the > > detection of usual power-on reasons, on purpose. > > > > Signed-off-by: Paul Kocialkowski <contact@paulk.fr> > Reviewed-by: Tom Rini <trini@konsulko.com> > > ... but since Sniper and KC1 are doing the same thing, and other OMAP > devices that are also Android devices will also be in the same camp, can > we perhaps include some of the above information in a comment, make > android_omap_reboot_mode (or something along those lines) in > arch/arm/cpu/armv7/omap-common/something-appropriate.c ? The way things are done now, a few distinct aspects are tied together in my approach: * reboot mode storage, which is Android-specific and also involves the boot command * valid power-on reason checking, which relies on twl code, that I'm not comfortable making part of the omap arch code * device-specific reboot mode setting (overriding omap reboot mode), e.g. from buttons So I think we could go with the following: * Making the twl code common on each twl power driver * Making the Android aspects common through functions dealing with the reboot- mode env variable and associated boot command, with their own Kconfig option * Keeping the global coordination between these in each board file, to handle device-specific input and quirks I'd rather make a clean new series to support that. What do you think?
On Tue, Mar 29, 2016 at 02:14:34PM +0200, Paul Kocialkowski wrote: > Le lundi 28 mars 2016 à 10:06 -0400, Tom Rini a écrit : > > On Mon, Mar 28, 2016 at 02:07:13PM +0200, Paul Kocialkowski wrote: > > > With the previous implementation, rebooting without registering a recognized > > > reboot mode would end up with U-Boot checking for a valid power-on reason, > > > which > > > might result in the device turning off (e.g. with no USB cable attached and > > > no > > > buttons pressed). > > > > > > Since this approach is not viable (breaks reboot in most cases), the > > > validity of > > > the reboot reason is checked (in turn, by checking that a warm reset > > > happened, > > > as there is no magic) to detect a reboot and the 'o' char is recognized to > > > indicate that power-off is required. Still, that might be overridden by the > > > detection of usual power-on reasons, on purpose. > > > > > > Signed-off-by: Paul Kocialkowski <contact@paulk.fr> > > Reviewed-by: Tom Rini <trini@konsulko.com> > > > > ... but since Sniper and KC1 are doing the same thing, and other OMAP > > devices that are also Android devices will also be in the same camp, can > > we perhaps include some of the above information in a comment, make > > android_omap_reboot_mode (or something along those lines) in > > arch/arm/cpu/armv7/omap-common/something-appropriate.c ? > > The way things are done now, a few distinct aspects are tied together in my > approach: > * reboot mode storage, which is Android-specific and also involves the boot > command > * valid power-on reason checking, which relies on twl code, that I'm not > comfortable making part of the omap arch code > * device-specific reboot mode setting (overriding omap reboot mode), e.g. from > buttons > > So I think we could go with the following: > * Making the twl code common on each twl power driver > * Making the Android aspects common through functions dealing with the reboot- > mode env variable and associated boot command, with their own Kconfig option > * Keeping the global coordination between these in each board file, to handle > device-specific input and quirks > > I'd rather make a clean new series to support that. > > What do you think? Since mutt is telling me I forgot to reply, sorry, yes, a follow-up sounds good, thanks!
diff --git a/board/amazon/kc1/kc1.c b/board/amazon/kc1/kc1.c index ca63af8..469a83e 100644 --- a/board/amazon/kc1/kc1.c +++ b/board/amazon/kc1/kc1.c @@ -88,10 +88,11 @@ int misc_init_r(void) char reboot_mode[2] = { 0 }; u32 data = 0; u32 value; + int rc; /* Reboot mode */ - omap_reboot_mode(reboot_mode, sizeof(reboot_mode)); + rc = omap_reboot_mode(reboot_mode, sizeof(reboot_mode)); /* USB ID pin pull-up indicates factory (fastboot) cable detection. */ gpio_request(KC1_GPIO_USB_ID, "USB_ID"); @@ -101,18 +102,7 @@ int misc_init_r(void) if (value) reboot_mode[0] = 'b'; - if (reboot_mode[0] > 0 && isascii(reboot_mode[0])) { - if (reboot_mode[0] == 'o') - twl6030_power_off(); - - if (!getenv("reboot-mode")) - setenv("reboot-mode", (char *)reboot_mode); - - omap_reboot_mode_clear(); - } else { - /* Reboot mode garbage may still be valid, so clear it. */ - omap_reboot_mode_clear(); - + if (rc < 0 || reboot_mode[0] == 'o') { /* * When not rebooting, valid power on reasons are either the * power button, charger plug or USB plug. @@ -126,6 +116,13 @@ int misc_init_r(void) twl6030_power_off(); } + if (reboot_mode[0] > 0 && isascii(reboot_mode[0])) { + if (!getenv("reboot-mode")) + setenv("reboot-mode", (char *)reboot_mode); + } + + omap_reboot_mode_clear(); + /* Serial number */ omap_die_id_serial();
With the previous implementation, rebooting without registering a recognized reboot mode would end up with U-Boot checking for a valid power-on reason, which might result in the device turning off (e.g. with no USB cable attached and no buttons pressed). Since this approach is not viable (breaks reboot in most cases), the validity of the reboot reason is checked (in turn, by checking that a warm reset happened, as there is no magic) to detect a reboot and the 'o' char is recognized to indicate that power-off is required. Still, that might be overridden by the detection of usual power-on reasons, on purpose. Signed-off-by: Paul Kocialkowski <contact@paulk.fr> --- board/amazon/kc1/kc1.c | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-)