Messages in this thread | | | Date | Tue, 22 Aug 2023 21:07:59 +0530 | Subject | Re: [PATCH v3 2/4] firmware: qcom_scm: disable SDI if required | From | Mukesh Ojha <> |
| |
On 8/16/2023 10:15 PM, Robert Marko wrote: > IPQ5018 has SDI (Secure Debug Image) enabled by TZ by default, and that > means that WDT being asserted or just trying to reboot will hang the board > in the debug mode and only pulling the power and repowering will help. > Some IPQ4019 boards like Google WiFI have it enabled as well. > > Luckily, SDI can be disabled via an SCM call. > > So, lets use the boolean DT property to identify boards that have SDI > enabled by default and use the SCM call to disable SDI during SCM probe. > It is important to disable it as soon as possible as we might have a WDT > assertion at any time which would then leave the board in debug mode, > thus disabling it during SCM removal is not enough. > > Signed-off-by: Robert Marko <robimarko@gmail.com> > --- > Changes in v3: > * Squashed ("firmware: qcom: scm: Add SDI disable support") and > ("firmware: qcom_scm: disable SDI if required") > --- > drivers/firmware/qcom_scm.c | 29 +++++++++++++++++++++++++++++ > drivers/firmware/qcom_scm.h | 1 + > 2 files changed, 30 insertions(+) > > diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c > index 06fe8aca870d..de9d1a11d097 100644 > --- a/drivers/firmware/qcom_scm.c > +++ b/drivers/firmware/qcom_scm.c > @@ -403,6 +403,29 @@ int qcom_scm_set_remote_state(u32 state, u32 id) > } > EXPORT_SYMBOL_GPL(qcom_scm_set_remote_state); > > +static int qcom_scm_disable_sdi(void) > +{ > + int ret; > + struct qcom_scm_desc desc = { > + .svc = QCOM_SCM_SVC_BOOT, > + .cmd = QCOM_SCM_BOOT_SDI_CONFIG, > + .args[0] = 1, /* Disable watchdog debug */ > + .args[1] = 0, /* Disable SDI */ > + .arginfo = QCOM_SCM_ARGS(2), > + .owner = ARM_SMCCC_OWNER_SIP, > + }; > + struct qcom_scm_res res; > + > + ret = qcom_scm_clk_enable(); > + if (ret) > + return ret; > + ret = qcom_scm_call(__scm->dev, &desc, &res);
Would you not be wanting this call to be atomic ?
> + > + qcom_scm_clk_disable(); > + > + return ret ? : res.result[0]; > +} > + > static int __qcom_scm_set_dload_mode(struct device *dev, bool enable) > { > struct qcom_scm_desc desc = { > @@ -1468,6 +1491,12 @@ static int qcom_scm_probe(struct platform_device *pdev) > if (download_mode) > qcom_scm_set_download_mode(true); > > + /* > + * Disable SDI if indicated by DT that it is enabled by default. > + */ > + if (of_property_read_bool(pdev->dev.of_node, "qcom,sdi-enabled")) > + qcom_scm_disable_sdi();
Why don't we do this call in qcom_scm_shutdown() also does it not conflict with above download_mode we have enabled download mode but disabling SDI means (hard reset) and will not be collecting crash dump?
-Mukesh
> + > return 0; > } > > diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h > index e6e512bd57d1..7b68fa820495 100644 > --- a/drivers/firmware/qcom_scm.h > +++ b/drivers/firmware/qcom_scm.h > @@ -80,6 +80,7 @@ extern int scm_legacy_call(struct device *dev, const struct qcom_scm_desc *desc, > #define QCOM_SCM_SVC_BOOT 0x01 > #define QCOM_SCM_BOOT_SET_ADDR 0x01 > #define QCOM_SCM_BOOT_TERMINATE_PC 0x02 > +#define QCOM_SCM_BOOT_SDI_CONFIG 0x09 > #define QCOM_SCM_BOOT_SET_DLOAD_MODE 0x10 > #define QCOM_SCM_BOOT_SET_ADDR_MC 0x11 > #define QCOM_SCM_BOOT_SET_REMOTE_STATE 0x0a
| |