RE: [PATCH 2/2] hw/arm/aspeed_ast27x0: Avoid hardcoded '256' in IRQ calculation
Hi Philippe > From: Philippe Mathieu-Daudé > Sent: Thursday, January 30, 2025 11:03 PM > To: [email protected]; [email protected] > Cc: Cédric Le Goater ; Peter Maydell > ; Steven Lee ; Troy > Lee ; Jamin Lin ; Andrew > Jeffery ; Joel Stanley > Subject: Re: [PATCH 2/2] hw/arm/aspeed_ast27x0: Avoid hardcoded '256' in > IRQ calculation > > On 1/11/24 17:11, Peter Maydell wrote: > > When calculating the index into the GIC's GPIO array for per-CPU > > interrupts, we have to start with the number of SPIs. The code > > currently hard-codes this to 'NUM_IRQS = 256'. However the number of > > SPIs is set separately and implicitly by the value of AST2700_MAX_IRQ, > > which is the number of SPIs plus 32 (since it is what we set the GIC > > num-irq property to). > > > > Define AST2700_MAX_IRQ as the total number of SPIs; this brings > > AST2700 into line with AST2600, which defines AST2600_MAX_IRQ as the > > number of SPIs not including the 32 internal interrupts. We can then > > use AST2700_MAX_IRQ instead of the hardcoded 256. > > > > Signed-off-by: Peter Maydell > > --- > > hw/arm/aspeed_ast27x0.c | 7 +++ > > 1 file changed, 3 insertions(+), 4 deletions(-) > > > > diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c index > > 5638a7a5781..7b246440952 100644 > > --- a/hw/arm/aspeed_ast27x0.c > > +++ b/hw/arm/aspeed_ast27x0.c > > @@ -66,7 +66,7 @@ static const hwaddr aspeed_soc_ast2700_memmap[] = > { > > [ASPEED_DEV_GPIO] = 0x14C0B000, > > }; > > > > -#define AST2700_MAX_IRQ 288 > > +#define AST2700_MAX_IRQ 256 > > > > /* Shared Peripheral Interrupt values below are offset by -32 from > datasheet */ > > static const int aspeed_soc_ast2700_irqmap[] = { @@ -403,7 +403,7 @@ > > static bool aspeed_soc_ast2700_gic_realize(DeviceState *dev, Error **errp) > > gicdev = DEVICE(&a->gic); > > qdev_prop_set_uint32(gicdev, "revision", 3); > > qdev_prop_set_uint32(gicdev, "num-cpu", sc->num_cpus); > > -qdev_prop_set_uint32(gicdev, "num-irq", AST2700_MAX_IRQ); > > +qdev_prop_set_uint32(gicdev, "num-irq", AST2700_MAX_IRQ + > > + GIC_INTERNAL); > > Pre-existing, GIC has AST2700_MAX_IRQ + GIC_INTERNAL IRQs, ... > > > > > redist_region_count = qlist_new(); > > qlist_append_int(redist_region_count, sc->num_cpus); @@ -417,8 > > +417,7 @@ static bool aspeed_soc_ast2700_gic_realize(DeviceState *dev, > > Error **errp) > > > > for (i = 0; i < sc->num_cpus; i++) { > > DeviceState *cpudev = DEVICE(&a->cpu[i]); > > -int NUM_IRQS = 256; > > -int intidbase = NUM_IRQS + i * GIC_INTERNAL; > > +int intidbase = AST2700_MAX_IRQ + i * GIC_INTERNAL; > > ... then our base IRQ starts at AST2700_MAX_IRQ + i * GIC_INTERNAL, having i > = [0..3]. Jamin, is that correct? > Correct. The value of "i" is 0 to 3 and the value of intidbase is 256, 288, 320 and 352. > > > > const int timer_irq[] = { > > [GTIMER_PHYS] = ARCH_TIMER_NS_EL1_IRQ,
Re: [PATCH 2/2] hw/arm/aspeed_ast27x0: Avoid hardcoded '256' in IRQ calculation
On 1/11/24 17:11, Peter Maydell wrote:
When calculating the index into the GIC's GPIO array for per-CPU
interrupts, we have to start with the number of SPIs. The code
currently hard-codes this to 'NUM_IRQS = 256'. However the number of
SPIs is set separately and implicitly by the value of
AST2700_MAX_IRQ, which is the number of SPIs plus 32 (since it is
what we set the GIC num-irq property to).
Define AST2700_MAX_IRQ as the total number of SPIs; this brings
AST2700 into line with AST2600, which defines AST2600_MAX_IRQ as the
number of SPIs not including the 32 internal interrupts. We can then
use AST2700_MAX_IRQ instead of the hardcoded 256.
Signed-off-by: Peter Maydell
---
hw/arm/aspeed_ast27x0.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c
index 5638a7a5781..7b246440952 100644
--- a/hw/arm/aspeed_ast27x0.c
+++ b/hw/arm/aspeed_ast27x0.c
@@ -66,7 +66,7 @@ static const hwaddr aspeed_soc_ast2700_memmap[] = {
[ASPEED_DEV_GPIO] = 0x14C0B000,
};
-#define AST2700_MAX_IRQ 288
+#define AST2700_MAX_IRQ 256
/* Shared Peripheral Interrupt values below are offset by -32 from datasheet */
static const int aspeed_soc_ast2700_irqmap[] = {
@@ -403,7 +403,7 @@ static bool aspeed_soc_ast2700_gic_realize(DeviceState
*dev, Error **errp)
gicdev = DEVICE(&a->gic);
qdev_prop_set_uint32(gicdev, "revision", 3);
qdev_prop_set_uint32(gicdev, "num-cpu", sc->num_cpus);
-qdev_prop_set_uint32(gicdev, "num-irq", AST2700_MAX_IRQ);
+qdev_prop_set_uint32(gicdev, "num-irq", AST2700_MAX_IRQ + GIC_INTERNAL);
Pre-existing, GIC has AST2700_MAX_IRQ + GIC_INTERNAL IRQs, ...
redist_region_count = qlist_new();
qlist_append_int(redist_region_count, sc->num_cpus);
@@ -417,8 +417,7 @@ static bool aspeed_soc_ast2700_gic_realize(DeviceState
*dev, Error **errp)
for (i = 0; i < sc->num_cpus; i++) {
DeviceState *cpudev = DEVICE(&a->cpu[i]);
-int NUM_IRQS = 256;
-int intidbase = NUM_IRQS + i * GIC_INTERNAL;
+int intidbase = AST2700_MAX_IRQ + i * GIC_INTERNAL;
... then our base IRQ starts at AST2700_MAX_IRQ + i * GIC_INTERNAL,
having i = [0..3]. Jamin, is that correct?
const int timer_irq[] = {
[GTIMER_PHYS] = ARCH_TIMER_NS_EL1_IRQ,
Re: [PATCH 2/2] hw/arm/aspeed_ast27x0: Avoid hardcoded '256' in IRQ calculation
On 1/11/24 13:11, Peter Maydell wrote: When calculating the index into the GIC's GPIO array for per-CPU interrupts, we have to start with the number of SPIs. The code currently hard-codes this to 'NUM_IRQS = 256'. However the number of SPIs is set separately and implicitly by the value of AST2700_MAX_IRQ, which is the number of SPIs plus 32 (since it is what we set the GIC num-irq property to). Define AST2700_MAX_IRQ as the total number of SPIs; this brings AST2700 into line with AST2600, which defines AST2600_MAX_IRQ as the number of SPIs not including the 32 internal interrupts. We can then use AST2700_MAX_IRQ instead of the hardcoded 256. Signed-off-by: Peter Maydell --- hw/arm/aspeed_ast27x0.c | 7 +++ 1 file changed, 3 insertions(+), 4 deletions(-) Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH 2/2] hw/arm/aspeed_ast27x0: Avoid hardcoded '256' in IRQ calculation
On 11/1/24 09:11, Peter Maydell wrote:
When calculating the index into the GIC's GPIO array for per-CPU
interrupts, we have to start with the number of SPIs. The code
currently hard-codes this to 'NUM_IRQS = 256'. However the number of
SPIs is set separately and implicitly by the value of
AST2700_MAX_IRQ, which is the number of SPIs plus 32 (since it is
what we set the GIC num-irq property to).
Define AST2700_MAX_IRQ as the total number of SPIs; this brings
AST2700 into line with AST2600, which defines AST2600_MAX_IRQ as the
number of SPIs not including the 32 internal interrupts. We can then
use AST2700_MAX_IRQ instead of the hardcoded 256.
Signed-off-by: Peter Maydell
---
hw/arm/aspeed_ast27x0.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c
index 5638a7a5781..7b246440952 100644
--- a/hw/arm/aspeed_ast27x0.c
+++ b/hw/arm/aspeed_ast27x0.c
@@ -66,7 +66,7 @@ static const hwaddr aspeed_soc_ast2700_memmap[] = {
[ASPEED_DEV_GPIO] = 0x14C0B000,
};
-#define AST2700_MAX_IRQ 288
+#define AST2700_MAX_IRQ 256
/* Shared Peripheral Interrupt values below are offset by -32 from datasheet */
static const int aspeed_soc_ast2700_irqmap[] = {
@@ -403,7 +403,7 @@ static bool aspeed_soc_ast2700_gic_realize(DeviceState
*dev, Error **errp)
gicdev = DEVICE(&a->gic);
qdev_prop_set_uint32(gicdev, "revision", 3);
qdev_prop_set_uint32(gicdev, "num-cpu", sc->num_cpus);
-qdev_prop_set_uint32(gicdev, "num-irq", AST2700_MAX_IRQ);
+qdev_prop_set_uint32(gicdev, "num-irq", AST2700_MAX_IRQ + GIC_INTERNAL);
redist_region_count = qlist_new();
qlist_append_int(redist_region_count, sc->num_cpus);
@@ -417,8 +417,7 @@ static bool aspeed_soc_ast2700_gic_realize(DeviceState
*dev, Error **errp)
for (i = 0; i < sc->num_cpus; i++) {
DeviceState *cpudev = DEVICE(&a->cpu[i]);
-int NUM_IRQS = 256;
-int intidbase = NUM_IRQS + i * GIC_INTERNAL;
+int intidbase = AST2700_MAX_IRQ + i * GIC_INTERNAL;
const int timer_irq[] = {
[GTIMER_PHYS] = ARCH_TIMER_NS_EL1_IRQ,
Reviewed-by: Pierrick Bouvier
