[PATCH 0/3] Add ST STM32MP2 GICv2 quirk for EOI split mode

[PATCH 0/3] Add ST STM32MP2 GICv2 quirk for EOI split mode

[Christian Bruel]

When using GIC EOI split mode, GICC_DIR fails to deactivate the interrupt,
leading to a WFI freeze. On ST MP2, GIC cpu interface is limitted to 4K,
thus GICC_DIR register is reachable with a 0x10000 remapping

When using GIC EOI split mode, the GICC_DIR fails to deactivate the
interrupt, causing core freeze on WFI. On the ST MP2, the GIC CPU interface
is limited to 4K, so the GICC_DIR register can be accessed remapping the
register to a 0x10000 offset.

Christian Bruel (3):
  dt-bindings: interrupt-controller: arm,gic: Add
    st,stm32mp2-cortex-a7-gic
  irqchip/gic: Use 0x10000 offset to access GICC_DIR
  arm64: dts: st: add st,stm32mp2-cortex-a7-gic in intc node in
    stm32mp251.dtsi

 .../interrupt-controller/arm,gic.yaml         |  1 +
 arch/arm64/boot/dts/st/stm32mp251.dtsi        |  2 +-
 drivers/irqchip/irq-gic.c                     | 47 ++++++++++++++++++-
 3 files changed, 48 insertions(+), 2 deletions(-)

-- 
2.34.1

[PATCH 1/3] dt-bindings: interrupt-controller: arm,gic: Add st,stm32mp2-cortex-a7-gic

[Christian Bruel]

Add st,stm32mp2-cortex-a7-gic compatible to support remapping of GICC_DIR

Signed-off-by: Christian Bruel <christian.bruel@foss.st.com>
---
 .../devicetree/bindings/interrupt-controller/arm,gic.yaml        | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/interrupt-controller/arm,gic.yaml b/Documentation/devicetree/bindings/interrupt-controller/arm,gic.yaml
index 7173c4b5a228..7ea7224b2f36 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/arm,gic.yaml
+++ b/Documentation/devicetree/bindings/interrupt-controller/arm,gic.yaml
@@ -37,6 +37,7 @@ properties:
               - arm,tc11mp-gic
               - qcom,msm-8660-qgic
               - qcom,msm-qgic2
+              - st,stm32mp2-cortex-a7-gic
 
       - items:
           - const: arm,gic-400
-- 
2.34.1

[PATCH 2/3] irqchip/gic: Use 0x10000 offset to access GICC_DIR on STM32MP2

[Christian Bruel]

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.1

[PATCH 3/3] arm64: dts: st: add st,stm32mp2-cortex-a7-gic in intc node in stm32mp251.dtsi

[Christian Bruel]

Add st,stm32mp2-cortex-a7-gic to enable the GICC_DIR register remap

Signed-off-by: Christian Bruel <christian.bruel@foss.st.com>
---
 arch/arm64/boot/dts/st/stm32mp251.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/st/stm32mp251.dtsi b/arch/arm64/boot/dts/st/stm32mp251.dtsi
index f3c6cdfd7008..030e5da67a7e 100644
--- a/arch/arm64/boot/dts/st/stm32mp251.dtsi
+++ b/arch/arm64/boot/dts/st/stm32mp251.dtsi
@@ -115,7 +115,7 @@ scmi_vdda18adc: regulator@7 {
 	};
 
 	intc: interrupt-controller@4ac00000 {
-		compatible = "arm,cortex-a7-gic";
+		compatible = "st,stm32mp2-cortex-a7-gic", "arm,cortex-a7-gic";
 		#interrupt-cells = <3>;
 		#address-cells = <1>;
 		interrupt-controller;
-- 
2.34.1

Re: [PATCH 0/3] Add ST STM32MP2 GICv2 quirk for EOI split mode

[Rob Herring (Arm)]

On Thu, 03 Apr 2025 14:28:02 +0200, Christian Bruel wrote:
> When using GIC EOI split mode, GICC_DIR fails to deactivate the interrupt,
> leading to a WFI freeze. On ST MP2, GIC cpu interface is limitted to 4K,
> thus GICC_DIR register is reachable with a 0x10000 remapping
> 
> When using GIC EOI split mode, the GICC_DIR fails to deactivate the
> interrupt, causing core freeze on WFI. On the ST MP2, the GIC CPU interface
> is limited to 4K, so the GICC_DIR register can be accessed remapping the
> register to a 0x10000 offset.
> 
> Christian Bruel (3):
>   dt-bindings: interrupt-controller: arm,gic: Add
>     st,stm32mp2-cortex-a7-gic
>   irqchip/gic: Use 0x10000 offset to access GICC_DIR
>   arm64: dts: st: add st,stm32mp2-cortex-a7-gic in intc node in
>     stm32mp251.dtsi
> 
>  .../interrupt-controller/arm,gic.yaml         |  1 +
>  arch/arm64/boot/dts/st/stm32mp251.dtsi        |  2 +-
>  drivers/irqchip/irq-gic.c                     | 47 ++++++++++++++++++-
>  3 files changed, 48 insertions(+), 2 deletions(-)
> 
> --
> 2.34.1
> 
> 
> 


My bot found new DTB warnings on the .dts files added or changed in this
series.

Some warnings may be from an existing SoC .dtsi. Or perhaps the warnings
are fixed by another series. Ultimately, it is up to the platform
maintainer whether these warnings are acceptable or not. No need to reply
unless the platform maintainer has comments.

If you already ran DT checks and didn't see these error(s), then
make sure dt-schema is up to date:

  pip3 install dtschema --upgrade


This patch series was applied (using b4) to base:
 Base: attempting to guess base-commit...
 Base: tags/next-20250403 (exact match)

If this is not the correct base, please add 'base-commit' tag
(or use b4 which does this automatically)

New warnings running 'make CHECK_DTBS=y for arch/arm64/boot/dts/st/' for 20250403122805.1574086-1-christian.bruel@foss.st.com:

arch/arm64/boot/dts/st/stm32mp257f-dk.dtb: interrupt-controller@4ac00000: compatible: 'oneOf' conditional failed, one must be fixed:
	['st,stm32mp2-cortex-a7-gic', 'arm,cortex-a7-gic'] is too long
arch/arm64/boot/dts/st/stm32mp257f-dk.dtb: interrupt-controller@4ac00000: compatible: 'oneOf' conditional failed, one must be fixed:
		['st,stm32mp2-cortex-a7-gic', 'arm,cortex-a7-gic'] is too long
		'nvidia,tegra210-agic' was expected
		'st,stm32mp2-cortex-a7-gic' is not one of ['nvidia,tegra186-agic', 'nvidia,tegra194-agic', 'nvidia,tegra234-agic']
	'arm,gic-400' was expected
	'arm,arm1176jzf-devchip-gic' was expected
	'brcm,brahma-b15-gic' was expected
	'arm,arm11mp-gic' was expected
	'arm,cortex-a15-gic' was expected
	from schema $id: http://devicetree.org/schemas/interrupt-controller/arm,gic.yaml#
arch/arm64/boot/dts/st/stm32mp257f-ev1.dtb: interrupt-controller@4ac00000: compatible: 'oneOf' conditional failed, one must be fixed:
	['st,stm32mp2-cortex-a7-gic', 'arm,cortex-a7-gic'] is too long
arch/arm64/boot/dts/st/stm32mp257f-ev1.dtb: interrupt-controller@4ac00000: compatible: 'oneOf' conditional failed, one must be fixed:
		['st,stm32mp2-cortex-a7-gic', 'arm,cortex-a7-gic'] is too long
		'nvidia,tegra210-agic' was expected
		'st,stm32mp2-cortex-a7-gic' is not one of ['nvidia,tegra186-agic', 'nvidia,tegra194-agic', 'nvidia,tegra234-agic']
	'arm,gic-400' was expected
	'arm,arm1176jzf-devchip-gic' was expected
	'brcm,brahma-b15-gic' was expected
	'arm,arm11mp-gic' was expected
	'arm,cortex-a15-gic' was expected
	from schema $id: http://devicetree.org/schemas/interrupt-controller/arm,gic.yaml#

Re: [PATCH 2/3] irqchip/gic: Use 0x10000 offset to access GICC_DIR on STM32MP2

[Thomas Gleixner]

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

Re: [PATCH 3/3] arm64: dts: st: add st,stm32mp2-cortex-a7-gic in intc node in stm32mp251.dtsi

[Marc Zyngier]

On Thu, 03 Apr 2025 13:28:05 +0100,
Christian Bruel <christian.bruel@foss.st.com> wrote:
> 
> Add st,stm32mp2-cortex-a7-gic to enable the GICC_DIR register remap
> 
> Signed-off-by: Christian Bruel <christian.bruel@foss.st.com>
> ---
>  arch/arm64/boot/dts/st/stm32mp251.dtsi | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/st/stm32mp251.dtsi b/arch/arm64/boot/dts/st/stm32mp251.dtsi
> index f3c6cdfd7008..030e5da67a7e 100644
> --- a/arch/arm64/boot/dts/st/stm32mp251.dtsi
> +++ b/arch/arm64/boot/dts/st/stm32mp251.dtsi
> @@ -115,7 +115,7 @@ scmi_vdda18adc: regulator@7 {
>  	};
>  
>  	intc: interrupt-controller@4ac00000 {
> -		compatible = "arm,cortex-a7-gic";
> +		compatible = "st,stm32mp2-cortex-a7-gic", "arm,cortex-a7-gic";

What nonsense is this? This is an *arm64* machine, with I expect a
GIC400. Where is this A7 compat coming from?

	M.

-- 
Jazz isn't dead. It just smells funny.

Re: [PATCH 2/3] irqchip/gic: Use 0x10000 offset to access GICC_DIR on STM32MP2

[Marc Zyngier]

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.

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.

Re: [PATCH 2/3] irqchip/gic: Use 0x10000 offset to access GICC_DIR on STM32MP2

[Christian Bruel]

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>;
>   	};
> 

Re: [PATCH 3/3] arm64: dts: st: add st,stm32mp2-cortex-a7-gic in intc node in stm32mp251.dtsi

[Christian Bruel]

On 4/3/25 19:27, Marc Zyngier wrote:
> On Thu, 03 Apr 2025 13:28:05 +0100,
> Christian Bruel <christian.bruel@foss.st.com> wrote:
>>
>> Add st,stm32mp2-cortex-a7-gic to enable the GICC_DIR register remap
>>
>> Signed-off-by: Christian Bruel <christian.bruel@foss.st.com>
>> ---
>>   arch/arm64/boot/dts/st/stm32mp251.dtsi | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/boot/dts/st/stm32mp251.dtsi b/arch/arm64/boot/dts/st/stm32mp251.dtsi
>> index f3c6cdfd7008..030e5da67a7e 100644
>> --- a/arch/arm64/boot/dts/st/stm32mp251.dtsi
>> +++ b/arch/arm64/boot/dts/st/stm32mp251.dtsi
>> @@ -115,7 +115,7 @@ scmi_vdda18adc: regulator@7 {
>>   	};
>>   
>>   	intc: interrupt-controller@4ac00000 {
>> -		compatible = "arm,cortex-a7-gic";
>> +		compatible = "st,stm32mp2-cortex-a7-gic", "arm,cortex-a7-gic";
> 
> What nonsense is this? This is an *arm64* machine, with I expect a
> GIC400. Where is this A7 compat coming from?

Probably historical, as the first port was for aarch32. I will fix this 
separately. thanks for the head up!

Christian

> 
> 	M.
> 

Re: [PATCH 2/3] irqchip/gic: Use 0x10000 offset to access GICC_DIR on STM32MP2

[Christian Bruel]

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

Re: [PATCH 3/3] arm64: dts: st: add st,stm32mp2-cortex-a7-gic in intc node in stm32mp251.dtsi

[Marc Zyngier]

On Fri, 04 Apr 2025 13:17:08 +0100,
Christian Bruel <christian.bruel@foss.st.com> wrote:
> 
> 
> 
> On 4/3/25 19:27, Marc Zyngier wrote:
> > On Thu, 03 Apr 2025 13:28:05 +0100,
> > Christian Bruel <christian.bruel@foss.st.com> wrote:
> >> 
> >> Add st,stm32mp2-cortex-a7-gic to enable the GICC_DIR register remap
> >> 
> >> Signed-off-by: Christian Bruel <christian.bruel@foss.st.com>
> >> ---
> >>   arch/arm64/boot/dts/st/stm32mp251.dtsi | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >> 
> >> diff --git a/arch/arm64/boot/dts/st/stm32mp251.dtsi b/arch/arm64/boot/dts/st/stm32mp251.dtsi
> >> index f3c6cdfd7008..030e5da67a7e 100644
> >> --- a/arch/arm64/boot/dts/st/stm32mp251.dtsi
> >> +++ b/arch/arm64/boot/dts/st/stm32mp251.dtsi
> >> @@ -115,7 +115,7 @@ scmi_vdda18adc: regulator@7 {
> >>   	};
> >>     	intc: interrupt-controller@4ac00000 {
> >> -		compatible = "arm,cortex-a7-gic";
> >> +		compatible = "st,stm32mp2-cortex-a7-gic", "arm,cortex-a7-gic";
> > 
> > What nonsense is this? This is an *arm64* machine, with I expect a
> > GIC400. Where is this A7 compat coming from?
> 
> Probably historical, as the first port was for aarch32. I will fix
> this separately. thanks for the head up!

Then while you're at it, you may want to consider removing the
"always-on" property in the timer, because I'm pretty sure the
comparator goes down in low power mode on A53 and A35, and loses its
value.

In general, only VMs can make use of this property.

	M.

-- 
Jazz isn't dead. It just smells funny.

Re: [PATCH 2/3] irqchip/gic: Use 0x10000 offset to access GICC_DIR on STM32MP2

[Marc Zyngier]

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.

Re: [PATCH 2/3] irqchip/gic: Use 0x10000 offset to access GICC_DIR on STM32MP2

[Christian Bruel]

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.
> 

Re: [PATCH 2/3] irqchip/gic: Use 0x10000 offset to access GICC_DIR on STM32MP2

[Christian Bruel]

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.
>