Re: [RESEND][PATCH v2] firmware/psci: add support for SYSTEM_RESET2
On Thu, Apr 11, 2019 at 09:26:37PM +0300, Aaro Koskinen wrote: > Hi, > > On Thu, Apr 11, 2019 at 05:49:36PM +0100, Sudeep Holla wrote: > > On Thu, Apr 11, 2019 at 11:42:28AM +, Koskinen, Aaro (Nokia - FI/Espoo) > > wrote: > > > From: Sudeep Holla [sudeep.ho...@arm.com]: > > > > static void psci_sys_reset(enum reboot_mode reboot_mode, const char > > > > *cmd) > > > > { > > > > + if ((reboot_mode == REBOOT_WARM || reboot_mode == REBOOT_SOFT) > > > > && > > > > > > I would omit the REBOOT_SOFT here. > > > > I included REBOOT_SOFT for 2 reasons: > > 1. drivers/firmware/efi/reboot.c - efi_reboot treats WARM and SOFT reboots > > same > > 2. If the vendors specific reboots are added and handled in EFI, I assume it > >will be categorised under REBOOT_SOFT. > > > > If that's wrong I can drop REBOOT_SOFT. > > Not a big issue, but it's just unclear what SOFT means. WARM at least maps > nicely to the PSCI spec. > OK, I will keep it for now. > > > > + psci_system_reset2_supported) > > > > + /* > > > > +* reset_type[31] = 0 (architectural) > > > > +* reset_type[30:0] = 0 (SYSTEM_WARM_RESET) > > > > +* cookie = 0 (ignored by the implementation) > > > > +*/ > > > > + invoke_psci_fn(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2), 0, > > > > 0, 0); > > > > + > > > >invoke_psci_fn(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0); > > > > > > Use else here, so that we fall back to system halt if SYSTEM_RESET2 fails. > > > > Will that not change current behaviour ? IOW, is that expected behaviour ? > > I am not sure if halt can be prefer over cold reboot in absence of warm/soft > > reboot when the system is request to reboot. From PSCI perspective, since > > SYSTEM_RESET is mandatory I prefer that unless Linux has any restriction > > on this behaviour. > > Hmm, so does it mean that even if firmware tells that SYSTEM_RESET2 > is implemented it does not imply that SYSTEM_WARM_RESET is > available? I.e. the firmware could choose to implement only some > vendor-specific resets but not architectural ones. In that case, could > we fall back to cold reset only if NOT_SUPPORTED is returned? My point > is that if the warm reset fails unexpectedly, we should halt the system > like we do if the cold reset fails. > OK, I understood. Sorry I was under the assumption that architectural reset was mandatory if SYSTEM_RESET2 is implemented. I checked the PSCI specification and I am wrong. So I am happy to add else as per your suggestion. -- Regards, Sudeep
Re: [RESEND][PATCH v2] firmware/psci: add support for SYSTEM_RESET2
Hi, On Thu, Apr 11, 2019 at 05:49:36PM +0100, Sudeep Holla wrote: > On Thu, Apr 11, 2019 at 11:42:28AM +, Koskinen, Aaro (Nokia - FI/Espoo) > wrote: > > From: Sudeep Holla [sudeep.ho...@arm.com]: > > > static void psci_sys_reset(enum reboot_mode reboot_mode, const char *cmd) > > > { > > > + if ((reboot_mode == REBOOT_WARM || reboot_mode == REBOOT_SOFT) && > > > > I would omit the REBOOT_SOFT here. > > I included REBOOT_SOFT for 2 reasons: > 1. drivers/firmware/efi/reboot.c - efi_reboot treats WARM and SOFT reboots > same > 2. If the vendors specific reboots are added and handled in EFI, I assume it >will be categorised under REBOOT_SOFT. > > If that's wrong I can drop REBOOT_SOFT. Not a big issue, but it's just unclear what SOFT means. WARM at least maps nicely to the PSCI spec. > > > + psci_system_reset2_supported) > > > + /* > > > +* reset_type[31] = 0 (architectural) > > > +* reset_type[30:0] = 0 (SYSTEM_WARM_RESET) > > > +* cookie = 0 (ignored by the implementation) > > > +*/ > > > + invoke_psci_fn(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2), 0, 0, > > > 0); > > > + > > >invoke_psci_fn(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0); > > > > Use else here, so that we fall back to system halt if SYSTEM_RESET2 fails. > > Will that not change current behaviour ? IOW, is that expected behaviour ? > I am not sure if halt can be prefer over cold reboot in absence of warm/soft > reboot when the system is request to reboot. From PSCI perspective, since > SYSTEM_RESET is mandatory I prefer that unless Linux has any restriction > on this behaviour. Hmm, so does it mean that even if firmware tells that SYSTEM_RESET2 is implemented it does not imply that SYSTEM_WARM_RESET is available? I.e. the firmware could choose to implement only some vendor-specific resets but not architectural ones. In that case, could we fall back to cold reset only if NOT_SUPPORTED is returned? My point is that if the warm reset fails unexpectedly, we should halt the system like we do if the cold reset fails. A.
Re: [RESEND][PATCH v2] firmware/psci: add support for SYSTEM_RESET2
On Thu, Apr 11, 2019 at 11:42:28AM +, Koskinen, Aaro (Nokia - FI/Espoo) wrote: > Hi, > > From: Sudeep Holla [sudeep.ho...@arm.com]: > > static void psci_sys_reset(enum reboot_mode reboot_mode, const char *cmd) > > { > > + if ((reboot_mode == REBOOT_WARM || reboot_mode == REBOOT_SOFT) && > > I would omit the REBOOT_SOFT here. > I included REBOOT_SOFT for 2 reasons: 1. drivers/firmware/efi/reboot.c - efi_reboot treats WARM and SOFT reboots same 2. If the vendors specific reboots are added and handled in EFI, I assume it will be categorised under REBOOT_SOFT. If that's wrong I can drop REBOOT_SOFT. > > + psci_system_reset2_supported) > > + /* > > +* reset_type[31] = 0 (architectural) > > +* reset_type[30:0] = 0 (SYSTEM_WARM_RESET) > > +* cookie = 0 (ignored by the implementation) > > +*/ > > + invoke_psci_fn(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2), 0, 0, 0); > > + > >invoke_psci_fn(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0); > > Use else here, so that we fall back to system halt if SYSTEM_RESET2 fails. > Will that not change current behaviour ? IOW, is that expected behaviour ? I am not sure if halt can be prefer over cold reboot in absence of warm/soft reboot when the system is request to reboot. From PSCI perspective, since SYSTEM_RESET is mandatory I prefer that unless Linux has any restriction on this behaviour. -- Regards, Sudeep
Re: [RESEND][PATCH v2] firmware/psci: add support for SYSTEM_RESET2
On Thu, Apr 11, 2019 at 12:03:04PM +0100, Mark Rutland wrote: > On Thu, Apr 11, 2019 at 11:33:46AM +0100, Sudeep Holla wrote: > > PSCI v1.1 introduced SYSTEM_RESET2 to allow both architectural resets > > where the semantics are described by the PSCI specification itself as > > well as vendor-specific resets. Currently only system warm reset > > semantics is defined as part of architectural resets by the specification. > > > > This patch implements support for SYSTEM_RESET2 by making using of > > reboot_mode passed by the reboot infrastructure in the kernel. > > > > Cc: Mark Rutland > > Cc: Lorenzo Pieralisi > > Signed-off-by: Sudeep Holla > > --- > > drivers/firmware/psci.c | 21 + > > include/uapi/linux/psci.h | 2 ++ > > 2 files changed, 23 insertions(+) > > > > Resending [1] based on the request. I hope to get some testing this time. > > Last time Xilinx asked multiple times but never got any review or testing > > https://lore.kernel.org/lkml/1525257003-8608-1-git-send-email-sudeep.ho...@arm.com/ > > > > diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c > > index c80ec1d03274..91748725534e 100644 > > --- a/drivers/firmware/psci.c > > +++ b/drivers/firmware/psci.c > > @@ -88,6 +88,7 @@ static u32 psci_function_id[PSCI_FN_MAX]; > > PSCI_1_0_EXT_POWER_STATE_TYPE_MASK) > > > > static u32 psci_cpu_suspend_feature; > > +static bool psci_system_reset2_supported; > > > > static inline bool psci_has_ext_power_state(void) > > { > > @@ -253,6 +254,15 @@ static int get_set_conduit_method(struct device_node > > *np) > > > > static void psci_sys_reset(enum reboot_mode reboot_mode, const char *cmd) > > { > > + if ((reboot_mode == REBOOT_WARM || reboot_mode == REBOOT_SOFT) && > > + psci_system_reset2_supported) > > + /* > > +* reset_type[31] = 0 (architectural) > > +* reset_type[30:0] = 0 (SYSTEM_WARM_RESET) > > +* cookie = 0 (ignored by the implementation) > > +*/ > > + invoke_psci_fn(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2), 0, 0, 0); > > Since the comment and invocation span multiple lines, could we please > wrap them in braces? > Yes, that would be better, will update it. > Other than that, this looks good to me, so: > > Acked-by: Mark Rutland > Thanks. -- Regards, Sudeep
RE: [RESEND][PATCH v2] firmware/psci: add support for SYSTEM_RESET2
Hi, From: Sudeep Holla [sudeep.ho...@arm.com]: > static void psci_sys_reset(enum reboot_mode reboot_mode, const char *cmd) > { > + if ((reboot_mode == REBOOT_WARM || reboot_mode == REBOOT_SOFT) && I would omit the REBOOT_SOFT here. > + psci_system_reset2_supported) > + /* > +* reset_type[31] = 0 (architectural) > +* reset_type[30:0] = 0 (SYSTEM_WARM_RESET) > +* cookie = 0 (ignored by the implementation) > +*/ > + invoke_psci_fn(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2), 0, 0, 0); > + >invoke_psci_fn(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0); Use else here, so that we fall back to system halt if SYSTEM_RESET2 fails. A.
Re: [RESEND][PATCH v2] firmware/psci: add support for SYSTEM_RESET2
On Thu, Apr 11, 2019 at 11:33:46AM +0100, Sudeep Holla wrote: > PSCI v1.1 introduced SYSTEM_RESET2 to allow both architectural resets > where the semantics are described by the PSCI specification itself as > well as vendor-specific resets. Currently only system warm reset > semantics is defined as part of architectural resets by the specification. > > This patch implements support for SYSTEM_RESET2 by making using of > reboot_mode passed by the reboot infrastructure in the kernel. > > Cc: Mark Rutland > Cc: Lorenzo Pieralisi > Signed-off-by: Sudeep Holla > --- > drivers/firmware/psci.c | 21 + > include/uapi/linux/psci.h | 2 ++ > 2 files changed, 23 insertions(+) > > Resending [1] based on the request. I hope to get some testing this time. > Last time Xilinx asked multiple times but never got any review or testing > https://lore.kernel.org/lkml/1525257003-8608-1-git-send-email-sudeep.ho...@arm.com/ > > diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c > index c80ec1d03274..91748725534e 100644 > --- a/drivers/firmware/psci.c > +++ b/drivers/firmware/psci.c > @@ -88,6 +88,7 @@ static u32 psci_function_id[PSCI_FN_MAX]; > PSCI_1_0_EXT_POWER_STATE_TYPE_MASK) > > static u32 psci_cpu_suspend_feature; > +static bool psci_system_reset2_supported; > > static inline bool psci_has_ext_power_state(void) > { > @@ -253,6 +254,15 @@ static int get_set_conduit_method(struct device_node *np) > > static void psci_sys_reset(enum reboot_mode reboot_mode, const char *cmd) > { > + if ((reboot_mode == REBOOT_WARM || reboot_mode == REBOOT_SOFT) && > + psci_system_reset2_supported) > + /* > + * reset_type[31] = 0 (architectural) > + * reset_type[30:0] = 0 (SYSTEM_WARM_RESET) > + * cookie = 0 (ignored by the implementation) > + */ > + invoke_psci_fn(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2), 0, 0, 0); Since the comment and invocation span multiple lines, could we please wrap them in braces? Other than that, this looks good to me, so: Acked-by: Mark Rutland ... I assume that Aaro will give this some testing. Thanks, Mark. > + > invoke_psci_fn(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0); > } > > @@ -451,6 +461,16 @@ static const struct platform_suspend_ops > psci_suspend_ops = { > .enter = psci_system_suspend_enter, > }; > > +static void __init psci_init_system_reset2(void) > +{ > + int ret; > + > + ret = psci_features(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2)); > + > + if (ret != PSCI_RET_NOT_SUPPORTED) > + psci_system_reset2_supported = true; > +} > + > static void __init psci_init_system_suspend(void) > { > int ret; > @@ -588,6 +608,7 @@ static int __init psci_probe(void) > psci_init_smccc(); > psci_init_cpu_suspend(); > psci_init_system_suspend(); > + psci_init_system_reset2(); > } > > return 0; > diff --git a/include/uapi/linux/psci.h b/include/uapi/linux/psci.h > index b3bcabe380da..5b0ba0062541 100644 > --- a/include/uapi/linux/psci.h > +++ b/include/uapi/linux/psci.h > @@ -49,8 +49,10 @@ > > #define PSCI_1_0_FN_PSCI_FEATURESPSCI_0_2_FN(10) > #define PSCI_1_0_FN_SYSTEM_SUSPEND PSCI_0_2_FN(14) > +#define PSCI_1_1_FN_SYSTEM_RESET2PSCI_0_2_FN(18) > > #define PSCI_1_0_FN64_SYSTEM_SUSPEND PSCI_0_2_FN64(14) > +#define PSCI_1_1_FN64_SYSTEM_RESET2 PSCI_0_2_FN64(18) > > /* PSCI v0.2 power state encoding for CPU_SUSPEND function */ > #define PSCI_0_2_POWER_STATE_ID_MASK 0x > -- > 2.17.1 >