| Message ID | 20240617-arm-psci-system_reset2-vendor-reboots-v5-3-086950f650c8@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On Mon, Jun 17, 2024 at 10:18:09AM -0700, 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. > I need to think through it but when you first introduced the generic Documentation/devicetree/bindings/power/reset/reboot-mode.yaml bindings I also looked at drivers/power/reset/reboot-mode.c I assumed this extension to that binding would reuse the same and PSCI would just do reboot_mode_register(). I didn't expect to see these changes. I might have missing something but since the bindings is still quite generic with additional cells that act as additional cookie for reboot call, I still think that should be possible. What am I missing here then ?
On Wed, Jun 19, 2024 at 02:51:43PM +0100, Sudeep Holla wrote: > On Mon, Jun 17, 2024 at 10:18:09AM -0700, 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. > > > > I need to think through it but when you first introduced the generic > Documentation/devicetree/bindings/power/reset/reboot-mode.yaml bindings > I also looked at drivers/power/reset/reboot-mode.c > > I assumed this extension to that binding would reuse the same and > PSCI would just do reboot_mode_register(). I didn't expect to see these > changes. I might have missing something but since the bindings is still > quite generic with additional cells that act as additional cookie for > reboot call, I still think that should be possible. > > What am I missing here then ? > Right, if that was only thing to "solve" to make it easy to use reboot-mode framework, I agree we should update reboot mode framework to work with the additional cells. There are a few other issues I mention above which, when combined, make me feel that PSCI is different enough from how reboot mode framework works that we shouldn't try to make PSCI work with the framework. Issues #1 and #2 are pretty easy to solve (whether they should be solved is different); I'm not sure a good approach to issue #3. Thanks, Elliot
Hi Sudeep and Sebastian, On Wed, Jun 19, 2024 at 08:28:06AM -0700, Elliot Berman wrote: > On Wed, Jun 19, 2024 at 02:51:43PM +0100, Sudeep Holla wrote: > > On Mon, Jun 17, 2024 at 10:18:09AM -0700, 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. > > > > > > > I need to think through it but when you first introduced the generic > > Documentation/devicetree/bindings/power/reset/reboot-mode.yaml bindings > > I also looked at drivers/power/reset/reboot-mode.c > > > > I assumed this extension to that binding would reuse the same and > > PSCI would just do reboot_mode_register(). I didn't expect to see these > > changes. I might have missing something but since the bindings is still > > quite generic with additional cells that act as additional cookie for > > reboot call, I still think that should be possible. > > > > What am I missing here then ? > > > > Right, if that was only thing to "solve" to make it easy to use > reboot-mode framework, I agree we should update reboot mode framework to > work with the additional cells. There are a few other issues I mention > above which, when combined, make me feel that PSCI is different enough > from how reboot mode framework works that we shouldn't try to make PSCI > work with the framework. Issues #1 and #2 are pretty easy to solve > (whether they should be solved is different); I'm not sure a good > approach to issue #3. > Does the reasoning I mention in the commit text make sense why PSCI should avoid using the reboot-mode.c framework? Thanks, Elliot
On Mon, Jun 17, 2024 at 10:18:09AM -0700, 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. Fair enough, good reason but as I mentioned I haven't looked at it in details to provide any suggestion or to assert it can be solved. > 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. I assumed it shouldn't be too difficult to extend it to support 2 parameters. > 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. This one seems not a strong reason in my opinion. It was not designed for cpuidle and it can't bind to that solely. If this becomes only reason, then we need to fix that.
diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c index d9629ff87861..e672b33b71d1 100644 --- a/drivers/firmware/psci/psci.c +++ b/drivers/firmware/psci/psci.c @@ -29,6 +29,8 @@ #include <asm/smp_plat.h> #include <asm/suspend.h> +#define REBOOT_PREFIX "mode-" + /* * While a 64-bit OS can make calls with SMC32 calling conventions, for some * calls it is necessary to use SMC64 to pass or return 64-bit values. @@ -79,6 +81,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; +static size_t num_psci_reset_params; + static inline bool psci_has_ext_power_state(void) { return psci_cpu_suspend_feature & @@ -305,9 +315,29 @@ static int get_set_conduit_method(const struct device_node *np) return 0; } +static void psci_vendor_sys_reset2(unsigned long action, void *data) +{ + const char *cmd = data; + 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); + pr_err("failed to perform reset \"%s\": %ld\n", + cmd, (long)ret); + } + } +} + static int psci_sys_reset(struct notifier_block *nb, unsigned long action, void *data) { + if (data && num_psci_reset_params) + psci_vendor_sys_reset2(action, data); + if ((reboot_mode == REBOOT_WARM || reboot_mode == REBOOT_SOFT) && psci_system_reset2_supported) { /* @@ -748,6 +778,68 @@ static const struct of_device_id psci_of_match[] __initconst = { {}, }; +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_elems_of_size(np, prop->name, sizeof(magic[0])); + 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, 2); + 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 == 2 ? 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 | 92 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 92 insertions(+)