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

Chris Johns chrisj at rtems.org
Fri Jul 8 07:55:37 UTC 2022


Hi,

I have finally managed to test this patch and it does not work on a
secure/non-secure Versal. I did a `distclean configure` and rebuild so it would
pick up the option changes.

The `BSP_ARM_GIC_ICC_IGRPEN0 write is still in the build and it is a secure access:

  /* Initialize the group 0 and 1 interrupt enable */
#ifdef BSP_ARM_GIC_ICC_IGRPEN0
  WRITE_SR(ICC_IGRPEN0, BSP_ARM_GIC_ICC_IGRPEN0);
    103e7e5c:   52800020        mov     w0, #0x1                        // #1
    103e7e60:   d518ccc0        msr     icc_igrpen0_el1, x0
#endif

I tested this piece of code and it is OK:

#ifdef BSP_ARM_GIC_ICC_IGRPEN1
  WRITE_SR(ICC_IGRPEN1, BSP_ARM_GIC_ICC_IGRPEN1);
    103e7e64:   d518cce0        msr     icc_igrpen1_el1, x0
#endif

Why is this register conditional because I think it is always needed?

I think the default for BSP_ARM_GIC_ICC_IGRPEN0 should be disabled.

Thanks
Chris

On 5/7/2022 3:53 pm, Sebastian Huber wrote:
> On 05/07/2022 00:28, Chris Johns wrote:
>> 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.
> 
> The Cortex-M usually have another interrupt controller.
> 
>>
>>> 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.
> 
> The real problem are people doing their first BSP development in isolation.
> There is plenty of bad code to copy from.
> 
>>
>> 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.
> 
> We just have to set up four registers in the GIC CPU Interface (the group enable
> and the binary point). I don't think we need another layer of complexity. The
> GICv3 support is added to a BSP via:
> 
> +- role: build-dependency
> +  uid: ../../objarmgic
> 
> Ok, this should be objarmgicv3.
> 


More information about the devel mailing list