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. > _______________________________________________ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel