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

Abdellatif El Khlifi
2022-11-16T13:03:24Z

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