[2/2] hw/arm/aspeed_ast27x0: Avoid hardcoded '256' in IRQ calculation

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 <peter.maydell@linaro.org>
---
 hw/arm/aspeed_ast27x0.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

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 <peter.maydell@linaro.org>
> ---
>   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 <pierrick.bouvier@linaro.org>

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 <peter.maydell@linaro.org>
> ---
>   hw/arm/aspeed_ast27x0.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

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 <peter.maydell@linaro.org>
> ---
>   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,

Hi Philippe

> From: Philippe Mathieu-Daudé <philmd@linaro.org>
> Sent: Thursday, January 30, 2025 11:03 PM
> To: qemu-arm@nongnu.org; qemu-devel@nongnu.org
> Cc: Cédric Le Goater <clg@kaod.org>; Peter Maydell
> <peter.maydell@linaro.org>; Steven Lee <steven_lee@aspeedtech.com>; Troy
> Lee <leetroy@gmail.com>; Jamin Lin <jamin_lin@aspeedtech.com>; Andrew
> Jeffery <andrew@codeconstruct.com.au>; Joel Stanley <joel@jms.id.au>
> 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 <peter.maydell@linaro.org>
> > ---
> >   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,