| Message ID | 20180424124436.26955-9-stanimir.varbanov@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@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. Thanks Vikash
Hi Vikash, On 05/02/2018 09:07 AM, vgarodia@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.varbanov@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?
Hi Stanimir, On 2018-05-09 16:45, Stanimir Varbanov wrote: > Hi Vikash, > > On 05/02/2018 09:07 AM, vgarodia@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.varbanov@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.
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, vgarodia@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.varbanov@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.
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); }
This adds suspend (power collapse) function with slightly different order of calls comparing with Venus 3xx. Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> --- drivers/media/platform/qcom/venus/hfi_venus.c | 52 +++++++++++++++++++++++++++ 1 file changed, 52 insertions(+)