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

[Robert Marko]

On Tue, 22 Aug 2023 at 17:38, Mukesh Ojha <quic_mojha@xxxxxxxxxxx> wrote:
>
>
>
> 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@xxxxxxxxx>
> > ---
> > 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 ?

This is implemented based off the downstream 5.4 kernel as I dont have
the SCM docs
so I dont know if its even supported in the atomic version.
>
> > +
> > +     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?

Because doing it in SCM removal is too late, what if we have a WDT
assertion and not a
regular reboot?
It would mean that the board will get stuck in the debug mode which is
not useful for users and
requires the power to be pulled in order to boot normally again.

I am not sure about the download mode, this is where insight from QCA
really help as I am
doing this with very limited docs.

Regards,
Robert
>
> -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