[PATCH 4/4] media: venus: add PIL support
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
@@ -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 {
+ compatible = "qcom,venus-pil-no-tz";
+ 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 */
+ ret = device_for_each_child(dev, core, store_firmware_dev);
+
+ ret = venus_boot(core);
if (ret)
goto err_runtime_disable;
@@ -303,14 +327,17 @@ static int venus_probe(struct platform_device *pdev)
if (ret)
goto err_core_deinit;
- ret = of_platform_populate(dev->of_node, NULL, NULL, dev);
+ ret = pm_runtime_put_sync(dev);
if (ret)
goto err_dev_unregister;
- ret = pm_runtime_put_sync(dev);
- if (ret)
+ iommu_domain = iommu_get_domain_for_dev(dev);
+ if (!iommu_domain)
goto err_dev_unregister;
+ iommu_domain->geometry.aperture_start = VENUS_FW_MEM_SIZE;
+ iommu_domain->geometry.aperture_end = VENUS_MAX_MEM_REGION;
+
return 0;
err_dev_unregister:
@@ -318,7 +345,7 @@ static int venus_probe(struct platform_device *pd
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
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 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,
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
