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

Mukesh Ojha
2023-09-11T21:30:53Z

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