[PATCH v9 06/10] arm_ffa: introduce the FF-A Sandbox driver

Abdellatif El Khlifi abdellatif.elkhlifi at arm.com
Tue Mar 14 13:55:25 CET 2023


On Fri, Mar 10, 2023 at 12:49:57PM -0800, Simon Glass wrote:
> Hi Abdellatif,
> 
> On Fri, 10 Mar 2023 at 06:10, Abdellatif El Khlifi
> <abdellatif.elkhlifi at arm.com> wrote:
> >
> > Provide a Sandbox driver to emulate the FF-A ABIs
> >
> > The emulated ABIs are those supported by the FF-A core driver
> > and according to FF-A specification v1.0.
> >
> > The Sandbox driver provides operations allowing the test
> > application to read the status of all the inspected ABIs
> > and perform functional tests based on that.
> >
> > sandbox driver supports only 64-bit direct messaging.
> >
> > Signed-off-by: Abdellatif El Khlifi <abdellatif.elkhlifi at arm.com>
> > Cc: Tom Rini <trini at konsulko.com>
> > Cc: Simon Glass <sjg at chromium.org>
> > Cc: Ilias Apalodimas <ilias.apalodimas at linaro.org>
> > Cc: Jens Wiklander <jens.wiklander at linaro.org>
> >
> > ---
> > Changelog:
> > ===============
> >
> > v9: align FF-A sandbox driver with FF-A discovery through DM
> >
> > v8: update ffa_bus_prvdata_get() to return a pointer rather than
> >     a pointer address
> >
> > v7: state that sandbox driver supports only 64-bit direct messaging
> >
> > v4: align sandbox driver with the new FF-A driver interfaces
> >     and new way of error handling
> >
> > v1: introduce the sandbox driver
> >
> >  MAINTAINERS                                   |   1 +
> >  arch/sandbox/dts/sandbox.dtsi                 |   4 +
> >  arch/sandbox/dts/test.dts                     |   4 +
> >  configs/sandbox64_defconfig                   |   2 +
> >  configs/sandbox_defconfig                     |   2 +
> >  doc/arch/arm64.ffa.rst                        |   4 +
> >  doc/arch/sandbox/sandbox.rst                  |   1 +
> >  drivers/firmware/arm-ffa/Kconfig              |  11 +-
> >  drivers/firmware/arm-ffa/Makefile             |   1 +
> >  drivers/firmware/arm-ffa/core.c               |  36 +-
> >  drivers/firmware/arm-ffa/sandbox.c            | 610 ++++++++++++++++++
> >  .../firmware/arm-ffa/sandbox_arm_ffa_priv.h   | 129 ++++
> >  include/arm_ffa.h                             |   5 +-
> >  include/sandbox_arm_ffa.h                     | 124 ++++
> >  14 files changed, 928 insertions(+), 6 deletions(-)
> >  create mode 100644 drivers/firmware/arm-ffa/sandbox.c
> >  create mode 100644 drivers/firmware/arm-ffa/sandbox_arm_ffa_priv.h
> >  create mode 100644 include/sandbox_arm_ffa.h
> >
> 
> Could you use 80 columns where possible? There seem to be a lot of
> things that extend beyond that without much of a reason.
> 
> Also you can use 'ulong' instead of 'unsigned long'. It is less
> verbose and a U-Boot standard.
> 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 18e9c2ce99..2b9d33e964 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -274,6 +274,7 @@ F:  doc/arch/arm64.ffa.rst
> >  F:     doc/usage/cmd/armffa.rst
> >  F:     drivers/firmware/arm-ffa/
> >  F:     include/arm_ffa.h
> > +F:     include/sandbox_arm_ffa.h
> >
> >  ARM FREESCALE IMX
> >  M:     Stefano Babic <sbabic at denx.de>
> > diff --git a/arch/sandbox/dts/sandbox.dtsi b/arch/sandbox/dts/sandbox.dtsi
> > index 30a305c4d2..059c273277 100644
> > --- a/arch/sandbox/dts/sandbox.dtsi
> > +++ b/arch/sandbox/dts/sandbox.dtsi
> > @@ -445,6 +445,10 @@
> >         thermal {
> >                 compatible = "sandbox,thermal";
> >         };
> > +
> > +       sandbox_arm_ffa {
> > +               compatible = "sandbox,arm_ffa";
> > +       };
> >  };
> >
> >  &cros_ec {
> > diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
> > index d72d7a567a..11dc6ed0d9 100644
> > --- a/arch/sandbox/dts/test.dts
> > +++ b/arch/sandbox/dts/test.dts
> > @@ -1802,6 +1802,10 @@
> >                 compatible = "u-boot,fwu-mdata-gpt";
> >                 fwu-mdata-store = <&mmc0>;
> >         };
> > +
> > +       sandbox_arm_ffa {
> > +               compatible = "sandbox,arm_ffa";
> > +       };
> 
> I see that you have this, so the driver should bind automatically.
> 
> Is the problem that you are trying to bind the sandbox emulator?
> Again, you can actually add that to the DT. See how this works for
> i2c, SPI, PCI, for example.
> 
> >  };
> >
> >  #include "sandbox_pmic.dtsi"
> > diff --git a/configs/sandbox64_defconfig b/configs/sandbox64_defconfig
> > index ccbc18aad0..35d4676cf7 100644
> > --- a/configs/sandbox64_defconfig
> > +++ b/configs/sandbox64_defconfig
> > @@ -259,3 +259,5 @@ CONFIG_FWU_MULTI_BANK_UPDATE=y
> >  CONFIG_UNIT_TEST=y
> >  CONFIG_UT_TIME=y
> >  CONFIG_UT_DM=y
> > +CONFIG_ARM_FFA_TRANSPORT=y
> > +CONFIG_SANDBOX_FFA=y
> > diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
> > index a0fbdad20a..8aab8dda31 100644
> > --- a/configs/sandbox_defconfig
> > +++ b/configs/sandbox_defconfig
> > @@ -336,3 +336,5 @@ CONFIG_TEST_FDTDEC=y
> >  CONFIG_UNIT_TEST=y
> >  CONFIG_UT_TIME=y
> >  CONFIG_UT_DM=y
> > +CONFIG_ARM_FFA_TRANSPORT=y
> > +CONFIG_SANDBOX_FFA=y
> > \ No newline at end of file
> > diff --git a/doc/arch/arm64.ffa.rst b/doc/arch/arm64.ffa.rst
> > index 8fad9ef3d0..555ee9a6ae 100644
> > --- a/doc/arch/arm64.ffa.rst
> > +++ b/doc/arch/arm64.ffa.rst
> > @@ -64,6 +64,10 @@ CONFIG_ARM_FFA_TRANSPORT
> >      Enables the FF-A bus driver. Turn this on if you want to use FF-A
> >      communication.
> >
> > +CONFIG_SANDBOX_FFA
> > +    Enables FF-A Sandbox driver. This emulates the FF-A ABIs handling under
> > +    Sandbox and provides functional tests for FF-A.
> 
> OK that is why I am confused. Please don't call this a driver. It is
> an emulator. When you have an emulator and a driver for the same thing
> it gets confusing.
> 
> > +
> >  FF-A ABIs under the hood
> >  ---------------------------------------
> >
> > diff --git a/doc/arch/sandbox/sandbox.rst b/doc/arch/sandbox/sandbox.rst
> > index cd7f8a2cb0..c5df372e00 100644
> > --- a/doc/arch/sandbox/sandbox.rst
> > +++ b/doc/arch/sandbox/sandbox.rst
> > @@ -200,6 +200,7 @@ Supported Drivers
> >
> >  U-Boot sandbox supports these emulations:
> >
> > +- Arm FF-A
> >  - Block devices
> >  - Chrome OS EC
> >  - GPIO
> > diff --git a/drivers/firmware/arm-ffa/Kconfig b/drivers/firmware/arm-ffa/Kconfig
> > index 2cfd7ef5fc..b5430eb6f4 100644
> > --- a/drivers/firmware/arm-ffa/Kconfig
> > +++ b/drivers/firmware/arm-ffa/Kconfig
> > @@ -2,9 +2,9 @@
> >
> >  config ARM_FFA_TRANSPORT
> >         bool "Enable Arm Firmware Framework for Armv8-A driver"
> > -       depends on DM && ARM64
> > -       select ARM_SMCCC
> > -       select ARM_SMCCC_FEATURES
> > +       depends on DM && (ARM64 || SANDBOX)
> > +       select ARM_SMCCC if !SANDBOX
> > +       select ARM_SMCCC_FEATURES if !SANDBOX
> >         imply CMD_ARMFFA
> >         select LIB_UUID
> >         select DEVRES
> > @@ -32,3 +32,8 @@ config ARM_FFA_TRANSPORT
> >
> >           For more details about the FF-A driver, please refer to doc/arch/arm64.ffa.rst
> >
> > +config SANDBOX_FFA
> > +       bool "FF-A Sandbox driver"
> > +       depends on ARM_FFA_TRANSPORT && SANDBOX
> > +       help
> > +         This emulates the FF-A handling under Sandbox and allows to test the FF-A driver
> > diff --git a/drivers/firmware/arm-ffa/Makefile b/drivers/firmware/arm-ffa/Makefile
> > index c8d83b4bc9..d22c1ba609 100644
> > --- a/drivers/firmware/arm-ffa/Makefile
> > +++ b/drivers/firmware/arm-ffa/Makefile
> > @@ -6,3 +6,4 @@
> >  #   Abdellatif El Khlifi <abdellatif.elkhlifi at arm.com>
> >
> >  obj-y += arm-ffa-uclass.o core.o
> > +obj-$(CONFIG_SANDBOX_FFA) += sandbox.o
> > diff --git a/drivers/firmware/arm-ffa/core.c b/drivers/firmware/arm-ffa/core.c
> > index 2210f5343c..0d2e6ff0d4 100644
> > --- a/drivers/firmware/arm-ffa/core.c
> > +++ b/drivers/firmware/arm-ffa/core.c
> > @@ -1042,6 +1042,7 @@ bool ffa_try_discovery(void)
> >         return true;
> >  }
> >
> > +#if !CONFIG_IS_ENABLED(SANDBOX_FFA)
> 
> We should not need #ifdefs here. The sandbox driver is just another
> driver, just like there is the ARM driver. We select the correct one
> using the devicetree and everything just works.
> 
> >  /**
> >   * __arm_ffa_fn_smc() - SMC wrapper
> >   * @args: FF-A ABI arguments to be copied to Xn registers
> > @@ -1069,6 +1070,7 @@ void __arm_ffa_fn_smc(ffa_value_t args, ffa_value_t *res)
> >   * The FF-A driver supports the SMCCCv1.2 extended input/output registers.
> >   * So, the legacy SMC invocation is not used.
> >   *
> > + * In Sandbox mode sandbox_arm_ffa is used to test arm_ffa driver.
> >   * Return:
> >   *
> >   * 0 on success. Otherwise, failure
> > @@ -1088,6 +1090,30 @@ ARM_SMCCC_FEATURE_DRIVER(arm_ffa) = {
> >         .driver_name = FFA_DRV_NAME,
> >         .is_supported = ffa_bus_is_supported,
> >  };
> > +#else
> > +/* SANDBOX_FFA */
> > +
> > +/**
> > + * ffa_bind() - The driver bind function
> > + * @dev:       the arm_ffa device
> > + * When using sandbox tries to discover the emulated FF-A bus.
> > + * Return:
> > + *
> > + * 0 on success.
> > + */
> > +static int ffa_bind(struct udevice *dev)
> 
> I don't think this is binding anything. How about ffa_get_dev() ?
> 
> > +{
> > +       bool ret;
> > +
> > +       log_info("[FFA] binding the device\n");
> > +
> > +       ret = ffa_try_discovery();
> > +       if (ret)
> > +               return 0;
> > +       else
> > +               return -ENODEV;
> > +}
> > +#endif
> >
> >  /**
> >   * ffa_set_smc_conduit() - Set the SMC conduit
> > @@ -1101,7 +1127,12 @@ ARM_SMCCC_FEATURE_DRIVER(arm_ffa) = {
> >   */
> >  static int ffa_set_smc_conduit(void)
> >  {
> > -       dscvry_info.invoke_ffa_fn = __arm_ffa_fn_smc;
> > +#if CONFIG_IS_ENABLED(SANDBOX_FFA)
> > +               dscvry_info.invoke_ffa_fn = sandbox_arm_ffa_smccc_smc;
> > +               log_info("[FFA] Using SMC emulation\n");
> > +#else
> > +               dscvry_info.invoke_ffa_fn = __arm_ffa_fn_smc;
> > +#endif
> 
> This needs to be reworked to go through the uclass to the correct
> driver. Basically you need an ARM driver and a sandbox driver (which
> attaches to the emulator).
> 
> There should not be #idefs in this sort of code...driver model should
> handle it. See other uclasses for examples.
> 
> >
> >         log_info("[FFA] Conduit is SMC\n");
> >
> > @@ -1246,4 +1277,7 @@ U_BOOT_DRIVER(arm_ffa) = {
> >         .remove = ffa_remove,
> >         .unbind = ffa_unbind,
> >         .ops            = &ffa_ops,
> > +#if CONFIG_IS_ENABLED(SANDBOX_FFA)
> > +       .bind           = ffa_bind,
> 
> Can drop this
> 
> > +#endif
> >  };
> > diff --git a/drivers/firmware/arm-ffa/sandbox.c b/drivers/firmware/arm-ffa/sandbox.c
> 
> sandbox_emul.c pehaps?
> 
> I am pretty sure this is an emulator
> 
> > new file mode 100644
> > index 0000000000..84c2fc929f
> > --- /dev/null
> > +++ b/drivers/firmware/arm-ffa/sandbox.c
> > @@ -0,0 +1,610 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright 2022-2023 Arm Limited and/or its affiliates <open-source-office at arm.com>
> > + *
> > + * Authors:
> > + *   Abdellatif El Khlifi <abdellatif.elkhlifi at arm.com>
> > + */
> > +
> > +#include <common.h>
> > +#include <dm.h>
> > +#include <mapmem.h>
> > +#include <string.h>
> > +#include <asm/global_data.h>
> > +#include <dm/device-internal.h>
> > +#include <dm/lists.h>
> > +#include <dm/root.h>
> > +#include <linux/errno.h>
> > +#include <linux/sizes.h>
> > +#include "sandbox_arm_ffa_priv.h"
> > +
> > +DECLARE_GLOBAL_DATA_PTR;
> > +
> > +/* The partitions (SPs) table */
> > +static struct ffa_partition_desc sandbox_partitions[SANDBOX_PARTITIONS_CNT] = {
> > +       {
> > +               .info = { .id = SANDBOX_SP1_ID, .exec_ctxt = 0x5687, .properties = 0x89325621 },
> > +               .sp_uuid = {
> > +                       .a1 = SANDBOX_SERVICE1_UUID_A1,
> > +                       .a2 = SANDBOX_SERVICE1_UUID_A2,
> > +                       .a3 = SANDBOX_SERVICE1_UUID_A3,
> > +                       .a4 = SANDBOX_SERVICE1_UUID_A4,
> > +               }
> > +       },
> > +       {
> > +               .info = { .id = SANDBOX_SP2_ID, .exec_ctxt = 0x9587, .properties = 0x45325621 },
> > +               .sp_uuid = {
> > +                       .a1 = SANDBOX_SERVICE2_UUID_A1,
> > +                       .a2 = SANDBOX_SERVICE2_UUID_A2,
> > +                       .a3 = SANDBOX_SERVICE2_UUID_A3,
> > +                       .a4 = SANDBOX_SERVICE2_UUID_A4,
> > +               }
> > +       },
> > +       {
> > +               .info = { .id = SANDBOX_SP3_ID, .exec_ctxt = 0x7687, .properties = 0x23325621 },
> > +               .sp_uuid = {
> > +                       .a1 = SANDBOX_SERVICE1_UUID_A1,
> > +                       .a2 = SANDBOX_SERVICE1_UUID_A2,
> > +                       .a3 = SANDBOX_SERVICE1_UUID_A3,
> > +                       .a4 = SANDBOX_SERVICE1_UUID_A4,
> > +               }
> > +       },
> > +       {
> > +               .info = { .id = SANDBOX_SP4_ID, .exec_ctxt = 0x1487, .properties = 0x70325621 },
> > +               .sp_uuid = {
> > +                       .a1 = SANDBOX_SERVICE2_UUID_A1,
> > +                       .a2 = SANDBOX_SERVICE2_UUID_A2,
> > +                       .a3 = SANDBOX_SERVICE2_UUID_A3,
> > +                       .a4 = SANDBOX_SERVICE2_UUID_A4,
> > +               }
> > +       }
> > +
> > +};
> > +
> > +/* Driver functions */
> > +
> > +/**
> > + * sandbox_ffa_version() - Emulated FFA_VERSION handler function
> > + * @dev:       the sandbox FF-A device
> > + * @{a0-a7} , res: The SMC call arguments and return structure.
> > + *
> > + * This is the function that emulates FFA_VERSION FF-A function.
> > + *
> > + * Return:
> > + *
> > + * 0 on success. Otherwise, failure
> > + */
> > +SANDBOX_SMC_FFA_ABI(ffa_version)
> 
> Instead of the macro can you write this out in full? It defeats ctags, etc.
> 
> > +{
> > +       struct sandbox_ffa_priv *priv = dev_get_priv(dev);
> > +
> > +       priv->fwk_version = FFA_VERSION_1_0;
> > +       res->a0 = priv->fwk_version;
> 
> Where is res defined?
> 
> > +
> > +       /* x1-x7 MBZ */
> > +       memset(FFA_X1X7_MBZ_REG_START, 0, FFA_X1X7_MBZ_CNT * sizeof(unsigned long));
> > +
> > +       return 0;
> > +}
> > +
> > +/**
> > + * sandbox_ffa_id_get() - Emulated FFA_ID_GET handler function
> > + * @dev:       the sandbox FF-A device
> > + * @{a0-a7} , res: The SMC call arguments and return structure.
> > + *
> > + * This is the function that emulates FFA_ID_GET FF-A function.
> 
> s/This is the function that e/E/g
> 
> > + *
> > + * Return:
> > + *
> > + * 0 on success. Otherwise, failure
> > + */
> > +SANDBOX_SMC_FFA_ABI(ffa_id_get)
> > +{
> > +       struct sandbox_ffa_priv *priv = dev_get_priv(dev);
> > +
> > +       res->a0 = FFA_SMC_32(FFA_SUCCESS);
> > +       res->a1 = 0;
> > +
> > +       priv->id = NS_PHYS_ENDPOINT_ID;
> > +       res->a2 = priv->id;
> > +
> > +       /* x3-x7 MBZ */
> > +       memset(FFA_X3_MBZ_REG_START, 0, FFA_X3X7_MBZ_CNT * sizeof(unsigned long));
> > +
> > +       return 0;
> > +}
> > +
> > +/**
> > + * sandbox_ffa_features() - Emulated FFA_FEATURES handler function
> > + * @dev:       the sandbox FF-A device
> > + * @{a0-a7} , res: The SMC call arguments and return structure.
> > + *
> > + * This is the function that emulates FFA_FEATURES FF-A function.
> > + *
> > + * Return:
> > + *
> > + * 0 on success. Otherwise, failure
> > + */
> > +SANDBOX_SMC_FFA_ABI(ffa_features)
> > +{
> > +       if (pargs->a1 == FFA_SMC_64(FFA_RXTX_MAP)) {
> > +               res->a0 = FFA_SMC_32(FFA_SUCCESS);
> > +               res->a2 = RXTX_BUFFERS_MIN_SIZE;
> > +               res->a3 = 0;
> > +               /* x4-x7 MBZ */
> > +               memset(FFA_X4X7_MBZ_REG_START,
> > +                      0, FFA_X4X7_MBZ_CNT * sizeof(unsigned long));
> > +       } else {
> > +               res->a0 = FFA_SMC_32(FFA_ERROR);
> > +               res->a2 = FFA_ERR_STAT_NOT_SUPPORTED;
> > +               /* x3-x7 MBZ */
> > +               memset(FFA_X3_MBZ_REG_START,
> > +                      0, FFA_X3X7_MBZ_CNT * sizeof(unsigned long));
> > +               log_err("[FFA] [Sandbox] FF-A interface 0x%lx not implemented\n", pargs->a1);
> 
> return -ENOSYS
> 
> > +       }
> > +
> > +       res->a1 = 0;
> > +
> > +       return 0;
> > +}
> > +
> 
> [..]
> 
> > +SANDBOX_SMC_FFA_ABI(ffa_msg_send_direct_req)
> > +{
> > +       u16 part_id;
> > +       struct sandbox_ffa_priv *priv = dev_get_priv(dev);
> > +
> > +       part_id = GET_DST_SP_ID(pargs->a1);
> > +
> > +       if ((GET_NS_PHYS_ENDPOINT_ID(pargs->a1) != priv->id) ||
> 
> drop extra brackets
> 
> > +           !sandbox_ffa_sp_valid(dev, part_id) || pargs->a2) {
> > +               res->a0 = FFA_SMC_32(FFA_ERROR);
> > +               res->a1 = 0;
> > +               res->a2 = FFA_ERR_STAT_INVALID_PARAMETERS;
> > +
> > +               /* x3-x7 MBZ */
> > +               memset(FFA_X3_MBZ_REG_START,
> > +                      0, FFA_X3X7_MBZ_CNT * sizeof(unsigned long));
> > +
> > +               return 0;
> > +       }
> > +
> > +       res->a0 = FFA_SMC_64(FFA_MSG_SEND_DIRECT_RESP);
> > +
> > +       res->a1 = PREP_SRC_SP_ID(part_id) |
> > +               PREP_NS_PHYS_ENDPOINT_ID(priv->id);
> > +
> > +       res->a2 = 0;
> > +
> > +       /* Return 0xff bytes as a response */
> > +       res->a3 = 0xffffffffffffffff;
> > +       res->a4 = 0xffffffffffffffff;
> > +       res->a5 = 0xffffffffffffffff;
> > +       res->a6 = 0xffffffffffffffff;
> > +       res->a7 = 0xffffffffffffffff;
> 
> Would -1UL work?
> 
> > +
> > +       return 0;
> > +}
> > +
> 
> [..]
> 
> > +static int sandbox_ffa_probe(struct udevice *dev)
> > +{
> > +       struct udevice *child_dev = NULL;
> > +       int ret;
> > +
> > +       ret = device_bind_driver(dev, FFA_DRV_NAME, FFA_DRV_NAME, &child_dev);
> 
> Is this binding the emulator? Add it to the DT instead.
> 
> If you put
> 
> .post_bind = dm_scan_fdt_dev()
> 
> in your uclass (like spi-uclass.c does) then it will binding
> child-node devices automatically. Then you can make your emulator a
> child node.
> 

Hi Simon,

Thanks for the comments. I'm happy to address them.

I'd like to explain the following points:

1/ The FF-A core driver is the driver that interacts with secure world. The driver is implemented in drivers/firmware/arm-ffa/core.c

The FF-A sandbox driver is the driver used to emulate the FF-A side of the secure world and interacts with the core driver.
The sandbox driver is implemented in drivers/firmware/arm-ffa/sandbox.c

If you prefer we call it FF-A sandbox emulator, please let me know.
I'm calling it sandbox driver based on what the checkpatch script is suggesting.

2/ As we agreed with Rob, in the patchset v9 , FF-A is seen as an architecture feature and is discovered by software.
The root device tree node is the PSCI device and the architecture features are children of the PSCI device.

When the PSCI driver is probed, it tries to discover the architecture features (including FF-A).
When the FF-A discovery succeeds, PSCI creates the FF-A device as a child of PSCI device, and binds the FF-A core driver with the FF-A core device.

This is done here [1] using device_bind_driver().

   => dm tree

    Class     Index  Probed  Driver                Name
   -----------------------------------------------------------
   ...
    firmware      0  [ + ]   psci                      |-- psci
    ffa                   0  [   ]   arm_ffa               |   `-- arm_ffa

Since PSCI is not supported in sandbox, we can not use PSCI driver to discover the emulated FF-A framework.
In place of the PSCI driver, FF-A discovery and binding is done in the FF-A sandbox driver at probe level (same way as PSCI).
The FF-A core device is bound and set as a child of the FF-A sandbox device.
This garantees the same behaviour as the real use case.

Is it OK to keep it like done for the PSCI please ? ( using device_bind_driver() )

3/ FF-A core device and FF-A sandbox device both belong to UCLASS_FFA.

In a real use case, there is 1 FF-A device (arm_ffa). However, when using sandbox there are 2 devices:

FF-A core device (arm_ffa)
FF-A sandbox device (sandbox_arm_ffa)

So, there is a good reason to use uclass_get_device_by_name() which garantees that we probe the right device.
In sandbox mode, the sandbox FF-A device should be probed first since it is responsible of discovering and binding the FF-A core device.
This is as close as possible to what PSCI driver does.

4/ In a real use case, we only need to use the PSCI device tree node as parent. In sandbox mode, we need to use the FF-A sandbox device tree node as parent.
5/ I'm happy to update the core driver operations as you suggested, thanks for the comments.
6/ I'm happy to rename the devices to arm-ffa and sandbox-arm-ffa

[1]: https://github.com/u-boot/u-boot/blob/master/drivers/firmware/psci.c#L158

Cheers
Abdellatif

> > +       if (ret) {
> > +               pr_err("%s was not bound: %d, aborting\n", FFA_DRV_NAME, ret);
> > +               return -ENODEV;
> > +       }
> > +
> > +       dev_set_parent_plat(child_dev, dev_get_plat(dev));
> > +
> > +       return 0;
> > +}
> > +
> > +static const struct udevice_id sandbox_ffa_id[] = {
> > +       { "sandbox,arm_ffa", 0 },
> > +       { },
> > +};
> > +
> > +/* Declaring the sandbox_arm_ffa driver under UCLASS_FFA */
> > +U_BOOT_DRIVER(sandbox_arm_ffa) = {
> > +       .name           = FFA_SANDBOX_DRV_NAME,
> > +       .of_match = sandbox_ffa_id,
> > +       .id             = UCLASS_FFA,
> > +       .probe          = sandbox_ffa_probe,
> > +       .priv_auto      = sizeof(struct sandbox_ffa_priv),
> > +};
> > diff --git a/drivers/firmware/arm-ffa/sandbox_arm_ffa_priv.h b/drivers/firmware/arm-ffa/sandbox_arm_ffa_priv.h
> > new file mode 100644
> > index 0000000000..c35d80de16
> > --- /dev/null
> > +++ b/drivers/firmware/arm-ffa/sandbox_arm_ffa_priv.h
> > @@ -0,0 +1,129 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * Copyright 2022-2023 Arm Limited and/or its affiliates <open-source-office at arm.com>
> > + *
> > + * Authors:
> > + *   Abdellatif El Khlifi <abdellatif.elkhlifi at arm.com>
> > + */
> > +
> > +#ifndef __SANDBOX_ARM_FFA_PRV_H
> > +#define __SANDBOX_ARM_FFA_PRV_H
> > +
> > +#include <sandbox_arm_ffa.h>
> > +#include "arm_ffa_priv.h"
> > +
> > +/* This header is exclusively used by the Sandbox FF-A driver and sandbox tests */
> > +
> > +/* FF-A core driver name */
> > +#define FFA_SANDBOX_DRV_NAME "sandbox_arm_ffa"
> > +
> > +/* FF-A ABIs internal error codes (as defined by the spec) */
> > +
> > +#define FFA_ERR_STAT_NOT_SUPPORTED     -1
> > +#define FFA_ERR_STAT_INVALID_PARAMETERS        -2
> > +#define FFA_ERR_STAT_NO_MEMORY -3
> > +#define FFA_ERR_STAT_BUSY      -4
> > +#define FFA_ERR_STAT_DENIED    -6
> 
> It's a convention to put negative values in brackets just in case of
> operator-precedence issues. Or you could use an enum.
> 
> > +
> > +/* Non-secure physical FF-A instance */
> > +#define NS_PHYS_ENDPOINT_ID (0)
> > +
> > +#define GET_NS_PHYS_ENDPOINT_ID_MASK           GENMASK(31, 16)
> > +#define GET_NS_PHYS_ENDPOINT_ID(x)             \
> > +                       ((u16)(FIELD_GET(GET_NS_PHYS_ENDPOINT_ID_MASK, (x))))
> > +
> > +/* Helper macro for reading the destination partition ID */
> > +#define GET_DST_SP_ID_MASK             GENMASK(15, 0)
> > +#define GET_DST_SP_ID(x)               \
> > +                       ((u16)(FIELD_GET(GET_DST_SP_ID_MASK, (x))))
> > +
> > +/* Helper macro for setting the source partition ID */
> > +#define PREP_SRC_SP_ID_MASK            GENMASK(31, 16)
> > +#define PREP_SRC_SP_ID(x)              \
> > +                       (FIELD_PREP(PREP_SRC_SP_ID_MASK, (x)))
> > +
> > +/* Helper macro for setting the destination endpoint ID */
> > +#define PREP_NS_PHYS_ENDPOINT_ID_MASK          GENMASK(15, 0)
> > +#define PREP_NS_PHYS_ENDPOINT_ID(x)            \
> > +                       (FIELD_PREP(PREP_NS_PHYS_ENDPOINT_ID_MASK, (x)))
> > +
> > +/*  RX/TX buffers minimum size */
> > +#define RXTX_BUFFERS_MIN_SIZE (RXTX_4K)
> > +#define RXTX_BUFFERS_MIN_PAGES (1)
> > +
> > +/* MBZ registers info */
> > +
> > +/* x1-x7 MBZ */
> > +#define FFA_X1X7_MBZ_CNT (7)
> > +#define FFA_X1X7_MBZ_REG_START (&res->a1)
> > +
> > +/* x4-x7 MBZ */
> > +#define FFA_X4X7_MBZ_CNT (4)
> > +#define FFA_X4X7_MBZ_REG_START (&res->a4)
> > +
> > +/* x3-x7 MBZ */
> > +#define FFA_X3X7_MBZ_CNT (5)
> > +#define FFA_X3_MBZ_REG_START (&res->a3)
> > +
> > +/* number of secure partitions emulated by the FF-A sandbox driver */
> > +#define SANDBOX_PARTITIONS_CNT (4)
> > +
> > +/* Binary data of services UUIDs emulated by the FF-A sandbox driver */
> > +
> > +/* service 1  UUID binary data (little-endian format) */
> > +#define SANDBOX_SERVICE1_UUID_A1       0xed32d533
> > +#define SANDBOX_SERVICE1_UUID_A2       0x99e64209
> > +#define SANDBOX_SERVICE1_UUID_A3       0x9cc02d72
> > +#define SANDBOX_SERVICE1_UUID_A4       0xcdd998a7
> > +
> > +/* service 2  UUID binary data (little-endian format) */
> > +#define SANDBOX_SERVICE2_UUID_A1       0xed32d544
> > +#define SANDBOX_SERVICE2_UUID_A2       0x99e64209
> > +#define SANDBOX_SERVICE2_UUID_A3       0x9cc02d72
> > +#define SANDBOX_SERVICE2_UUID_A4       0xcdd998a7
> > +
> > +/**
> > + * struct ffa_rxtxpair_info - structure hosting the RX/TX buffers flags
> > + * @rxbuf_owned:       RX buffer ownership flag (the owner is non secure world: the consumer)
> > + * @rxbuf_mapped:      RX buffer mapping flag
> > + * @txbuf_owned        TX buffer ownership flag
> > + * @txbuf_mapped:      TX buffer mapping flag
> > + * @rxtx_buf_size:     RX/TX buffers size as set by the FF-A core driver
> > + *
> > + * Data structure hosting the ownership/mapping flags of the RX/TX buffers
> > + * When a buffer is owned/mapped its corresponding flag is set to 1 otherwise 0.
> > + */
> > +struct ffa_rxtxpair_info {
> > +       u8 rxbuf_owned;
> > +       u8 rxbuf_mapped;
> > +       u8 txbuf_owned;
> > +       u8 txbuf_mapped;
> > +       u32 rxtx_buf_size;
> > +};
> > +
> > +/**
> > + * struct sandbox_ffa_priv - the driver private data structure
> > + *
> > + * @dev:       The arm_ffa device under u-boot driver model
> > + * @fwk_version:       FF-A framework version
> > + * @id:        u-boot endpoint ID
> > + * @partitions:        The partitions descriptors structure
> > + * @pair:      The RX/TX buffers pair
> > + * @pair_info: The RX/TX buffers pair flags and size
> > + * @conduit:   The selected conduit
> > + *
> > + * The driver data structure hosting all the emulated secure world data.
> > + */
> > +struct sandbox_ffa_priv {
> > +       struct udevice *dev;
> > +       u32 fwk_version;
> > +       u16 id;
> > +       struct ffa_partitions partitions;
> > +       struct ffa_rxtxpair pair;
> > +       struct ffa_rxtxpair_info pair_info;
> > +};
> > +
> > +#define SANDBOX_SMC_FFA_ABI(ffabi) static int sandbox_##ffabi(struct udevice *dev, \
> > +                                                             ffa_value_t *pargs, ffa_value_t *res)
> 
> drop that
> 
> [..]
> 
> Regards,
> Simon


More information about the U-Boot mailing list