[v6,4/4] venus: firmware: register separate platform_device for firmware loader
Commit Message
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
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.
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
>
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(-)
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
@@ -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>;
+ };
};
@@ -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);
@@ -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));
+}
@@ -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);