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

[Mukesh Ojha]

On 9/7/2023 12:02 PM, Sricharan Ramabadhran wrote:
> <snip ..>
>
> > > > > +     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.
> >
> > Ok,.
> >
> > Well, Kernel version does not guarantees us whether certain things
> > are supported or not in the firmware and it is not bound to any
> > particular firmware version;
> >
> > So, whatever firmware version it is running with, we should try to
> > support.
> >
> > Should we implement certain kind of call, if fastcall(atomic) is 
> > supported go-ahead otherwise fallback to slowcalls (interruptible)
> > calls, but this is completely out of the context of this patch.
>
>   I replied on older thread, was not in CC here, just saw this.
>
>   Agree, atomic api is out of this context and we could take it up
>   separately.
>
> > > > > +
> > > > > +     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.
> >
> > Agree.
>
>   For IPQ chipsets, SDI bit is used like below,
>
>     For abnormal resets (like WDT), should be set '1' for valid dump
>     collection.
>
>     For reboot, should be cleared to '0' to avoid dump collection which
>     is not required in this case.
>
>     For HLOS panic, is a don't care, dumps always get collected and
>     firmware takes care of clearing the SDI bit.
>
>     Mukesh,  Can you confirm if its same for msm also ?

Yes, it is same in MSM as well.

-Mukesh

> > Just a wild guess..
> >
> > Can we check if this call __qcom_scm_is_call_available() helps
> > to determine, if the certain soc has this SCM calls supported
> > and if it is there it can be disabled.
> >
> > __qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_BOOT, 
> > QCOM_SCM_BOOT_SDI_CONFIG)
>
>   Yes, as i mentioned in other thread, checking using
>   qcom_scm_is_call_available is better. That said, would require
>   testing on all IPQ/MSM socs to confirm if firmware supports it.
>
> > > 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.
> >
> > Download mode would not be reflected unless it is debug
> > board, whatever you write will not be allowed if it is a
> > secure device.
>
>    Yes, 'download mode' bit is similar, but that is used by the firmware
>    to determining whether to collect dumps on non-secure boards.
>    Specifically, 'SDI bit' on some socs is used by firmware to determine
>    if boot is happening from a 'abnormal crash', hence put DDR to
>    self-refresh etc for valid dumps.
>
> Regards,
>   Sricharan