Re: [PATCH v3 2/4] firmware: qcom_scm: disable SDI if required

Mukesh Ojha
2023-08-22T15:38:30Z

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