| Message ID | 1527884768-22392-3-git-send-email-vgarodia@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Jun 02, 2018 at 01:56:05AM +0530, Vikash Garodia wrote: > Add a new routine which abstracts the TZ call to > set the video hardware state. > > Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org> > --- > drivers/media/platform/qcom/venus/core.h | 5 +++++ > drivers/media/platform/qcom/venus/firmware.c | 28 +++++++++++++++++++++++++++ > drivers/media/platform/qcom/venus/firmware.h | 1 + > drivers/media/platform/qcom/venus/hfi_venus.c | 13 ++++--------- > 4 files changed, 38 insertions(+), 9 deletions(-) > > diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h > index 85e66e2..e7bfb63 100644 > --- a/drivers/media/platform/qcom/venus/core.h > +++ b/drivers/media/platform/qcom/venus/core.h > @@ -35,6 +35,11 @@ struct reg_val { > u32 value; > }; > > +enum tzbsp_video_state { > + TZBSP_VIDEO_SUSPEND = 0, > + TZBSP_VIDEO_RESUME You should put a comma after the last item to reduce future patch sizes. > +}; > + > struct venus_resources { > u64 dma_mask; > const struct freq_tbl *freq_tbl; > diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c > index 7d89b5a..b4664ed 100644 > --- a/drivers/media/platform/qcom/venus/firmware.c > +++ b/drivers/media/platform/qcom/venus/firmware.c > @@ -53,6 +53,34 @@ static void venus_reset_hw(struct venus_core *core) > /* Bring Arm9 out of reset */ > writel_relaxed(0, reg_base + WRAPPER_A9SS_SW_RESET); > } > + > +int venus_set_hw_state(enum tzbsp_video_state state, struct venus_core *core) > +{ > + int ret; > + struct device *dev = core->dev; If you get rid of the log message as you should, you don't need this. > + void __iomem *reg_base = core->base; > + > + switch (state) { > + case TZBSP_VIDEO_SUSPEND: > + if (qcom_scm_is_available()) > + ret = qcom_scm_set_remote_state(TZBSP_VIDEO_SUSPEND, 0); > + else > + writel_relaxed(1, reg_base + WRAPPER_A9SS_SW_RESET); You can just use core->base here and not bother making a local variable for it. > + break; > + case TZBSP_VIDEO_RESUME: > + if (qcom_scm_is_available()) > + ret = qcom_scm_set_remote_state(TZBSP_VIDEO_RESUME, 0); > + else > + venus_reset_hw(core); > + break; > + default: > + dev_err(dev, "invalid state\n"); state is a enum - you are highly unlikely to be calling it in your own code with a random value. It is smart to have the default, but you don't need the log message - that is just wasted space in the binary. > + break; > + } There are three paths in the switch statement that could end up with 'ret' being uninitialized here. Set it to 0 when you declare it. > + return ret; > +} > +EXPORT_SYMBOL_GPL(venus_set_hw_state); > + > int venus_boot(struct device *dev, const char *fwname) > { > const struct firmware *mdt; > diff --git a/drivers/media/platform/qcom/venus/firmware.h b/drivers/media/platform/qcom/venus/firmware.h > index 428efb5..1336729 100644 > --- a/drivers/media/platform/qcom/venus/firmware.h > +++ b/drivers/media/platform/qcom/venus/firmware.h > @@ -18,5 +18,6 @@ > > int venus_boot(struct device *dev, const char *fwname); > int venus_shutdown(struct device *dev); > +int venus_set_hw_state(enum tzbsp_video_state, struct venus_core *core); > > #endif > diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c > index f61d34b..797a9f5 100644 > --- a/drivers/media/platform/qcom/venus/hfi_venus.c > +++ b/drivers/media/platform/qcom/venus/hfi_venus.c > @@ -19,7 +19,6 @@ > #include <linux/interrupt.h> > #include <linux/iopoll.h> > #include <linux/kernel.h> > -#include <linux/qcom_scm.h> > #include <linux/slab.h> > > #include "core.h" > @@ -27,6 +26,7 @@ > #include "hfi_msgs.h" > #include "hfi_venus.h" > #include "hfi_venus_io.h" > +#include "firmware.h" > > #define HFI_MASK_QHDR_TX_TYPE 0xff000000 > #define HFI_MASK_QHDR_RX_TYPE 0x00ff0000 > @@ -55,11 +55,6 @@ > #define IFACEQ_VAR_LARGE_PKT_SIZE 512 > #define IFACEQ_VAR_HUGE_PKT_SIZE (1024 * 12) > > -enum tzbsp_video_state { > - TZBSP_VIDEO_STATE_SUSPEND = 0, > - TZBSP_VIDEO_STATE_RESUME > -}; > - > struct hfi_queue_table_header { > u32 version; > u32 size; > @@ -574,7 +569,7 @@ static int venus_power_off(struct venus_hfi_device *hdev) > if (!hdev->power_enabled) > return 0; > > - ret = qcom_scm_set_remote_state(TZBSP_VIDEO_STATE_SUSPEND, 0); > + ret = venus_set_hw_state(TZBSP_VIDEO_SUSPEND, hdev->core); > if (ret) > return ret; > > @@ -594,7 +589,7 @@ static int venus_power_on(struct venus_hfi_device *hdev) > if (hdev->power_enabled) > return 0; > > - ret = qcom_scm_set_remote_state(TZBSP_VIDEO_STATE_RESUME, 0); > + ret = venus_set_hw_state(TZBSP_VIDEO_RESUME, hdev->core); > if (ret) > goto err; > > @@ -607,7 +602,7 @@ static int venus_power_on(struct venus_hfi_device *hdev) > return 0; > > err_suspend: > - qcom_scm_set_remote_state(TZBSP_VIDEO_STATE_SUSPEND, 0); > + venus_set_hw_state(TZBSP_VIDEO_SUSPEND, hdev->core); > err: > hdev->power_enabled = false; > return ret;
Hi Jordan, Vikash, On Sat, Jun 2, 2018 at 6:21 AM Jordan Crouse <jcrouse@codeaurora.org> wrote: > > On Sat, Jun 02, 2018 at 01:56:05AM +0530, Vikash Garodia wrote: [snip] > > +int venus_set_hw_state(enum tzbsp_video_state state, struct venus_core *core) > > +{ > > + int ret; > > + struct device *dev = core->dev; > > If you get rid of the log message as you should, you don't need this. > > > + void __iomem *reg_base = core->base; > > + > > + switch (state) { > > + case TZBSP_VIDEO_SUSPEND: > > + if (qcom_scm_is_available()) > > + ret = qcom_scm_set_remote_state(TZBSP_VIDEO_SUSPEND, 0); > > + else > > + writel_relaxed(1, reg_base + WRAPPER_A9SS_SW_RESET); > > You can just use core->base here and not bother making a local variable for it. > > > + break; > > + case TZBSP_VIDEO_RESUME: > > + if (qcom_scm_is_available()) > > + ret = qcom_scm_set_remote_state(TZBSP_VIDEO_RESUME, 0); > > + else > > + venus_reset_hw(core); > > + break; > > + default: > > + dev_err(dev, "invalid state\n"); > > state is a enum - you are highly unlikely to be calling it in your own code with > a random value. It is smart to have the default, but you don't need the log > message - that is just wasted space in the binary. > > > + break; > > + } > > There are three paths in the switch statement that could end up with 'ret' being > uninitialized here. Set it to 0 when you declare it. Does this actually compile? The compiler should detect that ret is used uninitialized. Setting it to 0 at declaration time actually prevents compiler from doing that and makes it impossible to catch cases when the ret should actually be non-zero, e.g. the invalid enum value case. Given that this function is supposed to substitute existing calls into qcom_scm_set_remote_state(), why not just do something like this: if (qcom_scm_is_available()) return qcom_scm_set_remote_state(state, 0); switch (state) { case TZBSP_VIDEO_SUSPEND: writel_relaxed(1, reg_base + WRAPPER_A9SS_SW_RESET); break; case TZBSP_VIDEO_RESUME: venus_reset_hw(core); break; } return 0; Best regards, Tomasz
Hi Vikash, Thanks for the patch! On 1.06.2018 23:26, Vikash Garodia wrote: > Add a new routine which abstracts the TZ call to Actually the new routine abstracts Venus CPU state, Isn't it? > set the video hardware state. > > Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org> > --- > drivers/media/platform/qcom/venus/core.h | 5 +++++ > drivers/media/platform/qcom/venus/firmware.c | 28 +++++++++++++++++++++++++++ > drivers/media/platform/qcom/venus/firmware.h | 1 + > drivers/media/platform/qcom/venus/hfi_venus.c | 13 ++++--------- > 4 files changed, 38 insertions(+), 9 deletions(-) > > diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h > index 85e66e2..e7bfb63 100644 > --- a/drivers/media/platform/qcom/venus/core.h > +++ b/drivers/media/platform/qcom/venus/core.h > @@ -35,6 +35,11 @@ struct reg_val { > u32 value; > }; > > +enum tzbsp_video_state { > + TZBSP_VIDEO_SUSPEND = 0, > + TZBSP_VIDEO_RESUME > +}; please move this in firmware.c, for more see below. > + > struct venus_resources { > u64 dma_mask; > const struct freq_tbl *freq_tbl; > diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c > index 7d89b5a..b4664ed 100644 > --- a/drivers/media/platform/qcom/venus/firmware.c > +++ b/drivers/media/platform/qcom/venus/firmware.c > @@ -53,6 +53,34 @@ static void venus_reset_hw(struct venus_core *core) > /* Bring Arm9 out of reset */ > writel_relaxed(0, reg_base + WRAPPER_A9SS_SW_RESET); > } > + > +int venus_set_hw_state(enum tzbsp_video_state state, struct venus_core *core) can we put this function this way: venus_set_state(struct venus_core *core, bool on) so we set the state to On when we are power-up the venus CPU and Off when we power-down. by this way we really abstract the state, IMO. > +{ > + int ret; > + struct device *dev = core->dev; > + void __iomem *reg_base = core->base; just 'base' should be enough. > + > + switch (state) { > + case TZBSP_VIDEO_SUSPEND: > + if (qcom_scm_is_available()) You really shouldn't rely on this function (see the comment from Bjorn on first version of this patch series). I think we have to replace qcom_scm_is_available() with some flag which is reflecting does the firmware subnode is exist or not. In case it is not exist the we have to go with TZ scm calls. > + ret = qcom_scm_set_remote_state(TZBSP_VIDEO_SUSPEND, 0); > + else > + writel_relaxed(1, reg_base + WRAPPER_A9SS_SW_RESET); > + break; > + case TZBSP_VIDEO_RESUME: > + if (qcom_scm_is_available()) > + ret = qcom_scm_set_remote_state(TZBSP_VIDEO_RESUME, 0); > + else > + venus_reset_hw(core); > + break; > + default: > + dev_err(dev, "invalid state\n"); > + break; > + } > + return ret; > +} > +EXPORT_SYMBOL_GPL(venus_set_hw_state); > + regards, Stan
On 02-06-18, 01:56, Vikash Garodia wrote: > +int venus_set_hw_state(enum tzbsp_video_state state, struct venus_core *core) > +{ > + int ret; this should be init to 0 ... > + struct device *dev = core->dev; > + void __iomem *reg_base = core->base; > + > + switch (state) { > + case TZBSP_VIDEO_SUSPEND: > + if (qcom_scm_is_available()) > + ret = qcom_scm_set_remote_state(TZBSP_VIDEO_SUSPEND, 0); > + else > + writel_relaxed(1, reg_base + WRAPPER_A9SS_SW_RESET); > + break; > + case TZBSP_VIDEO_RESUME: > + if (qcom_scm_is_available()) > + ret = qcom_scm_set_remote_state(TZBSP_VIDEO_RESUME, 0); > + else > + venus_reset_hw(core); > + break; > + default: if it is default, ret contains garbage > + dev_err(dev, "invalid state\n"); > + break; > + } > + return ret; and that is returned. Compiler should complain about these ... > -enum tzbsp_video_state { > - TZBSP_VIDEO_STATE_SUSPEND = 0, > - TZBSP_VIDEO_STATE_RESUME > -}; ah you are moving existing defines, please mention this in changelog. Till this line I was expecting additions...
Hi Jordan, Tomasz, Thanks for your valuable review comments. On 2018-06-04 18:24, Tomasz Figa wrote: > Hi Jordan, Vikash, > > On Sat, Jun 2, 2018 at 6:21 AM Jordan Crouse <jcrouse@codeaurora.org> > wrote: >> >> On Sat, Jun 02, 2018 at 01:56:05AM +0530, Vikash Garodia wrote: > [snip] >> > +int venus_set_hw_state(enum tzbsp_video_state state, struct venus_core *core) >> > +{ >> > + int ret; >> > + struct device *dev = core->dev; >> >> If you get rid of the log message as you should, you don't need this. Would prefer to keep the log for cases when enum is expanded while the switch does not handle it. >> > + void __iomem *reg_base = core->base; >> > + >> > + switch (state) { >> > + case TZBSP_VIDEO_SUSPEND: >> > + if (qcom_scm_is_available()) >> > + ret = qcom_scm_set_remote_state(TZBSP_VIDEO_SUSPEND, 0); >> > + else >> > + writel_relaxed(1, reg_base + WRAPPER_A9SS_SW_RESET); >> >> You can just use core->base here and not bother making a local >> variable for it. Ok. >> > + break; >> > + case TZBSP_VIDEO_RESUME: >> > + if (qcom_scm_is_available()) >> > + ret = qcom_scm_set_remote_state(TZBSP_VIDEO_RESUME, 0); >> > + else >> > + venus_reset_hw(core); >> > + break; >> > + default: >> > + dev_err(dev, "invalid state\n"); >> >> state is a enum - you are highly unlikely to be calling it in your own >> code with >> a random value. It is smart to have the default, but you don't need >> the log >> message - that is just wasted space in the binary. Incase enum is expanded while the switch does not handle it, default will be useful. >> > + break; >> > + } >> >> There are three paths in the switch statement that could end up with >> 'ret' being >> uninitialized here. Set it to 0 when you declare it. > Does this actually compile? The compiler should detect that ret is > used uninitialized. Setting it to 0 at declaration time actually > prevents compiler from doing that and makes it impossible to catch > cases when the ret should actually be non-zero, e.g. the invalid enum > value case. It does compile while it gave me failure while doing the functional validation. I have fixed it in next series of patch. > Given that this function is supposed to substitute existing calls into > qcom_scm_set_remote_state(), why not just do something like this: > > if (qcom_scm_is_available()) > return qcom_scm_set_remote_state(state, 0); > > switch (state) { > case TZBSP_VIDEO_SUSPEND: > writel_relaxed(1, reg_base + WRAPPER_A9SS_SW_RESET); > break; > case TZBSP_VIDEO_RESUME: > venus_reset_hw(core); > break; > } > > return 0; This will not work as driver will write on the register irrespective of scm availability. > Best regards, > Tomasz
Hi Stanimir, Thanks for your valuable comments. On 2018-06-04 19:20, Stanimir Varbanov wrote: > Hi Vikash, > > Thanks for the patch! > > On 1.06.2018 23:26, Vikash Garodia wrote: >> Add a new routine which abstracts the TZ call to > > Actually the new routine abstracts Venus CPU state, Isn't it? Yes, its a Venus CPU state controlled by TZ. I can mention it as an abstraction of venus CPU state. >> set the video hardware state. >> >> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org> >> --- >> drivers/media/platform/qcom/venus/core.h | 5 +++++ >> drivers/media/platform/qcom/venus/firmware.c | 28 >> +++++++++++++++++++++++++++ >> drivers/media/platform/qcom/venus/firmware.h | 1 + >> drivers/media/platform/qcom/venus/hfi_venus.c | 13 ++++--------- >> 4 files changed, 38 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/media/platform/qcom/venus/core.h >> b/drivers/media/platform/qcom/venus/core.h >> index 85e66e2..e7bfb63 100644 >> --- a/drivers/media/platform/qcom/venus/core.h >> +++ b/drivers/media/platform/qcom/venus/core.h >> @@ -35,6 +35,11 @@ struct reg_val { >> u32 value; >> }; >> >> +enum tzbsp_video_state { >> + TZBSP_VIDEO_SUSPEND = 0, >> + TZBSP_VIDEO_RESUME >> +}; > > please move this in firmware.c, for more see below. > >> + >> struct venus_resources { >> u64 dma_mask; >> const struct freq_tbl *freq_tbl; >> diff --git a/drivers/media/platform/qcom/venus/firmware.c >> b/drivers/media/platform/qcom/venus/firmware.c >> index 7d89b5a..b4664ed 100644 >> --- a/drivers/media/platform/qcom/venus/firmware.c >> +++ b/drivers/media/platform/qcom/venus/firmware.c >> @@ -53,6 +53,34 @@ static void venus_reset_hw(struct venus_core *core) >> /* Bring Arm9 out of reset */ >> writel_relaxed(0, reg_base + WRAPPER_A9SS_SW_RESET); >> } >> + >> +int venus_set_hw_state(enum tzbsp_video_state state, struct >> venus_core *core) > > can we put this function this way: > > venus_set_state(struct venus_core *core, bool on) > > so we set the state to On when we are power-up the venus CPU and Off > when we power-down. > > by this way we really abstract the state, IMO. Good point. Will do in similar way. >> +{ >> + int ret; >> + struct device *dev = core->dev; >> + void __iomem *reg_base = core->base; > > just 'base' should be enough. Infact, comment from Jordan, we can remove it altogether. >> + >> + switch (state) { >> + case TZBSP_VIDEO_SUSPEND: >> + if (qcom_scm_is_available()) > > You really shouldn't rely on this function (see the comment from Bjorn > on first version of this patch series). > > I think we have to replace qcom_scm_is_available() with some flag which > is reflecting does the firmware subnode is exist or not. In case it is > not exist the we have to go with TZ scm calls. As we discussed, will keep it under the check for a valid firmware device. We can avoid the extra flag in that way. >> + ret = qcom_scm_set_remote_state(TZBSP_VIDEO_SUSPEND, 0); >> + else >> + writel_relaxed(1, reg_base + WRAPPER_A9SS_SW_RESET); >> + break; >> + case TZBSP_VIDEO_RESUME: >> + if (qcom_scm_is_available()) >> + ret = qcom_scm_set_remote_state(TZBSP_VIDEO_RESUME, 0); >> + else >> + venus_reset_hw(core); >> + break; >> + default: >> + dev_err(dev, "invalid state\n"); >> + break; >> + } >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(venus_set_hw_state); >> + > > regards, > Stan
On Wed, Jul 4, 2018 at 4:59 PM Vikash Garodia <vgarodia@codeaurora.org> wrote: > On 2018-06-04 18:24, Tomasz Figa wrote: > > On Sat, Jun 2, 2018 at 6:21 AM Jordan Crouse <jcrouse@codeaurora.org> > > wrote: > >> On Sat, Jun 02, 2018 at 01:56:05AM +0530, Vikash Garodia wrote: > > Given that this function is supposed to substitute existing calls into > > qcom_scm_set_remote_state(), why not just do something like this: > > > > if (qcom_scm_is_available()) > > return qcom_scm_set_remote_state(state, 0); > > > > switch (state) { > > case TZBSP_VIDEO_SUSPEND: > > writel_relaxed(1, reg_base + WRAPPER_A9SS_SW_RESET); > > break; > > case TZBSP_VIDEO_RESUME: > > venus_reset_hw(core); > > break; > > } > > > > return 0; > This will not work as driver will write on the register irrespective of > scm > availability. I'm sorry, where would it do so? The second line returns from the function inf SCM is available, so the rest of the function wouldn't be executed. Best regards, Tomasz
On 2018-07-04 14:30, Tomasz Figa wrote: > On Wed, Jul 4, 2018 at 4:59 PM Vikash Garodia <vgarodia@codeaurora.org> > wrote: >> On 2018-06-04 18:24, Tomasz Figa wrote: >> > On Sat, Jun 2, 2018 at 6:21 AM Jordan Crouse <jcrouse@codeaurora.org> >> > wrote: >> >> On Sat, Jun 02, 2018 at 01:56:05AM +0530, Vikash Garodia wrote: >> > Given that this function is supposed to substitute existing calls into >> > qcom_scm_set_remote_state(), why not just do something like this: >> > >> > if (qcom_scm_is_available()) >> > return qcom_scm_set_remote_state(state, 0); >> > >> > switch (state) { >> > case TZBSP_VIDEO_SUSPEND: >> > writel_relaxed(1, reg_base + WRAPPER_A9SS_SW_RESET); >> > break; >> > case TZBSP_VIDEO_RESUME: >> > venus_reset_hw(core); >> > break; >> > } >> > >> > return 0; >> This will not work as driver will write on the register irrespective >> of >> scm >> availability. > > I'm sorry, where would it do so? The second line returns from the > function inf SCM is available, so the rest of the function wouldn't be > executed. Ah!! you are right. That would work as well. I am ok with either way, but would recommend to keep it the existing way as it makes it little more readable. > Best regards, > Tomasz
On Wed, Jul 4, 2018 at 6:41 PM Vikash Garodia <vgarodia@codeaurora.org> wrote: > > On 2018-07-04 14:30, Tomasz Figa wrote: > > On Wed, Jul 4, 2018 at 4:59 PM Vikash Garodia <vgarodia@codeaurora.org> > > wrote: > >> On 2018-06-04 18:24, Tomasz Figa wrote: > >> > On Sat, Jun 2, 2018 at 6:21 AM Jordan Crouse <jcrouse@codeaurora.org> > >> > wrote: > >> >> On Sat, Jun 02, 2018 at 01:56:05AM +0530, Vikash Garodia wrote: > >> > Given that this function is supposed to substitute existing calls into > >> > qcom_scm_set_remote_state(), why not just do something like this: > >> > > >> > if (qcom_scm_is_available()) > >> > return qcom_scm_set_remote_state(state, 0); > >> > > >> > switch (state) { > >> > case TZBSP_VIDEO_SUSPEND: > >> > writel_relaxed(1, reg_base + WRAPPER_A9SS_SW_RESET); > >> > break; > >> > case TZBSP_VIDEO_RESUME: > >> > venus_reset_hw(core); > >> > break; > >> > } > >> > > >> > return 0; > >> This will not work as driver will write on the register irrespective > >> of > >> scm > >> availability. > > > > I'm sorry, where would it do so? The second line returns from the > > function inf SCM is available, so the rest of the function wouldn't be > > executed. > > Ah!! you are right. That would work as well. > I am ok with either way, but would recommend to keep it the existing way > as it makes it little more readable. I personally think the early exit is more readable, as it clearly separates the SCM and non-SCM part. Best regards, Tomasz
diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h index 85e66e2..e7bfb63 100644 --- a/drivers/media/platform/qcom/venus/core.h +++ b/drivers/media/platform/qcom/venus/core.h @@ -35,6 +35,11 @@ struct reg_val { u32 value; }; +enum tzbsp_video_state { + TZBSP_VIDEO_SUSPEND = 0, + TZBSP_VIDEO_RESUME +}; + struct venus_resources { u64 dma_mask; const struct freq_tbl *freq_tbl; diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c index 7d89b5a..b4664ed 100644 --- a/drivers/media/platform/qcom/venus/firmware.c +++ b/drivers/media/platform/qcom/venus/firmware.c @@ -53,6 +53,34 @@ static void venus_reset_hw(struct venus_core *core) /* Bring Arm9 out of reset */ writel_relaxed(0, reg_base + WRAPPER_A9SS_SW_RESET); } + +int venus_set_hw_state(enum tzbsp_video_state state, struct venus_core *core) +{ + int ret; + struct device *dev = core->dev; + void __iomem *reg_base = core->base; + + switch (state) { + case TZBSP_VIDEO_SUSPEND: + if (qcom_scm_is_available()) + ret = qcom_scm_set_remote_state(TZBSP_VIDEO_SUSPEND, 0); + else + writel_relaxed(1, reg_base + WRAPPER_A9SS_SW_RESET); + break; + case TZBSP_VIDEO_RESUME: + if (qcom_scm_is_available()) + ret = qcom_scm_set_remote_state(TZBSP_VIDEO_RESUME, 0); + else + venus_reset_hw(core); + break; + default: + dev_err(dev, "invalid state\n"); + break; + } + return ret; +} +EXPORT_SYMBOL_GPL(venus_set_hw_state); + int venus_boot(struct device *dev, const char *fwname) { const struct firmware *mdt; diff --git a/drivers/media/platform/qcom/venus/firmware.h b/drivers/media/platform/qcom/venus/firmware.h index 428efb5..1336729 100644 --- a/drivers/media/platform/qcom/venus/firmware.h +++ b/drivers/media/platform/qcom/venus/firmware.h @@ -18,5 +18,6 @@ int venus_boot(struct device *dev, const char *fwname); int venus_shutdown(struct device *dev); +int venus_set_hw_state(enum tzbsp_video_state, struct venus_core *core); #endif diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c index f61d34b..797a9f5 100644 --- a/drivers/media/platform/qcom/venus/hfi_venus.c +++ b/drivers/media/platform/qcom/venus/hfi_venus.c @@ -19,7 +19,6 @@ #include <linux/interrupt.h> #include <linux/iopoll.h> #include <linux/kernel.h> -#include <linux/qcom_scm.h> #include <linux/slab.h> #include "core.h" @@ -27,6 +26,7 @@ #include "hfi_msgs.h" #include "hfi_venus.h" #include "hfi_venus_io.h" +#include "firmware.h" #define HFI_MASK_QHDR_TX_TYPE 0xff000000 #define HFI_MASK_QHDR_RX_TYPE 0x00ff0000 @@ -55,11 +55,6 @@ #define IFACEQ_VAR_LARGE_PKT_SIZE 512 #define IFACEQ_VAR_HUGE_PKT_SIZE (1024 * 12) -enum tzbsp_video_state { - TZBSP_VIDEO_STATE_SUSPEND = 0, - TZBSP_VIDEO_STATE_RESUME -}; - struct hfi_queue_table_header { u32 version; u32 size; @@ -574,7 +569,7 @@ static int venus_power_off(struct venus_hfi_device *hdev) if (!hdev->power_enabled) return 0; - ret = qcom_scm_set_remote_state(TZBSP_VIDEO_STATE_SUSPEND, 0); + ret = venus_set_hw_state(TZBSP_VIDEO_SUSPEND, hdev->core); if (ret) return ret; @@ -594,7 +589,7 @@ static int venus_power_on(struct venus_hfi_device *hdev) if (hdev->power_enabled) return 0; - ret = qcom_scm_set_remote_state(TZBSP_VIDEO_STATE_RESUME, 0); + ret = venus_set_hw_state(TZBSP_VIDEO_RESUME, hdev->core); if (ret) goto err; @@ -607,7 +602,7 @@ static int venus_power_on(struct venus_hfi_device *hdev) return 0; err_suspend: - qcom_scm_set_remote_state(TZBSP_VIDEO_STATE_SUSPEND, 0); + venus_set_hw_state(TZBSP_VIDEO_SUSPEND, hdev->core); err: hdev->power_enabled = false; return ret;
Add a new routine which abstracts the TZ call to set the video hardware state. Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org> --- drivers/media/platform/qcom/venus/core.h | 5 +++++ drivers/media/platform/qcom/venus/firmware.c | 28 +++++++++++++++++++++++++++ drivers/media/platform/qcom/venus/firmware.h | 1 + drivers/media/platform/qcom/venus/hfi_venus.c | 13 ++++--------- 4 files changed, 38 insertions(+), 9 deletions(-)