| Message ID | 20241018-arm-psci-system_reset2-vendor-reboots-v6-0-50cbe88b0a24@quicinc.com |
|---|---|
| Headers | show |
| Series | Implement vendor resets for PSCI SYSTEM_RESET2 | expand |
On Fri, Oct 18, 2024 at 10:42:46PM -0700, Stephen Boyd wrote: > Quoting Elliot Berman (2024-10-18 12:39:48) > > diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c > > index 2328ca58bba6..60bc285622ce 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-" > > Maybe move this near the function that uses it. > > > + > > /* > > * 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. > > @@ -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); > > Do this intentionally return? Should it be some other function that's > __noreturn instead and a while (1) if the firmware returns back to the > kernel? > Yes, I think it's best to make sure we fall back to the architectural reset (whether it's the SYSTEM_RESET or architectural SYSTEM_RESET2) since device would reboot then. > > + } > > + } > > +} > > + > > 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) { > > /* > > @@ -750,6 +780,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])); > > Use of_property_count_u32_elems()? > > > + 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); > > ARRAY_SIZE(magic)? > > > + if (num < 0) { > > Should this be less than 1? > of_property_read_variable_u32_array should return -EOVERFLOW (or maybe -ENODATA) if the array is empty. I don't see it's possible for of_property_read_variable_u32_array() to return a non-negative value that's not 1 or 2. > > + 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; > > ARRAY_SIZE(magic)? > > > + param++; > > + num_psci_reset_params++; > > + } > > + > > + return 0;
Quoting Elliot Berman (2024-10-23 09:30:21) > On Fri, Oct 18, 2024 at 10:42:46PM -0700, Stephen Boyd wrote: > > Quoting Elliot Berman (2024-10-18 12:39:48) > > > diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c > > > index 2328ca58bba6..60bc285622ce 100644 > > > --- a/drivers/firmware/psci/psci.c > > > +++ b/drivers/firmware/psci/psci.c > > > @@ -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); > > > > Do this intentionally return? Should it be some other function that's > > __noreturn instead and a while (1) if the firmware returns back to the > > kernel? > > > > Yes, I think it's best to make sure we fall back to the architectural > reset (whether it's the SYSTEM_RESET or architectural SYSTEM_RESET2) > since device would reboot then. Ok. Please add a comment in the code so we know that it's intentional. > > > > + } > > > + } > > > +} > > > + > > > 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); > > > + I'd add a comment here as well indicating that a fallback is used. > > > if ((reboot_mode == REBOOT_WARM || reboot_mode == REBOOT_SOFT) && > > > psci_system_reset2_supported) { > > > /* > > > @@ -750,6 +780,68 @@ static const struct of_device_id psci_of_match[] __initconst = { > > > {}, [...] > > > + continue; > > > + > > > + num = of_property_read_variable_u32_array(np, prop->name, magic, 1, 2); > > > > ARRAY_SIZE(magic)? > > > > > + if (num < 0) { > > > > Should this be less than 1? > > > > of_property_read_variable_u32_array should return -EOVERFLOW (or maybe > -ENODATA) if the array is empty. I don't see it's possible for > of_property_read_variable_u32_array() to return a non-negative value > that's not 1 or 2. I think you're saying a return value of 0 is impossible? Ok. I was mostly looking at the usage of the return value later on in this patch and trying to understand why 0 would be allowed as a possible return value without looking at the details of of_property_read_variable_u32_array(). I guess the 1, 2 are the min/max though so it's fine.
The PSCI SYSTEM_RESET2 call allows vendor firmware to define additional reset types which could be mapped to the reboot argument. Setting up reboot on Qualcomm devices can be inconsistent from chipset to chipset. Generally, there is a PMIC register that gets written to decide the reboot type. There is also sometimes a cookie that can be written to indicate that the bootloader should behave differently than a regular boot. These knobs evolve over product generations and require more drivers. Qualcomm firmwares are beginning to expose vendor SYSTEM_RESET2 types to simplify driver requirements from Linux. Add support in PSCI to statically wire reboot mode commands from userspace to a vendor reset and cookie value using the device tree. The DT bindings are similar to reboot mode framework except that 2 integers are accepted (the type and cookie). Also, reboot mode framework is intended to program the cookies, but not actually reboot the host. PSCI SYSTEM_RESET2 does both. I've not added support for reading ACPI tables since I don't have any device which provides them + firmware that supports vendor SYSTEM_RESET2 types. --- Lorenzo and I are also looking for some feedback on whether it is safe to perform a vendor SYSTEM_RESET2 irrespective of the enum reboot_mode: https://lore.kernel.org/all/Zw5ffeYW5uRpsaG3@lpieralisi/ --- Previous discussions around SYSTEM_RESET2: - https://lore.kernel.org/lkml/20230724223057.1208122-2-quic_eberman@quicinc.com/T/ - https://lore.kernel.org/all/4a679542-b48d-7e11-f33a-63535a5c68cb@quicinc.com/ To: Bjorn Andersson <andersson@kernel.org> To: Konrad Dybcio <konrad.dybcio@linaro.org> To: Sebastian Reichel <sre@kernel.org> To: Rob Herring <robh@kernel.org> To: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org> To: Conor Dooley <conor+dt@kernel.org> To: Vinod Koul <vkoul@kernel.org> To: Andy Yan <andy.yan@rock-chips.com> To: Lorenzo Pieralisi <lpieralisi@kernel.org> To: Mark Rutland <mark.rutland@arm.com> To: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> To: Arnd Bergmann <arnd@arndb.de> To: Olof Johansson <olof@lixom.net>A To: Catalin Marinas <catalin.marinas@arm.com> To: Will Deacon <will@kernel.org> To: cros-qcom-dts-watchers@chromium.org Cc: Satya Durga Srinivasu Prabhala <quic_satyap@quicinc.com> Cc: Melody Olvera <quic_molvera@quicinc.com> Cc: Shivendra Pratap <quic_spratap@quicinc.com> Cc: devicetree@vger.kernel.org Cc: linux-kernel@vger.kernel.org Cc: linux-arm-kernel@lists.infradead.org Cc: Florian Fainelli <florian.fainelli@broadcom.com> Cc: linux-pm@vger.kernel.org Cc: linux-arm-msm@vger.kernel.org Signed-off-by: Elliot Berman <quic_eberman@quicinc.com> Changes in v6: - Rebase to v6.11 and fix trivial conflicts in qcm6490-idp - Add sa8775p-ride support (same as qcm6490-idp) - Link to v5: https://lore.kernel.org/r/20240617-arm-psci-system_reset2-vendor-reboots-v5-0-086950f650c8@quicinc.com Changes in v5: - Drop the nested "items" in prep for future dtschema tools - Link to v4: https://lore.kernel.org/r/20240611-arm-psci-system_reset2-vendor-reboots-v4-0-98f55aa74ae8@quicinc.com Changes in v4: - Change mode- properties from uint32-matrix to uint32-array - Restructure the reset-types node so only the restriction is in the if/then schemas and not the entire definition - Link to v3: https://lore.kernel.org/r/20240515-arm-psci-system_reset2-vendor-reboots-v3-0-16dd4f9c0ab4@quicinc.com Changes in v3: - Limit outer number of items to 1 for mode-* properties - Move the reboot-mode for psci under a subnode "reset-types" - Fix the DT node in qcm6490-idp so it doesn't overwrite the one from sc7820.dtsi - Link to v2: https://lore.kernel.org/r/20240414-arm-psci-system_reset2-vendor-reboots-v2-0-da9a055a648f@quicinc.com Changes in v2: - Fixes to schema as suggested by Rob and Krzysztof - Add qcm6490 idp as first Qualcomm device to support - Link to v1: https://lore.kernel.org/r/20231117-arm-psci-system_reset2-vendor-reboots-v1-0-03c4612153e2@quicinc.com Changes in v1: - Reference reboot-mode bindings as suggeted by Rob. - Link to RFC: https://lore.kernel.org/r/20231030-arm-psci-system_reset2-vendor-reboots-v1-0-dcdd63352ad1@quicinc.com To: Sebastian Reichel <sre@kernel.org> To: Rob Herring <robh@kernel.org> To: Krzysztof Kozlowski <krzk+dt@kernel.org> To: Conor Dooley <conor+dt@kernel.org> To: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> To: Vinod Koul <vkoul@kernel.org> To: Andy Yan <andy.yan@rock-chips.com> To: Lorenzo Pieralisi <lpieralisi@kernel.org> To: Mark Rutland <mark.rutland@arm.com> To: Bjorn Andersson <andersson@kernel.org> To: Konrad Dybcio <konradybcio@kernel.org> To: cros-qcom-dts-watchers@chromium.org Cc: linux-pm@vger.kernel.org Cc: devicetree@vger.kernel.org Cc: linux-kernel@vger.kernel.org Cc: linux-arm-kernel@lists.infradead.org Cc: linux-arm-msm@vger.kernel.org --- Elliot Berman (5): dt-bindings: power: reset: Convert mode-.* properties to array dt-bindings: arm: Document reboot mode magic firmware: psci: Read and use vendor reset types arm64: dts: qcom: Add PSCI SYSTEM_RESET2 types for qcm6490-idp arm64: dts: qcom: Add PSCI SYSTEM_RESET2 types for sa8775p-ride Documentation/devicetree/bindings/arm/psci.yaml | 43 ++++++++++ .../bindings/power/reset/nvmem-reboot-mode.yaml | 4 + .../devicetree/bindings/power/reset/qcom,pon.yaml | 7 ++ .../bindings/power/reset/reboot-mode.yaml | 4 +- .../bindings/power/reset/syscon-reboot-mode.yaml | 4 + arch/arm64/boot/dts/qcom/qcm6490-idp.dts | 7 ++ arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi | 7 ++ arch/arm64/boot/dts/qcom/sa8775p.dtsi | 2 +- arch/arm64/boot/dts/qcom/sc7280.dtsi | 2 +- drivers/firmware/psci/psci.c | 92 ++++++++++++++++++++++ include/linux/arm-smccc.h | 5 ++ 11 files changed, 173 insertions(+), 4 deletions(-) --- base-commit: 98f7e32f20d28ec452afb208f9cffc08448a2652 change-id: 20231016-arm-psci-system_reset2-vendor-reboots-cc3ad456c070 Best regards,