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

Fri Jan 13 19:00:28 CET 2023

Hi Abdellatif,

On Fri, 13 Jan 2023 at 03:44, Abdellatif El Khlifi
<abdellatif.elkhlifi at arm.com> wrote:
>
> On Thu, Jan 12, 2023 at 04:43:32PM -0700, Simon Glass wrote:
> > Hi Rob,
> >
> > On Wed, 11 Jan 2023 at 19:10, Rob Herring <robh at kernel.org> wrote:
> > >
> > > On Mon, Dec 19, 2022 at 1:21 PM Simon Glass <sjg at chromium.org> wrote:
> > > >
> > > > Hi Abdellatif,
> > > >
> > > > On Mon, 19 Dec 2022 at 04:12, Abdellatif El Khlifi
> > > > <abdellatif.elkhlifi at arm.com> wrote:
> > > > >
> > > > > On Mon, Dec 05, 2022 at 09:49:30AM -0600, Rob Herring wrote:
> > > > > > On Sun, Dec 4, 2022 at 1:22 PM Simon Glass <sjg at chromium.org> wrote:
> > > > > > >
> > > > > > > Hi Rob,
> > > > > > >
> > > > > > > On Tue, 29 Nov 2022 at 05:22, Rob Herring <robh at kernel.org> wrote:
> > > > > > > >
> > > > > > > > On Fri, Nov 25, 2022 at 3:18 PM Simon Glass <sjg at chromium.org> wrote:
> > > > > > > > >
> > > > > > > > > Hi Abdellatif,
> > > > > > > > >
> > > > > > > > > On Thu, 24 Nov 2022 at 06:21, Abdellatif El Khlifi <abdellatif.elkhlifi at 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.
> > > > > > > >
> > > > > > > > Where do you get UEFI out of this?
> > > > > > >
> > > > > > > I assume it was UEFI as there was no discussion about this in U-Boot.
> > > > > > > Which firmware project was consulted about this?
> > > > > > >
> > > > > > > >
> > > > > > > > It is the discoverability of hardware that is fixed (and we are stuck
> > > > > > > > with). We can't change hardware. The disoverability may be PCI
> > > > > > > > VID/PID, USB device descriptors, or nothing. We only use DT when those
> > > > > > > > are not sufficient. For a software interface, there is no reason to
> > > > > > > > make them non-discoverable as the interface can be fixed (at least for
> > > > > > > > new things like FF-A).
> > > > > > >
> > > > > > > Here I am talking about the controller itself, the top-level node in
> > > > > > > the device tree. For PCI this is a device tree node and it should be
> > > > > > > the same here. So I am not saying that the devices on the bus need to
> > > > > > > be in the device tree (that can be optional, but may be useful in some
> > > > > > > situations where it is status and known).
> > > > > >
> > > > > > Sure, the PCI host bridges are not discoverable, have a bunch of
> > > > > > resources, and do need to be in DT. The downstream devices only do if
> > > > > > they have extra resources such as when a device is soldered down on a
> > > > > > board rather than a standard slot.
> > > > > >
> > > > > > > We need something like:
> > > > > > >
> > > > > > > ff-a {
> > > > > > >     compatible = "something";
> > > > > > > };
> > > > > > >
> > > > > > > I don't know what mechanism is actually used to communicate with it,
> > > > > > > but that will be enough to get the top-level driver started.
> > > > > >
> > > > > > There's discovery of FF-A itself and then discovery of FF-A features
> > > > > > (e.g. partitions). Both of those are discoverable without DT. The
> > > > > > first is done by checking the SMCCC version, then checking for FF-A
> > > > > > presence and features. Putting this into DT is redundant. Worse, what
> > > > > > if they disagree?
> > > > >
> > > > > Hi Simon,
> > > > >
> > > > > Do you agree with Rob, Ilias and myself that it makes more sense
> > > > > FF-A bus is discovered without a DT node and following the same approach as
> > > > > Linux ? (FF-A bus doesn't have a HW controller and is a purely SW bus,
> > > > > no configuration/description needed at DT level).
> > > > >
> > > > > Your suggestions are always welcome.
> > > >
> > > > I'm sorry I don't agree with that. It does need a compatible string,
> > > > like PCI has. You can just add it in U-Boot if Linux won't accept the
> > > > binding.
> > >
> > > It's not like PCI as the host side of PCI has non-discoverable resources.
> >
> > OK I see. It is certainly an edge case.
> >
> > >
> > > This all could have been designed better, but hindsight is 20/20 and
> > > things evolved step by step. There are a bunch of firmware services
> > > that are all behind SMCCC. The first (upstream) was PSCI. IIRC, SMCCC
> > > was invented a bit after that, but generalized PSCI for other
> > > services. Since then more have been added. More services get added one
> > > by one and yes we added bindings for them. Because what's one more...
> > > But that really needs to stop. We're stuck with h/w that's not
> > > discoverable, there's zero reason to do that with s/w interfaces. If
> > > we could redo everything, we'd have a node for SMCCC and that's it
> > > unless there's h/w resources provided to the rest of DT. But we can't,
> > > so SMCCC is discovered by the presence of PSCI.
> >
> > I understand the background here, but if we don't take a stand on
> > this, this sort of thing will continue. Just because something works
> > in Linux does not mean that the binding (or lack of it) is good.
> >
> > The reasons to do this are:
> > - avoids needing to manually call device_bind()
> > - avoids extra plumbing in U-Boot
> > - provides visibility into what is in the system, by looking at the
> > DT, like documentation
> > - DT is how devices are bound in U-Boot
> >
> > You can see the problem if you look at ffa_device_get(). It is called
> > from ffa_bus_discover() which is a new addition into the board_init
> > list. We are trying to remove this list and certainly don't want new
> > things added!!
>
> Hi Simon,
>
> As stated in the v8 patchset cover letter [1] and readme [2], the FF-A bus is discoverable on demand at runtime.
>
> Clients (such as EFI) can discover the FF-A bus using ffa_bus_discover() API which triggers the
> discovery process.
>
> We no longer use the board_init list to discover the FF-A bus.

That's because you have moved it to the command - everything that
wants to use this has to call ffa_bus_discover()...!

>
> Please refer to the v8 patchset for the review.

It still needs a DT node and the binding needs to happen with driver
model. Please put that in. See also how the sandbox emulation is done
for I2C, SPI, USB, PCI and every other bus, with something in the DT.

We have dm_scan_other() which can pick up other buses but that is just
one function and we don't support multiple versions of it. We could
add that I suppose, but as I say, where does it end?

You can create a u-boot-ffa.dtsi file with the appropriate node and
include that in any board or SoC that uses this feature. Sandbox can
have its emulation node also. You will know that everything is good
when you can boot U-Boot, type 'dm tree' and see your device in there
without running any other commands.

Regards,
Simon

>
> Cheers
>
> [1]: https://lore.kernel.org/all/20221122131751.22747-1-abdellatif.elkhlifi@arm.com/
> [2]: https://lore.kernel.org/all/20221122131751.22747-4-abdellatif.elkhlifi@arm.com/#Z31doc:arch:arm64.ffa.rst
>
> >
> > We don't need to change this in the Linux implementation, just add a
> > top-level DT node for U-Boot. I don't understand why that is such a
> > big problem?
> >
> > Regards,
> > Simon