linux-kernel - Re: [PATCH v2 08/29] venus: hfi_venus: fix suspend function for venus 3xx versions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 21 May 2018 15:18:54 +0300
From:   Stanimir Varbanov <stanimir.varbanov@...aro.org>
To:     Tomasz Figa <tfiga@...omium.org>,
        Stanimir Varbanov <stanimir.varbanov@...aro.org>
Cc:     Mauro Carvalho Chehab <mchehab@...nel.org>,
        Hans Verkuil <hverkuil@...all.nl>,
        Linux Media Mailing List <linux-media@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        vgarodia@...eaurora.org
Subject: Re: [PATCH v2 08/29] venus: hfi_venus: fix suspend function for venus
 3xx versions

Hi Tomasz,

On 05/18/2018 06:14 PM, Tomasz Figa wrote:
> On Tue, May 15, 2018 at 5:11 PM Stanimir Varbanov <
> stanimir.varbanov@...aro.org> wrote:
> 
>> This fixes the suspend function for Venus 3xx versions by
>> add a check for WFI (wait for interrupt) bit. This bit
>> is on when the ARM9 is idle and entered in low power mode.
> 
>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@...aro.org>
>> ---
>>   drivers/media/platform/qcom/venus/hfi_venus.c    | 59
> ++++++++++++++++--------
>>   drivers/media/platform/qcom/venus/hfi_venus_io.h |  1 +
>>   2 files changed, 41 insertions(+), 19 deletions(-)
> 
>> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c
> b/drivers/media/platform/qcom/venus/hfi_venus.c
>> index 53546174aab8..aac351f699a0 100644
>> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
>> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
>> @@ -1447,7 +1447,7 @@ static int venus_suspend_3xx(struct venus_core
> *core)
>>   {
>>          struct venus_hfi_device *hdev = to_hfi_priv(core);
>>          struct device *dev = core->dev;
>> -       u32 ctrl_status, wfi_status;
>> +       u32 ctrl_status, cpu_status;
>>          int ret;
>>          int cnt = 100;
> 
>> @@ -1463,29 +1463,50 @@ static int venus_suspend_3xx(struct venus_core
> *core)
>>                  return -EINVAL;
>>          }
> 
>> -       ctrl_status = venus_readl(hdev, CPU_CS_SCIACMDARG0);
>> -       if (!(ctrl_status & CPU_CS_SCIACMDARG0_PC_READY)) {
>> -               wfi_status = venus_readl(hdev, WRAPPER_CPU_STATUS);
>> +       /*
>> +        * Power collapse sequence for Venus 3xx and 4xx versions:
>> +        * 1. Check for ARM9 and video core to be idle by checking WFI bit
>> +        *    (bit 0) in CPU status register and by checking Idle (bit
> 30) in
>> +        *    Control status register for video core.
>> +        * 2. Send a command to prepare for power collapse.
>> +        * 3. Check for WFI and PC_READY bits.
>> +        */
>> +
>> +       while (--cnt) {
>> +               cpu_status = venus_readl(hdev, WRAPPER_CPU_STATUS);
>>                  ctrl_status = venus_readl(hdev, CPU_CS_SCIACMDARG0);
> 
>> -               ret = venus_prepare_power_collapse(hdev, false);
>> -               if (ret) {
>> -                       dev_err(dev, "prepare for power collapse fail
> (%d)\n",
>> -                               ret);
>> -                       return ret;
>> -               }
>> +               if (cpu_status & WRAPPER_CPU_STATUS_WFI &&
>> +                   ctrl_status & CPU_CS_SCIACMDARG0_INIT_IDLE_MSG_MASK)
>> +                       break;
> 
>> -               cnt = 100;
>> -               while (cnt--) {
>> -                       wfi_status = venus_readl(hdev,
> WRAPPER_CPU_STATUS);
>> -                       ctrl_status = venus_readl(hdev,
> CPU_CS_SCIACMDARG0);
>> -                       if (ctrl_status & CPU_CS_SCIACMDARG0_PC_READY &&
>> -                           wfi_status & BIT(0))
>> -                               break;
>> -                       usleep_range(1000, 1500);
>> -               }
>> +               usleep_range(1000, 1500);
>>          }
> 
> To avoid opencoding the polling, I'd suggest doing a readx_poll_timeout()
> trick:

I like the idea, will try to rework that and use readx_poll_timeout.

<snip>

-- 
regards,
Stan