Hi,
On 05/09/2018 05:14 PM, Vikash Garodia wrote:
> Hi Stanimir,
>
> On 2018-05-09 16:45, Stanimir Varbanov wrote:
>> Hi Vikash,
>>
>> On 05/02/2018 09:07 AM, vgaro...@codeaurora.org wrote:
>>> Hello Stanimir,
>>>
>>> On 2018-04-24 18:14, Stanimir Varbanov wrote:
>>>> This adds suspend (power collapse) function with slightly
>>>> different order of calls comparing with Venus 3xx.
>>>>
>>>> Signed-off-by: Stanimir Varbanov <stanimir.varba...@linaro.org>
>>>> ---
>>>> drivers/media/platform/qcom/venus/hfi_venus.c | 52
>>>> +++++++++++++++++++++++++++
>>>> 1 file changed, 52 insertions(+)
>>>>
>>>> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c
>>>> b/drivers/media/platform/qcom/venus/hfi_venus.c
>>>> index 53546174aab8..f61d34bf61b4 100644
>>>> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
>>>> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
>>>> @@ -1443,6 +1443,55 @@ static int venus_suspend_1xx(struct venus_core
>>>> *core)
>>>> return 0;
>>>> }
>>>>
>>>> +static int venus_suspend_4xx(struct venus_core *core)
>>>> +{
>>>> + struct venus_hfi_device *hdev = to_hfi_priv(core);
>>>> + struct device *dev = core->dev;
>>>> + u32 val;
>>>> + int ret;
>>>> +
>>>> + if (!hdev->power_enabled || hdev->suspended)
>>>> + return 0;
>>>> +
>>>> + mutex_lock(&hdev->lock);
>>>> + ret = venus_is_valid_state(hdev);
>>>> + mutex_unlock(&hdev->lock);
>>>> +
>>>> + if (!ret) {
>>>> + dev_err(dev, "bad state, cannot suspend\n");
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + ret = venus_prepare_power_collapse(hdev, false);
>>>> + if (ret) {
>>>> + dev_err(dev, "prepare for power collapse fail (%d)\n", ret);
>>>> + return ret;
>>>> + }
>>>> +
>>>> + ret = readl_poll_timeout(core->base + CPU_CS_SCIACMDARG0, val,
>>>> + val & CPU_CS_SCIACMDARG0_PC_READY,
>>>> + POLL_INTERVAL_US, 100000);
>>>> + if (ret) {
>>>> + dev_err(dev, "Polling power collapse ready timed out\n");
>>>> + return ret;
>>>> + }
>>>> +
>>>> + mutex_lock(&hdev->lock);
>>>> +
>>>> + ret = venus_power_off(hdev);
>>>> + if (ret) {
>>>> + dev_err(dev, "venus_power_off (%d)\n", ret);
>>>> + mutex_unlock(&hdev->lock);
>>>> + return ret;
>>>> + }
>>>> +
>>>> + hdev->suspended = true;
>>>> +
>>>> + mutex_unlock(&hdev->lock);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> static int venus_suspend_3xx(struct venus_core *core)
>>>> {
>>>> struct venus_hfi_device *hdev = to_hfi_priv(core);
>>>> @@ -1507,6 +1556,9 @@ static int venus_suspend(struct venus_core *core)
>>>> if (core->res->hfi_version == HFI_VERSION_3XX)
>>>> return venus_suspend_3xx(core);
>>>>
>>>> + if (core->res->hfi_version == HFI_VERSION_4XX)
>>>> + return venus_suspend_4xx(core);
>>>> +
>>>> return venus_suspend_1xx(core);
>>>> }
>>>
>>> Let me brief on the power collapse sequence for Venus_4xx
>>> 1. Host checks for ARM9 and Video core to be idle.
>>> This can be done by checking for WFI bit (bit 0) in CPU status
>>> register for ARM9 and by checking bit 30 in Control status reg for video
>>> core/s.
>>> 2. Host then sends command to ARM9 to prepare for power collapse.
>>> 3. Host then checks for WFI bit and PC_READY bit before withdrawing
>>> going for power off.
>>>
>>> As per this patch, host is preparing for power collapse without checking
>>> for #1.
>>> Update the code to handle #3.
>>
>> This looks similar to suspend for Venus 3xx. Can you confirm that the
>> sequence of checks for 4xx is the same as 3xx?
>
> Do you mean the driver implementation for Suspend Venus 3xx or the hardware
> expectation for Venus 3xx ? If hardware expectation wise, the sequence is
> same for 3xx and 4xx.
> In the suspend implementation for 3xx, i see that the host just reads
> the WFI
> and idle status bits, but does not validate those bit value before
> preparing
> Venus for power collapse. Sequence #2 and #3 is followed for Venus 3xx
> implementation.
OK, than we can reuse venus_suspend_3xx function for 4xx, just need to
fix WFI and idle bit for #1.
--
regards,
Stan
