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

[Chris Johns]

On 4/7/2022 4:06 pm, Sebastian Huber wrote:
> On 04/07/2022 03:43, Chris Johns wrote:
>> 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?
> 
> I also thought about using a general define for this, however, the ICC_BPR0 and
> ICC_BPR1 have processor- and application specific values.

This maybe the case for R class devices. A and M should or will use TF-[A/M] and
I have no idea what boot stack is present for R so I cannot comment.

> My plan was to add BSP options for these registers in a follow up patch.

I see the spec support in this patch more of an issue than the exact define in
the code here. Others look and then copy what they see and per reg options is
rarely the best approach.

What about a BSP or class specific call to allow localised initialisation and
then we can avoid these defines? Maybe the BSP should set this up before
starting the system? I do not know which path is the best one.

Chris