diff mbox

[v3,3/4] venus: firmware: add no TZ boot and shutdown routine

Message ID 1530731212-30474-4-git-send-email-vgarodia@codeaurora.org (mailing list archive)
State Not Applicable, archived
Delegated to: Andy Gross
Headers show

Commit Message

Vikash Garodia July 4, 2018, 7:06 p.m. UTC 
Video hardware is mainly comprised of vcodec subsystem and video
control subsystem. Video control has ARM9 which executes the video
firmware instructions whereas vcodec does the video frame processing.
This change adds support to load the video firmware and bring ARM9
out of reset for platforms which does not have trustzone.

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

Comments

Stanimir Varbanov July 25, 2018, 9:36 a.m. UTC | #1 
Hi Vikash,

On 07/04/2018 10:06 PM, Vikash Garodia wrote:
> Video hardware is mainly comprised of vcodec subsystem and video
> control subsystem. Video control has ARM9 which executes the video
> firmware instructions whereas vcodec does the video frame processing.
> This change adds support to load the video firmware and bring ARM9
> out of reset for platforms which does not have trustzone.
> 
> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
> ---
>  drivers/media/platform/qcom/venus/core.c     |  6 +-
>  drivers/media/platform/qcom/venus/core.h     |  5 ++
>  drivers/media/platform/qcom/venus/firmware.c | 93 ++++++++++++++++++++++++++--
>  drivers/media/platform/qcom/venus/firmware.h |  2 +-
>  4 files changed, 98 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> index 75b9785..393994e 100644
> --- a/drivers/media/platform/qcom/venus/core.c
> +++ b/drivers/media/platform/qcom/venus/core.c
> @@ -76,7 +76,7 @@ static void venus_sys_error_handler(struct work_struct *work)
>  	hfi_core_deinit(core, true);
>  	hfi_destroy(core);
>  	mutex_lock(&core->lock);
> -	venus_shutdown(core->dev);
> +	venus_shutdown(core);
>  
>  	pm_runtime_put_sync(core->dev);
>  
> @@ -323,7 +323,7 @@ static int venus_probe(struct platform_device *pdev)
>  err_core_deinit:
>  	hfi_core_deinit(core, false);
>  err_venus_shutdown:
> -	venus_shutdown(dev);
> +	venus_shutdown(core);
>  err_runtime_disable:
>  	pm_runtime_set_suspended(dev);
>  	pm_runtime_disable(dev);
> @@ -344,7 +344,7 @@ static int venus_remove(struct platform_device *pdev)
>  	WARN_ON(ret);
>  
>  	hfi_destroy(core);
> -	venus_shutdown(dev);
> +	venus_shutdown(core);
>  	of_platform_depopulate(dev);
>  
>  	pm_runtime_put_sync(dev);
> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
> index eb5ee66..abe4ddf 100644
> --- a/drivers/media/platform/qcom/venus/core.h
> +++ b/drivers/media/platform/qcom/venus/core.h
> @@ -81,6 +81,10 @@ struct venus_caps {
>  	bool valid;	/* used only for Venus v1xx */
>  };
>  
> +struct video_firmware {
> +	struct device *dev;
> +	struct iommu_domain *iommu_domain;
> +};

add an empty line here

>  /**
>   * struct venus_core - holds core parameters valid for all instances
>   *
> @@ -129,6 +133,7 @@ struct venus_core {
>  	struct device *dev;
>  	struct device *dev_dec;
>  	struct device *dev_enc;
> +	struct video_firmware fw;
>  	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 6d6cca4..2a98d9e 100644
> --- a/drivers/media/platform/qcom/venus/firmware.c
> +++ b/drivers/media/platform/qcom/venus/firmware.c
> @@ -12,8 +12,10 @@
>   *
>   */
>  
> +#include <linux/platform_device.h>
>  #include <linux/device.h>
>  #include <linux/firmware.h>
> +#include <linux/iommu.h>
>  #include <linux/kernel.h>
>  #include <linux/io.h>
>  #include <linux/of.h>
> @@ -27,7 +29,8 @@
>  #include "hfi_venus_io.h"
>  
>  #define VENUS_PAS_ID			9
> -#define VENUS_FW_MEM_SIZE		(6 * SZ_1M)
> +#define VENUS_FW_MEM_SIZE		(5 * SZ_1M)

This change should be subject to a separate patch.

> +#define VENUS_FW_START_ADDR		0x0
>  
>  static void venus_reset_cpu(struct venus_core *core)
>  {
> @@ -121,6 +124,76 @@ static int venus_load_fw(struct venus_core *core, const char *fwname,
>  	return ret;
>  }
>  
> +int venus_boot_noTZ(struct venus_core *core, phys_addr_t mem_phys,
> +							size_t mem_size)

please use lowercase for function names and correct the size_t mem_size
indentation.

> +{
> +	struct iommu_domain *iommu;

s/iommu/iommu_dom ?

> +	struct device *dev;
> +	int ret;
> +
> +	if (!core->fw.dev)
> +		return -EPROBE_DEFER;
> +
> +	dev = core->fw.dev;

you could make this initialization in declaration and check for !dev
above in the if statement.

> +
> +	iommu = iommu_domain_alloc(&platform_bus_type);
> +	if (!iommu) {
> +		dev_err(dev, "Failed to allocate iommu domain\n");
> +		return -ENOMEM;
> +	}
> +
> +	ret = iommu_attach_device(iommu, dev);
> +	if (ret) {
> +		dev_err(dev, "could not attach device\n");
> +		goto err_attach;
> +	}
> +
> +	ret = iommu_map(iommu, VENUS_FW_START_ADDR, mem_phys, mem_size,
> +			IOMMU_READ|IOMMU_WRITE|IOMMU_PRIV);

spaces between IOMMU_xxx

> +	if (ret) {
> +		dev_err(dev, "could not map video firmware region\n");
> +		goto err_map;
> +	}

add blank line here

> +	core->fw.iommu_domain = iommu;
> +	venus_reset_cpu(core);
> +
> +	return 0;
> +
> +err_map:
> +	iommu_detach_device(iommu, dev);
> +err_attach:
> +	iommu_domain_free(iommu);
> +	return ret;
> +}
> +
> +int venus_shutdown_noTZ(struct venus_core *core)

s/noTZ/notz

> +{
> +	struct iommu_domain *iommu;
> +	size_t unmapped = 0;
> +	u32 reg;
> +	struct device *dev = core->fw.dev;
> +	void __iomem *reg_base = core->base;
> +
> +	/* Assert the reset to ARM9 */
> +	reg = readl_relaxed(reg_base + WRAPPER_A9SS_SW_RESET);
> +	reg |= BIT(4);

can we have a define for bit 4, probably where we defined
WRAPPER_A9SS_SW_RESET register.

> +	writel_relaxed(reg, reg_base + WRAPPER_A9SS_SW_RESET);
> +
> +	/* Make sure reset is asserted before the mapping is removed */
> +	mb();

mb() is used for compiler barrier as far as I know, isn't better to just
read the register and keep the comment?

> +
> +	iommu = core->fw.iommu_domain;
> +
> +	unmapped = iommu_unmap(iommu, VENUS_FW_START_ADDR, VENUS_FW_MEM_SIZE);
> +	if (unmapped != VENUS_FW_MEM_SIZE)
> +		dev_err(dev, "failed to unmap firmware\n");
> +
> +	iommu_detach_device(iommu, dev);
> +	iommu_domain_free(iommu);
> +
> +	return 0;
> +}
> +
>  int venus_boot(struct venus_core *core)
>  {
>  	phys_addr_t mem_phys;
> @@ -139,10 +212,22 @@ int venus_boot(struct venus_core *core)
>  		return -EINVAL;
>  	}
>  
> -	return qcom_scm_pas_auth_and_reset(VENUS_PAS_ID);
> +	if (core->no_tz)
> +		ret = venus_boot_noTZ(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 = 0;

no need to initialize to 0

> +
> +	if (core->no_tz)
> +		ret = venus_shutdown_noTZ(core);
> +	else
> +		ret = qcom_scm_pas_shutdown(VENUS_PAS_ID);
> +
> +	return ret;
>  }
> diff --git a/drivers/media/platform/qcom/venus/firmware.h b/drivers/media/platform/qcom/venus/firmware.h
> index 36a5fc2..92736d6 100644
> --- a/drivers/media/platform/qcom/venus/firmware.h
> +++ b/drivers/media/platform/qcom/venus/firmware.h
> @@ -17,7 +17,7 @@
>  struct device;
>  
>  int venus_boot(struct venus_core *core);
> -int venus_shutdown(struct device *dev);
> +int venus_shutdown(struct venus_core *core);
>  int venus_set_hw_state(struct venus_core *core, bool suspend);
>  
>  #endif
>
Vikash Garodia Aug. 6, 2018, 8:18 a.m. UTC | #2 
Hi Stanimir,

Thanks for your review.

On 2018-07-25 15:06, Stanimir Varbanov wrote:
> Hi Vikash,
> 
> On 07/04/2018 10:06 PM, Vikash Garodia wrote:
> 
<snip>

>> 
>>  #define VENUS_PAS_ID			9
>> -#define VENUS_FW_MEM_SIZE		(6 * SZ_1M)
>> +#define VENUS_FW_MEM_SIZE		(5 * SZ_1M)
> 
> This change should be subject to a separate patch.
Ok.

> 
<snip>

>> +	writel_relaxed(reg, reg_base + WRAPPER_A9SS_SW_RESET);
>> +
>> +	/* Make sure reset is asserted before the mapping is removed */
>> +	mb();
> 
> mb() is used for compiler barrier as far as I know, isn't better to 
> just
> read the register and keep the comment?

mb() can ensure that the instructions are ordered. This is needed to 
ensure
reset before the mapping is removed.

Thanks,
Vikash
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index 75b9785..393994e 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -76,7 +76,7 @@  static void venus_sys_error_handler(struct work_struct *work)
 	hfi_core_deinit(core, true);
 	hfi_destroy(core);
 	mutex_lock(&core->lock);
-	venus_shutdown(core->dev);
+	venus_shutdown(core);
 
 	pm_runtime_put_sync(core->dev);
 
@@ -323,7 +323,7 @@  static int venus_probe(struct platform_device *pdev)
 err_core_deinit:
 	hfi_core_deinit(core, false);
 err_venus_shutdown:
-	venus_shutdown(dev);
+	venus_shutdown(core);
 err_runtime_disable:
 	pm_runtime_set_suspended(dev);
 	pm_runtime_disable(dev);
@@ -344,7 +344,7 @@  static int venus_remove(struct platform_device *pdev)
 	WARN_ON(ret);
 
 	hfi_destroy(core);
-	venus_shutdown(dev);
+	venus_shutdown(core);
 	of_platform_depopulate(dev);
 
 	pm_runtime_put_sync(dev);
diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
index eb5ee66..abe4ddf 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -81,6 +81,10 @@  struct venus_caps {
 	bool valid;	/* used only for Venus v1xx */
 };
 
+struct video_firmware {
+	struct device *dev;
+	struct iommu_domain *iommu_domain;
+};
 /**
  * struct venus_core - holds core parameters valid for all instances
  *
@@ -129,6 +133,7 @@  struct venus_core {
 	struct device *dev;
 	struct device *dev_dec;
 	struct device *dev_enc;
+	struct video_firmware fw;
 	bool no_tz;
 	struct mutex lock;
 	struct list_head instances;
diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c
index 6d6cca4..2a98d9e 100644
--- a/drivers/media/platform/qcom/venus/firmware.c
+++ b/drivers/media/platform/qcom/venus/firmware.c
@@ -12,8 +12,10 @@ 
  *
  */
 
+#include <linux/platform_device.h>
 #include <linux/device.h>
 #include <linux/firmware.h>
+#include <linux/iommu.h>
 #include <linux/kernel.h>
 #include <linux/io.h>
 #include <linux/of.h>
@@ -27,7 +29,8 @@ 
 #include "hfi_venus_io.h"
 
 #define VENUS_PAS_ID			9
-#define VENUS_FW_MEM_SIZE		(6 * SZ_1M)
+#define VENUS_FW_MEM_SIZE		(5 * SZ_1M)
+#define VENUS_FW_START_ADDR		0x0
 
 static void venus_reset_cpu(struct venus_core *core)
 {
@@ -121,6 +124,76 @@  static int venus_load_fw(struct venus_core *core, const char *fwname,
 	return ret;
 }
 
+int venus_boot_noTZ(struct venus_core *core, phys_addr_t mem_phys,
+							size_t mem_size)
+{
+	struct iommu_domain *iommu;
+	struct device *dev;
+	int ret;
+
+	if (!core->fw.dev)
+		return -EPROBE_DEFER;
+
+	dev = core->fw.dev;
+
+	iommu = iommu_domain_alloc(&platform_bus_type);
+	if (!iommu) {
+		dev_err(dev, "Failed to allocate iommu domain\n");
+		return -ENOMEM;
+	}
+
+	ret = iommu_attach_device(iommu, dev);
+	if (ret) {
+		dev_err(dev, "could not attach device\n");
+		goto err_attach;
+	}
+
+	ret = iommu_map(iommu, VENUS_FW_START_ADDR, mem_phys, mem_size,
+			IOMMU_READ|IOMMU_WRITE|IOMMU_PRIV);
+	if (ret) {
+		dev_err(dev, "could not map video firmware region\n");
+		goto err_map;
+	}
+	core->fw.iommu_domain = iommu;
+	venus_reset_cpu(core);
+
+	return 0;
+
+err_map:
+	iommu_detach_device(iommu, dev);
+err_attach:
+	iommu_domain_free(iommu);
+	return ret;
+}
+
+int venus_shutdown_noTZ(struct venus_core *core)
+{
+	struct iommu_domain *iommu;
+	size_t unmapped = 0;
+	u32 reg;
+	struct device *dev = core->fw.dev;
+	void __iomem *reg_base = core->base;
+
+	/* Assert the reset to ARM9 */
+	reg = readl_relaxed(reg_base + WRAPPER_A9SS_SW_RESET);
+	reg |= BIT(4);
+	writel_relaxed(reg, reg_base + WRAPPER_A9SS_SW_RESET);
+
+	/* Make sure reset is asserted before the mapping is removed */
+	mb();
+
+	iommu = core->fw.iommu_domain;
+
+	unmapped = iommu_unmap(iommu, VENUS_FW_START_ADDR, VENUS_FW_MEM_SIZE);
+	if (unmapped != VENUS_FW_MEM_SIZE)
+		dev_err(dev, "failed to unmap firmware\n");
+
+	iommu_detach_device(iommu, dev);
+	iommu_domain_free(iommu);
+
+	return 0;
+}
+
 int venus_boot(struct venus_core *core)
 {
 	phys_addr_t mem_phys;
@@ -139,10 +212,22 @@  int venus_boot(struct venus_core *core)
 		return -EINVAL;
 	}
 
-	return qcom_scm_pas_auth_and_reset(VENUS_PAS_ID);
+	if (core->no_tz)
+		ret = venus_boot_noTZ(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 = 0;
+
+	if (core->no_tz)
+		ret = venus_shutdown_noTZ(core);
+	else
+		ret = qcom_scm_pas_shutdown(VENUS_PAS_ID);
+
+	return ret;
 }
diff --git a/drivers/media/platform/qcom/venus/firmware.h b/drivers/media/platform/qcom/venus/firmware.h
index 36a5fc2..92736d6 100644
--- a/drivers/media/platform/qcom/venus/firmware.h
+++ b/drivers/media/platform/qcom/venus/firmware.h
@@ -17,7 +17,7 @@ 
 struct device;
 
 int venus_boot(struct venus_core *core);
-int venus_shutdown(struct device *dev);
+int venus_shutdown(struct venus_core *core);
 int venus_set_hw_state(struct venus_core *core, bool suspend);
 
 #endif