Re: [PATCH v3 1/6] xen/arm: firmware: Add SCMI over SMC calls handling layer

[Michal Orzel <michal.orzel@xxxxxxx>]

On 18/12/2024 11:11, Andrei Cherechesu (OSS) wrote:
> 
> 
> From: Andrei Cherechesu <andrei.cherechesu@xxxxxxx>
> 
> Introduce the SCMI-SMC layer to have some basic degree of
> awareness about SCMI calls that are based on the ARM System
> Control and Management Interface (SCMI) specification (DEN0056E).
> 
> The SCMI specification includes various protocols for managing
> system-level resources, such as: clocks, pins, reset, system power,
> power domains, performance domains, etc. The clients are named
> "SCMI agents" and the server is named "SCMI platform".
> 
> Only support the shared-memory based transport with SMCs as
> the doorbell mechanism for notifying the platform. Also, this
> implementation only handles the "arm,scmi-smc" compatible,
> requiring the following properties:
>         - "arm,smc-id" (unique SMC ID)
>         - "shmem" (one or more phandles pointing to shmem zones
>         for each channel)
> 
> The initialization is done as initcall, since we need
> SMCs, and PSCI should already probe EL3 FW for SMCCC support.
> If no "arm,scmi-smc" compatible node is found in the host
> DT, the initialization fails silently, as it's not mandatory.
> Otherwise, we get the 'arm,smc-id' DT property from the node,
> to know the SCMI SMC ID we handle. The 'shmem' memory ranges
> are not validated, as the SMC calls are only passed through
> to EL3 FW if coming from the hardware domain.
> 
> Create a new 'firmware' folder to keep the SCMI code separate
> from the generic ARM code.
> 
> Signed-off-by: Andrei Cherechesu <andrei.cherechesu@xxxxxxx>
> Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> ---
>  xen/arch/arm/Kconfig                         |   2 +
>  xen/arch/arm/Makefile                        |   1 +
>  xen/arch/arm/firmware/Kconfig                |  13 ++
>  xen/arch/arm/firmware/Makefile               |   1 +
>  xen/arch/arm/firmware/scmi-smc.c             | 166 +++++++++++++++++++
>  xen/arch/arm/include/asm/firmware/scmi-smc.h |  46 +++++
>  6 files changed, 229 insertions(+)
>  create mode 100644 xen/arch/arm/firmware/Kconfig
>  create mode 100644 xen/arch/arm/firmware/Makefile
>  create mode 100644 xen/arch/arm/firmware/scmi-smc.c
>  create mode 100644 xen/arch/arm/include/asm/firmware/scmi-smc.h
> 
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index 604aba4996..23dc7162a7 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -271,6 +271,8 @@ config PARTIAL_EMULATION
>           not been emulated to their complete functionality. Enabling this 
> might
>           result in unwanted/non-spec compliant behavior.
> 
> +source "arch/arm/firmware/Kconfig"
> +
>  endmenu
> 
>  menu "ARM errata workaround via the alternative framework"
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index e4ad1ce851..8c696c2011 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -6,6 +6,7 @@ obj-$(CONFIG_HAS_PCI) += pci/
>  ifneq ($(CONFIG_NO_PLAT),y)
>  obj-y += platforms/
>  endif
> +obj-y += firmware/
>  obj-$(CONFIG_TEE) += tee/
>  obj-$(CONFIG_HAS_VPCI) += vpci.o
> 
> diff --git a/xen/arch/arm/firmware/Kconfig b/xen/arch/arm/firmware/Kconfig
> new file mode 100644
> index 0000000000..817da745fd
> --- /dev/null
> +++ b/xen/arch/arm/firmware/Kconfig
> @@ -0,0 +1,13 @@
> +menu "Firmware Drivers"
> +
> +config SCMI_SMC
> +       bool "Forward SCMI over SMC calls from hwdom to EL3 firmware"
> +       default y
> +       help
> +         This option enables basic awareness for SCMI calls using SMC as
> +         doorbell mechanism and Shared Memory for transport ("arm,scmi-smc"
> +         compatible only). The value of "arm,smc-id" DT property from SCMI
> +         firmware node is used to trap and forward corresponding SCMI SMCs
> +         to firmware running at EL3, for calls coming from the hardware 
> domain.
> +
> +endmenu
> diff --git a/xen/arch/arm/firmware/Makefile b/xen/arch/arm/firmware/Makefile
> new file mode 100644
> index 0000000000..a5e4542666
> --- /dev/null
> +++ b/xen/arch/arm/firmware/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_SCMI_SMC) += scmi-smc.o
> diff --git a/xen/arch/arm/firmware/scmi-smc.c 
> b/xen/arch/arm/firmware/scmi-smc.c
> new file mode 100644
> index 0000000000..62657308d6
> --- /dev/null
> +++ b/xen/arch/arm/firmware/scmi-smc.c
> @@ -0,0 +1,166 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * xen/arch/arm/firmware/scmi-smc.c
> + *
> + * ARM System Control and Management Interface (SCMI) over SMC
> + * Generic handling layer
> + *
> + * Andrei Cherechesu <andrei.cherechesu@xxxxxxx>
> + * Copyright 2024 NXP
> + */
> +
> +#include <xen/acpi.h>
> +#include <xen/device_tree.h>
> +#include <xen/errno.h>
> +#include <xen/init.h>
> +#include <xen/sched.h>
> +#include <xen/types.h>
> +
> +#include <asm/smccc.h>
> +#include <asm/firmware/scmi-smc.h>
> +
> +#define SCMI_SMC_ID_PROP   "arm,smc-id"
> +
> +static bool __ro_after_init scmi_support;
I don't understand the need for this variable. IMO it's useless, given that in 
next patch you do:

...
if ( scmi_is_enabled() )
    return scmi_handle_smc(regs);

return false;

which could simply be changed to:
...
return scmi_handle_smc(regs);

and the variable, functions for it, etc. could be removed which would simplify 
the code.

[...]

> +err:
CODING_STYLE: this should be indented with one space.

~Michal