Re: [PATCH 4/4] media: venus: add PIL support
Hi, On 05/22/2018 04:02 PM, Stanimir Varbanov wrote: > Hi Vikash, > > On 05/17/2018 02:32 PM, Vikash Garodia wrote: >> This adds support to load the video firmware >> and bring ARM9 out of reset. This is useful >> for platforms which does not have trustzone >> to reset the ARM9. >> >> Signed-off-by: Vikash Garodia >> --- >> .../devicetree/bindings/media/qcom,venus.txt | 8 +- >> drivers/media/platform/qcom/venus/core.c | 67 +++-- >> drivers/media/platform/qcom/venus/core.h | 6 + >> drivers/media/platform/qcom/venus/firmware.c | 163 >> + >> drivers/media/platform/qcom/venus/firmware.h | 10 +- >> 5 files changed, 217 insertions(+), 37 deletions(-) >> >> >> -int venus_shutdown(struct device *dev) >> +int venus_boot_noTZ(struct venus_core *core, phys_addr_t mem_phys, >> +size_t mem_size) >> { >> -return qcom_scm_pas_shutdown(VENUS_PAS_ID); >> +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; >> +} >> + >> +iommu->geometry.aperture_start = 0x0; >> +iommu->geometry.aperture_end = VENUS_FW_MEM_SIZE; > > The same comment for geometry params as for venus_probe is valid here. Infact aperture_end will be overwritten by arm-smmu driver in the next call to iommu_attach_device(), and by chance geometry.force_aperture will become true. I wonder is that geometry params are supposed to be used by drivers or by iommu drivers? > >> + >> +ret = iommu_attach_device(iommu, dev); >> +if (ret) { >> +dev_err(dev, "could not attach device\n"); >> +goto err_attach; >> +} >> + >> +ret = iommu_map(iommu, core->fw.iova, mem_phys, mem_size, >> +IOMMU_READ|IOMMU_WRITE|IOMMU_PRIV); > > iova is not initialized and is zero, maybe we don't need that variable > in the venus_firmware structure? > >> +if (ret) { >> +dev_err(dev, "could not map video firmware region\n"); >> +goto err_map; >> +} >> +core->fw.iommu_domain = iommu; >> +venus_reset_hw(core); >> + >> +return 0; >> + >> +err_map: >> +iommu_detach_device(iommu, dev); >> +err_attach: >> +iommu_domain_free(iommu); >> +return ret; >> } >> + -- regards, Stan
Re: [PATCH 4/4] media: venus: add PIL support
Hi Vikash, On 05/17/2018 02:32 PM, Vikash Garodia wrote: > This adds support to load the video firmware > and bring ARM9 out of reset. This is useful > for platforms which does not have trustzone > to reset the ARM9. > > Signed-off-by: Vikash Garodia > --- > .../devicetree/bindings/media/qcom,venus.txt | 8 +- > drivers/media/platform/qcom/venus/core.c | 67 +++-- > drivers/media/platform/qcom/venus/core.h | 6 + > drivers/media/platform/qcom/venus/firmware.c | 163 > + > drivers/media/platform/qcom/venus/firmware.h | 10 +- > 5 files changed, 217 insertions(+), 37 deletions(-) > > diff --git a/Documentation/devicetree/bindings/media/qcom,venus.txt > b/Documentation/devicetree/bindings/media/qcom,venus.txt > index 00d0d1b..0ff0f2d 100644 > --- a/Documentation/devicetree/bindings/media/qcom,venus.txt > +++ b/Documentation/devicetree/bindings/media/qcom,venus.txt for this change in DT binding you have to cc devicetree ML. And probably it could be separate patch. > @@ -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, one optional firmware subnode. > > Every of video-encoder or video-decoder subnode should have: > > @@ -79,6 +79,8 @@ 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 sub node must contain the iommus specifiers for ARM9. > + > * An Example > video-codec@1d0 { > compatible = "qcom,msm8916-venus"; > @@ -105,4 +107,8 @@ Every of video-encoder or video-decoder subnode should > have: > clock-names = "core"; > power-domains = <&mmcc VENUS_CORE1_GDSC>; > }; > + firmware { venus-firmware > + compatible = "qcom,venus-pil-no-tz"; this should be following the other subnodes compatible names: compatible = "venus-firmware"; > + iommus = <&apps_smmu 0x10b2 0x0>; > + } > }; > diff --git a/drivers/media/platform/qcom/venus/core.c > b/drivers/media/platform/qcom/venus/core.c > index 1308488..16910558 100644 > --- a/drivers/media/platform/qcom/venus/core.c > +++ b/drivers/media/platform/qcom/venus/core.c > @@ -22,6 +22,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -30,6 +31,7 @@ > #include "vdec.h" > #include "venc.h" > #include "firmware.h" > +#include "hfi_venus.h" > > static void venus_event_notify(struct venus_core *core, u32 event) > { > @@ -76,7 +78,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); > > @@ -84,7 +86,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); > > @@ -179,6 +181,20 @@ static u32 to_v4l2_codec_type(u32 codec) > } > } > > +static int store_firmware_dev(struct device *dev, void *data) > +{ > + struct venus_core *core; > + > + core = (struct venus_core *)data; > + if (!core) > + return -EINVAL; > + > + if (of_device_is_compatible(dev->of_node, "qcom,venus-pil-no-tz")) > + core->fw.dev = dev; > + > + return 0; > +} > + > static int venus_enumerate_codecs(struct venus_core *core, u32 type) > { > const struct hfi_inst_ops dummy_ops = {}; > @@ -229,6 +245,7 @@ static int venus_probe(struct platform_device *pdev) > struct device *dev = &pdev->dev; > struct venus_core *core; > struct resource *r; > + struct iommu_domain *iommu_domain; > int ret; > > core = devm_kzalloc(dev, sizeof(*core), GFP_KERNEL); > @@ -279,7 +296,14 @@ static int venus_probe(struct platform_device *pdev) > if (ret < 0) > goto err_runtime_disable; > > - ret = venus_boot(dev, core->res->fwname); > + ret = of_platform_populate(dev->of_node, NULL, NULL, dev); > + if (ret) > + goto err_runtime_disable; > + > + /* Attempt to register child devices */ This comment is wrong, the child devices are created by of_platform_populate above. > + ret = device_for_each_child(dev, core, store_firmware_dev); Why we need these complex gymnastics to get struct device pointer when that could be done in venus_firmware .probe method? I think the answer is because you want to avoid having venus-firmware.ko (because you have to have separate struct device for iommu SID). In that case it would be be
Re: [PATCH 4/4] media: venus: add PIL support
Hi Trilok, On 2018-05-18 06:10, Trilok Soni wrote: Hi Vikash, On 5/17/2018 4:32 AM, Vikash Garodia wrote: This adds support to load the video firmware and bring ARM9 out of reset. This is useful for platforms which does not have trustzone to reset the ARM9. ARM9 = video core here? May be commit text needs little bit more detail. Yes, ARM9 here refers to the CPU running the firmware inside video core. I can add some more detail on the same. +static int store_firmware_dev(struct device *dev, void *data) +{ + struct venus_core *core; + + core = (struct venus_core *)data; No need of casting. Ok. Will remove the casting. + if (!core) + return -EINVAL; + + if (of_device_is_compatible(dev->of_node, "qcom,venus-pil-no-tz")) + core->fw.dev = dev; + + return 0; +} + - ret = venus_boot(dev, core->res->fwname); + ret = of_platform_populate(dev->of_node, NULL, NULL, dev); + if (ret) + goto err_runtime_disable; + + /* Attempt to register child devices */ + ret = device_for_each_child(dev, core, store_firmware_dev); + and not ret check needed? Not needed. The fn (store_firmware_dev) just stores the child device pointer. Later in the driver, if the child device pointer is not populated, probe is deferred. Again, child device for which this populate is added, is an optional child node. + ret = venus_boot(core); if (ret) goto err_runtime_disable;
Re: [PATCH 4/4] media: venus: add PIL support
Hi Vikash, On 5/17/2018 4:32 AM, Vikash Garodia wrote: This adds support to load the video firmware and bring ARM9 out of reset. This is useful for platforms which does not have trustzone to reset the ARM9. ARM9 = video core here? May be commit text needs little bit more detail. +static int store_firmware_dev(struct device *dev, void *data) +{ + struct venus_core *core; + + core = (struct venus_core *)data; No need of casting. + if (!core) + return -EINVAL; + + if (of_device_is_compatible(dev->of_node, "qcom,venus-pil-no-tz")) + core->fw.dev = dev; + + return 0; +} + - ret = venus_boot(dev, core->res->fwname); + ret = of_platform_populate(dev->of_node, NULL, NULL, dev); + if (ret) + goto err_runtime_disable; + + /* Attempt to register child devices */ + ret = device_for_each_child(dev, core, store_firmware_dev); + and not ret check needed? + ret = venus_boot(core); if (ret) goto err_runtime_disable; -- ---Trilok Soni Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project