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

[Sricharan Ramabadhran <quic_srichara@quicinc.com>]

<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 ?
> 
> 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