[PATCH v2 0/5] Venus updates - PIL

[PATCH v2 0/5] Venus updates - PIL

[Vikash Garodia]

Hello,

Here is v2 with following comments addressed:

* drop 1/4 patch from v1 and use relevant api to load
  firmware without PAS.
* add some details on ARM9 role in video hardware.
* abstract scm calls to set hardware state.
* remove setting aperture range for firmware and vcodec
  context banks.
* add misc code review comments related to return
  handling, unwanted cast, etc.

Comments are welcome!

Vikash Garodia (5):
  media: venus: add a routine to reset ARM9
  media: venus: add a routine to set venus state
  venus: add check to make scm calls
  media: venus: add no TZ boot and shutdown routine
  venus: register separate driver for firmware device

 .../devicetree/bindings/media/qcom,venus.txt       |   8 +-
 drivers/media/platform/qcom/venus/core.c           |  58 +++++-
 drivers/media/platform/qcom/venus/core.h           |  10 +
 drivers/media/platform/qcom/venus/firmware.c       | 217 ++++++++++++++++++---
 drivers/media/platform/qcom/venus/firmware.h       |  12 +-
 drivers/media/platform/qcom/venus/hfi_venus.c      |  13 +-
 drivers/media/platform/qcom/venus/hfi_venus_io.h   |   5 +
 7 files changed, 275 insertions(+), 48 deletions(-)

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

[PATCH v2 1/5] media: venus: add a routine to reset ARM9

[Vikash Garodia]

Add a new routine to reset the ARM9 and brings it
out of reset. This is in preparation to add PIL
functionality in venus driver.

Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
---
 drivers/media/platform/qcom/venus/firmware.c     | 26 ++++++++++++++++++++++++
 drivers/media/platform/qcom/venus/hfi_venus_io.h |  5 +++++
 2 files changed, 31 insertions(+)

diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c
index 521d4b3..7d89b5a 100644
--- a/drivers/media/platform/qcom/venus/firmware.c
+++ b/drivers/media/platform/qcom/venus/firmware.c
@@ -14,6 +14,7 @@
 
 #include <linux/device.h>
 #include <linux/firmware.h>
+#include <linux/delay.h>
 #include <linux/kernel.h>
 #include <linux/io.h>
 #include <linux/of.h>
@@ -22,11 +23,36 @@
 #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)
 
+static void venus_reset_hw(struct venus_core *core)
+{
+	void __iomem *reg_base = core->base;
+
+	writel(0, reg_base + WRAPPER_FW_START_ADDR);
+	writel(VENUS_FW_MEM_SIZE, reg_base + WRAPPER_FW_END_ADDR);
+	writel(0, reg_base + WRAPPER_CPA_START_ADDR);
+	writel(VENUS_FW_MEM_SIZE, reg_base + WRAPPER_CPA_END_ADDR);
+	writel(0x0, reg_base + WRAPPER_CPU_CGC_DIS);
+	writel(0x0, reg_base + WRAPPER_CPU_CLOCK_CONFIG);
+
+	/* Make sure all register writes are committed. */
+	mb();
+
+	/*
+	 * Need to wait 10 cycles of internal clocks before bringing ARM9
+	 * out of reset.
+	 */
+	udelay(1);
+
+	/* Bring Arm9 out of reset */
+	writel_relaxed(0, reg_base + WRAPPER_A9SS_SW_RESET);
+}
 int venus_boot(struct device *dev, const char *fwname)
 {
 	const struct firmware *mdt;
diff --git a/drivers/media/platform/qcom/venus/hfi_venus_io.h b/drivers/media/platform/qcom/venus/hfi_venus_io.h
index 76f4793..39afa5d 100644
--- a/drivers/media/platform/qcom/venus/hfi_venus_io.h
+++ b/drivers/media/platform/qcom/venus/hfi_venus_io.h
@@ -109,6 +109,11 @@
 #define WRAPPER_CPU_CGC_DIS			(WRAPPER_BASE + 0x2010)
 #define WRAPPER_CPU_STATUS			(WRAPPER_BASE + 0x2014)
 #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_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 v2 1/5] media: venus: add a routine to reset ARM9

[Stanimir Varbanov]

Hi Vikash,

On  1.06.2018 23:26, Vikash Garodia wrote:
> Add a new routine to reset the ARM9 and brings it
> out of reset. This is in preparation to add PIL
> functionality in venus driver.

please squash this patch with 4/5. I don't see a reason to add a 
function which is not used. Shouldn't this produce gcc warnings?

> 
> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
> ---
>   drivers/media/platform/qcom/venus/firmware.c     | 26 ++++++++++++++++++++++++
>   drivers/media/platform/qcom/venus/hfi_venus_io.h |  5 +++++
>   2 files changed, 31 insertions(+)
> 
> diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c
> index 521d4b3..7d89b5a 100644
> --- a/drivers/media/platform/qcom/venus/firmware.c
> +++ b/drivers/media/platform/qcom/venus/firmware.c
> @@ -14,6 +14,7 @@
>   
>   #include <linux/device.h>
>   #include <linux/firmware.h>
> +#include <linux/delay.h>
>   #include <linux/kernel.h>
>   #include <linux/io.h>
>   #include <linux/of.h>
> @@ -22,11 +23,36 @@
>   #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)
>   
> +static void venus_reset_hw(struct venus_core *core)

can we rename this to venus_reset_cpu? reset_hw sounds like we reset 
vcodec IPs, so I think we can be more exact here.

> +{
> +	void __iomem *reg_base = core->base;

just 'base', please.

> +
> +	writel(0, reg_base + WRAPPER_FW_START_ADDR);
> +	writel(VENUS_FW_MEM_SIZE, reg_base + WRAPPER_FW_END_ADDR);
> +	writel(0, reg_base + WRAPPER_CPA_START_ADDR);
> +	writel(VENUS_FW_MEM_SIZE, reg_base + WRAPPER_CPA_END_ADDR);
> +	writel(0x0, reg_base + WRAPPER_CPU_CGC_DIS);
> +	writel(0x0, reg_base + WRAPPER_CPU_CLOCK_CONFIG);
> +
> +	/* Make sure all register writes are committed. */
> +	mb();

the comment says "register writes" hence shouldn't this be wmb? Also if 
we are going to have explicit memory barrier why not use writel_relaxed?

> +
> +	/*
> +	 * Need to wait 10 cycles of internal clocks before bringing ARM9

Do we know what is the minimum frequency of the internal ARM9 clocks? 
I.e does 1us is enough for all venus versions.

> +	 * out of reset.
> +	 */
> +	udelay(1);
> +
> +	/* Bring Arm9 out of reset */

ARM9

> +	writel_relaxed(0, reg_base + WRAPPER_A9SS_SW_RESET);

in my opinion we should have a wmb here too

regards,
Stan

Re: [PATCH v2 1/5] media: venus: add a routine to reset ARM9

[Vinod]

On 02-06-18, 01:15, Stanimir Varbanov wrote:
> Hi Vikash,
> 
> On  1.06.2018 23:26, Vikash Garodia wrote:
> > Add a new routine to reset the ARM9 and brings it
> > out of reset. This is in preparation to add PIL
> > functionality in venus driver.
> 
> please squash this patch with 4/5. I don't see a reason to add a function
> which is not used. Shouldn't this produce gcc warnings?

Yes this would but in a multi patch series that is okay as subsequent
patches would use that and end result in no warning.

Splitting logically is good and typical practice in kernel to add the
routine followed by usages..

-- 
~Vinod

Re: [PATCH v2 1/5] media: venus: add a routine to reset ARM9

[Alexandre Courbot]

On Tue, Jun 5, 2018 at 7:57 PM Vinod <vkoul@kernel.org> wrote:
>
> On 02-06-18, 01:15, Stanimir Varbanov wrote:
> > Hi Vikash,
> >
> > On  1.06.2018 23:26, Vikash Garodia wrote:
> > > Add a new routine to reset the ARM9 and brings it
> > > out of reset. This is in preparation to add PIL
> > > functionality in venus driver.
> >
> > please squash this patch with 4/5. I don't see a reason to add a function
> > which is not used. Shouldn't this produce gcc warnings?
>
> Yes this would but in a multi patch series that is okay as subsequent
> patches would use that and end result in no warning.

Except during bisect.

>
> Splitting logically is good and typical practice in kernel to add the
> routine followed by usages..

Is it in this case though? If this code was shared across multiple
users I could understand but this function is only used locally (and
only in one place IIUC). Also the patch is not so big that the code
would become confusing if this was squashed into 4/5. I don't see any
reason to keep this separate.

Re: [PATCH v2 1/5] media: venus: add a routine to reset ARM9

[Vikash Garodia]

Hi Stanimir,

Thanks for the review.

On 2018-06-02 03:45, Stanimir Varbanov wrote:
> Hi Vikash,
> 
> On  1.06.2018 23:26, Vikash Garodia wrote:
>> Add a new routine to reset the ARM9 and brings it
>> out of reset. This is in preparation to add PIL
>> functionality in venus driver.
> 
> please squash this patch with 4/5. I don't see a reason to add a
> function which is not used. Shouldn't this produce gcc warnings?

Will squash the api definition with the patch using it.

>> 
>> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
>> ---
>>   drivers/media/platform/qcom/venus/firmware.c     | 26 
>> ++++++++++++++++++++++++
>>   drivers/media/platform/qcom/venus/hfi_venus_io.h |  5 +++++
>>   2 files changed, 31 insertions(+)
>> 
>> diff --git a/drivers/media/platform/qcom/venus/firmware.c 
>> b/drivers/media/platform/qcom/venus/firmware.c
>> index 521d4b3..7d89b5a 100644
>> --- a/drivers/media/platform/qcom/venus/firmware.c
>> +++ b/drivers/media/platform/qcom/venus/firmware.c
>> @@ -14,6 +14,7 @@
>>     #include <linux/device.h>
>>   #include <linux/firmware.h>
>> +#include <linux/delay.h>
>>   #include <linux/kernel.h>
>>   #include <linux/io.h>
>>   #include <linux/of.h>
>> @@ -22,11 +23,36 @@
>>   #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)
>>   +static void venus_reset_hw(struct venus_core *core)
> 
> can we rename this to venus_reset_cpu? reset_hw sounds like we reset
> vcodec IPs, so I think we can be more exact here.
> 
>> +{
>> +	void __iomem *reg_base = core->base;
> 
> just 'base', please.
Ok.

>> +
>> +	writel(0, reg_base + WRAPPER_FW_START_ADDR);
>> +	writel(VENUS_FW_MEM_SIZE, reg_base + WRAPPER_FW_END_ADDR);
>> +	writel(0, reg_base + WRAPPER_CPA_START_ADDR);
>> +	writel(VENUS_FW_MEM_SIZE, reg_base + WRAPPER_CPA_END_ADDR);
>> +	writel(0x0, reg_base + WRAPPER_CPU_CGC_DIS);
>> +	writel(0x0, reg_base + WRAPPER_CPU_CLOCK_CONFIG);
>> +
>> +	/* Make sure all register writes are committed. */
>> +	mb();
> 
> the comment says "register writes" hence shouldn't this be wmb? Also
> if we are going to have explicit memory barrier why not use
> writel_relaxed?
>> +
>> +	/*
>> +	 * Need to wait 10 cycles of internal clocks before bringing ARM9
> 
> Do we know what is the minimum frequency of the internal ARM9 clocks?
> I.e does 1us is enough for all venus versions.

ARM9 is yet not brought out of reset at this point. Sleep might be added
to ensure that the register writes are completed. But as Bjorn 
commented,
only way to ensure registers are written is to read them back. I do not 
have
proper justification of sleep as this code was used for many chipset 
bringup.
I can try by removing the sleep and if goes well, we can live without 
it.

>> +	 * out of reset.
>> +	 */
>> +	udelay(1);
>> +
>> +	/* Bring Arm9 out of reset */
> 
> ARM9
> 
>> +	writel_relaxed(0, reg_base + WRAPPER_A9SS_SW_RESET);
> 
> in my opinion we should have a wmb here too
I guess if sleep is removed, we will be left with only the above 
instruction.
so no need to ensure the access order.

> regards,
> Stan

Re: [PATCH v2 1/5] media: venus: add a routine to reset ARM9

[Bjorn Andersson]

On Fri 01 Jun 13:26 PDT 2018, Vikash Garodia wrote:
> +static void venus_reset_hw(struct venus_core *core)
> +{
> +	void __iomem *reg_base = core->base;
> +
> +	writel(0, reg_base + WRAPPER_FW_START_ADDR);
> +	writel(VENUS_FW_MEM_SIZE, reg_base + WRAPPER_FW_END_ADDR);
> +	writel(0, reg_base + WRAPPER_CPA_START_ADDR);
> +	writel(VENUS_FW_MEM_SIZE, reg_base + WRAPPER_CPA_END_ADDR);
> +	writel(0x0, reg_base + WRAPPER_CPU_CGC_DIS);
> +	writel(0x0, reg_base + WRAPPER_CPU_CLOCK_CONFIG);
> +
> +	/* Make sure all register writes are committed. */
> +	mb();

wmb() doesn't wait until the writes are completed, it simply ensures
that any writes before it are performed before any writes after it.

If you really want to ensure that these configs has hit the hardware
before you sleep, read back the value of the WRAPPER_CPU_CLOCK_CONFIG
register.

> +
> +	/*
> +	 * Need to wait 10 cycles of internal clocks before bringing ARM9
> +	 * out of reset.
> +	 */
> +	udelay(1);
> +
> +	/* Bring Arm9 out of reset */
> +	writel_relaxed(0, reg_base + WRAPPER_A9SS_SW_RESET);

There's no harm in using writel() here...

> +}

Regards,
Bjorn

[PATCH v2 2/5] media: venus: add a routine to set venus state

[Vikash Garodia]

Add a new routine which abstracts the TZ call to
set the video hardware state.

Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
---
 drivers/media/platform/qcom/venus/core.h      |  5 +++++
 drivers/media/platform/qcom/venus/firmware.c  | 28 +++++++++++++++++++++++++++
 drivers/media/platform/qcom/venus/firmware.h  |  1 +
 drivers/media/platform/qcom/venus/hfi_venus.c | 13 ++++---------
 4 files changed, 38 insertions(+), 9 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
index 85e66e2..e7bfb63 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -35,6 +35,11 @@ struct reg_val {
 	u32 value;
 };
 
+enum tzbsp_video_state {
+	TZBSP_VIDEO_SUSPEND = 0,
+	TZBSP_VIDEO_RESUME
+};
+
 struct venus_resources {
 	u64 dma_mask;
 	const struct freq_tbl *freq_tbl;
diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c
index 7d89b5a..b4664ed 100644
--- a/drivers/media/platform/qcom/venus/firmware.c
+++ b/drivers/media/platform/qcom/venus/firmware.c
@@ -53,6 +53,34 @@ static void venus_reset_hw(struct venus_core *core)
 	/* Bring Arm9 out of reset */
 	writel_relaxed(0, reg_base + WRAPPER_A9SS_SW_RESET);
 }
+
+int venus_set_hw_state(enum tzbsp_video_state state, struct venus_core *core)
+{
+	int ret;
+	struct device *dev = core->dev;
+	void __iomem *reg_base = core->base;
+
+	switch (state) {
+	case TZBSP_VIDEO_SUSPEND:
+		if (qcom_scm_is_available())
+			ret = qcom_scm_set_remote_state(TZBSP_VIDEO_SUSPEND, 0);
+		else
+			writel_relaxed(1, reg_base + WRAPPER_A9SS_SW_RESET);
+		break;
+	case TZBSP_VIDEO_RESUME:
+		if (qcom_scm_is_available())
+			ret = qcom_scm_set_remote_state(TZBSP_VIDEO_RESUME, 0);
+		else
+			venus_reset_hw(core);
+		break;
+	default:
+		dev_err(dev, "invalid state\n");
+		break;
+	}
+	return ret;
+}
+EXPORT_SYMBOL_GPL(venus_set_hw_state);
+
 int venus_boot(struct device *dev, const char *fwname)
 {
 	const struct firmware *mdt;
diff --git a/drivers/media/platform/qcom/venus/firmware.h b/drivers/media/platform/qcom/venus/firmware.h
index 428efb5..1336729 100644
--- a/drivers/media/platform/qcom/venus/firmware.h
+++ b/drivers/media/platform/qcom/venus/firmware.h
@@ -18,5 +18,6 @@
 
 int venus_boot(struct device *dev, const char *fwname);
 int venus_shutdown(struct device *dev);
+int venus_set_hw_state(enum tzbsp_video_state, struct venus_core *core);
 
 #endif
diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
index f61d34b..797a9f5 100644
--- a/drivers/media/platform/qcom/venus/hfi_venus.c
+++ b/drivers/media/platform/qcom/venus/hfi_venus.c
@@ -19,7 +19,6 @@
 #include <linux/interrupt.h>
 #include <linux/iopoll.h>
 #include <linux/kernel.h>
-#include <linux/qcom_scm.h>
 #include <linux/slab.h>
 
 #include "core.h"
@@ -27,6 +26,7 @@
 #include "hfi_msgs.h"
 #include "hfi_venus.h"
 #include "hfi_venus_io.h"
+#include "firmware.h"
 
 #define HFI_MASK_QHDR_TX_TYPE		0xff000000
 #define HFI_MASK_QHDR_RX_TYPE		0x00ff0000
@@ -55,11 +55,6 @@
 #define IFACEQ_VAR_LARGE_PKT_SIZE	512
 #define IFACEQ_VAR_HUGE_PKT_SIZE	(1024 * 12)
 
-enum tzbsp_video_state {
-	TZBSP_VIDEO_STATE_SUSPEND = 0,
-	TZBSP_VIDEO_STATE_RESUME
-};
-
 struct hfi_queue_table_header {
 	u32 version;
 	u32 size;
@@ -574,7 +569,7 @@ static int venus_power_off(struct venus_hfi_device *hdev)
 	if (!hdev->power_enabled)
 		return 0;
 
-	ret = qcom_scm_set_remote_state(TZBSP_VIDEO_STATE_SUSPEND, 0);
+	ret = venus_set_hw_state(TZBSP_VIDEO_SUSPEND, hdev->core);
 	if (ret)
 		return ret;
 
@@ -594,7 +589,7 @@ static int venus_power_on(struct venus_hfi_device *hdev)
 	if (hdev->power_enabled)
 		return 0;
 
-	ret = qcom_scm_set_remote_state(TZBSP_VIDEO_STATE_RESUME, 0);
+	ret = venus_set_hw_state(TZBSP_VIDEO_RESUME, hdev->core);
 	if (ret)
 		goto err;
 
@@ -607,7 +602,7 @@ static int venus_power_on(struct venus_hfi_device *hdev)
 	return 0;
 
 err_suspend:
-	qcom_scm_set_remote_state(TZBSP_VIDEO_STATE_SUSPEND, 0);
+	venus_set_hw_state(TZBSP_VIDEO_SUSPEND, hdev->core);
 err:
 	hdev->power_enabled = false;
 	return ret;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2 2/5] media: venus: add a routine to set venus state

[Jordan Crouse]

On Sat, Jun 02, 2018 at 01:56:05AM +0530, Vikash Garodia wrote:
> Add a new routine which abstracts the TZ call to
> set the video hardware state.
> 
> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
> ---
>  drivers/media/platform/qcom/venus/core.h      |  5 +++++
>  drivers/media/platform/qcom/venus/firmware.c  | 28 +++++++++++++++++++++++++++
>  drivers/media/platform/qcom/venus/firmware.h  |  1 +
>  drivers/media/platform/qcom/venus/hfi_venus.c | 13 ++++---------
>  4 files changed, 38 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
> index 85e66e2..e7bfb63 100644
> --- a/drivers/media/platform/qcom/venus/core.h
> +++ b/drivers/media/platform/qcom/venus/core.h
> @@ -35,6 +35,11 @@ struct reg_val {
>  	u32 value;
>  };
>  
> +enum tzbsp_video_state {
> +	TZBSP_VIDEO_SUSPEND = 0,
> +	TZBSP_VIDEO_RESUME

You should put a comma after the last item to reduce future patch sizes.

> +};
> +
>  struct venus_resources {
>  	u64 dma_mask;
>  	const struct freq_tbl *freq_tbl;
> diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c
> index 7d89b5a..b4664ed 100644
> --- a/drivers/media/platform/qcom/venus/firmware.c
> +++ b/drivers/media/platform/qcom/venus/firmware.c
> @@ -53,6 +53,34 @@ static void venus_reset_hw(struct venus_core *core)
>  	/* Bring Arm9 out of reset */
>  	writel_relaxed(0, reg_base + WRAPPER_A9SS_SW_RESET);
>  }
> +
> +int venus_set_hw_state(enum tzbsp_video_state state, struct venus_core *core)
> +{
> +	int ret;
> +	struct device *dev = core->dev;

If you get rid of the log message as you should, you don't need this.

> +	void __iomem *reg_base = core->base;
> +
> +	switch (state) {
> +	case TZBSP_VIDEO_SUSPEND:
> +		if (qcom_scm_is_available())
> +			ret = qcom_scm_set_remote_state(TZBSP_VIDEO_SUSPEND, 0);
> +		else
> +			writel_relaxed(1, reg_base + WRAPPER_A9SS_SW_RESET);

You can just use core->base here and not bother making a local variable for it.

> +		break;
> +	case TZBSP_VIDEO_RESUME:
> +		if (qcom_scm_is_available())
> +			ret = qcom_scm_set_remote_state(TZBSP_VIDEO_RESUME, 0);
> +		else
> +			venus_reset_hw(core);
> +		break;
> +	default:
> +		dev_err(dev, "invalid state\n");

state is a enum - you are highly unlikely to be calling it in your own code with
a random value.  It is smart to have the default, but you don't need the log
message - that is just wasted space in the binary.

> +		break;
> +	}

There are three paths in the switch statement that could end up with 'ret' being
uninitialized here.  Set it to 0 when you declare it.

> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(venus_set_hw_state);
> +
>  int venus_boot(struct device *dev, const char *fwname)
>  {
>  	const struct firmware *mdt;
> diff --git a/drivers/media/platform/qcom/venus/firmware.h b/drivers/media/platform/qcom/venus/firmware.h
> index 428efb5..1336729 100644
> --- a/drivers/media/platform/qcom/venus/firmware.h
> +++ b/drivers/media/platform/qcom/venus/firmware.h
> @@ -18,5 +18,6 @@
>  
>  int venus_boot(struct device *dev, const char *fwname);
>  int venus_shutdown(struct device *dev);
> +int venus_set_hw_state(enum tzbsp_video_state, struct venus_core *core);
>  
>  #endif
> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
> index f61d34b..797a9f5 100644
> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
> @@ -19,7 +19,6 @@
>  #include <linux/interrupt.h>
>  #include <linux/iopoll.h>
>  #include <linux/kernel.h>
> -#include <linux/qcom_scm.h>
>  #include <linux/slab.h>
>  
>  #include "core.h"
> @@ -27,6 +26,7 @@
>  #include "hfi_msgs.h"
>  #include "hfi_venus.h"
>  #include "hfi_venus_io.h"
> +#include "firmware.h"
>  
>  #define HFI_MASK_QHDR_TX_TYPE		0xff000000
>  #define HFI_MASK_QHDR_RX_TYPE		0x00ff0000
> @@ -55,11 +55,6 @@
>  #define IFACEQ_VAR_LARGE_PKT_SIZE	512
>  #define IFACEQ_VAR_HUGE_PKT_SIZE	(1024 * 12)
>  
> -enum tzbsp_video_state {
> -	TZBSP_VIDEO_STATE_SUSPEND = 0,
> -	TZBSP_VIDEO_STATE_RESUME
> -};
> -
>  struct hfi_queue_table_header {
>  	u32 version;
>  	u32 size;
> @@ -574,7 +569,7 @@ static int venus_power_off(struct venus_hfi_device *hdev)
>  	if (!hdev->power_enabled)
>  		return 0;
>  
> -	ret = qcom_scm_set_remote_state(TZBSP_VIDEO_STATE_SUSPEND, 0);
> +	ret = venus_set_hw_state(TZBSP_VIDEO_SUSPEND, hdev->core);
>  	if (ret)
>  		return ret;
>  
> @@ -594,7 +589,7 @@ static int venus_power_on(struct venus_hfi_device *hdev)
>  	if (hdev->power_enabled)
>  		return 0;
>  
> -	ret = qcom_scm_set_remote_state(TZBSP_VIDEO_STATE_RESUME, 0);
> +	ret = venus_set_hw_state(TZBSP_VIDEO_RESUME, hdev->core);
>  	if (ret)
>  		goto err;
>  
> @@ -607,7 +602,7 @@ static int venus_power_on(struct venus_hfi_device *hdev)
>  	return 0;
>  
>  err_suspend:
> -	qcom_scm_set_remote_state(TZBSP_VIDEO_STATE_SUSPEND, 0);
> +	venus_set_hw_state(TZBSP_VIDEO_SUSPEND, hdev->core);
>  err:
>  	hdev->power_enabled = false;
>  	return ret;

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

Re: [PATCH v2 2/5] media: venus: add a routine to set venus state

[Tomasz Figa]

Hi Jordan, Vikash,

On Sat, Jun 2, 2018 at 6:21 AM Jordan Crouse <jcrouse@codeaurora.org> wrote:
>
> On Sat, Jun 02, 2018 at 01:56:05AM +0530, Vikash Garodia wrote:
[snip]
> > +int venus_set_hw_state(enum tzbsp_video_state state, struct venus_core *core)
> > +{
> > +     int ret;
> > +     struct device *dev = core->dev;
>
> If you get rid of the log message as you should, you don't need this.
>
> > +     void __iomem *reg_base = core->base;
> > +
> > +     switch (state) {
> > +     case TZBSP_VIDEO_SUSPEND:
> > +             if (qcom_scm_is_available())
> > +                     ret = qcom_scm_set_remote_state(TZBSP_VIDEO_SUSPEND, 0);
> > +             else
> > +                     writel_relaxed(1, reg_base + WRAPPER_A9SS_SW_RESET);
>
> You can just use core->base here and not bother making a local variable for it.
>
> > +             break;
> > +     case TZBSP_VIDEO_RESUME:
> > +             if (qcom_scm_is_available())
> > +                     ret = qcom_scm_set_remote_state(TZBSP_VIDEO_RESUME, 0);
> > +             else
> > +                     venus_reset_hw(core);
> > +             break;
> > +     default:
> > +             dev_err(dev, "invalid state\n");
>
> state is a enum - you are highly unlikely to be calling it in your own code with
> a random value.  It is smart to have the default, but you don't need the log
> message - that is just wasted space in the binary.
>
> > +             break;
> > +     }
>
> There are three paths in the switch statement that could end up with 'ret' being
> uninitialized here.  Set it to 0 when you declare it.

Does this actually compile? The compiler should detect that ret is
used uninitialized. Setting it to 0 at declaration time actually
prevents compiler from doing that and makes it impossible to catch
cases when the ret should actually be non-zero, e.g. the invalid enum
value case.

Given that this function is supposed to substitute existing calls into
qcom_scm_set_remote_state(), why not just do something like this:

        if (qcom_scm_is_available())
                return qcom_scm_set_remote_state(state, 0);

        switch (state) {
        case TZBSP_VIDEO_SUSPEND:
                writel_relaxed(1, reg_base + WRAPPER_A9SS_SW_RESET);
                break;
        case TZBSP_VIDEO_RESUME:
                venus_reset_hw(core);
                break;
        }

        return 0;

Best regards,
Tomasz

Re: [PATCH v2 2/5] media: venus: add a routine to set venus state

[Vikash Garodia]

Hi Jordan, Tomasz,

Thanks for your valuable review comments.

On 2018-06-04 18:24, Tomasz Figa wrote:
> Hi Jordan, Vikash,
> 
> On Sat, Jun 2, 2018 at 6:21 AM Jordan Crouse <jcrouse@codeaurora.org> 
> wrote:
>> 
>> On Sat, Jun 02, 2018 at 01:56:05AM +0530, Vikash Garodia wrote:
> [snip]
>> > +int venus_set_hw_state(enum tzbsp_video_state state, struct venus_core *core)
>> > +{
>> > +     int ret;
>> > +     struct device *dev = core->dev;
>> 
>> If you get rid of the log message as you should, you don't need this.

Would prefer to keep the log for cases when enum is expanded while the 
switch does
not handle it.

>> > +     void __iomem *reg_base = core->base;
>> > +
>> > +     switch (state) {
>> > +     case TZBSP_VIDEO_SUSPEND:
>> > +             if (qcom_scm_is_available())
>> > +                     ret = qcom_scm_set_remote_state(TZBSP_VIDEO_SUSPEND, 0);
>> > +             else
>> > +                     writel_relaxed(1, reg_base + WRAPPER_A9SS_SW_RESET);
>> 
>> You can just use core->base here and not bother making a local 
>> variable for it.
Ok.

>> > +             break;
>> > +     case TZBSP_VIDEO_RESUME:
>> > +             if (qcom_scm_is_available())
>> > +                     ret = qcom_scm_set_remote_state(TZBSP_VIDEO_RESUME, 0);
>> > +             else
>> > +                     venus_reset_hw(core);
>> > +             break;
>> > +     default:
>> > +             dev_err(dev, "invalid state\n");
>> 
>> state is a enum - you are highly unlikely to be calling it in your own 
>> code with
>> a random value.  It is smart to have the default, but you don't need 
>> the log
>> message - that is just wasted space in the binary.

Incase enum is expanded while the switch does not handle it, default 
will be useful.

>> > +             break;
>> > +     }
>> 
>> There are three paths in the switch statement that could end up with 
>> 'ret' being
>> uninitialized here.  Set it to 0 when you declare it.

> Does this actually compile? The compiler should detect that ret is
> used uninitialized. Setting it to 0 at declaration time actually
> prevents compiler from doing that and makes it impossible to catch
> cases when the ret should actually be non-zero, e.g. the invalid enum
> value case.

It does compile while it gave me failure while doing the functional 
validation.
I have fixed it in next series of patch.

> Given that this function is supposed to substitute existing calls into
> qcom_scm_set_remote_state(), why not just do something like this:
> 
>         if (qcom_scm_is_available())
>                 return qcom_scm_set_remote_state(state, 0);
> 
>         switch (state) {
>         case TZBSP_VIDEO_SUSPEND:
>                 writel_relaxed(1, reg_base + WRAPPER_A9SS_SW_RESET);
>                 break;
>         case TZBSP_VIDEO_RESUME:
>                 venus_reset_hw(core);
>                 break;
>         }
> 
>         return 0;
This will not work as driver will write on the register irrespective of 
scm
availability.

> Best regards,
> Tomasz

Re: [PATCH v2 2/5] media: venus: add a routine to set venus state

[Tomasz Figa]

On Wed, Jul 4, 2018 at 4:59 PM Vikash Garodia <vgarodia@codeaurora.org> wrote:
> On 2018-06-04 18:24, Tomasz Figa wrote:
> > On Sat, Jun 2, 2018 at 6:21 AM Jordan Crouse <jcrouse@codeaurora.org>
> > wrote:
> >> On Sat, Jun 02, 2018 at 01:56:05AM +0530, Vikash Garodia wrote:
> > Given that this function is supposed to substitute existing calls into
> > qcom_scm_set_remote_state(), why not just do something like this:
> >
> >         if (qcom_scm_is_available())
> >                 return qcom_scm_set_remote_state(state, 0);
> >
> >         switch (state) {
> >         case TZBSP_VIDEO_SUSPEND:
> >                 writel_relaxed(1, reg_base + WRAPPER_A9SS_SW_RESET);
> >                 break;
> >         case TZBSP_VIDEO_RESUME:
> >                 venus_reset_hw(core);
> >                 break;
> >         }
> >
> >         return 0;
> This will not work as driver will write on the register irrespective of
> scm
> availability.

I'm sorry, where would it do so? The second line returns from the
function inf SCM is available, so the rest of the function wouldn't be
executed.

Best regards,
Tomasz

Re: [PATCH v2 2/5] media: venus: add a routine to set venus state

[Vikash Garodia]

On 2018-07-04 14:30, Tomasz Figa wrote:
> On Wed, Jul 4, 2018 at 4:59 PM Vikash Garodia <vgarodia@codeaurora.org> 
> wrote:
>> On 2018-06-04 18:24, Tomasz Figa wrote:
>> > On Sat, Jun 2, 2018 at 6:21 AM Jordan Crouse <jcrouse@codeaurora.org>
>> > wrote:
>> >> On Sat, Jun 02, 2018 at 01:56:05AM +0530, Vikash Garodia wrote:
>> > Given that this function is supposed to substitute existing calls into
>> > qcom_scm_set_remote_state(), why not just do something like this:
>> >
>> >         if (qcom_scm_is_available())
>> >                 return qcom_scm_set_remote_state(state, 0);
>> >
>> >         switch (state) {
>> >         case TZBSP_VIDEO_SUSPEND:
>> >                 writel_relaxed(1, reg_base + WRAPPER_A9SS_SW_RESET);
>> >                 break;
>> >         case TZBSP_VIDEO_RESUME:
>> >                 venus_reset_hw(core);
>> >                 break;
>> >         }
>> >
>> >         return 0;
>> This will not work as driver will write on the register irrespective 
>> of
>> scm
>> availability.
> 
> I'm sorry, where would it do so? The second line returns from the
> function inf SCM is available, so the rest of the function wouldn't be
> executed.

Ah!! you are right. That would work as well.
I am ok with either way, but would recommend to keep it the existing way
as it makes it little more readable.

> Best regards,
> Tomasz

Re: [PATCH v2 2/5] media: venus: add a routine to set venus state

[Tomasz Figa]

On Wed, Jul 4, 2018 at 6:41 PM Vikash Garodia <vgarodia@codeaurora.org> wrote:
>
> On 2018-07-04 14:30, Tomasz Figa wrote:
> > On Wed, Jul 4, 2018 at 4:59 PM Vikash Garodia <vgarodia@codeaurora.org>
> > wrote:
> >> On 2018-06-04 18:24, Tomasz Figa wrote:
> >> > On Sat, Jun 2, 2018 at 6:21 AM Jordan Crouse <jcrouse@codeaurora.org>
> >> > wrote:
> >> >> On Sat, Jun 02, 2018 at 01:56:05AM +0530, Vikash Garodia wrote:
> >> > Given that this function is supposed to substitute existing calls into
> >> > qcom_scm_set_remote_state(), why not just do something like this:
> >> >
> >> >         if (qcom_scm_is_available())
> >> >                 return qcom_scm_set_remote_state(state, 0);
> >> >
> >> >         switch (state) {
> >> >         case TZBSP_VIDEO_SUSPEND:
> >> >                 writel_relaxed(1, reg_base + WRAPPER_A9SS_SW_RESET);
> >> >                 break;
> >> >         case TZBSP_VIDEO_RESUME:
> >> >                 venus_reset_hw(core);
> >> >                 break;
> >> >         }
> >> >
> >> >         return 0;
> >> This will not work as driver will write on the register irrespective
> >> of
> >> scm
> >> availability.
> >
> > I'm sorry, where would it do so? The second line returns from the
> > function inf SCM is available, so the rest of the function wouldn't be
> > executed.
>
> Ah!! you are right. That would work as well.
> I am ok with either way, but would recommend to keep it the existing way
> as it makes it little more readable.

I personally think the early exit is more readable, as it clearly
separates the SCM and non-SCM part.

Best regards,
Tomasz

Re: [PATCH v2 2/5] media: venus: add a routine to set venus state

[Stanimir Varbanov]

Hi Vikash,

Thanks for the patch!

On  1.06.2018 23:26, Vikash Garodia wrote:
> Add a new routine which abstracts the TZ call to

Actually the new routine abstracts Venus CPU state, Isn't it?

> set the video hardware state.
> 
> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
> ---
>   drivers/media/platform/qcom/venus/core.h      |  5 +++++
>   drivers/media/platform/qcom/venus/firmware.c  | 28 +++++++++++++++++++++++++++
>   drivers/media/platform/qcom/venus/firmware.h  |  1 +
>   drivers/media/platform/qcom/venus/hfi_venus.c | 13 ++++---------
>   4 files changed, 38 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
> index 85e66e2..e7bfb63 100644
> --- a/drivers/media/platform/qcom/venus/core.h
> +++ b/drivers/media/platform/qcom/venus/core.h
> @@ -35,6 +35,11 @@ struct reg_val {
>   	u32 value;
>   };
>   
> +enum tzbsp_video_state {
> +	TZBSP_VIDEO_SUSPEND = 0,
> +	TZBSP_VIDEO_RESUME
> +};

please move this in firmware.c, for more see below.

> +
>   struct venus_resources {
>   	u64 dma_mask;
>   	const struct freq_tbl *freq_tbl;
> diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c
> index 7d89b5a..b4664ed 100644
> --- a/drivers/media/platform/qcom/venus/firmware.c
> +++ b/drivers/media/platform/qcom/venus/firmware.c
> @@ -53,6 +53,34 @@ static void venus_reset_hw(struct venus_core *core)
>   	/* Bring Arm9 out of reset */
>   	writel_relaxed(0, reg_base + WRAPPER_A9SS_SW_RESET);
>   }
> +
> +int venus_set_hw_state(enum tzbsp_video_state state, struct venus_core *core)

can we put this function this way:

venus_set_state(struct venus_core *core, bool on)

so we set the state to On when we are power-up the venus CPU and Off
when we power-down.

by this way we really abstract the state, IMO.

> +{
> +	int ret;
> +	struct device *dev = core->dev;
> +	void __iomem *reg_base = core->base;

just 'base' should be enough.

> +
> +	switch (state) {
> +	case TZBSP_VIDEO_SUSPEND:
> +		if (qcom_scm_is_available())

You really shouldn't rely on this function (see the comment from Bjorn
on first version of this patch series).

I think we have to replace qcom_scm_is_available() with some flag which
is reflecting does the firmware subnode is exist or not. In case it is
not exist the we have to go with TZ scm calls.

> +			ret = qcom_scm_set_remote_state(TZBSP_VIDEO_SUSPEND, 0);
> +		else
> +			writel_relaxed(1, reg_base + WRAPPER_A9SS_SW_RESET);
> +		break;
> +	case TZBSP_VIDEO_RESUME:
> +		if (qcom_scm_is_available())
> +			ret = qcom_scm_set_remote_state(TZBSP_VIDEO_RESUME, 0);
> +		else
> +			venus_reset_hw(core);
> +		break;
> +	default:
> +		dev_err(dev, "invalid state\n");
> +		break;
> +	}
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(venus_set_hw_state);
> +

regards,
Stan

Re: [PATCH v2 2/5] media: venus: add a routine to set venus state

[Vikash Garodia]

Hi Stanimir,

Thanks for your valuable comments.

On 2018-06-04 19:20, Stanimir Varbanov wrote:
> Hi Vikash,
> 
> Thanks for the patch!
> 
> On  1.06.2018 23:26, Vikash Garodia wrote:
>> Add a new routine which abstracts the TZ call to
> 
> Actually the new routine abstracts Venus CPU state, Isn't it?

Yes, its a Venus CPU state controlled by TZ. I can mention it as
an abstraction of venus CPU state.

>> set the video hardware state.
>> 
>> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
>> ---
>>   drivers/media/platform/qcom/venus/core.h      |  5 +++++
>>   drivers/media/platform/qcom/venus/firmware.c  | 28 
>> +++++++++++++++++++++++++++
>>   drivers/media/platform/qcom/venus/firmware.h  |  1 +
>>   drivers/media/platform/qcom/venus/hfi_venus.c | 13 ++++---------
>>   4 files changed, 38 insertions(+), 9 deletions(-)
>> 
>> diff --git a/drivers/media/platform/qcom/venus/core.h 
>> b/drivers/media/platform/qcom/venus/core.h
>> index 85e66e2..e7bfb63 100644
>> --- a/drivers/media/platform/qcom/venus/core.h
>> +++ b/drivers/media/platform/qcom/venus/core.h
>> @@ -35,6 +35,11 @@ struct reg_val {
>>   	u32 value;
>>   };
>> 
>> +enum tzbsp_video_state {
>> +	TZBSP_VIDEO_SUSPEND = 0,
>> +	TZBSP_VIDEO_RESUME
>> +};
> 
> please move this in firmware.c, for more see below.
> 
>> +
>>   struct venus_resources {
>>   	u64 dma_mask;
>>   	const struct freq_tbl *freq_tbl;
>> diff --git a/drivers/media/platform/qcom/venus/firmware.c 
>> b/drivers/media/platform/qcom/venus/firmware.c
>> index 7d89b5a..b4664ed 100644
>> --- a/drivers/media/platform/qcom/venus/firmware.c
>> +++ b/drivers/media/platform/qcom/venus/firmware.c
>> @@ -53,6 +53,34 @@ static void venus_reset_hw(struct venus_core *core)
>>   	/* Bring Arm9 out of reset */
>>   	writel_relaxed(0, reg_base + WRAPPER_A9SS_SW_RESET);
>>   }
>> +
>> +int venus_set_hw_state(enum tzbsp_video_state state, struct 
>> venus_core *core)
> 
> can we put this function this way:
> 
> venus_set_state(struct venus_core *core, bool on)
> 
> so we set the state to On when we are power-up the venus CPU and Off
> when we power-down.
> 
> by this way we really abstract the state, IMO.

Good point. Will do in similar way.

>> +{
>> +	int ret;
>> +	struct device *dev = core->dev;
>> +	void __iomem *reg_base = core->base;
> 
> just 'base' should be enough.

Infact, comment from Jordan, we can remove it altogether.

>> +
>> +	switch (state) {
>> +	case TZBSP_VIDEO_SUSPEND:
>> +		if (qcom_scm_is_available())
> 
> You really shouldn't rely on this function (see the comment from Bjorn
> on first version of this patch series).
> 
> I think we have to replace qcom_scm_is_available() with some flag which
> is reflecting does the firmware subnode is exist or not. In case it is
> not exist the we have to go with TZ scm calls.

As we discussed, will keep it under the check for a valid firmware 
device.
We can avoid the extra flag in that way.

>> +			ret = qcom_scm_set_remote_state(TZBSP_VIDEO_SUSPEND, 0);
>> +		else
>> +			writel_relaxed(1, reg_base + WRAPPER_A9SS_SW_RESET);
>> +		break;
>> +	case TZBSP_VIDEO_RESUME:
>> +		if (qcom_scm_is_available())
>> +			ret = qcom_scm_set_remote_state(TZBSP_VIDEO_RESUME, 0);
>> +		else
>> +			venus_reset_hw(core);
>> +		break;
>> +	default:
>> +		dev_err(dev, "invalid state\n");
>> +		break;
>> +	}
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(venus_set_hw_state);
>> +
> 
> regards,
> Stan

Re: [PATCH v2 2/5] media: venus: add a routine to set venus state

[Vinod]

On 02-06-18, 01:56, Vikash Garodia wrote:

> +int venus_set_hw_state(enum tzbsp_video_state state, struct venus_core *core)
> +{
> +	int ret;

this should be init to 0 ...

> +	struct device *dev = core->dev;
> +	void __iomem *reg_base = core->base;
> +
> +	switch (state) {
> +	case TZBSP_VIDEO_SUSPEND:
> +		if (qcom_scm_is_available())
> +			ret = qcom_scm_set_remote_state(TZBSP_VIDEO_SUSPEND, 0);
> +		else
> +			writel_relaxed(1, reg_base + WRAPPER_A9SS_SW_RESET);
> +		break;
> +	case TZBSP_VIDEO_RESUME:
> +		if (qcom_scm_is_available())
> +			ret = qcom_scm_set_remote_state(TZBSP_VIDEO_RESUME, 0);
> +		else
> +			venus_reset_hw(core);
> +		break;
> +	default:

if it is default, ret contains garbage

> +		dev_err(dev, "invalid state\n");
> +		break;
> +	}
> +	return ret;

and that is returned.

Compiler should complain about these ...

> -enum tzbsp_video_state {
> -	TZBSP_VIDEO_STATE_SUSPEND = 0,
> -	TZBSP_VIDEO_STATE_RESUME
> -};

ah you are moving existing defines, please mention this in changelog.
Till this line I was expecting additions...
-- 
~Vinod

[PATCH v2 3/5] venus: add check to make scm calls

[Vikash Garodia]

Split the boot api into firmware load and hardware
boot. Also add the checks to invoke scm calls only
if the platform has the required support.

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

diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index 1308488..9a95f9a 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);
 
@@ -279,7 +279,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 b4664ed..cb7f48ef 100644
--- a/drivers/media/platform/qcom/venus/firmware.c
+++ b/drivers/media/platform/qcom/venus/firmware.c
@@ -81,40 +81,35 @@ int venus_set_hw_state(enum tzbsp_video_state state, struct venus_core *core)
 }
 EXPORT_SYMBOL_GPL(venus_set_hw_state);
 
-int venus_boot(struct device *dev, const char *fwname)
+static int venus_load_fw(struct device *dev, 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 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;
-
 	node = of_parse_phandle(dev->of_node, "memory-region", 0);
 	if (!node) {
 		dev_err(dev, "no memory-region specified\n");
 		return -EINVAL;
 	}
-
 	ret = of_address_to_resource(node, 0, &r);
 	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;
 	}
 
@@ -128,25 +123,49 @@ int venus_boot(struct device *dev, const char *fwname)
 		release_firmware(mdt);
 		goto err_unmap;
 	}
-
-	ret = qcom_mdt_load(dev, mdt, fwname, VENUS_PAS_ID, mem_va, mem_phys,
-			    mem_size);
+	if (qcom_scm_is_available())
+		ret = qcom_mdt_load(dev, mdt, fwname, VENUS_PAS_ID, mem_va,
+				*mem_phys, *mem_size, NULL);
+	else
+		ret = qcom_mdt_load_no_init(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)
+{
+	phys_addr_t mem_phys;
+	size_t mem_size;
+	int ret;
+	struct device *dev;
+
+	if (!IS_ENABLED(CONFIG_QCOM_MDT_LOADER))
+		return -EPROBE_DEFER;
+
+	dev = core->dev;
+
+	ret = venus_load_fw(dev, core->res->fwname, &mem_phys, &mem_size);
+	if (ret) {
+		dev_err(dev, "fail to load video firmware\n");
+		return -EINVAL;
+	}
+
+	if (qcom_scm_is_available())
+		ret = qcom_scm_pas_auth_and_reset(VENUS_PAS_ID);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(venus_boot);
 
 int venus_shutdown(struct device *dev)
 {
-	return qcom_scm_pas_shutdown(VENUS_PAS_ID);
+	int ret = 0;
+
+	if (qcom_scm_is_available())
+		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 1336729..0916826 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(enum tzbsp_video_state, struct venus_core *core);
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2 3/5] venus: add check to make scm calls

[Jordan Crouse]

On Sat, Jun 02, 2018 at 01:56:06AM +0530, Vikash Garodia wrote:
> Split the boot api into firmware load and hardware
> boot. Also add the checks to invoke scm calls only
> if the platform has the required support.
> 
> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
> ---
>  drivers/media/platform/qcom/venus/core.c     |  4 +-
>  drivers/media/platform/qcom/venus/firmware.c | 65 ++++++++++++++++++----------
>  drivers/media/platform/qcom/venus/firmware.h |  2 +-
>  3 files changed, 45 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> index 1308488..9a95f9a 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);
>  
> @@ -279,7 +279,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 b4664ed..cb7f48ef 100644
> --- a/drivers/media/platform/qcom/venus/firmware.c
> +++ b/drivers/media/platform/qcom/venus/firmware.c
> @@ -81,40 +81,35 @@ int venus_set_hw_state(enum tzbsp_video_state state, struct venus_core *core)
>  }
>  EXPORT_SYMBOL_GPL(venus_set_hw_state);
>  
> -int venus_boot(struct device *dev, const char *fwname)
> +static int venus_load_fw(struct device *dev, 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 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;
> -
>  	node = of_parse_phandle(dev->of_node, "memory-region", 0);
>  	if (!node) {
>  		dev_err(dev, "no memory-region specified\n");
>  		return -EINVAL;
>  	}
> -

Unrelated whitespace change.  Not needed.
>  	ret = of_address_to_resource(node, 0, &r);
>  	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;
>  	}
>  
> @@ -128,25 +123,49 @@ int venus_boot(struct device *dev, const char *fwname)
>  		release_firmware(mdt);
>  		goto err_unmap;
>  	}
> -
> -	ret = qcom_mdt_load(dev, mdt, fwname, VENUS_PAS_ID, mem_va, mem_phys,
> -			    mem_size);
> +	if (qcom_scm_is_available())
> +		ret = qcom_mdt_load(dev, mdt, fwname, VENUS_PAS_ID, mem_va,
> +				*mem_phys, *mem_size, NULL);
> +	else
> +		ret = qcom_mdt_load_no_init(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)
> +{
> +	phys_addr_t mem_phys;
> +	size_t mem_size;
> +	int ret;
> +	struct device *dev;
> +
> +	if (!IS_ENABLED(CONFIG_QCOM_MDT_LOADER))
> +		return -EPROBE_DEFER;
> +
> +	dev = core->dev;
> +
> +	ret = venus_load_fw(dev, core->res->fwname, &mem_phys, &mem_size);
> +	if (ret) {
> +		dev_err(dev, "fail to load video firmware\n");
> +		return -EINVAL;
> +	}
> +
> +	if (qcom_scm_is_available())
> +		ret = qcom_scm_pas_auth_and_reset(VENUS_PAS_ID);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(venus_boot);
>  
>  int venus_shutdown(struct device *dev)
>  {
> -	return qcom_scm_pas_shutdown(VENUS_PAS_ID);
> +	int ret = 0;
> +
> +	if (qcom_scm_is_available())
> +		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 1336729..0916826 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(enum tzbsp_video_state, struct venus_core *core);

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

Re: [PATCH v2 3/5] venus: add check to make scm calls

[Tomasz Figa]

Hi Vikash,

On Sat, Jun 2, 2018 at 5:27 AM Vikash Garodia <vgarodia@codeaurora.org> wrote:
[snip]
> +int venus_boot(struct venus_core *core)
> +{
> +       phys_addr_t mem_phys;
> +       size_t mem_size;
> +       int ret;
> +       struct device *dev;
> +
> +       if (!IS_ENABLED(CONFIG_QCOM_MDT_LOADER))
> +               return -EPROBE_DEFER;

Why are we deferring probe here? The option will not magically become
enabled after probe is retried.

Best regards,
Tomasz

Re: [PATCH v2 3/5] venus: add check to make scm calls

[Bjorn Andersson]

On Mon 04 Jun 05:58 PDT 2018, Tomasz Figa wrote:

> Hi Vikash,
> 
> On Sat, Jun 2, 2018 at 5:27 AM Vikash Garodia <vgarodia@codeaurora.org> wrote:
> [snip]
> > +int venus_boot(struct venus_core *core)
> > +{
> > +       phys_addr_t mem_phys;
> > +       size_t mem_size;
> > +       int ret;
> > +       struct device *dev;
> > +
> > +       if (!IS_ENABLED(CONFIG_QCOM_MDT_LOADER))
> > +               return -EPROBE_DEFER;
> 
> Why are we deferring probe here? The option will not magically become
> enabled after probe is retried.
> 

The original code should have read:

	if (IS_ENABLED(CONFIG_QCOM_MDT_LOADER) && !qcom_scm_is_available())
		return -EPROBE_DEFER;

The code does depend on CONFIG_QCOM_MDT_LOADER regardless of it using
scm for firmware verification.

Regards,
Bjorn

[PATCH v2 4/5] media: venus: 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     |  5 ++
 drivers/media/platform/qcom/venus/firmware.c | 86 ++++++++++++++++++++++++++--
 drivers/media/platform/qcom/venus/firmware.h |  7 ++-
 4 files changed, 94 insertions(+), 10 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index 9a95f9a..101612b 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);
 
@@ -318,7 +318,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);
@@ -339,7 +339,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 e7bfb63..f04e25e 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -85,6 +85,10 @@ struct venus_caps {
 	bool valid;
 };
 
+struct video_firmware {
+	struct device *dev;
+	struct iommu_domain *iommu_domain;
+};
 /**
  * struct venus_core - holds core parameters valid for all instances
  *
@@ -129,6 +133,7 @@ struct venus_core {
 	struct device *dev;
 	struct device *dev_dec;
 	struct device *dev_enc;
+	struct video_firmware fw;
 	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 cb7f48ef..058d544 100644
--- a/drivers/media/platform/qcom/venus/firmware.c
+++ b/drivers/media/platform/qcom/venus/firmware.c
@@ -12,8 +12,10 @@
  *
  */
 
+#include <linux/platform_device.h>
 #include <linux/device.h>
 #include <linux/firmware.h>
+#include <linux/iommu.h>
 #include <linux/delay.h>
 #include <linux/kernel.h>
 #include <linux/io.h>
@@ -27,9 +29,6 @@
 #include "firmware.h"
 #include "hfi_venus_io.h"
 
-#define VENUS_PAS_ID			9
-#define VENUS_FW_MEM_SIZE		(6 * SZ_1M)
-
 static void venus_reset_hw(struct venus_core *core)
 {
 	void __iomem *reg_base = core->base;
@@ -125,7 +124,7 @@ static int venus_load_fw(struct device *dev, const char *fwname,
 	}
 	if (qcom_scm_is_available())
 		ret = qcom_mdt_load(dev, mdt, fwname, VENUS_PAS_ID, mem_va,
-				*mem_phys, *mem_size, NULL);
+				*mem_phys, *mem_size);
 	else
 		ret = qcom_mdt_load_no_init(dev, mdt, fwname, VENUS_PAS_ID,
 				mem_va, *mem_phys, *mem_size, NULL);
@@ -136,6 +135,77 @@ static int venus_load_fw(struct device *dev, const char *fwname,
 	memunmap(mem_va);
 	return ret;
 }
+
+int venus_boot_noTZ(struct venus_core *core, phys_addr_t mem_phys,
+							size_t mem_size)
+{
+	struct iommu_domain *iommu;
+	struct device *dev;
+	int ret;
+
+	if (!core->fw.dev)
+		return -EPROBE_DEFER;
+
+	dev = core->fw.dev;
+
+	iommu = iommu_domain_alloc(&platform_bus_type);
+	if (!iommu) {
+		dev_err(dev, "Failed to allocate iommu domain\n");
+		return -ENOMEM;
+	}
+
+	ret = iommu_attach_device(iommu, dev);
+	if (ret) {
+		dev_err(dev, "could not attach device\n");
+		goto err_attach;
+	}
+
+	ret = iommu_map(iommu, 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;
+	venus_reset_hw(core);
+
+	return 0;
+
+err_map:
+	iommu_detach_device(iommu, dev);
+err_attach:
+	iommu_domain_free(iommu);
+	return ret;
+}
+
+int venus_shutdown_noTZ(struct venus_core *core)
+{
+	struct iommu_domain *iommu;
+	size_t unmapped = 0;
+	u32 reg;
+	struct device *dev = core->fw.dev;
+	void __iomem *reg_base = core->base;
+
+	/* Assert the reset to ARM9 */
+	reg = readl_relaxed(reg_base + WRAPPER_A9SS_SW_RESET);
+	reg |= BIT(4);
+	writel_relaxed(reg, 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)
 {
 	phys_addr_t mem_phys;
@@ -156,16 +226,20 @@ int venus_boot(struct venus_core *core)
 
 	if (qcom_scm_is_available())
 		ret = qcom_scm_pas_auth_and_reset(VENUS_PAS_ID);
+	else
+		ret = venus_boot_noTZ(core, mem_phys, mem_size);
 
 	return ret;
 }
-EXPORT_SYMBOL_GPL(venus_boot);
 
-int venus_shutdown(struct device *dev)
+int venus_shutdown(struct venus_core *core)
 {
 	int ret = 0;
 
 	if (qcom_scm_is_available())
 		ret = qcom_scm_pas_shutdown(VENUS_PAS_ID);
+	else
+		ret = venus_shutdown_noTZ(core);
+
 	return ret;
 }
diff --git a/drivers/media/platform/qcom/venus/firmware.h b/drivers/media/platform/qcom/venus/firmware.h
index 0916826..67fdd89 100644
--- a/drivers/media/platform/qcom/venus/firmware.h
+++ b/drivers/media/platform/qcom/venus/firmware.h
@@ -14,10 +14,15 @@
 #ifndef __VENUS_FIRMWARE_H__
 #define __VENUS_FIRMWARE_H__
 
+#define VENUS_PAS_ID			9
+#define VENUS_FW_START_ADDR		0x0
+#define VENUS_FW_MEM_SIZE		(6 * SZ_1M)
+#define VENUS_MAX_MEM_REGION	0xE0000000
+
 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(enum tzbsp_video_state, struct venus_core *core);
 
 #endif
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2 4/5] media: venus: add no TZ boot and shutdown routine

[Jordan Crouse]

On Sat, Jun 02, 2018 at 01:56:07AM +0530, Vikash Garodia 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     |  5 ++
>  drivers/media/platform/qcom/venus/firmware.c | 86 ++++++++++++++++++++++++++--
>  drivers/media/platform/qcom/venus/firmware.h |  7 ++-
>  4 files changed, 94 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> index 9a95f9a..101612b 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);
>  
> @@ -318,7 +318,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);
> @@ -339,7 +339,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 e7bfb63..f04e25e 100644
> --- a/drivers/media/platform/qcom/venus/core.h
> +++ b/drivers/media/platform/qcom/venus/core.h
> @@ -85,6 +85,10 @@ struct venus_caps {
>  	bool valid;
>  };
>  
> +struct video_firmware {
> +	struct device *dev;
> +	struct iommu_domain *iommu_domain;
> +};
>  /**
>   * struct venus_core - holds core parameters valid for all instances
>   *
> @@ -129,6 +133,7 @@ struct venus_core {
>  	struct device *dev;
>  	struct device *dev_dec;
>  	struct device *dev_enc;
> +	struct video_firmware fw;
>  	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 cb7f48ef..058d544 100644
> --- a/drivers/media/platform/qcom/venus/firmware.c
> +++ b/drivers/media/platform/qcom/venus/firmware.c
> @@ -12,8 +12,10 @@
>   *
>   */
>  
> +#include <linux/platform_device.h>
>  #include <linux/device.h>
>  #include <linux/firmware.h>
> +#include <linux/iommu.h>
>  #include <linux/delay.h>
>  #include <linux/kernel.h>
>  #include <linux/io.h>
> @@ -27,9 +29,6 @@
>  #include "firmware.h"
>  #include "hfi_venus_io.h"
>  
> -#define VENUS_PAS_ID			9
> -#define VENUS_FW_MEM_SIZE		(6 * SZ_1M)
> -
>  static void venus_reset_hw(struct venus_core *core)
>  {
>  	void __iomem *reg_base = core->base;
> @@ -125,7 +124,7 @@ static int venus_load_fw(struct device *dev, const char *fwname,
>  	}
>  	if (qcom_scm_is_available())
>  		ret = qcom_mdt_load(dev, mdt, fwname, VENUS_PAS_ID, mem_va,
> -				*mem_phys, *mem_size, NULL);
> +				*mem_phys, *mem_size);
>  	else
>  		ret = qcom_mdt_load_no_init(dev, mdt, fwname, VENUS_PAS_ID,
>  				mem_va, *mem_phys, *mem_size, NULL);
> @@ -136,6 +135,77 @@ static int venus_load_fw(struct device *dev, const char *fwname,
>  	memunmap(mem_va);
>  	return ret;
>  }
> +
> +int venus_boot_noTZ(struct venus_core *core, phys_addr_t mem_phys,

I'm not super enthusiastic about the capital letters, but I know what you're
going for so if everybody else is cool with it, I can be too.

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

Should be "could not attach device to iommu", so you know what part of the code you
are in.

> +		goto err_attach;
> +	}
> +
> +	ret = iommu_map(iommu, VENUS_FW_START_ADDR, mem_phys, mem_size,
> +			IOMMU_READ|IOMMU_WRITE|IOMMU_PRIV);

Do you really want to be mapping your firmware memory as IOMMU_WRITE?

> +	if (ret) {
> +		dev_err(dev, "could not map video firmware region\n");
> +		goto err_map;
> +	}
> +	core->fw.iommu_domain = iommu;
> +	venus_reset_hw(core);
> +
> +	return 0;
> +
> +err_map:
> +	iommu_detach_device(iommu, dev);
> +err_attach:
> +	iommu_domain_free(iommu);
> +	return ret;
> +}
> +
> +int venus_shutdown_noTZ(struct venus_core *core)
> +{
> +	struct iommu_domain *iommu;
> +	size_t unmapped = 0;
> +	u32 reg;
> +	struct device *dev = core->fw.dev;
> +	void __iomem *reg_base = core->base;
> +
> +	/* Assert the reset to ARM9 */
> +	reg = readl_relaxed(reg_base + WRAPPER_A9SS_SW_RESET);
> +	reg |= BIT(4);
> +	writel_relaxed(reg, reg_base + WRAPPER_A9SS_SW_RESET);

You could just use core->base directly in both these cases and not bother with
the local variable.

> +
> +	/* 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)
>  {
>  	phys_addr_t mem_phys;
> @@ -156,16 +226,20 @@ int venus_boot(struct venus_core *core)
>  
>  	if (qcom_scm_is_available())
>  		ret = qcom_scm_pas_auth_and_reset(VENUS_PAS_ID);
> +	else
> +		ret = venus_boot_noTZ(core, mem_phys, mem_size);
>  
>  	return ret;
>  }
> -EXPORT_SYMBOL_GPL(venus_boot);
>  
> -int venus_shutdown(struct device *dev)
> +int venus_shutdown(struct venus_core *core)
>  {
>  	int ret = 0;
>  
>  	if (qcom_scm_is_available())
>  		ret = qcom_scm_pas_shutdown(VENUS_PAS_ID);
> +	else
> +		ret = venus_shutdown_noTZ(core);
> +
>  	return ret;
>  }
> diff --git a/drivers/media/platform/qcom/venus/firmware.h b/drivers/media/platform/qcom/venus/firmware.h
> index 0916826..67fdd89 100644
> --- a/drivers/media/platform/qcom/venus/firmware.h
> +++ b/drivers/media/platform/qcom/venus/firmware.h
> @@ -14,10 +14,15 @@
>  #ifndef __VENUS_FIRMWARE_H__
>  #define __VENUS_FIRMWARE_H__
>  
> +#define VENUS_PAS_ID			9
> +#define VENUS_FW_START_ADDR		0x0
> +#define VENUS_FW_MEM_SIZE		(6 * SZ_1M)
> +#define VENUS_MAX_MEM_REGION	0xE0000000
> +
>  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(enum tzbsp_video_state, struct venus_core *core);
>  
>  #endif

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

Re: [PATCH v2 4/5] media: venus: add no TZ boot and shutdown routine

[Tomasz Figa]

Hi Vikash,

On Sat, Jun 2, 2018 at 5:27 AM Vikash Garodia <vgarodia@codeaurora.org> wrote:
[snip]
> +int venus_boot_noTZ(struct venus_core *core, phys_addr_t mem_phys,
> +                                                       size_t mem_size)
> +{
> +       struct iommu_domain *iommu;
> +       struct device *dev;
> +       int ret;
> +
> +       if (!core->fw.dev)
> +               return -EPROBE_DEFER;

Is it really possible that the device appears after the probe is retried?

> +
> +       dev = core->fw.dev;
> +
> +       iommu = iommu_domain_alloc(&platform_bus_type);
> +       if (!iommu) {
> +               dev_err(dev, "Failed to allocate iommu domain\n");
> +               return -ENOMEM;
> +       }
> +
> +       ret = iommu_attach_device(iommu, dev);
> +       if (ret) {
> +               dev_err(dev, "could not attach device\n");
> +               goto err_attach;
> +       }
> +
> +       ret = iommu_map(iommu, 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;
> +       }

I'm not very familiar with translation capabilities of ARM SMMU, so
that might be an elementary question, but could you explain how this
works? Will this make the firmware device (transfers with firmware
PASID) use different page directory from the main device (all the
other PASIDs; used with DMA mapping API)?

+Will and Robin, just in case.

> +       core->fw.iommu_domain = iommu;
> +       venus_reset_hw(core);
> +
> +       return 0;
> +
> +err_map:
> +       iommu_detach_device(iommu, dev);
> +err_attach:
> +       iommu_domain_free(iommu);
> +       return ret;
> +}
[snip]
> diff --git a/drivers/media/platform/qcom/venus/firmware.h b/drivers/media/platform/qcom/venus/firmware.h
> index 0916826..67fdd89 100644
> --- a/drivers/media/platform/qcom/venus/firmware.h
> +++ b/drivers/media/platform/qcom/venus/firmware.h
> @@ -14,10 +14,15 @@
>  #ifndef __VENUS_FIRMWARE_H__
>  #define __VENUS_FIRMWARE_H__
>
> +#define VENUS_PAS_ID                   9

Shouldn't this normally be given in DT?

Best regards,
Tomasz

[PATCH v2 5/5] venus: register separate driver for firmware device

[Vikash Garodia]

A separate child device is added for video firmware.
This is needed to
[1] configure the firmware context bank with the desired SID.
[2] ensure that the iova for firmware region is from 0x0.

Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
---
 .../devicetree/bindings/media/qcom,venus.txt       |  8 +++-
 drivers/media/platform/qcom/venus/core.c           | 48 +++++++++++++++++++---
 drivers/media/platform/qcom/venus/firmware.c       | 20 ++++++++-
 drivers/media/platform/qcom/venus/firmware.h       |  2 +
 4 files changed, 71 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/qcom,venus.txt b/Documentation/devicetree/bindings/media/qcom,venus.txt
index 00d0d1b..701cbe8 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, one optional firmware subnode.
 
 Every of video-encoder or video-decoder subnode should have:
 
@@ -79,6 +79,8 @@ 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 sub node must contain the iommus specifiers for ARM9.
+
 * An Example
 	video-codec@1d00000 {
 		compatible = "qcom,msm8916-venus";
@@ -105,4 +107,8 @@ Every of video-encoder or video-decoder subnode should have:
 			clock-names = "core";
 			power-domains = <&mmcc VENUS_CORE1_GDSC>;
 		};
+		venus-firmware {
+			compatible = "qcom,venus-firmware-no-tz";
+			iommus = <&apps_smmu 0x10b2 0x0>;
+		}
 	};
diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index 101612b..5cfb3c2 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -179,6 +179,19 @@ static u32 to_v4l2_codec_type(u32 codec)
 	}
 }
 
+static int store_firmware_dev(struct device *dev, void *data)
+{
+	struct venus_core *core = data;
+
+	if (!core)
+		return -EINVAL;
+
+	if (of_device_is_compatible(dev->of_node, "qcom,venus-firmware-no-tz"))
+		core->fw.dev = dev;
+
+	return 0;
+}
+
 static int venus_enumerate_codecs(struct venus_core *core, u32 type)
 {
 	const struct hfi_inst_ops dummy_ops = {};
@@ -279,6 +292,13 @@ 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;
+
+	/* Attempt to store firmware device */
+	device_for_each_child(dev, core, store_firmware_dev);
+
 	ret = venus_boot(core);
 	if (ret)
 		goto err_runtime_disable;
@@ -303,10 +323,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;
@@ -483,7 +499,29 @@ static __maybe_unused int venus_runtime_resume(struct device *dev)
 		.pm = &venus_pm_ops,
 	},
 };
-module_platform_driver(qcom_venus_driver);
+
+static int __init venus_init(void)
+{
+	int ret;
+
+	ret = platform_driver_register(&qcom_video_firmware_driver);
+	if (ret)
+		return ret;
+
+	ret = platform_driver_register(&qcom_venus_driver);
+	if (ret)
+		platform_driver_unregister(&qcom_video_firmware_driver);
+
+	return ret;
+}
+module_init(venus_init);
+
+static void __exit venus_exit(void)
+{
+	platform_driver_unregister(&qcom_venus_driver);
+	platform_driver_unregister(&qcom_video_firmware_driver);
+}
+module_exit(venus_exit);
 
 MODULE_ALIAS("platform:qcom-venus");
 MODULE_DESCRIPTION("Qualcomm Venus video encoder and decoder driver");
diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c
index 058d544..ed29d10 100644
--- a/drivers/media/platform/qcom/venus/firmware.c
+++ b/drivers/media/platform/qcom/venus/firmware.c
@@ -12,6 +12,7 @@
  *
  */
 
+#include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/device.h>
 #include <linux/firmware.h>
@@ -124,7 +125,7 @@ static int venus_load_fw(struct device *dev, const char *fwname,
 	}
 	if (qcom_scm_is_available())
 		ret = qcom_mdt_load(dev, mdt, fwname, VENUS_PAS_ID, mem_va,
-				*mem_phys, *mem_size);
+				*mem_phys, *mem_size, NULL);
 	else
 		ret = qcom_mdt_load_no_init(dev, mdt, fwname, VENUS_PAS_ID,
 				mem_va, *mem_phys, *mem_size, NULL);
@@ -243,3 +244,20 @@ int venus_shutdown(struct venus_core *core)
 
 	return ret;
 }
+
+static const struct of_device_id firmware_dt_match[] = {
+	{ .compatible = "qcom,venus-firmware-no-tz" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, firmware_dt_match);
+
+struct platform_driver qcom_video_firmware_driver = {
+	.driver = {
+			.name = "qcom-video-firmware",
+			.of_match_table = firmware_dt_match,
+	},
+};
+
+MODULE_ALIAS("platform:qcom-video-firmware");
+MODULE_DESCRIPTION("Qualcomm Venus firmware driver");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/media/platform/qcom/venus/firmware.h b/drivers/media/platform/qcom/venus/firmware.h
index 67fdd89..23c0409 100644
--- a/drivers/media/platform/qcom/venus/firmware.h
+++ b/drivers/media/platform/qcom/venus/firmware.h
@@ -21,6 +21,8 @@
 
 struct device;
 
+extern struct platform_driver qcom_video_firmware_driver;
+
 int venus_boot(struct venus_core *core);
 int venus_shutdown(struct venus_core *core);
 int venus_set_hw_state(enum tzbsp_video_state, struct venus_core *core);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2 5/5] venus: register separate driver for firmware device

[Jordan Crouse]

On Sat, Jun 02, 2018 at 01:56:08AM +0530, Vikash Garodia wrote:
> A separate child device is added for video firmware.
> This is needed to
> [1] configure the firmware context bank with the desired SID.
> [2] ensure that the iova for firmware region is from 0x0.
> 
> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
> ---
>  .../devicetree/bindings/media/qcom,venus.txt       |  8 +++-
>  drivers/media/platform/qcom/venus/core.c           | 48 +++++++++++++++++++---
>  drivers/media/platform/qcom/venus/firmware.c       | 20 ++++++++-
>  drivers/media/platform/qcom/venus/firmware.h       |  2 +
>  4 files changed, 71 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/qcom,venus.txt b/Documentation/devicetree/bindings/media/qcom,venus.txt
> index 00d0d1b..701cbe8 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, one optional firmware subnode.
>  
>  Every of video-encoder or video-decoder subnode should have:
>  
> @@ -79,6 +79,8 @@ 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 sub node must contain the iommus specifiers for ARM9.
> +
>  * An Example
>  	video-codec@1d00000 {
>  		compatible = "qcom,msm8916-venus";
> @@ -105,4 +107,8 @@ Every of video-encoder or video-decoder subnode should have:
>  			clock-names = "core";
>  			power-domains = <&mmcc VENUS_CORE1_GDSC>;
>  		};
> +		venus-firmware {
> +			compatible = "qcom,venus-firmware-no-tz";
> +			iommus = <&apps_smmu 0x10b2 0x0>;
> +		}
>  	};
> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> index 101612b..5cfb3c2 100644
> --- a/drivers/media/platform/qcom/venus/core.c
> +++ b/drivers/media/platform/qcom/venus/core.c
> @@ -179,6 +179,19 @@ static u32 to_v4l2_codec_type(u32 codec)
>  	}
>  }
>  
> +static int store_firmware_dev(struct device *dev, void *data)
> +{
> +	struct venus_core *core = data;
> +
> +	if (!core)
> +		return -EINVAL;

Core is not going to be null here - you don't need to check it.

<snip>

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

Re: [PATCH v2 5/5] venus: register separate driver for firmware device

[Tomasz Figa]

Hi Vikash,

On Sat, Jun 2, 2018 at 5:27 AM Vikash Garodia <vgarodia@codeaurora.org> wrote:
>
> A separate child device is added for video firmware.
> This is needed to
> [1] configure the firmware context bank with the desired SID.
> [2] ensure that the iova for firmware region is from 0x0.
>
> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
> ---
>  .../devicetree/bindings/media/qcom,venus.txt       |  8 +++-
>  drivers/media/platform/qcom/venus/core.c           | 48 +++++++++++++++++++---
>  drivers/media/platform/qcom/venus/firmware.c       | 20 ++++++++-
>  drivers/media/platform/qcom/venus/firmware.h       |  2 +
>  4 files changed, 71 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/media/qcom,venus.txt b/Documentation/devicetree/bindings/media/qcom,venus.txt
> index 00d0d1b..701cbe8 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, one optional firmware subnode.
>
>  Every of video-encoder or video-decoder subnode should have:
>
> @@ -79,6 +79,8 @@ 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 sub node must contain the iommus specifiers for ARM9.

Please document the compatible string here as well.

> +
>  * An Example
>         video-codec@1d00000 {
>                 compatible = "qcom,msm8916-venus";
> @@ -105,4 +107,8 @@ Every of video-encoder or video-decoder subnode should have:
>                         clock-names = "core";
>                         power-domains = <&mmcc VENUS_CORE1_GDSC>;
>                 };
> +               venus-firmware {
> +                       compatible = "qcom,venus-firmware-no-tz";

I don't think "-no-tz" should be mentioned here in DT, since it's a
firmware/software detail.

> +                       iommus = <&apps_smmu 0x10b2 0x0>;
> +               }
>         };
> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> index 101612b..5cfb3c2 100644
> --- a/drivers/media/platform/qcom/venus/core.c
> +++ b/drivers/media/platform/qcom/venus/core.c
> @@ -179,6 +179,19 @@ static u32 to_v4l2_codec_type(u32 codec)
>         }
>  }
>
> +static int store_firmware_dev(struct device *dev, void *data)
> +{
> +       struct venus_core *core = data;
> +
> +       if (!core)
> +               return -EINVAL;
> +

No need to check this AFAICT.

> +       if (of_device_is_compatible(dev->of_node, "qcom,venus-firmware-no-tz"))
> +               core->fw.dev = dev;
> +
> +       return 0;
> +}
> +
>  static int venus_enumerate_codecs(struct venus_core *core, u32 type)
>  {
>         const struct hfi_inst_ops dummy_ops = {};
> @@ -279,6 +292,13 @@ 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;
> +
> +       /* Attempt to store firmware device */
> +       device_for_each_child(dev, core, store_firmware_dev);
> +
>         ret = venus_boot(core);
>         if (ret)
>                 goto err_runtime_disable;
> @@ -303,10 +323,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;
> @@ -483,7 +499,29 @@ static __maybe_unused int venus_runtime_resume(struct device *dev)
>                 .pm = &venus_pm_ops,
>         },
>  };
> -module_platform_driver(qcom_venus_driver);
> +
> +static int __init venus_init(void)
> +{
> +       int ret;
> +
> +       ret = platform_driver_register(&qcom_video_firmware_driver);
> +       if (ret)
> +               return ret;

Do we really need this firmware driver? As far as I can see, the
approach used here should work even without any driver bound to the
firmware device.

Best regards,
Tomasz

Re: [PATCH v2 5/5] venus: register separate driver for firmware device

[Stanimir Varbanov]

Hi Tomasz,

On 06/04/2018 04:18 PM, Tomasz Figa wrote:
> Hi Vikash,
> 
> On Sat, Jun 2, 2018 at 5:27 AM Vikash Garodia <vgarodia@codeaurora.org> wrote:
>>
>> A separate child device is added for video firmware.
>> This is needed to
>> [1] configure the firmware context bank with the desired SID.
>> [2] ensure that the iova for firmware region is from 0x0.
>>
>> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
>> ---
>>  .../devicetree/bindings/media/qcom,venus.txt       |  8 +++-
>>  drivers/media/platform/qcom/venus/core.c           | 48 +++++++++++++++++++---
>>  drivers/media/platform/qcom/venus/firmware.c       | 20 ++++++++-
>>  drivers/media/platform/qcom/venus/firmware.h       |  2 +
>>  4 files changed, 71 insertions(+), 7 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/media/qcom,venus.txt b/Documentation/devicetree/bindings/media/qcom,venus.txt
>> index 00d0d1b..701cbe8 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, one optional firmware subnode.
>>
>>  Every of video-encoder or video-decoder subnode should have:
>>
>> @@ -79,6 +79,8 @@ 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 sub node must contain the iommus specifiers for ARM9.
> 
> Please document the compatible string here as well.
> 
>> +
>>  * An Example
>>         video-codec@1d00000 {
>>                 compatible = "qcom,msm8916-venus";
>> @@ -105,4 +107,8 @@ Every of video-encoder or video-decoder subnode should have:
>>                         clock-names = "core";
>>                         power-domains = <&mmcc VENUS_CORE1_GDSC>;
>>                 };
>> +               venus-firmware {
>> +                       compatible = "qcom,venus-firmware-no-tz";
> 
> I don't think "-no-tz" should be mentioned here in DT, since it's a
> firmware/software detail.

I have to agree with Tomasz, non-tz or tz is a software detail and it
shouldn't be reflected in compatible string.

Also I'm not sure but what will happen if this video-firmware subnode is
not added, do you expect that backward compatibility is satisfied for
older venus versions?

> 
>> +                       iommus = <&apps_smmu 0x10b2 0x0>;
>> +               }
>>         };
>> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
>> index 101612b..5cfb3c2 100644
>> --- a/drivers/media/platform/qcom/venus/core.c
>> +++ b/drivers/media/platform/qcom/venus/core.c
>> @@ -179,6 +179,19 @@ static u32 to_v4l2_codec_type(u32 codec)
>>         }
>>  }
>>
>> +static int store_firmware_dev(struct device *dev, void *data)
>> +{
>> +       struct venus_core *core = data;
>> +
>> +       if (!core)
>> +               return -EINVAL;
>> +
> 
> No need to check this AFAICT.>
>> +       if (of_device_is_compatible(dev->of_node, "qcom,venus-firmware-no-tz"))
>> +               core->fw.dev = dev;
>> +
>> +       return 0;
>> +}
>> +
>>  static int venus_enumerate_codecs(struct venus_core *core, u32 type)
>>  {
>>         const struct hfi_inst_ops dummy_ops = {};
>> @@ -279,6 +292,13 @@ 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;
>> +
>> +       /* Attempt to store firmware device */
>> +       device_for_each_child(dev, core, store_firmware_dev);
>> +
>>         ret = venus_boot(core);
>>         if (ret)
>>                 goto err_runtime_disable;
>> @@ -303,10 +323,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;
>> @@ -483,7 +499,29 @@ static __maybe_unused int venus_runtime_resume(struct device *dev)
>>                 .pm = &venus_pm_ops,
>>         },
>>  };
>> -module_platform_driver(qcom_venus_driver);
>> +
>> +static int __init venus_init(void)
>> +{
>> +       int ret;
>> +
>> +       ret = platform_driver_register(&qcom_video_firmware_driver);
>> +       if (ret)
>> +               return ret;
> 
> Do we really need this firmware driver? As far as I can see, the
> approach used here should work even without any driver bound to the
> firmware device.

We need device/driver bind because we need to call dma_configure() which
internally doing iommus sID parsing.

-- 
regards,
Stan

Re: [PATCH v2 5/5] venus: register separate driver for firmware device

[Tomasz Figa]

On Mon, Jun 4, 2018 at 10:56 PM Stanimir Varbanov
<stanimir.varbanov@linaro.org> wrote:
>
> Hi Tomasz,
>
> On 06/04/2018 04:18 PM, Tomasz Figa wrote:
> > Hi Vikash,
> >
> > On Sat, Jun 2, 2018 at 5:27 AM Vikash Garodia <vgarodia@codeaurora.org> wrote:
> >> +static int __init venus_init(void)
> >> +{
> >> +       int ret;
> >> +
> >> +       ret = platform_driver_register(&qcom_video_firmware_driver);
> >> +       if (ret)
> >> +               return ret;
> >
> > Do we really need this firmware driver? As far as I can see, the
> > approach used here should work even without any driver bound to the
> > firmware device.
>
> We need device/driver bind because we need to call dma_configure() which
> internally doing iommus sID parsing.

I can see some drivers calling of_dma_configure() directly:
https://elixir.bootlin.com/linux/latest/ident/of_dma_configure

I'm not sure if it's more elegant, but should at least require less code.

By the way, can we really assume that probe of firmware platform
device really completes before we call venus_boot()?

Best regards,
Tomasz

Re: [PATCH v2 5/5] venus: register separate driver for firmware device

[Stanimir Varbanov]

Cc: Arnd

On 06/05/2018 07:08 AM, Tomasz Figa wrote:
> On Mon, Jun 4, 2018 at 10:56 PM Stanimir Varbanov
> <stanimir.varbanov@linaro.org> wrote:
>>
>> Hi Tomasz,
>>
>> On 06/04/2018 04:18 PM, Tomasz Figa wrote:
>>> Hi Vikash,
>>>
>>> On Sat, Jun 2, 2018 at 5:27 AM Vikash Garodia <vgarodia@codeaurora.org> wrote:
>>>> +static int __init venus_init(void)
>>>> +{
>>>> +       int ret;
>>>> +
>>>> +       ret = platform_driver_register(&qcom_video_firmware_driver);
>>>> +       if (ret)
>>>> +               return ret;
>>>
>>> Do we really need this firmware driver? As far as I can see, the
>>> approach used here should work even without any driver bound to the
>>> firmware device.
>>
>> We need device/driver bind because we need to call dma_configure() which
>> internally doing iommus sID parsing.
> 
> I can see some drivers calling of_dma_configure() directly:
> https://elixir.bootlin.com/linux/latest/ident/of_dma_configure
> 
> I'm not sure if it's more elegant, but should at least require less code.

I think that in this case of non-TZ where we do iommu mapping by hand we
can use shared-dma-pool reserved memory see how venus_boot has been
implemented in the beginning [1].

Arnd what do you think?

Some background, we have a use-case where the memory for firmware needs
to be mapped by the venus driver by hand instead of TZ firmware calls.
I.e. we want to support both, iommu mapping from the driver and mapping
done by TZ firmware. How we will differentiate what mapping (TZ or
non-TZ) will be used is a separate issue.

> 
> By the way, can we really assume that probe of firmware platform
> device really completes before we call venus_boot()?

I'd say we cannot.

-- 
regards,
Stan

[1] https://lkml.org/lkml/2017/4/28/214

Re: [PATCH v2 5/5] venus: register separate driver for firmware device

[Tomasz Figa]

On Tue, Jun 5, 2018 at 5:45 PM Stanimir Varbanov
<stanimir.varbanov@linaro.org> wrote:
>
> Cc: Arnd
>
> On 06/05/2018 07:08 AM, Tomasz Figa wrote:
> > On Mon, Jun 4, 2018 at 10:56 PM Stanimir Varbanov
> > <stanimir.varbanov@linaro.org> wrote:
> >>
> >> Hi Tomasz,
> >>
> >> On 06/04/2018 04:18 PM, Tomasz Figa wrote:
> >>> Hi Vikash,
> >>>
> >>> On Sat, Jun 2, 2018 at 5:27 AM Vikash Garodia <vgarodia@codeaurora.org> wrote:
> >>>> +static int __init venus_init(void)
> >>>> +{
> >>>> +       int ret;
> >>>> +
> >>>> +       ret = platform_driver_register(&qcom_video_firmware_driver);
> >>>> +       if (ret)
> >>>> +               return ret;
> >>>
> >>> Do we really need this firmware driver? As far as I can see, the
> >>> approach used here should work even without any driver bound to the
> >>> firmware device.
> >>
> >> We need device/driver bind because we need to call dma_configure() which
> >> internally doing iommus sID parsing.
> >
> > I can see some drivers calling of_dma_configure() directly:
> > https://elixir.bootlin.com/linux/latest/ident/of_dma_configure
> >
> > I'm not sure if it's more elegant, but should at least require less code.
>
> I think that in this case of non-TZ where we do iommu mapping by hand we
> can use shared-dma-pool reserved memory see how venus_boot has been
> implemented in the beginning [1].

I might have misunderstood something, but wasn't the shared-dma-pool
about reserving physical memory, while the venus firmware problem is
about reserving certain range of IOVA?

>
> Arnd what do you think?
>
> Some background, we have a use-case where the memory for firmware needs
> to be mapped by the venus driver by hand instead of TZ firmware calls.
> I.e. we want to support both, iommu mapping from the driver and mapping
> done by TZ firmware. How we will differentiate what mapping (TZ or
> non-TZ) will be used is a separate issue.
>
> >
> > By the way, can we really assume that probe of firmware platform
> > device really completes before we call venus_boot()?
>
> I'd say we cannot.

Looking at current implementation in driver core,
of_platform_populate() would actually trigger a synchronous probe, so
I guess it could work. However, I'm not sure if this is a general
guarantee here or it's an implementation detail that shouldn't be
relied on.

If we end up really need to have this platform_driver, I guess we
could call platform_driver_probe() after of_platform_populate(),
rather than pre-registering the driver. That seems to be the way to
ensure that the probe is synchronous and we can also check that a
matching device was found by the return value.

Best regards,
Tomasz

Re: [PATCH v2 5/5] venus: register separate driver for firmware device

[Bjorn Andersson]

On Mon 04 Jun 06:56 PDT 2018, Stanimir Varbanov wrote:
> On 06/04/2018 04:18 PM, Tomasz Figa wrote:
> > On Sat, Jun 2, 2018 at 5:27 AM Vikash Garodia <vgarodia@codeaurora.org> wrote:
[..]
> >> +               venus-firmware {
> >> +                       compatible = "qcom,venus-firmware-no-tz";
> > 
> > I don't think "-no-tz" should be mentioned here in DT, since it's a
> > firmware/software detail.
> 
> I have to agree with Tomasz, non-tz or tz is a software detail and it
> shouldn't be reflected in compatible string.
> 

While it is software, the alternative boot and security configuration
does imply different requirements on how the driver deals with the
hardware. I'm not sure how you expect the kernel to be informed about
the abilities of the boot/security capabilities if it's not passed
through DT.


In the other cases of firmware loading for co-processors this means that
a number of additional resources (clocks, resets) needs to be specified
in the DT node; something it seems like Venus doesn't have to do.

> Also I'm not sure but what will happen if this video-firmware subnode is
> not added, do you expect that backward compatibility is satisfied for
> older venus versions?
> 

I do expect that the driver should be possible to run on a 845 with the
normal TZ based security model we've seen on e.g. 820. I don't know the
details of Venus well enough to see if this differentiation would be
sufficient.

Regards,
Bjorn

Re: [PATCH v2 5/5] venus: register separate driver for firmware device

[Rob Herring]

On Sat, Jun 02, 2018 at 01:56:08AM +0530, Vikash Garodia wrote:
> A separate child device is added for video firmware.
> This is needed to
> [1] configure the firmware context bank with the desired SID.
> [2] ensure that the iova for firmware region is from 0x0.
> 
> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
> ---
>  .../devicetree/bindings/media/qcom,venus.txt       |  8 +++-
>  drivers/media/platform/qcom/venus/core.c           | 48 +++++++++++++++++++---
>  drivers/media/platform/qcom/venus/firmware.c       | 20 ++++++++-
>  drivers/media/platform/qcom/venus/firmware.h       |  2 +
>  4 files changed, 71 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/qcom,venus.txt b/Documentation/devicetree/bindings/media/qcom,venus.txt
> index 00d0d1b..701cbe8 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, one optional firmware subnode.
>  
>  Every of video-encoder or video-decoder subnode should have:
>  
> @@ -79,6 +79,8 @@ 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 sub node must contain the iommus specifiers for ARM9.
> +
>  * An Example
>  	video-codec@1d00000 {
>  		compatible = "qcom,msm8916-venus";
> @@ -105,4 +107,8 @@ Every of video-encoder or video-decoder subnode should have:
>  			clock-names = "core";
>  			power-domains = <&mmcc VENUS_CORE1_GDSC>;
>  		};
> +		venus-firmware {
> +			compatible = "qcom,venus-firmware-no-tz";
> +			iommus = <&apps_smmu 0x10b2 0x0>;

This mostly looks like you are adding a node in order to create a 
platform device. DT is not the only way to create platform devices and 
shouldn't be used when the device is not really a separate h/w device. 
Plus it seems like it is debatable that you even need a driver.

For iommus, just move it up to the parent (or add to existing prop).

Rob

Re: [PATCH v2 5/5] venus: register separate driver for firmware device

[Tomasz Figa]

Hi Rob,

On Wed, Jun 6, 2018 at 6:08 AM Rob Herring <robh@kernel.org> wrote:
>
> On Sat, Jun 02, 2018 at 01:56:08AM +0530, Vikash Garodia wrote:
> > A separate child device is added for video firmware.
> > This is needed to
> > [1] configure the firmware context bank with the desired SID.
> > [2] ensure that the iova for firmware region is from 0x0.
> >
> > Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
> > ---
> >  .../devicetree/bindings/media/qcom,venus.txt       |  8 +++-
> >  drivers/media/platform/qcom/venus/core.c           | 48 +++++++++++++++++++---
> >  drivers/media/platform/qcom/venus/firmware.c       | 20 ++++++++-
> >  drivers/media/platform/qcom/venus/firmware.h       |  2 +
> >  4 files changed, 71 insertions(+), 7 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/media/qcom,venus.txt b/Documentation/devicetree/bindings/media/qcom,venus.txt
> > index 00d0d1b..701cbe8 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, one optional firmware subnode.
> >
> >  Every of video-encoder or video-decoder subnode should have:
> >
> > @@ -79,6 +79,8 @@ 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 sub node must contain the iommus specifiers for ARM9.
> > +
> >  * An Example
> >       video-codec@1d00000 {
> >               compatible = "qcom,msm8916-venus";
> > @@ -105,4 +107,8 @@ Every of video-encoder or video-decoder subnode should have:
> >                       clock-names = "core";
> >                       power-domains = <&mmcc VENUS_CORE1_GDSC>;
> >               };
> > +             venus-firmware {
> > +                     compatible = "qcom,venus-firmware-no-tz";
> > +                     iommus = <&apps_smmu 0x10b2 0x0>;
>
> This mostly looks like you are adding a node in order to create a
> platform device. DT is not the only way to create platform devices and
> shouldn't be used when the device is not really a separate h/w device.
> Plus it seems like it is debatable that you even need a driver.
>
> For iommus, just move it up to the parent (or add to existing prop).

As far as I understood the issue from reading this series and also
talking a bit with Stanimir, there are multiple (physical?) ports from
the Venus hardware block and that includes one dedicated for firmware
loading, which has IOVA range restrictions up to 6 MiBs or something
like that.

If we add the firmware port to the iommus property of the main node,
we would bind it to the same IOVA address space as the other ports and
so it would be part of the main full 32-bit IOMMU domain.

Best regards,
Tomasz

Re: [PATCH v2 5/5] venus: register separate driver for firmware device

[Rob Herring]

On Tue, Jun 5, 2018 at 11:46 PM, Tomasz Figa <tfiga@google.com> wrote:
> Hi Rob,
>
> On Wed, Jun 6, 2018 at 6:08 AM Rob Herring <robh@kernel.org> wrote:
>>
>> On Sat, Jun 02, 2018 at 01:56:08AM +0530, Vikash Garodia wrote:
>> > A separate child device is added for video firmware.
>> > This is needed to
>> > [1] configure the firmware context bank with the desired SID.
>> > [2] ensure that the iova for firmware region is from 0x0.
>> >
>> > Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
>> > ---
>> >  .../devicetree/bindings/media/qcom,venus.txt       |  8 +++-
>> >  drivers/media/platform/qcom/venus/core.c           | 48 +++++++++++++++++++---
>> >  drivers/media/platform/qcom/venus/firmware.c       | 20 ++++++++-
>> >  drivers/media/platform/qcom/venus/firmware.h       |  2 +
>> >  4 files changed, 71 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/Documentation/devicetree/bindings/media/qcom,venus.txt b/Documentation/devicetree/bindings/media/qcom,venus.txt
>> > index 00d0d1b..701cbe8 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, one optional firmware subnode.
>> >
>> >  Every of video-encoder or video-decoder subnode should have:
>> >
>> > @@ -79,6 +79,8 @@ 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 sub node must contain the iommus specifiers for ARM9.
>> > +
>> >  * An Example
>> >       video-codec@1d00000 {
>> >               compatible = "qcom,msm8916-venus";
>> > @@ -105,4 +107,8 @@ Every of video-encoder or video-decoder subnode should have:
>> >                       clock-names = "core";
>> >                       power-domains = <&mmcc VENUS_CORE1_GDSC>;
>> >               };
>> > +             venus-firmware {
>> > +                     compatible = "qcom,venus-firmware-no-tz";
>> > +                     iommus = <&apps_smmu 0x10b2 0x0>;
>>
>> This mostly looks like you are adding a node in order to create a
>> platform device. DT is not the only way to create platform devices and
>> shouldn't be used when the device is not really a separate h/w device.
>> Plus it seems like it is debatable that you even need a driver.
>>
>> For iommus, just move it up to the parent (or add to existing prop).
>
> As far as I understood the issue from reading this series and also
> talking a bit with Stanimir, there are multiple (physical?) ports from
> the Venus hardware block and that includes one dedicated for firmware
> loading, which has IOVA range restrictions up to 6 MiBs or something
> like that.
>
> If we add the firmware port to the iommus property of the main node,
> we would bind it to the same IOVA address space as the other ports and
> so it would be part of the main full 32-bit IOMMU domain.

Sounds like an OS limitation, not a DT problem.

That being said, I suppose we can live with having this sub-node if we
can't fix or work-around this limitation.

Rob

Re: [PATCH v2 5/5] venus: register separate driver for firmware device

[Vikash Garodia]

On 2018-06-06 10:16, Tomasz Figa wrote:
> Hi Rob,
> 
> On Wed, Jun 6, 2018 at 6:08 AM Rob Herring <robh@kernel.org> wrote:
>> 
>> On Sat, Jun 02, 2018 at 01:56:08AM +0530, Vikash Garodia wrote:
>> > A separate child device is added for video firmware.
>> > This is needed to
>> > [1] configure the firmware context bank with the desired SID.
>> > [2] ensure that the iova for firmware region is from 0x0.
>> >
>> > Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
>> > ---
>> >  .../devicetree/bindings/media/qcom,venus.txt       |  8 +++-
>> >  drivers/media/platform/qcom/venus/core.c           | 48 +++++++++++++++++++---
>> >  drivers/media/platform/qcom/venus/firmware.c       | 20 ++++++++-
>> >  drivers/media/platform/qcom/venus/firmware.h       |  2 +
>> >  4 files changed, 71 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/Documentation/devicetree/bindings/media/qcom,venus.txt b/Documentation/devicetree/bindings/media/qcom,venus.txt
>> > index 00d0d1b..701cbe8 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, one optional firmware subnode.
>> >
>> >  Every of video-encoder or video-decoder subnode should have:
>> >
>> > @@ -79,6 +79,8 @@ 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 sub node must contain the iommus specifiers for ARM9.
>> > +
>> >  * An Example
>> >       video-codec@1d00000 {
>> >               compatible = "qcom,msm8916-venus";
>> > @@ -105,4 +107,8 @@ Every of video-encoder or video-decoder subnode should have:
>> >                       clock-names = "core";
>> >                       power-domains = <&mmcc VENUS_CORE1_GDSC>;
>> >               };
>> > +             venus-firmware {
>> > +                     compatible = "qcom,venus-firmware-no-tz";
>> > +                     iommus = <&apps_smmu 0x10b2 0x0>;
>> 
>> This mostly looks like you are adding a node in order to create a
>> platform device. DT is not the only way to create platform devices and
>> shouldn't be used when the device is not really a separate h/w device.
>> Plus it seems like it is debatable that you even need a driver.
>> 
>> For iommus, just move it up to the parent (or add to existing prop).
> 
> As far as I understood the issue from reading this series and also
> talking a bit with Stanimir, there are multiple (physical?) ports from
> the Venus hardware block and that includes one dedicated for firmware
> loading, which has IOVA range restrictions up to 6 MiBs or something
> like that.
> 
> If we add the firmware port to the iommus property of the main node,
> we would bind it to the same IOVA address space as the other ports and
> so it would be part of the main full 32-bit IOMMU domain.

Not really port-wise, but the restriction part is right. Once the 
firmware
is loaded, the ARM9 can only execute those firmware instructions if it 
is
present in iova address 0x0.
Merging it to parent device cannot guarantee that the firmware memory is
mapped from 0x0.

> Best regards,
> Tomasz