[PATCH v2] irq/arm-gicv3.h: Customize ICC_IGRPEN0/1 init

[Chris Johns]

On 1/7/2022 11:21 pm, Sebastian Huber wrote:
> Use the existing WRITE_SR() abstraction to access the interrupt group 0 and 1
> enable registers.  This fixes the build for the AArch32 target.
> ---
>  bsps/include/dev/irq/arm-gicv3.h              | 30 ++++++++-----------
>  spec/build/bsps/aarch64/a53/grp.yml           |  2 ++
>  spec/build/bsps/aarch64/a53/obj.yml           |  1 -
>  spec/build/bsps/aarch64/a72/grp.yml           |  2 ++
>  spec/build/bsps/aarch64/a72/obj.yml           |  1 -
>  spec/build/bsps/aarch64/grp.yml               |  1 -
>  spec/build/bsps/aarch64/xilinx-versal/grp.yml |  2 ++
>  spec/build/bsps/aarch64/xilinx-versal/obj.yml |  1 -
>  spec/build/bsps/arm/fvp/grp.yml               |  2 ++
>  spec/build/bsps/arm/fvp/obj.yml               |  1 -
>  spec/build/bsps/arm/grp.yml                   |  1 -
>  spec/build/bsps/objarmgic.yml                 | 21 +++++++++++++
>  spec/build/bsps/optarmgic-icc-igrpen0.yml     | 20 +++++++++++++
>  spec/build/bsps/optarmgic-icc-igrpen1.yml     | 17 +++++++++++
>  14 files changed, 78 insertions(+), 24 deletions(-)
>  create mode 100644 spec/build/bsps/objarmgic.yml
>  create mode 100644 spec/build/bsps/optarmgic-icc-igrpen0.yml
>  create mode 100644 spec/build/bsps/optarmgic-icc-igrpen1.yml
> 
> diff --git a/bsps/include/dev/irq/arm-gicv3.h b/bsps/include/dev/irq/arm-gicv3.h
> index a79368ebdf..4cd8cfaaed 100644
> --- a/bsps/include/dev/irq/arm-gicv3.h
> +++ b/bsps/include/dev/irq/arm-gicv3.h
> @@ -116,9 +116,11 @@ extern "C" {
>  #else /* ARM_MULTILIB_ARCH_V4 */
>  
>  /* AArch64 GICv3 registers are not named in GCC */
> -#define ICC_IGRPEN0 "S3_0_C12_C12_6, %0"
> -#define ICC_IGRPEN1 "S3_0_C12_C12_7, %0"
> +#define ICC_IGRPEN0_EL1 "S3_0_C12_C12_6, %0"
> +#define ICC_IGRPEN1_EL1 "S3_0_C12_C12_7, %0"
>  #define ICC_IGRPEN1_EL3 "S3_6_C12_C12_7, %0"
> +#define ICC_IGRPEN0 ICC_IGRPEN0_EL1
> +#define ICC_IGRPEN1 ICC_IGRPEN1_EL1
>  #define ICC_PMR     "S3_0_C4_C6_0, %0"
>  #define ICC_EOIR1   "S3_0_C12_C12_1, %0"
>  #define ICC_SRE     "S3_0_C12_C12_5, %0"
> @@ -300,20 +302,6 @@ static void gicv3_init_dist(volatile gic_dist *dist)
>    }
>  }
>  
> -/*
> - * A better way to access these registers than special opcodes
> - */
> -#define isb() __asm __volatile("isb" : : : "memory")
> -
> -#define  WRITE_SPECIALREG(reg, _val)                                    \
> -  __asm __volatile("msr  " __STRING(reg) ", %0" : : "r"((uint64_t)_val))
> -
> -#define  gic_icc_write(reg, val)            \
> -do {                                        \
> -  WRITE_SPECIALREG(icc_ ##reg ##_el1, val); \
> -  isb();                                    \
> -} while (0)
> -
>  static void gicv3_init_cpu_interface(uint32_t cpu_index)
>  {
>    uint32_t sre_value = 0x7;
> @@ -334,8 +322,14 @@ static void gicv3_init_cpu_interface(uint32_t cpu_index)
>      sgi_ppi->icspiprior[id] = PRIORITY_DEFAULT;
>    }
>  
> -  /* Enable interrupt groups 0 and 1 */
> -  gic_icc_write(IGRPEN1, 1);
> +  /* Initialize the group 0 and 1 interrupt enable */
> +#ifdef BSP_ARM_GIC_ICC_IGRPEN0
> +  WRITE_SR(ICC_IGRPEN0, BSP_ARM_GIC_ICC_IGRPEN0);
> +#endif

I think more things will surface in this driver over time and configuring at the
per register level may become unwieldy. This write depends on the EL levels
available, secure/non-secure support and the boot wrapper used for the different
classes of devices. Maybe the config support and macro reflecting this would
serve us better over time?

> +#ifdef BSP_ARM_GIC_ICC_IGRPEN1
> +  WRITE_SR(ICC_IGRPEN1, BSP_ARM_GIC_ICC_IGRPEN1);
> +#endif

I am yet to see if this works on a secure/non-secure Arvm8-A at EL1. The
hardware is being used on another task and I need access to it. My review of the
code looks OK but I would like to make sure first.

Chris