| Message ID | 20241107-arm-psci-system_reset2-vendor-reboots-v8-3-e8715fa65cb5@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Implement vendor resets for PSCI SYSTEM_RESET2 | expand |
On Thu, Nov 07, 2024 at 03:38:27PM -0800, Elliot Berman wrote: > SoC vendors have different types of resets and are controlled through > various registers. For instance, Qualcomm chipsets can reboot to a > "download mode" that allows a RAM dump to be collected. Another example > is they also support writing a cookie that can be read by bootloader > during next boot. PSCI offers a mechanism, SYSTEM_RESET2, for these > vendor reset types to be implemented without requiring drivers for every > register/cookie. > > Add support in PSCI to statically map reboot mode commands from > userspace to a vendor reset and cookie value using the device tree. > > A separate initcall is needed to parse the devicetree, instead of using > psci_dt_init because mm isn't sufficiently set up to allocate memory. Nit: information below this point is more a cover letter than for the commit log. > Reboot mode framework is close but doesn't quite fit with the > design and requirements for PSCI SYSTEM_RESET2. Some of these issues can > be solved but doesn't seem reasonable in sum: > 1. reboot mode registers against the reboot_notifier_list, which is too > early to call SYSTEM_RESET2. PSCI would need to remember the reset > type from the reboot-mode framework callback and use it > psci_sys_reset. > 2. reboot mode assumes only one cookie/parameter is described in the > device tree. SYSTEM_RESET2 uses 2: one for the type and one for > cookie. > 3. psci cpuidle driver already registers a driver against the > arm,psci-1.0 compatible. Refactoring would be needed to have both a > cpuidle and reboot-mode driver. > > Tested-by: Florian Fainelli <florian.fainelli@broadcom.com> > Signed-off-by: Elliot Berman <quic_eberman@quicinc.com> > --- > drivers/firmware/psci/psci.c | 104 +++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 104 insertions(+) > > diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c > index 2328ca58bba61fdb677ac20a1a7447882cd0cf22..e60e3f8749c5a6732c51d23a2c1f453361132d9a 100644 > --- a/drivers/firmware/psci/psci.c > +++ b/drivers/firmware/psci/psci.c > @@ -79,6 +79,14 @@ struct psci_0_1_function_ids get_psci_0_1_function_ids(void) > static u32 psci_cpu_suspend_feature; > static bool psci_system_reset2_supported; > > +struct psci_reset_param { > + const char *mode; > + u32 reset_type; > + u32 cookie; > +}; > +static struct psci_reset_param *psci_reset_params __ro_after_init; > +static size_t num_psci_reset_params __ro_after_init; > + > static inline bool psci_has_ext_power_state(void) > { > return psci_cpu_suspend_feature & > @@ -305,9 +313,38 @@ static int get_set_conduit_method(const struct device_node *np) > return 0; > } > > +static void psci_vendor_system_reset2(const char *cmd) > +{ > + unsigned long ret; > + size_t i; > + > + for (i = 0; i < num_psci_reset_params; i++) { > + if (!strcmp(psci_reset_params[i].mode, cmd)) { > + ret = invoke_psci_fn(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2), > + psci_reset_params[i].reset_type, > + psci_reset_params[i].cookie, 0); > + /* > + * if vendor reset fails, log it and fall back to > + * architecture reset types > + */ > + pr_err("failed to perform reset \"%s\": %ld\n", cmd, > + (long)ret); > + return; > + } > + } > +} > + > static int psci_sys_reset(struct notifier_block *nb, unsigned long action, > void *data) > { > + /* > + * try to do the vendor system_reset2 > + * If the reset fails or there wasn't a match on the command, > + * fall back to architectural resets > + */ > + if (data && num_psci_reset_params) > + psci_vendor_system_reset2(data); > + > if ((reboot_mode == REBOOT_WARM || reboot_mode == REBOOT_SOFT) && > psci_system_reset2_supported) { This is a mess. To issue architectural warm reset we check reboot_mode, for vendor resets we ignore it - there is no rationale, that's the point I am making. Also see my question on the other thread re: user space and reset "modes". I appreciate we are not making progress but I don't want to pick up the pieces later after merging this code - it is unclear to me what's the best path forward - I would like to understand how other platforms/arches behave in this respect. > /* > @@ -750,6 +787,73 @@ static const struct of_device_id psci_of_match[] __initconst = { > {}, > }; > > +#define REBOOT_PREFIX "mode-" > + > +static int __init psci_init_system_reset2_modes(void) > +{ > + const size_t len = strlen(REBOOT_PREFIX); > + struct psci_reset_param *param; > + struct device_node *psci_np __free(device_node) = NULL; > + struct device_node *np __free(device_node) = NULL; > + struct property *prop; > + size_t count = 0; > + u32 magic[2]; > + int num; > + > + if (!psci_system_reset2_supported) > + return 0; > + > + psci_np = of_find_matching_node(NULL, psci_of_match); > + if (!psci_np) > + return 0; > + > + np = of_find_node_by_name(psci_np, "reset-types"); > + if (!np) > + return 0; > + > + for_each_property_of_node(np, prop) { > + if (strncmp(prop->name, REBOOT_PREFIX, len)) > + continue; > + num = of_property_count_u32_elems(np, prop->name); > + if (num != 1 && num != 2) > + continue; > + > + count++; > + } > + > + param = psci_reset_params = > + kcalloc(count, sizeof(*psci_reset_params), GFP_KERNEL); > + if (!psci_reset_params) > + return -ENOMEM; > + > + for_each_property_of_node(np, prop) { > + if (strncmp(prop->name, REBOOT_PREFIX, len)) > + continue; > + > + param->mode = kstrdup_const(prop->name + len, GFP_KERNEL); FWIW - I think you need to keep the logic in the previous loop into account because that's what is used to allocate param, it is not a given that param is valid at this stage if I am not mistaken - the previous loop checked: num = of_property_count_u32_elems(np, prop->name); if (num != 1 && num != 2) continue; Lorenzo > + if (!param->mode) > + continue; > + > + num = of_property_read_variable_u32_array(np, prop->name, magic, > + 1, ARRAY_SIZE(magic)); > + if (num < 0) { > + pr_warn("Failed to parse vendor reboot mode %s\n", > + param->mode); > + kfree_const(param->mode); > + continue; > + } > + > + /* Force reset type to be in vendor space */ > + param->reset_type = PSCI_1_1_RESET_TYPE_VENDOR_START | magic[0]; > + param->cookie = num > 1 ? magic[1] : 0; > + param++; > + num_psci_reset_params++; > + } > + > + return 0; > +} > +arch_initcall(psci_init_system_reset2_modes); > + > int __init psci_dt_init(void) > { > struct device_node *np; > > -- > 2.34.1 >
On Fri, Nov 15, 2024 at 02:51:16PM +0100, Lorenzo Pieralisi wrote: > On Thu, Nov 07, 2024 at 03:38:27PM -0800, Elliot Berman wrote: > > SoC vendors have different types of resets and are controlled through > > various registers. For instance, Qualcomm chipsets can reboot to a > > "download mode" that allows a RAM dump to be collected. Another example > > is they also support writing a cookie that can be read by bootloader > > during next boot. PSCI offers a mechanism, SYSTEM_RESET2, for these > > vendor reset types to be implemented without requiring drivers for every > > register/cookie. > > > > Add support in PSCI to statically map reboot mode commands from > > userspace to a vendor reset and cookie value using the device tree. > > > > A separate initcall is needed to parse the devicetree, instead of using > > psci_dt_init because mm isn't sufficiently set up to allocate memory. > > Nit: information below this point is more a cover letter than for the > commit log. > > > Reboot mode framework is close but doesn't quite fit with the > > design and requirements for PSCI SYSTEM_RESET2. Some of these issues can > > be solved but doesn't seem reasonable in sum: > > 1. reboot mode registers against the reboot_notifier_list, which is too > > early to call SYSTEM_RESET2. PSCI would need to remember the reset > > type from the reboot-mode framework callback and use it > > psci_sys_reset. > > 2. reboot mode assumes only one cookie/parameter is described in the > > device tree. SYSTEM_RESET2 uses 2: one for the type and one for > > cookie. > > 3. psci cpuidle driver already registers a driver against the > > arm,psci-1.0 compatible. Refactoring would be needed to have both a > > cpuidle and reboot-mode driver. > > > > Tested-by: Florian Fainelli <florian.fainelli@broadcom.com> > > Signed-off-by: Elliot Berman <quic_eberman@quicinc.com> > > --- > > drivers/firmware/psci/psci.c | 104 +++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 104 insertions(+) > > > > diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c > > index 2328ca58bba61fdb677ac20a1a7447882cd0cf22..e60e3f8749c5a6732c51d23a2c1f453361132d9a 100644 > > --- a/drivers/firmware/psci/psci.c > > +++ b/drivers/firmware/psci/psci.c > > @@ -79,6 +79,14 @@ struct psci_0_1_function_ids get_psci_0_1_function_ids(void) > > static u32 psci_cpu_suspend_feature; > > static bool psci_system_reset2_supported; > > > > +struct psci_reset_param { > > + const char *mode; > > + u32 reset_type; > > + u32 cookie; > > +}; > > +static struct psci_reset_param *psci_reset_params __ro_after_init; > > +static size_t num_psci_reset_params __ro_after_init; > > + > > static inline bool psci_has_ext_power_state(void) > > { > > return psci_cpu_suspend_feature & > > @@ -305,9 +313,38 @@ static int get_set_conduit_method(const struct device_node *np) > > return 0; > > } > > > > +static void psci_vendor_system_reset2(const char *cmd) > > +{ > > + unsigned long ret; > > + size_t i; > > + > > + for (i = 0; i < num_psci_reset_params; i++) { > > + if (!strcmp(psci_reset_params[i].mode, cmd)) { > > + ret = invoke_psci_fn(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2), > > + psci_reset_params[i].reset_type, > > + psci_reset_params[i].cookie, 0); > > + /* > > + * if vendor reset fails, log it and fall back to > > + * architecture reset types > > + */ > > + pr_err("failed to perform reset \"%s\": %ld\n", cmd, > > + (long)ret); > > + return; > > + } > > + } > > +} > > + > > static int psci_sys_reset(struct notifier_block *nb, unsigned long action, > > void *data) > > { > > + /* > > + * try to do the vendor system_reset2 > > + * If the reset fails or there wasn't a match on the command, > > + * fall back to architectural resets > > + */ > > + if (data && num_psci_reset_params) > > + psci_vendor_system_reset2(data); > > + > > if ((reboot_mode == REBOOT_WARM || reboot_mode == REBOOT_SOFT) && > > psci_system_reset2_supported) { > > This is a mess. To issue architectural warm reset we check reboot_mode, > for vendor resets we ignore it - there is no rationale, that's the point > I am making. If I expand the comment to: * try todo the vendor system_reset2 * If the reset fails or there wasn't a match on the command, * fall back to architectural resets. * Ignore reboot_mode enum to behave like setting a cookie, which don't * care about the reboot_mode. Help to address this concern? > > Also see my question on the other thread re: user space and reset > "modes". > > I appreciate we are not making progress but I don't want to pick up > the pieces later after merging this code - it is unclear to me what's > the best path forward - I would like to understand how other > platforms/arches behave in this respect. > I went through the couple hundred drivers which register reboot and restart handlers. The majority don't care about reboot command nor reboot_mode enum. The few that do: Two drivers which I could find which care about the reboot command don't look at the reboot_mode argument. [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/firmware/efi/efibc.c?h=v6.11#n35 [2]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/power/reset/reboot-mode.c?h=v6.11#n42 One driver looks at the reboot command overrides the reboot_mode argument: [3]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/watchdog/pnx4008_wdt.c?h=v6.11#n125 I wasn't able to find any platform/arches which check the reboot_mode before reading the reboot command. > > /* > > @@ -750,6 +787,73 @@ static const struct of_device_id psci_of_match[] __initconst = { > > {}, > > }; > > > > +#define REBOOT_PREFIX "mode-" > > + > > +static int __init psci_init_system_reset2_modes(void) > > +{ > > + const size_t len = strlen(REBOOT_PREFIX); > > + struct psci_reset_param *param; > > + struct device_node *psci_np __free(device_node) = NULL; > > + struct device_node *np __free(device_node) = NULL; > > + struct property *prop; > > + size_t count = 0; > > + u32 magic[2]; > > + int num; > > + > > + if (!psci_system_reset2_supported) > > + return 0; > > + > > + psci_np = of_find_matching_node(NULL, psci_of_match); > > + if (!psci_np) > > + return 0; > > + > > + np = of_find_node_by_name(psci_np, "reset-types"); > > + if (!np) > > + return 0; > > + > > + for_each_property_of_node(np, prop) { > > + if (strncmp(prop->name, REBOOT_PREFIX, len)) > > + continue; > > + num = of_property_count_u32_elems(np, prop->name); > > + if (num != 1 && num != 2) > > + continue; > > + > > + count++; > > + } > > + > > + param = psci_reset_params = > > + kcalloc(count, sizeof(*psci_reset_params), GFP_KERNEL); > > + if (!psci_reset_params) > > + return -ENOMEM; > > + > > + for_each_property_of_node(np, prop) { > > + if (strncmp(prop->name, REBOOT_PREFIX, len)) > > + continue; > > + > > + param->mode = kstrdup_const(prop->name + len, GFP_KERNEL); > > FWIW - I think you need to keep the logic in the previous loop into account > because that's what is used to allocate param, it is not a given that > param is valid at this stage if I am not mistaken - the previous loop > checked: > > num = of_property_count_u32_elems(np, prop->name); > if (num != 1 && num != 2) > continue; of_property_read_variable_u32_array() performs effectively the same check. It returns -EOVERFLOW if it couldn't find enough (== 0) or too many values (>2). I currently have the added bonus of complaining in dmesg about the bad reboot mode property, instead of silently ignoring. - Elliot > > + if (!param->mode) > > + continue; > > + > > + num = of_property_read_variable_u32_array(np, prop->name, magic, > > + 1, ARRAY_SIZE(magic)); > > + if (num < 0) { > > + pr_warn("Failed to parse vendor reboot mode %s\n", > > + param->mode); > > + kfree_const(param->mode); > > + continue; > > + } > > + > > + /* Force reset type to be in vendor space */ > > + param->reset_type = PSCI_1_1_RESET_TYPE_VENDOR_START | magic[0]; > > + param->cookie = num > 1 ? magic[1] : 0; > > + param++; > > + num_psci_reset_params++; > > + } > > + > > + return 0; > > +} > > +arch_initcall(psci_init_system_reset2_modes); > > + > > int __init psci_dt_init(void) > > { > > struct device_node *np; > > > > -- > > 2.34.1 > >
On Fri, Nov 15, 2024 at 11:08:13AM -0800, Elliot Berman wrote: > On Fri, Nov 15, 2024 at 02:51:16PM +0100, Lorenzo Pieralisi wrote: > > On Thu, Nov 07, 2024 at 03:38:27PM -0800, Elliot Berman wrote: > > > SoC vendors have different types of resets and are controlled through > > > various registers. For instance, Qualcomm chipsets can reboot to a > > > "download mode" that allows a RAM dump to be collected. Another example > > > is they also support writing a cookie that can be read by bootloader > > > during next boot. PSCI offers a mechanism, SYSTEM_RESET2, for these > > > vendor reset types to be implemented without requiring drivers for every > > > register/cookie. > > > > > > Add support in PSCI to statically map reboot mode commands from > > > userspace to a vendor reset and cookie value using the device tree. > > > > > > A separate initcall is needed to parse the devicetree, instead of using > > > psci_dt_init because mm isn't sufficiently set up to allocate memory. > > > > Nit: information below this point is more a cover letter than for the > > commit log. > > > > > Reboot mode framework is close but doesn't quite fit with the > > > design and requirements for PSCI SYSTEM_RESET2. Some of these issues can > > > be solved but doesn't seem reasonable in sum: > > > 1. reboot mode registers against the reboot_notifier_list, which is too > > > early to call SYSTEM_RESET2. PSCI would need to remember the reset > > > type from the reboot-mode framework callback and use it > > > psci_sys_reset. > > > 2. reboot mode assumes only one cookie/parameter is described in the > > > device tree. SYSTEM_RESET2 uses 2: one for the type and one for > > > cookie. > > > 3. psci cpuidle driver already registers a driver against the > > > arm,psci-1.0 compatible. Refactoring would be needed to have both a > > > cpuidle and reboot-mode driver. > > > > > > Tested-by: Florian Fainelli <florian.fainelli@broadcom.com> > > > Signed-off-by: Elliot Berman <quic_eberman@quicinc.com> > > > --- > > > drivers/firmware/psci/psci.c | 104 +++++++++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 104 insertions(+) > > > > > > diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c > > > index 2328ca58bba61fdb677ac20a1a7447882cd0cf22..e60e3f8749c5a6732c51d23a2c1f453361132d9a 100644 > > > --- a/drivers/firmware/psci/psci.c > > > +++ b/drivers/firmware/psci/psci.c > > > @@ -79,6 +79,14 @@ struct psci_0_1_function_ids get_psci_0_1_function_ids(void) > > > static u32 psci_cpu_suspend_feature; > > > static bool psci_system_reset2_supported; > > > > > > +struct psci_reset_param { > > > + const char *mode; > > > + u32 reset_type; > > > + u32 cookie; > > > +}; > > > +static struct psci_reset_param *psci_reset_params __ro_after_init; > > > +static size_t num_psci_reset_params __ro_after_init; > > > + > > > static inline bool psci_has_ext_power_state(void) > > > { > > > return psci_cpu_suspend_feature & > > > @@ -305,9 +313,38 @@ static int get_set_conduit_method(const struct device_node *np) > > > return 0; > > > } > > > > > > +static void psci_vendor_system_reset2(const char *cmd) > > > +{ > > > + unsigned long ret; > > > + size_t i; > > > + > > > + for (i = 0; i < num_psci_reset_params; i++) { > > > + if (!strcmp(psci_reset_params[i].mode, cmd)) { > > > + ret = invoke_psci_fn(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2), > > > + psci_reset_params[i].reset_type, > > > + psci_reset_params[i].cookie, 0); > > > + /* > > > + * if vendor reset fails, log it and fall back to > > > + * architecture reset types > > > + */ > > > + pr_err("failed to perform reset \"%s\": %ld\n", cmd, > > > + (long)ret); > > > + return; > > > + } > > > + } > > > +} > > > + > > > static int psci_sys_reset(struct notifier_block *nb, unsigned long action, > > > void *data) > > > { > > > + /* > > > + * try to do the vendor system_reset2 > > > + * If the reset fails or there wasn't a match on the command, > > > + * fall back to architectural resets > > > + */ > > > + if (data && num_psci_reset_params) > > > + psci_vendor_system_reset2(data); > > > + > > > if ((reboot_mode == REBOOT_WARM || reboot_mode == REBOOT_SOFT) && > > > psci_system_reset2_supported) { > > > > This is a mess. To issue architectural warm reset we check reboot_mode, > > for vendor resets we ignore it - there is no rationale, that's the point > > I am making. > > If I expand the comment to: > > > * try todo the vendor system_reset2 > * If the reset fails or there wasn't a match on the command, > * fall back to architectural resets. > * Ignore reboot_mode enum to behave like setting a cookie, which don't > * care about the reboot_mode. /* * Check if the system supports vendor resets and issue * SYSTEM_RESET2 if the reboot command matches a vendor reset. * Ignore reboot_mode and execute SYSTEM_RESET2 with type and * cookie as defined by the firmware bindings. * * If the reset fails or there is not a match for the command * fall back to architectural resets; reset type detection in * this case will be done using reboot_mode. */ ? > Help to address this concern? Not entirely, sorry, I will get back to this. > > Also see my question on the other thread re: user space and reset > > "modes". > > > > I appreciate we are not making progress but I don't want to pick up > > the pieces later after merging this code - it is unclear to me what's > > the best path forward - I would like to understand how other > > platforms/arches behave in this respect. > > > > I went through the couple hundred drivers which register reboot and > restart handlers. The majority don't care about reboot command nor > reboot_mode enum. The few that do: > > Two drivers which I could find which care about the reboot command don't > look at the reboot_mode argument. > > [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/firmware/efi/efibc.c?h=v6.11#n35 > [2]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/power/reset/reboot-mode.c?h=v6.11#n42 > > One driver looks at the reboot command overrides the reboot_mode > argument: > > [3]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/watchdog/pnx4008_wdt.c?h=v6.11#n125 Thanks for doing that, that helps. > I wasn't able to find any platform/arches which check the reboot_mode > before reading the reboot command. > > > > /* > > > @@ -750,6 +787,73 @@ static const struct of_device_id psci_of_match[] __initconst = { > > > {}, > > > }; > > > > > > +#define REBOOT_PREFIX "mode-" > > > + > > > +static int __init psci_init_system_reset2_modes(void) > > > +{ > > > + const size_t len = strlen(REBOOT_PREFIX); > > > + struct psci_reset_param *param; > > > + struct device_node *psci_np __free(device_node) = NULL; > > > + struct device_node *np __free(device_node) = NULL; > > > + struct property *prop; > > > + size_t count = 0; > > > + u32 magic[2]; > > > + int num; > > > + > > > + if (!psci_system_reset2_supported) > > > + return 0; > > > + > > > + psci_np = of_find_matching_node(NULL, psci_of_match); > > > + if (!psci_np) > > > + return 0; > > > + > > > + np = of_find_node_by_name(psci_np, "reset-types"); > > > + if (!np) > > > + return 0; > > > + > > > + for_each_property_of_node(np, prop) { > > > + if (strncmp(prop->name, REBOOT_PREFIX, len)) > > > + continue; > > > + num = of_property_count_u32_elems(np, prop->name); > > > + if (num != 1 && num != 2) > > > + continue; > > > + > > > + count++; > > > + } > > > + > > > + param = psci_reset_params = > > > + kcalloc(count, sizeof(*psci_reset_params), GFP_KERNEL); > > > + if (!psci_reset_params) > > > + return -ENOMEM; > > > + > > > + for_each_property_of_node(np, prop) { > > > + if (strncmp(prop->name, REBOOT_PREFIX, len)) > > > + continue; > > > + > > > + param->mode = kstrdup_const(prop->name + len, GFP_KERNEL); > > > > FWIW - I think you need to keep the logic in the previous loop into account > > because that's what is used to allocate param, it is not a given that > > param is valid at this stage if I am not mistaken - the previous loop > > checked: > > > > num = of_property_count_u32_elems(np, prop->name); > > if (num != 1 && num != 2) > > continue; > > of_property_read_variable_u32_array() performs effectively the same > check. It returns -EOVERFLOW if it couldn't find enough (== 0) or too > many values (>2). I currently have the added bonus of complaining in > dmesg about the bad reboot mode property, instead of silently ignoring. Right but we are dereferencing param (param->mode) before carrying out that check. Thanks, Lorenzo > > - Elliot > > > > + if (!param->mode) > > > + continue; > > > + > > > + num = of_property_read_variable_u32_array(np, prop->name, magic, > > > + 1, ARRAY_SIZE(magic)); > > > + if (num < 0) { > > > + pr_warn("Failed to parse vendor reboot mode %s\n", > > > + param->mode); > > > + kfree_const(param->mode); > > > + continue; > > > + } > > > + > > > + /* Force reset type to be in vendor space */ > > > + param->reset_type = PSCI_1_1_RESET_TYPE_VENDOR_START | magic[0]; > > > + param->cookie = num > 1 ? magic[1] : 0; > > > + param++; > > > + num_psci_reset_params++; > > > + } > > > + > > > + return 0; > > > +} > > > +arch_initcall(psci_init_system_reset2_modes); > > > + > > > int __init psci_dt_init(void) > > > { > > > struct device_node *np; > > > > > > -- > > > 2.34.1 > > >
On Mon, Nov 18, 2024 at 01:26:48PM +0100, Lorenzo Pieralisi wrote: > On Fri, Nov 15, 2024 at 11:08:13AM -0800, Elliot Berman wrote: > > On Fri, Nov 15, 2024 at 02:51:16PM +0100, Lorenzo Pieralisi wrote: > > > On Thu, Nov 07, 2024 at 03:38:27PM -0800, Elliot Berman wrote: > > > > SoC vendors have different types of resets and are controlled through > > > > various registers. For instance, Qualcomm chipsets can reboot to a > > > > "download mode" that allows a RAM dump to be collected. Another example > > > > is they also support writing a cookie that can be read by bootloader > > > > during next boot. PSCI offers a mechanism, SYSTEM_RESET2, for these > > > > vendor reset types to be implemented without requiring drivers for every > > > > register/cookie. > > > > > > > > Add support in PSCI to statically map reboot mode commands from > > > > userspace to a vendor reset and cookie value using the device tree. > > > > > > > > A separate initcall is needed to parse the devicetree, instead of using > > > > psci_dt_init because mm isn't sufficiently set up to allocate memory. > > > > > > Nit: information below this point is more a cover letter than for the > > > commit log. > > > > > > > Reboot mode framework is close but doesn't quite fit with the > > > > design and requirements for PSCI SYSTEM_RESET2. Some of these issues can > > > > be solved but doesn't seem reasonable in sum: > > > > 1. reboot mode registers against the reboot_notifier_list, which is too > > > > early to call SYSTEM_RESET2. PSCI would need to remember the reset > > > > type from the reboot-mode framework callback and use it > > > > psci_sys_reset. > > > > 2. reboot mode assumes only one cookie/parameter is described in the > > > > device tree. SYSTEM_RESET2 uses 2: one for the type and one for > > > > cookie. > > > > 3. psci cpuidle driver already registers a driver against the > > > > arm,psci-1.0 compatible. Refactoring would be needed to have both a > > > > cpuidle and reboot-mode driver. > > > > > > > > Tested-by: Florian Fainelli <florian.fainelli@broadcom.com> > > > > Signed-off-by: Elliot Berman <quic_eberman@quicinc.com> > > > > --- > > > > drivers/firmware/psci/psci.c | 104 +++++++++++++++++++++++++++++++++++++++++++ > > > > 1 file changed, 104 insertions(+) > > > > > > > > diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c > > > > index 2328ca58bba61fdb677ac20a1a7447882cd0cf22..e60e3f8749c5a6732c51d23a2c1f453361132d9a 100644 > > > > --- a/drivers/firmware/psci/psci.c > > > > +++ b/drivers/firmware/psci/psci.c > > > > @@ -79,6 +79,14 @@ struct psci_0_1_function_ids get_psci_0_1_function_ids(void) > > > > static u32 psci_cpu_suspend_feature; > > > > static bool psci_system_reset2_supported; > > > > > > > > +struct psci_reset_param { > > > > + const char *mode; > > > > + u32 reset_type; > > > > + u32 cookie; > > > > +}; > > > > +static struct psci_reset_param *psci_reset_params __ro_after_init; > > > > +static size_t num_psci_reset_params __ro_after_init; > > > > + > > > > static inline bool psci_has_ext_power_state(void) > > > > { > > > > return psci_cpu_suspend_feature & > > > > @@ -305,9 +313,38 @@ static int get_set_conduit_method(const struct device_node *np) > > > > return 0; > > > > } > > > > > > > > +static void psci_vendor_system_reset2(const char *cmd) > > > > +{ > > > > + unsigned long ret; > > > > + size_t i; > > > > + > > > > + for (i = 0; i < num_psci_reset_params; i++) { > > > > + if (!strcmp(psci_reset_params[i].mode, cmd)) { > > > > + ret = invoke_psci_fn(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2), > > > > + psci_reset_params[i].reset_type, > > > > + psci_reset_params[i].cookie, 0); > > > > + /* > > > > + * if vendor reset fails, log it and fall back to > > > > + * architecture reset types > > > > + */ > > > > + pr_err("failed to perform reset \"%s\": %ld\n", cmd, > > > > + (long)ret); > > > > + return; > > > > + } > > > > + } > > > > +} > > > > + > > > > static int psci_sys_reset(struct notifier_block *nb, unsigned long action, > > > > void *data) > > > > { > > > > + /* > > > > + * try to do the vendor system_reset2 > > > > + * If the reset fails or there wasn't a match on the command, > > > > + * fall back to architectural resets > > > > + */ > > > > + if (data && num_psci_reset_params) > > > > + psci_vendor_system_reset2(data); > > > > + > > > > if ((reboot_mode == REBOOT_WARM || reboot_mode == REBOOT_SOFT) && > > > > psci_system_reset2_supported) { > > > > > > This is a mess. To issue architectural warm reset we check reboot_mode, > > > for vendor resets we ignore it - there is no rationale, that's the point > > > I am making. > > > > If I expand the comment to: > > > > > > * try todo the vendor system_reset2 > > * If the reset fails or there wasn't a match on the command, > > * fall back to architectural resets. > > * Ignore reboot_mode enum to behave like setting a cookie, which don't > > * care about the reboot_mode. > > /* > * Check if the system supports vendor resets and issue > * SYSTEM_RESET2 if the reboot command matches a vendor reset. > * Ignore reboot_mode and execute SYSTEM_RESET2 with type and > * cookie as defined by the firmware bindings. > * > * If the reset fails or there is not a match for the command > * fall back to architectural resets; reset type detection in > * this case will be done using reboot_mode. > */ > > ? > > > Help to address this concern? > > Not entirely, sorry, I will get back to this. > > > > Also see my question on the other thread re: user space and reset > > > "modes". > > > > > > I appreciate we are not making progress but I don't want to pick up > > > the pieces later after merging this code - it is unclear to me what's > > > the best path forward - I would like to understand how other > > > platforms/arches behave in this respect. > > > > > > > I went through the couple hundred drivers which register reboot and > > restart handlers. The majority don't care about reboot command nor > > reboot_mode enum. The few that do: > > > > Two drivers which I could find which care about the reboot command don't > > look at the reboot_mode argument. > > > > [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/firmware/efi/efibc.c?h=v6.11#n35 > > [2]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/power/reset/reboot-mode.c?h=v6.11#n42 > > > > One driver looks at the reboot command overrides the reboot_mode > > argument: > > > > [3]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/watchdog/pnx4008_wdt.c?h=v6.11#n125 > > Thanks for doing that, that helps. > > > I wasn't able to find any platform/arches which check the reboot_mode > > before reading the reboot command. > > > > > > /* > > > > @@ -750,6 +787,73 @@ static const struct of_device_id psci_of_match[] __initconst = { > > > > {}, > > > > }; > > > > > > > > +#define REBOOT_PREFIX "mode-" > > > > + > > > > +static int __init psci_init_system_reset2_modes(void) > > > > +{ > > > > + const size_t len = strlen(REBOOT_PREFIX); > > > > + struct psci_reset_param *param; > > > > + struct device_node *psci_np __free(device_node) = NULL; > > > > + struct device_node *np __free(device_node) = NULL; > > > > + struct property *prop; > > > > + size_t count = 0; > > > > + u32 magic[2]; > > > > + int num; > > > > + > > > > + if (!psci_system_reset2_supported) > > > > + return 0; > > > > + > > > > + psci_np = of_find_matching_node(NULL, psci_of_match); > > > > + if (!psci_np) > > > > + return 0; > > > > + > > > > + np = of_find_node_by_name(psci_np, "reset-types"); > > > > + if (!np) > > > > + return 0; > > > > + > > > > + for_each_property_of_node(np, prop) { > > > > + if (strncmp(prop->name, REBOOT_PREFIX, len)) > > > > + continue; > > > > + num = of_property_count_u32_elems(np, prop->name); > > > > + if (num != 1 && num != 2) > > > > + continue; > > > > + > > > > + count++; > > > > + } > > > > + > > > > + param = psci_reset_params = > > > > + kcalloc(count, sizeof(*psci_reset_params), GFP_KERNEL); > > > > + if (!psci_reset_params) > > > > + return -ENOMEM; > > > > + > > > > + for_each_property_of_node(np, prop) { > > > > + if (strncmp(prop->name, REBOOT_PREFIX, len)) > > > > + continue; > > > > + > > > > + param->mode = kstrdup_const(prop->name + len, GFP_KERNEL); > > > > > > FWIW - I think you need to keep the logic in the previous loop into account > > > because that's what is used to allocate param, it is not a given that > > > param is valid at this stage if I am not mistaken - the previous loop > > > checked: > > > > > > num = of_property_count_u32_elems(np, prop->name); > > > if (num != 1 && num != 2) > > > continue; > > > > of_property_read_variable_u32_array() performs effectively the same > > check. It returns -EOVERFLOW if it couldn't find enough (== 0) or too > > many values (>2). I currently have the added bonus of complaining in > > dmesg about the bad reboot mode property, instead of silently ignoring. > > Right but we are dereferencing param (param->mode) before carrying out that > check. > Ah, right, I see the problem. I'll send out another version after settling on the other part of the discussion! Thanks, Elliot
On Thu, Nov 07, 2024 at 03:38:27PM -0800, Elliot Berman wrote: > SoC vendors have different types of resets and are controlled through > various registers. For instance, Qualcomm chipsets can reboot to a > "download mode" that allows a RAM dump to be collected. Another example > is they also support writing a cookie that can be read by bootloader > during next boot. PSCI offers a mechanism, SYSTEM_RESET2, for these > vendor reset types to be implemented without requiring drivers for every > register/cookie. > > Add support in PSCI to statically map reboot mode commands from > userspace to a vendor reset and cookie value using the device tree. > > A separate initcall is needed to parse the devicetree, instead of using > psci_dt_init because mm isn't sufficiently set up to allocate memory. > > Reboot mode framework is close but doesn't quite fit with the > design and requirements for PSCI SYSTEM_RESET2. Some of these issues can > be solved but doesn't seem reasonable in sum: > 1. reboot mode registers against the reboot_notifier_list, which is too > early to call SYSTEM_RESET2. PSCI would need to remember the reset > type from the reboot-mode framework callback and use it > psci_sys_reset. > 2. reboot mode assumes only one cookie/parameter is described in the > device tree. SYSTEM_RESET2 uses 2: one for the type and one for > cookie. > 3. psci cpuidle driver already registers a driver against the > arm,psci-1.0 compatible. Refactoring would be needed to have both a > cpuidle and reboot-mode driver. > > Tested-by: Florian Fainelli <florian.fainelli@broadcom.com> > Signed-off-by: Elliot Berman <quic_eberman@quicinc.com> > --- > drivers/firmware/psci/psci.c | 104 +++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 104 insertions(+) > > diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c > index 2328ca58bba61fdb677ac20a1a7447882cd0cf22..e60e3f8749c5a6732c51d23a2c1f453361132d9a 100644 > --- a/drivers/firmware/psci/psci.c > +++ b/drivers/firmware/psci/psci.c > @@ -79,6 +79,14 @@ struct psci_0_1_function_ids get_psci_0_1_function_ids(void) > static u32 psci_cpu_suspend_feature; > static bool psci_system_reset2_supported; > > +struct psci_reset_param { > + const char *mode; > + u32 reset_type; > + u32 cookie; > +}; > +static struct psci_reset_param *psci_reset_params __ro_after_init; > +static size_t num_psci_reset_params __ro_after_init; > + > static inline bool psci_has_ext_power_state(void) > { > return psci_cpu_suspend_feature & > @@ -305,9 +313,38 @@ static int get_set_conduit_method(const struct device_node *np) > return 0; > } > > +static void psci_vendor_system_reset2(const char *cmd) > +{ > + unsigned long ret; > + size_t i; > + > + for (i = 0; i < num_psci_reset_params; i++) { > + if (!strcmp(psci_reset_params[i].mode, cmd)) { > + ret = invoke_psci_fn(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2), > + psci_reset_params[i].reset_type, > + psci_reset_params[i].cookie, 0); > + /* > + * if vendor reset fails, log it and fall back to > + * architecture reset types > + */ > + pr_err("failed to perform reset \"%s\": %ld\n", cmd, > + (long)ret); > + return; > + } > + } > +} > + > static int psci_sys_reset(struct notifier_block *nb, unsigned long action, > void *data) > { > + /* > + * try to do the vendor system_reset2 > + * If the reset fails or there wasn't a match on the command, > + * fall back to architectural resets > + */ > + if (data && num_psci_reset_params) > + psci_vendor_system_reset2(data); Mulling over this. If a command (data) was provided and a PSCI vendor reset parsed at boot, if the vendor reset fails, isn't it correct to just fail reboot instead of falling back to architectural resets ? What's missing is defining the "contract" between the LINUX_REBOOT_CMD_RESTART2 arg parameter and the kernel reboot type that is executed. I do wonder whether this is an opportunity to deprecate reboot_mode altogether on arm64 (I think that the relationship between REBOOT_WARM and REBOOT_SOFT with PSCI arch warm reset is already loose - let alone falling back to cold reset if reboot_mode == REBOOT_GPIO - which does not make any sense at all simply because REBOOT_GPIO is ill-defined to say the least). Thoughts ? Thanks, Lorenzo > + > if ((reboot_mode == REBOOT_WARM || reboot_mode == REBOOT_SOFT) && > psci_system_reset2_supported) { > /* > @@ -750,6 +787,73 @@ static const struct of_device_id psci_of_match[] __initconst = { > {}, > }; > > +#define REBOOT_PREFIX "mode-" > + > +static int __init psci_init_system_reset2_modes(void) > +{ > + const size_t len = strlen(REBOOT_PREFIX); > + struct psci_reset_param *param; > + struct device_node *psci_np __free(device_node) = NULL; > + struct device_node *np __free(device_node) = NULL; > + struct property *prop; > + size_t count = 0; > + u32 magic[2]; > + int num; > + > + if (!psci_system_reset2_supported) > + return 0; > + > + psci_np = of_find_matching_node(NULL, psci_of_match); > + if (!psci_np) > + return 0; > + > + np = of_find_node_by_name(psci_np, "reset-types"); > + if (!np) > + return 0; > + > + for_each_property_of_node(np, prop) { > + if (strncmp(prop->name, REBOOT_PREFIX, len)) > + continue; > + num = of_property_count_u32_elems(np, prop->name); > + if (num != 1 && num != 2) > + continue; > + > + count++; > + } > + > + param = psci_reset_params = > + kcalloc(count, sizeof(*psci_reset_params), GFP_KERNEL); > + if (!psci_reset_params) > + return -ENOMEM; > + > + for_each_property_of_node(np, prop) { > + if (strncmp(prop->name, REBOOT_PREFIX, len)) > + continue; > + > + param->mode = kstrdup_const(prop->name + len, GFP_KERNEL); > + if (!param->mode) > + continue; > + > + num = of_property_read_variable_u32_array(np, prop->name, magic, > + 1, ARRAY_SIZE(magic)); > + if (num < 0) { > + pr_warn("Failed to parse vendor reboot mode %s\n", > + param->mode); > + kfree_const(param->mode); > + continue; > + } > + > + /* Force reset type to be in vendor space */ > + param->reset_type = PSCI_1_1_RESET_TYPE_VENDOR_START | magic[0]; > + param->cookie = num > 1 ? magic[1] : 0; > + param++; > + num_psci_reset_params++; > + } > + > + return 0; > +} > +arch_initcall(psci_init_system_reset2_modes); > + > int __init psci_dt_init(void) > { > struct device_node *np; > > -- > 2.34.1 >
On Wed, Feb 26, 2025 at 03:28:47PM +0100, Lorenzo Pieralisi wrote: > On Thu, Nov 07, 2024 at 03:38:27PM -0800, Elliot Berman wrote: > > SoC vendors have different types of resets and are controlled through > > various registers. For instance, Qualcomm chipsets can reboot to a > > "download mode" that allows a RAM dump to be collected. Another example > > is they also support writing a cookie that can be read by bootloader > > during next boot. PSCI offers a mechanism, SYSTEM_RESET2, for these > > vendor reset types to be implemented without requiring drivers for every > > register/cookie. > > > > Add support in PSCI to statically map reboot mode commands from > > userspace to a vendor reset and cookie value using the device tree. > > > > A separate initcall is needed to parse the devicetree, instead of using > > psci_dt_init because mm isn't sufficiently set up to allocate memory. > > > > Reboot mode framework is close but doesn't quite fit with the > > design and requirements for PSCI SYSTEM_RESET2. Some of these issues can > > be solved but doesn't seem reasonable in sum: > > 1. reboot mode registers against the reboot_notifier_list, which is too > > early to call SYSTEM_RESET2. PSCI would need to remember the reset > > type from the reboot-mode framework callback and use it > > psci_sys_reset. > > 2. reboot mode assumes only one cookie/parameter is described in the > > device tree. SYSTEM_RESET2 uses 2: one for the type and one for > > cookie. > > 3. psci cpuidle driver already registers a driver against the > > arm,psci-1.0 compatible. Refactoring would be needed to have both a > > cpuidle and reboot-mode driver. > > > > Tested-by: Florian Fainelli <florian.fainelli@broadcom.com> > > Signed-off-by: Elliot Berman <quic_eberman@quicinc.com> > > --- > > drivers/firmware/psci/psci.c | 104 +++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 104 insertions(+) > > > > diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c > > index 2328ca58bba61fdb677ac20a1a7447882cd0cf22..e60e3f8749c5a6732c51d23a2c1f453361132d9a 100644 > > --- a/drivers/firmware/psci/psci.c > > +++ b/drivers/firmware/psci/psci.c > > @@ -79,6 +79,14 @@ struct psci_0_1_function_ids get_psci_0_1_function_ids(void) > > static u32 psci_cpu_suspend_feature; > > static bool psci_system_reset2_supported; > > > > +struct psci_reset_param { > > + const char *mode; > > + u32 reset_type; > > + u32 cookie; > > +}; > > +static struct psci_reset_param *psci_reset_params __ro_after_init; > > +static size_t num_psci_reset_params __ro_after_init; > > + > > static inline bool psci_has_ext_power_state(void) > > { > > return psci_cpu_suspend_feature & > > @@ -305,9 +313,38 @@ static int get_set_conduit_method(const struct device_node *np) > > return 0; > > } > > > > +static void psci_vendor_system_reset2(const char *cmd) > > +{ > > + unsigned long ret; > > + size_t i; > > + > > + for (i = 0; i < num_psci_reset_params; i++) { > > + if (!strcmp(psci_reset_params[i].mode, cmd)) { > > + ret = invoke_psci_fn(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2), > > + psci_reset_params[i].reset_type, > > + psci_reset_params[i].cookie, 0); > > + /* > > + * if vendor reset fails, log it and fall back to > > + * architecture reset types > > + */ > > + pr_err("failed to perform reset \"%s\": %ld\n", cmd, > > + (long)ret); > > + return; > > + } > > + } > > +} > > + > > static int psci_sys_reset(struct notifier_block *nb, unsigned long action, > > void *data) > > { > > + /* > > + * try to do the vendor system_reset2 > > + * If the reset fails or there wasn't a match on the command, > > + * fall back to architectural resets > > + */ > > + if (data && num_psci_reset_params) > > + psci_vendor_system_reset2(data); > > Mulling over this. If a command (data) was provided and a PSCI vendor > reset parsed at boot, if the vendor reset fails, isn't it correct to > just fail reboot instead of falling back to architectural resets ? > Sure! I can change the behavior here. > What's missing is defining the "contract" between the > LINUX_REBOOT_CMD_RESTART2 arg parameter and the kernel reboot > type that is executed. > > I do wonder whether this is an opportunity to deprecate reboot_mode > altogether on arm64 (I think that the relationship between REBOOT_WARM > and REBOOT_SOFT with PSCI arch warm reset is already loose - let alone > falling back to cold reset if reboot_mode == REBOOT_GPIO - which does > not make any sense at all simply because REBOOT_GPIO is ill-defined to > say the least). > > Thoughts ? > I'm not opposed to seeing how we can deprecate the reboot_mode enum for arm64, but I think this should be an independent effort from the vendor resets. I'm worried this takes us down the "don't let perfect be the enemy of good" path to support vendor resets. I haven't seen much interest in this issue outside the two of us, and thus changing important infrastructure like reboot seems to me to be a long shot. Thanks, Elliot
diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c index 2328ca58bba61fdb677ac20a1a7447882cd0cf22..e60e3f8749c5a6732c51d23a2c1f453361132d9a 100644 --- a/drivers/firmware/psci/psci.c +++ b/drivers/firmware/psci/psci.c @@ -79,6 +79,14 @@ struct psci_0_1_function_ids get_psci_0_1_function_ids(void) static u32 psci_cpu_suspend_feature; static bool psci_system_reset2_supported; +struct psci_reset_param { + const char *mode; + u32 reset_type; + u32 cookie; +}; +static struct psci_reset_param *psci_reset_params __ro_after_init; +static size_t num_psci_reset_params __ro_after_init; + static inline bool psci_has_ext_power_state(void) { return psci_cpu_suspend_feature & @@ -305,9 +313,38 @@ static int get_set_conduit_method(const struct device_node *np) return 0; } +static void psci_vendor_system_reset2(const char *cmd) +{ + unsigned long ret; + size_t i; + + for (i = 0; i < num_psci_reset_params; i++) { + if (!strcmp(psci_reset_params[i].mode, cmd)) { + ret = invoke_psci_fn(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2), + psci_reset_params[i].reset_type, + psci_reset_params[i].cookie, 0); + /* + * if vendor reset fails, log it and fall back to + * architecture reset types + */ + pr_err("failed to perform reset \"%s\": %ld\n", cmd, + (long)ret); + return; + } + } +} + static int psci_sys_reset(struct notifier_block *nb, unsigned long action, void *data) { + /* + * try to do the vendor system_reset2 + * If the reset fails or there wasn't a match on the command, + * fall back to architectural resets + */ + if (data && num_psci_reset_params) + psci_vendor_system_reset2(data); + if ((reboot_mode == REBOOT_WARM || reboot_mode == REBOOT_SOFT) && psci_system_reset2_supported) { /* @@ -750,6 +787,73 @@ static const struct of_device_id psci_of_match[] __initconst = { {}, }; +#define REBOOT_PREFIX "mode-" + +static int __init psci_init_system_reset2_modes(void) +{ + const size_t len = strlen(REBOOT_PREFIX); + struct psci_reset_param *param; + struct device_node *psci_np __free(device_node) = NULL; + struct device_node *np __free(device_node) = NULL; + struct property *prop; + size_t count = 0; + u32 magic[2]; + int num; + + if (!psci_system_reset2_supported) + return 0; + + psci_np = of_find_matching_node(NULL, psci_of_match); + if (!psci_np) + return 0; + + np = of_find_node_by_name(psci_np, "reset-types"); + if (!np) + return 0; + + for_each_property_of_node(np, prop) { + if (strncmp(prop->name, REBOOT_PREFIX, len)) + continue; + num = of_property_count_u32_elems(np, prop->name); + if (num != 1 && num != 2) + continue; + + count++; + } + + param = psci_reset_params = + kcalloc(count, sizeof(*psci_reset_params), GFP_KERNEL); + if (!psci_reset_params) + return -ENOMEM; + + for_each_property_of_node(np, prop) { + if (strncmp(prop->name, REBOOT_PREFIX, len)) + continue; + + param->mode = kstrdup_const(prop->name + len, GFP_KERNEL); + if (!param->mode) + continue; + + num = of_property_read_variable_u32_array(np, prop->name, magic, + 1, ARRAY_SIZE(magic)); + if (num < 0) { + pr_warn("Failed to parse vendor reboot mode %s\n", + param->mode); + kfree_const(param->mode); + continue; + } + + /* Force reset type to be in vendor space */ + param->reset_type = PSCI_1_1_RESET_TYPE_VENDOR_START | magic[0]; + param->cookie = num > 1 ? magic[1] : 0; + param++; + num_psci_reset_params++; + } + + return 0; +} +arch_initcall(psci_init_system_reset2_modes); + int __init psci_dt_init(void) { struct device_node *np;
SoC vendors have different types of resets and are controlled through various registers. For instance, Qualcomm chipsets can reboot to a "download mode" that allows a RAM dump to be collected. Another example is they also support writing a cookie that can be read by bootloader during next boot. PSCI offers a mechanism, SYSTEM_RESET2, for these vendor reset types to be implemented without requiring drivers for every register/cookie. Add support in PSCI to statically map reboot mode commands from userspace to a vendor reset and cookie value using the device tree. A separate initcall is needed to parse the devicetree, instead of using psci_dt_init because mm isn't sufficiently set up to allocate memory. Reboot mode framework is close but doesn't quite fit with the design and requirements for PSCI SYSTEM_RESET2. Some of these issues can be solved but doesn't seem reasonable in sum: 1. reboot mode registers against the reboot_notifier_list, which is too early to call SYSTEM_RESET2. PSCI would need to remember the reset type from the reboot-mode framework callback and use it psci_sys_reset. 2. reboot mode assumes only one cookie/parameter is described in the device tree. SYSTEM_RESET2 uses 2: one for the type and one for cookie. 3. psci cpuidle driver already registers a driver against the arm,psci-1.0 compatible. Refactoring would be needed to have both a cpuidle and reboot-mode driver. Tested-by: Florian Fainelli <florian.fainelli@broadcom.com> Signed-off-by: Elliot Berman <quic_eberman@quicinc.com> --- drivers/firmware/psci/psci.c | 104 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 104 insertions(+)