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

Tue Jul 5 05:53:16 UTC 2022

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.

-- 
embedded brains GmbH
Herr Sebastian HUBER
Dornierstr. 4
82178 Puchheim
Germany
email: sebastian.huber at embedded-brains.de
phone: +49-89-18 94 741 - 16
fax:   +49-89-18 94 741 - 08

Registergericht: Amtsgericht München
Registernummer: HRB 157899
Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler
Unsere Datenschutzerklärung finden Sie hier:
https://embedded-brains.de/datenschutzerklaerung/