| Message ID | 1582577858-12410-3-git-send-email-eberman@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ARM PSCI: Add support for vendor-specific SYSTEM_RESET2 | expand |
On Mon, Feb 24, 2020 at 12:57:37PM -0800, Elliot Berman wrote: > Some implementors of PSCI may relax the requirements of the PSCI > architectural warm reset. In order to comply with PSCI specification, a > different reset_type value must be used. The alternate PSCI > SYSTEM_RESET2 may be used in all warm/soft reboot scenarios, replacing > the architectural warm reset. > > Signed-off-by: Elliot Berman <eberman@codeaurora.org> > --- > drivers/firmware/psci/psci.c | 22 ++++++++++++++++++---- > include/uapi/linux/psci.h | 2 ++ > 2 files changed, 20 insertions(+), 4 deletions(-) > > diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c > index 2937d44..8f4609c 100644 > --- a/drivers/firmware/psci/psci.c > +++ b/drivers/firmware/psci/psci.c > @@ -90,6 +90,8 @@ static u32 psci_function_id[PSCI_FN_MAX]; > > static u32 psci_cpu_suspend_feature; > static bool psci_system_reset2_supported; > +static u32 psci_sys_reset2_reset_param = > + PSCI_1_1_SYSTEM_RESET2_SYSTEM_WARM_RESET; > > static inline bool psci_has_ext_power_state(void) > { > @@ -272,11 +274,10 @@ 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) Sorry for missing this earlier, you can move this comment above if others agree to not drop that info. I am fine with removing it too. E > * cookie = 0 (ignored by the implementation) > */ > - invoke_psci_fn(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2), 0, 0, 0); > + invoke_psci_fn(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2), > + psci_sys_reset2_reset_param, 0, 0); > } else { > invoke_psci_fn(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0); > } > @@ -493,6 +494,7 @@ typedef int (*psci_initcall_t)(const struct device_node *); > static int __init psci_0_2_init(struct device_node *np) > { > int err; > + u32 param; > > err = get_set_conduit_method(np); > if (err) > @@ -505,7 +507,19 @@ static int __init psci_0_2_init(struct device_node *np) > * can be carried out according to the specific version reported > * by firmware > */ > - return psci_probe(); > + err = psci_probe(); > + if (err) > + return err; > + > + if (psci_system_reset2_supported && > + !of_property_read_u32(np, "arm,psci-sys-reset2-param", ¶m)) { > + if ((s32)param > 0) What is the point on signed comparison here ? You are assuming all vendor reset also as architecture by doing so which is wrong. > + pr_warn("%08x is an invalid architectural reset type.\n", > + param); I thought the point was to have vendor reset here. Based on the 3/3 you see to have vendor reset bit set, you ignore that by doing signed comparison which is wrong and even the message is wrong. Specification defines only one architectural reset(WARM RESET) and all others need to be vendor specific. -- Regards, Sudeep
On 2/25/2020 3:03 AM, Sudeep Holla wrote: > On Mon, Feb 24, 2020 at 12:57:37PM -0800, Elliot Berman wrote: >> @@ -493,6 +494,7 @@ typedef int (*psci_initcall_t)(const struct device_node *); >> static int __init psci_0_2_init(struct device_node *np) >> { >> int err; >> + u32 param; >> >> err = get_set_conduit_method(np); >> if (err) >> @@ -505,7 +507,19 @@ static int __init psci_0_2_init(struct device_node *np) >> * can be carried out according to the specific version reported >> * by firmware >> */ >> - return psci_probe(); >> + err = psci_probe(); >> + if (err) >> + return err; >> + >> + if (psci_system_reset2_supported && >> + !of_property_read_u32(np, "arm,psci-sys-reset2-param", ¶m)) { >> + if ((s32)param > 0) > > What is the point on signed comparison here ? You are assuming all vendor > reset also as architecture by doing so which is wrong. > >> + pr_warn("%08x is an invalid architectural reset type.\n", >> + param); > > I thought the point was to have vendor reset here. Based on the 3/3 you > see to have vendor reset bit set, you ignore that by doing signed comparison > which is wrong and even the message is wrong. Specification defines only > one architectural reset(WARM RESET) and all others need to be vendor specific. > > -- > Regards, > Sudeep > I might've gone crazy, but all vendor-specific reset types would be negative when cast as s32. Thus the check returns true only for an invalid architectural reset type. I can switch to checking bits instead of using cast in v3 to avoid the confusion. Alternatively, I could rename the DT property to "arm,psci-sys-reset2-vendor-param" and then always set the 31st bit so that it is impossible to provide an invalid architectural reset type in DT. Let me know what is preferred.
On Tue, Feb 25, 2020 at 05:37:53PM -0800, Elliot Berman wrote: [...] > Alternatively, I could rename the DT property to > "arm,psci-sys-reset2-vendor-param" Yes much better. > and then always set the 31st bit so that it is impossible to provide an > invalid architectural reset type in DT. > Indeed this is what I was expecting and hence raised issue here. So that one can't make use of architectural ID space just by adding param in DT. -- Regards, Sudeep
On Wed, Feb 26, 2020 at 11:59:12AM +0000, Sudeep Holla wrote: > On Tue, Feb 25, 2020 at 05:37:53PM -0800, Elliot Berman wrote: > > [...] > > > Alternatively, I could rename the DT property to > > "arm,psci-sys-reset2-vendor-param" > > Yes much better. > Just seconds after sending this I got a doubt as how is this being solved with ACPI. How is vendor specific reset achieved on ACPI system especially with other OSes ? -- Regards, Sudeep
On Mon, Feb 24, 2020 at 12:57:37PM -0800, Elliot Berman wrote: > Some implementors of PSCI may relax the requirements of the PSCI > architectural warm reset. In order to comply with PSCI specification, a > different reset_type value must be used. The alternate PSCI > SYSTEM_RESET2 may be used in all warm/soft reboot scenarios, replacing > the architectural warm reset. As with the binding patch, this sounds like a workaround for a firmware bug. Can you please elaborate on what exactly your firmware does in this case? Thanks, Mark. > > Signed-off-by: Elliot Berman <eberman@codeaurora.org> > --- > drivers/firmware/psci/psci.c | 22 ++++++++++++++++++---- > include/uapi/linux/psci.h | 2 ++ > 2 files changed, 20 insertions(+), 4 deletions(-) > > diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c > index 2937d44..8f4609c 100644 > --- a/drivers/firmware/psci/psci.c > +++ b/drivers/firmware/psci/psci.c > @@ -90,6 +90,8 @@ static u32 psci_function_id[PSCI_FN_MAX]; > > static u32 psci_cpu_suspend_feature; > static bool psci_system_reset2_supported; > +static u32 psci_sys_reset2_reset_param = > + PSCI_1_1_SYSTEM_RESET2_SYSTEM_WARM_RESET; > > static inline bool psci_has_ext_power_state(void) > { > @@ -272,11 +274,10 @@ 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); > + invoke_psci_fn(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2), > + psci_sys_reset2_reset_param, 0, 0); > } else { > invoke_psci_fn(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0); > } > @@ -493,6 +494,7 @@ typedef int (*psci_initcall_t)(const struct device_node *); > static int __init psci_0_2_init(struct device_node *np) > { > int err; > + u32 param; > > err = get_set_conduit_method(np); > if (err) > @@ -505,7 +507,19 @@ static int __init psci_0_2_init(struct device_node *np) > * can be carried out according to the specific version reported > * by firmware > */ > - return psci_probe(); > + err = psci_probe(); > + if (err) > + return err; > + > + if (psci_system_reset2_supported && > + !of_property_read_u32(np, "arm,psci-sys-reset2-param", ¶m)) { > + if ((s32)param > 0) > + pr_warn("%08x is an invalid architectural reset type.\n", > + param); > + psci_sys_reset2_reset_param = param; > + } > + > + return 0; > } > > /* > diff --git a/include/uapi/linux/psci.h b/include/uapi/linux/psci.h > index 2fcad1d..d786ec8 100644 > --- a/include/uapi/linux/psci.h > +++ b/include/uapi/linux/psci.h > @@ -55,6 +55,8 @@ > #define PSCI_1_0_FN64_SYSTEM_SUSPEND PSCI_0_2_FN64(14) > #define PSCI_1_1_FN64_SYSTEM_RESET2 PSCI_0_2_FN64(18) > > +#define PSCI_1_1_SYSTEM_RESET2_SYSTEM_WARM_RESET 0 > + > /* PSCI v0.2 power state encoding for CPU_SUSPEND function */ > #define PSCI_0_2_POWER_STATE_ID_MASK 0xffff > #define PSCI_0_2_POWER_STATE_ID_SHIFT 0 > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project
diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c index 2937d44..8f4609c 100644 --- a/drivers/firmware/psci/psci.c +++ b/drivers/firmware/psci/psci.c @@ -90,6 +90,8 @@ static u32 psci_function_id[PSCI_FN_MAX]; static u32 psci_cpu_suspend_feature; static bool psci_system_reset2_supported; +static u32 psci_sys_reset2_reset_param = + PSCI_1_1_SYSTEM_RESET2_SYSTEM_WARM_RESET; static inline bool psci_has_ext_power_state(void) { @@ -272,11 +274,10 @@ 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); + invoke_psci_fn(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2), + psci_sys_reset2_reset_param, 0, 0); } else { invoke_psci_fn(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0); } @@ -493,6 +494,7 @@ typedef int (*psci_initcall_t)(const struct device_node *); static int __init psci_0_2_init(struct device_node *np) { int err; + u32 param; err = get_set_conduit_method(np); if (err) @@ -505,7 +507,19 @@ static int __init psci_0_2_init(struct device_node *np) * can be carried out according to the specific version reported * by firmware */ - return psci_probe(); + err = psci_probe(); + if (err) + return err; + + if (psci_system_reset2_supported && + !of_property_read_u32(np, "arm,psci-sys-reset2-param", ¶m)) { + if ((s32)param > 0) + pr_warn("%08x is an invalid architectural reset type.\n", + param); + psci_sys_reset2_reset_param = param; + } + + return 0; } /* diff --git a/include/uapi/linux/psci.h b/include/uapi/linux/psci.h index 2fcad1d..d786ec8 100644 --- a/include/uapi/linux/psci.h +++ b/include/uapi/linux/psci.h @@ -55,6 +55,8 @@ #define PSCI_1_0_FN64_SYSTEM_SUSPEND PSCI_0_2_FN64(14) #define PSCI_1_1_FN64_SYSTEM_RESET2 PSCI_0_2_FN64(18) +#define PSCI_1_1_SYSTEM_RESET2_SYSTEM_WARM_RESET 0 + /* PSCI v0.2 power state encoding for CPU_SUSPEND function */ #define PSCI_0_2_POWER_STATE_ID_MASK 0xffff #define PSCI_0_2_POWER_STATE_ID_SHIFT 0
Some implementors of PSCI may relax the requirements of the PSCI architectural warm reset. In order to comply with PSCI specification, a different reset_type value must be used. The alternate PSCI SYSTEM_RESET2 may be used in all warm/soft reboot scenarios, replacing the architectural warm reset. Signed-off-by: Elliot Berman <eberman@codeaurora.org> --- drivers/firmware/psci/psci.c | 22 ++++++++++++++++++---- include/uapi/linux/psci.h | 2 ++ 2 files changed, 20 insertions(+), 4 deletions(-)