Re: [PATCH v8 03/10] arm_ffa: introduce Arm FF-A low-level driver

[Simon Glass <sjg@chromium.org>]

Hi Abdellatif,

On Thu, 24 Nov 2022 at 06:21, Abdellatif El Khlifi <
abdellatif.elkhlifi@arm.com> wrote:
>
> On Tue, Nov 22, 2022 at 07:09:16PM -0700, Simon Glass wrote:
> >  should be called 'priov' and should beHi Abdellatif,
> >

[..]

> > > +/**
> > > + * ffa_device_get - create, bind and probe the arm_ffa device
> > > + * @pdev: the address of a device pointer (to be filled when the
arm_ffa bus device is created
> > > + *       successfully)
> > > + *
> > > + * This function makes sure the arm_ffa device is
> > > + * created, bound to this driver, probed and ready to use.
> > > + * Arm FF-A transport is implemented through a single U-Boot
> > > + * device managing the FF-A bus (arm_ffa).
> > > + *
> > > + * Return:
> > > + *
> > > + * 0 on success. Otherwise, failure
> > > + */
> > > +int ffa_device_get(struct udevice **pdev)
> > > +{
> > > +       int ret;
> > > +       struct udevice *dev = NULL;
> > > +
> > > +       ret = device_bind(dm_root(), DM_DRIVER_GET(arm_ffa),
FFA_DRV_NAME, NULL, ofnode_null(),
> > > +                         &dev);
> >
> > Please add a DT binding. Even if only temporary, we need something for
this.
>
> Thanks for the feedback. I'm happy to address all the comments.
>
> Regarding DT binding and FF-A discovery. We agreed with Linaro and Rob
Herring
> about the following:
>
> - DT is only for what we failed to make discoverable. For hardware, we're
stuck
>   with it. We shouldn't repeat that for software interfaces. This
approach is
>   already applied in the FF-A kernel driver which comes with no DT
support and
>   discovers the bus with bus_register() API [1].

This may be the UEFI view, but it is not how U-Boot works. This is not
something we are 'stuck' with. It is how we define what is present on a
device. This is how the PCI bus works in U-Boot. It is best practice in
U-Boot to use the device tree to make this things visible and configurable.
Unlike with Linux there is no other way to provide configuration needed by
these devices.

>
> - FF-A bus in U-Boot is used on demand by clients. We don't want to set
it up
>   if no client is using it. For example, the EFI MM client tries to
discover the
>   FF-A bus, if it succeeds it uses it. Otherwise, it uses OP-TEE protocol
[2].
>   EFI MM client tries to discover the FF-A bus by calling
ffa_bus_discover()
>   which is a discovery API provided by the FF-A core driver.

You are talking about probing, I think. Lazy init is build into U-Boot and
works fine. We don't need to invent another way to do it. Probe the device
if it is needed, but bind it always.

>
> Here is an overview about what ffa_bus_discover() does :
>
> client -> ffa_bus_discover() -> ffa_device_get() -> { device_bind() ,
device_probe() }
>
> device_probe() -> ffa_probe() -> { at this stage the FF-A driver tries to
discover the bus by executing FF-A discovery ABIs }

Yes it needs a bit of clean-up, to be honest. You need to add a device
pointer to the calls as well.

>
> Let's see what Ilias and Rob think about FF-A discovery and DT support.
>
> Cheers
> Abdellatif
>
> [1]:
https://elixir.bootlin.com/linux/v6.1-rc4/source/drivers/firmware/arm_ffa/bus.c#L213
> [2]:
https://lore.kernel.org/all/20221122131751.22747-10-abdellatif.elkhlifi@arm.com/
>

Regards,
SImon