diff mbox series

[v6,2/4] venus: firmware: move load firmware in a separate function

Message ID 1535034528-11590-3-git-send-email-vgarodia@codeaurora.org (mailing list archive)
State New, archived
Headers show
Series Venus updates - PIL | expand

Commit Message

Vikash Garodia Aug. 23, 2018, 2:28 p.m. UTC 
Separate firmware loading part into a new function.

Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
---
 drivers/media/platform/qcom/venus/core.c     |  4 +-
 drivers/media/platform/qcom/venus/firmware.c | 55 ++++++++++++++++++----------
 drivers/media/platform/qcom/venus/firmware.h |  2 +-
 3 files changed, 38 insertions(+), 23 deletions(-)

Comments

Alexandre Courbot Aug. 24, 2018, 7:39 a.m. UTC | #1 
On Thu, Aug 23, 2018 at 11:29 PM Vikash Garodia <vgarodia@codeaurora.org> wrote:
>
> Separate firmware loading part into a new function.
>
> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
> ---
>  drivers/media/platform/qcom/venus/core.c     |  4 +-
>  drivers/media/platform/qcom/venus/firmware.c | 55 ++++++++++++++++++----------
>  drivers/media/platform/qcom/venus/firmware.h |  2 +-
>  3 files changed, 38 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> index bb6add9..75b9785 100644
> --- a/drivers/media/platform/qcom/venus/core.c
> +++ b/drivers/media/platform/qcom/venus/core.c
> @@ -84,7 +84,7 @@ static void venus_sys_error_handler(struct work_struct *work)
>
>         pm_runtime_get_sync(core->dev);
>
> -       ret |= venus_boot(core->dev, core->res->fwname);
> +       ret |= venus_boot(core);
>
>         ret |= hfi_core_resume(core, true);
>
> @@ -284,7 +284,7 @@ static int venus_probe(struct platform_device *pdev)
>         if (ret < 0)
>                 goto err_runtime_disable;
>
> -       ret = venus_boot(dev, core->res->fwname);
> +       ret = venus_boot(core);
>         if (ret)
>                 goto err_runtime_disable;
>
> diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c
> index a9d042e..34224eb 100644
> --- a/drivers/media/platform/qcom/venus/firmware.c
> +++ b/drivers/media/platform/qcom/venus/firmware.c
> @@ -60,20 +60,18 @@ int venus_set_hw_state(struct venus_core *core, bool resume)
>         return 0;
>  }
>
> -int venus_boot(struct device *dev, const char *fwname)
> +static int venus_load_fw(struct venus_core *core, const char *fwname,
> +                        phys_addr_t *mem_phys, size_t *mem_size)

Following the remarks of the previous patch, you would have mem_phys
and mem_size as members of venus_core (probably renamed as fw_mem_addr
and fw_mem_size).

>  {
>         const struct firmware *mdt;
>         struct device_node *node;
> -       phys_addr_t mem_phys;
> +       struct device *dev;
>         struct resource r;
>         ssize_t fw_size;
> -       size_t mem_size;
>         void *mem_va;
>         int ret;
>
> -       if (!IS_ENABLED(CONFIG_QCOM_MDT_LOADER) || !qcom_scm_is_available())
> -               return -EPROBE_DEFER;

!IS_ENABLED(CONFIG_QCOM_MDT_LOADER) is not a condition that can change
at runtime, and returning -EPROBE_DEFER in that case seems erroneous
to me. Instead, wouldn't it make more sense to make the driver depend
on QCOM_MDT_LOADER?

> -
> +       dev = core->dev;
>         node = of_parse_phandle(dev->of_node, "memory-region", 0);
>         if (!node) {
>                 dev_err(dev, "no memory-region specified\n");
> @@ -84,16 +82,16 @@ int venus_boot(struct device *dev, const char *fwname)
>         if (ret)
>                 return ret;
>
> -       mem_phys = r.start;
> -       mem_size = resource_size(&r);
> +       *mem_phys = r.start;
> +       *mem_size = resource_size(&r);
>
> -       if (mem_size < VENUS_FW_MEM_SIZE)
> +       if (*mem_size < VENUS_FW_MEM_SIZE)
>                 return -EINVAL;
>
> -       mem_va = memremap(r.start, mem_size, MEMREMAP_WC);
> +       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);
> +                       &r.start, *mem_size);
>                 return -ENOMEM;
>         }
>
> @@ -108,23 +106,40 @@ int venus_boot(struct device *dev, const char *fwname)
>                 goto err_unmap;
>         }
>
> -       ret = qcom_mdt_load(dev, mdt, fwname, VENUS_PAS_ID, mem_va, mem_phys,
> -                           mem_size, NULL);
> +       if (core->no_tz)
> +               ret = qcom_mdt_load_no_init(dev, mdt, fwname, VENUS_PAS_ID,
> +                                           mem_va, *mem_phys, *mem_size, NULL);
> +       else
> +               ret = qcom_mdt_load(dev, mdt, fwname, VENUS_PAS_ID,
> +                                   mem_va, *mem_phys, *mem_size, NULL);
>
>         release_firmware(mdt);
>
> -       if (ret)
> -               goto err_unmap;
> -
> -       ret = qcom_scm_pas_auth_and_reset(VENUS_PAS_ID);
> -       if (ret)
> -               goto err_unmap;
> -
>  err_unmap:
>         memunmap(mem_va);
>         return ret;
>  }
>
> +int venus_boot(struct venus_core *core)
> +{
> +       struct device *dev = core->dev;
> +       phys_addr_t mem_phys;
> +       size_t mem_size;
> +       int ret;
> +
> +       if (!IS_ENABLED(CONFIG_QCOM_MDT_LOADER) ||
> +           (!core->no_tz && !qcom_scm_is_available()))
> +               return -EPROBE_DEFER;

Same remark as above here.
Stanimir Varbanov Aug. 24, 2018, 9:01 a.m. UTC | #2 
Hi Alex,

On 08/24/2018 10:39 AM, Alexandre Courbot wrote:
> On Thu, Aug 23, 2018 at 11:29 PM Vikash Garodia <vgarodia@codeaurora.org> wrote:
>>
>> Separate firmware loading part into a new function.
>>
>> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
>> ---
>>  drivers/media/platform/qcom/venus/core.c     |  4 +-
>>  drivers/media/platform/qcom/venus/firmware.c | 55 ++++++++++++++++++----------
>>  drivers/media/platform/qcom/venus/firmware.h |  2 +-
>>  3 files changed, 38 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
>> index bb6add9..75b9785 100644
>> --- a/drivers/media/platform/qcom/venus/core.c
>> +++ b/drivers/media/platform/qcom/venus/core.c
>> @@ -84,7 +84,7 @@ static void venus_sys_error_handler(struct work_struct *work)
>>
>>         pm_runtime_get_sync(core->dev);
>>
>> -       ret |= venus_boot(core->dev, core->res->fwname);
>> +       ret |= venus_boot(core);
>>
>>         ret |= hfi_core_resume(core, true);
>>
>> @@ -284,7 +284,7 @@ static int venus_probe(struct platform_device *pdev)
>>         if (ret < 0)
>>                 goto err_runtime_disable;
>>
>> -       ret = venus_boot(dev, core->res->fwname);
>> +       ret = venus_boot(core);
>>         if (ret)
>>                 goto err_runtime_disable;
>>
>> diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c
>> index a9d042e..34224eb 100644
>> --- a/drivers/media/platform/qcom/venus/firmware.c
>> +++ b/drivers/media/platform/qcom/venus/firmware.c
>> @@ -60,20 +60,18 @@ int venus_set_hw_state(struct venus_core *core, bool resume)
>>         return 0;
>>  }
>>
>> -int venus_boot(struct device *dev, const char *fwname)
>> +static int venus_load_fw(struct venus_core *core, const char *fwname,
>> +                        phys_addr_t *mem_phys, size_t *mem_size)
> 
> Following the remarks of the previous patch, you would have mem_phys
> and mem_size as members of venus_core (probably renamed as fw_mem_addr
> and fw_mem_size).
> 
>>  {
>>         const struct firmware *mdt;
>>         struct device_node *node;
>> -       phys_addr_t mem_phys;
>> +       struct device *dev;
>>         struct resource r;
>>         ssize_t fw_size;
>> -       size_t mem_size;
>>         void *mem_va;
>>         int ret;
>>
>> -       if (!IS_ENABLED(CONFIG_QCOM_MDT_LOADER) || !qcom_scm_is_available())
>> -               return -EPROBE_DEFER;
> 
> !IS_ENABLED(CONFIG_QCOM_MDT_LOADER) is not a condition that can change
> at runtime, and returning -EPROBE_DEFER in that case seems erroneous
> to me. Instead, wouldn't it make more sense to make the driver depend
> on QCOM_MDT_LOADER?

That was made on purpose, for more info git show b8f9bdc151e4a
Alexandre Courbot Aug. 27, 2018, 3:10 a.m. UTC | #3 
On Fri, Aug 24, 2018 at 6:01 PM Stanimir Varbanov
<stanimir.varbanov@linaro.org> wrote:
>
> Hi Alex,
>
> On 08/24/2018 10:39 AM, Alexandre Courbot wrote:
> > On Thu, Aug 23, 2018 at 11:29 PM Vikash Garodia <vgarodia@codeaurora.org> wrote:
> >>
> >> Separate firmware loading part into a new function.
> >>
> >> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
> >> ---
> >>  drivers/media/platform/qcom/venus/core.c     |  4 +-
> >>  drivers/media/platform/qcom/venus/firmware.c | 55 ++++++++++++++++++----------
> >>  drivers/media/platform/qcom/venus/firmware.h |  2 +-
> >>  3 files changed, 38 insertions(+), 23 deletions(-)
> >>
> >> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> >> index bb6add9..75b9785 100644
> >> --- a/drivers/media/platform/qcom/venus/core.c
> >> +++ b/drivers/media/platform/qcom/venus/core.c
> >> @@ -84,7 +84,7 @@ static void venus_sys_error_handler(struct work_struct *work)
> >>
> >>         pm_runtime_get_sync(core->dev);
> >>
> >> -       ret |= venus_boot(core->dev, core->res->fwname);
> >> +       ret |= venus_boot(core);
> >>
> >>         ret |= hfi_core_resume(core, true);
> >>
> >> @@ -284,7 +284,7 @@ static int venus_probe(struct platform_device *pdev)
> >>         if (ret < 0)
> >>                 goto err_runtime_disable;
> >>
> >> -       ret = venus_boot(dev, core->res->fwname);
> >> +       ret = venus_boot(core);
> >>         if (ret)
> >>                 goto err_runtime_disable;
> >>
> >> diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c
> >> index a9d042e..34224eb 100644
> >> --- a/drivers/media/platform/qcom/venus/firmware.c
> >> +++ b/drivers/media/platform/qcom/venus/firmware.c
> >> @@ -60,20 +60,18 @@ int venus_set_hw_state(struct venus_core *core, bool resume)
> >>         return 0;
> >>  }
> >>
> >> -int venus_boot(struct device *dev, const char *fwname)
> >> +static int venus_load_fw(struct venus_core *core, const char *fwname,
> >> +                        phys_addr_t *mem_phys, size_t *mem_size)
> >
> > Following the remarks of the previous patch, you would have mem_phys
> > and mem_size as members of venus_core (probably renamed as fw_mem_addr
> > and fw_mem_size).
> >
> >>  {
> >>         const struct firmware *mdt;
> >>         struct device_node *node;
> >> -       phys_addr_t mem_phys;
> >> +       struct device *dev;
> >>         struct resource r;
> >>         ssize_t fw_size;
> >> -       size_t mem_size;
> >>         void *mem_va;
> >>         int ret;
> >>
> >> -       if (!IS_ENABLED(CONFIG_QCOM_MDT_LOADER) || !qcom_scm_is_available())
> >> -               return -EPROBE_DEFER;
> >
> > !IS_ENABLED(CONFIG_QCOM_MDT_LOADER) is not a condition that can change
> > at runtime, and returning -EPROBE_DEFER in that case seems erroneous
> > to me. Instead, wouldn't it make more sense to make the driver depend
> > on QCOM_MDT_LOADER?
>
> That was made on purpose, for more info git show b8f9bdc151e4a

Ah, I see. Still, in the current form it seems like
qcom_scm_is_available() is not resolved thanks to a compiler
optimization. Wouldn't it be more explicit to do something like

#if IS_ENABLED(CONFIG_QCOM_MDT_LOADER)
if (!qcom_scm_is_available())
    return -EPROBE_DEFER;
#endif

instead?
diff mbox series

Patch

diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index bb6add9..75b9785 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -84,7 +84,7 @@  static void venus_sys_error_handler(struct work_struct *work)
 
 	pm_runtime_get_sync(core->dev);
 
-	ret |= venus_boot(core->dev, core->res->fwname);
+	ret |= venus_boot(core);
 
 	ret |= hfi_core_resume(core, true);
 
@@ -284,7 +284,7 @@  static int venus_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto err_runtime_disable;
 
-	ret = venus_boot(dev, core->res->fwname);
+	ret = venus_boot(core);
 	if (ret)
 		goto err_runtime_disable;
 
diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c
index a9d042e..34224eb 100644
--- a/drivers/media/platform/qcom/venus/firmware.c
+++ b/drivers/media/platform/qcom/venus/firmware.c
@@ -60,20 +60,18 @@  int venus_set_hw_state(struct venus_core *core, bool resume)
 	return 0;
 }
 
-int venus_boot(struct device *dev, const char *fwname)
+static int venus_load_fw(struct venus_core *core, const char *fwname,
+			 phys_addr_t *mem_phys, size_t *mem_size)
 {
 	const struct firmware *mdt;
 	struct device_node *node;
-	phys_addr_t mem_phys;
+	struct device *dev;
 	struct resource r;
 	ssize_t fw_size;
-	size_t mem_size;
 	void *mem_va;
 	int ret;
 
-	if (!IS_ENABLED(CONFIG_QCOM_MDT_LOADER) || !qcom_scm_is_available())
-		return -EPROBE_DEFER;
-
+	dev = core->dev;
 	node = of_parse_phandle(dev->of_node, "memory-region", 0);
 	if (!node) {
 		dev_err(dev, "no memory-region specified\n");
@@ -84,16 +82,16 @@  int venus_boot(struct device *dev, const char *fwname)
 	if (ret)
 		return ret;
 
-	mem_phys = r.start;
-	mem_size = resource_size(&r);
+	*mem_phys = r.start;
+	*mem_size = resource_size(&r);
 
-	if (mem_size < VENUS_FW_MEM_SIZE)
+	if (*mem_size < VENUS_FW_MEM_SIZE)
 		return -EINVAL;
 
-	mem_va = memremap(r.start, mem_size, MEMREMAP_WC);
+	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);
+			&r.start, *mem_size);
 		return -ENOMEM;
 	}
 
@@ -108,23 +106,40 @@  int venus_boot(struct device *dev, const char *fwname)
 		goto err_unmap;
 	}
 
-	ret = qcom_mdt_load(dev, mdt, fwname, VENUS_PAS_ID, mem_va, mem_phys,
-			    mem_size, NULL);
+	if (core->no_tz)
+		ret = qcom_mdt_load_no_init(dev, mdt, fwname, VENUS_PAS_ID,
+					    mem_va, *mem_phys, *mem_size, NULL);
+	else
+		ret = qcom_mdt_load(dev, mdt, fwname, VENUS_PAS_ID,
+				    mem_va, *mem_phys, *mem_size, NULL);
 
 	release_firmware(mdt);
 
-	if (ret)
-		goto err_unmap;
-
-	ret = qcom_scm_pas_auth_and_reset(VENUS_PAS_ID);
-	if (ret)
-		goto err_unmap;
-
 err_unmap:
 	memunmap(mem_va);
 	return ret;
 }
 
+int venus_boot(struct venus_core *core)
+{
+	struct device *dev = core->dev;
+	phys_addr_t mem_phys;
+	size_t mem_size;
+	int ret;
+
+	if (!IS_ENABLED(CONFIG_QCOM_MDT_LOADER) ||
+	    (!core->no_tz && !qcom_scm_is_available()))
+		return -EPROBE_DEFER;
+
+	ret = venus_load_fw(core, core->res->fwname, &mem_phys, &mem_size);
+	if (ret) {
+		dev_err(dev, "fail to load video firmware\n");
+		return -EINVAL;
+	}
+
+	return qcom_scm_pas_auth_and_reset(VENUS_PAS_ID);
+}
+
 int venus_shutdown(struct device *dev)
 {
 	return qcom_scm_pas_shutdown(VENUS_PAS_ID);
diff --git a/drivers/media/platform/qcom/venus/firmware.h b/drivers/media/platform/qcom/venus/firmware.h
index 397570c..1343747 100644
--- a/drivers/media/platform/qcom/venus/firmware.h
+++ b/drivers/media/platform/qcom/venus/firmware.h
@@ -16,7 +16,7 @@ 
 
 struct device;
 
-int venus_boot(struct device *dev, const char *fwname);
+int venus_boot(struct venus_core *core);
 int venus_shutdown(struct device *dev);
 int venus_set_hw_state(struct venus_core *core, bool suspend);