| Message ID | 1460120039-2497-4-git-send-email-linux@roeck-us.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> +static int psci_sys_reset(struct notifier_block *np, unsigned long action,
Minor: notifier_block *nb instead of *np
On Fri, Apr 08, 2016 at 05:53:56AM -0700, Guenter Roeck wrote: > Register with kernel restart handler instead of setting arm_pm_restart > directly. This enables support for replacing the PSCI restart handler > with a different handler if necessary for a specific board. > > Select a priority of 129 to indicate a higher than default priority, but > keep it as low as possible since PSCI reset is known to fail on some > boards. For reference, which boards? It's unfortunate that that a PSCI 0.2+ implementation would be lacking a working SYSTEM_RESET implementation, and it's certainly a mistake to discourage. > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > --- > It might make sense to introduce a restart-priority property for devicetree > based configurations, but I am not sure if this would be acceptable. From the DT side, I'm not keen on properties for priorities. They're incredibly fragile and don't really encode a HW property. A better option would be to have a property to describe how the PSCI implementation is broken (e.g. broken-system-reset), and not register the handler at all in that case. Thanks, Mark. > drivers/firmware/psci.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c > index 11bfee8b79a9..99fab3ac3fd5 100644 > --- a/drivers/firmware/psci.c > +++ b/drivers/firmware/psci.c > @@ -231,11 +231,18 @@ static int get_set_conduit_method(struct device_node *np) > return 0; > } > > -static void psci_sys_reset(enum reboot_mode reboot_mode, const char *cmd) > +static int psci_sys_reset(struct notifier_block *np, unsigned long action, > + void *data) > { > invoke_psci_fn(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0); > + return NOTIFY_DONE; > } > > +static struct notifier_block psci_sys_reset_nb = { > + .notifier_call = psci_sys_reset, > + .priority = 129, > +}; > + > static void psci_sys_poweroff(void) > { > invoke_psci_fn(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0); > @@ -461,7 +468,7 @@ static void __init psci_0_2_set_functions(void) > > psci_ops.migrate_info_type = psci_migrate_info_type; > > - arm_pm_restart = psci_sys_reset; > + register_restart_handler(&psci_sys_reset_nb); > > pm_power_off = psci_sys_poweroff; > } > -- > 2.5.0 >
Dear Mark, On Wed, 13 Apr 2016 12:05:19 +0100 Mark Rutland wrote: > On Fri, Apr 08, 2016 at 05:53:56AM -0700, Guenter Roeck wrote: > > Register with kernel restart handler instead of setting arm_pm_restart > > directly. This enables support for replacing the PSCI restart handler > > with a different handler if necessary for a specific board. > > > > Select a priority of 129 to indicate a higher than default priority, but > > keep it as low as possible since PSCI reset is known to fail on some > > boards. > > For reference, which boards? > > It's unfortunate that that a PSCI 0.2+ implementation would be lacking a > working SYSTEM_RESET implementation, and it's certainly a mistake to > discourage. I may understand the case: on some platforms, the only reset way is to trigger the wdt, for various reason the underly firmware isn't convenient to touch the wdt. But I'd like 127 or lower for the default priority for the above case, because various wdt reset_handler priority is 128. Thanks, Jisheng > > > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > > --- > > It might make sense to introduce a restart-priority property for devicetree > > based configurations, but I am not sure if this would be acceptable. > > From the DT side, I'm not keen on properties for priorities. They're > incredibly fragile and don't really encode a HW property. > > A better option would be to have a property to describe how the PSCI > implementation is broken (e.g. broken-system-reset), and not register > the handler at all in that case. > > Thanks, > Mark. > > > drivers/firmware/psci.c | 11 +++++++++-- > > 1 file changed, 9 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c > > index 11bfee8b79a9..99fab3ac3fd5 100644 > > --- a/drivers/firmware/psci.c > > +++ b/drivers/firmware/psci.c > > @@ -231,11 +231,18 @@ static int get_set_conduit_method(struct device_node *np) > > return 0; > > } > > > > -static void psci_sys_reset(enum reboot_mode reboot_mode, const char *cmd) > > +static int psci_sys_reset(struct notifier_block *np, unsigned long action, > > + void *data) > > { > > invoke_psci_fn(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0); > > + return NOTIFY_DONE; > > } > > > > +static struct notifier_block psci_sys_reset_nb = { > > + .notifier_call = psci_sys_reset, > > + .priority = 129, > > +}; > > + > > static void psci_sys_poweroff(void) > > { > > invoke_psci_fn(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0); > > @@ -461,7 +468,7 @@ static void __init psci_0_2_set_functions(void) > > > > psci_ops.migrate_info_type = psci_migrate_info_type; > > > > - arm_pm_restart = psci_sys_reset; > > + register_restart_handler(&psci_sys_reset_nb); > > > > pm_power_off = psci_sys_poweroff; > > } > > -- > > 2.5.0 > > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 04/13/2016 04:05 AM, Mark Rutland wrote: > On Fri, Apr 08, 2016 at 05:53:56AM -0700, Guenter Roeck wrote: >> Register with kernel restart handler instead of setting arm_pm_restart >> directly. This enables support for replacing the PSCI restart handler >> with a different handler if necessary for a specific board. >> >> Select a priority of 129 to indicate a higher than default priority, but >> keep it as low as possible since PSCI reset is known to fail on some >> boards. > > For reference, which boards? > Salvator-X, reported by Geert Uytterhoeven. Wolfram Sang also reported that it is broken on a board he is using, but I don't recall if it is the same board. > It's unfortunate that that a PSCI 0.2+ implementation would be lacking a > working SYSTEM_RESET implementation, and it's certainly a mistake to > discourage. > >> Signed-off-by: Guenter Roeck <linux@roeck-us.net> >> --- >> It might make sense to introduce a restart-priority property for devicetree >> based configurations, but I am not sure if this would be acceptable. > >>From the DT side, I'm not keen on properties for priorities. They're > incredibly fragile and don't really encode a HW property. > Depends. It is a convenient means to say "primary restart method" or "may be broken". > A better option would be to have a property to describe how the PSCI > implementation is broken (e.g. broken-system-reset), and not register > the handler at all in that case. > ... just like this. I'll look into it. Thanks, Guenter > Thanks, > Mark. > >> drivers/firmware/psci.c | 11 +++++++++-- >> 1 file changed, 9 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c >> index 11bfee8b79a9..99fab3ac3fd5 100644 >> --- a/drivers/firmware/psci.c >> +++ b/drivers/firmware/psci.c >> @@ -231,11 +231,18 @@ static int get_set_conduit_method(struct device_node *np) >> return 0; >> } >> >> -static void psci_sys_reset(enum reboot_mode reboot_mode, const char *cmd) >> +static int psci_sys_reset(struct notifier_block *np, unsigned long action, >> + void *data) >> { >> invoke_psci_fn(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0); >> + return NOTIFY_DONE; >> } >> >> +static struct notifier_block psci_sys_reset_nb = { >> + .notifier_call = psci_sys_reset, >> + .priority = 129, >> +}; >> + >> static void psci_sys_poweroff(void) >> { >> invoke_psci_fn(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0); >> @@ -461,7 +468,7 @@ static void __init psci_0_2_set_functions(void) >> >> psci_ops.migrate_info_type = psci_migrate_info_type; >> >> - arm_pm_restart = psci_sys_reset; >> + register_restart_handler(&psci_sys_reset_nb); >> >> pm_power_off = psci_sys_poweroff; >> } >> -- >> 2.5.0 >> >
On Wed, Apr 13, 2016 at 3:10 PM, Guenter Roeck <linux@roeck-us.net> wrote: > On 04/13/2016 04:05 AM, Mark Rutland wrote: >> On Fri, Apr 08, 2016 at 05:53:56AM -0700, Guenter Roeck wrote: >>> >>> Register with kernel restart handler instead of setting arm_pm_restart >>> directly. This enables support for replacing the PSCI restart handler >>> with a different handler if necessary for a specific board. >>> >>> Select a priority of 129 to indicate a higher than default priority, but >>> keep it as low as possible since PSCI reset is known to fail on some >>> boards. >> >> For reference, which boards? >> > Salvator-X, reported by Geert Uytterhoeven. Wolfram Sang also reported > that it is broken on a board he is using, but I don't recall if it is > the same board. Yes it is. >> It's unfortunate that that a PSCI 0.2+ implementation would be lacking a >> working SYSTEM_RESET implementation, and it's certainly a mistake to >> discourage. >> >>> Signed-off-by: Guenter Roeck <linux@roeck-us.net> >>> --- >>> It might make sense to introduce a restart-priority property for >>> devicetree >>> based configurations, but I am not sure if this would be acceptable. >> >>> From the DT side, I'm not keen on properties for priorities. They're >> incredibly fragile and don't really encode a HW property. >> > Depends. It is a convenient means to say "primary restart method" or > "may be broken". The issue is supposed to be fixed in a more recent firmware, which I still have to try. DT indeed isn't the right place to work around this. What we need is a blacklist of bad firmware versions... Or Perfect Firmware from Day One on (like Perfect DT from Day One ;-) Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Wed, Apr 13, 2016 at 03:22:44PM +0200, Geert Uytterhoeven wrote: > On Wed, Apr 13, 2016 at 3:10 PM, Guenter Roeck <linux@roeck-us.net> wrote: > > On 04/13/2016 04:05 AM, Mark Rutland wrote: > >> On Fri, Apr 08, 2016 at 05:53:56AM -0700, Guenter Roeck wrote: > >>> > >>> Register with kernel restart handler instead of setting arm_pm_restart > >>> directly. This enables support for replacing the PSCI restart handler > >>> with a different handler if necessary for a specific board. > >>> > >>> Select a priority of 129 to indicate a higher than default priority, but > >>> keep it as low as possible since PSCI reset is known to fail on some > >>> boards. > >> > >> For reference, which boards? > >> > > Salvator-X, reported by Geert Uytterhoeven. Wolfram Sang also reported > > that it is broken on a board he is using, but I don't recall if it is > > the same board. > > Yes it is. > > >> It's unfortunate that that a PSCI 0.2+ implementation would be lacking a > >> working SYSTEM_RESET implementation, and it's certainly a mistake to > >> discourage. > >> > >>> Signed-off-by: Guenter Roeck <linux@roeck-us.net> > >>> --- > >>> It might make sense to introduce a restart-priority property for > >>> devicetree > >>> based configurations, but I am not sure if this would be acceptable. > >> > >>> From the DT side, I'm not keen on properties for priorities. They're > >> incredibly fragile and don't really encode a HW property. > >> > > Depends. It is a convenient means to say "primary restart method" or > > "may be broken". > > The issue is supposed to be fixed in a more recent firmware, which I still have > to try. > > DT indeed isn't the right place to work around this. What we need is a > blacklist of bad firmware versions... > Or Perfect Firmware from Day One on (like Perfect DT from Day One ;-) > That makes things quite tricky. Best I can think of is a series of boolean devicetree properties, such as broken-reset-handler last-resort-restart-handler secondary-restart-handler default-restart-handler primary-restart-handler which ends up being quite similar to the 'restart-priority' property. I'll do this as follow-up patch, though - I do not see the point holding up the series for this, and it is really a separate problem. I'll send rev2 with the various Acked-by: and Reviewed-by: tags as well as the variable rename suggested by Wolfram. Thanks, Guenter
> That makes things quite tricky. Best I can think of is a series of boolean > devicetree properties, such as > > broken-reset-handler > last-resort-restart-handler > secondary-restart-handler > default-restart-handler > primary-restart-handler > > which ends up being quite similar to the 'restart-priority' property. I'll > do this as follow-up patch, though Please CC me on this. I wanted to tackle this problem as well today. My findings/conclusions so far: * There is one driver bringing 'priority' directly to DT already: gpio-restart * Watchdog priorities are board dependant * Having the priorities clear at boot-time is safer than configuring them at run-time * The linux scheme (0-255) shouldn't be enforced in DT So, I wondered about a "priority" binding which just states "the higher, the more important". Then any OS can decide what to do with it. In the Linux case, this could be: sort them and give them priority 256 - position_in_sorted_list. Opinions? > - I do not see the point holding up the series for this, and it is > really a separate problem. Ack.
On 04/14/2016 01:52 AM, Wolfram Sang wrote: > >> That makes things quite tricky. Best I can think of is a series of boolean >> devicetree properties, such as >> >> broken-reset-handler >> last-resort-restart-handler >> secondary-restart-handler >> default-restart-handler >> primary-restart-handler >> >> which ends up being quite similar to the 'restart-priority' property. I'll >> do this as follow-up patch, though > > Please CC me on this. I wanted to tackle this problem as well today. My Sure. > findings/conclusions so far: > > * There is one driver bringing 'priority' directly to DT already: gpio-restart > Correct. > * Watchdog priorities are board dependant > > * Having the priorities clear at boot-time is safer than configuring them > at run-time > Correct. > * The linux scheme (0-255) shouldn't be enforced in DT > > So, I wondered about a "priority" binding which just states "the higher, > the more important". Then any OS can decide what to do with it. In the > Linux case, this could be: sort them and give them priority 256 - > position_in_sorted_list. > "the higher, the more important" makes sense to me. We don't have to enforce the linux scheme, though that happens to be the same (the priority argument in the notifier block takes an int, so it would not even be necessary to adjust it unless someone specifies 0xffffffff). > Opinions? > I am fine either way - boolean properties or numbers, with a personal preference for numbers as more flexible. Whatever is acceptable for the community is fine with me. Guenter >> - I do not see the point holding up the series for this, and it is >> really a separate problem. > > Ack. >
> "the higher, the more important" makes sense to me. We don't have to > enforce the linux scheme, though that happens to be the same (the priority > argument in the notifier block takes an int, so it would not even be > necessary to adjust it unless someone specifies 0xffffffff). I think we should enforce the scheme internally (but not in DT, of course): a) it is documented to be in the range 0-255 b) it should be valid to prioritize the watchdogs with 1,2,3 in DT. If we don't apply the '255 - pos_in_sorted_list' value, then the priority could be below some machine default of 128, or? > I am fine either way - boolean properties or numbers, with a personal > preference for numbers as more flexible. Same here. Time to go to the DT list probably.
diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c index 11bfee8b79a9..99fab3ac3fd5 100644 --- a/drivers/firmware/psci.c +++ b/drivers/firmware/psci.c @@ -231,11 +231,18 @@ static int get_set_conduit_method(struct device_node *np) return 0; } -static void psci_sys_reset(enum reboot_mode reboot_mode, const char *cmd) +static int psci_sys_reset(struct notifier_block *np, unsigned long action, + void *data) { invoke_psci_fn(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0); + return NOTIFY_DONE; } +static struct notifier_block psci_sys_reset_nb = { + .notifier_call = psci_sys_reset, + .priority = 129, +}; + static void psci_sys_poweroff(void) { invoke_psci_fn(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0); @@ -461,7 +468,7 @@ static void __init psci_0_2_set_functions(void) psci_ops.migrate_info_type = psci_migrate_info_type; - arm_pm_restart = psci_sys_reset; + register_restart_handler(&psci_sys_reset_nb); pm_power_off = psci_sys_poweroff; }
Register with kernel restart handler instead of setting arm_pm_restart directly. This enables support for replacing the PSCI restart handler with a different handler if necessary for a specific board. Select a priority of 129 to indicate a higher than default priority, but keep it as low as possible since PSCI reset is known to fail on some boards. Signed-off-by: Guenter Roeck <linux@roeck-us.net> --- It might make sense to introduce a restart-priority property for devicetree based configurations, but I am not sure if this would be acceptable. drivers/firmware/psci.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)