| Message ID | 1535034528-11590-2-git-send-email-vgarodia@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Andy Gross |
| Headers | show |
| Series | Venus updates - PIL | expand |
On Thu, Aug 23, 2018 at 11:29 PM Vikash Garodia <vgarodia@codeaurora.org> wrote: > > Add routine to reset the ARM9 and brings it out of reset. Also > abstract the Venus CPU state handling with a new function. This > is in preparation to add PIL functionality in venus driver. > > Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org> > --- > drivers/media/platform/qcom/venus/core.h | 2 ++ > drivers/media/platform/qcom/venus/firmware.c | 33 ++++++++++++++++++++++++ > drivers/media/platform/qcom/venus/firmware.h | 11 ++++++++ > drivers/media/platform/qcom/venus/hfi_venus.c | 13 +++------- > drivers/media/platform/qcom/venus/hfi_venus_io.h | 7 +++++ > 5 files changed, 57 insertions(+), 9 deletions(-) > > diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h > index 2f02365..dfd5c10 100644 > --- a/drivers/media/platform/qcom/venus/core.h > +++ b/drivers/media/platform/qcom/venus/core.h > @@ -98,6 +98,7 @@ struct venus_caps { > * @dev: convenience struct device pointer > * @dev_dec: convenience struct device pointer for decoder device > * @dev_enc: convenience struct device pointer for encoder device > + * @no_tz: a flag that suggests presence of trustzone > * @lock: a lock for this strucure > * @instances: a list_head of all instances > * @insts_count: num of instances > @@ -129,6 +130,7 @@ struct venus_core { > struct device *dev; > struct device *dev_dec; > struct device *dev_enc; > + bool no_tz; > struct mutex lock; > struct list_head instances; > atomic_t insts_count; > diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c > index c4a5778..a9d042e 100644 > --- a/drivers/media/platform/qcom/venus/firmware.c > +++ b/drivers/media/platform/qcom/venus/firmware.c > @@ -22,10 +22,43 @@ > #include <linux/sizes.h> > #include <linux/soc/qcom/mdt_loader.h> > > +#include "core.h" > #include "firmware.h" > +#include "hfi_venus_io.h" > > #define VENUS_PAS_ID 9 > #define VENUS_FW_MEM_SIZE (6 * SZ_1M) This is making a strong assumption about the size of the FW memory region, which in practice is not always true (I had to reduce it to 5MB). How about having this as a member of venus_core, which is initialized in venus_load_fw() from the actual size of the memory region? You could do this as an extra patch that comes before this one.
Hi Alex, On 08/24/2018 10:38 AM, Alexandre Courbot wrote: > On Thu, Aug 23, 2018 at 11:29 PM Vikash Garodia <vgarodia@codeaurora.org> wrote: >> >> Add routine to reset the ARM9 and brings it out of reset. Also >> abstract the Venus CPU state handling with a new function. This >> is in preparation to add PIL functionality in venus driver. >> >> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org> >> --- >> drivers/media/platform/qcom/venus/core.h | 2 ++ >> drivers/media/platform/qcom/venus/firmware.c | 33 ++++++++++++++++++++++++ >> drivers/media/platform/qcom/venus/firmware.h | 11 ++++++++ >> drivers/media/platform/qcom/venus/hfi_venus.c | 13 +++------- >> drivers/media/platform/qcom/venus/hfi_venus_io.h | 7 +++++ >> 5 files changed, 57 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h >> index 2f02365..dfd5c10 100644 >> --- a/drivers/media/platform/qcom/venus/core.h >> +++ b/drivers/media/platform/qcom/venus/core.h >> @@ -98,6 +98,7 @@ struct venus_caps { >> * @dev: convenience struct device pointer >> * @dev_dec: convenience struct device pointer for decoder device >> * @dev_enc: convenience struct device pointer for encoder device >> + * @no_tz: a flag that suggests presence of trustzone >> * @lock: a lock for this strucure >> * @instances: a list_head of all instances >> * @insts_count: num of instances >> @@ -129,6 +130,7 @@ struct venus_core { >> struct device *dev; >> struct device *dev_dec; >> struct device *dev_enc; >> + bool no_tz; >> struct mutex lock; >> struct list_head instances; >> atomic_t insts_count; >> diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c >> index c4a5778..a9d042e 100644 >> --- a/drivers/media/platform/qcom/venus/firmware.c >> +++ b/drivers/media/platform/qcom/venus/firmware.c >> @@ -22,10 +22,43 @@ >> #include <linux/sizes.h> >> #include <linux/soc/qcom/mdt_loader.h> >> >> +#include "core.h" >> #include "firmware.h" >> +#include "hfi_venus_io.h" >> >> #define VENUS_PAS_ID 9 >> #define VENUS_FW_MEM_SIZE (6 * SZ_1M) > > This is making a strong assumption about the size of the FW memory > region, which in practice is not always true (I had to reduce it to > 5MB). How about having this as a member of venus_core, which is Why you reduced to 5MB? Is there an issue with 6MB or you don't want to waste reserved memory? > initialized in venus_load_fw() from the actual size of the memory > region? You could do this as an extra patch that comes before this > one. > The size is 6MB by historical reasons and they are no more valid, so I think we could safely decrease to 5MB. I could prepare a patch for that.
On 2018-08-24 14:27, Stanimir Varbanov wrote: > Hi Alex, > > On 08/24/2018 10:38 AM, Alexandre Courbot wrote: >> On Thu, Aug 23, 2018 at 11:29 PM Vikash Garodia >> <vgarodia@codeaurora.org> wrote: >>> [snip] >>> index c4a5778..a9d042e 100644 >>> --- a/drivers/media/platform/qcom/venus/firmware.c >>> +++ b/drivers/media/platform/qcom/venus/firmware.c >>> @@ -22,10 +22,43 @@ >>> #include <linux/sizes.h> >>> #include <linux/soc/qcom/mdt_loader.h> >>> >>> +#include "core.h" >>> #include "firmware.h" >>> +#include "hfi_venus_io.h" >>> >>> #define VENUS_PAS_ID 9 >>> #define VENUS_FW_MEM_SIZE (6 * SZ_1M) >> >> This is making a strong assumption about the size of the FW memory >> region, which in practice is not always true (I had to reduce it to >> 5MB). How about having this as a member of venus_core, which is > > Why you reduced to 5MB? Is there an issue with 6MB or you don't want to > waste reserved memory? > >> initialized in venus_load_fw() from the actual size of the memory >> region? You could do this as an extra patch that comes before this >> one. I would go with existing design than relying on the size specified in the memory-region for venus. size loaded is always taken from DT while the VENUS_FW_MEM_SIZE serves the purpose of sanity check. > The size is 6MB by historical reasons and they are no more valid, so I > think we could safely decrease to 5MB. I could prepare a patch for > that. Thanks Stan. Initial patch in this series had 5MB. We discussed earlier to keep it as is and take it as a separate patch to update from 6MB to 5MB.
On Fri, Aug 24, 2018 at 5:57 PM Stanimir Varbanov <stanimir.varbanov@linaro.org> wrote: > > Hi Alex, > > On 08/24/2018 10:38 AM, Alexandre Courbot wrote: > > On Thu, Aug 23, 2018 at 11:29 PM Vikash Garodia <vgarodia@codeaurora.org> wrote: > >> > >> Add routine to reset the ARM9 and brings it out of reset. Also > >> abstract the Venus CPU state handling with a new function. This > >> is in preparation to add PIL functionality in venus driver. > >> > >> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org> > >> --- > >> drivers/media/platform/qcom/venus/core.h | 2 ++ > >> drivers/media/platform/qcom/venus/firmware.c | 33 ++++++++++++++++++++++++ > >> drivers/media/platform/qcom/venus/firmware.h | 11 ++++++++ > >> drivers/media/platform/qcom/venus/hfi_venus.c | 13 +++------- > >> drivers/media/platform/qcom/venus/hfi_venus_io.h | 7 +++++ > >> 5 files changed, 57 insertions(+), 9 deletions(-) > >> > >> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h > >> index 2f02365..dfd5c10 100644 > >> --- a/drivers/media/platform/qcom/venus/core.h > >> +++ b/drivers/media/platform/qcom/venus/core.h > >> @@ -98,6 +98,7 @@ struct venus_caps { > >> * @dev: convenience struct device pointer > >> * @dev_dec: convenience struct device pointer for decoder device > >> * @dev_enc: convenience struct device pointer for encoder device > >> + * @no_tz: a flag that suggests presence of trustzone > >> * @lock: a lock for this strucure > >> * @instances: a list_head of all instances > >> * @insts_count: num of instances > >> @@ -129,6 +130,7 @@ struct venus_core { > >> struct device *dev; > >> struct device *dev_dec; > >> struct device *dev_enc; > >> + bool no_tz; > >> struct mutex lock; > >> struct list_head instances; > >> atomic_t insts_count; > >> diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c > >> index c4a5778..a9d042e 100644 > >> --- a/drivers/media/platform/qcom/venus/firmware.c > >> +++ b/drivers/media/platform/qcom/venus/firmware.c > >> @@ -22,10 +22,43 @@ > >> #include <linux/sizes.h> > >> #include <linux/soc/qcom/mdt_loader.h> > >> > >> +#include "core.h" > >> #include "firmware.h" > >> +#include "hfi_venus_io.h" > >> > >> #define VENUS_PAS_ID 9 > >> #define VENUS_FW_MEM_SIZE (6 * SZ_1M) > > > > This is making a strong assumption about the size of the FW memory > > region, which in practice is not always true (I had to reduce it to > > 5MB). How about having this as a member of venus_core, which is > > Why you reduced to 5MB? Is there an issue with 6MB or you don't want to > waste reserved memory? The DT layout of our board only has 5MB reserved for Venus. > > initialized in venus_load_fw() from the actual size of the memory > > region? You could do this as an extra patch that comes before this > > one. > > > > The size is 6MB by historical reasons and they are no more valid, so I > think we could safely decrease to 5MB. I could prepare a patch for that. Whether we settle with 6MB or 5MB, that size remains arbitrary and not based on the actual firmware size. And __qcom_mdt_load() does check that the firmware fits the memory area. So I don't understand what extra safety is added by ensuring the memory region is larger than a given number of megabytes?
On 08/27/2018 06:04 AM, Alexandre Courbot wrote: > On Fri, Aug 24, 2018 at 5:57 PM Stanimir Varbanov > <stanimir.varbanov@linaro.org> wrote: >> >> Hi Alex, >> >> On 08/24/2018 10:38 AM, Alexandre Courbot wrote: >>> On Thu, Aug 23, 2018 at 11:29 PM Vikash Garodia <vgarodia@codeaurora.org> wrote: >>>> >>>> Add routine to reset the ARM9 and brings it out of reset. Also >>>> abstract the Venus CPU state handling with a new function. This >>>> is in preparation to add PIL functionality in venus driver. >>>> >>>> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org> >>>> --- >>>> drivers/media/platform/qcom/venus/core.h | 2 ++ >>>> drivers/media/platform/qcom/venus/firmware.c | 33 ++++++++++++++++++++++++ >>>> drivers/media/platform/qcom/venus/firmware.h | 11 ++++++++ >>>> drivers/media/platform/qcom/venus/hfi_venus.c | 13 +++------- >>>> drivers/media/platform/qcom/venus/hfi_venus_io.h | 7 +++++ >>>> 5 files changed, 57 insertions(+), 9 deletions(-) >>>> >>>> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h >>>> index 2f02365..dfd5c10 100644 >>>> --- a/drivers/media/platform/qcom/venus/core.h >>>> +++ b/drivers/media/platform/qcom/venus/core.h >>>> @@ -98,6 +98,7 @@ struct venus_caps { >>>> * @dev: convenience struct device pointer >>>> * @dev_dec: convenience struct device pointer for decoder device >>>> * @dev_enc: convenience struct device pointer for encoder device >>>> + * @no_tz: a flag that suggests presence of trustzone >>>> * @lock: a lock for this strucure >>>> * @instances: a list_head of all instances >>>> * @insts_count: num of instances >>>> @@ -129,6 +130,7 @@ struct venus_core { >>>> struct device *dev; >>>> struct device *dev_dec; >>>> struct device *dev_enc; >>>> + bool no_tz; >>>> struct mutex lock; >>>> struct list_head instances; >>>> atomic_t insts_count; >>>> diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c >>>> index c4a5778..a9d042e 100644 >>>> --- a/drivers/media/platform/qcom/venus/firmware.c >>>> +++ b/drivers/media/platform/qcom/venus/firmware.c >>>> @@ -22,10 +22,43 @@ >>>> #include <linux/sizes.h> >>>> #include <linux/soc/qcom/mdt_loader.h> >>>> >>>> +#include "core.h" >>>> #include "firmware.h" >>>> +#include "hfi_venus_io.h" >>>> >>>> #define VENUS_PAS_ID 9 >>>> #define VENUS_FW_MEM_SIZE (6 * SZ_1M) >>> >>> This is making a strong assumption about the size of the FW memory >>> region, which in practice is not always true (I had to reduce it to >>> 5MB). How about having this as a member of venus_core, which is >> >> Why you reduced to 5MB? Is there an issue with 6MB or you don't want to >> waste reserved memory? > > The DT layout of our board only has 5MB reserved for Venus. > >>> initialized in venus_load_fw() from the actual size of the memory >>> region? You could do this as an extra patch that comes before this >>> one. >>> >> >> The size is 6MB by historical reasons and they are no more valid, so I >> think we could safely decrease to 5MB. I could prepare a patch for that. > > Whether we settle with 6MB or 5MB, that size remains arbitrary and not > based on the actual firmware size. And __qcom_mdt_load() does check If we go with 5MB it will not be arbitrary, cause all firmware we have support to are expecting that size of memory. > that the firmware fits the memory area. So I don't understand what > extra safety is added by ensuring the memory region is larger than a > given number of megabytes? > OK, is that fine for you? Drop size and check does memory region size is big enough to contain the firmware. diff --git a/drivers/media/platform/qcom/venus/firmware.c b/driver/media/platform/qcom/venus/firmware.c index c4a577848dd7..9bf0d21e02d4 100644 --- a/drivers/media/platform/qcom/venus/firmware.c +++ b/drivers/media/platform/qcom/venus/firmware.c @@ -25,7 +25,6 @@ #include "firmware.h" #define VENUS_PAS_ID 9 -#define VENUS_FW_MEM_SIZE (6 * SZ_1M) int venus_boot(struct device *dev, const char *fwname) { @@ -54,25 +53,28 @@ int venus_boot(struct device *dev, const char *fwname) mem_phys = r.start; mem_size = resource_size(&r); - if (mem_size < VENUS_FW_MEM_SIZE) - return -EINVAL; - - mem_va = memremap(r.start, mem_size, MEMREMAP_WC); - if (!mem_va) { - dev_err(dev, "unable to map memory region: %pa+%zx\n", - &r.start, mem_size); - return -ENOMEM; - } - ret = request_firmware(&mdt, fwname, dev); if (ret < 0) - goto err_unmap; + return ret; fw_size = qcom_mdt_get_size(mdt); if (fw_size < 0) { ret = fw_size; release_firmware(mdt); - goto err_unmap; + return ret; + } + + if (mem_size < fw_size) { + release_firmware(mdt); + return -EINVAL; + } + + mem_va = memremap(r.start, mem_size, MEMREMAP_WC); + if (!mem_va) { + release_firmware(mdt); + dev_err(dev, "unable to map memory region: %pa+%zx\n", + &r.start, mem_size); + return -ENOMEM; } ret = qcom_mdt_load(dev, mdt, fwname, VENUS_PAS_ID, mem_va, mem_phys,
On Mon, Aug 27, 2018 at 7:56 PM Stanimir Varbanov <stanimir.varbanov@linaro.org> wrote: > > > > On 08/27/2018 06:04 AM, Alexandre Courbot wrote: > > On Fri, Aug 24, 2018 at 5:57 PM Stanimir Varbanov > > <stanimir.varbanov@linaro.org> wrote: > >> > >> Hi Alex, > >> > >> On 08/24/2018 10:38 AM, Alexandre Courbot wrote: > >>> On Thu, Aug 23, 2018 at 11:29 PM Vikash Garodia <vgarodia@codeaurora.org> wrote: > >>>> > >>>> Add routine to reset the ARM9 and brings it out of reset. Also > >>>> abstract the Venus CPU state handling with a new function. This > >>>> is in preparation to add PIL functionality in venus driver. > >>>> > >>>> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org> > >>>> --- > >>>> drivers/media/platform/qcom/venus/core.h | 2 ++ > >>>> drivers/media/platform/qcom/venus/firmware.c | 33 ++++++++++++++++++++++++ > >>>> drivers/media/platform/qcom/venus/firmware.h | 11 ++++++++ > >>>> drivers/media/platform/qcom/venus/hfi_venus.c | 13 +++------- > >>>> drivers/media/platform/qcom/venus/hfi_venus_io.h | 7 +++++ > >>>> 5 files changed, 57 insertions(+), 9 deletions(-) > >>>> > >>>> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h > >>>> index 2f02365..dfd5c10 100644 > >>>> --- a/drivers/media/platform/qcom/venus/core.h > >>>> +++ b/drivers/media/platform/qcom/venus/core.h > >>>> @@ -98,6 +98,7 @@ struct venus_caps { > >>>> * @dev: convenience struct device pointer > >>>> * @dev_dec: convenience struct device pointer for decoder device > >>>> * @dev_enc: convenience struct device pointer for encoder device > >>>> + * @no_tz: a flag that suggests presence of trustzone > >>>> * @lock: a lock for this strucure > >>>> * @instances: a list_head of all instances > >>>> * @insts_count: num of instances > >>>> @@ -129,6 +130,7 @@ struct venus_core { > >>>> struct device *dev; > >>>> struct device *dev_dec; > >>>> struct device *dev_enc; > >>>> + bool no_tz; > >>>> struct mutex lock; > >>>> struct list_head instances; > >>>> atomic_t insts_count; > >>>> diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c > >>>> index c4a5778..a9d042e 100644 > >>>> --- a/drivers/media/platform/qcom/venus/firmware.c > >>>> +++ b/drivers/media/platform/qcom/venus/firmware.c > >>>> @@ -22,10 +22,43 @@ > >>>> #include <linux/sizes.h> > >>>> #include <linux/soc/qcom/mdt_loader.h> > >>>> > >>>> +#include "core.h" > >>>> #include "firmware.h" > >>>> +#include "hfi_venus_io.h" > >>>> > >>>> #define VENUS_PAS_ID 9 > >>>> #define VENUS_FW_MEM_SIZE (6 * SZ_1M) > >>> > >>> This is making a strong assumption about the size of the FW memory > >>> region, which in practice is not always true (I had to reduce it to > >>> 5MB). How about having this as a member of venus_core, which is > >> > >> Why you reduced to 5MB? Is there an issue with 6MB or you don't want to > >> waste reserved memory? > > > > The DT layout of our board only has 5MB reserved for Venus. > > > >>> initialized in venus_load_fw() from the actual size of the memory > >>> region? You could do this as an extra patch that comes before this > >>> one. > >>> > >> > >> The size is 6MB by historical reasons and they are no more valid, so I > >> think we could safely decrease to 5MB. I could prepare a patch for that. > > > > Whether we settle with 6MB or 5MB, that size remains arbitrary and not > > based on the actual firmware size. And __qcom_mdt_load() does check > > If we go with 5MB it will not be arbitrary, cause all firmware we have > support to are expecting that size of memory. > > > that the firmware fits the memory area. So I don't understand what > > extra safety is added by ensuring the memory region is larger than a > > given number of megabytes? > > > > OK, is that fine for you? Drop size and check does memory region size is > big enough to contain the firmware. > > diff --git a/drivers/media/platform/qcom/venus/firmware.c > b/driver/media/platform/qcom/venus/firmware.c > index c4a577848dd7..9bf0d21e02d4 100644 > --- a/drivers/media/platform/qcom/venus/firmware.c > +++ b/drivers/media/platform/qcom/venus/firmware.c > @@ -25,7 +25,6 @@ > #include "firmware.h" > > #define VENUS_PAS_ID 9 > -#define VENUS_FW_MEM_SIZE (6 * SZ_1M) > > int venus_boot(struct device *dev, const char *fwname) > { > @@ -54,25 +53,28 @@ int venus_boot(struct device *dev, const char *fwname) > mem_phys = r.start; > mem_size = resource_size(&r); > > - if (mem_size < VENUS_FW_MEM_SIZE) > - return -EINVAL; > - > - mem_va = memremap(r.start, mem_size, MEMREMAP_WC); > - if (!mem_va) { > - dev_err(dev, "unable to map memory region: %pa+%zx\n", > - &r.start, mem_size); > - return -ENOMEM; > - } > - > ret = request_firmware(&mdt, fwname, dev); > if (ret < 0) > - goto err_unmap; > + return ret; > > fw_size = qcom_mdt_get_size(mdt); > if (fw_size < 0) { > ret = fw_size; > release_firmware(mdt); > - goto err_unmap; > + return ret; > + } > + > + if (mem_size < fw_size) { > + release_firmware(mdt); > + return -EINVAL; > + } > + > + mem_va = memremap(r.start, mem_size, MEMREMAP_WC); > + if (!mem_va) { > + release_firmware(mdt); > + dev_err(dev, "unable to map memory region: %pa+%zx\n", > + &r.start, mem_size); > + return -ENOMEM; > } That looks reasonable to me, at least if the firmware does not require extra memory beyond its reported size. But you know the firmware better, so your call! :)
diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h index 2f02365..dfd5c10 100644 --- a/drivers/media/platform/qcom/venus/core.h +++ b/drivers/media/platform/qcom/venus/core.h @@ -98,6 +98,7 @@ struct venus_caps { * @dev: convenience struct device pointer * @dev_dec: convenience struct device pointer for decoder device * @dev_enc: convenience struct device pointer for encoder device + * @no_tz: a flag that suggests presence of trustzone * @lock: a lock for this strucure * @instances: a list_head of all instances * @insts_count: num of instances @@ -129,6 +130,7 @@ struct venus_core { struct device *dev; struct device *dev_dec; struct device *dev_enc; + bool no_tz; struct mutex lock; struct list_head instances; atomic_t insts_count; diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c index c4a5778..a9d042e 100644 --- a/drivers/media/platform/qcom/venus/firmware.c +++ b/drivers/media/platform/qcom/venus/firmware.c @@ -22,10 +22,43 @@ #include <linux/sizes.h> #include <linux/soc/qcom/mdt_loader.h> +#include "core.h" #include "firmware.h" +#include "hfi_venus_io.h" #define VENUS_PAS_ID 9 #define VENUS_FW_MEM_SIZE (6 * SZ_1M) +#define VENUS_FW_START_ADDR 0x0 + +static void venus_reset_cpu(struct venus_core *core) +{ + void __iomem *base = core->base; + + writel(0, base + WRAPPER_FW_START_ADDR); + writel(VENUS_FW_MEM_SIZE, base + WRAPPER_FW_END_ADDR); + writel(0, base + WRAPPER_CPA_START_ADDR); + writel(VENUS_FW_MEM_SIZE, base + WRAPPER_CPA_END_ADDR); + writel(VENUS_FW_MEM_SIZE, base + WRAPPER_NP_START_ADDR); + writel(VENUS_FW_MEM_SIZE, base + WRAPPER_NP_END_ADDR); + writel(0x0, base + WRAPPER_CPU_CGC_DIS); + writel(0x0, base + WRAPPER_CPU_CLOCK_CONFIG); + + /* Bring ARM9 out of reset */ + writel(0, base + WRAPPER_A9SS_SW_RESET); +} + +int venus_set_hw_state(struct venus_core *core, bool resume) +{ + if (!core->no_tz) + return qcom_scm_set_remote_state(resume, 0); + + if (resume) + venus_reset_cpu(core); + else + writel(1, core->base + WRAPPER_A9SS_SW_RESET); + + return 0; +} int venus_boot(struct device *dev, const char *fwname) { diff --git a/drivers/media/platform/qcom/venus/firmware.h b/drivers/media/platform/qcom/venus/firmware.h index 428efb5..397570c 100644 --- a/drivers/media/platform/qcom/venus/firmware.h +++ b/drivers/media/platform/qcom/venus/firmware.h @@ -18,5 +18,16 @@ int venus_boot(struct device *dev, const char *fwname); int venus_shutdown(struct device *dev); +int venus_set_hw_state(struct venus_core *core, bool suspend); + +static inline int venus_set_hw_state_suspend(struct venus_core *core) +{ + return venus_set_hw_state(core, false); +} + +static inline int venus_set_hw_state_resume(struct venus_core *core) +{ + return venus_set_hw_state(core, true); +} #endif diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c index 1240855..074837e 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; @@ -575,7 +570,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_suspend(hdev->core); if (ret) return ret; @@ -595,7 +590,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_resume(hdev->core); if (ret) goto err; @@ -608,7 +603,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_suspend(hdev->core); err: hdev->power_enabled = false; return ret; diff --git a/drivers/media/platform/qcom/venus/hfi_venus_io.h b/drivers/media/platform/qcom/venus/hfi_venus_io.h index def0926..483348d 100644 --- a/drivers/media/platform/qcom/venus/hfi_venus_io.h +++ b/drivers/media/platform/qcom/venus/hfi_venus_io.h @@ -112,6 +112,13 @@ #define WRAPPER_CPU_STATUS (WRAPPER_BASE + 0x2014) #define WRAPPER_CPU_STATUS_WFI BIT(0) #define WRAPPER_SW_RESET (WRAPPER_BASE + 0x3000) +#define WRAPPER_CPA_START_ADDR (WRAPPER_BASE + 0x1020) +#define WRAPPER_CPA_END_ADDR (WRAPPER_BASE + 0x1024) +#define WRAPPER_FW_START_ADDR (WRAPPER_BASE + 0x1028) +#define WRAPPER_FW_END_ADDR (WRAPPER_BASE + 0x102C) +#define WRAPPER_NP_START_ADDR (WRAPPER_BASE + 0x1030) +#define WRAPPER_NP_END_ADDR (WRAPPER_BASE + 0x1034) +#define WRAPPER_A9SS_SW_RESET (WRAPPER_BASE + 0x3000) /* Venus 4xx */ #define WRAPPER_VCODEC0_MMCC_POWER_STATUS (WRAPPER_BASE + 0x90)
Add routine to reset the ARM9 and brings it out of reset. Also abstract the Venus CPU state handling with a new function. This is in preparation to add PIL functionality in venus driver. Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org> --- drivers/media/platform/qcom/venus/core.h | 2 ++ drivers/media/platform/qcom/venus/firmware.c | 33 ++++++++++++++++++++++++ drivers/media/platform/qcom/venus/firmware.h | 11 ++++++++ drivers/media/platform/qcom/venus/hfi_venus.c | 13 +++------- drivers/media/platform/qcom/venus/hfi_venus_io.h | 7 +++++ 5 files changed, 57 insertions(+), 9 deletions(-)