Re: [PATCH 08/28] venus: hfi_venus: add suspend function for 4xx version

[Vikash Garodia]

Hi Stanimir,

On 2018-05-09 16:45, Stanimir Varbanov wrote:
> Hi Vikash,
>
> On 05/02/2018 09:07 AM, vgarodia@xxxxxxxxxxxxxx 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.varbanov@xxxxxxxxxx>
> > > ---
> > > Â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.