[PATCH v6 0/4] Venus updates - PIL

[PATCH v6 0/4] Venus updates - PIL

[Vikash Garodia]

Hello,

Here is v6 with following comments addressed:

* 4/4 from earlier series was dropped as .probe was not needed.
* indentation as per checkpatch --strict option.
* tested on Venus v4 hardware. 

Stanimir Varbanov (1):
  venus: firmware: register separate platform_device for firmware loader

Vikash Garodia (3):
  venus: firmware: add routine to reset ARM9
  venus: firmware: move load firmware in a separate function
  venus: firmware: add no TZ boot and shutdown routine

 .../devicetree/bindings/media/qcom,venus.txt       |  13 +-
 drivers/media/platform/qcom/venus/core.c           |  24 ++-
 drivers/media/platform/qcom/venus/core.h           |   9 +
 drivers/media/platform/qcom/venus/firmware.c       | 223 +++++++++++++++++++--
 drivers/media/platform/qcom/venus/firmware.h       |  17 +-
 drivers/media/platform/qcom/venus/hfi_venus.c      |  13 +-
 drivers/media/platform/qcom/venus/hfi_venus_io.h   |   8 +
 7 files changed, 265 insertions(+), 42 deletions(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v6 1/4] venus: firmware: add routine to reset ARM9

[Vikash Garodia]

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)
+#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)
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v6 1/4] venus: firmware: add routine to reset ARM9

[Alexandre Courbot]

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.

Re: [PATCH v6 1/4] venus: firmware: add routine to reset ARM9

[Stanimir Varbanov]

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.

-- 
regards,
Stan

Re: [PATCH v6 1/4] venus: firmware: add routine to reset ARM9

[Vikash Garodia]

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.

Re: [PATCH v6 1/4] venus: firmware: add routine to reset ARM9

[Alexandre Courbot]

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?

Re: [PATCH v6 1/4] venus: firmware: add routine to reset ARM9

[Stanimir Varbanov]

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,


-- 
regards,
Stan

Re: [PATCH v6 1/4] venus: firmware: add routine to reset ARM9

[Alexandre Courbot]

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! :)

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

[Vikash Garodia]

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)
 {
 	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);
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Alexandre Courbot]

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.

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

[Stanimir Varbanov]

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

-- 
regards,
Stan

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

[Alexandre Courbot]

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?

[PATCH v6 3/4] venus: firmware: add no TZ boot and shutdown routine

[Vikash Garodia]

Video hardware is mainly comprised of vcodec subsystem and video
control subsystem. Video control has ARM9 which executes the video
firmware instructions whereas vcodec does the video frame processing.
This change adds support to load the video firmware and bring ARM9
out of reset for platforms which does not have trustzone.

Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
---
 drivers/media/platform/qcom/venus/core.c         |  6 +-
 drivers/media/platform/qcom/venus/core.h         |  7 ++
 drivers/media/platform/qcom/venus/firmware.c     | 90 +++++++++++++++++++++++-
 drivers/media/platform/qcom/venus/firmware.h     |  2 +-
 drivers/media/platform/qcom/venus/hfi_venus_io.h |  1 +
 5 files changed, 99 insertions(+), 7 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index 75b9785..393994e 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -76,7 +76,7 @@ static void venus_sys_error_handler(struct work_struct *work)
 	hfi_core_deinit(core, true);
 	hfi_destroy(core);
 	mutex_lock(&core->lock);
-	venus_shutdown(core->dev);
+	venus_shutdown(core);
 
 	pm_runtime_put_sync(core->dev);
 
@@ -323,7 +323,7 @@ static int venus_probe(struct platform_device *pdev)
 err_core_deinit:
 	hfi_core_deinit(core, false);
 err_venus_shutdown:
-	venus_shutdown(dev);
+	venus_shutdown(core);
 err_runtime_disable:
 	pm_runtime_set_suspended(dev);
 	pm_runtime_disable(dev);
@@ -344,7 +344,7 @@ static int venus_remove(struct platform_device *pdev)
 	WARN_ON(ret);
 
 	hfi_destroy(core);
-	venus_shutdown(dev);
+	venus_shutdown(core);
 	of_platform_depopulate(dev);
 
 	pm_runtime_put_sync(dev);
diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
index dfd5c10..b2cb359 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -81,6 +81,11 @@ struct venus_caps {
 	bool valid;	/* used only for Venus v1xx */
 };
 
+struct video_firmware {
+	struct device *dev;
+	struct iommu_domain *iommu_domain;
+};
+
 /**
  * struct venus_core - holds core parameters valid for all instances
  *
@@ -98,6 +103,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
+ * @fw:		a struct for venus firmware info
  * @no_tz:	a flag that suggests presence of trustzone
  * @lock:	a lock for this strucure
  * @instances:	a list_head of all instances
@@ -130,6 +136,7 @@ struct venus_core {
 	struct device *dev;
 	struct device *dev_dec;
 	struct device *dev_enc;
+	struct video_firmware fw;
 	bool no_tz;
 	struct mutex lock;
 	struct list_head instances;
diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c
index 34224eb..79b3858 100644
--- a/drivers/media/platform/qcom/venus/firmware.c
+++ b/drivers/media/platform/qcom/venus/firmware.c
@@ -15,9 +15,11 @@
 #include <linux/device.h>
 #include <linux/firmware.h>
 #include <linux/kernel.h>
+#include <linux/iommu.h>
 #include <linux/io.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
+#include <linux/platform_device.h>
 #include <linux/qcom_scm.h>
 #include <linux/sizes.h>
 #include <linux/soc/qcom/mdt_loader.h>
@@ -120,6 +122,76 @@ static int venus_load_fw(struct venus_core *core, const char *fwname,
 	return ret;
 }
 
+static int venus_boot_no_tz(struct venus_core *core, phys_addr_t mem_phys,
+			    size_t mem_size)
+{
+	struct iommu_domain *iommu_dom;
+	struct device *dev;
+	int ret;
+
+	dev = core->fw.dev;
+	if (!dev)
+		return -EPROBE_DEFER;
+
+	iommu_dom = iommu_domain_alloc(&platform_bus_type);
+	if (!iommu_dom) {
+		dev_err(dev, "Failed to allocate iommu domain\n");
+		return -ENOMEM;
+	}
+
+	ret = iommu_attach_device(iommu_dom, dev);
+	if (ret) {
+		dev_err(dev, "could not attach device\n");
+		goto err_attach;
+	}
+
+	ret = iommu_map(iommu_dom, VENUS_FW_START_ADDR, mem_phys, mem_size,
+			IOMMU_READ | IOMMU_WRITE | IOMMU_PRIV);
+	if (ret) {
+		dev_err(dev, "could not map video firmware region\n");
+		goto err_map;
+	}
+
+	core->fw.iommu_domain = iommu_dom;
+	venus_reset_cpu(core);
+
+	return 0;
+
+err_map:
+	iommu_detach_device(iommu_dom, dev);
+err_attach:
+	iommu_domain_free(iommu_dom);
+	return ret;
+}
+
+static int venus_shutdown_no_tz(struct venus_core *core)
+{
+	struct iommu_domain *iommu;
+	size_t unmapped;
+	u32 reg;
+	struct device *dev = core->fw.dev;
+	void __iomem *base = core->base;
+
+	/* Assert the reset to ARM9 */
+	reg = readl_relaxed(base + WRAPPER_A9SS_SW_RESET);
+	reg |= WRAPPER_A9SS_SW_RESET_BIT;
+	writel_relaxed(reg, base + WRAPPER_A9SS_SW_RESET);
+
+	/* Make sure reset is asserted before the mapping is removed */
+	mb();
+
+	iommu = core->fw.iommu_domain;
+
+	unmapped = iommu_unmap(iommu, VENUS_FW_START_ADDR, VENUS_FW_MEM_SIZE);
+	if (unmapped != VENUS_FW_MEM_SIZE)
+		dev_err(dev, "failed to unmap firmware\n");
+
+	iommu_detach_device(iommu, dev);
+	iommu_domain_free(iommu);
+
+	return 0;
+}
+
 int venus_boot(struct venus_core *core)
 {
 	struct device *dev = core->dev;
@@ -137,10 +209,22 @@ int venus_boot(struct venus_core *core)
 		return -EINVAL;
 	}
 
-	return qcom_scm_pas_auth_and_reset(VENUS_PAS_ID);
+	if (core->no_tz)
+		ret = venus_boot_no_tz(core, mem_phys, mem_size);
+	else
+		ret = qcom_scm_pas_auth_and_reset(VENUS_PAS_ID);
+
+	return ret;
 }
 
-int venus_shutdown(struct device *dev)
+int venus_shutdown(struct venus_core *core)
 {
-	return qcom_scm_pas_shutdown(VENUS_PAS_ID);
+	int ret;
+
+	if (core->no_tz)
+		ret = venus_shutdown_no_tz(core);
+	else
+		ret = qcom_scm_pas_shutdown(VENUS_PAS_ID);
+
+	return ret;
 }
diff --git a/drivers/media/platform/qcom/venus/firmware.h b/drivers/media/platform/qcom/venus/firmware.h
index 1343747..f41b615 100644
--- a/drivers/media/platform/qcom/venus/firmware.h
+++ b/drivers/media/platform/qcom/venus/firmware.h
@@ -17,7 +17,7 @@
 struct device;
 
 int venus_boot(struct venus_core *core);
-int venus_shutdown(struct device *dev);
+int venus_shutdown(struct venus_core *core);
 int venus_set_hw_state(struct venus_core *core, bool suspend);
 
 static inline int venus_set_hw_state_suspend(struct venus_core *core)
diff --git a/drivers/media/platform/qcom/venus/hfi_venus_io.h b/drivers/media/platform/qcom/venus/hfi_venus_io.h
index 483348d..cf63864 100644
--- a/drivers/media/platform/qcom/venus/hfi_venus_io.h
+++ b/drivers/media/platform/qcom/venus/hfi_venus_io.h
@@ -119,6 +119,7 @@
 #define WRAPPER_NP_START_ADDR			(WRAPPER_BASE + 0x1030)
 #define WRAPPER_NP_END_ADDR			(WRAPPER_BASE + 0x1034)
 #define WRAPPER_A9SS_SW_RESET			(WRAPPER_BASE + 0x3000)
+#define WRAPPER_A9SS_SW_RESET_BIT		BIT(4)
 
 /* Venus 4xx */
 #define WRAPPER_VCODEC0_MMCC_POWER_STATUS	(WRAPPER_BASE + 0x90)
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v6 3/4] venus: firmware: add no TZ boot and shutdown routine

[Alexandre Courbot]

On Thu, Aug 23, 2018 at 11:29 PM Vikash Garodia <vgarodia@codeaurora.org> wrote:
>
> Video hardware is mainly comprised of vcodec subsystem and video
> control subsystem. Video control has ARM9 which executes the video
> firmware instructions whereas vcodec does the video frame processing.
> This change adds support to load the video firmware and bring ARM9
> out of reset for platforms which does not have trustzone.
>
> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
> ---
>  drivers/media/platform/qcom/venus/core.c         |  6 +-
>  drivers/media/platform/qcom/venus/core.h         |  7 ++
>  drivers/media/platform/qcom/venus/firmware.c     | 90 +++++++++++++++++++++++-
>  drivers/media/platform/qcom/venus/firmware.h     |  2 +-
>  drivers/media/platform/qcom/venus/hfi_venus_io.h |  1 +
>  5 files changed, 99 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> index 75b9785..393994e 100644
> --- a/drivers/media/platform/qcom/venus/core.c
> +++ b/drivers/media/platform/qcom/venus/core.c
> @@ -76,7 +76,7 @@ static void venus_sys_error_handler(struct work_struct *work)
>         hfi_core_deinit(core, true);
>         hfi_destroy(core);
>         mutex_lock(&core->lock);
> -       venus_shutdown(core->dev);
> +       venus_shutdown(core);
>
>         pm_runtime_put_sync(core->dev);
>
> @@ -323,7 +323,7 @@ static int venus_probe(struct platform_device *pdev)
>  err_core_deinit:
>         hfi_core_deinit(core, false);
>  err_venus_shutdown:
> -       venus_shutdown(dev);
> +       venus_shutdown(core);
>  err_runtime_disable:
>         pm_runtime_set_suspended(dev);
>         pm_runtime_disable(dev);
> @@ -344,7 +344,7 @@ static int venus_remove(struct platform_device *pdev)
>         WARN_ON(ret);
>
>         hfi_destroy(core);
> -       venus_shutdown(dev);
> +       venus_shutdown(core);
>         of_platform_depopulate(dev);
>
>         pm_runtime_put_sync(dev);
> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
> index dfd5c10..b2cb359 100644
> --- a/drivers/media/platform/qcom/venus/core.h
> +++ b/drivers/media/platform/qcom/venus/core.h
> @@ -81,6 +81,11 @@ struct venus_caps {
>         bool valid;     /* used only for Venus v1xx */
>  };
>
> +struct video_firmware {
> +       struct device *dev;
> +       struct iommu_domain *iommu_domain;
> +};
> +
>  /**
>   * struct venus_core - holds core parameters valid for all instances
>   *
> @@ -98,6 +103,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
> + * @fw:                a struct for venus firmware info
>   * @no_tz:     a flag that suggests presence of trustzone
>   * @lock:      a lock for this strucure
>   * @instances: a list_head of all instances
> @@ -130,6 +136,7 @@ struct venus_core {
>         struct device *dev;
>         struct device *dev_dec;
>         struct device *dev_enc;
> +       struct video_firmware fw;

Since struct video_firmware is only used here I think you can declare
it inline, i.e.

    struct {
        struct device *dev;
        struct iommu_domain *iommu_domain;
    } fw;

This structure is actually a good candidate to hold the firmware
memory area start address and size.

>         bool no_tz;
>         struct mutex lock;
>         struct list_head instances;
> diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c
> index 34224eb..79b3858 100644
> --- a/drivers/media/platform/qcom/venus/firmware.c
> +++ b/drivers/media/platform/qcom/venus/firmware.c
> @@ -15,9 +15,11 @@
>  #include <linux/device.h>
>  #include <linux/firmware.h>
>  #include <linux/kernel.h>
> +#include <linux/iommu.h>
>  #include <linux/io.h>
>  #include <linux/of.h>
>  #include <linux/of_address.h>
> +#include <linux/platform_device.h>
>  #include <linux/qcom_scm.h>
>  #include <linux/sizes.h>
>  #include <linux/soc/qcom/mdt_loader.h>
> @@ -120,6 +122,76 @@ static int venus_load_fw(struct venus_core *core, const char *fwname,
>         return ret;
>  }
>
> +static int venus_boot_no_tz(struct venus_core *core, phys_addr_t mem_phys,
> +                           size_t mem_size)

After moving the firmware address and size into venus_core you won't
need the last two arguments.

> +{
> +       struct iommu_domain *iommu_dom;
> +       struct device *dev;
> +       int ret;
> +
> +       dev = core->fw.dev;
> +       if (!dev)
> +               return -EPROBE_DEFER;
> +
> +       iommu_dom = iommu_domain_alloc(&platform_bus_type);
> +       if (!iommu_dom) {
> +               dev_err(dev, "Failed to allocate iommu domain\n");
> +               return -ENOMEM;
> +       }
> +
> +       ret = iommu_attach_device(iommu_dom, dev);
> +       if (ret) {
> +               dev_err(dev, "could not attach device\n");
> +               goto err_attach;
> +       }

I think like the above belongs more in venus_firmware_init()
(introduced in patch 4/4) than here. There is no reason to
detach/reattach the iommu if we stop the firmware.

> +
> +       ret = iommu_map(iommu_dom, VENUS_FW_START_ADDR, mem_phys, mem_size,
> +                       IOMMU_READ | IOMMU_WRITE | IOMMU_PRIV);
> +       if (ret) {
> +               dev_err(dev, "could not map video firmware region\n");
> +               goto err_map;
> +       }

Maybe this too?

> +
> +       core->fw.iommu_domain = iommu_dom;
> +       venus_reset_cpu(core);
> +
> +       return 0;
> +
> +err_map:
> +       iommu_detach_device(iommu_dom, dev);
> +err_attach:
> +       iommu_domain_free(iommu_dom);
> +       return ret;
> +}
> +
> +static int venus_shutdown_no_tz(struct venus_core *core)
> +{
> +       struct iommu_domain *iommu;
> +       size_t unmapped;
> +       u32 reg;
> +       struct device *dev = core->fw.dev;
> +       void __iomem *base = core->base;
> +
> +       /* Assert the reset to ARM9 */
> +       reg = readl_relaxed(base + WRAPPER_A9SS_SW_RESET);
> +       reg |= WRAPPER_A9SS_SW_RESET_BIT;
> +       writel_relaxed(reg, base + WRAPPER_A9SS_SW_RESET);
> +
> +       /* Make sure reset is asserted before the mapping is removed */
> +       mb();
> +
> +       iommu = core->fw.iommu_domain;
> +
> +       unmapped = iommu_unmap(iommu, VENUS_FW_START_ADDR, VENUS_FW_MEM_SIZE);
> +       if (unmapped != VENUS_FW_MEM_SIZE)
> +               dev_err(dev, "failed to unmap firmware\n");
> +
> +       iommu_detach_device(iommu, dev);
> +       iommu_domain_free(iommu);

Accordingly, this would also be moved into venus_firmware_deinit().

Re: [PATCH v6 3/4] venus: firmware: add no TZ boot and shutdown routine

[Vikash Garodia]

Hi Alex,

On 2018-08-24 13:09, Alexandre Courbot wrote:
> On Thu, Aug 23, 2018 at 11:29 PM Vikash Garodia 
> <vgarodia@codeaurora.org> wrote:

[snip]

>> +struct video_firmware {
>> +       struct device *dev;
>> +       struct iommu_domain *iommu_domain;
>> +};
>> +
>>  /**
>>   * struct venus_core - holds core parameters valid for all instances
>>   *
>> @@ -98,6 +103,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
>> + * @fw:                a struct for venus firmware info
>>   * @no_tz:     a flag that suggests presence of trustzone
>>   * @lock:      a lock for this strucure
>>   * @instances: a list_head of all instances
>> @@ -130,6 +136,7 @@ struct venus_core {
>>         struct device *dev;
>>         struct device *dev_dec;
>>         struct device *dev_enc;
>> +       struct video_firmware fw;
> 
> Since struct video_firmware is only used here I think you can declare
> it inline, i.e.
> 
>     struct {
>         struct device *dev;
>         struct iommu_domain *iommu_domain;
>     } fw;
> 
> This structure is actually a good candidate to hold the firmware
> memory area start address and size.

I can make it inline.
Memory area and size are common parameters populated
locally while loading the firmware with or without tz. Firmware struct 
has
info more specific to firmware device.

[snip]

> 
>> +{
>> +       struct iommu_domain *iommu_dom;
>> +       struct device *dev;
>> +       int ret;
>> +
>> +       dev = core->fw.dev;
>> +       if (!dev)
>> +               return -EPROBE_DEFER;
>> +
>> +       iommu_dom = iommu_domain_alloc(&platform_bus_type);
>> +       if (!iommu_dom) {
>> +               dev_err(dev, "Failed to allocate iommu domain\n");
>> +               return -ENOMEM;
>> +       }
>> +
>> +       ret = iommu_attach_device(iommu_dom, dev);
>> +       if (ret) {
>> +               dev_err(dev, "could not attach device\n");
>> +               goto err_attach;
>> +       }
> 
> I think like the above belongs more in venus_firmware_init()
> (introduced in patch 4/4) than here. There is no reason to
> detach/reattach the iommu if we stop the firmware.

Consider the case when we want to reload the firmware during error 
recovery.
Boot and shutdown will be needed in such case without the need to 
populate
the firmware device again.

[snip]

>> +static int venus_shutdown_no_tz(struct venus_core *core)
>> +{
>> +       struct iommu_domain *iommu;
>> +       size_t unmapped;
>> +       u32 reg;
>> +       struct device *dev = core->fw.dev;
>> +       void __iomem *base = core->base;
>> +
>> +       /* Assert the reset to ARM9 */
>> +       reg = readl_relaxed(base + WRAPPER_A9SS_SW_RESET);
>> +       reg |= WRAPPER_A9SS_SW_RESET_BIT;
>> +       writel_relaxed(reg, base + WRAPPER_A9SS_SW_RESET);
>> +
>> +       /* Make sure reset is asserted before the mapping is removed 
>> */
>> +       mb();
>> +
>> +       iommu = core->fw.iommu_domain;
>> +
>> +       unmapped = iommu_unmap(iommu, VENUS_FW_START_ADDR, 
>> VENUS_FW_MEM_SIZE);
>> +       if (unmapped != VENUS_FW_MEM_SIZE)
>> +               dev_err(dev, "failed to unmap firmware\n");
>> +
>> +       iommu_detach_device(iommu, dev);
>> +       iommu_domain_free(iommu);
> 
> Accordingly, this would also be moved into venus_firmware_deinit().

Same explanation here as well.

Thanks,
Vikash

Re: [PATCH v6 3/4] venus: firmware: add no TZ boot and shutdown routine

[Alexandre Courbot]

On Fri, Aug 24, 2018 at 9:26 PM Vikash Garodia <vgarodia@codeaurora.org> wrote:
>
> Hi Alex,
>
> On 2018-08-24 13:09, Alexandre Courbot wrote:
> > On Thu, Aug 23, 2018 at 11:29 PM Vikash Garodia
> > <vgarodia@codeaurora.org> wrote:
>
> [snip]
>
> >> +struct video_firmware {
> >> +       struct device *dev;
> >> +       struct iommu_domain *iommu_domain;
> >> +};
> >> +
> >>  /**
> >>   * struct venus_core - holds core parameters valid for all instances
> >>   *
> >> @@ -98,6 +103,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
> >> + * @fw:                a struct for venus firmware info
> >>   * @no_tz:     a flag that suggests presence of trustzone
> >>   * @lock:      a lock for this strucure
> >>   * @instances: a list_head of all instances
> >> @@ -130,6 +136,7 @@ struct venus_core {
> >>         struct device *dev;
> >>         struct device *dev_dec;
> >>         struct device *dev_enc;
> >> +       struct video_firmware fw;
> >
> > Since struct video_firmware is only used here I think you can declare
> > it inline, i.e.
> >
> >     struct {
> >         struct device *dev;
> >         struct iommu_domain *iommu_domain;
> >     } fw;
> >
> > This structure is actually a good candidate to hold the firmware
> > memory area start address and size.
>
> I can make it inline.
> Memory area and size are common parameters populated
> locally while loading the firmware with or without tz. Firmware struct
> has
> info more specific to firmware device.
>
> [snip]
>
> >
> >> +{
> >> +       struct iommu_domain *iommu_dom;
> >> +       struct device *dev;
> >> +       int ret;
> >> +
> >> +       dev = core->fw.dev;
> >> +       if (!dev)
> >> +               return -EPROBE_DEFER;
> >> +
> >> +       iommu_dom = iommu_domain_alloc(&platform_bus_type);
> >> +       if (!iommu_dom) {
> >> +               dev_err(dev, "Failed to allocate iommu domain\n");
> >> +               return -ENOMEM;
> >> +       }
> >> +
> >> +       ret = iommu_attach_device(iommu_dom, dev);
> >> +       if (ret) {
> >> +               dev_err(dev, "could not attach device\n");
> >> +               goto err_attach;
> >> +       }
> >
> > I think like the above belongs more in venus_firmware_init()
> > (introduced in patch 4/4) than here. There is no reason to
> > detach/reattach the iommu if we stop the firmware.
>
> Consider the case when we want to reload the firmware during error
> recovery.
> Boot and shutdown will be needed in such case without the need to
> populate
> the firmware device again.

Is there a need to reattach the iommu domain in case of an error?

Re: [PATCH v6 3/4] venus: firmware: add no TZ boot and shutdown routine

[Vikash Garodia]

On 2018-08-27 08:36, Alexandre Courbot wrote:
> On Fri, Aug 24, 2018 at 9:26 PM Vikash Garodia 
> <vgarodia@codeaurora.org> wrote:
>> 
>> Hi Alex,
>> 
>> On 2018-08-24 13:09, Alexandre Courbot wrote:
>> > On Thu, Aug 23, 2018 at 11:29 PM Vikash Garodia
>> > <vgarodia@codeaurora.org> wrote:
>> 
>> [snip]
>> 
>> >> +struct video_firmware {
>> >> +       struct device *dev;
>> >> +       struct iommu_domain *iommu_domain;
>> >> +};
>> >> +
>> >>  /**
>> >>   * struct venus_core - holds core parameters valid for all instances
>> >>   *
>> >> @@ -98,6 +103,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
>> >> + * @fw:                a struct for venus firmware info
>> >>   * @no_tz:     a flag that suggests presence of trustzone
>> >>   * @lock:      a lock for this strucure
>> >>   * @instances: a list_head of all instances
>> >> @@ -130,6 +136,7 @@ struct venus_core {
>> >>         struct device *dev;
>> >>         struct device *dev_dec;
>> >>         struct device *dev_enc;
>> >> +       struct video_firmware fw;
>> >
>> > Since struct video_firmware is only used here I think you can declare
>> > it inline, i.e.
>> >
>> >     struct {
>> >         struct device *dev;
>> >         struct iommu_domain *iommu_domain;
>> >     } fw;
>> >
>> > This structure is actually a good candidate to hold the firmware
>> > memory area start address and size.
>> 
>> I can make it inline.
>> Memory area and size are common parameters populated
>> locally while loading the firmware with or without tz. Firmware struct
>> has
>> info more specific to firmware device.
>> 
>> [snip]
>> 
>> >
>> >> +{
>> >> +       struct iommu_domain *iommu_dom;
>> >> +       struct device *dev;
>> >> +       int ret;
>> >> +
>> >> +       dev = core->fw.dev;
>> >> +       if (!dev)
>> >> +               return -EPROBE_DEFER;
>> >> +
>> >> +       iommu_dom = iommu_domain_alloc(&platform_bus_type);
>> >> +       if (!iommu_dom) {
>> >> +               dev_err(dev, "Failed to allocate iommu domain\n");
>> >> +               return -ENOMEM;
>> >> +       }
>> >> +
>> >> +       ret = iommu_attach_device(iommu_dom, dev);
>> >> +       if (ret) {
>> >> +               dev_err(dev, "could not attach device\n");
>> >> +               goto err_attach;
>> >> +       }
>> >
>> > I think like the above belongs more in venus_firmware_init()
>> > (introduced in patch 4/4) than here. There is no reason to
>> > detach/reattach the iommu if we stop the firmware.
>> 
>> Consider the case when we want to reload the firmware during error
>> recovery.
>> Boot and shutdown will be needed in such case without the need to
>> populate
>> the firmware device again.
> 
> Is there a need to reattach the iommu domain in case of an error?

re-attach is not needed. We can have alloc/attach in init and 
detach/free in deinit.
map/reset and unmap/reset can continue to remain in boot and shutdown 
calls. Let me
know if this is good, i can repatch the series.

Re: [PATCH v6 3/4] venus: firmware: add no TZ boot and shutdown routine

[Alexandre Courbot]

On Mon, Aug 27, 2018 at 9:49 PM Vikash Garodia <vgarodia@codeaurora.org> wrote:
>
> On 2018-08-27 08:36, Alexandre Courbot wrote:
> > On Fri, Aug 24, 2018 at 9:26 PM Vikash Garodia
> > <vgarodia@codeaurora.org> wrote:
> >>
> >> Hi Alex,
> >>
> >> On 2018-08-24 13:09, Alexandre Courbot wrote:
> >> > On Thu, Aug 23, 2018 at 11:29 PM Vikash Garodia
> >> > <vgarodia@codeaurora.org> wrote:
> >>
> >> [snip]
> >>
> >> >> +struct video_firmware {
> >> >> +       struct device *dev;
> >> >> +       struct iommu_domain *iommu_domain;
> >> >> +};
> >> >> +
> >> >>  /**
> >> >>   * struct venus_core - holds core parameters valid for all instances
> >> >>   *
> >> >> @@ -98,6 +103,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
> >> >> + * @fw:                a struct for venus firmware info
> >> >>   * @no_tz:     a flag that suggests presence of trustzone
> >> >>   * @lock:      a lock for this strucure
> >> >>   * @instances: a list_head of all instances
> >> >> @@ -130,6 +136,7 @@ struct venus_core {
> >> >>         struct device *dev;
> >> >>         struct device *dev_dec;
> >> >>         struct device *dev_enc;
> >> >> +       struct video_firmware fw;
> >> >
> >> > Since struct video_firmware is only used here I think you can declare
> >> > it inline, i.e.
> >> >
> >> >     struct {
> >> >         struct device *dev;
> >> >         struct iommu_domain *iommu_domain;
> >> >     } fw;
> >> >
> >> > This structure is actually a good candidate to hold the firmware
> >> > memory area start address and size.
> >>
> >> I can make it inline.
> >> Memory area and size are common parameters populated
> >> locally while loading the firmware with or without tz. Firmware struct
> >> has
> >> info more specific to firmware device.
> >>
> >> [snip]
> >>
> >> >
> >> >> +{
> >> >> +       struct iommu_domain *iommu_dom;
> >> >> +       struct device *dev;
> >> >> +       int ret;
> >> >> +
> >> >> +       dev = core->fw.dev;
> >> >> +       if (!dev)
> >> >> +               return -EPROBE_DEFER;
> >> >> +
> >> >> +       iommu_dom = iommu_domain_alloc(&platform_bus_type);
> >> >> +       if (!iommu_dom) {
> >> >> +               dev_err(dev, "Failed to allocate iommu domain\n");
> >> >> +               return -ENOMEM;
> >> >> +       }
> >> >> +
> >> >> +       ret = iommu_attach_device(iommu_dom, dev);
> >> >> +       if (ret) {
> >> >> +               dev_err(dev, "could not attach device\n");
> >> >> +               goto err_attach;
> >> >> +       }
> >> >
> >> > I think like the above belongs more in venus_firmware_init()
> >> > (introduced in patch 4/4) than here. There is no reason to
> >> > detach/reattach the iommu if we stop the firmware.
> >>
> >> Consider the case when we want to reload the firmware during error
> >> recovery.
> >> Boot and shutdown will be needed in such case without the need to
> >> populate
> >> the firmware device again.
> >
> > Is there a need to reattach the iommu domain in case of an error?
>
> re-attach is not needed. We can have alloc/attach in init and
> detach/free in deinit.
> map/reset and unmap/reset can continue to remain in boot and shutdown
> calls. Let me
> know if this is good, i can repatch the series.

Yeah, the idea is to avoid repeating operations that do not need to be. Thanks!

[PATCH v6 4/4] venus: firmware: register separate platform_device for firmware loader

[Vikash Garodia]

From: Stanimir Varbanov <stanimir.varbanov@linaro.org>

This registers a firmware platform_device and associate it with
video-firmware DT subnode. Then calls dma configure to initialize
dma and iommu.

Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
---
 .../devicetree/bindings/media/qcom,venus.txt       | 13 +++++-
 drivers/media/platform/qcom/venus/core.c           | 14 +++++--
 drivers/media/platform/qcom/venus/firmware.c       | 49 ++++++++++++++++++++++
 drivers/media/platform/qcom/venus/firmware.h       |  2 +
 4 files changed, 73 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/qcom,venus.txt b/Documentation/devicetree/bindings/media/qcom,venus.txt
index 00d0d1b..7e04586 100644
--- a/Documentation/devicetree/bindings/media/qcom,venus.txt
+++ b/Documentation/devicetree/bindings/media/qcom,venus.txt
@@ -53,7 +53,7 @@
 
 * Subnodes
 The Venus video-codec node must contain two subnodes representing
-video-decoder and video-encoder.
+video-decoder and video-encoder, and one optional firmware subnode.
 
 Every of video-encoder or video-decoder subnode should have:
 
@@ -79,6 +79,13 @@ Every of video-encoder or video-decoder subnode should have:
 		    power domain which is responsible for collapsing
 		    and restoring power to the subcore.
 
+The firmware subnode must have:
+
+- iommus:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: A list of phandle and IOMMU specifier pairs.
+
 * An Example
 	video-codec@1d00000 {
 		compatible = "qcom,msm8916-venus";
@@ -105,4 +112,8 @@ Every of video-encoder or video-decoder subnode should have:
 			clock-names = "core";
 			power-domains = <&mmcc VENUS_CORE1_GDSC>;
 		};
+
+		video-firmware {
+			iommus = <&apps_iommu 0x10b2 0x0>;
+		};
 	};
diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index 393994e..3bd3b8a 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -284,6 +284,14 @@ static int venus_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto err_runtime_disable;
 
+	ret = of_platform_populate(dev->of_node, NULL, NULL, dev);
+	if (ret)
+		goto err_runtime_disable;
+
+	ret = venus_firmware_init(core);
+	if (ret)
+		goto err_runtime_disable;
+
 	ret = venus_boot(core);
 	if (ret)
 		goto err_runtime_disable;
@@ -308,10 +316,6 @@ static int venus_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_core_deinit;
 
-	ret = of_platform_populate(dev->of_node, NULL, NULL, dev);
-	if (ret)
-		goto err_dev_unregister;
-
 	ret = pm_runtime_put_sync(dev);
 	if (ret)
 		goto err_dev_unregister;
@@ -347,6 +351,8 @@ static int venus_remove(struct platform_device *pdev)
 	venus_shutdown(core);
 	of_platform_depopulate(dev);
 
+	venus_firmware_deinit(core);
+
 	pm_runtime_put_sync(dev);
 	pm_runtime_disable(dev);
 
diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c
index 79b3858..86a26fb 100644
--- a/drivers/media/platform/qcom/venus/firmware.c
+++ b/drivers/media/platform/qcom/venus/firmware.c
@@ -20,6 +20,7 @@
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/platform_device.h>
+#include <linux/of_device.h>
 #include <linux/qcom_scm.h>
 #include <linux/sizes.h>
 #include <linux/soc/qcom/mdt_loader.h>
@@ -228,3 +229,51 @@ int venus_shutdown(struct venus_core *core)
 
 	return ret;
 }
+
+int venus_firmware_init(struct venus_core *core)
+{
+	struct platform_device_info info;
+	struct platform_device *pdev;
+	struct device_node *np;
+	int ret;
+
+	np = of_get_child_by_name(core->dev->of_node, "video-firmware");
+	if (!np)
+		return 0;
+
+	memset(&info, 0, sizeof(info));
+	info.fwnode = &np->fwnode;
+	info.parent = core->dev;
+	info.name = np->name;
+	info.dma_mask = DMA_BIT_MASK(32);
+
+	pdev = platform_device_register_full(&info);
+	if (IS_ERR(pdev)) {
+		of_node_put(np);
+		return PTR_ERR(pdev);
+	}
+
+	pdev->dev.of_node = np;
+
+	ret = of_dma_configure(&pdev->dev, np);
+	if (ret)
+		dev_err(core->dev, "dma configure fail\n");
+
+	of_node_put(np);
+
+	if (ret)
+		return ret;
+
+	core->no_tz = true;
+	core->fw.dev = &pdev->dev;
+
+	return 0;
+}
+
+void venus_firmware_deinit(struct venus_core *core)
+{
+	if (!core->fw.dev)
+		return;
+
+	platform_device_unregister(to_platform_device(core->fw.dev));
+}
diff --git a/drivers/media/platform/qcom/venus/firmware.h b/drivers/media/platform/qcom/venus/firmware.h
index f41b615..119a9a4 100644
--- a/drivers/media/platform/qcom/venus/firmware.h
+++ b/drivers/media/platform/qcom/venus/firmware.h
@@ -16,6 +16,8 @@
 
 struct device;
 
+int venus_firmware_init(struct venus_core *core);
+void venus_firmware_deinit(struct venus_core *core);
 int venus_boot(struct venus_core *core);
 int venus_shutdown(struct venus_core *core);
 int venus_set_hw_state(struct venus_core *core, bool suspend);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v6 4/4] venus: firmware: register separate platform_device for firmware loader

[Stephen Boyd]

Quoting Vikash Garodia (2018-08-23 07:28:48)
> From: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> 
> This registers a firmware platform_device and associate it with
> video-firmware DT subnode. Then calls dma configure to initialize
> dma and iommu.

Yes, but why? Commit text isn't supposed to say what is obvious from the
code.

Re: [PATCH v6 4/4] venus: firmware: register separate platform_device for firmware loader

[Alexandre Courbot]

On Thu, Aug 23, 2018 at 11:29 PM Vikash Garodia <vgarodia@codeaurora.org> wrote:
>
> From: Stanimir Varbanov <stanimir.varbanov@linaro.org>
>
> This registers a firmware platform_device and associate it with
> video-firmware DT subnode. Then calls dma configure to initialize
> dma and iommu.
>
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> ---
>  .../devicetree/bindings/media/qcom,venus.txt       | 13 +++++-
>  drivers/media/platform/qcom/venus/core.c           | 14 +++++--
>  drivers/media/platform/qcom/venus/firmware.c       | 49 ++++++++++++++++++++++
>  drivers/media/platform/qcom/venus/firmware.h       |  2 +
>  4 files changed, 73 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/media/qcom,venus.txt b/Documentation/devicetree/bindings/media/qcom,venus.txt
> index 00d0d1b..7e04586 100644
> --- a/Documentation/devicetree/bindings/media/qcom,venus.txt
> +++ b/Documentation/devicetree/bindings/media/qcom,venus.txt
> @@ -53,7 +53,7 @@
>
>  * Subnodes
>  The Venus video-codec node must contain two subnodes representing
> -video-decoder and video-encoder.
> +video-decoder and video-encoder, and one optional firmware subnode.

Just noticed that the document does not explain in which case the
firmware subnode must be used. Maybe we should have a sentence
explaining that without it we will be using TrustZone to load the
firmware?

>
>  Every of video-encoder or video-decoder subnode should have:
>
> @@ -79,6 +79,13 @@ Every of video-encoder or video-decoder subnode should have:
>                     power domain which is responsible for collapsing
>                     and restoring power to the subcore.
>
> +The firmware subnode must have:
> +
> +- iommus:
> +       Usage: required
> +       Value type: <prop-encoded-array>
> +       Definition: A list of phandle and IOMMU specifier pairs.
> +
>  * An Example
>         video-codec@1d00000 {
>                 compatible = "qcom,msm8916-venus";
> @@ -105,4 +112,8 @@ Every of video-encoder or video-decoder subnode should have:
>                         clock-names = "core";
>                         power-domains = <&mmcc VENUS_CORE1_GDSC>;
>                 };
> +
> +               video-firmware {
> +                       iommus = <&apps_iommu 0x10b2 0x0>;
> +               };
>         };
> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> index 393994e..3bd3b8a 100644
> --- a/drivers/media/platform/qcom/venus/core.c
> +++ b/drivers/media/platform/qcom/venus/core.c
> @@ -284,6 +284,14 @@ static int venus_probe(struct platform_device *pdev)
>         if (ret < 0)
>                 goto err_runtime_disable;
>
> +       ret = of_platform_populate(dev->of_node, NULL, NULL, dev);
> +       if (ret)
> +               goto err_runtime_disable;
> +
> +       ret = venus_firmware_init(core);
> +       if (ret)
> +               goto err_runtime_disable;
> +
>         ret = venus_boot(core);
>         if (ret)
>                 goto err_runtime_disable;
> @@ -308,10 +316,6 @@ static int venus_probe(struct platform_device *pdev)
>         if (ret)
>                 goto err_core_deinit;
>
> -       ret = of_platform_populate(dev->of_node, NULL, NULL, dev);
> -       if (ret)
> -               goto err_dev_unregister;
> -
>         ret = pm_runtime_put_sync(dev);
>         if (ret)
>                 goto err_dev_unregister;
> @@ -347,6 +351,8 @@ static int venus_remove(struct platform_device *pdev)
>         venus_shutdown(core);
>         of_platform_depopulate(dev);
>
> +       venus_firmware_deinit(core);
> +
>         pm_runtime_put_sync(dev);
>         pm_runtime_disable(dev);
>
> diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c
> index 79b3858..86a26fb 100644
> --- a/drivers/media/platform/qcom/venus/firmware.c
> +++ b/drivers/media/platform/qcom/venus/firmware.c
> @@ -20,6 +20,7 @@
>  #include <linux/of.h>
>  #include <linux/of_address.h>
>  #include <linux/platform_device.h>
> +#include <linux/of_device.h>
>  #include <linux/qcom_scm.h>
>  #include <linux/sizes.h>
>  #include <linux/soc/qcom/mdt_loader.h>
> @@ -228,3 +229,51 @@ int venus_shutdown(struct venus_core *core)
>
>         return ret;
>  }
> +
> +int venus_firmware_init(struct venus_core *core)
> +{
> +       struct platform_device_info info;
> +       struct platform_device *pdev;
> +       struct device_node *np;
> +       int ret;
> +
> +       np = of_get_child_by_name(core->dev->of_node, "video-firmware");
> +       if (!np)
> +               return 0;
> +
> +       memset(&info, 0, sizeof(info));
> +       info.fwnode = &np->fwnode;
> +       info.parent = core->dev;
> +       info.name = np->name;
> +       info.dma_mask = DMA_BIT_MASK(32);
> +
> +       pdev = platform_device_register_full(&info);
> +       if (IS_ERR(pdev)) {
> +               of_node_put(np);
> +               return PTR_ERR(pdev);
> +       }
> +
> +       pdev->dev.of_node = np;
> +
> +       ret = of_dma_configure(&pdev->dev, np);
> +       if (ret)
> +               dev_err(core->dev, "dma configure fail\n");
> +
> +       of_node_put(np);
> +
> +       if (ret)
> +               return ret;
> +
> +       core->no_tz = true;
> +       core->fw.dev = &pdev->dev;
> +
> +       return 0;
> +}
> +
> +void venus_firmware_deinit(struct venus_core *core)
> +{
> +       if (!core->fw.dev)
> +               return;
> +
> +       platform_device_unregister(to_platform_device(core->fw.dev));
> +}
> diff --git a/drivers/media/platform/qcom/venus/firmware.h b/drivers/media/platform/qcom/venus/firmware.h
> index f41b615..119a9a4 100644
> --- a/drivers/media/platform/qcom/venus/firmware.h
> +++ b/drivers/media/platform/qcom/venus/firmware.h
> @@ -16,6 +16,8 @@
>
>  struct device;
>
> +int venus_firmware_init(struct venus_core *core);
> +void venus_firmware_deinit(struct venus_core *core);
>  int venus_boot(struct venus_core *core);
>  int venus_shutdown(struct venus_core *core);
>  int venus_set_hw_state(struct venus_core *core, bool suspend);
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>

Re: [PATCH v6 4/4] venus: firmware: register separate platform_device for firmware loader

[Rob Herring]

On Thu, Aug 23, 2018 at 07:58:48PM +0530, Vikash Garodia wrote:
> From: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> 
> This registers a firmware platform_device and associate it with
> video-firmware DT subnode. Then calls dma configure to initialize
> dma and iommu.
> 
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> ---
>  .../devicetree/bindings/media/qcom,venus.txt       | 13 +++++-

In the future, please split binding patches.

Reviewed-by: Rob Herring <robh@kernel.org>

>  drivers/media/platform/qcom/venus/core.c           | 14 +++++--
>  drivers/media/platform/qcom/venus/firmware.c       | 49 ++++++++++++++++++++++
>  drivers/media/platform/qcom/venus/firmware.h       |  2 +
>  4 files changed, 73 insertions(+), 5 deletions(-)

Re: [PATCH v6 4/4] venus: firmware: register separate platform_device for firmware loader

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 7699 bytes --]

Hi Stanimir,

I love your patch! Yet something to improve:

[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on v4.19-rc1 next-20180829]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Vikash-Garodia/Venus-updates-PIL/20180824-023823
base:   git://linuxtv.org/media_tree.git master
config: arm-allmodconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.2.0 make.cross ARCH=arm 

All errors (new ones prefixed by >>):

   drivers/media/platform/qcom/venus/firmware.c: In function 'venus_load_fw':
   drivers/media/platform/qcom/venus/firmware.c:113:9: error: implicit declaration of function 'qcom_mdt_load_no_init'; did you mean 'qcom_mdt_load'? [-Werror=implicit-function-declaration]
      ret = qcom_mdt_load_no_init(dev, mdt, fwname, VENUS_PAS_ID,
            ^~~~~~~~~~~~~~~~~~~~~
            qcom_mdt_load
   drivers/media/platform/qcom/venus/firmware.c: In function 'venus_firmware_init':
>> drivers/media/platform/qcom/venus/firmware.c:258:8: error: too few arguments to function 'of_dma_configure'
     ret = of_dma_configure(&pdev->dev, np);
           ^~~~~~~~~~~~~~~~
   In file included from drivers/media/platform/qcom/venus/firmware.c:23:0:
   include/linux/of_device.h:58:5: note: declared here
    int of_dma_configure(struct device *dev,
        ^~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/of_dma_configure +258 drivers/media/platform/qcom/venus/firmware.c

    65	
    66	static int venus_load_fw(struct venus_core *core, const char *fwname,
    67				 phys_addr_t *mem_phys, size_t *mem_size)
    68	{
    69		const struct firmware *mdt;
    70		struct device_node *node;
    71		struct device *dev;
    72		struct resource r;
    73		ssize_t fw_size;
    74		void *mem_va;
    75		int ret;
    76	
    77		dev = core->dev;
    78		node = of_parse_phandle(dev->of_node, "memory-region", 0);
    79		if (!node) {
    80			dev_err(dev, "no memory-region specified\n");
    81			return -EINVAL;
    82		}
    83	
    84		ret = of_address_to_resource(node, 0, &r);
    85		if (ret)
    86			return ret;
    87	
    88		*mem_phys = r.start;
    89		*mem_size = resource_size(&r);
    90	
    91		if (*mem_size < VENUS_FW_MEM_SIZE)
    92			return -EINVAL;
    93	
    94		mem_va = memremap(r.start, *mem_size, MEMREMAP_WC);
    95		if (!mem_va) {
    96			dev_err(dev, "unable to map memory region: %pa+%zx\n",
    97				&r.start, *mem_size);
    98			return -ENOMEM;
    99		}
   100	
   101		ret = request_firmware(&mdt, fwname, dev);
   102		if (ret < 0)
   103			goto err_unmap;
   104	
   105		fw_size = qcom_mdt_get_size(mdt);
   106		if (fw_size < 0) {
   107			ret = fw_size;
   108			release_firmware(mdt);
   109			goto err_unmap;
   110		}
   111	
   112		if (core->no_tz)
 > 113			ret = qcom_mdt_load_no_init(dev, mdt, fwname, VENUS_PAS_ID,
   114						    mem_va, *mem_phys, *mem_size, NULL);
   115		else
   116			ret = qcom_mdt_load(dev, mdt, fwname, VENUS_PAS_ID,
   117					    mem_va, *mem_phys, *mem_size, NULL);
   118	
   119		release_firmware(mdt);
   120	
   121	err_unmap:
   122		memunmap(mem_va);
   123		return ret;
   124	}
   125	
   126	static int venus_boot_no_tz(struct venus_core *core, phys_addr_t mem_phys,
   127				    size_t mem_size)
   128	{
   129		struct iommu_domain *iommu_dom;
   130		struct device *dev;
   131		int ret;
   132	
   133		dev = core->fw.dev;
   134		if (!dev)
   135			return -EPROBE_DEFER;
   136	
   137		iommu_dom = iommu_domain_alloc(&platform_bus_type);
   138		if (!iommu_dom) {
   139			dev_err(dev, "Failed to allocate iommu domain\n");
   140			return -ENOMEM;
   141		}
   142	
   143		ret = iommu_attach_device(iommu_dom, dev);
   144		if (ret) {
   145			dev_err(dev, "could not attach device\n");
   146			goto err_attach;
   147		}
   148	
   149		ret = iommu_map(iommu_dom, VENUS_FW_START_ADDR, mem_phys, mem_size,
   150				IOMMU_READ | IOMMU_WRITE | IOMMU_PRIV);
   151		if (ret) {
   152			dev_err(dev, "could not map video firmware region\n");
   153			goto err_map;
   154		}
   155	
   156		core->fw.iommu_domain = iommu_dom;
   157		venus_reset_cpu(core);
   158	
   159		return 0;
   160	
   161	err_map:
   162		iommu_detach_device(iommu_dom, dev);
   163	err_attach:
   164		iommu_domain_free(iommu_dom);
   165		return ret;
   166	}
   167	
   168	static int venus_shutdown_no_tz(struct venus_core *core)
   169	{
   170		struct iommu_domain *iommu;
   171		size_t unmapped;
   172		u32 reg;
   173		struct device *dev = core->fw.dev;
   174		void __iomem *base = core->base;
   175	
   176		/* Assert the reset to ARM9 */
   177		reg = readl_relaxed(base + WRAPPER_A9SS_SW_RESET);
   178		reg |= WRAPPER_A9SS_SW_RESET_BIT;
   179		writel_relaxed(reg, base + WRAPPER_A9SS_SW_RESET);
   180	
   181		/* Make sure reset is asserted before the mapping is removed */
   182		mb();
   183	
   184		iommu = core->fw.iommu_domain;
   185	
   186		unmapped = iommu_unmap(iommu, VENUS_FW_START_ADDR, VENUS_FW_MEM_SIZE);
   187		if (unmapped != VENUS_FW_MEM_SIZE)
   188			dev_err(dev, "failed to unmap firmware\n");
   189	
   190		iommu_detach_device(iommu, dev);
   191		iommu_domain_free(iommu);
   192	
   193		return 0;
   194	}
   195	
   196	int venus_boot(struct venus_core *core)
   197	{
   198		struct device *dev = core->dev;
   199		phys_addr_t mem_phys;
   200		size_t mem_size;
   201		int ret;
   202	
   203		if (!IS_ENABLED(CONFIG_QCOM_MDT_LOADER) ||
   204		    (!core->no_tz && !qcom_scm_is_available()))
   205			return -EPROBE_DEFER;
   206	
   207		ret = venus_load_fw(core, core->res->fwname, &mem_phys, &mem_size);
   208		if (ret) {
   209			dev_err(dev, "fail to load video firmware\n");
   210			return -EINVAL;
   211		}
   212	
   213		if (core->no_tz)
   214			ret = venus_boot_no_tz(core, mem_phys, mem_size);
   215		else
   216			ret = qcom_scm_pas_auth_and_reset(VENUS_PAS_ID);
   217	
   218		return ret;
   219	}
   220	
   221	int venus_shutdown(struct venus_core *core)
   222	{
   223		int ret;
   224	
   225		if (core->no_tz)
   226			ret = venus_shutdown_no_tz(core);
   227		else
   228			ret = qcom_scm_pas_shutdown(VENUS_PAS_ID);
   229	
   230		return ret;
   231	}
   232	
   233	int venus_firmware_init(struct venus_core *core)
   234	{
   235		struct platform_device_info info;
   236		struct platform_device *pdev;
   237		struct device_node *np;
   238		int ret;
   239	
   240		np = of_get_child_by_name(core->dev->of_node, "video-firmware");
   241		if (!np)
   242			return 0;
   243	
   244		memset(&info, 0, sizeof(info));
   245		info.fwnode = &np->fwnode;
   246		info.parent = core->dev;
   247		info.name = np->name;
   248		info.dma_mask = DMA_BIT_MASK(32);
   249	
   250		pdev = platform_device_register_full(&info);
   251		if (IS_ERR(pdev)) {
   252			of_node_put(np);
   253			return PTR_ERR(pdev);
   254		}
   255	
   256		pdev->dev.of_node = np;
   257	
 > 258		ret = of_dma_configure(&pdev->dev, np);
   259		if (ret)
   260			dev_err(core->dev, "dma configure fail\n");
   261	
   262		of_node_put(np);
   263	
   264		if (ret)
   265			return ret;
   266	
   267		core->no_tz = true;
   268		core->fw.dev = &pdev->dev;
   269	
   270		return 0;
   271	}
   272	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 66120 bytes --]

Re: [PATCH v6 0/4] Venus updates - PIL

[Alexandre Courbot]

Hi Vikash,

On Thu, Aug 23, 2018 at 11:29 PM Vikash Garodia <vgarodia@codeaurora.org> wrote:
>
> Hello,
>
> Here is v6 with following comments addressed:
>
> * 4/4 from earlier series was dropped as .probe was not needed.
> * indentation as per checkpatch --strict option.
> * tested on Venus v4 hardware.

I have tested this series and it seems to be working fine! Thanks for
pushing it forward!

I have made a few comments inline, but some may be difficult to apply
without reorganizing the series a bit. If my explanations are not
clear, I can take care of submitting the next spin of this series if
you wish.