Re: [PATCH v3 3/4] arm_ffa: introduce Arm FF-A low-level driver

[Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>]

On Tue, Nov 15, 2022 at 08:24:24AM -0700, Simon Glass wrote:
> Hi,
> 
> On Mon, 1 Aug 2022 at 11:21, Abdellatif El Khlifi
> <abdellatif.elkhlifi@arm.com> wrote:
> >
> > Add the driver implementing Arm Firmware Framework for Armv8-A v1.0
> >
> > The Firmware Framework for Arm A-profile processors (FF-A)
> > describes interfaces (ABIs) that standardize communication
> > between the Secure World and Normal World leveraging TrustZone
> > technology.
> >
> > This driver uses 64-bit registers as per SMCCCv1.2 spec and comes
> > on top of the SMCCC layer. The driver provides the FF-A ABIs needed for
> > querying the FF-A framework from the secure world.
> >
> > 32-bit version of the ABIs is supported and 64-bit version of FFA_RXTX_MAP
> > and FFA_MSG_SEND_DIRECT_{REQ, RESP}.
> >
> > In u-boot FF-A design, FF-A is considered as a discoverable bus.
> > The Secure World is considered as one entity to communicate with
> > using the FF-A bus. FF-A communication is handled by one device and
> > one instance (the bus). This FF-A driver takes care of all the
> > interactions between Normal world and Secure World.
> >
> > The driver exports its operations to be used by upper layers.
> >
> > Exported operations:
> >
> > - partition_info_get
> > - sync_send_receive
> > - rxtx_unmap
> >
> > This implementation provides an optional feature to copy the driver data
> > to EFI runtime area.
> >
> > Signed-off-by: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> > Cc: Tom Rini <trini@konsulko.com>
> > Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > Cc: Jens Wiklander <jens.wiklander@linaro.org>
> > ---
> >  MAINTAINERS                                |    6 +
> >  common/board_r.c                           |    7 +
> >  drivers/Kconfig                            |    2 +
> >  drivers/Makefile                           |    1 +
> >  drivers/arm-ffa/Kconfig                    |   33 +
> >  drivers/arm-ffa/Makefile                   |    7 +
> >  drivers/arm-ffa/arm-ffa-uclass.c           |   16 +
> >  drivers/arm-ffa/arm_ffa_prv.h              |  219 ++++
> >  drivers/arm-ffa/core.c                     | 1338 ++++++++++++++++++++
> >  drivers/arm-ffa/efi_ffa_runtime_data_mgr.c |   94 ++
> >  include/arm_ffa.h                          |  132 ++
> >  include/dm/uclass-id.h                     |    1 +
> >  include/uuid.h                             |    8 +
> >  lib/efi_loader/efi_boottime.c              |   17 +
> >  lib/uuid.c                                 |   65 +
> >  15 files changed, 1946 insertions(+)
> >  create mode 100644 drivers/arm-ffa/Kconfig
> >  create mode 100644 drivers/arm-ffa/Makefile
> >  create mode 100644 drivers/arm-ffa/arm-ffa-uclass.c
> >  create mode 100644 drivers/arm-ffa/arm_ffa_prv.h
> >  create mode 100644 drivers/arm-ffa/core.c
> >  create mode 100644 drivers/arm-ffa/.c
> >  create mode 100644 include/arm_ffa.h
> 
> Please add something to doc/ so people know what this is.
> 
> Since you are adding a new uclass you need a sandbox driver and tests.
> 
> The driver appears to have no operations, but there is a bus_ops. The
> ops should go in the driver, I suspect, and should pass the device as
> the first arg.
> 
> Can FFA_ERR_STAT_SUCCESS be 0 so you don't have to sprinkle the code with it?
> 
> Why is it using EFI things? Can this driver only be used with UEFI? I
> hope not, if it is an official way of updating firmware.
> 
> Please don't add more things to board_r.c - we are trying to remove
> this init over time. If it is a device it should be probed as needed.
> 
> Is there a device tree binding?
> 
> Also should this go in drivers/misc instead of creating a whole new subdir?

Hi Simon, thanks for reviewing.

All the above comments have already been addressed in the new versions
of the patchset. Please refer to the latest version v7 [1].

By the way I'd like to highlight the following:

- The FF-A driver documentation is at doc/arch/arm64.ffa.rst, please
  refer to it since it provides helpful details about the FF-A support
  in U-Boot
- The patchset comes with Sandbox driver and tests [2]
- The driver has operations defined in struct ffa_bus_ops (include/arm_ffa.h).
  ffa_bus_ops_get() gets the ops. All these are in the driver (drivers/firmware/arm-ffa/core.c)
- The FF-A bus has only 1 device. No multiple instances. So passing the
  device doesn't make sense in our case
- FFA_ERR_STAT_SUCCESS has been removed and replaced with 0
- The driver is independent from EFI and can be compiled without EFI
- FF-A bus discovery has been removed from the initcall level (board_r.c)
  Discovery is done on demand. Clients can call ffa_bus_discover() when
  they want to use the FF-A bus. As an example of how clients initiate
  discovery please refer to the FF-A MM comms client [3].
- As done in the Linux kernel, the FF-A bus doesn't have a device tree
  binding since there is no peripheral associated with FF-A. At the
  early stages of this patchset, we double checked with the device tree
  maintainer and the decision was no device tree for FF-A
- The links below are from the U-Boot mailing list mirror in lore.kernel.org

Cheers.

[1]: https://lore.kernel.org/all/20221107192055.21669-1-abdellatif.elkhlifi@arm.com/
[2]: https://lore.kernel.org/all/20221107192055.21669-7-abdellatif.elkhlifi@arm.com/
     https://lore.kernel.org/all/20221107192055.21669-8-abdellatif.elkhlifi@arm.com/
     https://lore.kernel.org/all/20221107192055.21669-9-abdellatif.elkhlifi@arm.com/
[3]: https://lore.kernel.org/all/20221107192055.21669-10-abdellatif.elkhlifi@arm.com/
> 
> Regards,
> Simon