[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