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

[Tomasz Figa]

On Tue, Jun 5, 2018 at 5:45 PM Stanimir Varbanov
<stanimir.varbanov@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxxxxxx> 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