[PATCH 0/4] Venus remoteproc driver

[PATCH 0/4] Venus remoteproc driver

[Stanimir Varbanov]

Hi all,

The first patch adds a call to qcom SCM driver which helps to change
the Venus core state, in practice it is used for supend and resume
the video core. Second patch add two SCM calls needed by Venus
remoteproc driver in order to load and authenticate the firmware.

Next two patches add dt binding document and the Venus remoteproc
driver itself.

Comments are welcome.

regards,
Stan

Stanimir Varbanov (4):
  firmware: qcom: scm: add a video command for state setting
  firmware: qcom: scm: add iommu scm calls for pg table
  dt-binding: remoteproc: venus rproc dt binding document
  remoteproc: qcom: add Venus video core firmware loader driver

 .../devicetree/bindings/remoteproc/qcom,venus.txt  |  33 +++
 drivers/firmware/qcom_scm-32.c                     |  18 ++
 drivers/firmware/qcom_scm-64.c                     |  58 ++++++
 drivers/firmware/qcom_scm.c                        |  28 +++
 drivers/firmware/qcom_scm.h                        |  13 ++
 drivers/remoteproc/Kconfig                         |  10 +
 drivers/remoteproc/Makefile                        |   1 +
 drivers/remoteproc/qcom_venus_pil.c                | 226 +++++++++++++++++++++
 include/linux/qcom_scm.h                           |   5 +
 9 files changed, 392 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,venus.txt
 create mode 100644 drivers/remoteproc/qcom_venus_pil.c

-- 
2.7.4

[PATCH 1/4] firmware: qcom: scm: add a video command for state setting

[Stanimir Varbanov]

This scm call is used to change the video core state, more
specifically it is used to suspend and resume the core.

Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
---
 drivers/firmware/qcom_scm-32.c | 18 ++++++++++++++++++
 drivers/firmware/qcom_scm-64.c | 16 ++++++++++++++++
 drivers/firmware/qcom_scm.c    | 16 ++++++++++++++++
 drivers/firmware/qcom_scm.h    |  2 ++
 include/linux/qcom_scm.h       |  2 ++
 5 files changed, 54 insertions(+)

diff --git a/drivers/firmware/qcom_scm-32.c b/drivers/firmware/qcom_scm-32.c
index c6aeedbdcbb0..82c1d8d0d36b 100644
--- a/drivers/firmware/qcom_scm-32.c
+++ b/drivers/firmware/qcom_scm-32.c
@@ -560,3 +560,21 @@ int __qcom_scm_pas_mss_reset(struct device *dev, bool reset)
 
 	return ret ? : le32_to_cpu(out);
 }
+
+int __qcom_scm_video_set_state(struct device *dev, u32 state, u32 spare)
+{
+	struct {
+		__le32 state;
+		__le32 spare;
+	} req;
+	__le32 scm_ret = 0;
+	int ret;
+
+	req.state = cpu_to_le32(state);
+	req.spare = cpu_to_le32(spare);
+
+	ret = qcom_scm_call(dev, QCOM_SCM_SVC_BOOT, QCOM_SCM_VIDEO_SET_STATE,
+			    &req, sizeof(req), &scm_ret, sizeof(scm_ret));
+
+	return ret ? : le32_to_cpu(scm_ret);
+}
diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c
index 4a0f5ead4fb5..68484ea2aa51 100644
--- a/drivers/firmware/qcom_scm-64.c
+++ b/drivers/firmware/qcom_scm-64.c
@@ -358,3 +358,19 @@ int __qcom_scm_pas_mss_reset(struct device *dev, bool reset)
 
 	return ret ? : res.a1;
 }
+
+int __qcom_scm_video_set_state(struct device *dev, u32 state, u32 spare)
+{
+	struct qcom_scm_desc desc = {0};
+	struct arm_smccc_res res;
+	int ret;
+
+	desc.args[0] = state;
+	desc.args[1] = spare;
+	desc.arginfo = QCOM_SCM_ARGS(2);
+
+	ret = qcom_scm_call(dev, QCOM_SCM_SVC_BOOT, QCOM_SCM_VIDEO_SET_STATE,
+			    &desc, &res);
+
+	return ret ? : res.a1;
+}
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index e64a501adbf4..3b0e31c48639 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -317,6 +317,22 @@ bool qcom_scm_is_available(void)
 }
 EXPORT_SYMBOL(qcom_scm_is_available);
 
+int qcom_scm_video_set_state(u32 state, u32 spare)
+{
+	int ret;
+
+	ret = qcom_scm_clk_enable();
+	if (ret)
+		return ret;
+
+	ret = __qcom_scm_video_set_state(__scm->dev, state, spare);
+
+	qcom_scm_clk_disable();
+
+	return ret;
+}
+EXPORT_SYMBOL(qcom_scm_video_set_state);
+
 static int qcom_scm_probe(struct platform_device *pdev)
 {
 	struct qcom_scm *scm;
diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
index 3584b00fe7e6..4830559b2639 100644
--- a/drivers/firmware/qcom_scm.h
+++ b/drivers/firmware/qcom_scm.h
@@ -15,6 +15,8 @@
 #define QCOM_SCM_SVC_BOOT		0x1
 #define QCOM_SCM_BOOT_ADDR		0x1
 #define QCOM_SCM_BOOT_ADDR_MC		0x11
+#define QCOM_SCM_VIDEO_SET_STATE	0xa
+extern int __qcom_scm_video_set_state(struct device *dev, u32 state, u32 spare);
 
 #define QCOM_SCM_FLAG_HLOS		0x01
 #define QCOM_SCM_FLAG_COLDBOOT_MC	0x02
diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h
index cc32ab852fbc..2ece81a6b078 100644
--- a/include/linux/qcom_scm.h
+++ b/include/linux/qcom_scm.h
@@ -46,4 +46,6 @@ extern void qcom_scm_cpu_power_down(u32 flags);
 
 extern u32 qcom_scm_get_version(void);
 
+extern int qcom_scm_video_set_state(u32 state, u32 spare);
+
 #endif
-- 
2.7.4

[PATCH 2/4] firmware: qcom: scm: add iommu scm calls for pg table

[Stanimir Varbanov]

Those two scm calls are used to get the size of secure iommu
page table and to pass physical memory address for this page
table. The calls are used by remoteproc venus driver to load
the firmware.

Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
---
 drivers/firmware/qcom_scm-64.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 drivers/firmware/qcom_scm.c    | 12 ++++++++++++
 drivers/firmware/qcom_scm.h    | 11 +++++++++++
 include/linux/qcom_scm.h       |  3 +++
 4 files changed, 68 insertions(+)

diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c
index 68484ea2aa51..ffcfbb31ae7a 100644
--- a/drivers/firmware/qcom_scm-64.c
+++ b/drivers/firmware/qcom_scm-64.c
@@ -374,3 +374,45 @@ int __qcom_scm_video_set_state(struct device *dev, u32 state, u32 spare)
 
 	return ret ? : res.a1;
 }
+
+int __qcom_scm_iommu_secure_ptbl_size(struct device *dev, u32 spare,
+				      size_t *size)
+{
+	struct qcom_scm_desc desc = {0};
+	struct arm_smccc_res res;
+	int ret;
+
+	desc.args[0] = spare;
+	desc.arginfo = QCOM_SCM_ARGS(1);
+
+	ret = qcom_scm_call(dev, QCOM_SCM_SVC_MP,
+			    QCOM_SCM_IOMMU_SECURE_PTBL_SIZE, &desc, &res);
+
+	if (size)
+		*size = res.a1;
+
+	return ret ? : res.a2;
+}
+
+int __qcom_scm_iommu_secure_ptbl_init(struct device *dev, u64 addr, u32 size,
+				      u32 spare)
+{
+	struct qcom_scm_desc desc = {0};
+	struct arm_smccc_res res;
+	int ret;
+
+	desc.args[0] = addr;
+	desc.args[1] = size;
+	desc.args[2] = spare;
+	desc.arginfo = QCOM_SCM_ARGS(3, QCOM_SCM_RW, QCOM_SCM_VAL,
+				     QCOM_SCM_VAL);
+
+	ret = qcom_scm_call(dev, QCOM_SCM_SVC_MP,
+			    QCOM_SCM_IOMMU_SECURE_PTBL_INIT, &desc, &res);
+
+	/* the pg table has been initialized already, ignore the error */
+	if (ret == -EPERM)
+		ret = 0;
+
+	return ret;
+}
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 3b0e31c48639..aa77d31b885c 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -333,6 +333,18 @@ int qcom_scm_video_set_state(u32 state, u32 spare)
 }
 EXPORT_SYMBOL(qcom_scm_video_set_state);
 
+int qcom_scm_iommu_secure_ptbl_size(u32 spare, size_t *size)
+{
+	return __qcom_scm_iommu_secure_ptbl_size(__scm->dev, spare, size);
+}
+EXPORT_SYMBOL(qcom_scm_iommu_secure_ptbl_size);
+
+int qcom_scm_iommu_secure_ptbl_init(u64 addr, u32 size, u32 spare)
+{
+	return __qcom_scm_iommu_secure_ptbl_init(__scm->dev, addr, size, spare);
+}
+EXPORT_SYMBOL(qcom_scm_iommu_secure_ptbl_init);
+
 static int qcom_scm_probe(struct platform_device *pdev)
 {
 	struct qcom_scm *scm;
diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
index 4830559b2639..edeb0038e5fc 100644
--- a/drivers/firmware/qcom_scm.h
+++ b/drivers/firmware/qcom_scm.h
@@ -58,8 +58,17 @@ extern int  __qcom_scm_pas_auth_and_reset(struct device *dev, u32 peripheral);
 extern int  __qcom_scm_pas_shutdown(struct device *dev, u32 peripheral);
 extern int  __qcom_scm_pas_mss_reset(struct device *dev, bool reset);
 
+#define QCOM_SCM_SVC_MP			0xc
+#define QCOM_SCM_IOMMU_SECURE_PTBL_SIZE	3
+#define QCOM_SCM_IOMMU_SECURE_PTBL_INIT	4
+extern int __qcom_scm_iommu_secure_ptbl_size(struct device *dev, u32 spare,
+					     size_t *size);
+extern int __qcom_scm_iommu_secure_ptbl_init(struct device *dev, u64 addr,
+					     u32 size, u32 spare);
+
 /* common error codes */
 #define QCOM_SCM_V2_EBUSY	-12
+#define QCOM_SCM_NOT_PERMITTED	-8
 #define QCOM_SCM_ENOMEM		-5
 #define QCOM_SCM_EOPNOTSUPP	-4
 #define QCOM_SCM_EINVAL_ADDR	-3
@@ -81,6 +90,8 @@ static inline int qcom_scm_remap_error(int err)
 		return -ENOMEM;
 	case QCOM_SCM_V2_EBUSY:
 		return -EBUSY;
+	case QCOM_SCM_NOT_PERMITTED:
+		return -EPERM;
 	}
 	return -EINVAL;
 }
diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h
index 2ece81a6b078..a8fb98c36cce 100644
--- a/include/linux/qcom_scm.h
+++ b/include/linux/qcom_scm.h
@@ -48,4 +48,7 @@ extern u32 qcom_scm_get_version(void);
 
 extern int qcom_scm_video_set_state(u32 state, u32 spare);
 
+extern int qcom_scm_iommu_secure_ptbl_size(u32 spare, size_t *size);
+extern int qcom_scm_iommu_secure_ptbl_init(u64 addr, u32 size, u32 spare);
+
 #endif
-- 
2.7.4

Re: [PATCH 2/4] firmware: qcom: scm: add iommu scm calls for pg table

[kbuild test robot]

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

Hi Stanimir,

[auto build test ERROR on next-20160819]
[also build test ERROR on v4.8-rc3]
[cannot apply to linus/master linux/master remoteproc/for-next v4.8-rc2 v4.8-rc1 v4.7-rc7]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
[Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on]
[Check https://git-scm.com/docs/git-format-patch for more information]

url:    https://github.com/0day-ci/linux/commits/Stanimir-Varbanov/firmware-qcom-scm-add-a-video-command-for-state-setting/20160820-002016
config: arm-multi_v7_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 5.4.0-6) 5.4.0 20160609
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm 

All errors (new ones prefixed by >>):

   drivers/built-in.o: In function `qcom_scm_iommu_secure_ptbl_size':
>> sunxi_sid.c:(.text+0x462958): undefined reference to `__qcom_scm_iommu_secure_ptbl_size'
   drivers/built-in.o: In function `qcom_scm_iommu_secure_ptbl_init':
>> sunxi_sid.c:(.text+0x462984): undefined reference to `__qcom_scm_iommu_secure_ptbl_init'

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

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 38719 bytes --]

Re: [PATCH 2/4] firmware: qcom: scm: add iommu scm calls for pg table

[Gupta, Puja]

On 8/19/2016 8:53 AM, Stanimir Varbanov wrote:
> Those two scm calls are used to get the size of secure iommu
> page table and to pass physical memory address for this page
> table. The calls are used by remoteproc venus driver to load
> the firmware.
Do we really need these additional scm calls for venus? why can't we 
just reuse existing __qcom_scm_pas_mem_setup() call?
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> ---
>   drivers/firmware/qcom_scm-64.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>   drivers/firmware/qcom_scm.c    | 12 ++++++++++++
>   drivers/firmware/qcom_scm.h    | 11 +++++++++++
>   include/linux/qcom_scm.h       |  3 +++
>   4 files changed, 68 insertions(+)
>
> diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c
> index 68484ea2aa51..ffcfbb31ae7a 100644
> --- a/drivers/firmware/qcom_scm-64.c
> +++ b/drivers/firmware/qcom_scm-64.c
> @@ -374,3 +374,45 @@ int __qcom_scm_video_set_state(struct device *dev, u32 state, u32 spare)
>   
>   	return ret ? : res.a1;
>   }
> +
> +int __qcom_scm_iommu_secure_ptbl_size(struct device *dev, u32 spare,
> +				      size_t *size)
> +{
> +	struct qcom_scm_desc desc = {0};
> +	struct arm_smccc_res res;
> +	int ret;
> +
> +	desc.args[0] = spare;
> +	desc.arginfo = QCOM_SCM_ARGS(1);
> +
> +	ret = qcom_scm_call(dev, QCOM_SCM_SVC_MP,
> +			    QCOM_SCM_IOMMU_SECURE_PTBL_SIZE, &desc, &res);
> +
> +	if (size)
> +		*size = res.a1;
> +
> +	return ret ? : res.a2;
> +}
> +
> +int __qcom_scm_iommu_secure_ptbl_init(struct device *dev, u64 addr, u32 size,
> +				      u32 spare)
> +{
> +	struct qcom_scm_desc desc = {0};
> +	struct arm_smccc_res res;
> +	int ret;
> +
> +	desc.args[0] = addr;
> +	desc.args[1] = size;
> +	desc.args[2] = spare;
> +	desc.arginfo = QCOM_SCM_ARGS(3, QCOM_SCM_RW, QCOM_SCM_VAL,
> +				     QCOM_SCM_VAL);
> +
> +	ret = qcom_scm_call(dev, QCOM_SCM_SVC_MP,
> +			    QCOM_SCM_IOMMU_SECURE_PTBL_INIT, &desc, &res);
> +
> +	/* the pg table has been initialized already, ignore the error */
> +	if (ret == -EPERM)
> +		ret = 0;
> +
> +	return ret;
> +}
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index 3b0e31c48639..aa77d31b885c 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -333,6 +333,18 @@ int qcom_scm_video_set_state(u32 state, u32 spare)
>   }
>   EXPORT_SYMBOL(qcom_scm_video_set_state);
>   
> +int qcom_scm_iommu_secure_ptbl_size(u32 spare, size_t *size)
> +{
> +	return __qcom_scm_iommu_secure_ptbl_size(__scm->dev, spare, size);
> +}
> +EXPORT_SYMBOL(qcom_scm_iommu_secure_ptbl_size);
> +
> +int qcom_scm_iommu_secure_ptbl_init(u64 addr, u32 size, u32 spare)
> +{
> +	return __qcom_scm_iommu_secure_ptbl_init(__scm->dev, addr, size, spare);
> +}
> +EXPORT_SYMBOL(qcom_scm_iommu_secure_ptbl_init);
> +
>   static int qcom_scm_probe(struct platform_device *pdev)
>   {
>   	struct qcom_scm *scm;
> diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
> index 4830559b2639..edeb0038e5fc 100644
> --- a/drivers/firmware/qcom_scm.h
> +++ b/drivers/firmware/qcom_scm.h
> @@ -58,8 +58,17 @@ extern int  __qcom_scm_pas_auth_and_reset(struct device *dev, u32 peripheral);
>   extern int  __qcom_scm_pas_shutdown(struct device *dev, u32 peripheral);
>   extern int  __qcom_scm_pas_mss_reset(struct device *dev, bool reset);
>   
> +#define QCOM_SCM_SVC_MP			0xc
> +#define QCOM_SCM_IOMMU_SECURE_PTBL_SIZE	3
> +#define QCOM_SCM_IOMMU_SECURE_PTBL_INIT	4
> +extern int __qcom_scm_iommu_secure_ptbl_size(struct device *dev, u32 spare,
> +					     size_t *size);
> +extern int __qcom_scm_iommu_secure_ptbl_init(struct device *dev, u64 addr,
> +					     u32 size, u32 spare);
> +
>   /* common error codes */
>   #define QCOM_SCM_V2_EBUSY	-12
> +#define QCOM_SCM_NOT_PERMITTED	-8
>   #define QCOM_SCM_ENOMEM		-5
>   #define QCOM_SCM_EOPNOTSUPP	-4
>   #define QCOM_SCM_EINVAL_ADDR	-3
> @@ -81,6 +90,8 @@ static inline int qcom_scm_remap_error(int err)
>   		return -ENOMEM;
>   	case QCOM_SCM_V2_EBUSY:
>   		return -EBUSY;
> +	case QCOM_SCM_NOT_PERMITTED:
> +		return -EPERM;
>   	}
>   	return -EINVAL;
>   }
> diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h
> index 2ece81a6b078..a8fb98c36cce 100644
> --- a/include/linux/qcom_scm.h
> +++ b/include/linux/qcom_scm.h
> @@ -48,4 +48,7 @@ extern u32 qcom_scm_get_version(void);
>   
>   extern int qcom_scm_video_set_state(u32 state, u32 spare);
>   
> +extern int qcom_scm_iommu_secure_ptbl_size(u32 spare, size_t *size);
> +extern int qcom_scm_iommu_secure_ptbl_init(u64 addr, u32 size, u32 spare);
> +
>   #endif
Puja

Re: [PATCH 2/4] firmware: qcom: scm: add iommu scm calls for pg table

[Stanimir Varbanov]

Hi Puja,

On 08/24/2016 09:35 PM, Gupta, Puja wrote:
> On 8/19/2016 8:53 AM, Stanimir Varbanov wrote:
>> Those two scm calls are used to get the size of secure iommu
>> page table and to pass physical memory address for this page
>> table. The calls are used by remoteproc venus driver to load
>> the firmware.

> Do we really need these additional scm calls for venus? why can't we
> just reuse existing __qcom_scm_pas_mem_setup() call?

We are using __qcom_scm_pas_mem_setup() but it will return error if I
did not provide page table memory. At least for apq8016 this step looks
like is mandatory.

Do you have some idea how those iommu calls can be avoided?

-- 
regards,
Stan

Re: [PATCH 2/4] firmware: qcom: scm: add iommu scm calls for pg table

[Andy Gross]

On Wed, Aug 24, 2016 at 11:35:29AM -0700, Gupta, Puja wrote:
> On 8/19/2016 8:53 AM, Stanimir Varbanov wrote:
> >Those two scm calls are used to get the size of secure iommu
> >page table and to pass physical memory address for this page
> >table. The calls are used by remoteproc venus driver to load
> >the firmware.
> Do we really need these additional scm calls for venus? why can't we just
> reuse existing __qcom_scm_pas_mem_setup() call?

Keep in mind this is with the smmu-v1 on 8916.  It seems like the downstream
kernel has to do extra stuff when dealing with the older smmu.  The newer
platforms handle this quasi-transparently.

Regards,

Andy

[PATCH 3/4] dt-binding: remoteproc: venus rproc dt binding document

[Stanimir Varbanov]

Add devicetree binding document for Venus remote processor.

Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
---
 .../devicetree/bindings/remoteproc/qcom,venus.txt  | 33 ++++++++++++++++++++++
 1 file changed, 33 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,venus.txt

diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt b/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt
new file mode 100644
index 000000000000..06a2db60fa38
--- /dev/null
+++ b/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt
@@ -0,0 +1,33 @@
+Qualcomm Venus Peripheral Image Loader
+
+This document defines the binding for a component that loads and boots firmware
+on the Qualcomm Venus remote processor core.
+
+- compatible:
+	Usage: required
+	Value type: <string>
+	Definition: must contain "qcom,venus-pil"
+
+- memory-region:
+	Usage: required
+	Value type: <phandle>
+	Definition: a phandle to a node describing reserved memory
+
+* An example
+	reserved-memory {
+		#address-cells = <2>;
+		#size-cells = <2>;
+		ranges;
+
+		venus_mem: venus@89900000 {
+			compatible = "shared-dma-pool";
+			reg = <0x0 0x89900000 0x0 0x800000>;
+			alignment = <0x1000>;
+			no-map;
+		};
+	};
+
+	rproc_venus@0 {
+		compatible = "qcom,venus-pil";
+		memory-region = <&venus_mem>;
+	};
\ No newline at end of file
-- 
2.7.4

Re: [PATCH 3/4] dt-binding: remoteproc: venus rproc dt binding document

[Rob Herring]

On Fri, Aug 19, 2016 at 06:53:19PM +0300, Stanimir Varbanov wrote:
> Add devicetree binding document for Venus remote processor.
> 
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> ---
>  .../devicetree/bindings/remoteproc/qcom,venus.txt  | 33 ++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,venus.txt
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt b/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt
> new file mode 100644
> index 000000000000..06a2db60fa38
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt
> @@ -0,0 +1,33 @@
> +Qualcomm Venus Peripheral Image Loader
> +
> +This document defines the binding for a component that loads and boots firmware
> +on the Qualcomm Venus remote processor core.

This does not make sense to me. Venus is the video encoder/decoder h/w, 
right? Why is the firmware loader separate from the codec block? Why 
rproc is used? Are there multiple clients? Naming it rproc_venus implies 
there aren't. And why does the firmware loading need 8MB of memory at a 
fixed address?

> +
> +- compatible:
> +	Usage: required
> +	Value type: <string>
> +	Definition: must contain "qcom,venus-pil"
> +
> +- memory-region:
> +	Usage: required
> +	Value type: <phandle>
> +	Definition: a phandle to a node describing reserved memory
> +
> +* An example
> +	reserved-memory {
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges;
> +
> +		venus_mem: venus@89900000 {
> +			compatible = "shared-dma-pool";
> +			reg = <0x0 0x89900000 0x0 0x800000>;
> +			alignment = <0x1000>;
> +			no-map;
> +		};
> +	};
> +
> +	rproc_venus@0 {
> +		compatible = "qcom,venus-pil";
> +		memory-region = <&venus_mem>;
> +	};
> \ No newline at end of file
> -- 
> 2.7.4
> 

Re: [PATCH 3/4] dt-binding: remoteproc: venus rproc dt binding document

[Bjorn Andersson]

On Tue 23 Aug 10:32 PDT 2016, Rob Herring wrote:

> On Fri, Aug 19, 2016 at 06:53:19PM +0300, Stanimir Varbanov wrote:
> > Add devicetree binding document for Venus remote processor.
> > 
> > Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> > ---
> >  .../devicetree/bindings/remoteproc/qcom,venus.txt  | 33 ++++++++++++++++++++++
> >  1 file changed, 33 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,venus.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt b/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt
> > new file mode 100644
> > index 000000000000..06a2db60fa38
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt
> > @@ -0,0 +1,33 @@
> > +Qualcomm Venus Peripheral Image Loader
> > +
> > +This document defines the binding for a component that loads and boots firmware
> > +on the Qualcomm Venus remote processor core.
> 
> This does not make sense to me. Venus is the video encoder/decoder h/w, 
> right?

Yes, Venus is an ARM based thing doing video encoding and decoding.

> Why is the firmware loader separate from the codec block?
> Why rproc is used?

Implementation wise it shares structure and almost all the logic with
other remoteprocs in the Qualcomm platform; you load firmware into a
memory region, you grab a few resources (clocks, regulators,
power-domains), you jump into TrustZone for signature checks and you
release the resources as the remote is booted and have voted for these
with the RPM.

But there is also a second operation mode, where one of the Hexagon DSPs
"imitates" a Venus core; with slightly different transport mechanism for
transferring the command stream - so the Venus node might operate on a
non-Venus hardware.


That said, the Venus node (in Venus-hw mode) has a 1:1 life cycle with
the power-on-state of the remoteproc. So perhaps we should describe the
two parts in one DT node and have the rproc-venus implementation spawn
the v4l driver when the remote is running...

But that would mean that on a 8064 we would have 5-6 nodes describing
standalone remoteprocs and one describing the exact same thing but in a
completely different way.


If we keep it as two nodes, I think it would be better to describe the
video-part as a child of the venus-rproc; to show the link between the
two parts.

> Are there multiple clients?
> Naming it rproc_venus implies there aren't.

I'm still investigating this, but it looks like rproc part of the
8060/8960/8064 "vidc" is very similar.

> And why does the firmware loading need 8MB of memory at a fixed address?
> 

On msm8974 the Venus should be loaded into a 5MB region with a fixed
address, perhaps just because of some memory budgeting document. On 8916
it looks (downstream) like all we need is the size and it can be
positioned wherever.

But I would say this is not a property of the rproc-venus, but rather
about system configuration and the firmware. As such I think we should
omit the memory reserve from the example and make sure the
implementation can deal with either a fixed or only-sized reserved
memory region.

Regards,
Bjorn

Re: [PATCH 3/4] dt-binding: remoteproc: venus rproc dt binding document

[Stanimir Varbanov]

Hi Rob,

On 08/23/2016 08:32 PM, Rob Herring wrote:
> On Fri, Aug 19, 2016 at 06:53:19PM +0300, Stanimir Varbanov wrote:
>> Add devicetree binding document for Venus remote processor.
>>
>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
>> ---
>>  .../devicetree/bindings/remoteproc/qcom,venus.txt  | 33 ++++++++++++++++++++++
>>  1 file changed, 33 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,venus.txt
>>
>> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt b/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt
>> new file mode 100644
>> index 000000000000..06a2db60fa38
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt
>> @@ -0,0 +1,33 @@
>> +Qualcomm Venus Peripheral Image Loader
>> +
>> +This document defines the binding for a component that loads and boots firmware
>> +on the Qualcomm Venus remote processor core.
> 
> This does not make sense to me. Venus is the video encoder/decoder h/w, 
> right? Why is the firmware loader separate from the codec block? Why 
> rproc is used? Are there multiple clients? Naming it rproc_venus implies 
> there aren't. And why does the firmware loading need 8MB of memory at a 
> fixed address?
> 

The firmware for Venus case is 5MB. And here is 8MB because of
dma_alloc_from_coherent size restriction.

The address is not really fixed, cause the firmware could support
relocation. In this example I just picked up the next free memory region
in memory-reserved from msm8916.dtsi.

regards,
Stan

Re: [PATCH 3/4] dt-binding: remoteproc: venus rproc dt binding document

[Bjorn Andersson]

On Wed 24 Aug 08:36 PDT 2016, Stanimir Varbanov wrote:

> Hi Rob,
> 
> On 08/23/2016 08:32 PM, Rob Herring wrote:
> > On Fri, Aug 19, 2016 at 06:53:19PM +0300, Stanimir Varbanov wrote:
> >> Add devicetree binding document for Venus remote processor.
> >>
> >> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> >> ---
> >>  .../devicetree/bindings/remoteproc/qcom,venus.txt  | 33 ++++++++++++++++++++++
> >>  1 file changed, 33 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,venus.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt b/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt
> >> new file mode 100644
> >> index 000000000000..06a2db60fa38
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt
> >> @@ -0,0 +1,33 @@
> >> +Qualcomm Venus Peripheral Image Loader
> >> +
> >> +This document defines the binding for a component that loads and boots firmware
> >> +on the Qualcomm Venus remote processor core.
> > 
> > This does not make sense to me. Venus is the video encoder/decoder h/w, 
> > right? Why is the firmware loader separate from the codec block? Why 
> > rproc is used? Are there multiple clients? Naming it rproc_venus implies 
> > there aren't. And why does the firmware loading need 8MB of memory at a 
> > fixed address?
> > 
> 
> The firmware for Venus case is 5MB. And here is 8MB because of
> dma_alloc_from_coherent size restriction.
> 

Then you should specify it 5MB large and we'll have to deal with this
implementation issue in the code. I've created a JIRA ticket for
the dma_alloc_from_coherent() behavior.

> The address is not really fixed, cause the firmware could support
> relocation. In this example I just picked up the next free memory region
> in memory-reserved from msm8916.dtsi.
> 

In 8974 we do have a physical region where it's expected to be loaded.

So in line with upcoming remoteproc work we should support referencing a
reserved-memory node with either reg or size.

In the case of spotting a "reg" we're currently better off using
ioremap. We're looking at getting the remoteproc core to deal with this
mess.


So, on 8916 I think you should use the form:

venus_mem: venus {
	size = <0x500000>;
};

And I don't think you should use the shared-dma-pool compatible, because
this is not a region for multiple devices to allocate dma memory out of.


Regards,
Bjorn

Re: [PATCH 3/4] dt-binding: remoteproc: venus rproc dt binding document

[Stanimir Varbanov]

Hi Bjorn,

On 08/25/2016 03:05 AM, Bjorn Andersson wrote:
> On Wed 24 Aug 08:36 PDT 2016, Stanimir Varbanov wrote:
> 
>> Hi Rob,
>>
>> On 08/23/2016 08:32 PM, Rob Herring wrote:
>>> On Fri, Aug 19, 2016 at 06:53:19PM +0300, Stanimir Varbanov wrote:
>>>> Add devicetree binding document for Venus remote processor.
>>>>
>>>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
>>>> ---
>>>>  .../devicetree/bindings/remoteproc/qcom,venus.txt  | 33 ++++++++++++++++++++++
>>>>  1 file changed, 33 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,venus.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt b/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt
>>>> new file mode 100644
>>>> index 000000000000..06a2db60fa38
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt
>>>> @@ -0,0 +1,33 @@
>>>> +Qualcomm Venus Peripheral Image Loader
>>>> +
>>>> +This document defines the binding for a component that loads and boots firmware
>>>> +on the Qualcomm Venus remote processor core.
>>>
>>> This does not make sense to me. Venus is the video encoder/decoder h/w, 
>>> right? Why is the firmware loader separate from the codec block? Why 
>>> rproc is used? Are there multiple clients? Naming it rproc_venus implies 
>>> there aren't. And why does the firmware loading need 8MB of memory at a 
>>> fixed address?
>>>
>>
>> The firmware for Venus case is 5MB. And here is 8MB because of
>> dma_alloc_from_coherent size restriction.
>>
> 
> Then you should specify it 5MB large and we'll have to deal with this
> implementation issue in the code. I've created a JIRA ticket for
> the dma_alloc_from_coherent() behavior.

Infact it should be 5MB + ~100KB for iommu page table.

> 
>> The address is not really fixed, cause the firmware could support
>> relocation. In this example I just picked up the next free memory region
>> in memory-reserved from msm8916.dtsi.
>>
> 
> In 8974 we do have a physical region where it's expected to be loaded.
> 
> So in line with upcoming remoteproc work we should support referencing a
> reserved-memory node with either reg or size.
> 
> In the case of spotting a "reg" we're currently better off using
> ioremap. We're looking at getting the remoteproc core to deal with this
> mess.

You mean that remoteproc core will parse memory-region property?

> 
> 
> So, on 8916 I think you should use the form:
> 
> venus_mem: venus {
> 	size = <0x500000>;
> };

Don't forget that the physical address where the firmware is stored has
some range, the scm call will fail if it is out of the expected range,
probably because of some security reasons. So maybe alloc-ranges should
be specified here.

> 
> And I don't think you should use the shared-dma-pool compatible, because
> this is not a region for multiple devices to allocate dma memory out of.

Then I cannot reuse reserved-mem infrastructure.

-- 
regards,
Stan

Re: [PATCH 3/4] dt-binding: remoteproc: venus rproc dt binding document

[Bjorn Andersson]

On Thu 25 Aug 04:10 PDT 2016, Stanimir Varbanov wrote:

> Hi Bjorn,
> 
> On 08/25/2016 03:05 AM, Bjorn Andersson wrote:
> > On Wed 24 Aug 08:36 PDT 2016, Stanimir Varbanov wrote:
> > 
> >> Hi Rob,
> >>
> >> On 08/23/2016 08:32 PM, Rob Herring wrote:
> >>> On Fri, Aug 19, 2016 at 06:53:19PM +0300, Stanimir Varbanov wrote:
> >>>> Add devicetree binding document for Venus remote processor.
> >>>>
> >>>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> >>>> ---
> >>>>  .../devicetree/bindings/remoteproc/qcom,venus.txt  | 33 ++++++++++++++++++++++
> >>>>  1 file changed, 33 insertions(+)
> >>>>  create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,venus.txt
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt b/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt
> >>>> new file mode 100644
> >>>> index 000000000000..06a2db60fa38
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt
> >>>> @@ -0,0 +1,33 @@
> >>>> +Qualcomm Venus Peripheral Image Loader
> >>>> +
> >>>> +This document defines the binding for a component that loads and boots firmware
> >>>> +on the Qualcomm Venus remote processor core.
> >>>
> >>> This does not make sense to me. Venus is the video encoder/decoder h/w, 
> >>> right? Why is the firmware loader separate from the codec block? Why 
> >>> rproc is used? Are there multiple clients? Naming it rproc_venus implies 
> >>> there aren't. And why does the firmware loading need 8MB of memory at a 
> >>> fixed address?
> >>>
> >>
> >> The firmware for Venus case is 5MB. And here is 8MB because of
> >> dma_alloc_from_coherent size restriction.
> >>
> > 
> > Then you should specify it 5MB large and we'll have to deal with this
> > implementation issue in the code. I've created a JIRA ticket for
> > the dma_alloc_from_coherent() behavior.
> 
> Infact it should be 5MB + ~100KB for iommu page table.
> 

Trying to wrap my head around how the iommu part works here. The
downstream code seems to indicate that this is a "generic" secure iommu
interface - used by venus, camera and kgsl; likely for dealing with DRM
protected buffers.

As such the iommu tables are not part of the venus rproc; I believe they
should either be tied into the msm-iommu driver or perhaps exposed as
its own iommu(?).


But I presume from your inclusion that you've concluded that the venus
firmware we have refuses to execute without these tables at least
initialized, is this correct?

> > 
> >> The address is not really fixed, cause the firmware could support
> >> relocation. In this example I just picked up the next free memory region
> >> in memory-reserved from msm8916.dtsi.
> >>
> > 
> > In 8974 we do have a physical region where it's expected to be loaded.
> > 
> > So in line with upcoming remoteproc work we should support referencing a
> > reserved-memory node with either reg or size.
> > 
> > In the case of spotting a "reg" we're currently better off using
> > ioremap. We're looking at getting the remoteproc core to deal with this
> > mess.
> 
> You mean that remoteproc core will parse memory-region property?
> 

It has to, because it's a quite common scenario for remoteproc drivers
to either get its backing memory from a static region or be restricted
to part of system ram - properties that reserved-memory and
memory-region captures already.

> > 
> > 
> > So, on 8916 I think you should use the form:
> > 
> > venus_mem: venus {
> > 	size = <0x500000>;
> > };
> 
> Don't forget that the physical address where the firmware is stored has
> some range, the scm call will fail if it is out of the expected range,
> probably because of some security reasons. So maybe alloc-ranges should
> be specified here.
> 

Thanks for highlighting this.

> > 
> > And I don't think you should use the shared-dma-pool compatible, because
> > this is not a region for multiple devices to allocate dma memory out of.
> 
> Then I cannot reuse reserved-mem infrastructure.
> 

You're right. If I understand the code correctly we need to use the
compatible shared-dma-pool and mark it either "no-map" or "reusable", to
be able to use dma_alloc_coherent().


But I presume we have the implementation issue of dma_alloc_coherent()
failing in either case with the 5MB size. I think we need to look into
that - and have created a JIRA ticket for it.

Regards,
Bjorn

Re: [PATCH 3/4] dt-binding: remoteproc: venus rproc dt binding document

[Stanimir Varbanov]

Hi,

On 08/27/2016 01:23 AM, Bjorn Andersson wrote:
> On Thu 25 Aug 04:10 PDT 2016, Stanimir Varbanov wrote:
> 
>> Hi Bjorn,
>>
>> On 08/25/2016 03:05 AM, Bjorn Andersson wrote:
>>> On Wed 24 Aug 08:36 PDT 2016, Stanimir Varbanov wrote:
>>>
>>>> Hi Rob,
>>>>
>>>> On 08/23/2016 08:32 PM, Rob Herring wrote:
>>>>> On Fri, Aug 19, 2016 at 06:53:19PM +0300, Stanimir Varbanov wrote:
>>>>>> Add devicetree binding document for Venus remote processor.
>>>>>>
>>>>>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
>>>>>> ---
>>>>>>  .../devicetree/bindings/remoteproc/qcom,venus.txt  | 33 ++++++++++++++++++++++
>>>>>>  1 file changed, 33 insertions(+)
>>>>>>  create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,venus.txt
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt b/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt
>>>>>> new file mode 100644
>>>>>> index 000000000000..06a2db60fa38
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt
>>>>>> @@ -0,0 +1,33 @@
>>>>>> +Qualcomm Venus Peripheral Image Loader
>>>>>> +
>>>>>> +This document defines the binding for a component that loads and boots firmware
>>>>>> +on the Qualcomm Venus remote processor core.
>>>>>
>>>>> This does not make sense to me. Venus is the video encoder/decoder h/w, 
>>>>> right? Why is the firmware loader separate from the codec block? Why 
>>>>> rproc is used? Are there multiple clients? Naming it rproc_venus implies 
>>>>> there aren't. And why does the firmware loading need 8MB of memory at a 
>>>>> fixed address?
>>>>>
>>>>
>>>> The firmware for Venus case is 5MB. And here is 8MB because of
>>>> dma_alloc_from_coherent size restriction.
>>>>
>>>
>>> Then you should specify it 5MB large and we'll have to deal with this
>>> implementation issue in the code. I've created a JIRA ticket for
>>> the dma_alloc_from_coherent() behavior.
>>
>> Infact it should be 5MB + ~100KB for iommu page table.
>>
> 
> Trying to wrap my head around how the iommu part works here. The
> downstream code seems to indicate that this is a "generic" secure iommu
> interface - used by venus, camera and kgsl; likely for dealing with DRM
> protected buffers.

The secure iommu interface is for content protected buffers. But these
secure iommu contexts aren't used by msm DRM nor Venus in mainline. In
Venus case I use non-secure iommu context for data buffers.

> 
> As such the iommu tables are not part of the venus rproc; I believe they
> should either be tied into the msm-iommu driver or perhaps exposed as
> its own iommu(?).

The page tables are in msm-iommu driver.

> 
> 
> But I presume from your inclusion that you've concluded that the venus
> firmware we have refuses to execute without these tables at least
> initialized, is this correct?

Yes, the SMC call for PAS memory-setup will fail if this page table is
not initialized.

> 
>>>
>>>> The address is not really fixed, cause the firmware could support
>>>> relocation. In this example I just picked up the next free memory region
>>>> in memory-reserved from msm8916.dtsi.
>>>>
>>>
>>> In 8974 we do have a physical region where it's expected to be loaded.
>>>
>>> So in line with upcoming remoteproc work we should support referencing a
>>> reserved-memory node with either reg or size.
>>>
>>> In the case of spotting a "reg" we're currently better off using
>>> ioremap. We're looking at getting the remoteproc core to deal with this
>>> mess.
>>
>> You mean that remoteproc core will parse memory-region property?
>>
> 
> It has to, because it's a quite common scenario for remoteproc drivers
> to either get its backing memory from a static region or be restricted
> to part of system ram - properties that reserved-memory and
> memory-region captures already.

OK, I have no issues with that. My concern is the manual parsing of
'memory-region' and 'reg' properties in remoteproc core.

So that idea is to have generic binding for rproc, that would be good.

> 
>>>
>>>
>>> So, on 8916 I think you should use the form:
>>>
>>> venus_mem: venus {
>>> 	size = <0x500000>;
>>> };
>>
>> Don't forget that the physical address where the firmware is stored has
>> some range, the scm call will fail if it is out of the expected range,
>> probably because of some security reasons. So maybe alloc-ranges should
>> be specified here.
>>
> 
> Thanks for highlighting this.
> 
>>>
>>> And I don't think you should use the shared-dma-pool compatible, because
>>> this is not a region for multiple devices to allocate dma memory out of.
>>
>> Then I cannot reuse reserved-mem infrastructure.
>>
> 
> You're right. If I understand the code correctly we need to use the
> compatible shared-dma-pool and mark it either "no-map" or "reusable", to
> be able to use dma_alloc_coherent().

correct.

> 
> 
> But I presume we have the implementation issue of dma_alloc_coherent()
> failing in either case with the 5MB size. I think we need to look into

I'd be good to include Marek Szyprowski? At least he will know what
design restrictions there are.

> that - and have created a JIRA ticket for it.
> 
> Regards,
> Bjorn
> 

-- 
regards,
Stan

Re: [PATCH 3/4] dt-binding: remoteproc: venus rproc dt binding document

[Bjorn Andersson]

On Mon 29 Aug 04:48 PDT 2016, Stanimir Varbanov wrote:

[..]
> > Trying to wrap my head around how the iommu part works here. The
> > downstream code seems to indicate that this is a "generic" secure iommu
> > interface - used by venus, camera and kgsl; likely for dealing with DRM
> > protected buffers.
> 
> The secure iommu interface is for content protected buffers. But these
> secure iommu contexts aren't used by msm DRM nor Venus in mainline. In
> Venus case I use non-secure iommu context for data buffers.
> 

We must consider the case when DRM, VFE and Venus handles protected
buffers.

> > 
> > As such the iommu tables are not part of the venus rproc; I believe they
> > should either be tied into the msm-iommu driver or perhaps exposed as
> > its own iommu(?).
> 
> The page tables are in msm-iommu driver.
> 

So, just to verify your answer, the msm-iommu driver will handle both
protected and unprotected?

> > 
> > 
> > But I presume from your inclusion that you've concluded that the venus
> > firmware we have refuses to execute without these tables at least
> > initialized, is this correct?
> 
> Yes, the SMC call for PAS memory-setup will fail if this page table is
> not initialized.
> 

If the msm-iommu driver will handle the protected buffers (or if there
will be a separate iommu driver for protected buffers) it should issue
these calls, to not be dependant on the rproc-venus driver.

With that I think we should make the rproc-venus driver depend on this
being setup (even if this means creating a "dummy" driver for the
protected iommu handling for now).

> > 
> >>>
> >>>> The address is not really fixed, cause the firmware could support
> >>>> relocation. In this example I just picked up the next free memory region
> >>>> in memory-reserved from msm8916.dtsi.
> >>>>
> >>>
> >>> In 8974 we do have a physical region where it's expected to be loaded.
> >>>
> >>> So in line with upcoming remoteproc work we should support referencing a
> >>> reserved-memory node with either reg or size.
> >>>
> >>> In the case of spotting a "reg" we're currently better off using
> >>> ioremap. We're looking at getting the remoteproc core to deal with this
> >>> mess.
> >>
> >> You mean that remoteproc core will parse memory-region property?
> >>
> > 
> > It has to, because it's a quite common scenario for remoteproc drivers
> > to either get its backing memory from a static region or be restricted
> > to part of system ram - properties that reserved-memory and
> > memory-region captures already.
> 
> OK, I have no issues with that. My concern is the manual parsing of
> 'memory-region' and 'reg' properties in remoteproc core.
> 
> So that idea is to have generic binding for rproc, that would be good.
> 

I do share your concerns here. But it's a recurring issue with
remoteproc drivers.

[..]
> > But I presume we have the implementation issue of dma_alloc_coherent()
> > failing in either case with the 5MB size. I think we need to look into
> 
> I'd be good to include Marek Szyprowski? At least he will know what
> design restrictions there are.
> 

Please do. The more I look at this the more I think we must use the
existing infrastructure for allocating "dma memory". Getting
dma_alloc_coherent() supporting non-power-of-2 memory regions would
allow us to use the existing infrastructure, for both fixed and
dynamically placed memory carveouts in remoteproc.

Regards,
Bjorn

Re: [PATCH 3/4] dt-binding: remoteproc: venus rproc dt binding document

[Stanimir Varbanov]

Hi,

Cc: Marek

On 08/30/2016 08:17 PM, Bjorn Andersson wrote:
> On Mon 29 Aug 04:48 PDT 2016, Stanimir Varbanov wrote:
> 
> [..]
>>> Trying to wrap my head around how the iommu part works here. The
>>> downstream code seems to indicate that this is a "generic" secure iommu
>>> interface - used by venus, camera and kgsl; likely for dealing with DRM
>>> protected buffers.
>>
>> The secure iommu interface is for content protected buffers. But these
>> secure iommu contexts aren't used by msm DRM nor Venus in mainline. In
>> Venus case I use non-secure iommu context for data buffers.
>>
> 
> We must consider the case when DRM, VFE and Venus handles protected
> buffers.

That would be interesting topic.

> 
>>>
>>> As such the iommu tables are not part of the venus rproc; I believe they
>>> should either be tied into the msm-iommu driver or perhaps exposed as
>>> its own iommu(?).
>>
>> The page tables are in msm-iommu driver.
>>
> 
> So, just to verify your answer, the msm-iommu driver will handle both
> protected and unprotected?

yes, at least for Venus case, the secured buffers are tied to different
iommu contexts (and stream IDs). But this needs to be verified more
carefully.

> 
>>>
>>>
>>> But I presume from your inclusion that you've concluded that the venus
>>> firmware we have refuses to execute without these tables at least
>>> initialized, is this correct?
>>
>> Yes, the SMC call for PAS memory-setup will fail if this page table is
>> not initialized.
>>
> 
> If the msm-iommu driver will handle the protected buffers (or if there
> will be a separate iommu driver for protected buffers) it should issue
> these calls, to not be dependant on the rproc-venus driver.

For msm8916 we don't have iommu driver in mainline, I don't know what is
the status with msm8996.

> 
> With that I think we should make the rproc-venus driver depend on this
> being setup (even if this means creating a "dummy" driver for the
> protected iommu handling for now).

This sounds scary :)

> 
>>>
>>>>>
>>>>>> The address is not really fixed, cause the firmware could support
>>>>>> relocation. In this example I just picked up the next free memory region
>>>>>> in memory-reserved from msm8916.dtsi.
>>>>>>
>>>>>
>>>>> In 8974 we do have a physical region where it's expected to be loaded.
>>>>>
>>>>> So in line with upcoming remoteproc work we should support referencing a
>>>>> reserved-memory node with either reg or size.
>>>>>
>>>>> In the case of spotting a "reg" we're currently better off using
>>>>> ioremap. We're looking at getting the remoteproc core to deal with this
>>>>> mess.
>>>>
>>>> You mean that remoteproc core will parse memory-region property?
>>>>
>>>
>>> It has to, because it's a quite common scenario for remoteproc drivers
>>> to either get its backing memory from a static region or be restricted
>>> to part of system ram - properties that reserved-memory and
>>> memory-region captures already.
>>
>> OK, I have no issues with that. My concern is the manual parsing of
>> 'memory-region' and 'reg' properties in remoteproc core.
>>
>> So that idea is to have generic binding for rproc, that would be good.
>>
> 
> I do share your concerns here. But it's a recurring issue with
> remoteproc drivers.
> 
> [..]
>>> But I presume we have the implementation issue of dma_alloc_coherent()
>>> failing in either case with the 5MB size. I think we need to look into
>>
>> I'd be good to include Marek Szyprowski? At least he will know what
>> design restrictions there are.
>>
> 
> Please do. The more I look at this the more I think we must use the
> existing infrastructure for allocating "dma memory". Getting
> dma_alloc_coherent() supporting non-power-of-2 memory regions would

Just to be precise it should be dma_alloc_from_coherent().

Marek, what is your opinion on that, can we make dma_alloc_from_coherent
able to allocate memory for sizes with bigger granularity.

For your convenience here [1] is the mail thread.

> allow us to use the existing infrastructure, for both fixed and
> dynamically placed memory carveouts in remoteproc.

[1] https://lkml.org/lkml/2016/8/19/570

-- 
regards,
Stan

Re: [PATCH 3/4] dt-binding: remoteproc: venus rproc dt binding document

[Andy Gross]

On Thu, Sep 01, 2016 at 05:58:17PM +0300, Stanimir Varbanov wrote:
> Hi,
> 
> Cc: Marek
> 
> On 08/30/2016 08:17 PM, Bjorn Andersson wrote:
> > On Mon 29 Aug 04:48 PDT 2016, Stanimir Varbanov wrote:
> > 
> > [..]
> >>> Trying to wrap my head around how the iommu part works here. The
> >>> downstream code seems to indicate that this is a "generic" secure iommu
> >>> interface - used by venus, camera and kgsl; likely for dealing with DRM
> >>> protected buffers.
> >>
> >> The secure iommu interface is for content protected buffers. But these
> >> secure iommu contexts aren't used by msm DRM nor Venus in mainline. In
> >> Venus case I use non-secure iommu context for data buffers.
> >>
> > 
> > We must consider the case when DRM, VFE and Venus handles protected
> > buffers.
> 
> That would be interesting topic.
> 
> > 
> >>>
> >>> As such the iommu tables are not part of the venus rproc; I believe they
> >>> should either be tied into the msm-iommu driver or perhaps exposed as
> >>> its own iommu(?).
> >>
> >> The page tables are in msm-iommu driver.
> >>
> > 
> > So, just to verify your answer, the msm-iommu driver will handle both
> > protected and unprotected?
> 
> yes, at least for Venus case, the secured buffers are tied to different
> iommu contexts (and stream IDs). But this needs to be verified more
> carefully.
> 
> > 
> >>>
> >>>
> >>> But I presume from your inclusion that you've concluded that the venus
> >>> firmware we have refuses to execute without these tables at least
> >>> initialized, is this correct?
> >>
> >> Yes, the SMC call for PAS memory-setup will fail if this page table is
> >> not initialized.
> >>
> > 
> > If the msm-iommu driver will handle the protected buffers (or if there
> > will be a separate iommu driver for protected buffers) it should issue
> > these calls, to not be dependant on the rproc-venus driver.
> 
> For msm8916 we don't have iommu driver in mainline, I don't know what is
> the status with msm8996.

The iommu v2 transparently handles this and removes the requirement for the
specific SCM page tbl init calls.  However, the memory still has to be
configured to be accessible from the remote processor.

<snip>


Andy

Re: [PATCH 3/4] dt-binding: remoteproc: venus rproc dt binding document

[Marek Szyprowski]

Hi,


On 2016-09-01 16:58, Stanimir Varbanov wrote:
> Hi,
>
> Cc: Marek
>

...

>>>> But I presume we have the implementation issue of dma_alloc_coherent()
>>>> failing in either case with the 5MB size. I think we need to look into
>>> I'd be good to include Marek Szyprowski? At least he will know what
>>> design restrictions there are.
>>>
>> Please do. The more I look at this the more I think we must use the
>> existing infrastructure for allocating "dma memory". Getting
>> dma_alloc_coherent() supporting non-power-of-2 memory regions would
> Just to be precise it should be dma_alloc_from_coherent().
>
> Marek, what is your opinion on that, can we make dma_alloc_from_coherent
> able to allocate memory for sizes with bigger granularity.
>
> For your convenience here [1] is the mail thread.

There should be no technical restrictions to add support for bigger 
granularity
than power-of-2. dma_alloc_from_coherent uses standard bitmap based 
allocator,
so it already support tracking allocations of arbitrary size. However 
for the
small allocations (smaller than 64KiB?, 512KiB?) it would make sense to keep
nearest-power-of-2 round up to prevent memory fragmentation.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH 3/4] dt-binding: remoteproc: venus rproc dt binding document

[Bjorn Andersson]

On Fri 02 Sep 04:52 PDT 2016, Marek Szyprowski wrote:

> Hi,
> 
> 
> On 2016-09-01 16:58, Stanimir Varbanov wrote:
> >Hi,
> >
> >Cc: Marek
> >
> 
> ...
> 
> >>>>But I presume we have the implementation issue of dma_alloc_coherent()
> >>>>failing in either case with the 5MB size. I think we need to look into
> >>>I'd be good to include Marek Szyprowski? At least he will know what
> >>>design restrictions there are.
> >>>
> >>Please do. The more I look at this the more I think we must use the
> >>existing infrastructure for allocating "dma memory". Getting
> >>dma_alloc_coherent() supporting non-power-of-2 memory regions would
> >Just to be precise it should be dma_alloc_from_coherent().
> >
> >Marek, what is your opinion on that, can we make dma_alloc_from_coherent
> >able to allocate memory for sizes with bigger granularity.
> >
> >For your convenience here [1] is the mail thread.
> 
> There should be no technical restrictions to add support for bigger
> granularity than power-of-2. dma_alloc_from_coherent uses standard
> bitmap based allocator, so it already support tracking allocations of
> arbitrary size.

I believe we should be able to change the parameter of
bitmap_{find_free,release,allocate}_region() to take a size rather than
an order.

The mask used in __reg_op() is an unsigned long, that is stamped over
the region to be masked or cleared, so there are some clear restrictions
in what parameters we can pass there - without having to break this
operation up in steps.

But if drive the offset by taking the next power-of-two of the size and
then align the number of bits to min(count, BITS_PER_LONG) we should
retain the performance characteristics and requirements of __reg_op().

> However for the small allocations (smaller than 64KiB?, 512KiB?) it
> would make sense to keep nearest-power-of-2 round up to prevent memory
> fragmentation.

But in our case each bit matches a single page, so by making sure the
mask always fills the unsigned long in the larger cases we would end up
with having to align things to 128kb (or 256kb if unsigned long is 64
bit).


Does this sound reasonable?

Regards,
Bjorn

Re: [PATCH 3/4] dt-binding: remoteproc: venus rproc dt binding document

[Stanimir Varbanov]

Hi Bjorn,

On 09/02/2016 11:12 PM, Bjorn Andersson wrote:
> On Fri 02 Sep 04:52 PDT 2016, Marek Szyprowski wrote:
> 
>> Hi,
>>
>>
>> On 2016-09-01 16:58, Stanimir Varbanov wrote:
>>> Hi,
>>>
>>> Cc: Marek
>>>
>>
>> ...
>>
>>>>>> But I presume we have the implementation issue of dma_alloc_coherent()
>>>>>> failing in either case with the 5MB size. I think we need to look into
>>>>> I'd be good to include Marek Szyprowski? At least he will know what
>>>>> design restrictions there are.
>>>>>
>>>> Please do. The more I look at this the more I think we must use the
>>>> existing infrastructure for allocating "dma memory". Getting
>>>> dma_alloc_coherent() supporting non-power-of-2 memory regions would
>>> Just to be precise it should be dma_alloc_from_coherent().
>>>
>>> Marek, what is your opinion on that, can we make dma_alloc_from_coherent
>>> able to allocate memory for sizes with bigger granularity.
>>>
>>> For your convenience here [1] is the mail thread.
>>
>> There should be no technical restrictions to add support for bigger
>> granularity than power-of-2. dma_alloc_from_coherent uses standard
>> bitmap based allocator, so it already support tracking allocations of
>> arbitrary size.
> 
> I believe we should be able to change the parameter of
> bitmap_{find_free,release,allocate}_region() to take a size rather than
> an order.
> 
> The mask used in __reg_op() is an unsigned long, that is stamped over
> the region to be masked or cleared, so there are some clear restrictions
> in what parameters we can pass there - without having to break this
> operation up in steps.
> 
> But if drive the offset by taking the next power-of-two of the size and
> then align the number of bits to min(count, BITS_PER_LONG) we should
> retain the performance characteristics and requirements of __reg_op().
> 
>> However for the small allocations (smaller than 64KiB?, 512KiB?) it
>> would make sense to keep nearest-power-of-2 round up to prevent memory
>> fragmentation.
> 
> But in our case each bit matches a single page, so by making sure the
> mask always fills the unsigned long in the larger cases we would end up
> with having to align things to 128kb (or 256kb if unsigned long is 64
> bit).
> 
> 
> Does this sound reasonable?

I haven't looked in bitmap details, but can't we reuse genalloc?

-- 
regards,
Stan

Re: [PATCH 3/4] dt-binding: remoteproc: venus rproc dt binding document

[Marek Szyprowski]

Hi Bjorn,


On 2016-09-02 22:12, Bjorn Andersson wrote:
> On Fri 02 Sep 04:52 PDT 2016, Marek Szyprowski wrote:
>
>> On 2016-09-01 16:58, Stanimir Varbanov wrote:
>> ...
>>>>>> But I presume we have the implementation issue of dma_alloc_coherent()
>>>>>> failing in either case with the 5MB size. I think we need to look into
>>>>> I'd be good to include Marek Szyprowski? At least he will know what
>>>>> design restrictions there are.
>>>>>
>>>> Please do. The more I look at this the more I think we must use the
>>>> existing infrastructure for allocating "dma memory". Getting
>>>> dma_alloc_coherent() supporting non-power-of-2 memory regions would
>>> Just to be precise it should be dma_alloc_from_coherent().
>>>
>>> Marek, what is your opinion on that, can we make dma_alloc_from_coherent
>>> able to allocate memory for sizes with bigger granularity.
>>>
>>> For your convenience here [1] is the mail thread.
>> There should be no technical restrictions to add support for bigger
>> granularity than power-of-2. dma_alloc_from_coherent uses standard
>> bitmap based allocator, so it already support tracking allocations of
>> arbitrary size.
> I believe we should be able to change the parameter of
> bitmap_{find_free,release,allocate}_region() to take a size rather than
> an order.
>
> The mask used in __reg_op() is an unsigned long, that is stamped over
> the region to be masked or cleared, so there are some clear restrictions
> in what parameters we can pass there - without having to break this
> operation up in steps.
>
> But if drive the offset by taking the next power-of-two of the size and
> then align the number of bits to min(count, BITS_PER_LONG) we should
> retain the performance characteristics and requirements of __reg_op().
>
>> However for the small allocations (smaller than 64KiB?, 512KiB?) it
>> would make sense to keep nearest-power-of-2 round up to prevent memory
>> fragmentation.
> But in our case each bit matches a single page, so by making sure the
> mask always fills the unsigned long in the larger cases we would end up
> with having to align things to 128kb (or 256kb if unsigned long is 64
> bit).

By preventing memory fragmentation I wanted to align small allocations (less
than the mentioned 64KiB or 512KiB) to nearest-power-of-2 of their size just
like it is done now.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

[PATCH 4/4] remoteproc: qcom: add Venus video core firmware loader driver

[Stanimir Varbanov]

This driver will load and authenticate the Venus firmware and
bringing it core out of reset. Those two functionalities are
via secure monitor calls to trusted environment.

Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
---
 drivers/remoteproc/Kconfig          |  10 ++
 drivers/remoteproc/Makefile         |   1 +
 drivers/remoteproc/qcom_venus_pil.c | 226 ++++++++++++++++++++++++++++++++++++
 3 files changed, 237 insertions(+)
 create mode 100644 drivers/remoteproc/qcom_venus_pil.c

diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index f76b3b1dca1a..90f66cfe054f 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -107,6 +107,16 @@ config QCOM_WCNSS_PIL
 	  Say y here to support the Peripheral Image Loader for the Qualcomm
 	  Wireless Connectivity Subsystem.
 
+config QCOM_VENUS_PIL
+	tristate "Qualcomm Venus Peripheral Image Loader"
+	depends on OF && ARCH_QCOM
+	depends on QCOM_SCM
+	select QCOM_MDT_LOADER
+	select REMOTEPROC
+	help
+	  Say y here to support Qualcomm Peripherial Image Loader for the
+	  Venus remote processor.
+
 config ST_REMOTEPROC
 	tristate "ST remoteproc support"
 	depends on ARCH_STI
diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
index 6dfb62ed643f..852711b89844 100644
--- a/drivers/remoteproc/Makefile
+++ b/drivers/remoteproc/Makefile
@@ -15,4 +15,5 @@ obj-$(CONFIG_QCOM_MDT_LOADER)		+= qcom_mdt_loader.o
 obj-$(CONFIG_QCOM_Q6V5_PIL)		+= qcom_q6v5_pil.o
 obj-$(CONFIG_QCOM_WCNSS_IRIS)		+= qcom_wcnss_iris.o
 obj-$(CONFIG_QCOM_WCNSS_PIL)		+= qcom_wcnss.o
+obj-$(CONFIG_QCOM_VENUS_PIL)            += qcom_venus_pil.o
 obj-$(CONFIG_ST_REMOTEPROC)		+= st_remoteproc.o
diff --git a/drivers/remoteproc/qcom_venus_pil.c b/drivers/remoteproc/qcom_venus_pil.c
new file mode 100644
index 000000000000..891c7d9a96bf
--- /dev/null
+++ b/drivers/remoteproc/qcom_venus_pil.c
@@ -0,0 +1,226 @@
+/*
+ * Qualcomm Venus Peripheral Image Loader
+ *
+ * Copyright (C) 2016 Linaro Ltd
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/dma-mapping.h>
+#include <linux/firmware.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_reserved_mem.h>
+#include <linux/platform_device.h>
+#include <linux/qcom_scm.h>
+#include <linux/remoteproc.h>
+
+#include "qcom_mdt_loader.h"
+#include "remoteproc_internal.h"
+
+#define VENUS_CRASH_REASON_SMEM		425
+#define VENUS_FIRMWARE_NAME		"venus.mdt"
+#define VENUS_PAS_ID			9
+#define VENUS_FW_MEM_SIZE		SZ_8M
+#define VENUS_PGTABLE_OFFSET		(6 * SZ_1M)
+
+struct qcom_venus {
+	struct device *dev;
+	struct rproc *rproc;
+	phys_addr_t fw_addr;
+	phys_addr_t mem_phys;
+	void *mem_va;
+	size_t mem_size;
+	size_t pgtable_sz;
+};
+
+static int venus_load(struct rproc *rproc, const struct firmware *fw)
+{
+	struct qcom_venus *venus = rproc->priv;
+	phys_addr_t pa;
+	size_t fw_size;
+	bool relocate;
+	int ret;
+
+	ret = qcom_scm_pas_init_image(VENUS_PAS_ID, fw->data, fw->size);
+	if (ret) {
+		dev_err(&rproc->dev, "invalid firmware metadata (%d)\n", ret);
+		return -EINVAL;
+	}
+
+	ret = qcom_mdt_parse(fw, &venus->fw_addr, &fw_size, &relocate);
+	if (ret) {
+		dev_err(&rproc->dev, "failed to parse mdt header (%d)\n", ret);
+		return ret;
+	}
+
+	if (fw_size + venus->pgtable_sz > venus->mem_size)
+		return -ENOMEM;
+
+	pa = relocate ? venus->mem_phys : venus->fw_addr;
+
+	ret = qcom_scm_pas_mem_setup(VENUS_PAS_ID, pa, fw_size);
+	if (ret) {
+		dev_err(&rproc->dev, "unable to setup memory (%d)\n", ret);
+		return -EINVAL;
+	}
+
+	return qcom_mdt_load(rproc, fw, VENUS_FIRMWARE_NAME);
+}
+
+static const struct rproc_fw_ops venus_fw_ops = {
+	.find_rsc_table = qcom_mdt_find_rsc_table,
+	.load = venus_load,
+};
+
+static int venus_start(struct rproc *rproc)
+{
+	struct qcom_venus *venus = rproc->priv;
+	int ret;
+
+	ret = qcom_scm_pas_auth_and_reset(VENUS_PAS_ID);
+	if (ret)
+		dev_err(venus->dev,
+			"authentication image and release reset failed (%d)\n",
+			ret);
+
+	return ret;
+}
+
+static int venus_stop(struct rproc *rproc)
+{
+	struct qcom_venus *venus = rproc->priv;
+	int ret;
+
+	ret = qcom_scm_pas_shutdown(VENUS_PAS_ID);
+	if (ret)
+		dev_err(venus->dev, "failed to shutdown: %d\n", ret);
+
+	return ret;
+}
+
+static void *venus_da_to_va(struct rproc *rproc, u64 da, int len)
+{
+	struct qcom_venus *venus = rproc->priv;
+	s64 offset;
+
+	offset = da - venus->fw_addr;
+
+	if (offset < 0 || offset + len > venus->mem_size)
+		return NULL;
+
+	return venus->mem_va + offset;
+}
+
+static const struct rproc_ops venus_ops = {
+	.start = venus_start,
+	.stop = venus_stop,
+	.da_to_va = venus_da_to_va,
+};
+
+static int venus_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct qcom_venus *venus;
+	struct rproc *rproc;
+	dma_addr_t pgaddr;
+	int ret;
+
+	if (!qcom_scm_is_available())
+		return -EPROBE_DEFER;
+
+	if (!qcom_scm_pas_supported(VENUS_PAS_ID)) {
+		dev_err(dev, "PAS is not available for venus\n");
+		return -ENXIO;
+	}
+
+	ret = of_reserved_mem_device_init(dev);
+	if (ret)
+		return ret;
+
+	rproc = rproc_alloc(dev, pdev->name, &venus_ops, VENUS_FIRMWARE_NAME,
+			    sizeof(*venus));
+	if (!rproc) {
+		dev_err(dev, "unable to allocate remoteproc\n");
+		ret = -ENOMEM;
+		goto release_mem;
+	}
+
+	rproc->fw_ops = &venus_fw_ops;
+	venus = rproc->priv;
+	venus->dev = dev;
+	venus->rproc = rproc;
+	venus->mem_size = VENUS_FW_MEM_SIZE;
+
+	platform_set_drvdata(pdev, venus);
+
+	venus->mem_va = dma_alloc_coherent(dev, venus->mem_size,
+					   &venus->mem_phys, GFP_KERNEL);
+	if (!venus->mem_va) {
+		ret = -ENOMEM;
+		goto free_rproc;
+	}
+
+	ret = qcom_scm_iommu_secure_ptbl_size(0, &venus->pgtable_sz);
+	if (ret)
+		goto free_mem;
+
+	pgaddr = venus->mem_phys + VENUS_PGTABLE_OFFSET;
+
+	ret = qcom_scm_iommu_secure_ptbl_init(pgaddr, venus->pgtable_sz, 0);
+	if (ret)
+		goto free_mem;
+
+	ret = rproc_add(rproc);
+	if (ret)
+		goto free_mem;
+
+	return 0;
+
+free_mem:
+	dma_free_coherent(dev, venus->mem_size, venus->mem_va, venus->mem_phys);
+free_rproc:
+	rproc_put(rproc);
+release_mem:
+	of_reserved_mem_device_release(dev);
+
+	return ret;
+}
+
+static int venus_remove(struct platform_device *pdev)
+{
+	struct qcom_venus *venus = platform_get_drvdata(pdev);
+	struct device *dev = venus->dev;
+
+	rproc_del(venus->rproc);
+	rproc_put(venus->rproc);
+	dma_free_coherent(dev, venus->mem_size, venus->mem_va, venus->mem_phys);
+	of_reserved_mem_device_release(dev);
+
+	return 0;
+}
+
+static const struct of_device_id venus_of_match[] = {
+	{ .compatible = "qcom,venus-pil" },
+	{ },
+};
+
+static struct platform_driver venus_driver = {
+	.probe = venus_probe,
+	.remove = venus_remove,
+	.driver = {
+		.name = "qcom-venus-pil",
+		.of_match_table = venus_of_match,
+	},
+};
+
+module_platform_driver(venus_driver);
+MODULE_DESCRIPTION("Peripheral Image Loader for Venus");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

Re: [PATCH 4/4] remoteproc: qcom: add Venus video core firmware loader driver

[Stanimir Varbanov]

Hi Bjorn,

I want to resubmit this remoteproc driver without secure page table scm
call, do you have something to note/add before that?

On 08/19/2016 06:53 PM, Stanimir Varbanov wrote:
> This driver will load and authenticate the Venus firmware and
> bringing it core out of reset. Those two functionalities are
> via secure monitor calls to trusted environment.
> 
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> ---
>  drivers/remoteproc/Kconfig          |  10 ++
>  drivers/remoteproc/Makefile         |   1 +
>  drivers/remoteproc/qcom_venus_pil.c | 226 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 237 insertions(+)
>  create mode 100644 drivers/remoteproc/qcom_venus_pil.c
> 


-- 
regards,
Stan