When GIC_4KNOT64K bit in the GIC configuration register is
0 (64KB), address block is modified in such a way than only the
first 4KB of the GIC cpu interface are accessible with default
offsets.
With this bit mapping GICC_DIR register is accessible at
offset 0x10000 instead of 0x1000, thus remap accordingly
Use st,stm32mp2-cortex-a7-gic for this purpose.
Signed-off-by: Christian Bruel <christian.bruel@foss.st.com>
---
drivers/irqchip/irq-gic.c | 47 ++++++++++++++++++++++++++++++++++++++-
1 file changed, 46 insertions(+), 1 deletion(-)
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 6503573557fd..d61dcd0eb4c6 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -72,6 +72,7 @@ struct gic_chip_data {
union gic_base cpu_base;
void __iomem *raw_dist_base;
void __iomem *raw_cpu_base;
+ phys_addr_t cpu_phys_base;
u32 percpu_offset;
#if defined(CONFIG_CPU_PM) || defined(CONFIG_ARM_GIC_PM)
u32 saved_spi_enable[DIV_ROUND_UP(1020, 32)];
@@ -108,6 +109,8 @@ static DEFINE_RAW_SPINLOCK(cpu_map_lock);
#endif
+static DEFINE_STATIC_KEY_FALSE(gic_stm32mp2_gicc_dir_access);
+
static DEFINE_STATIC_KEY_FALSE(needs_rmw_access);
/*
@@ -225,6 +228,8 @@ static void gic_eoi_irq(struct irq_data *d)
writel_relaxed(hwirq, gic_cpu_base(d) + GIC_CPU_EOI);
}
+#define GIC_STM32MP2_CPU_DEACTIVATE 0x10000
+
static void gic_eoimode1_eoi_irq(struct irq_data *d)
{
irq_hw_number_t hwirq = irqd_to_hwirq(d);
@@ -236,7 +241,10 @@ static void gic_eoimode1_eoi_irq(struct irq_data *d)
if (hwirq < 16)
hwirq = this_cpu_read(sgi_intid);
- writel_relaxed(hwirq, gic_cpu_base(d) + GIC_CPU_DEACTIVATE);
+ if (static_branch_unlikely(&gic_stm32mp2_gicc_dir_access))
+ writel_relaxed(hwirq, gic_cpu_base(d) + GIC_STM32MP2_CPU_DEACTIVATE);
+ else
+ writel_relaxed(hwirq, gic_cpu_base(d) + GIC_CPU_DEACTIVATE);
}
static int gic_irq_set_irqchip_state(struct irq_data *d,
@@ -1377,17 +1385,50 @@ static bool gic_enable_rmw_access(void *data)
return false;
}
+/*
+ * 8kB GICC range is not accessible with the default 4kB translation
+ * granule. 0x1000 offset is accessible at 64kB translation.
+ */
+static bool gic_8kbaccess(void *data)
+{
+ struct gic_chip_data *gic = data;
+ void __iomem *alt;
+
+ if (!is_hyp_mode_available())
+ return false;
+
+ alt = ioremap(gic->cpu_phys_base, GIC_STM32MP2_CPU_DEACTIVATE + 4);
+ if (!alt) {
+ pr_err("Unable to remap GICC_DIR register\n");
+ return false;
+ }
+
+ iounmap(gic->raw_cpu_base);
+ gic->raw_cpu_base = alt;
+
+ static_branch_enable(&gic_stm32mp2_gicc_dir_access);
+
+ return true;
+}
+
static const struct gic_quirk gic_quirks[] = {
{
.desc = "broken byte access",
.compatible = "arm,pl390",
.init = gic_enable_rmw_access,
},
+ {
+ .desc = "4kB GICC access disabled",
+ .compatible = "st,stm32mp2-cortex-a7-gic",
+ .init = gic_8kbaccess,
+ },
{ },
};
static int gic_of_setup(struct gic_chip_data *gic, struct device_node *node)
{
+ struct resource cpuif_res;
+
if (!gic || !node)
return -EINVAL;
@@ -1395,6 +1436,8 @@ static int gic_of_setup(struct gic_chip_data *gic, struct device_node *node)
if (WARN(!gic->raw_dist_base, "unable to map gic dist registers\n"))
goto error;
+ of_address_to_resource(node, 1, &cpuif_res);
+ gic->cpu_phys_base = cpuif_res.start;
gic->raw_cpu_base = of_iomap(node, 1);
if (WARN(!gic->raw_cpu_base, "unable to map gic cpu registers\n"))
goto error;
@@ -1510,6 +1553,7 @@ gic_of_init(struct device_node *node, struct device_node *parent)
gic_cnt++;
return 0;
}
+
IRQCHIP_DECLARE(gic_400, "arm,gic-400", gic_of_init);
IRQCHIP_DECLARE(arm11mp_gic, "arm,arm11mp-gic", gic_of_init);
IRQCHIP_DECLARE(arm1176jzf_dc_gic, "arm,arm1176jzf-devchip-gic", gic_of_init);
@@ -1519,6 +1563,7 @@ IRQCHIP_DECLARE(cortex_a7_gic, "arm,cortex-a7-gic", gic_of_init);
IRQCHIP_DECLARE(msm_8660_qgic, "qcom,msm-8660-qgic", gic_of_init);
IRQCHIP_DECLARE(msm_qgic2, "qcom,msm-qgic2", gic_of_init);
IRQCHIP_DECLARE(pl390, "arm,pl390", gic_of_init);
+IRQCHIP_DECLARE(stm32mp2_cortex_a7_gic, "st,stm32mp2-cortex-a7-gic", gic_of_init);
#ifdef CONFIG_ACPI
static struct
--
2.34.1On Thu, 03 Apr 2025 13:28:04 +0100,
Christian Bruel <christian.bruel@foss.st.com> wrote:
>
> When GIC_4KNOT64K bit in the GIC configuration register is
> 0 (64KB), address block is modified in such a way than only the
> first 4KB of the GIC cpu interface are accessible with default
> offsets.
> With this bit mapping GICC_DIR register is accessible at
> offset 0x10000 instead of 0x1000, thus remap accordingly
And I'm pretty sure the whole of the GICC range is correctly
accessible at offset 0xF000, giving you the full 8kB you need. That's
because each page of the GIC is aliased over two 64kB blocks, as per
the integration guidelines so that MMU isolation can be provided on a
64kB boundary.
Funnily enough, all it takes is to adjust GICC region. You can either:
- make it 128kB wide, and the driver will take care of it (details in
gic_check_eoimode()). On one of my boxes that is similarly
configured, I get:
[ 0.000000] NR_IRQS: 64, nr_irqs: 64, preallocated irqs: 0
[ 0.000000] GIC: Adjusting CPU interface base to 0x00000000780af000
[ 0.000000] Root IRQ handler: gic_handle_irq
[ 0.000000] GIC: Using split EOI/Deactivate mode
See below for what I expect to be the correct fix.
- make it 8kB wide from offset 0xF000.
Unless the ST HW folks have been even more creative, none of this
overly complicated stuff should be necessary. Just describe the HW
correctly.
M.
diff --git a/arch/arm64/boot/dts/st/stm32mp251.dtsi b/arch/arm64/boot/dts/st/stm32mp251.dtsi
index f3c6cdfd7008..97b7a7106a02 100644
--- a/arch/arm64/boot/dts/st/stm32mp251.dtsi
+++ b/arch/arm64/boot/dts/st/stm32mp251.dtsi
@@ -120,7 +120,7 @@ intc: interrupt-controller@4ac00000 {
#address-cells = <1>;
interrupt-controller;
reg = <0x0 0x4ac10000 0x0 0x1000>,
- <0x0 0x4ac20000 0x0 0x2000>,
+ <0x0 0x4ac20000 0x0 0x20000>,
<0x0 0x4ac40000 0x0 0x2000>,
<0x0 0x4ac60000 0x0 0x2000>;
};
--
Jazz isn't dead. It just smells funny.
On 4/3/25 19:50, Marc Zyngier wrote:
> On Thu, 03 Apr 2025 13:28:04 +0100,
> Christian Bruel <christian.bruel@foss.st.com> wrote:
>>
>> When GIC_4KNOT64K bit in the GIC configuration register is
>> 0 (64KB), address block is modified in such a way than only the
>> first 4KB of the GIC cpu interface are accessible with default
>> offsets.
>> With this bit mapping GICC_DIR register is accessible at
>> offset 0x10000 instead of 0x1000, thus remap accordingly
>
> And I'm pretty sure the whole of the GICC range is correctly
> accessible at offset 0xF000, giving you the full 8kB you need. That's
> because each page of the GIC is aliased over two 64kB blocks, as per
> the integration guidelines so that MMU isolation can be provided on a
> 64kB boundary.
Thanks a lot for this explanation, indeed this works like a charm.
>
> Funnily enough, all it takes is to adjust GICC region. You can either:
>
> - make it 128kB wide, and the driver will take care of it (details in
> gic_check_eoimode()). On one of my boxes that is similarly
> configured, I get:
>
> [ 0.000000] NR_IRQS: 64, nr_irqs: 64, preallocated irqs: 0
> [ 0.000000] GIC: Adjusting CPU interface base to 0x00000000780af000
> [ 0.000000] Root IRQ handler: gic_handle_irq
> [ 0.000000] GIC: Using split EOI/Deactivate mode
>
> See below for what I expect to be the correct fix.
>
> - make it 8kB wide from offset 0xF000.
I checked both and they work. I will go for the former to show real 8kB
size to be exposed in the DT. And there are a few other platforms that
use this alias
>
> Unless the ST HW folks have been even more creative, none of this
> overly complicated stuff should be necessary. Just describe the HW
> correctly.
I was unable to find this information in the GIC-400 trm
(https://developer.arm.com/documentation/ddi0471/b/programmers-model/gic-400-register-map).
Now I also prefer to use GICC alias at
offset 0xf000 as suggested rather than the quirk solution
thank you very much
Christian
>
> M.
>
> diff --git a/arch/arm64/boot/dts/st/stm32mp251.dtsi b/arch/arm64/boot/dts/st/stm32mp251.dtsi
> index f3c6cdfd7008..97b7a7106a02 100644
> --- a/arch/arm64/boot/dts/st/stm32mp251.dtsi
> +++ b/arch/arm64/boot/dts/st/stm32mp251.dtsi
> @@ -120,7 +120,7 @@ intc: interrupt-controller@4ac00000 {
> #address-cells = <1>;
> interrupt-controller;
> reg = <0x0 0x4ac10000 0x0 0x1000>,
> - <0x0 0x4ac20000 0x0 0x2000>,
> + <0x0 0x4ac20000 0x0 0x20000>,
> <0x0 0x4ac40000 0x0 0x2000>,
> <0x0 0x4ac60000 0x0 0x2000>;
> };
>
On Fri, 04 Apr 2025 13:15:05 +0100, Christian Bruel <christian.bruel@foss.st.com> wrote: > > > > On 4/3/25 19:50, Marc Zyngier wrote: > > On Thu, 03 Apr 2025 13:28:04 +0100, > > Christian Bruel <christian.bruel@foss.st.com> wrote: > >> > >> When GIC_4KNOT64K bit in the GIC configuration register is > >> 0 (64KB), address block is modified in such a way than only the > >> first 4KB of the GIC cpu interface are accessible with default > >> offsets. > >> With this bit mapping GICC_DIR register is accessible at > >> offset 0x10000 instead of 0x1000, thus remap accordingly > > > > And I'm pretty sure the whole of the GICC range is correctly > > accessible at offset 0xF000, giving you the full 8kB you need. That's > > because each page of the GIC is aliased over two 64kB blocks, as per > > the integration guidelines so that MMU isolation can be provided on a > > 64kB boundary. > > Thanks a lot for this explanation, indeed this works like a charm. > > > > > Funnily enough, all it takes is to adjust GICC region. You can either: > > > > - make it 128kB wide, and the driver will take care of it (details in > > gic_check_eoimode()). On one of my boxes that is similarly > > configured, I get: > > > > [ 0.000000] NR_IRQS: 64, nr_irqs: 64, preallocated irqs: 0 > > [ 0.000000] GIC: Adjusting CPU interface base to 0x00000000780af000 > > [ 0.000000] Root IRQ handler: gic_handle_irq > > [ 0.000000] GIC: Using split EOI/Deactivate mode > > > > See below for what I expect to be the correct fix. > > - make it 8kB wide from offset 0xF000. > > I checked both and they work. I will go for the former to show real > 8kB size to be exposed in the DT. And there are a few other > platforms that use this alias I think 8kB the wrong option. The GIC *is* supposed to be integrated over 128kB on arm64 platforms (there was some documentation about that back in the days, but it has become impossible to search anything on ARM's stupidly broken website. My recollection is that it was bundled with the GICv2m "specification" (only half a page!). Furthermore, you are supposed to describe the HW. Not your interpretation of it. Correctly written SW targeting arm64 know about this anyway. > > Unless the ST HW folks have been even more creative, none of this > > overly complicated stuff should be necessary. Just describe the HW > > correctly. > > I was unable to find this information in the GIC-400 trm > (https://developer.arm.com/documentation/ddi0471/b/programmers-model/gic-400-register-map). Now > I also prefer to use GICC alias at > offset 0xf000 as suggested rather than the quirk solution Again, this isn't a quirk. It's the one true way for 64bit platforms that can use pages bigger than 4kB. That's the purpose of the 4Kn64K parameter in the integration, dropping bits [15:12] from the PA presented to the CPU interface. M. -- Jazz isn't dead. It just smells funny.
On 4/4/25 15:36, Marc Zyngier wrote: > On Fri, 04 Apr 2025 13:15:05 +0100, > Christian Bruel <christian.bruel@foss.st.com> wrote: >> >> >> >> On 4/3/25 19:50, Marc Zyngier wrote: >>> On Thu, 03 Apr 2025 13:28:04 +0100, >>> Christian Bruel <christian.bruel@foss.st.com> wrote: >>>> >>>> When GIC_4KNOT64K bit in the GIC configuration register is >>>> 0 (64KB), address block is modified in such a way than only the >>>> first 4KB of the GIC cpu interface are accessible with default >>>> offsets. >>>> With this bit mapping GICC_DIR register is accessible at >>>> offset 0x10000 instead of 0x1000, thus remap accordingly >>> >>> And I'm pretty sure the whole of the GICC range is correctly >>> accessible at offset 0xF000, giving you the full 8kB you need. That's >>> because each page of the GIC is aliased over two 64kB blocks, as per >>> the integration guidelines so that MMU isolation can be provided on a >>> 64kB boundary. >> >> Thanks a lot for this explanation, indeed this works like a charm. >> >>> >>> Funnily enough, all it takes is to adjust GICC region. You can either: >>> >>> - make it 128kB wide, and the driver will take care of it (details in >>> gic_check_eoimode()). On one of my boxes that is similarly >>> configured, I get: >>> >>> [ 0.000000] NR_IRQS: 64, nr_irqs: 64, preallocated irqs: 0 >>> [ 0.000000] GIC: Adjusting CPU interface base to 0x00000000780af000 >>> [ 0.000000] Root IRQ handler: gic_handle_irq >>> [ 0.000000] GIC: Using split EOI/Deactivate mode >>> >>> See below for what I expect to be the correct fix. >>> - make it 8kB wide from offset 0xF000. >> >> I checked both and they work. I will go for the former to show real >> 8kB size to be exposed in the DT. And there are a few other >> platforms that use this alias > > I think 8kB the wrong option. The GIC *is* supposed to be integrated > over 128kB on arm64 platforms (there was some documentation about that > back in the days, but it has become impossible to search anything on > ARM's stupidly broken website. My recollection is that it was bundled > with the GICv2m "specification" (only half a page!). > > Furthermore, you are supposed to describe the HW. Not your > interpretation of it. Correctly written SW targeting arm64 know about > this anyway. greping other platforms there are a bunch 0xf000 offset 8KB mapped: amd/amd-seattle-soc.dtsi arm/corstone1000.dtsi arm/foundation-v8-gicv3.dtsi arm/juno-base.dtsi mediatek/mt8516.dtsi but, looking at the stm32mp25 memory map (1) page 239: 0x4AC22000 - 0x4AC3FFFF 120 Reserved - 0x4AC20000 - 0x4AC21FFF 8 GICC I can know guess that the "Reserved" 120kB is for aliasing the 64kB blocks. Thus describing the GICC 128KB range size makes sense similarly 4KB + 120KB Reserved for GICH and 8KB + 120KB Reserved for GICV (1) https://www.st.com/resource/en/reference_manual/rm0457-stm32mp25xx-advanced-armbased-3264bit-mpus-stmicroelectronics.pdf > >>> Unless the ST HW folks have been even more creative, none of this >>> overly complicated stuff should be necessary. Just describe the HW >>> correctly. >> >> I was unable to find this information in the GIC-400 trm >> (https://developer.arm.com/documentation/ddi0471/b/programmers-model/gic-400-register-map). Now >> I also prefer to use GICC alias at >> offset 0xf000 as suggested rather than the quirk solution > > Again, this isn't a quirk. It's the one true way for 64bit platforms > that can use pages bigger than 4kB. That's the purpose of the 4Kn64K > parameter in the integration, dropping bits [15:12] from the PA > presented to the CPU interface. there might be a misunderstanding, I was referring to my dropped quirk that I now dropped, not your options thanks Christian > > M. >
On 4/4/25 15:36, Marc Zyngier wrote: > On Fri, 04 Apr 2025 13:15:05 +0100, > Christian Bruel <christian.bruel@foss.st.com> wrote: >> >> >> >> On 4/3/25 19:50, Marc Zyngier wrote: >>> On Thu, 03 Apr 2025 13:28:04 +0100, >>> Christian Bruel <christian.bruel@foss.st.com> wrote: >>>> >>>> When GIC_4KNOT64K bit in the GIC configuration register is >>>> 0 (64KB), address block is modified in such a way than only the >>>> first 4KB of the GIC cpu interface are accessible with default >>>> offsets. >>>> With this bit mapping GICC_DIR register is accessible at >>>> offset 0x10000 instead of 0x1000, thus remap accordingly >>> >>> And I'm pretty sure the whole of the GICC range is correctly >>> accessible at offset 0xF000, giving you the full 8kB you need. That's >>> because each page of the GIC is aliased over two 64kB blocks, as per >>> the integration guidelines so that MMU isolation can be provided on a >>> 64kB boundary. >> >> Thanks a lot for this explanation, indeed this works like a charm. >> >>> >>> Funnily enough, all it takes is to adjust GICC region. You can either: >>> >>> - make it 128kB wide, and the driver will take care of it (details in >>> gic_check_eoimode()). On one of my boxes that is similarly >>> configured, I get: >>> >>> [ 0.000000] NR_IRQS: 64, nr_irqs: 64, preallocated irqs: 0 >>> [ 0.000000] GIC: Adjusting CPU interface base to 0x00000000780af000 >>> [ 0.000000] Root IRQ handler: gic_handle_irq >>> [ 0.000000] GIC: Using split EOI/Deactivate mode >>> >>> See below for what I expect to be the correct fix. >>> - make it 8kB wide from offset 0xF000. >> >> I checked both and they work. I will go for the former to show real >> 8kB size to be exposed in the DT. And there are a few other >> platforms that use this alias > > I think 8kB the wrong option. The GIC *is* supposed to be integrated > over 128kB on arm64 platforms (there was some documentation about that > back in the days, but it has become impossible to search anything on > ARM's stupidly broken website. My recollection is that it was bundled > with the GICv2m "specification" (only half a page!). > > Furthermore, you are supposed to describe the HW. Not your > interpretation of it. Correctly written SW targeting arm64 know about > this anyway. greping other platforms there are a bunch 0xf000 offset 8KB mapped: amd/amd-seattle-soc.dtsi arm/corstone1000.dtsi arm/foundation-v8-gicv3.dtsi arm/juno-base.dtsi mediatek/mt8516.dtsi but, looking at the stm32mp25 memory map (1) 0x4AC22000 - 0x4AC3FFFF 120 Reserved - 0x4AC20000 - 0x4AC21FFF 8 GICC I can know guess that the Reserved 120kB is for aliasing the 64kB blocks. Thus describing the GICC range at 128kB makes sense similarly 4 kB + 120 Reserved for GICH and 8kB + 120 Reserved for GICV (1) https://www.st.com/resource/en/reference_manual/rm0457-stm32mp25xx-advanced-armbased-3264bit-mpus-stmicroelectronics.pdf > >>> Unless the ST HW folks have been even more creative, none of this >>> overly complicated stuff should be necessary. Just describe the HW >>> correctly. >> >> I was unable to find this information in the GIC-400 trm >> (https://developer.arm.com/documentation/ddi0471/b/programmers-model/gic-400-register-map). Now >> I also prefer to use GICC alias at >> offset 0xf000 as suggested rather than the quirk solution > > Again, this isn't a quirk. It's the one true way for 64bit platforms > that can use pages bigger than 4kB. That's the purpose of the 4Kn64K > parameter in the integration, dropping bits [15:12] from the PA > presented to the CPU interface. sorry, misunderstanding, I was referring about my dropped quirk that I now dropped, not your options thanks Christian > > M. >
On Thu, Apr 03 2025 at 14:28, Christian Bruel wrote:
> When GIC_4KNOT64K bit in the GIC configuration register is
> 0 (64KB), address block is modified in such a way than only the
s/than/that/
> first 4KB of the GIC cpu interface are accessible with default
> offsets.
> With this bit mapping GICC_DIR register is accessible at
What's 'this bit mapping' ? This sentence does not parse.
> offset 0x10000 instead of 0x1000, thus remap accordingly
...
> +/*
> + * 8kB GICC range is not accessible with the default 4kB translation
> + * granule. 0x1000 offset is accessible at 64kB translation.
> + */
I have a hard time to map this comment to the change log, which suggests
to me that this is the other way round.
> +static bool gic_8kbaccess(void *data)
> +{
> + struct gic_chip_data *gic = data;
> + void __iomem *alt;
> +
> + if (!is_hyp_mode_available())
> + return false;
> +
> + alt = ioremap(gic->cpu_phys_base, GIC_STM32MP2_CPU_DEACTIVATE + 4);
> + if (!alt) {
> + pr_err("Unable to remap GICC_DIR register\n");
> + return false;
That's a hack because in case that the remap fails, this leaves the
thing enabled, but disfunctional.
Thanks,
tglx
Hello Thomas,
thanks for your comments.
After Marc's suggestion we found a better solution. So dropping this
patch set.
Christian
On 4/3/25 17:43, Thomas Gleixner wrote:
> On Thu, Apr 03 2025 at 14:28, Christian Bruel wrote:
>
>> When GIC_4KNOT64K bit in the GIC configuration register is
>> 0 (64KB), address block is modified in such a way than only the
>
> s/than/that/
>
>> first 4KB of the GIC cpu interface are accessible with default
>> offsets.
>> With this bit mapping GICC_DIR register is accessible at
>
> What's 'this bit mapping' ? This sentence does not parse.
>
>> offset 0x10000 instead of 0x1000, thus remap accordingly
>
> ...
>
>> +/*
>> + * 8kB GICC range is not accessible with the default 4kB translation
>> + * granule. 0x1000 offset is accessible at 64kB translation.
>> + */
>
> I have a hard time to map this comment to the change log, which suggests
> to me that this is the other way round.
>
>> +static bool gic_8kbaccess(void *data)
>> +{
>> + struct gic_chip_data *gic = data;
>> + void __iomem *alt;
>> +
>> + if (!is_hyp_mode_available())
>> + return false;
>> +
>> + alt = ioremap(gic->cpu_phys_base, GIC_STM32MP2_CPU_DEACTIVATE + 4);
>> + if (!alt) {
>> + pr_err("Unable to remap GICC_DIR register\n");
>> + return false;
>
> That's a hack because in case that the remap fails, this leaves the
> thing enabled, but disfunctional.
>
> Thanks,
>
> tglx
© 2016 - 2025 Red Hat, Inc.