[v6,4/4] venus: firmware: register separate platform_device for firmware loader
diff mbox series

Message ID 1535034528-11590-5-git-send-email-vgarodia@codeaurora.org
State Not Applicable
Headers show
Series
  • Venus updates - PIL
Related show

Commit Message

Vikash Garodia Aug. 23, 2018, 2:28 p.m. UTC
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>
---
 .../devicetree/bindings/media/qcom,venus.txt       | 13 +++++-
 drivers/media/platform/qcom/venus/core.c           | 14 +++++--
 drivers/media/platform/qcom/venus/firmware.c       | 49 ++++++++++++++++++++++
 drivers/media/platform/qcom/venus/firmware.h       |  2 +
 4 files changed, 73 insertions(+), 5 deletions(-)

Comments

Stephen Boyd Aug. 24, 2018, 6:45 a.m. UTC | #1
Quoting Vikash Garodia (2018-08-23 07:28:48)
> 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.

Yes, but why? Commit text isn't supposed to say what is obvious from the
code.
Alexandre Courbot Aug. 24, 2018, 7:39 a.m. UTC | #2
On Thu, Aug 23, 2018 at 11:29 PM Vikash Garodia <vgarodia@codeaurora.org> 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.
>
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> ---
>  .../devicetree/bindings/media/qcom,venus.txt       | 13 +++++-
>  drivers/media/platform/qcom/venus/core.c           | 14 +++++--
>  drivers/media/platform/qcom/venus/firmware.c       | 49 ++++++++++++++++++++++
>  drivers/media/platform/qcom/venus/firmware.h       |  2 +
>  4 files changed, 73 insertions(+), 5 deletions(-)
>
> 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.

Just noticed that the document does not explain in which case the
firmware subnode must be used. Maybe we should have a sentence
explaining that without it we will be using TrustZone to load the
firmware?

>
>  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>;
> +               };
>         };
> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> index 393994e..3bd3b8a 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(core);
>         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/firmware.c b/drivers/media/platform/qcom/venus/firmware.c
> index 79b3858..86a26fb 100644
> --- a/drivers/media/platform/qcom/venus/firmware.c
> +++ b/drivers/media/platform/qcom/venus/firmware.c
> @@ -20,6 +20,7 @@
>  #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>
> @@ -228,3 +229,51 @@ int venus_shutdown(struct venus_core *core)
>
>         return ret;
>  }
> +
> +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);
> +       if (ret)
> +               dev_err(core->dev, "dma configure fail\n");
> +
> +       of_node_put(np);
> +
> +       if (ret)
> +               return ret;
> +
> +       core->no_tz = true;
> +       core->fw.dev = &pdev->dev;
> +
> +       return 0;
> +}
> +
> +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 f41b615..119a9a4 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 venus_core *core);
>  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
>
Rob Herring Aug. 28, 2018, 10:15 p.m. UTC | #3
On Thu, Aug 23, 2018 at 07:58:48PM +0530, 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.
> 
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> ---
>  .../devicetree/bindings/media/qcom,venus.txt       | 13 +++++-

In the future, please split binding patches.

Reviewed-by: Rob Herring <robh@kernel.org>

>  drivers/media/platform/qcom/venus/core.c           | 14 +++++--
>  drivers/media/platform/qcom/venus/firmware.c       | 49 ++++++++++++++++++++++
>  drivers/media/platform/qcom/venus/firmware.h       |  2 +
>  4 files changed, 73 insertions(+), 5 deletions(-)
kbuild test robot Aug. 30, 2018, 3:53 a.m. UTC | #4
Hi Stanimir,

I love your patch! Yet something to improve:

[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on v4.19-rc1 next-20180829]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Vikash-Garodia/Venus-updates-PIL/20180824-023823
base:   git://linuxtv.org/media_tree.git master
config: arm-allmodconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.2.0 make.cross ARCH=arm 

All errors (new ones prefixed by >>):

   drivers/media/platform/qcom/venus/firmware.c: In function 'venus_load_fw':
   drivers/media/platform/qcom/venus/firmware.c:113:9: error: implicit declaration of function 'qcom_mdt_load_no_init'; did you mean 'qcom_mdt_load'? [-Werror=implicit-function-declaration]
      ret = qcom_mdt_load_no_init(dev, mdt, fwname, VENUS_PAS_ID,
            ^~~~~~~~~~~~~~~~~~~~~
            qcom_mdt_load
   drivers/media/platform/qcom/venus/firmware.c: In function 'venus_firmware_init':
>> drivers/media/platform/qcom/venus/firmware.c:258:8: error: too few arguments to function 'of_dma_configure'
     ret = of_dma_configure(&pdev->dev, np);
           ^~~~~~~~~~~~~~~~
   In file included from drivers/media/platform/qcom/venus/firmware.c:23:0:
   include/linux/of_device.h:58:5: note: declared here
    int of_dma_configure(struct device *dev,
        ^~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/of_dma_configure +258 drivers/media/platform/qcom/venus/firmware.c

    65	
    66	static int venus_load_fw(struct venus_core *core, const char *fwname,
    67				 phys_addr_t *mem_phys, size_t *mem_size)
    68	{
    69		const struct firmware *mdt;
    70		struct device_node *node;
    71		struct device *dev;
    72		struct resource r;
    73		ssize_t fw_size;
    74		void *mem_va;
    75		int ret;
    76	
    77		dev = core->dev;
    78		node = of_parse_phandle(dev->of_node, "memory-region", 0);
    79		if (!node) {
    80			dev_err(dev, "no memory-region specified\n");
    81			return -EINVAL;
    82		}
    83	
    84		ret = of_address_to_resource(node, 0, &r);
    85		if (ret)
    86			return ret;
    87	
    88		*mem_phys = r.start;
    89		*mem_size = resource_size(&r);
    90	
    91		if (*mem_size < VENUS_FW_MEM_SIZE)
    92			return -EINVAL;
    93	
    94		mem_va = memremap(r.start, *mem_size, MEMREMAP_WC);
    95		if (!mem_va) {
    96			dev_err(dev, "unable to map memory region: %pa+%zx\n",
    97				&r.start, *mem_size);
    98			return -ENOMEM;
    99		}
   100	
   101		ret = request_firmware(&mdt, fwname, dev);
   102		if (ret < 0)
   103			goto err_unmap;
   104	
   105		fw_size = qcom_mdt_get_size(mdt);
   106		if (fw_size < 0) {
   107			ret = fw_size;
   108			release_firmware(mdt);
   109			goto err_unmap;
   110		}
   111	
   112		if (core->no_tz)
 > 113			ret = qcom_mdt_load_no_init(dev, mdt, fwname, VENUS_PAS_ID,
   114						    mem_va, *mem_phys, *mem_size, NULL);
   115		else
   116			ret = qcom_mdt_load(dev, mdt, fwname, VENUS_PAS_ID,
   117					    mem_va, *mem_phys, *mem_size, NULL);
   118	
   119		release_firmware(mdt);
   120	
   121	err_unmap:
   122		memunmap(mem_va);
   123		return ret;
   124	}
   125	
   126	static int venus_boot_no_tz(struct venus_core *core, phys_addr_t mem_phys,
   127				    size_t mem_size)
   128	{
   129		struct iommu_domain *iommu_dom;
   130		struct device *dev;
   131		int ret;
   132	
   133		dev = core->fw.dev;
   134		if (!dev)
   135			return -EPROBE_DEFER;
   136	
   137		iommu_dom = iommu_domain_alloc(&platform_bus_type);
   138		if (!iommu_dom) {
   139			dev_err(dev, "Failed to allocate iommu domain\n");
   140			return -ENOMEM;
   141		}
   142	
   143		ret = iommu_attach_device(iommu_dom, dev);
   144		if (ret) {
   145			dev_err(dev, "could not attach device\n");
   146			goto err_attach;
   147		}
   148	
   149		ret = iommu_map(iommu_dom, VENUS_FW_START_ADDR, mem_phys, mem_size,
   150				IOMMU_READ | IOMMU_WRITE | IOMMU_PRIV);
   151		if (ret) {
   152			dev_err(dev, "could not map video firmware region\n");
   153			goto err_map;
   154		}
   155	
   156		core->fw.iommu_domain = iommu_dom;
   157		venus_reset_cpu(core);
   158	
   159		return 0;
   160	
   161	err_map:
   162		iommu_detach_device(iommu_dom, dev);
   163	err_attach:
   164		iommu_domain_free(iommu_dom);
   165		return ret;
   166	}
   167	
   168	static int venus_shutdown_no_tz(struct venus_core *core)
   169	{
   170		struct iommu_domain *iommu;
   171		size_t unmapped;
   172		u32 reg;
   173		struct device *dev = core->fw.dev;
   174		void __iomem *base = core->base;
   175	
   176		/* Assert the reset to ARM9 */
   177		reg = readl_relaxed(base + WRAPPER_A9SS_SW_RESET);
   178		reg |= WRAPPER_A9SS_SW_RESET_BIT;
   179		writel_relaxed(reg, base + WRAPPER_A9SS_SW_RESET);
   180	
   181		/* Make sure reset is asserted before the mapping is removed */
   182		mb();
   183	
   184		iommu = core->fw.iommu_domain;
   185	
   186		unmapped = iommu_unmap(iommu, VENUS_FW_START_ADDR, VENUS_FW_MEM_SIZE);
   187		if (unmapped != VENUS_FW_MEM_SIZE)
   188			dev_err(dev, "failed to unmap firmware\n");
   189	
   190		iommu_detach_device(iommu, dev);
   191		iommu_domain_free(iommu);
   192	
   193		return 0;
   194	}
   195	
   196	int venus_boot(struct venus_core *core)
   197	{
   198		struct device *dev = core->dev;
   199		phys_addr_t mem_phys;
   200		size_t mem_size;
   201		int ret;
   202	
   203		if (!IS_ENABLED(CONFIG_QCOM_MDT_LOADER) ||
   204		    (!core->no_tz && !qcom_scm_is_available()))
   205			return -EPROBE_DEFER;
   206	
   207		ret = venus_load_fw(core, core->res->fwname, &mem_phys, &mem_size);
   208		if (ret) {
   209			dev_err(dev, "fail to load video firmware\n");
   210			return -EINVAL;
   211		}
   212	
   213		if (core->no_tz)
   214			ret = venus_boot_no_tz(core, mem_phys, mem_size);
   215		else
   216			ret = qcom_scm_pas_auth_and_reset(VENUS_PAS_ID);
   217	
   218		return ret;
   219	}
   220	
   221	int venus_shutdown(struct venus_core *core)
   222	{
   223		int ret;
   224	
   225		if (core->no_tz)
   226			ret = venus_shutdown_no_tz(core);
   227		else
   228			ret = qcom_scm_pas_shutdown(VENUS_PAS_ID);
   229	
   230		return ret;
   231	}
   232	
   233	int venus_firmware_init(struct venus_core *core)
   234	{
   235		struct platform_device_info info;
   236		struct platform_device *pdev;
   237		struct device_node *np;
   238		int ret;
   239	
   240		np = of_get_child_by_name(core->dev->of_node, "video-firmware");
   241		if (!np)
   242			return 0;
   243	
   244		memset(&info, 0, sizeof(info));
   245		info.fwnode = &np->fwnode;
   246		info.parent = core->dev;
   247		info.name = np->name;
   248		info.dma_mask = DMA_BIT_MASK(32);
   249	
   250		pdev = platform_device_register_full(&info);
   251		if (IS_ERR(pdev)) {
   252			of_node_put(np);
   253			return PTR_ERR(pdev);
   254		}
   255	
   256		pdev->dev.of_node = np;
   257	
 > 258		ret = of_dma_configure(&pdev->dev, np);
   259		if (ret)
   260			dev_err(core->dev, "dma configure fail\n");
   261	
   262		of_node_put(np);
   263	
   264		if (ret)
   265			return ret;
   266	
   267		core->no_tz = true;
   268		core->fw.dev = &pdev->dev;
   269	
   270		return 0;
   271	}
   272	

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

Patch
diff mbox series

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>;
+		};
 	};
diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index 393994e..3bd3b8a 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(core);
 	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/firmware.c b/drivers/media/platform/qcom/venus/firmware.c
index 79b3858..86a26fb 100644
--- a/drivers/media/platform/qcom/venus/firmware.c
+++ b/drivers/media/platform/qcom/venus/firmware.c
@@ -20,6 +20,7 @@ 
 #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>
@@ -228,3 +229,51 @@  int venus_shutdown(struct venus_core *core)
 
 	return ret;
 }
+
+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);
+	if (ret)
+		dev_err(core->dev, "dma configure fail\n");
+
+	of_node_put(np);
+
+	if (ret)
+		return ret;
+
+	core->no_tz = true;
+	core->fw.dev = &pdev->dev;
+
+	return 0;
+}
+
+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 f41b615..119a9a4 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 venus_core *core);
 int venus_set_hw_state(struct venus_core *core, bool suspend);