[PATCH v9 0/5] Venus updates - PIL

[PATCH v9 0/5] Venus updates - PIL

[Vikash Garodia]

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

Vikash Garodia (4):
  venus: firmware: add routine to reset ARM9
  venus: firmware: move load firmware in a separate function
  venus: firmware: add no TZ boot and shutdown routine
  dt-bindings: media: Document bindings for venus firmware device

 .../devicetree/bindings/media/qcom,venus.txt       |  13 +-
 drivers/media/platform/qcom/venus/core.c           |  24 ++-
 drivers/media/platform/qcom/venus/core.h           |   6 +
 drivers/media/platform/qcom/venus/firmware.c       | 234 +++++++++++++++++++--
 drivers/media/platform/qcom/venus/firmware.h       |  17 +-
 drivers/media/platform/qcom/venus/hfi_venus.c      |  13 +-
 drivers/media/platform/qcom/venus/hfi_venus_io.h   |   8 +
 7 files changed, 272 insertions(+), 43 deletions(-)

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

[PATCH v9 1/5] venus: firmware: add routine to reset ARM9

[Vikash Garodia]

Add routine to reset the ARM9 and brings it out of reset. Also
abstract the Venus CPU state handling with a new function. This
is in preparation to add PIL functionality in venus driver.

Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
---
 drivers/media/platform/qcom/venus/core.h         |  2 ++
 drivers/media/platform/qcom/venus/firmware.c     | 33 ++++++++++++++++++++++++
 drivers/media/platform/qcom/venus/firmware.h     | 11 ++++++++
 drivers/media/platform/qcom/venus/hfi_venus.c    | 13 +++-------
 drivers/media/platform/qcom/venus/hfi_venus_io.h |  7 +++++
 5 files changed, 57 insertions(+), 9 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
index 2f02365..dfd5c10 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -98,6 +98,7 @@ struct venus_caps {
  * @dev:		convenience struct device pointer
  * @dev_dec:	convenience struct device pointer for decoder device
  * @dev_enc:	convenience struct device pointer for encoder device
+ * @no_tz:	a flag that suggests presence of trustzone
  * @lock:	a lock for this strucure
  * @instances:	a list_head of all instances
  * @insts_count:	num of instances
@@ -129,6 +130,7 @@ struct venus_core {
 	struct device *dev;
 	struct device *dev_dec;
 	struct device *dev_enc;
+	bool no_tz;
 	struct mutex lock;
 	struct list_head instances;
 	atomic_t insts_count;
diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c
index c4a5778..f2ae2f0 100644
--- a/drivers/media/platform/qcom/venus/firmware.c
+++ b/drivers/media/platform/qcom/venus/firmware.c
@@ -22,10 +22,43 @@
 #include <linux/sizes.h>
 #include <linux/soc/qcom/mdt_loader.h>
 
+#include "core.h"
 #include "firmware.h"
+#include "hfi_venus_io.h"
 
 #define VENUS_PAS_ID			9
 #define VENUS_FW_MEM_SIZE		(6 * SZ_1M)
+#define VENUS_FW_START_ADDR		0x0
+
+static void venus_reset_cpu(struct venus_core *core)
+{
+	void __iomem *base = core->base;
+
+	writel(0, base + WRAPPER_FW_START_ADDR);
+	writel(VENUS_FW_MEM_SIZE, base + WRAPPER_FW_END_ADDR);
+	writel(0, base + WRAPPER_CPA_START_ADDR);
+	writel(VENUS_FW_MEM_SIZE, base + WRAPPER_CPA_END_ADDR);
+	writel(VENUS_FW_MEM_SIZE, base + WRAPPER_NONPIX_START_ADDR);
+	writel(VENUS_FW_MEM_SIZE, base + WRAPPER_NONPIX_END_ADDR);
+	writel(0x0, base + WRAPPER_CPU_CGC_DIS);
+	writel(0x0, base + WRAPPER_CPU_CLOCK_CONFIG);
+
+	/* Bring ARM9 out of reset */
+	writel(0, base + WRAPPER_A9SS_SW_RESET);
+}
+
+int venus_set_hw_state(struct venus_core *core, bool resume)
+{
+	if (!core->no_tz)
+		return qcom_scm_set_remote_state(resume, 0);
+
+	if (resume)
+		venus_reset_cpu(core);
+	else
+		writel(1, core->base + WRAPPER_A9SS_SW_RESET);
+
+	return 0;
+}
 
 int venus_boot(struct device *dev, const char *fwname)
 {
diff --git a/drivers/media/platform/qcom/venus/firmware.h b/drivers/media/platform/qcom/venus/firmware.h
index 428efb5..397570c 100644
--- a/drivers/media/platform/qcom/venus/firmware.h
+++ b/drivers/media/platform/qcom/venus/firmware.h
@@ -18,5 +18,16 @@
 
 int venus_boot(struct device *dev, const char *fwname);
 int venus_shutdown(struct device *dev);
+int venus_set_hw_state(struct venus_core *core, bool suspend);
+
+static inline int venus_set_hw_state_suspend(struct venus_core *core)
+{
+	return venus_set_hw_state(core, false);
+}
+
+static inline int venus_set_hw_state_resume(struct venus_core *core)
+{
+	return venus_set_hw_state(core, true);
+}
 
 #endif
diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
index 1240855..074837e 100644
--- a/drivers/media/platform/qcom/venus/hfi_venus.c
+++ b/drivers/media/platform/qcom/venus/hfi_venus.c
@@ -19,7 +19,6 @@
 #include <linux/interrupt.h>
 #include <linux/iopoll.h>
 #include <linux/kernel.h>
-#include <linux/qcom_scm.h>
 #include <linux/slab.h>
 
 #include "core.h"
@@ -27,6 +26,7 @@
 #include "hfi_msgs.h"
 #include "hfi_venus.h"
 #include "hfi_venus_io.h"
+#include "firmware.h"
 
 #define HFI_MASK_QHDR_TX_TYPE		0xff000000
 #define HFI_MASK_QHDR_RX_TYPE		0x00ff0000
@@ -55,11 +55,6 @@
 #define IFACEQ_VAR_LARGE_PKT_SIZE	512
 #define IFACEQ_VAR_HUGE_PKT_SIZE	(1024 * 12)
 
-enum tzbsp_video_state {
-	TZBSP_VIDEO_STATE_SUSPEND = 0,
-	TZBSP_VIDEO_STATE_RESUME
-};
-
 struct hfi_queue_table_header {
 	u32 version;
 	u32 size;
@@ -575,7 +570,7 @@ static int venus_power_off(struct venus_hfi_device *hdev)
 	if (!hdev->power_enabled)
 		return 0;
 
-	ret = qcom_scm_set_remote_state(TZBSP_VIDEO_STATE_SUSPEND, 0);
+	ret = venus_set_hw_state_suspend(hdev->core);
 	if (ret)
 		return ret;
 
@@ -595,7 +590,7 @@ static int venus_power_on(struct venus_hfi_device *hdev)
 	if (hdev->power_enabled)
 		return 0;
 
-	ret = qcom_scm_set_remote_state(TZBSP_VIDEO_STATE_RESUME, 0);
+	ret = venus_set_hw_state_resume(hdev->core);
 	if (ret)
 		goto err;
 
@@ -608,7 +603,7 @@ static int venus_power_on(struct venus_hfi_device *hdev)
 	return 0;
 
 err_suspend:
-	qcom_scm_set_remote_state(TZBSP_VIDEO_STATE_SUSPEND, 0);
+	venus_set_hw_state_suspend(hdev->core);
 err:
 	hdev->power_enabled = false;
 	return ret;
diff --git a/drivers/media/platform/qcom/venus/hfi_venus_io.h b/drivers/media/platform/qcom/venus/hfi_venus_io.h
index def0926..d69f51b 100644
--- a/drivers/media/platform/qcom/venus/hfi_venus_io.h
+++ b/drivers/media/platform/qcom/venus/hfi_venus_io.h
@@ -112,6 +112,13 @@
 #define WRAPPER_CPU_STATUS			(WRAPPER_BASE + 0x2014)
 #define WRAPPER_CPU_STATUS_WFI			BIT(0)
 #define WRAPPER_SW_RESET			(WRAPPER_BASE + 0x3000)
+#define WRAPPER_CPA_START_ADDR			(WRAPPER_BASE + 0x1020)
+#define WRAPPER_CPA_END_ADDR			(WRAPPER_BASE + 0x1024)
+#define WRAPPER_FW_START_ADDR			(WRAPPER_BASE + 0x1028)
+#define WRAPPER_FW_END_ADDR			(WRAPPER_BASE + 0x102C)
+#define WRAPPER_NONPIX_START_ADDR		(WRAPPER_BASE + 0x1030)
+#define WRAPPER_NONPIX_END_ADDR			(WRAPPER_BASE + 0x1034)
+#define WRAPPER_A9SS_SW_RESET			(WRAPPER_BASE + 0x3000)
 
 /* Venus 4xx */
 #define WRAPPER_VCODEC0_MMCC_POWER_STATUS	(WRAPPER_BASE + 0x90)
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v9 2/5] venus: firmware: move load firmware in a separate function

[Vikash Garodia]

Separate firmware loading part into a new function.

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

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

[PATCH v9 3/5] venus: firmware: register separate platform_device for firmware loader

[Vikash Garodia]

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

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

Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
---
 drivers/media/platform/qcom/venus/core.c     | 14 +++++---
 drivers/media/platform/qcom/venus/core.h     |  3 ++
 drivers/media/platform/qcom/venus/firmware.c | 54 ++++++++++++++++++++++++++++
 drivers/media/platform/qcom/venus/firmware.h |  2 ++
 4 files changed, 69 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index 75b9785..440f25f 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -284,6 +284,14 @@ static int venus_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto err_runtime_disable;
 
+	ret = of_platform_populate(dev->of_node, NULL, NULL, dev);
+	if (ret)
+		goto err_runtime_disable;
+
+	ret = venus_firmware_init(core);
+	if (ret)
+		goto err_runtime_disable;
+
 	ret = venus_boot(core);
 	if (ret)
 		goto err_runtime_disable;
@@ -308,10 +316,6 @@ static int venus_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_core_deinit;
 
-	ret = of_platform_populate(dev->of_node, NULL, NULL, dev);
-	if (ret)
-		goto err_dev_unregister;
-
 	ret = pm_runtime_put_sync(dev);
 	if (ret)
 		goto err_dev_unregister;
@@ -347,6 +351,8 @@ static int venus_remove(struct platform_device *pdev)
 	venus_shutdown(dev);
 	of_platform_depopulate(dev);
 
+	venus_firmware_deinit(core);
+
 	pm_runtime_put_sync(dev);
 	pm_runtime_disable(dev);
 
diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
index dfd5c10..6f2c82d 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -130,6 +130,9 @@ struct venus_core {
 	struct device *dev;
 	struct device *dev_dec;
 	struct device *dev_enc;
+	struct video_firmware {
+		struct device *dev;
+	} fw;
 	bool no_tz;
 	struct mutex lock;
 	struct list_head instances;
diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c
index 16faba6..f4d2b4b 100644
--- a/drivers/media/platform/qcom/venus/firmware.c
+++ b/drivers/media/platform/qcom/venus/firmware.c
@@ -18,6 +18,8 @@
 #include <linux/io.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/of_device.h>
 #include <linux/qcom_scm.h>
 #include <linux/sizes.h>
 #include <linux/soc/qcom/mdt_loader.h>
@@ -144,3 +146,55 @@ int venus_shutdown(struct device *dev)
 {
 	return qcom_scm_pas_shutdown(VENUS_PAS_ID);
 }
+
+int venus_firmware_init(struct venus_core *core)
+{
+	struct platform_device_info info;
+	struct platform_device *pdev;
+	struct device_node *np;
+	int ret;
+
+	np = of_get_child_by_name(core->dev->of_node, "video-firmware");
+	if (!np)
+		return 0;
+
+	memset(&info, 0, sizeof(info));
+	info.fwnode = &np->fwnode;
+	info.parent = core->dev;
+	info.name = np->name;
+	info.dma_mask = DMA_BIT_MASK(32);
+
+	pdev = platform_device_register_full(&info);
+	if (IS_ERR(pdev)) {
+		of_node_put(np);
+		return PTR_ERR(pdev);
+	}
+
+	pdev->dev.of_node = np;
+
+	ret = of_dma_configure(&pdev->dev, np, true);
+	if (ret) {
+		dev_err(core->dev, "dma configure fail\n");
+		goto err_unregister;
+	}
+
+	core->fw.dev = &pdev->dev;
+	core->no_tz = true;
+
+	of_node_put(np);
+
+	return 0;
+
+err_unregister:
+	platform_device_unregister(pdev);
+	of_node_put(np);
+	return ret;
+}
+
+void venus_firmware_deinit(struct venus_core *core)
+{
+	if (!core->fw.dev)
+		return;
+
+	platform_device_unregister(to_platform_device(core->fw.dev));
+}
diff --git a/drivers/media/platform/qcom/venus/firmware.h b/drivers/media/platform/qcom/venus/firmware.h
index 1343747..fd7edf0 100644
--- a/drivers/media/platform/qcom/venus/firmware.h
+++ b/drivers/media/platform/qcom/venus/firmware.h
@@ -16,6 +16,8 @@
 
 struct device;
 
+int venus_firmware_init(struct venus_core *core);
+void venus_firmware_deinit(struct venus_core *core);
 int venus_boot(struct venus_core *core);
 int venus_shutdown(struct device *dev);
 int venus_set_hw_state(struct venus_core *core, bool suspend);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v9 4/5] venus: firmware: add no TZ boot and shutdown routine

[Vikash Garodia]

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

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

diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index 440f25f..3bd3b8a 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -76,7 +76,7 @@ static void venus_sys_error_handler(struct work_struct *work)
 	hfi_core_deinit(core, true);
 	hfi_destroy(core);
 	mutex_lock(&core->lock);
-	venus_shutdown(core->dev);
+	venus_shutdown(core);
 
 	pm_runtime_put_sync(core->dev);
 
@@ -327,7 +327,7 @@ static int venus_probe(struct platform_device *pdev)
 err_core_deinit:
 	hfi_core_deinit(core, false);
 err_venus_shutdown:
-	venus_shutdown(dev);
+	venus_shutdown(core);
 err_runtime_disable:
 	pm_runtime_set_suspended(dev);
 	pm_runtime_disable(dev);
@@ -348,7 +348,7 @@ static int venus_remove(struct platform_device *pdev)
 	WARN_ON(ret);
 
 	hfi_destroy(core);
-	venus_shutdown(dev);
+	venus_shutdown(core);
 	of_platform_depopulate(dev);
 
 	venus_firmware_deinit(core);
diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
index 6f2c82d..baaa1cc 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -132,6 +132,7 @@ struct venus_core {
 	struct device *dev_enc;
 	struct video_firmware {
 		struct device *dev;
+		struct iommu_domain *iommu_domain;
 	} fw;
 	bool no_tz;
 	struct mutex lock;
diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c
index f4d2b4b..3cf9313 100644
--- a/drivers/media/platform/qcom/venus/firmware.c
+++ b/drivers/media/platform/qcom/venus/firmware.c
@@ -15,6 +15,7 @@
 #include <linux/device.h>
 #include <linux/firmware.h>
 #include <linux/kernel.h>
+#include <linux/iommu.h>
 #include <linux/io.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
@@ -122,6 +123,56 @@ static int venus_load_fw(struct venus_core *core, const char *fwname,
 	return ret;
 }
 
+static int venus_boot_no_tz(struct venus_core *core, phys_addr_t mem_phys,
+			    size_t mem_size)
+{
+	struct iommu_domain *iommu;
+	struct device *dev;
+	int ret;
+
+	dev = core->fw.dev;
+	if (!dev)
+		return -EPROBE_DEFER;
+
+	iommu = core->fw.iommu_domain;
+
+	ret = iommu_map(iommu, VENUS_FW_START_ADDR, mem_phys, mem_size,
+			IOMMU_READ | IOMMU_WRITE | IOMMU_PRIV);
+	if (ret) {
+		dev_err(dev, "could not map video firmware region\n");
+		return ret;
+	}
+
+	venus_reset_cpu(core);
+
+	return 0;
+}
+
+static int venus_shutdown_no_tz(struct venus_core *core)
+{
+	struct iommu_domain *iommu;
+	size_t unmapped;
+	u32 reg;
+	struct device *dev = core->fw.dev;
+	void __iomem *base = core->base;
+
+	/* Assert the reset to ARM9 */
+	reg = readl_relaxed(base + WRAPPER_A9SS_SW_RESET);
+	reg |= WRAPPER_A9SS_SW_RESET_BIT;
+	writel_relaxed(reg, base + WRAPPER_A9SS_SW_RESET);
+
+	/* Make sure reset is asserted before the mapping is removed */
+	mb();
+
+	iommu = core->fw.iommu_domain;
+
+	unmapped = iommu_unmap(iommu, VENUS_FW_START_ADDR, VENUS_FW_MEM_SIZE);
+	if (unmapped != VENUS_FW_MEM_SIZE)
+		dev_err(dev, "failed to unmap firmware\n");
+
+	return 0;
+}
+
 int venus_boot(struct venus_core *core)
 {
 	struct device *dev = core->dev;
@@ -139,17 +190,30 @@ int venus_boot(struct venus_core *core)
 		return -EINVAL;
 	}
 
-	return qcom_scm_pas_auth_and_reset(VENUS_PAS_ID);
+	if (core->no_tz)
+		ret = venus_boot_no_tz(core, mem_phys, mem_size);
+	else
+		ret = qcom_scm_pas_auth_and_reset(VENUS_PAS_ID);
+
+	return ret;
 }
 
-int venus_shutdown(struct device *dev)
+int venus_shutdown(struct venus_core *core)
 {
-	return qcom_scm_pas_shutdown(VENUS_PAS_ID);
+	int ret;
+
+	if (core->no_tz)
+		ret = venus_shutdown_no_tz(core);
+	else
+		ret = qcom_scm_pas_shutdown(VENUS_PAS_ID);
+
+	return ret;
 }
 
 int venus_firmware_init(struct venus_core *core)
 {
 	struct platform_device_info info;
+	struct iommu_domain *iommu_dom;
 	struct platform_device *pdev;
 	struct device_node *np;
 	int ret;
@@ -179,12 +243,29 @@ int venus_firmware_init(struct venus_core *core)
 	}
 
 	core->fw.dev = &pdev->dev;
+
+	iommu_dom = iommu_domain_alloc(&platform_bus_type);
+	if (!iommu_dom) {
+		dev_err(core->fw.dev, "Failed to allocate iommu domain\n");
+		ret = -ENOMEM;
+		goto err_unregister;
+	}
+
+	ret = iommu_attach_device(iommu_dom, core->fw.dev);
+	if (ret) {
+		dev_err(core->fw.dev, "could not attach device\n");
+		goto err_iommu_free;
+	}
+
 	core->no_tz = true;
+	core->fw.iommu_domain = iommu_dom;
 
 	of_node_put(np);
 
 	return 0;
 
+err_iommu_free:
+	iommu_domain_free(iommu_dom);
 err_unregister:
 	platform_device_unregister(pdev);
 	of_node_put(np);
@@ -193,8 +274,15 @@ int venus_firmware_init(struct venus_core *core)
 
 void venus_firmware_deinit(struct venus_core *core)
 {
+	struct iommu_domain *iommu;
+
 	if (!core->fw.dev)
 		return;
 
+	iommu = core->fw.iommu_domain;
+
+	iommu_detach_device(iommu, core->fw.dev);
+	iommu_domain_free(iommu);
+
 	platform_device_unregister(to_platform_device(core->fw.dev));
 }
diff --git a/drivers/media/platform/qcom/venus/firmware.h b/drivers/media/platform/qcom/venus/firmware.h
index fd7edf0..119a9a4 100644
--- a/drivers/media/platform/qcom/venus/firmware.h
+++ b/drivers/media/platform/qcom/venus/firmware.h
@@ -19,7 +19,7 @@
 int venus_firmware_init(struct venus_core *core);
 void venus_firmware_deinit(struct venus_core *core);
 int venus_boot(struct venus_core *core);
-int venus_shutdown(struct device *dev);
+int venus_shutdown(struct venus_core *core);
 int venus_set_hw_state(struct venus_core *core, bool suspend);
 
 static inline int venus_set_hw_state_suspend(struct venus_core *core)
diff --git a/drivers/media/platform/qcom/venus/hfi_venus_io.h b/drivers/media/platform/qcom/venus/hfi_venus_io.h
index d69f51b..ef0c72a 100644
--- a/drivers/media/platform/qcom/venus/hfi_venus_io.h
+++ b/drivers/media/platform/qcom/venus/hfi_venus_io.h
@@ -119,6 +119,7 @@
 #define WRAPPER_NONPIX_START_ADDR		(WRAPPER_BASE + 0x1030)
 #define WRAPPER_NONPIX_END_ADDR			(WRAPPER_BASE + 0x1034)
 #define WRAPPER_A9SS_SW_RESET			(WRAPPER_BASE + 0x3000)
+#define WRAPPER_A9SS_SW_RESET_BIT		BIT(4)
 
 /* Venus 4xx */
 #define WRAPPER_VCODEC0_MMCC_POWER_STATUS	(WRAPPER_BASE + 0x90)
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v9 5/5] dt-bindings: media: Document bindings for venus firmware device

[Vikash Garodia]

Add devicetree binding documentation for firmware loader for video
hardware running on qualcomm chip.

Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
---
 Documentation/devicetree/bindings/media/qcom,venus.txt | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/media/qcom,venus.txt b/Documentation/devicetree/bindings/media/qcom,venus.txt
index 00d0d1b..7e04586 100644
--- a/Documentation/devicetree/bindings/media/qcom,venus.txt
+++ b/Documentation/devicetree/bindings/media/qcom,venus.txt
@@ -53,7 +53,7 @@
 
 * Subnodes
 The Venus video-codec node must contain two subnodes representing
-video-decoder and video-encoder.
+video-decoder and video-encoder, and one optional firmware subnode.
 
 Every of video-encoder or video-decoder subnode should have:
 
@@ -79,6 +79,13 @@ Every of video-encoder or video-decoder subnode should have:
 		    power domain which is responsible for collapsing
 		    and restoring power to the subcore.
 
+The firmware subnode must have:
+
+- iommus:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: A list of phandle and IOMMU specifier pairs.
+
 * An Example
 	video-codec@1d00000 {
 		compatible = "qcom,msm8916-venus";
@@ -105,4 +112,8 @@ Every of video-encoder or video-decoder subnode should have:
 			clock-names = "core";
 			power-domains = <&mmcc VENUS_CORE1_GDSC>;
 		};
+
+		video-firmware {
+			iommus = <&apps_iommu 0x10b2 0x0>;
+		};
 	};
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v9 1/5] venus: firmware: add routine to reset ARM9

[Alexandre Courbot]

On Wed, Sep 19, 2018 at 8:43 AM Vikash Garodia <vgarodia@codeaurora.org> wrote:
>
> Add routine to reset the ARM9 and brings it out of reset. Also
> abstract the Venus CPU state handling with a new function. This
> is in preparation to add PIL functionality in venus driver.
>
> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
> ---
>  drivers/media/platform/qcom/venus/core.h         |  2 ++
>  drivers/media/platform/qcom/venus/firmware.c     | 33 ++++++++++++++++++++++++
>  drivers/media/platform/qcom/venus/firmware.h     | 11 ++++++++
>  drivers/media/platform/qcom/venus/hfi_venus.c    | 13 +++-------
>  drivers/media/platform/qcom/venus/hfi_venus_io.h |  7 +++++
>  5 files changed, 57 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
> index 2f02365..dfd5c10 100644
> --- a/drivers/media/platform/qcom/venus/core.h
> +++ b/drivers/media/platform/qcom/venus/core.h
> @@ -98,6 +98,7 @@ struct venus_caps {
>   * @dev:               convenience struct device pointer
>   * @dev_dec:   convenience struct device pointer for decoder device
>   * @dev_enc:   convenience struct device pointer for encoder device
> + * @no_tz:     a flag that suggests presence of trustzone

Looks like it suggests the absence of trustzone?

Actually I would rename it as use_tz and set it if TrustZone is used.
This would avoid double-negative statements like what we see below.

>   * @lock:      a lock for this strucure
>   * @instances: a list_head of all instances
>   * @insts_count:       num of instances
> @@ -129,6 +130,7 @@ struct venus_core {
>         struct device *dev;
>         struct device *dev_dec;
>         struct device *dev_enc;
> +       bool no_tz;
>         struct mutex lock;
>         struct list_head instances;
>         atomic_t insts_count;
> diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c
> index c4a5778..f2ae2f0 100644
> --- a/drivers/media/platform/qcom/venus/firmware.c
> +++ b/drivers/media/platform/qcom/venus/firmware.c
> @@ -22,10 +22,43 @@
>  #include <linux/sizes.h>
>  #include <linux/soc/qcom/mdt_loader.h>
>
> +#include "core.h"
>  #include "firmware.h"
> +#include "hfi_venus_io.h"
>
>  #define VENUS_PAS_ID                   9
>  #define VENUS_FW_MEM_SIZE              (6 * SZ_1M)
> +#define VENUS_FW_START_ADDR            0x0
> +
> +static void venus_reset_cpu(struct venus_core *core)
> +{
> +       void __iomem *base = core->base;
> +
> +       writel(0, base + WRAPPER_FW_START_ADDR);
> +       writel(VENUS_FW_MEM_SIZE, base + WRAPPER_FW_END_ADDR);
> +       writel(0, base + WRAPPER_CPA_START_ADDR);
> +       writel(VENUS_FW_MEM_SIZE, base + WRAPPER_CPA_END_ADDR);
> +       writel(VENUS_FW_MEM_SIZE, base + WRAPPER_NONPIX_START_ADDR);
> +       writel(VENUS_FW_MEM_SIZE, base + WRAPPER_NONPIX_END_ADDR);
> +       writel(0x0, base + WRAPPER_CPU_CGC_DIS);
> +       writel(0x0, base + WRAPPER_CPU_CLOCK_CONFIG);
> +
> +       /* Bring ARM9 out of reset */
> +       writel(0, base + WRAPPER_A9SS_SW_RESET);
> +}
> +
> +int venus_set_hw_state(struct venus_core *core, bool resume)
> +{
> +       if (!core->no_tz)

This is the kind of double negative statement I was referring do
above: "if we do not not have TrustZone". Turning it into

    if (core->use_tz)

would save the reader a few neurons. :)

> +               return qcom_scm_set_remote_state(resume, 0);
> +
> +       if (resume)
> +               venus_reset_cpu(core);
> +       else
> +               writel(1, core->base + WRAPPER_A9SS_SW_RESET);
> +
> +       return 0;
> +}
>
>  int venus_boot(struct device *dev, const char *fwname)
>  {
> diff --git a/drivers/media/platform/qcom/venus/firmware.h b/drivers/media/platform/qcom/venus/firmware.h
> index 428efb5..397570c 100644
> --- a/drivers/media/platform/qcom/venus/firmware.h
> +++ b/drivers/media/platform/qcom/venus/firmware.h
> @@ -18,5 +18,16 @@
>
>  int venus_boot(struct device *dev, const char *fwname);
>  int venus_shutdown(struct device *dev);
> +int venus_set_hw_state(struct venus_core *core, bool suspend);
> +
> +static inline int venus_set_hw_state_suspend(struct venus_core *core)
> +{
> +       return venus_set_hw_state(core, false);
> +}
> +
> +static inline int venus_set_hw_state_resume(struct venus_core *core)
> +{
> +       return venus_set_hw_state(core, true);
> +}

I think these two venus_set_hw_state_suspend() and
venus_set_hw_state_resume() are superfluous, if you want to make the
state explicit you can also define an enum { SUSPEND, RESUME } to use
as argument of venus_set_hw_state() and call it directly.

>
>  #endif
> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
> index 1240855..074837e 100644
> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
> @@ -19,7 +19,6 @@
>  #include <linux/interrupt.h>
>  #include <linux/iopoll.h>
>  #include <linux/kernel.h>
> -#include <linux/qcom_scm.h>
>  #include <linux/slab.h>
>
>  #include "core.h"
> @@ -27,6 +26,7 @@
>  #include "hfi_msgs.h"
>  #include "hfi_venus.h"
>  #include "hfi_venus_io.h"
> +#include "firmware.h"
>
>  #define HFI_MASK_QHDR_TX_TYPE          0xff000000
>  #define HFI_MASK_QHDR_RX_TYPE          0x00ff0000
> @@ -55,11 +55,6 @@
>  #define IFACEQ_VAR_LARGE_PKT_SIZE      512
>  #define IFACEQ_VAR_HUGE_PKT_SIZE       (1024 * 12)
>
> -enum tzbsp_video_state {
> -       TZBSP_VIDEO_STATE_SUSPEND = 0,
> -       TZBSP_VIDEO_STATE_RESUME
> -};
> -
>  struct hfi_queue_table_header {
>         u32 version;
>         u32 size;
> @@ -575,7 +570,7 @@ static int venus_power_off(struct venus_hfi_device *hdev)
>         if (!hdev->power_enabled)
>                 return 0;
>
> -       ret = qcom_scm_set_remote_state(TZBSP_VIDEO_STATE_SUSPEND, 0);
> +       ret = venus_set_hw_state_suspend(hdev->core);
>         if (ret)
>                 return ret;
>
> @@ -595,7 +590,7 @@ static int venus_power_on(struct venus_hfi_device *hdev)
>         if (hdev->power_enabled)
>                 return 0;
>
> -       ret = qcom_scm_set_remote_state(TZBSP_VIDEO_STATE_RESUME, 0);
> +       ret = venus_set_hw_state_resume(hdev->core);
>         if (ret)
>                 goto err;
>
> @@ -608,7 +603,7 @@ static int venus_power_on(struct venus_hfi_device *hdev)
>         return 0;
>
>  err_suspend:
> -       qcom_scm_set_remote_state(TZBSP_VIDEO_STATE_SUSPEND, 0);
> +       venus_set_hw_state_suspend(hdev->core);
>  err:
>         hdev->power_enabled = false;
>         return ret;
> diff --git a/drivers/media/platform/qcom/venus/hfi_venus_io.h b/drivers/media/platform/qcom/venus/hfi_venus_io.h
> index def0926..d69f51b 100644
> --- a/drivers/media/platform/qcom/venus/hfi_venus_io.h
> +++ b/drivers/media/platform/qcom/venus/hfi_venus_io.h
> @@ -112,6 +112,13 @@
>  #define WRAPPER_CPU_STATUS                     (WRAPPER_BASE + 0x2014)
>  #define WRAPPER_CPU_STATUS_WFI                 BIT(0)
>  #define WRAPPER_SW_RESET                       (WRAPPER_BASE + 0x3000)
> +#define WRAPPER_CPA_START_ADDR                 (WRAPPER_BASE + 0x1020)
> +#define WRAPPER_CPA_END_ADDR                   (WRAPPER_BASE + 0x1024)
> +#define WRAPPER_FW_START_ADDR                  (WRAPPER_BASE + 0x1028)
> +#define WRAPPER_FW_END_ADDR                    (WRAPPER_BASE + 0x102C)
> +#define WRAPPER_NONPIX_START_ADDR              (WRAPPER_BASE + 0x1030)
> +#define WRAPPER_NONPIX_END_ADDR                        (WRAPPER_BASE + 0x1034)
> +#define WRAPPER_A9SS_SW_RESET                  (WRAPPER_BASE + 0x3000)
>
>  /* Venus 4xx */
>  #define WRAPPER_VCODEC0_MMCC_POWER_STATUS      (WRAPPER_BASE + 0x90)
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>

Re: [PATCH v9 5/5] dt-bindings: media: Document bindings for venus firmware device

[Alexandre Courbot]

On Wed, Sep 19, 2018 at 8:44 AM Vikash Garodia <vgarodia@codeaurora.org> wrote:
>
> Add devicetree binding documentation for firmware loader for video
> hardware running on qualcomm chip.
>
> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
> ---
>  Documentation/devicetree/bindings/media/qcom,venus.txt | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/media/qcom,venus.txt b/Documentation/devicetree/bindings/media/qcom,venus.txt
> index 00d0d1b..7e04586 100644
> --- a/Documentation/devicetree/bindings/media/qcom,venus.txt
> +++ b/Documentation/devicetree/bindings/media/qcom,venus.txt
> @@ -53,7 +53,7 @@
>
>  * Subnodes
>  The Venus video-codec node must contain two subnodes representing
> -video-decoder and video-encoder.
> +video-decoder and video-encoder, and one optional firmware subnode.

I think I mentioned this in my previous review, but it would be nice
to explain in which circumstances the firmware node is optional. I.e.
it should not be specified if TrustZone is used.

>
>  Every of video-encoder or video-decoder subnode should have:
>
> @@ -79,6 +79,13 @@ Every of video-encoder or video-decoder subnode should have:
>                     power domain which is responsible for collapsing
>                     and restoring power to the subcore.
>
> +The firmware subnode must have:
> +
> +- iommus:
> +       Usage: required
> +       Value type: <prop-encoded-array>
> +       Definition: A list of phandle and IOMMU specifier pairs.
> +
>  * An Example
>         video-codec@1d00000 {
>                 compatible = "qcom,msm8916-venus";
> @@ -105,4 +112,8 @@ Every of video-encoder or video-decoder subnode should have:
>                         clock-names = "core";
>                         power-domains = <&mmcc VENUS_CORE1_GDSC>;
>                 };
> +
> +               video-firmware {
> +                       iommus = <&apps_iommu 0x10b2 0x0>;
> +               };
>         };
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>

Re: [PATCH v9 1/5] venus: firmware: add routine to reset ARM9

[Stanimir Varbanov]

Hi Alex,

On 09/19/2018 10:32 AM, Alexandre Courbot wrote:
> On Wed, Sep 19, 2018 at 8:43 AM Vikash Garodia <vgarodia@codeaurora.org> wrote:
>>
>> Add routine to reset the ARM9 and brings it out of reset. Also
>> abstract the Venus CPU state handling with a new function. This
>> is in preparation to add PIL functionality in venus driver.
>>
>> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
>> ---
>>  drivers/media/platform/qcom/venus/core.h         |  2 ++
>>  drivers/media/platform/qcom/venus/firmware.c     | 33 ++++++++++++++++++++++++
>>  drivers/media/platform/qcom/venus/firmware.h     | 11 ++++++++
>>  drivers/media/platform/qcom/venus/hfi_venus.c    | 13 +++-------
>>  drivers/media/platform/qcom/venus/hfi_venus_io.h |  7 +++++
>>  5 files changed, 57 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
>> index 2f02365..dfd5c10 100644
>> --- a/drivers/media/platform/qcom/venus/core.h
>> +++ b/drivers/media/platform/qcom/venus/core.h
>> @@ -98,6 +98,7 @@ struct venus_caps {
>>   * @dev:               convenience struct device pointer
>>   * @dev_dec:   convenience struct device pointer for decoder device
>>   * @dev_enc:   convenience struct device pointer for encoder device
>> + * @no_tz:     a flag that suggests presence of trustzone
> 
> Looks like it suggests the absence of trustzone?
> 
> Actually I would rename it as use_tz and set it if TrustZone is used.
> This would avoid double-negative statements like what we see below.

I find this suggestion reasonable.

> 
>>   * @lock:      a lock for this strucure
>>   * @instances: a list_head of all instances
>>   * @insts_count:       num of instances
>> @@ -129,6 +130,7 @@ struct venus_core {
>>         struct device *dev;
>>         struct device *dev_dec;
>>         struct device *dev_enc;
>> +       bool no_tz;
>>         struct mutex lock;
>>         struct list_head instances;
>>         atomic_t insts_count;
>> diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c
>> index c4a5778..f2ae2f0 100644
>> --- a/drivers/media/platform/qcom/venus/firmware.c
>> +++ b/drivers/media/platform/qcom/venus/firmware.c
>> @@ -22,10 +22,43 @@
>>  #include <linux/sizes.h>
>>  #include <linux/soc/qcom/mdt_loader.h>
>>
>> +#include "core.h"
>>  #include "firmware.h"
>> +#include "hfi_venus_io.h"
>>
>>  #define VENUS_PAS_ID                   9
>>  #define VENUS_FW_MEM_SIZE              (6 * SZ_1M)
>> +#define VENUS_FW_START_ADDR            0x0
>> +
>> +static void venus_reset_cpu(struct venus_core *core)
>> +{
>> +       void __iomem *base = core->base;
>> +
>> +       writel(0, base + WRAPPER_FW_START_ADDR);
>> +       writel(VENUS_FW_MEM_SIZE, base + WRAPPER_FW_END_ADDR);
>> +       writel(0, base + WRAPPER_CPA_START_ADDR);
>> +       writel(VENUS_FW_MEM_SIZE, base + WRAPPER_CPA_END_ADDR);
>> +       writel(VENUS_FW_MEM_SIZE, base + WRAPPER_NONPIX_START_ADDR);
>> +       writel(VENUS_FW_MEM_SIZE, base + WRAPPER_NONPIX_END_ADDR);
>> +       writel(0x0, base + WRAPPER_CPU_CGC_DIS);
>> +       writel(0x0, base + WRAPPER_CPU_CLOCK_CONFIG);
>> +
>> +       /* Bring ARM9 out of reset */
>> +       writel(0, base + WRAPPER_A9SS_SW_RESET);
>> +}
>> +
>> +int venus_set_hw_state(struct venus_core *core, bool resume)
>> +{
>> +       if (!core->no_tz)
> 
> This is the kind of double negative statement I was referring do
> above: "if we do not not have TrustZone". Turning it into
> 
>     if (core->use_tz)
> 
> would save the reader a few neurons. :)
> 
>> +               return qcom_scm_set_remote_state(resume, 0);
>> +
>> +       if (resume)
>> +               venus_reset_cpu(core);
>> +       else
>> +               writel(1, core->base + WRAPPER_A9SS_SW_RESET);
>> +
>> +       return 0;
>> +}
>>
>>  int venus_boot(struct device *dev, const char *fwname)
>>  {
>> diff --git a/drivers/media/platform/qcom/venus/firmware.h b/drivers/media/platform/qcom/venus/firmware.h
>> index 428efb5..397570c 100644
>> --- a/drivers/media/platform/qcom/venus/firmware.h
>> +++ b/drivers/media/platform/qcom/venus/firmware.h
>> @@ -18,5 +18,16 @@
>>
>>  int venus_boot(struct device *dev, const char *fwname);
>>  int venus_shutdown(struct device *dev);
>> +int venus_set_hw_state(struct venus_core *core, bool suspend);
>> +
>> +static inline int venus_set_hw_state_suspend(struct venus_core *core)
>> +{
>> +       return venus_set_hw_state(core, false);
>> +}
>> +
>> +static inline int venus_set_hw_state_resume(struct venus_core *core)
>> +{
>> +       return venus_set_hw_state(core, true);
>> +}
> 
> I think these two venus_set_hw_state_suspend() and
> venus_set_hw_state_resume() are superfluous, if you want to make the
> state explicit you can also define an enum { SUSPEND, RESUME } to use
> as argument of venus_set_hw_state() and call it directly.

Infact this was by my request, and I wanted to avoid enum and have the
type of the action in the function name and also avoid one extra
function argument. Of course it is a matter of taste.

-- 
regards,
Stan

Re: [PATCH v9 1/5] venus: firmware: add routine to reset ARM9

[Vikash Garodia]

On 2018-09-19 20:30, Stanimir Varbanov wrote:
> Hi Alex,
> 
> On 09/19/2018 10:32 AM, Alexandre Courbot wrote:
>> On Wed, Sep 19, 2018 at 8:43 AM Vikash Garodia 
>> <vgarodia@codeaurora.org> wrote:
>>> 
>>> Add routine to reset the ARM9 and brings it out of reset. Also
>>> abstract the Venus CPU state handling with a new function. This
>>> is in preparation to add PIL functionality in venus driver.
>>> 
>>> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
>>> ---
>>>  drivers/media/platform/qcom/venus/core.h         |  2 ++
>>>  drivers/media/platform/qcom/venus/firmware.c     | 33 
>>> ++++++++++++++++++++++++
>>>  drivers/media/platform/qcom/venus/firmware.h     | 11 ++++++++
>>>  drivers/media/platform/qcom/venus/hfi_venus.c    | 13 +++-------
>>>  drivers/media/platform/qcom/venus/hfi_venus_io.h |  7 +++++
>>>  5 files changed, 57 insertions(+), 9 deletions(-)
>>> 
>>> diff --git a/drivers/media/platform/qcom/venus/core.h 
>>> b/drivers/media/platform/qcom/venus/core.h
>>> index 2f02365..dfd5c10 100644
>>> --- a/drivers/media/platform/qcom/venus/core.h
>>> +++ b/drivers/media/platform/qcom/venus/core.h
>>> @@ -98,6 +98,7 @@ struct venus_caps {
>>>   * @dev:               convenience struct device pointer
>>>   * @dev_dec:   convenience struct device pointer for decoder device
>>>   * @dev_enc:   convenience struct device pointer for encoder device
>>> + * @no_tz:     a flag that suggests presence of trustzone
>> 
>> Looks like it suggests the absence of trustzone?
>> 
>> Actually I would rename it as use_tz and set it if TrustZone is used.
>> This would avoid double-negative statements like what we see below.
> 
> I find this suggestion reasonable.

Initially i planned to keep it as a positive flag. The reason behind 
keeping it
as no_tz was to keep the default value of this flag to 0 indicating tz 
is present
by default.
I can switch it to use_tz though and set it in firmware_init after the 
presence of
firmware node is checked.

>> 
>>>   * @lock:      a lock for this strucure
>>>   * @instances: a list_head of all instances
>>>   * @insts_count:       num of instances
>>> @@ -129,6 +130,7 @@ struct venus_core {
>>>         struct device *dev;
>>>         struct device *dev_dec;
>>>         struct device *dev_enc;
>>> +       bool no_tz;
>>>         struct mutex lock;
>>>         struct list_head instances;
>>>         atomic_t insts_count;
>>> diff --git a/drivers/media/platform/qcom/venus/firmware.c 
>>> b/drivers/media/platform/qcom/venus/firmware.c
>>> index c4a5778..f2ae2f0 100644
>>> --- a/drivers/media/platform/qcom/venus/firmware.c
>>> +++ b/drivers/media/platform/qcom/venus/firmware.c
>>> @@ -22,10 +22,43 @@
>>>  #include <linux/sizes.h>
>>>  #include <linux/soc/qcom/mdt_loader.h>
>>> 
>>> +#include "core.h"
>>>  #include "firmware.h"
>>> +#include "hfi_venus_io.h"
>>> 
>>>  #define VENUS_PAS_ID                   9
>>>  #define VENUS_FW_MEM_SIZE              (6 * SZ_1M)
>>> +#define VENUS_FW_START_ADDR            0x0
>>> +
>>> +static void venus_reset_cpu(struct venus_core *core)
>>> +{
>>> +       void __iomem *base = core->base;
>>> +
>>> +       writel(0, base + WRAPPER_FW_START_ADDR);
>>> +       writel(VENUS_FW_MEM_SIZE, base + WRAPPER_FW_END_ADDR);
>>> +       writel(0, base + WRAPPER_CPA_START_ADDR);
>>> +       writel(VENUS_FW_MEM_SIZE, base + WRAPPER_CPA_END_ADDR);
>>> +       writel(VENUS_FW_MEM_SIZE, base + WRAPPER_NONPIX_START_ADDR);
>>> +       writel(VENUS_FW_MEM_SIZE, base + WRAPPER_NONPIX_END_ADDR);
>>> +       writel(0x0, base + WRAPPER_CPU_CGC_DIS);
>>> +       writel(0x0, base + WRAPPER_CPU_CLOCK_CONFIG);
>>> +
>>> +       /* Bring ARM9 out of reset */
>>> +       writel(0, base + WRAPPER_A9SS_SW_RESET);
>>> +}
>>> +
>>> +int venus_set_hw_state(struct venus_core *core, bool resume)
>>> +{
>>> +       if (!core->no_tz)
>> 
>> This is the kind of double negative statement I was referring do
>> above: "if we do not not have TrustZone". Turning it into
>> 
>>     if (core->use_tz)
>> 
>> would save the reader a few neurons. :)
>> 
>>> +               return qcom_scm_set_remote_state(resume, 0);
>>> +
>>> +       if (resume)
>>> +               venus_reset_cpu(core);
>>> +       else
>>> +               writel(1, core->base + WRAPPER_A9SS_SW_RESET);
>>> +
>>> +       return 0;
>>> +}
>>> 
>>>  int venus_boot(struct device *dev, const char *fwname)
>>>  {
>>> diff --git a/drivers/media/platform/qcom/venus/firmware.h 
>>> b/drivers/media/platform/qcom/venus/firmware.h
>>> index 428efb5..397570c 100644
>>> --- a/drivers/media/platform/qcom/venus/firmware.h
>>> +++ b/drivers/media/platform/qcom/venus/firmware.h
>>> @@ -18,5 +18,16 @@
>>> 
>>>  int venus_boot(struct device *dev, const char *fwname);
>>>  int venus_shutdown(struct device *dev);
>>> +int venus_set_hw_state(struct venus_core *core, bool suspend);
>>> +
>>> +static inline int venus_set_hw_state_suspend(struct venus_core 
>>> *core)
>>> +{
>>> +       return venus_set_hw_state(core, false);
>>> +}
>>> +
>>> +static inline int venus_set_hw_state_resume(struct venus_core *core)
>>> +{
>>> +       return venus_set_hw_state(core, true);
>>> +}
>> 
>> I think these two venus_set_hw_state_suspend() and
>> venus_set_hw_state_resume() are superfluous, if you want to make the
>> state explicit you can also define an enum { SUSPEND, RESUME } to use
>> as argument of venus_set_hw_state() and call it directly.
> 
> Infact this was by my request, and I wanted to avoid enum and have the
> type of the action in the function name and also avoid one extra
> function argument. Of course it is a matter of taste.

Re: [PATCH v9 1/5] venus: firmware: add routine to reset ARM9

[Alexandre Courbot]

On Thu, Sep 20, 2018 at 2:55 AM Vikash Garodia <vgarodia@codeaurora.org> wrote:
>
> On 2018-09-19 20:30, Stanimir Varbanov wrote:
> > Hi Alex,
> >
> > On 09/19/2018 10:32 AM, Alexandre Courbot wrote:
> >> On Wed, Sep 19, 2018 at 8:43 AM Vikash Garodia
> >> <vgarodia@codeaurora.org> wrote:
> >>>
> >>> Add routine to reset the ARM9 and brings it out of reset. Also
> >>> abstract the Venus CPU state handling with a new function. This
> >>> is in preparation to add PIL functionality in venus driver.
> >>>
> >>> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
> >>> ---
> >>>  drivers/media/platform/qcom/venus/core.h         |  2 ++
> >>>  drivers/media/platform/qcom/venus/firmware.c     | 33
> >>> ++++++++++++++++++++++++
> >>>  drivers/media/platform/qcom/venus/firmware.h     | 11 ++++++++
> >>>  drivers/media/platform/qcom/venus/hfi_venus.c    | 13 +++-------
> >>>  drivers/media/platform/qcom/venus/hfi_venus_io.h |  7 +++++
> >>>  5 files changed, 57 insertions(+), 9 deletions(-)
> >>>
> >>> diff --git a/drivers/media/platform/qcom/venus/core.h
> >>> b/drivers/media/platform/qcom/venus/core.h
> >>> index 2f02365..dfd5c10 100644
> >>> --- a/drivers/media/platform/qcom/venus/core.h
> >>> +++ b/drivers/media/platform/qcom/venus/core.h
> >>> @@ -98,6 +98,7 @@ struct venus_caps {
> >>>   * @dev:               convenience struct device pointer
> >>>   * @dev_dec:   convenience struct device pointer for decoder device
> >>>   * @dev_enc:   convenience struct device pointer for encoder device
> >>> + * @no_tz:     a flag that suggests presence of trustzone
> >>
> >> Looks like it suggests the absence of trustzone?
> >>
> >> Actually I would rename it as use_tz and set it if TrustZone is used.
> >> This would avoid double-negative statements like what we see below.
> >
> > I find this suggestion reasonable.
>
> Initially i planned to keep it as a positive flag. The reason behind
> keeping it
> as no_tz was to keep the default value of this flag to 0 indicating tz
> is present
> by default.
> I can switch it to use_tz though and set it in firmware_init after the
> presence of
> firmware node is checked.

Making sure the flag is explicitly initialized instead of relying on
default initialization is another good reason to have that change
IMHO. :)

>
> >>
> >>>   * @lock:      a lock for this strucure
> >>>   * @instances: a list_head of all instances
> >>>   * @insts_count:       num of instances
> >>> @@ -129,6 +130,7 @@ struct venus_core {
> >>>         struct device *dev;
> >>>         struct device *dev_dec;
> >>>         struct device *dev_enc;
> >>> +       bool no_tz;
> >>>         struct mutex lock;
> >>>         struct list_head instances;
> >>>         atomic_t insts_count;
> >>> diff --git a/drivers/media/platform/qcom/venus/firmware.c
> >>> b/drivers/media/platform/qcom/venus/firmware.c
> >>> index c4a5778..f2ae2f0 100644
> >>> --- a/drivers/media/platform/qcom/venus/firmware.c
> >>> +++ b/drivers/media/platform/qcom/venus/firmware.c
> >>> @@ -22,10 +22,43 @@
> >>>  #include <linux/sizes.h>
> >>>  #include <linux/soc/qcom/mdt_loader.h>
> >>>
> >>> +#include "core.h"
> >>>  #include "firmware.h"
> >>> +#include "hfi_venus_io.h"
> >>>
> >>>  #define VENUS_PAS_ID                   9
> >>>  #define VENUS_FW_MEM_SIZE              (6 * SZ_1M)
> >>> +#define VENUS_FW_START_ADDR            0x0
> >>> +
> >>> +static void venus_reset_cpu(struct venus_core *core)
> >>> +{
> >>> +       void __iomem *base = core->base;
> >>> +
> >>> +       writel(0, base + WRAPPER_FW_START_ADDR);
> >>> +       writel(VENUS_FW_MEM_SIZE, base + WRAPPER_FW_END_ADDR);
> >>> +       writel(0, base + WRAPPER_CPA_START_ADDR);
> >>> +       writel(VENUS_FW_MEM_SIZE, base + WRAPPER_CPA_END_ADDR);
> >>> +       writel(VENUS_FW_MEM_SIZE, base + WRAPPER_NONPIX_START_ADDR);
> >>> +       writel(VENUS_FW_MEM_SIZE, base + WRAPPER_NONPIX_END_ADDR);
> >>> +       writel(0x0, base + WRAPPER_CPU_CGC_DIS);
> >>> +       writel(0x0, base + WRAPPER_CPU_CLOCK_CONFIG);
> >>> +
> >>> +       /* Bring ARM9 out of reset */
> >>> +       writel(0, base + WRAPPER_A9SS_SW_RESET);
> >>> +}
> >>> +
> >>> +int venus_set_hw_state(struct venus_core *core, bool resume)
> >>> +{
> >>> +       if (!core->no_tz)
> >>
> >> This is the kind of double negative statement I was referring do
> >> above: "if we do not not have TrustZone". Turning it into
> >>
> >>     if (core->use_tz)
> >>
> >> would save the reader a few neurons. :)
> >>
> >>> +               return qcom_scm_set_remote_state(resume, 0);
> >>> +
> >>> +       if (resume)
> >>> +               venus_reset_cpu(core);
> >>> +       else
> >>> +               writel(1, core->base + WRAPPER_A9SS_SW_RESET);
> >>> +
> >>> +       return 0;
> >>> +}
> >>>
> >>>  int venus_boot(struct device *dev, const char *fwname)
> >>>  {
> >>> diff --git a/drivers/media/platform/qcom/venus/firmware.h
> >>> b/drivers/media/platform/qcom/venus/firmware.h
> >>> index 428efb5..397570c 100644
> >>> --- a/drivers/media/platform/qcom/venus/firmware.h
> >>> +++ b/drivers/media/platform/qcom/venus/firmware.h
> >>> @@ -18,5 +18,16 @@
> >>>
> >>>  int venus_boot(struct device *dev, const char *fwname);
> >>>  int venus_shutdown(struct device *dev);
> >>> +int venus_set_hw_state(struct venus_core *core, bool suspend);
> >>> +
> >>> +static inline int venus_set_hw_state_suspend(struct venus_core
> >>> *core)
> >>> +{
> >>> +       return venus_set_hw_state(core, false);
> >>> +}
> >>> +
> >>> +static inline int venus_set_hw_state_resume(struct venus_core *core)
> >>> +{
> >>> +       return venus_set_hw_state(core, true);
> >>> +}
> >>
> >> I think these two venus_set_hw_state_suspend() and
> >> venus_set_hw_state_resume() are superfluous, if you want to make the
> >> state explicit you can also define an enum { SUSPEND, RESUME } to use
> >> as argument of venus_set_hw_state() and call it directly.
> >
> > Infact this was by my request, and I wanted to avoid enum and have the
> > type of the action in the function name and also avoid one extra
> > function argument. Of course it is a matter of taste.

That's reasonable as well. I really don't feel strongly about this, so
please feel free to keep it as it is.

Re: [PATCH v9 5/5] dt-bindings: media: Document bindings for venus firmware device

[Rob Herring]

On Wed, Sep 19, 2018 at 05:13:12AM +0530, Vikash Garodia wrote:
> Add devicetree binding documentation for firmware loader for video
> hardware running on qualcomm chip.
> 
> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
> ---
>  Documentation/devicetree/bindings/media/qcom,venus.txt | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)

Something change in this? If not, then please add acks/reviewed-bys when 
posting new versions.

Rob

Re: [PATCH v9 5/5] dt-bindings: media: Document bindings for venus firmware device

[Vikash Garodia]

Hi Rob,

On 2018-09-25 20:54, Rob Herring wrote:
> On Wed, Sep 19, 2018 at 05:13:12AM +0530, Vikash Garodia wrote:
>> Add devicetree binding documentation for firmware loader for video
>> hardware running on qualcomm chip.
>> 
>> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
>> ---
>>  Documentation/devicetree/bindings/media/qcom,venus.txt | 13 
>> ++++++++++++-
>>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> Something change in this? If not, then please add acks/reviewed-bys 
> when
> posting new versions.

Same as the one reviewed by you earlier. Will add your review-by 
signature.

Re: [PATCH v9 3/5] venus: firmware: register separate platform_device for firmware loader

[Stanimir Varbanov]

Hi Vikash,

On 09/19/2018 02:43 AM, Vikash Garodia wrote:
> From: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> 
> This registers a firmware platform_device and associate it with
> video-firmware DT subnode. Then calls dma configure to initialize
> dma and iommu.

Please replace the description with something like this:

This registers a firmware platform_device and associates it with
video-firmware DT subnode, by that way we are able to parse iommu
configuration.

> 
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> ---
>  drivers/media/platform/qcom/venus/core.c     | 14 +++++---
>  drivers/media/platform/qcom/venus/core.h     |  3 ++
>  drivers/media/platform/qcom/venus/firmware.c | 54 ++++++++++++++++++++++++++++
>  drivers/media/platform/qcom/venus/firmware.h |  2 ++
>  4 files changed, 69 insertions(+), 4 deletions(-)
> 

-- 
regards,
Stan

Re: [PATCH v9 1/5] venus: firmware: add routine to reset ARM9

[Stanimir Varbanov]

Hi,

On 09/20/2018 06:31 AM, Alexandre Courbot wrote:
> On Thu, Sep 20, 2018 at 2:55 AM Vikash Garodia <vgarodia@codeaurora.org> wrote:
>>
>> On 2018-09-19 20:30, Stanimir Varbanov wrote:
>>> Hi Alex,
>>>
>>> On 09/19/2018 10:32 AM, Alexandre Courbot wrote:
>>>> On Wed, Sep 19, 2018 at 8:43 AM Vikash Garodia
>>>> <vgarodia@codeaurora.org> wrote:
>>>>>
>>>>> Add routine to reset the ARM9 and brings it out of reset. Also
>>>>> abstract the Venus CPU state handling with a new function. This
>>>>> is in preparation to add PIL functionality in venus driver.
>>>>>
>>>>> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
>>>>> ---
>>>>>  drivers/media/platform/qcom/venus/core.h         |  2 ++
>>>>>  drivers/media/platform/qcom/venus/firmware.c     | 33
>>>>> ++++++++++++++++++++++++
>>>>>  drivers/media/platform/qcom/venus/firmware.h     | 11 ++++++++
>>>>>  drivers/media/platform/qcom/venus/hfi_venus.c    | 13 +++-------
>>>>>  drivers/media/platform/qcom/venus/hfi_venus_io.h |  7 +++++
>>>>>  5 files changed, 57 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/drivers/media/platform/qcom/venus/core.h
>>>>> b/drivers/media/platform/qcom/venus/core.h
>>>>> index 2f02365..dfd5c10 100644
>>>>> --- a/drivers/media/platform/qcom/venus/core.h
>>>>> +++ b/drivers/media/platform/qcom/venus/core.h
>>>>> @@ -98,6 +98,7 @@ struct venus_caps {
>>>>>   * @dev:               convenience struct device pointer
>>>>>   * @dev_dec:   convenience struct device pointer for decoder device
>>>>>   * @dev_enc:   convenience struct device pointer for encoder device
>>>>> + * @no_tz:     a flag that suggests presence of trustzone
>>>>
>>>> Looks like it suggests the absence of trustzone?
>>>>
>>>> Actually I would rename it as use_tz and set it if TrustZone is used.
>>>> This would avoid double-negative statements like what we see below.
>>>
>>> I find this suggestion reasonable.
>>
>> Initially i planned to keep it as a positive flag. The reason behind
>> keeping it
>> as no_tz was to keep the default value of this flag to 0 indicating tz
>> is present
>> by default.
>> I can switch it to use_tz though and set it in firmware_init after the
>> presence of
>> firmware node is checked.
> 
> Making sure the flag is explicitly initialized instead of relying on
> default initialization is another good reason to have that change
> IMHO. :)

Vikash, care to send a new version, or will fix that with follow up patches?

-- 
regards,
Stan

Re: [PATCH v9 1/5] venus: firmware: add routine to reset ARM9

[Vikash Garodia]

Hi Stanimir,

On 2018-10-01 20:00, Stanimir Varbanov wrote:
> Hi,
> 
> On 09/20/2018 06:31 AM, Alexandre Courbot wrote:
>> On Thu, Sep 20, 2018 at 2:55 AM Vikash Garodia 
>> <vgarodia@codeaurora.org> wrote:
>>> 
>>> On 2018-09-19 20:30, Stanimir Varbanov wrote:
>>>> Hi Alex,
>>>> 
>>>> On 09/19/2018 10:32 AM, Alexandre Courbot wrote:
>>>>> On Wed, Sep 19, 2018 at 8:43 AM Vikash Garodia
>>>>> <vgarodia@codeaurora.org> wrote:
>>>>>> 
>>>>>> Add routine to reset the ARM9 and brings it out of reset. Also
>>>>>> abstract the Venus CPU state handling with a new function. This
>>>>>> is in preparation to add PIL functionality in venus driver.
>>>>>> 
>>>>>> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
>>>>>> ---
>>>>>>  drivers/media/platform/qcom/venus/core.h         |  2 ++
>>>>>>  drivers/media/platform/qcom/venus/firmware.c     | 33
>>>>>> ++++++++++++++++++++++++
>>>>>>  drivers/media/platform/qcom/venus/firmware.h     | 11 ++++++++
>>>>>>  drivers/media/platform/qcom/venus/hfi_venus.c    | 13 +++-------
>>>>>>  drivers/media/platform/qcom/venus/hfi_venus_io.h |  7 +++++
>>>>>>  5 files changed, 57 insertions(+), 9 deletions(-)
>>>>>> 
>>>>>> diff --git a/drivers/media/platform/qcom/venus/core.h
>>>>>> b/drivers/media/platform/qcom/venus/core.h
>>>>>> index 2f02365..dfd5c10 100644
>>>>>> --- a/drivers/media/platform/qcom/venus/core.h
>>>>>> +++ b/drivers/media/platform/qcom/venus/core.h
>>>>>> @@ -98,6 +98,7 @@ struct venus_caps {
>>>>>>   * @dev:               convenience struct device pointer
>>>>>>   * @dev_dec:   convenience struct device pointer for decoder 
>>>>>> device
>>>>>>   * @dev_enc:   convenience struct device pointer for encoder 
>>>>>> device
>>>>>> + * @no_tz:     a flag that suggests presence of trustzone
>>>>> 
>>>>> Looks like it suggests the absence of trustzone?
>>>>> 
>>>>> Actually I would rename it as use_tz and set it if TrustZone is 
>>>>> used.
>>>>> This would avoid double-negative statements like what we see below.
>>>> 
>>>> I find this suggestion reasonable.
>>> 
>>> Initially i planned to keep it as a positive flag. The reason behind
>>> keeping it
>>> as no_tz was to keep the default value of this flag to 0 indicating 
>>> tz
>>> is present
>>> by default.
>>> I can switch it to use_tz though and set it in firmware_init after 
>>> the
>>> presence of
>>> firmware node is checked.
>> 
>> Making sure the flag is explicitly initialized instead of relying on
>> default initialization is another good reason to have that change
>> IMHO. :)
> 
> Vikash, care to send a new version, or will fix that with follow up 
> patches?

I will provide a new series with the suggested change.

Thanks,
Vikash