[v2,5/5] venus: register separate driver for firmware device

  A separate child device is added for video firmware.
This is needed to
[1] configure the firmware context bank with the desired SID.
[2] ensure that the iova for firmware region is from 0x0.

Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
---
 .../devicetree/bindings/media/qcom,venus.txt       |  8 +++-
 drivers/media/platform/qcom/venus/core.c           | 48 +++++++++++++++++++---
 drivers/media/platform/qcom/venus/firmware.c       | 20 ++++++++-
 drivers/media/platform/qcom/venus/firmware.h       |  2 +
 4 files changed, 71 insertions(+), 7 deletions(-)
  

On Sat, Jun 02, 2018 at 01:56:08AM +0530, Vikash Garodia wrote:
> A separate child device is added for video firmware.
> This is needed to
> [1] configure the firmware context bank with the desired SID.
> [2] ensure that the iova for firmware region is from 0x0.
> 
> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
> ---
>  .../devicetree/bindings/media/qcom,venus.txt       |  8 +++-
>  drivers/media/platform/qcom/venus/core.c           | 48 +++++++++++++++++++---
>  drivers/media/platform/qcom/venus/firmware.c       | 20 ++++++++-
>  drivers/media/platform/qcom/venus/firmware.h       |  2 +
>  4 files changed, 71 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/qcom,venus.txt b/Documentation/devicetree/bindings/media/qcom,venus.txt
> index 00d0d1b..701cbe8 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@1d00000 {
>  		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>;
>  		};
> +		venus-firmware {
> +			compatible = "qcom,venus-firmware-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 101612b..5cfb3c2 100644
> --- a/drivers/media/platform/qcom/venus/core.c
> +++ b/drivers/media/platform/qcom/venus/core.c
> @@ -179,6 +179,19 @@ static u32 to_v4l2_codec_type(u32 codec)
>  	}
>  }
>  
> +static int store_firmware_dev(struct device *dev, void *data)
> +{
> +	struct venus_core *core = data;
> +
> +	if (!core)
> +		return -EINVAL;

Core is not going to be null here - you don't need to check it.

<snip>
  

Hi Vikash,

On Sat, Jun 2, 2018 at 5:27 AM Vikash Garodia <vgarodia@codeaurora.org> wrote:
>
> A separate child device is added for video firmware.
> This is needed to
> [1] configure the firmware context bank with the desired SID.
> [2] ensure that the iova for firmware region is from 0x0.
>
> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
> ---
>  .../devicetree/bindings/media/qcom,venus.txt       |  8 +++-
>  drivers/media/platform/qcom/venus/core.c           | 48 +++++++++++++++++++---
>  drivers/media/platform/qcom/venus/firmware.c       | 20 ++++++++-
>  drivers/media/platform/qcom/venus/firmware.h       |  2 +
>  4 files changed, 71 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/media/qcom,venus.txt b/Documentation/devicetree/bindings/media/qcom,venus.txt
> index 00d0d1b..701cbe8 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.

Please document the compatible string here as well.

> +
>  * An Example
>         video-codec@1d00000 {
>                 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>;
>                 };
> +               venus-firmware {
> +                       compatible = "qcom,venus-firmware-no-tz";

I don't think "-no-tz" should be mentioned here in DT, since it's a
firmware/software detail.

> +                       iommus = <&apps_smmu 0x10b2 0x0>;
> +               }
>         };
> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> index 101612b..5cfb3c2 100644
> --- a/drivers/media/platform/qcom/venus/core.c
> +++ b/drivers/media/platform/qcom/venus/core.c
> @@ -179,6 +179,19 @@ static u32 to_v4l2_codec_type(u32 codec)
>         }
>  }
>
> +static int store_firmware_dev(struct device *dev, void *data)
> +{
> +       struct venus_core *core = data;
> +
> +       if (!core)
> +               return -EINVAL;
> +

No need to check this AFAICT.

> +       if (of_device_is_compatible(dev->of_node, "qcom,venus-firmware-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 = {};
> @@ -279,6 +292,13 @@ 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;
> +
> +       /* Attempt to store firmware device */
> +       device_for_each_child(dev, core, store_firmware_dev);
> +
>         ret = venus_boot(core);
>         if (ret)
>                 goto err_runtime_disable;
> @@ -303,10 +323,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;
> @@ -483,7 +499,29 @@ static __maybe_unused int venus_runtime_resume(struct device *dev)
>                 .pm = &venus_pm_ops,
>         },
>  };
> -module_platform_driver(qcom_venus_driver);
> +
> +static int __init venus_init(void)
> +{
> +       int ret;
> +
> +       ret = platform_driver_register(&qcom_video_firmware_driver);
> +       if (ret)
> +               return ret;

Do we really need this firmware driver? As far as I can see, the
approach used here should work even without any driver bound to the
firmware device.

Best regards,
Tomasz
  

Hi Tomasz,

On 06/04/2018 04:18 PM, Tomasz Figa wrote:
> Hi Vikash,
> 
> On Sat, Jun 2, 2018 at 5:27 AM Vikash Garodia <vgarodia@codeaurora.org> wrote:
>>
>> A separate child device is added for video firmware.
>> This is needed to
>> [1] configure the firmware context bank with the desired SID.
>> [2] ensure that the iova for firmware region is from 0x0.
>>
>> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
>> ---
>>  .../devicetree/bindings/media/qcom,venus.txt       |  8 +++-
>>  drivers/media/platform/qcom/venus/core.c           | 48 +++++++++++++++++++---
>>  drivers/media/platform/qcom/venus/firmware.c       | 20 ++++++++-
>>  drivers/media/platform/qcom/venus/firmware.h       |  2 +
>>  4 files changed, 71 insertions(+), 7 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/media/qcom,venus.txt b/Documentation/devicetree/bindings/media/qcom,venus.txt
>> index 00d0d1b..701cbe8 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.
> 
> Please document the compatible string here as well.
> 
>> +
>>  * An Example
>>         video-codec@1d00000 {
>>                 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>;
>>                 };
>> +               venus-firmware {
>> +                       compatible = "qcom,venus-firmware-no-tz";
> 
> I don't think "-no-tz" should be mentioned here in DT, since it's a
> firmware/software detail.

I have to agree with Tomasz, non-tz or tz is a software detail and it
shouldn't be reflected in compatible string.

Also I'm not sure but what will happen if this video-firmware subnode is
not added, do you expect that backward compatibility is satisfied for
older venus versions?

> 
>> +                       iommus = <&apps_smmu 0x10b2 0x0>;
>> +               }
>>         };
>> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
>> index 101612b..5cfb3c2 100644
>> --- a/drivers/media/platform/qcom/venus/core.c
>> +++ b/drivers/media/platform/qcom/venus/core.c
>> @@ -179,6 +179,19 @@ static u32 to_v4l2_codec_type(u32 codec)
>>         }
>>  }
>>
>> +static int store_firmware_dev(struct device *dev, void *data)
>> +{
>> +       struct venus_core *core = data;
>> +
>> +       if (!core)
>> +               return -EINVAL;
>> +
> 
> No need to check this AFAICT.>
>> +       if (of_device_is_compatible(dev->of_node, "qcom,venus-firmware-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 = {};
>> @@ -279,6 +292,13 @@ 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;
>> +
>> +       /* Attempt to store firmware device */
>> +       device_for_each_child(dev, core, store_firmware_dev);
>> +
>>         ret = venus_boot(core);
>>         if (ret)
>>                 goto err_runtime_disable;
>> @@ -303,10 +323,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;
>> @@ -483,7 +499,29 @@ static __maybe_unused int venus_runtime_resume(struct device *dev)
>>                 .pm = &venus_pm_ops,
>>         },
>>  };
>> -module_platform_driver(qcom_venus_driver);
>> +
>> +static int __init venus_init(void)
>> +{
>> +       int ret;
>> +
>> +       ret = platform_driver_register(&qcom_video_firmware_driver);
>> +       if (ret)
>> +               return ret;
> 
> Do we really need this firmware driver? As far as I can see, the
> approach used here should work even without any driver bound to the
> firmware device.

We need device/driver bind because we need to call dma_configure() which
internally doing iommus sID parsing.
  

On Mon, Jun 4, 2018 at 10:56 PM Stanimir Varbanov
<stanimir.varbanov@linaro.org> wrote:
>
> Hi Tomasz,
>
> On 06/04/2018 04:18 PM, Tomasz Figa wrote:
> > Hi Vikash,
> >
> > On Sat, Jun 2, 2018 at 5:27 AM Vikash Garodia <vgarodia@codeaurora.org> wrote:
> >> +static int __init venus_init(void)
> >> +{
> >> +       int ret;
> >> +
> >> +       ret = platform_driver_register(&qcom_video_firmware_driver);
> >> +       if (ret)
> >> +               return ret;
> >
> > Do we really need this firmware driver? As far as I can see, the
> > approach used here should work even without any driver bound to the
> > firmware device.
>
> We need device/driver bind because we need to call dma_configure() which
> internally doing iommus sID parsing.

I can see some drivers calling of_dma_configure() directly:
https://elixir.bootlin.com/linux/latest/ident/of_dma_configure

I'm not sure if it's more elegant, but should at least require less code.

By the way, can we really assume that probe of firmware platform
device really completes before we call venus_boot()?

Best regards,
Tomasz
  

Cc: Arnd

On 06/05/2018 07:08 AM, Tomasz Figa wrote:
> On Mon, Jun 4, 2018 at 10:56 PM Stanimir Varbanov
> <stanimir.varbanov@linaro.org> wrote:
>>
>> Hi Tomasz,
>>
>> On 06/04/2018 04:18 PM, Tomasz Figa wrote:
>>> Hi Vikash,
>>>
>>> On Sat, Jun 2, 2018 at 5:27 AM Vikash Garodia <vgarodia@codeaurora.org> wrote:
>>>> +static int __init venus_init(void)
>>>> +{
>>>> +       int ret;
>>>> +
>>>> +       ret = platform_driver_register(&qcom_video_firmware_driver);
>>>> +       if (ret)
>>>> +               return ret;
>>>
>>> Do we really need this firmware driver? As far as I can see, the
>>> approach used here should work even without any driver bound to the
>>> firmware device.
>>
>> We need device/driver bind because we need to call dma_configure() which
>> internally doing iommus sID parsing.
> 
> I can see some drivers calling of_dma_configure() directly:
> https://elixir.bootlin.com/linux/latest/ident/of_dma_configure
> 
> I'm not sure if it's more elegant, but should at least require less code.

I think that in this case of non-TZ where we do iommu mapping by hand we
can use shared-dma-pool reserved memory see how venus_boot has been
implemented in the beginning [1].

Arnd what do you think?

Some background, we have a use-case where the memory for firmware needs
to be mapped by the venus driver by hand instead of TZ firmware calls.
I.e. we want to support both, iommu mapping from the driver and mapping
done by TZ firmware. How we will differentiate what mapping (TZ or
non-TZ) will be used is a separate issue.

> 
> By the way, can we really assume that probe of firmware platform
> device really completes before we call venus_boot()?

I'd say we cannot.
  

On Sat, Jun 02, 2018 at 01:56:08AM +0530, Vikash Garodia wrote:
> A separate child device is added for video firmware.
> This is needed to
> [1] configure the firmware context bank with the desired SID.
> [2] ensure that the iova for firmware region is from 0x0.
> 
> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
> ---
>  .../devicetree/bindings/media/qcom,venus.txt       |  8 +++-
>  drivers/media/platform/qcom/venus/core.c           | 48 +++++++++++++++++++---
>  drivers/media/platform/qcom/venus/firmware.c       | 20 ++++++++-
>  drivers/media/platform/qcom/venus/firmware.h       |  2 +
>  4 files changed, 71 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/qcom,venus.txt b/Documentation/devicetree/bindings/media/qcom,venus.txt
> index 00d0d1b..701cbe8 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@1d00000 {
>  		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>;
>  		};
> +		venus-firmware {
> +			compatible = "qcom,venus-firmware-no-tz";
> +			iommus = <&apps_smmu 0x10b2 0x0>;

This mostly looks like you are adding a node in order to create a 
platform device. DT is not the only way to create platform devices and 
shouldn't be used when the device is not really a separate h/w device. 
Plus it seems like it is debatable that you even need a driver.

For iommus, just move it up to the parent (or add to existing prop).

Rob
  

Hi Rob,

On Wed, Jun 6, 2018 at 6:08 AM Rob Herring <robh@kernel.org> wrote:
>
> On Sat, Jun 02, 2018 at 01:56:08AM +0530, Vikash Garodia wrote:
> > A separate child device is added for video firmware.
> > This is needed to
> > [1] configure the firmware context bank with the desired SID.
> > [2] ensure that the iova for firmware region is from 0x0.
> >
> > Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
> > ---
> >  .../devicetree/bindings/media/qcom,venus.txt       |  8 +++-
> >  drivers/media/platform/qcom/venus/core.c           | 48 +++++++++++++++++++---
> >  drivers/media/platform/qcom/venus/firmware.c       | 20 ++++++++-
> >  drivers/media/platform/qcom/venus/firmware.h       |  2 +
> >  4 files changed, 71 insertions(+), 7 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/media/qcom,venus.txt b/Documentation/devicetree/bindings/media/qcom,venus.txt
> > index 00d0d1b..701cbe8 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@1d00000 {
> >               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>;
> >               };
> > +             venus-firmware {
> > +                     compatible = "qcom,venus-firmware-no-tz";
> > +                     iommus = <&apps_smmu 0x10b2 0x0>;
>
> This mostly looks like you are adding a node in order to create a
> platform device. DT is not the only way to create platform devices and
> shouldn't be used when the device is not really a separate h/w device.
> Plus it seems like it is debatable that you even need a driver.
>
> For iommus, just move it up to the parent (or add to existing prop).

As far as I understood the issue from reading this series and also
talking a bit with Stanimir, there are multiple (physical?) ports from
the Venus hardware block and that includes one dedicated for firmware
loading, which has IOVA range restrictions up to 6 MiBs or something
like that.

If we add the firmware port to the iommus property of the main node,
we would bind it to the same IOVA address space as the other ports and
so it would be part of the main full 32-bit IOMMU domain.

Best regards,
Tomasz
  

On Tue, Jun 5, 2018 at 5:45 PM Stanimir Varbanov
<stanimir.varbanov@linaro.org> wrote:
>
> Cc: Arnd
>
> On 06/05/2018 07:08 AM, Tomasz Figa wrote:
> > On Mon, Jun 4, 2018 at 10:56 PM Stanimir Varbanov
> > <stanimir.varbanov@linaro.org> wrote:
> >>
> >> Hi Tomasz,
> >>
> >> On 06/04/2018 04:18 PM, Tomasz Figa wrote:
> >>> Hi Vikash,
> >>>
> >>> On Sat, Jun 2, 2018 at 5:27 AM Vikash Garodia <vgarodia@codeaurora.org> wrote:
> >>>> +static int __init venus_init(void)
> >>>> +{
> >>>> +       int ret;
> >>>> +
> >>>> +       ret = platform_driver_register(&qcom_video_firmware_driver);
> >>>> +       if (ret)
> >>>> +               return ret;
> >>>
> >>> Do we really need this firmware driver? As far as I can see, the
> >>> approach used here should work even without any driver bound to the
> >>> firmware device.
> >>
> >> We need device/driver bind because we need to call dma_configure() which
> >> internally doing iommus sID parsing.
> >
> > I can see some drivers calling of_dma_configure() directly:
> > https://elixir.bootlin.com/linux/latest/ident/of_dma_configure
> >
> > I'm not sure if it's more elegant, but should at least require less code.
>
> I think that in this case of non-TZ where we do iommu mapping by hand we
> can use shared-dma-pool reserved memory see how venus_boot has been
> implemented in the beginning [1].

I might have misunderstood something, but wasn't the shared-dma-pool
about reserving physical memory, while the venus firmware problem is
about reserving certain range of IOVA?

>
> Arnd what do you think?
>
> Some background, we have a use-case where the memory for firmware needs
> to be mapped by the venus driver by hand instead of TZ firmware calls.
> I.e. we want to support both, iommu mapping from the driver and mapping
> done by TZ firmware. How we will differentiate what mapping (TZ or
> non-TZ) will be used is a separate issue.
>
> >
> > By the way, can we really assume that probe of firmware platform
> > device really completes before we call venus_boot()?
>
> I'd say we cannot.

Looking at current implementation in driver core,
of_platform_populate() would actually trigger a synchronous probe, so
I guess it could work. However, I'm not sure if this is a general
guarantee here or it's an implementation detail that shouldn't be
relied on.

If we end up really need to have this platform_driver, I guess we
could call platform_driver_probe() after of_platform_populate(),
rather than pre-registering the driver. That seems to be the way to
ensure that the probe is synchronous and we can also check that a
matching device was found by the return value.

Best regards,
Tomasz
  

On Tue, Jun 5, 2018 at 11:46 PM, Tomasz Figa <tfiga@google.com> wrote:
> Hi Rob,
>
> On Wed, Jun 6, 2018 at 6:08 AM Rob Herring <robh@kernel.org> wrote:
>>
>> On Sat, Jun 02, 2018 at 01:56:08AM +0530, Vikash Garodia wrote:
>> > A separate child device is added for video firmware.
>> > This is needed to
>> > [1] configure the firmware context bank with the desired SID.
>> > [2] ensure that the iova for firmware region is from 0x0.
>> >
>> > Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
>> > ---
>> >  .../devicetree/bindings/media/qcom,venus.txt       |  8 +++-
>> >  drivers/media/platform/qcom/venus/core.c           | 48 +++++++++++++++++++---
>> >  drivers/media/platform/qcom/venus/firmware.c       | 20 ++++++++-
>> >  drivers/media/platform/qcom/venus/firmware.h       |  2 +
>> >  4 files changed, 71 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/Documentation/devicetree/bindings/media/qcom,venus.txt b/Documentation/devicetree/bindings/media/qcom,venus.txt
>> > index 00d0d1b..701cbe8 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@1d00000 {
>> >               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>;
>> >               };
>> > +             venus-firmware {
>> > +                     compatible = "qcom,venus-firmware-no-tz";
>> > +                     iommus = <&apps_smmu 0x10b2 0x0>;
>>
>> This mostly looks like you are adding a node in order to create a
>> platform device. DT is not the only way to create platform devices and
>> shouldn't be used when the device is not really a separate h/w device.
>> Plus it seems like it is debatable that you even need a driver.
>>
>> For iommus, just move it up to the parent (or add to existing prop).
>
> As far as I understood the issue from reading this series and also
> talking a bit with Stanimir, there are multiple (physical?) ports from
> the Venus hardware block and that includes one dedicated for firmware
> loading, which has IOVA range restrictions up to 6 MiBs or something
> like that.
>
> If we add the firmware port to the iommus property of the main node,
> we would bind it to the same IOVA address space as the other ports and
> so it would be part of the main full 32-bit IOMMU domain.

Sounds like an OS limitation, not a DT problem.

That being said, I suppose we can live with having this sub-node if we
can't fix or work-around this limitation.

Rob
  

On 2018-06-06 10:16, Tomasz Figa wrote:
> Hi Rob,
> 
> On Wed, Jun 6, 2018 at 6:08 AM Rob Herring <robh@kernel.org> wrote:
>> 
>> On Sat, Jun 02, 2018 at 01:56:08AM +0530, Vikash Garodia wrote:
>> > A separate child device is added for video firmware.
>> > This is needed to
>> > [1] configure the firmware context bank with the desired SID.
>> > [2] ensure that the iova for firmware region is from 0x0.
>> >
>> > Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
>> > ---
>> >  .../devicetree/bindings/media/qcom,venus.txt       |  8 +++-
>> >  drivers/media/platform/qcom/venus/core.c           | 48 +++++++++++++++++++---
>> >  drivers/media/platform/qcom/venus/firmware.c       | 20 ++++++++-
>> >  drivers/media/platform/qcom/venus/firmware.h       |  2 +
>> >  4 files changed, 71 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/Documentation/devicetree/bindings/media/qcom,venus.txt b/Documentation/devicetree/bindings/media/qcom,venus.txt
>> > index 00d0d1b..701cbe8 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@1d00000 {
>> >               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>;
>> >               };
>> > +             venus-firmware {
>> > +                     compatible = "qcom,venus-firmware-no-tz";
>> > +                     iommus = <&apps_smmu 0x10b2 0x0>;
>> 
>> This mostly looks like you are adding a node in order to create a
>> platform device. DT is not the only way to create platform devices and
>> shouldn't be used when the device is not really a separate h/w device.
>> Plus it seems like it is debatable that you even need a driver.
>> 
>> For iommus, just move it up to the parent (or add to existing prop).
> 
> As far as I understood the issue from reading this series and also
> talking a bit with Stanimir, there are multiple (physical?) ports from
> the Venus hardware block and that includes one dedicated for firmware
> loading, which has IOVA range restrictions up to 6 MiBs or something
> like that.
> 
> If we add the firmware port to the iommus property of the main node,
> we would bind it to the same IOVA address space as the other ports and
> so it would be part of the main full 32-bit IOMMU domain.

Not really port-wise, but the restriction part is right. Once the 
firmware
is loaded, the ARM9 can only execute those firmware instructions if it 
is
present in iova address 0x0.
Merging it to parent device cannot guarantee that the firmware memory is
mapped from 0x0.

> Best regards,
> Tomasz
  

On Mon 04 Jun 06:56 PDT 2018, Stanimir Varbanov wrote:
> On 06/04/2018 04:18 PM, Tomasz Figa wrote:
> > On Sat, Jun 2, 2018 at 5:27 AM Vikash Garodia <vgarodia@codeaurora.org> wrote:
[..]
> >> +               venus-firmware {
> >> +                       compatible = "qcom,venus-firmware-no-tz";
> > 
> > I don't think "-no-tz" should be mentioned here in DT, since it's a
> > firmware/software detail.
> 
> I have to agree with Tomasz, non-tz or tz is a software detail and it
> shouldn't be reflected in compatible string.
> 

While it is software, the alternative boot and security configuration
does imply different requirements on how the driver deals with the
hardware. I'm not sure how you expect the kernel to be informed about
the abilities of the boot/security capabilities if it's not passed
through DT.


In the other cases of firmware loading for co-processors this means that
a number of additional resources (clocks, resets) needs to be specified
in the DT node; something it seems like Venus doesn't have to do.

> Also I'm not sure but what will happen if this video-firmware subnode is
> not added, do you expect that backward compatibility is satisfied for
> older venus versions?
> 

I do expect that the driver should be possible to run on a 845 with the
normal TZ based security model we've seen on e.g. 820. I don't know the
details of Venus well enough to see if this differentiation would be
sufficient.

Regards,
Bjorn