Re: [PATCH v3 1/3] ArmPkg: Add ARM SMC Architecture functions to ArmStdSmc.h

["Leif Lindholm" <leif@nuviainc.com>]

On Tue, Nov 10, 2020 at 17:17:46 -0700, Rebecca Cran wrote:
> The ARM SMC Architecture functions were missing from ArmStdSmc.h.
> Add them, based on the SMC Calling Convention version 1.2 specification.
> 
> Signed-off-by: Rebecca Cran <rebecca@nuviainc.com>
> ---
>  ArmPkg/Include/IndustryStandard/ArmStdSmc.h | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/ArmPkg/Include/IndustryStandard/ArmStdSmc.h b/ArmPkg/Include/IndustryStandard/ArmStdSmc.h
> index 3509eb680f18..90f32c2249cd 100644
> --- a/ArmPkg/Include/IndustryStandard/ArmStdSmc.h
> +++ b/ArmPkg/Include/IndustryStandard/ArmStdSmc.h
> @@ -1,5 +1,6 @@
>  /** @file
>  *
> +*  Copyright (c) 2020, NUVIA Inc. All rights reserved.
>  *  Copyright (c) 2012-2017, ARM Limited. All rights reserved.
>  *
>  *  SPDX-License-Identifier: BSD-2-Clause-Patent
> @@ -52,6 +53,18 @@
>  #define ARM_SMC_MM_RET_DENIED              -3
>  #define ARM_SMC_MM_RET_NO_MEMORY           -4
>  
> +// ARM Architecture Calls
> +#define ARM_SMC_ID_ARCH_VERSION      0x80000000

This one does not actually contain "ARCH" in the spec, and it
references SMCCC version speficically.

> +#define ARM_SMC_ID_ARCH_FEATURES     0x80000001
> +#define ARM_SMC_ID_ARCH_SOC_ID       0x80000002
> +#define ARM_SMC_ID_ARCH_WORKAROUND_1 0x80008000
> +#define ARM_SMC_ID_ARCH_WORKAROUND_2 0x80007FFF

TF-A defines this whole block as:

#define SMCCC_VERSION                   U(0x80000000)
#define SMCCC_ARCH_FEATURES             U(0x80000001)
#define SMCCC_ARCH_SOC_ID               U(0x80000002)
#define SMCCC_ARCH_WORKAROUND_1         U(0x80008000)
#define SMCCC_ARCH_WORKAROUND_2         U(0x80007FFF)

I think I prefer these shorter versions, which also addresses the
abovementioned comment. (Not the U() bit, that's TF-A idiom.)

> +#define ARM_SMC_ARCH_RET_SUCCESS            0
> +#define ARM_SMC_ARCH_RET_NOT_SUPPORTED     -1
> +#define ARM_SMC_ARCH_RET_NOT_REQUIRED      -2
> +#define ARM_SMC_ARCH_RET_INVALID_PARAMETER -3

And these as:

#define SMC_ARCH_CALL_SUCCESS           0
#define SMC_ARCH_CALL_NOT_SUPPORTED     -1
#define SMC_ARCH_CALL_NOT_REQUIRED      -2
#define SMC_ARCH_CALL_INVAL_PARAM       -3

Not sure I prefer these, but I think we cold drop the ARM_ prefix.

/
    Leif


> +
>  /*
>   * Power State Coordination Interface (PSCI) calls cover a subset of the
>   * Standard Service Call range.
> -- 
> 2.26.2
>