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

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

On Tue, Nov 22, 2022 at 07:09:16PM -0700, Simon Glass wrote:
>  should be called 'priov' and should beHi Abdellatif,
> 
> On Tue, 22 Nov 2022 at 06:18, Abdellatif El Khlifi
> <abdellatif.elkhlifi@arm.com> wrote:
> >
> > Add the core driver implementing Arm Firmware Framework for Armv8-A v1.0
> >
> > The Firmware Framework for Arm A-profile processors (FF-A v1.0) [1]
> > 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.
> >
> > The driver uses SMC32 calling convention which means using the first
> > 32-bit data of the Xn registers.
> >
> > All supported ABIs come with their 32-bit version except FFA_RXTX_MAP
> > which has 64-bit version supported.
> >
> > Both 32-bit and 64-bit direct messaging are supported which allows both
> > 32-bit and 64-bit clients to use the FF-A bus.
> >
> > 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
> >
> > For more details please refer to the driver documentation [2].
> >
> > [1]: https://developer.arm.com/documentation/den0077/latest/
> > [2]: doc/arch/arm64.ffa.rst
> >
> > Signed-off-by: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> > Cc: Tom Rini <trini@konsulko.com>
> > Cc: Simon Glass <sjg@chromium.org>
> > Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > Cc: Jens Wiklander <jens.wiklander@linaro.org>
> >
> > ---
> >
> > Changelog:
> > ===============
> >
> > v8:
> >
> > * make ffa_get_partitions_info() second argument to be an SP count in both
> >   modes
> > * update ffa_bus_prvdata_get() to return a pointer rather than a pointer
> >   address
> > * remove packing from ffa_partition_info and ffa_send_direct_data structures
> > * pass the FF-A bus device to the bus operations
> >
> > v7:
> >
> > * add support for 32-bit direct messaging
> > * rename be_uuid_str_to_le_bin() to uuid_str_to_le_bin()
> > * improve the declaration of error handling mapping
> > * stating in doc/arch/arm64.ffa.rst that EFI runtime is not supported
> >
> > v6:
> >
> > * drop use of EFI runtime support (We decided with Linaro to add this later)
> > * drop discovery from initcalls (discovery will be on demand by FF-A users)
> > * set the alignment of the RX/TX buffers to the larger translation granule size
> > * move FF-A RX/TX buffers unmapping at ExitBootServices() to a separate commit
> > * update the documentation and move it to doc/arch/arm64.ffa.rst
> >
> > v4:
> >
> > * add doc/README.ffa.drv
> > * moving the FF-A driver work to drivers/firmware/arm-ffa
> > * use less #ifdefs in lib/efi_loader/efi_boottime.c and replace
> >   #if defined by #if CONFIG_IS_ENABLED
> > * improving error handling by mapping the FF-A errors to standard errors
> >   and logs
> > * replacing panics with an error log and returning an error code
> > * improving features discovery in FFA_FEATURES by introducing
> >   rxtx_min_pages private data field
> > * add ffa_remove and ffa_unbind functions
> > * improve how the driver behaves when bus discovery is done more than
> >   once
> >
> > v3:
> >
> > * align the interfaces of the U-Boot FF-A driver with those in the linux
> >   FF-A driver
> > * remove the FF-A helper layer
> > * make the U-Boot FF-A driver independent from EFI
> > * provide an optional config that enables copying the driver data to EFI
> >   runtime section at ExitBootServices service
> > * use 64-bit version of FFA_RXTX_MAP, FFA_MSG_SEND_DIRECT_{REQ, RESP}
> >
> > v2:
> >
> > * make FF-A bus discoverable using device_{bind, probe} APIs
> > * remove device tree support
> >
> > v1:
> >
> > * introduce FF-A bus driver with device tree support
> >
> > MAINTAINERS                               |    7 +
> >  doc/arch/arm64.ffa.rst                    |  218 ++++
> >  doc/arch/index.rst                        |    1 +
> >  drivers/Kconfig                           |    2 +
> >  drivers/Makefile                          |    1 +
> >  drivers/firmware/arm-ffa/Kconfig          |   30 +
> >  drivers/firmware/arm-ffa/Makefile         |    6 +
> >  drivers/firmware/arm-ffa/arm-ffa-uclass.c |   16 +
> >  drivers/firmware/arm-ffa/arm_ffa_prv.h    |  200 ++++
> >  drivers/firmware/arm-ffa/core.c           | 1315 +++++++++++++++++++++
> >  include/arm_ffa.h                         |   97 ++
> >  include/dm/uclass-id.h                    |    4 +
> >  12 files changed, 1897 insertions(+)
> >  create mode 100644 doc/arch/arm64.ffa.rst
> >  create mode 100644 drivers/firmware/arm-ffa/Kconfig
> >  create mode 100644 drivers/firmware/arm-ffa/Makefile
> >  create mode 100644 drivers/firmware/arm-ffa/arm-ffa-uclass.c
> >  create mode 100644 drivers/firmware/arm-ffa/arm_ffa_prv.h
> >  create mode 100644 drivers/firmware/arm-ffa/core.c
> >  create mode 100644 include/arm_ffa.h
> 
> This looks mostly OK to me. I have a few comments below.
> [..]
> 
> > diff --git a/drivers/firmware/arm-ffa/arm_ffa_prv.h b/drivers/firmware/arm-ffa/arm_ffa_prv.h
> > new file mode 100644
> > index 0000000000..4eea7dc036
> > --- /dev/null
> > +++ b/drivers/firmware/arm-ffa/arm_ffa_prv.h
> > @@ -0,0 +1,200 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * (C) Copyright 2022 ARM Limited
> > + * Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> > + */
> > +
> > +#ifndef __ARM_FFA_PRV_H
> > +#define __ARM_FFA_PRV_H
> > +
> > +#include <arm_ffa.h>
> > +#include <linux/bitfield.h>
> > +#include <linux/bitops.h>
> > +#include <linux/arm-smccc.h>
> > +
> > +/*
> > + * This header is private. It is exclusively used by the FF-A driver
> > + */
> 
> /* ...*/
> 
> is the single-line comment style
> 
> > +
> > +/* FF-A core driver name */
> > +#define FFA_DRV_NAME "arm_ffa"
> > +
> > +/* FF-A driver version definitions */
> > +
> > +#define MAJOR_VERSION_MASK             GENMASK(30, 16)
> > +#define MINOR_VERSION_MASK             GENMASK(15, 0)
> > +#define GET_FFA_MAJOR_VERSION(x)               \
> > +                               ((u16)(FIELD_GET(MAJOR_VERSION_MASK, (x))))
> > +#define GET_FFA_MINOR_VERSION(x)               \
> > +                               ((u16)(FIELD_GET(MINOR_VERSION_MASK, (x))))
> > +#define PACK_VERSION_INFO(major, minor)                        \
> > +       (FIELD_PREP(MAJOR_VERSION_MASK, (major)) |      \
> > +        FIELD_PREP(MINOR_VERSION_MASK, (minor)))
> > +
> > +#define FFA_MAJOR_VERSION              (1)
> > +#define FFA_MINOR_VERSION              (0)
> > +#define FFA_VERSION_1_0                \
> > +                       PACK_VERSION_INFO(FFA_MAJOR_VERSION, FFA_MINOR_VERSION)
> > +
> > +/* Endpoint ID mask (u-boot endpoint ID) */
> > +
> > +#define GET_SELF_ENDPOINT_ID_MASK              GENMASK(15, 0)
> > +#define GET_SELF_ENDPOINT_ID(x)                \
> > +                       ((u16)(FIELD_GET(GET_SELF_ENDPOINT_ID_MASK, (x))))
> > +
> > +#define PREP_SELF_ENDPOINT_ID_MASK             GENMASK(31, 16)
> > +#define PREP_SELF_ENDPOINT_ID(x)               \
> > +                       (FIELD_PREP(PREP_SELF_ENDPOINT_ID_MASK, (x)))
> > +
> > +/* Partition endpoint ID mask  (partition with which u-boot communicates with) */
> > +
> > +#define PREP_PART_ENDPOINT_ID_MASK             GENMASK(15, 0)
> > +#define PREP_PART_ENDPOINT_ID(x)               \
> > +                       (FIELD_PREP(PREP_PART_ENDPOINT_ID_MASK, (x)))
> > +
> > +/*
> > + * Definitions of the Arm FF-A interfaces supported by the Arm FF-A driver
> > + */
> > +
> > +#define FFA_SMC(calling_convention, func_num)                          \
> > +       ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, (calling_convention),   \
> > +                          ARM_SMCCC_OWNER_STANDARD, (func_num))
> > +
> > +#define FFA_SMC_32(func_num)                           FFA_SMC(ARM_SMCCC_SMC_32, (func_num))
> > +#define FFA_SMC_64(func_num)                           FFA_SMC(ARM_SMCCC_SMC_64, (func_num))
> > +
> > +enum ffa_abis {
> > +       FFA_ERROR                 = 0x60,
> > +       FFA_SUCCESS               = 0x61,
> > +       FFA_INTERRUPT             = 0x62,
> > +       FFA_VERSION               = 0x63,
> > +       FFA_FEATURES              = 0x64,
> > +       FFA_RX_RELEASE            = 0x65,
> > +       FFA_RXTX_MAP              = 0x66,
> > +       FFA_RXTX_UNMAP            = 0x67,
> > +       FFA_PARTITION_INFO_GET    = 0x68,
> > +       FFA_ID_GET                = 0x69,
> > +       FFA_RUN                   = 0x6D,
> > +       FFA_MSG_SEND_DIRECT_REQ   = 0x6F,
> 
> Can you use lower-case hex consistently?
> 
> > +       FFA_MSG_SEND_DIRECT_RESP  = 0x70,
> > +
> > +       /* to be updated when adding new FFA IDs */
> > +       FFA_FIRST_ID              = FFA_ERROR, /* lowest number ID*/
> > +       FFA_LAST_ID               = FFA_MSG_SEND_DIRECT_RESP, /* highest number ID*/
> 
> not: spaces before */
> 
> 
> > +};
> > +
> > +enum ffa_abi_errcode {
> > +       NOT_SUPPORTED = 1,
> > +       INVALID_PARAMETERS,
> > +       NO_MEMORY,
> > +       BUSY,
> > +       INTERRUPTED,
> > +       DENIED,
> > +       RETRY,
> > +       ABORTED,
> > +       MAX_NUMBER_FFA_ERR
> > +};
> > +
> > +/* container structure and helper macros to map between an FF-A error and relevant error log */
> > +struct ffa_abi_errmap {
> > +       char *err_str[MAX_NUMBER_FFA_ERR];
> > +};
> > +
> > +#define FFA_ERRMAP_COUNT (FFA_LAST_ID - FFA_FIRST_ID + 1)
> > +#define FFA_ID_TO_ERRMAP_ID(ffa_id) ((ffa_id) - FFA_FIRST_ID)
> > +
> > +/* The FF-A SMC function definitions */
> > +
> > +typedef struct arm_smccc_1_2_regs ffa_value_t;
> > +typedef void (*invoke_ffa_fn_t)(ffa_value_t args, ffa_value_t *res);
> 
> that needs a comment
> 
> [..]
> 
> > +/**
> > + * struct ffa_rxtxpair - structure hosting the RX/TX buffers virtual addresses
> > + * @rxbuf:     virtual address of the RX buffer
> > + * @txbuf:     virtual address of the TX buffer
> > + * @rxtx_min_pages:    RX/TX buffers minimum size in pages
> > + *
> > + * Data structure hosting the virtual addresses of the mapped RX/TX buffers
> 
> Just a nit but it catches my eye. We know this is a struct. You can
> just say "Hosts the ..."
> 
> [..]
> 
> 
> > diff --git a/drivers/firmware/arm-ffa/core.c b/drivers/firmware/arm-ffa/core.c
> > new file mode 100644
> > index 0000000000..0b1f8e6a07
> > --- /dev/null
> > +++ b/drivers/firmware/arm-ffa/core.c
> > @@ -0,0 +1,1315 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * (C) Copyright 2022 ARM Limited
> > + * Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> > + */
> > +
> > +#include "arm_ffa_prv.h"
> > +#include <asm/global_data.h>
> > +#include <common.h>
> > +#include <dm.h>
> > +#include <dm/device-internal.h>
> > +#include <dm/devres.h>
> > +#include <dm/root.h>
> > +#include <linux/errno.h>
> > +#include <linux/sizes.h>
> > +#include <log.h>
> > +#include <malloc.h>
> > +#include <string.h>
> > +#include <uuid.h>
> 
> See this:
> 
> https://u-boot.readthedocs.io/en/latest/develop/codingstyle.html?highlight=style#include-files
> 
> [..]
> 
> > +/*
> > + * Driver core functions
> > + */
> > +
> > +/**
> > + * ffa_remove_device - removes the arm_ffa device
> > + * @dev:       the device to be removed
> > + *
> > + * This function makes sure the arm_ffa device is removed
> > + * No need to free the kmalloced data when the device is destroyed.
> > + * It's automatically done by devm management by
> > + * device_remove() -> device_free() -> devres_release_probe().
> > + *
> > + * Return:
> > + *
> > + * 0 on success. Otherwise, failure
> > + */
> > +int ffa_remove_device(struct udevice *dev)
> > +{
> > +       int ret;
> > +
> > +       if (!dev) {
> > +               ffa_err("no udevice found");
> > +               return -ENODEV;
> > +       }
> > +
> > +       ret = device_remove(dev, DM_REMOVE_NORMAL);
> > +       if (ret) {
> > +               ffa_err("unable to remove. err:%d\n", ret);
> > +               return ret;
> > +       }
> > +
> > +       ffa_info("device removed and freed");
> > +
> > +       ret = device_unbind(dev);
> > +       if (ret) {
> > +               ffa_err("unable to unbind. err:%d\n", ret);
> > +               return ret;
> > +       }
> > +
> > +       ffa_info("device unbound");
> > +
> > +       return 0;
> > +}
> 
> Do we need this function? We should not be unbinding devices. Even
> removing them should be done elsewhere  if needed. But why?
> 
> > +
> > +/**
> > + * 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].

- 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.

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 }

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/

> [..]
> 
> > +static int ffa_get_version(void)
> > +{
> > +       u16 major, minor;
> > +       ffa_value_t res = {0};
> > +       int ffa_errno;
> > +
> > +       ffa_priv_data->invoke_ffa_fn((ffa_value_t){
> > +                       .a0 = FFA_SMC_32(FFA_VERSION), .a1 = FFA_VERSION_1_0,
> > +                       }, &res);
> > +
> > +       ffa_errno = res.a0;
> > +       if (ffa_errno < 0) {
> > +               ffa_print_error_log(FFA_VERSION, ffa_errno);
> > +               return ffa_to_std_errno(ffa_errno);
> > +       }
> > +
> > +       major = GET_FFA_MAJOR_VERSION(res.a0);
> > +       minor = GET_FFA_MINOR_VERSION(res.a0);
> > +
> > +       ffa_info("FF-A driver %d.%d\nFF-A framework %d.%d",
> > +                FFA_MAJOR_VERSION, FFA_MINOR_VERSION, major, minor);
> > +
> > +       if ((major == FFA_MAJOR_VERSION && minor >= FFA_MINOR_VERSION)) {
> 
> nit: Drop extra brackets
> 
> > +               ffa_info("Versions are compatible ");
> > +
> > +               ffa_priv_data->fwk_version = res.a0;
> > +
> > +               return 0;
> > +       }
> > +
> > +       ffa_err("versions are incompatible\nExpected: %d.%d , Found: %d.%d\n",
> > +               FFA_MAJOR_VERSION, FFA_MINOR_VERSION, major, minor);
> > +
> > +       return -EPROTONOSUPPORT;
> > +}
> > +
> > +/**
> > + * ffa_get_endpoint_id - FFA_ID_GET handler function
> 
> I believe these should have () at the end, so:
> 
> ffa_get_endpoint_id() - FFA_ID_GET handler function
> 
> > + *
> > + * This function implements FFA_ID_GET FF-A function
> > + * to get from the secure world u-boot endpoint ID
> > + *
> > + * Return:
> > + *
> > + * 0 on success. Otherwise, failure
> > + */
> [..]
> 
> > +/**
> > + * ffa_free_rxtx_buffers - frees the RX/TX buffers
> > + *
> > + * This  function  frees the RX/TX buffers
> > + *
> > + */
> > +static void ffa_free_rxtx_buffers(void)
> > +{
> > +       ffa_info("Freeing RX/TX buffers");
> > +
> > +       if (ffa_priv_data->pair.rxbuf) {
> > +               free((void *)ffa_priv_data->pair.rxbuf);
> 
> You can't cast an address to a pointer in this way. Should use
> map_sysmem() or store a pointer.
> 
> > +               ffa_priv_data->pair.rxbuf = 0;
> > +       }
> > +
> > +       if (ffa_priv_data->pair.txbuf) {
> > +               free((void *)ffa_priv_data->pair.txbuf);
> > +               ffa_priv_data->pair.txbuf = 0;
> > +       }
> > +}
> > +[..]
> 
> > +       /*
> > +        * make sure the buffers are cleared before use
> > +        */
> > +       memset((void *)ffa_priv_data->pair.rxbuf, 0, bytes);
> > +       memset((void *)ffa_priv_data->pair.txbuf, 0, bytes);
> 
> Yes these should be pointers everywhere, since you are casting all the time.
> 
> > +
> > +       return 0;
> > +}
> > +
> [..]
> 
> > +/**
> > + * ffa_read_partitions_info - reads the data queried by FFA_PARTITION_INFO_GET
> > + *                                                     and saves it in the private structure
> 
> Can you fit all these titles on one line, like:
> 
> ffa_read_partitions_info() - Read partition information
> 
> <insert longer description here?>
> 
> > + * @count: The number of partitions queried
> > + * @part_uuid: Pointer to the partition(s) UUID
> > + *
> > + * This function reads the partitions information
> > + * returned by the FFA_PARTITION_INFO_GET and saves it in the private
> > + * data structure.
> > + *
> > + * Return:
> > + *
> > + * The private data structure is updated with the partition(s) information
> > + * 0 is returned on success. Otherwise, failure
> > + */
> 
> [..]
> [..]
> 
> > +static int ffa_msg_send_direct_req(struct udevice *dev, u16 dst_part_id,
> > +                                  struct ffa_send_direct_data *msg, bool is_smc64)
> > +{
> > +       ffa_value_t res = {0};
> > +       int ffa_errno;
> > +       u64 req_mode, resp_mode;
> > +
> > +       if (!ffa_priv_data || !ffa_priv_data->invoke_ffa_fn)
> > +               return -EINVAL;
> 
> ffa_priv_data should be called 'priv' and attached to the device with
> device_get_priv()
> [..]
> 
> > +/**
> > + * ffa_probe - The driver probe function
> > + * @dev:       the arm_ffa device
> > + *
> > + * Probing is done at boot time and triggered by the uclass device discovery.
> > + * At probe level the following actions are done:
> > + *     - setting the conduit
> > + *     - querying the FF-A framework version
> > + *     - querying from secure world the u-boot endpoint ID
> > + *     - querying from secure world the supported features of FFA_RXTX_MAP
> > + *     - mapping the RX/TX buffers
> > + *     - querying from secure world all the partitions information
> > + *
> > + * All data queried from secure world is saved in the resident private data structure.
> > + *
> > + * The probe will fail if either FF-A framework is not detected or the
> > + * FF-A requests are not behaving correctly. This ensures that the
> > + * driver is not installed and its operations are not exported to the clients.
> > + *
> > + * Return:
> > + *
> > + * 0 on success. Otherwise, failure
> > + */
> > +static int ffa_probe(struct udevice *dev)
> > +{
> > +       int ret;
> > +
> > +       ret = ffa_alloc_prvdata(dev);
> 
> don't do this, see above.
> 
> [..]
> 
> > diff --git a/include/arm_ffa.h b/include/arm_ffa.h
> > new file mode 100644
> > index 0000000000..74b16174c2
> > --- /dev/null
> > +++ b/include/arm_ffa.h
> > @@ -0,0 +1,97 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * (C) Copyright 2022 ARM Limited
> > + * Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> > + */
> > +
> > +#ifndef __ARM_FFA_H
> > +#define __ARM_FFA_H
> > +
> > +#include <linux/printk.h>
> > +
> > +/*
> > + * This header is public. It can be used by clients to access
> > + * data structures and definitions they need
> > + */
> > +
> > +/*
> > + * Macros for displaying logs
> > + */
> > +
> > +#define ffa_info(fmt, ...)  pr_info("[FFA] " fmt "\n", ##__VA_ARGS__)
> > +#define ffa_err(fmt, ...)  pr_err("[FFA] " fmt "\n", ##__VA_ARGS__)
> 
> You could use log_info(), log_err()
> 
> > +
> > +/*
> > + * struct ffa_partition_info - Partition information descriptor
> > + * @id:        Partition ID
> > + * @exec_ctxt: Execution context count
> > + * @properties:        Partition properties
> > + *
> > + * Data structure containing information about partitions instantiated in the system
> > + * This structure is filled with the data queried by FFA_PARTITION_INFO_GET
> > + */
> > +struct ffa_partition_info {
> > +       u16 id;
> > +       u16 exec_ctxt;
> > +/* partition supports receipt of direct requests */
> > +#define FFA_PARTITION_DIRECT_RECV      BIT(0)
> > +/* partition can send direct requests. */
> > +#define FFA_PARTITION_DIRECT_SEND      BIT(1)
> > +/* partition can send and receive indirect messages. */
> > +#define FFA_PARTITION_INDIRECT_MSG     BIT(2)
> > +       u32 properties;
> > +};
> > +
> > +/*
> > + * struct ffa_send_direct_data - Data structure hosting the data
> > + *                                       used by FFA_MSG_SEND_DIRECT_{REQ,RESP}
> > + * @data0-4:   Data read/written from/to x3-x7 registers
> > + *
> > + * Data structure containing the data to be sent by FFA_MSG_SEND_DIRECT_REQ
> > + * or read from FFA_MSG_SEND_DIRECT_RESP
> > + */
> > +
> > +/* For use with FFA_MSG_SEND_DIRECT_{REQ,RESP} which pass data via registers */
> > +struct ffa_send_direct_data {
> > +       unsigned long data0; /* w3/x3 */
> > +       unsigned long data1; /* w4/x4 */
> > +       unsigned long data2; /* w5/x5 */
> > +       unsigned long data3; /* w6/x6 */
> > +       unsigned long data4; /* w7/x7 */
> > +};
> > +
> > +struct udevice;
> > +
> > +/**
> > + * struct ffa_bus_ops - The driver operations structure
> > + * @partition_info_get:        callback for the FFA_PARTITION_INFO_GET
> > + * @sync_send_receive: callback for the FFA_MSG_SEND_DIRECT_REQ
> > + * @rxtx_unmap:        callback for the FFA_RXTX_UNMAP
> > + *
> > + * The data structure providing all the operations supported by the driver.
> > + * This structure is EFI runtime resident.
> > + */
> > +struct ffa_bus_ops {
> > +       int (*partition_info_get)(struct udevice *dev, const char *uuid_str,
> > +                                 u32 *sp_count, struct ffa_partition_info *buffer);
> > +       int (*sync_send_receive)(struct udevice *dev, u16 dst_part_id,
> > +                                struct ffa_send_direct_data *msg,
> > +                                bool is_smc64);
> > +       int (*rxtx_unmap)(struct udevice *dev);
> > +};
> 
> Shouldn't this be the .ops member in your driver?
> 
> > +
> > +/**
> > + * The device driver and the Uclass driver public functions
> > + */
> > +
> > +/**
> > + * ffa_bus_ops_get - driver operations getter
> > + */
> > +const struct ffa_bus_ops *ffa_bus_ops_get(void);
> 
> See how this is done in other uclasses, e.g. spi_get_ops()
> 
> Regards,
> SImon