All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/4] Venus remoteproc driver
@ 2016-08-19 15:53 ` Stanimir Varbanov
  0 siblings, 0 replies; 34+ messages in thread
From: Stanimir Varbanov @ 2016-08-19 15:53 UTC (permalink / raw)
  To: Andy Gross, Ohad Ben-Cohen, Bjorn Andersson, Stephen Boyd,
	Rob Herring, Mark Rutland
  Cc: Rob Clark, Srinivas Kandagatla, linux-kernel, linux-arm-msm,
	linux-remoteproc, devicetree, 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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH 0/4] Venus remoteproc driver
@ 2016-08-19 15:53 ` Stanimir Varbanov
  0 siblings, 0 replies; 34+ messages in thread
From: Stanimir Varbanov @ 2016-08-19 15:53 UTC (permalink / raw)
  To: Andy Gross, Ohad Ben-Cohen, Bjorn Andersson, Stephen Boyd,
	Rob Herring, Mark Rutland
  Cc: Rob Clark, Srinivas Kandagatla,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-msm-u79uwXL29TY76Z2rM5mHXA,
	linux-remoteproc-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA, 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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH 1/4] firmware: qcom: scm: add a video command for state setting
@ 2016-08-19 15:53   ` Stanimir Varbanov
  0 siblings, 0 replies; 34+ messages in thread
From: Stanimir Varbanov @ 2016-08-19 15:53 UTC (permalink / raw)
  To: Andy Gross, Ohad Ben-Cohen, Bjorn Andersson, Stephen Boyd,
	Rob Herring, Mark Rutland
  Cc: Rob Clark, Srinivas Kandagatla, linux-kernel, linux-arm-msm,
	linux-remoteproc, devicetree, 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

^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH 1/4] firmware: qcom: scm: add a video command for state setting
@ 2016-08-19 15:53   ` Stanimir Varbanov
  0 siblings, 0 replies; 34+ messages in thread
From: Stanimir Varbanov @ 2016-08-19 15:53 UTC (permalink / raw)
  To: Andy Gross, Ohad Ben-Cohen, Bjorn Andersson, Stephen Boyd,
	Rob Herring, Mark Rutland
  Cc: Rob Clark, Srinivas Kandagatla,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-msm-u79uwXL29TY76Z2rM5mHXA,
	linux-remoteproc-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA, 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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH 2/4] firmware: qcom: scm: add iommu scm calls for pg table
  2016-08-19 15:53 ` Stanimir Varbanov
  (?)
  (?)
@ 2016-08-19 15:53 ` Stanimir Varbanov
  2016-08-22 16:29     ` kbuild test robot
  2016-08-24 18:35   ` Gupta, Puja
  -1 siblings, 2 replies; 34+ messages in thread
From: Stanimir Varbanov @ 2016-08-19 15:53 UTC (permalink / raw)
  To: Andy Gross, Ohad Ben-Cohen, Bjorn Andersson, Stephen Boyd,
	Rob Herring, Mark Rutland
  Cc: Rob Clark, Srinivas Kandagatla, linux-kernel, linux-arm-msm,
	linux-remoteproc, devicetree, 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

^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH 3/4] dt-binding: remoteproc: venus rproc dt binding document
  2016-08-19 15:53 ` Stanimir Varbanov
                   ` (2 preceding siblings ...)
  (?)
@ 2016-08-19 15:53 ` Stanimir Varbanov
  2016-08-23 17:32   ` Rob Herring
  -1 siblings, 1 reply; 34+ messages in thread
From: Stanimir Varbanov @ 2016-08-19 15:53 UTC (permalink / raw)
  To: Andy Gross, Ohad Ben-Cohen, Bjorn Andersson, Stephen Boyd,
	Rob Herring, Mark Rutland
  Cc: Rob Clark, Srinivas Kandagatla, linux-kernel, linux-arm-msm,
	linux-remoteproc, devicetree, 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

^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH 4/4] remoteproc: qcom: add Venus video core firmware loader driver
  2016-08-19 15:53 ` Stanimir Varbanov
                   ` (3 preceding siblings ...)
  (?)
@ 2016-08-19 15:53 ` Stanimir Varbanov
  2016-10-18 16:23     ` Stanimir Varbanov
  -1 siblings, 1 reply; 34+ messages in thread
From: Stanimir Varbanov @ 2016-08-19 15:53 UTC (permalink / raw)
  To: Andy Gross, Ohad Ben-Cohen, Bjorn Andersson, Stephen Boyd,
	Rob Herring, Mark Rutland
  Cc: Rob Clark, Srinivas Kandagatla, linux-kernel, linux-arm-msm,
	linux-remoteproc, devicetree, 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

^ permalink raw reply related	[flat|nested] 34+ messages in thread

* Re: [PATCH 2/4] firmware: qcom: scm: add iommu scm calls for pg table
@ 2016-08-22 16:29     ` kbuild test robot
  0 siblings, 0 replies; 34+ messages in thread
From: kbuild test robot @ 2016-08-22 16:29 UTC (permalink / raw)
  To: Stanimir Varbanov
  Cc: kbuild-all, Andy Gross, Ohad Ben-Cohen, Bjorn Andersson,
	Stephen Boyd, Rob Herring, Mark Rutland, Rob Clark,
	Srinivas Kandagatla, linux-kernel, linux-arm-msm,
	linux-remoteproc, devicetree

[-- 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 --]

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 2/4] firmware: qcom: scm: add iommu scm calls for pg table
@ 2016-08-22 16:29     ` kbuild test robot
  0 siblings, 0 replies; 34+ messages in thread
From: kbuild test robot @ 2016-08-22 16:29 UTC (permalink / raw)
  Cc: kbuild-all-JC7UmRfGjtg, Andy Gross, Ohad Ben-Cohen,
	Bjorn Andersson, Stephen Boyd, Rob Herring, Mark Rutland,
	Rob Clark, Srinivas Kandagatla,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-msm-u79uwXL29TY76Z2rM5mHXA,
	linux-remoteproc-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA, Stanimir Varbanov

[-- 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 --]

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 2/4] firmware: qcom: scm: add iommu scm calls for pg table
@ 2016-08-22 16:29     ` kbuild test robot
  0 siblings, 0 replies; 34+ messages in thread
From: kbuild test robot @ 2016-08-22 16:29 UTC (permalink / raw)
  To: Stanimir Varbanov
  Cc: kbuild-all, Andy Gross, Ohad Ben-Cohen, Bjorn Andersson,
	Stephen Boyd, Rob Herring, Mark Rutland, Rob Clark,
	Srinivas Kandagatla, linux-kernel, linux-arm-msm,
	linux-remoteproc, devicetree, Stanimir Varbanov

[-- 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 --]

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 3/4] dt-binding: remoteproc: venus rproc dt binding document
  2016-08-19 15:53 ` [PATCH 3/4] dt-binding: remoteproc: venus rproc dt binding document Stanimir Varbanov
@ 2016-08-23 17:32   ` Rob Herring
  2016-08-23 18:21     ` Bjorn Andersson
  2016-08-24 15:36       ` Stanimir Varbanov
  0 siblings, 2 replies; 34+ messages in thread
From: Rob Herring @ 2016-08-23 17:32 UTC (permalink / raw)
  To: Stanimir Varbanov
  Cc: Andy Gross, Ohad Ben-Cohen, Bjorn Andersson, Stephen Boyd,
	Mark Rutland, Rob Clark, Srinivas Kandagatla, linux-kernel,
	linux-arm-msm, linux-remoteproc, devicetree

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
> 

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 3/4] dt-binding: remoteproc: venus rproc dt binding document
  2016-08-23 17:32   ` Rob Herring
@ 2016-08-23 18:21     ` Bjorn Andersson
  2016-08-24 15:36       ` Stanimir Varbanov
  1 sibling, 0 replies; 34+ messages in thread
From: Bjorn Andersson @ 2016-08-23 18:21 UTC (permalink / raw)
  To: Rob Herring
  Cc: Stanimir Varbanov, Andy Gross, Ohad Ben-Cohen, Stephen Boyd,
	Mark Rutland, Rob Clark, Srinivas Kandagatla, linux-kernel,
	linux-arm-msm, linux-remoteproc, devicetree

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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 3/4] dt-binding: remoteproc: venus rproc dt binding document
  2016-08-23 17:32   ` Rob Herring
@ 2016-08-24 15:36       ` Stanimir Varbanov
  2016-08-24 15:36       ` Stanimir Varbanov
  1 sibling, 0 replies; 34+ messages in thread
From: Stanimir Varbanov @ 2016-08-24 15:36 UTC (permalink / raw)
  To: Rob Herring, Stanimir Varbanov
  Cc: Andy Gross, Ohad Ben-Cohen, Bjorn Andersson, Stephen Boyd,
	Mark Rutland, Rob Clark, Srinivas Kandagatla, linux-kernel,
	linux-arm-msm, linux-remoteproc, devicetree

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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 3/4] dt-binding: remoteproc: venus rproc dt binding document
@ 2016-08-24 15:36       ` Stanimir Varbanov
  0 siblings, 0 replies; 34+ messages in thread
From: Stanimir Varbanov @ 2016-08-24 15:36 UTC (permalink / raw)
  To: Rob Herring, Stanimir Varbanov
  Cc: Andy Gross, Ohad Ben-Cohen, Bjorn Andersson, Stephen Boyd,
	Mark Rutland, Rob Clark, Srinivas Kandagatla,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-msm-u79uwXL29TY76Z2rM5mHXA,
	linux-remoteproc-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA

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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 2/4] firmware: qcom: scm: add iommu scm calls for pg table
  2016-08-19 15:53 ` [PATCH 2/4] firmware: qcom: scm: add iommu scm calls for pg table Stanimir Varbanov
  2016-08-22 16:29     ` kbuild test robot
@ 2016-08-24 18:35   ` Gupta, Puja
  2016-08-25  9:08     ` Stanimir Varbanov
  2016-08-30 21:15     ` Andy Gross
  1 sibling, 2 replies; 34+ messages in thread
From: Gupta, Puja @ 2016-08-24 18:35 UTC (permalink / raw)
  To: Stanimir Varbanov, Andy Gross, Ohad Ben-Cohen, Bjorn Andersson,
	Stephen Boyd, Rob Herring, Mark Rutland
  Cc: Rob Clark, Srinivas Kandagatla, linux-kernel, linux-arm-msm,
	linux-remoteproc, devicetree

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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 3/4] dt-binding: remoteproc: venus rproc dt binding document
  2016-08-24 15:36       ` Stanimir Varbanov
  (?)
@ 2016-08-25  0:05       ` Bjorn Andersson
  2016-08-25 11:10           ` Stanimir Varbanov
  -1 siblings, 1 reply; 34+ messages in thread
From: Bjorn Andersson @ 2016-08-25  0:05 UTC (permalink / raw)
  To: Stanimir Varbanov
  Cc: Rob Herring, Stanimir Varbanov, Andy Gross, Ohad Ben-Cohen,
	Stephen Boyd, Mark Rutland, Rob Clark, Srinivas Kandagatla,
	linux-kernel, linux-arm-msm, linux-remoteproc, devicetree

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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 2/4] firmware: qcom: scm: add iommu scm calls for pg table
  2016-08-24 18:35   ` Gupta, Puja
@ 2016-08-25  9:08     ` Stanimir Varbanov
  2016-08-30 21:15     ` Andy Gross
  1 sibling, 0 replies; 34+ messages in thread
From: Stanimir Varbanov @ 2016-08-25  9:08 UTC (permalink / raw)
  To: Gupta, Puja, Stanimir Varbanov, Andy Gross, Ohad Ben-Cohen,
	Bjorn Andersson, Stephen Boyd, Rob Herring, Mark Rutland
  Cc: Rob Clark, Srinivas Kandagatla, linux-kernel, linux-arm-msm,
	linux-remoteproc, devicetree

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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 3/4] dt-binding: remoteproc: venus rproc dt binding document
  2016-08-25  0:05       ` Bjorn Andersson
@ 2016-08-25 11:10           ` Stanimir Varbanov
  0 siblings, 0 replies; 34+ messages in thread
From: Stanimir Varbanov @ 2016-08-25 11:10 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Rob Herring, Stanimir Varbanov, Andy Gross, Ohad Ben-Cohen,
	Stephen Boyd, Mark Rutland, Rob Clark, Srinivas Kandagatla,
	linux-kernel, linux-arm-msm, linux-remoteproc, devicetree

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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 3/4] dt-binding: remoteproc: venus rproc dt binding document
@ 2016-08-25 11:10           ` Stanimir Varbanov
  0 siblings, 0 replies; 34+ messages in thread
From: Stanimir Varbanov @ 2016-08-25 11:10 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Rob Herring, Stanimir Varbanov, Andy Gross, Ohad Ben-Cohen,
	Stephen Boyd, Mark Rutland, Rob Clark, Srinivas Kandagatla,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-msm-u79uwXL29TY76Z2rM5mHXA,
	linux-remoteproc-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA

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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 3/4] dt-binding: remoteproc: venus rproc dt binding document
@ 2016-08-26 22:23             ` Bjorn Andersson
  0 siblings, 0 replies; 34+ messages in thread
From: Bjorn Andersson @ 2016-08-26 22:23 UTC (permalink / raw)
  To: Stanimir Varbanov
  Cc: Rob Herring, Andy Gross, Ohad Ben-Cohen, Stephen Boyd,
	Mark Rutland, Rob Clark, Srinivas Kandagatla, linux-kernel,
	linux-arm-msm, linux-remoteproc, devicetree

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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 3/4] dt-binding: remoteproc: venus rproc dt binding document
@ 2016-08-26 22:23             ` Bjorn Andersson
  0 siblings, 0 replies; 34+ messages in thread
From: Bjorn Andersson @ 2016-08-26 22:23 UTC (permalink / raw)
  To: Stanimir Varbanov
  Cc: Rob Herring, Andy Gross, Ohad Ben-Cohen, Stephen Boyd,
	Mark Rutland, Rob Clark, Srinivas Kandagatla,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-msm-u79uwXL29TY76Z2rM5mHXA,
	linux-remoteproc-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA

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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 3/4] dt-binding: remoteproc: venus rproc dt binding document
  2016-08-26 22:23             ` Bjorn Andersson
  (?)
@ 2016-08-29 11:48             ` Stanimir Varbanov
  2016-08-30 17:17               ` Bjorn Andersson
  -1 siblings, 1 reply; 34+ messages in thread
From: Stanimir Varbanov @ 2016-08-29 11:48 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Rob Herring, Andy Gross, Ohad Ben-Cohen, Stephen Boyd,
	Mark Rutland, Rob Clark, Srinivas Kandagatla, linux-kernel,
	linux-arm-msm, linux-remoteproc, devicetree

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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 3/4] dt-binding: remoteproc: venus rproc dt binding document
  2016-08-29 11:48             ` Stanimir Varbanov
@ 2016-08-30 17:17               ` Bjorn Andersson
  2016-09-01 14:58                 ` Stanimir Varbanov
  0 siblings, 1 reply; 34+ messages in thread
From: Bjorn Andersson @ 2016-08-30 17:17 UTC (permalink / raw)
  To: Stanimir Varbanov
  Cc: Rob Herring, Andy Gross, Ohad Ben-Cohen, Stephen Boyd,
	Mark Rutland, Rob Clark, Srinivas Kandagatla, linux-kernel,
	linux-arm-msm, linux-remoteproc, devicetree

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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 2/4] firmware: qcom: scm: add iommu scm calls for pg table
  2016-08-24 18:35   ` Gupta, Puja
  2016-08-25  9:08     ` Stanimir Varbanov
@ 2016-08-30 21:15     ` Andy Gross
  1 sibling, 0 replies; 34+ messages in thread
From: Andy Gross @ 2016-08-30 21:15 UTC (permalink / raw)
  To: Gupta, Puja
  Cc: Stanimir Varbanov, Ohad Ben-Cohen, Bjorn Andersson, Stephen Boyd,
	Rob Herring, Mark Rutland, Rob Clark, Srinivas Kandagatla,
	linux-kernel, linux-arm-msm, linux-remoteproc, devicetree

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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 3/4] dt-binding: remoteproc: venus rproc dt binding document
  2016-08-30 17:17               ` Bjorn Andersson
@ 2016-09-01 14:58                 ` Stanimir Varbanov
  2016-09-01 20:46                     ` Andy Gross
  2016-09-02 11:52                   ` Marek Szyprowski
  0 siblings, 2 replies; 34+ messages in thread
From: Stanimir Varbanov @ 2016-09-01 14:58 UTC (permalink / raw)
  To: Bjorn Andersson, Stanimir Varbanov
  Cc: Rob Herring, Andy Gross, Ohad Ben-Cohen, Stephen Boyd,
	Mark Rutland, Rob Clark, Srinivas Kandagatla, linux-kernel,
	linux-arm-msm, linux-remoteproc, devicetree, Marek Szyprowski

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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 3/4] dt-binding: remoteproc: venus rproc dt binding document
@ 2016-09-01 20:46                     ` Andy Gross
  0 siblings, 0 replies; 34+ messages in thread
From: Andy Gross @ 2016-09-01 20:46 UTC (permalink / raw)
  To: Stanimir Varbanov
  Cc: Bjorn Andersson, Rob Herring, Ohad Ben-Cohen, Stephen Boyd,
	Mark Rutland, Rob Clark, Srinivas Kandagatla, linux-kernel,
	linux-arm-msm, linux-remoteproc, devicetree, Marek Szyprowski

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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 3/4] dt-binding: remoteproc: venus rproc dt binding document
@ 2016-09-01 20:46                     ` Andy Gross
  0 siblings, 0 replies; 34+ messages in thread
From: Andy Gross @ 2016-09-01 20:46 UTC (permalink / raw)
  To: Stanimir Varbanov
  Cc: Bjorn Andersson, Rob Herring, Ohad Ben-Cohen, Stephen Boyd,
	Mark Rutland, Rob Clark, Srinivas Kandagatla,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-msm-u79uwXL29TY76Z2rM5mHXA,
	linux-remoteproc-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA, Marek Szyprowski

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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 3/4] dt-binding: remoteproc: venus rproc dt binding document
  2016-09-01 14:58                 ` Stanimir Varbanov
  2016-09-01 20:46                     ` Andy Gross
@ 2016-09-02 11:52                   ` Marek Szyprowski
  2016-09-02 20:12                     ` Bjorn Andersson
  1 sibling, 1 reply; 34+ messages in thread
From: Marek Szyprowski @ 2016-09-02 11:52 UTC (permalink / raw)
  To: Stanimir Varbanov, Bjorn Andersson
  Cc: Rob Herring, Andy Gross, Ohad Ben-Cohen, Stephen Boyd,
	Mark Rutland, Rob Clark, Srinivas Kandagatla, linux-kernel,
	linux-arm-msm, linux-remoteproc, devicetree

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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 3/4] dt-binding: remoteproc: venus rproc dt binding document
  2016-09-02 11:52                   ` Marek Szyprowski
@ 2016-09-02 20:12                     ` Bjorn Andersson
  2016-09-07 11:52                         ` Stanimir Varbanov
       [not found]                       ` <CGME20160915084644eucas1p1bd3f2078d4e1cb3acfa0ea87557bff4f@eucas1p1.samsung.com>
  0 siblings, 2 replies; 34+ messages in thread
From: Bjorn Andersson @ 2016-09-02 20:12 UTC (permalink / raw)
  To: Marek Szyprowski
  Cc: Stanimir Varbanov, Rob Herring, Andy Gross, Ohad Ben-Cohen,
	Stephen Boyd, Mark Rutland, Rob Clark, Srinivas Kandagatla,
	linux-kernel, linux-arm-msm, linux-remoteproc, devicetree

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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 3/4] dt-binding: remoteproc: venus rproc dt binding document
  2016-09-02 20:12                     ` Bjorn Andersson
@ 2016-09-07 11:52                         ` Stanimir Varbanov
       [not found]                       ` <CGME20160915084644eucas1p1bd3f2078d4e1cb3acfa0ea87557bff4f@eucas1p1.samsung.com>
  1 sibling, 0 replies; 34+ messages in thread
From: Stanimir Varbanov @ 2016-09-07 11:52 UTC (permalink / raw)
  To: Bjorn Andersson, Marek Szyprowski
  Cc: Rob Herring, Andy Gross, Ohad Ben-Cohen, Stephen Boyd,
	Mark Rutland, Rob Clark, Srinivas Kandagatla, linux-kernel,
	linux-arm-msm, linux-remoteproc, devicetree

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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 3/4] dt-binding: remoteproc: venus rproc dt binding document
@ 2016-09-07 11:52                         ` Stanimir Varbanov
  0 siblings, 0 replies; 34+ messages in thread
From: Stanimir Varbanov @ 2016-09-07 11:52 UTC (permalink / raw)
  To: Bjorn Andersson, Marek Szyprowski
  Cc: Rob Herring, Andy Gross, Ohad Ben-Cohen, Stephen Boyd,
	Mark Rutland, Rob Clark, Srinivas Kandagatla,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-msm-u79uwXL29TY76Z2rM5mHXA,
	linux-remoteproc-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA

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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 3/4] dt-binding: remoteproc: venus rproc dt binding document
       [not found]                       ` <CGME20160915084644eucas1p1bd3f2078d4e1cb3acfa0ea87557bff4f@eucas1p1.samsung.com>
@ 2016-09-15  8:46                         ` Marek Szyprowski
  0 siblings, 0 replies; 34+ messages in thread
From: Marek Szyprowski @ 2016-09-15  8:46 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Stanimir Varbanov, Rob Herring, Andy Gross, Ohad Ben-Cohen,
	Stephen Boyd, Mark Rutland, Rob Clark, Srinivas Kandagatla,
	linux-kernel, linux-arm-msm, linux-remoteproc, devicetree

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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 4/4] remoteproc: qcom: add Venus video core firmware loader driver
@ 2016-10-18 16:23     ` Stanimir Varbanov
  0 siblings, 0 replies; 34+ messages in thread
From: Stanimir Varbanov @ 2016-10-18 16:23 UTC (permalink / raw)
  To: Andy Gross, Ohad Ben-Cohen, Bjorn Andersson, Stephen Boyd,
	Rob Herring, Mark Rutland
  Cc: Rob Clark, Srinivas Kandagatla, linux-kernel, linux-arm-msm,
	linux-remoteproc, devicetree

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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 4/4] remoteproc: qcom: add Venus video core firmware loader driver
@ 2016-10-18 16:23     ` Stanimir Varbanov
  0 siblings, 0 replies; 34+ messages in thread
From: Stanimir Varbanov @ 2016-10-18 16:23 UTC (permalink / raw)
  To: Andy Gross, Ohad Ben-Cohen, Bjorn Andersson, Stephen Boyd,
	Rob Herring, Mark Rutland
  Cc: Rob Clark, Srinivas Kandagatla,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-msm-u79uwXL29TY76Z2rM5mHXA,
	linux-remoteproc-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA

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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 34+ messages in thread

end of thread, other threads:[~2016-10-18 16:23 UTC | newest]

Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-19 15:53 [PATCH 0/4] Venus remoteproc driver Stanimir Varbanov
2016-08-19 15:53 ` Stanimir Varbanov
2016-08-19 15:53 ` [PATCH 1/4] firmware: qcom: scm: add a video command for state setting Stanimir Varbanov
2016-08-19 15:53   ` Stanimir Varbanov
2016-08-19 15:53 ` [PATCH 2/4] firmware: qcom: scm: add iommu scm calls for pg table Stanimir Varbanov
2016-08-22 16:29   ` kbuild test robot
2016-08-22 16:29     ` kbuild test robot
2016-08-22 16:29     ` kbuild test robot
2016-08-24 18:35   ` Gupta, Puja
2016-08-25  9:08     ` Stanimir Varbanov
2016-08-30 21:15     ` Andy Gross
2016-08-19 15:53 ` [PATCH 3/4] dt-binding: remoteproc: venus rproc dt binding document Stanimir Varbanov
2016-08-23 17:32   ` Rob Herring
2016-08-23 18:21     ` Bjorn Andersson
2016-08-24 15:36     ` Stanimir Varbanov
2016-08-24 15:36       ` Stanimir Varbanov
2016-08-25  0:05       ` Bjorn Andersson
2016-08-25 11:10         ` Stanimir Varbanov
2016-08-25 11:10           ` Stanimir Varbanov
2016-08-26 22:23           ` Bjorn Andersson
2016-08-26 22:23             ` Bjorn Andersson
2016-08-29 11:48             ` Stanimir Varbanov
2016-08-30 17:17               ` Bjorn Andersson
2016-09-01 14:58                 ` Stanimir Varbanov
2016-09-01 20:46                   ` Andy Gross
2016-09-01 20:46                     ` Andy Gross
2016-09-02 11:52                   ` Marek Szyprowski
2016-09-02 20:12                     ` Bjorn Andersson
2016-09-07 11:52                       ` Stanimir Varbanov
2016-09-07 11:52                         ` Stanimir Varbanov
     [not found]                       ` <CGME20160915084644eucas1p1bd3f2078d4e1cb3acfa0ea87557bff4f@eucas1p1.samsung.com>
2016-09-15  8:46                         ` Marek Szyprowski
2016-08-19 15:53 ` [PATCH 4/4] remoteproc: qcom: add Venus video core firmware loader driver Stanimir Varbanov
2016-10-18 16:23   ` Stanimir Varbanov
2016-10-18 16:23     ` Stanimir Varbanov

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.