Message ID | 1535034528-11590-5-git-send-email-vgarodia@codeaurora.org |
---|---|
State | Not Applicable, archived |
Headers | show |
Series | Venus updates - PIL | expand |
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
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);