* [PATCH 0/2] hw/arm/aspeed_ast27x0: minor IRQ number cleanup @ 2024-11-01 16:11 Peter Maydell 2024-11-01 16:11 ` [PATCH 1/2] hw/arm/aspeed_ast27x0: Use bsa.h for PPI definitions Peter Maydell ` (2 more replies) 0 siblings, 3 replies; 10+ messages in thread From: Peter Maydell @ 2024-11-01 16:11 UTC (permalink / raw) To: qemu-arm, qemu-devel Cc: Cédric Le Goater, Steven Lee, Troy Lee, Jamin Lin, Andrew Jeffery, Joel Stanley In the course of a conversation on IRC with somebody who was using the ast27x0 code as a model for a new board, I noticed that the code currently defines a local ARCH_GIC_MAINT_IRQ with a different value from the constant of the same name that we define in the include/hw/arm/bsa.h header. This patchset cleans that up and also another minor awkwardness that I spotted in the process. thanks -- PMM Peter Maydell (2): hw/arm/aspeed_ast27x0: Use bsa.h for PPI definitions hw/arm/aspeed_ast27x0: Avoid hardcoded '256' in IRQ calculation hw/arm/aspeed_ast27x0.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) -- 2.34.1 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] hw/arm/aspeed_ast27x0: Use bsa.h for PPI definitions 2024-11-01 16:11 [PATCH 0/2] hw/arm/aspeed_ast27x0: minor IRQ number cleanup Peter Maydell @ 2024-11-01 16:11 ` Peter Maydell 2024-11-01 18:22 ` Pierrick Bouvier 2024-11-04 10:23 ` Philippe Mathieu-Daudé 2024-11-01 16:11 ` [PATCH 2/2] hw/arm/aspeed_ast27x0: Avoid hardcoded '256' in IRQ calculation Peter Maydell 2024-11-02 14:59 ` [PATCH 0/2] hw/arm/aspeed_ast27x0: minor IRQ number cleanup Cédric Le Goater 2 siblings, 2 replies; 10+ messages in thread From: Peter Maydell @ 2024-11-01 16:11 UTC (permalink / raw) To: qemu-arm, qemu-devel Cc: Cédric Le Goater, Steven Lee, Troy Lee, Jamin Lin, Andrew Jeffery, Joel Stanley Use the private peripheral interrupt definitions from bsa.h instead of defining them locally. Note that bsa.h defines these values as INTID values, which are all 16 greater than the PPI values that we were previously using. So we refactor the code to use INTID-based values to match that. This is the same thing we did in commit d40ab068c07d9 for sbsa-ref. It removes the "same constant, different values" confusion where this board code and bsa.h both define an ARCH_GIC_MAINT_IRQ, and allows us to use symbolic names for the timer interrupt IDs. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/arm/aspeed_ast27x0.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c index dca660eb6be..5638a7a5781 100644 --- a/hw/arm/aspeed_ast27x0.c +++ b/hw/arm/aspeed_ast27x0.c @@ -13,6 +13,7 @@ #include "qapi/error.h" #include "hw/misc/unimp.h" #include "hw/arm/aspeed_soc.h" +#include "hw/arm/bsa.h" #include "qemu/module.h" #include "qemu/error-report.h" #include "hw/i2c/aspeed_i2c.h" @@ -416,28 +417,28 @@ 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, ARCH_GIC_MAINT_IRQ = 9, VIRTUAL_PMU_IRQ = 7; - int ppibase = NUM_IRQS + i * GIC_INTERNAL + GIC_NR_SGIS; + int NUM_IRQS = 256; + int intidbase = NUM_IRQS + i * GIC_INTERNAL; const int timer_irq[] = { - [GTIMER_PHYS] = 14, - [GTIMER_VIRT] = 11, - [GTIMER_HYP] = 10, - [GTIMER_SEC] = 13, + [GTIMER_PHYS] = ARCH_TIMER_NS_EL1_IRQ, + [GTIMER_VIRT] = ARCH_TIMER_VIRT_IRQ, + [GTIMER_HYP] = ARCH_TIMER_NS_EL2_IRQ, + [GTIMER_SEC] = ARCH_TIMER_S_EL1_IRQ, }; int j; for (j = 0; j < ARRAY_SIZE(timer_irq); j++) { qdev_connect_gpio_out(cpudev, j, - qdev_get_gpio_in(gicdev, ppibase + timer_irq[j])); + qdev_get_gpio_in(gicdev, intidbase + timer_irq[j])); } qemu_irq irq = qdev_get_gpio_in(gicdev, - ppibase + ARCH_GIC_MAINT_IRQ); + intidbase + ARCH_GIC_MAINT_IRQ); qdev_connect_gpio_out_named(cpudev, "gicv3-maintenance-interrupt", 0, irq); qdev_connect_gpio_out_named(cpudev, "pmu-interrupt", 0, - qdev_get_gpio_in(gicdev, ppibase + VIRTUAL_PMU_IRQ)); + qdev_get_gpio_in(gicdev, intidbase + VIRTUAL_PMU_IRQ)); sysbus_connect_irq(gicbusdev, i, qdev_get_gpio_in(cpudev, ARM_CPU_IRQ)); sysbus_connect_irq(gicbusdev, i + sc->num_cpus, -- 2.34.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] hw/arm/aspeed_ast27x0: Use bsa.h for PPI definitions 2024-11-01 16:11 ` [PATCH 1/2] hw/arm/aspeed_ast27x0: Use bsa.h for PPI definitions Peter Maydell @ 2024-11-01 18:22 ` Pierrick Bouvier 2024-11-04 10:23 ` Philippe Mathieu-Daudé 1 sibling, 0 replies; 10+ messages in thread From: Pierrick Bouvier @ 2024-11-01 18:22 UTC (permalink / raw) To: Peter Maydell, qemu-arm, qemu-devel Cc: Cédric Le Goater, Steven Lee, Troy Lee, Jamin Lin, Andrew Jeffery, Joel Stanley On 11/1/24 09:11, Peter Maydell wrote: > Use the private peripheral interrupt definitions from bsa.h instead > of defining them locally. > > Note that bsa.h defines these values as INTID values, which are all > 16 greater than the PPI values that we were previously using. So we > refactor the code to use INTID-based values to match that. > > This is the same thing we did in commit d40ab068c07d9 for sbsa-ref. > It removes the "same constant, different values" confusion where this > board code and bsa.h both define an ARCH_GIC_MAINT_IRQ, and allows us > to use symbolic names for the timer interrupt IDs. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > hw/arm/aspeed_ast27x0.c | 19 ++++++++++--------- > 1 file changed, 10 insertions(+), 9 deletions(-) > > diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c > index dca660eb6be..5638a7a5781 100644 > --- a/hw/arm/aspeed_ast27x0.c > +++ b/hw/arm/aspeed_ast27x0.c > @@ -13,6 +13,7 @@ > #include "qapi/error.h" > #include "hw/misc/unimp.h" > #include "hw/arm/aspeed_soc.h" > +#include "hw/arm/bsa.h" > #include "qemu/module.h" > #include "qemu/error-report.h" > #include "hw/i2c/aspeed_i2c.h" > @@ -416,28 +417,28 @@ 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, ARCH_GIC_MAINT_IRQ = 9, VIRTUAL_PMU_IRQ = 7; > - int ppibase = NUM_IRQS + i * GIC_INTERNAL + GIC_NR_SGIS; > + int NUM_IRQS = 256; > + int intidbase = NUM_IRQS + i * GIC_INTERNAL; > > const int timer_irq[] = { > - [GTIMER_PHYS] = 14, > - [GTIMER_VIRT] = 11, > - [GTIMER_HYP] = 10, > - [GTIMER_SEC] = 13, > + [GTIMER_PHYS] = ARCH_TIMER_NS_EL1_IRQ, > + [GTIMER_VIRT] = ARCH_TIMER_VIRT_IRQ, > + [GTIMER_HYP] = ARCH_TIMER_NS_EL2_IRQ, > + [GTIMER_SEC] = ARCH_TIMER_S_EL1_IRQ, > }; > int j; > > for (j = 0; j < ARRAY_SIZE(timer_irq); j++) { > qdev_connect_gpio_out(cpudev, j, > - qdev_get_gpio_in(gicdev, ppibase + timer_irq[j])); > + qdev_get_gpio_in(gicdev, intidbase + timer_irq[j])); > } > > qemu_irq irq = qdev_get_gpio_in(gicdev, > - ppibase + ARCH_GIC_MAINT_IRQ); > + intidbase + ARCH_GIC_MAINT_IRQ); > qdev_connect_gpio_out_named(cpudev, "gicv3-maintenance-interrupt", > 0, irq); > qdev_connect_gpio_out_named(cpudev, "pmu-interrupt", 0, > - qdev_get_gpio_in(gicdev, ppibase + VIRTUAL_PMU_IRQ)); > + qdev_get_gpio_in(gicdev, intidbase + VIRTUAL_PMU_IRQ)); > > sysbus_connect_irq(gicbusdev, i, qdev_get_gpio_in(cpudev, ARM_CPU_IRQ)); > sysbus_connect_irq(gicbusdev, i + sc->num_cpus, Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] hw/arm/aspeed_ast27x0: Use bsa.h for PPI definitions 2024-11-01 16:11 ` [PATCH 1/2] hw/arm/aspeed_ast27x0: Use bsa.h for PPI definitions Peter Maydell 2024-11-01 18:22 ` Pierrick Bouvier @ 2024-11-04 10:23 ` Philippe Mathieu-Daudé 1 sibling, 0 replies; 10+ messages in thread From: Philippe Mathieu-Daudé @ 2024-11-04 10:23 UTC (permalink / raw) To: Peter Maydell, qemu-arm, qemu-devel Cc: Cédric Le Goater, Steven Lee, Troy Lee, Jamin Lin, Andrew Jeffery, Joel Stanley On 1/11/24 13:11, Peter Maydell wrote: > Use the private peripheral interrupt definitions from bsa.h instead > of defining them locally. > > Note that bsa.h defines these values as INTID values, which are all > 16 greater than the PPI values that we were previously using. So we > refactor the code to use INTID-based values to match that. > > This is the same thing we did in commit d40ab068c07d9 for sbsa-ref. > It removes the "same constant, different values" confusion where this > board code and bsa.h both define an ARCH_GIC_MAINT_IRQ, and allows us > to use symbolic names for the timer interrupt IDs. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > hw/arm/aspeed_ast27x0.c | 19 ++++++++++--------- > 1 file changed, 10 insertions(+), 9 deletions(-) Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/2] hw/arm/aspeed_ast27x0: Avoid hardcoded '256' in IRQ calculation 2024-11-01 16:11 [PATCH 0/2] hw/arm/aspeed_ast27x0: minor IRQ number cleanup Peter Maydell 2024-11-01 16:11 ` [PATCH 1/2] hw/arm/aspeed_ast27x0: Use bsa.h for PPI definitions Peter Maydell @ 2024-11-01 16:11 ` Peter Maydell 2024-11-01 18:23 ` Pierrick Bouvier ` (2 more replies) 2024-11-02 14:59 ` [PATCH 0/2] hw/arm/aspeed_ast27x0: minor IRQ number cleanup Cédric Le Goater 2 siblings, 3 replies; 10+ messages in thread From: Peter Maydell @ 2024-11-01 16:11 UTC (permalink / raw) To: qemu-arm, qemu-devel Cc: Cédric Le Goater, Steven Lee, Troy Lee, Jamin Lin, Andrew Jeffery, Joel Stanley 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, -- 2.34.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] hw/arm/aspeed_ast27x0: Avoid hardcoded '256' in IRQ calculation 2024-11-01 16:11 ` [PATCH 2/2] hw/arm/aspeed_ast27x0: Avoid hardcoded '256' in IRQ calculation Peter Maydell @ 2024-11-01 18:23 ` Pierrick Bouvier 2024-11-04 10:24 ` Philippe Mathieu-Daudé 2025-01-30 15:02 ` Philippe Mathieu-Daudé 2 siblings, 0 replies; 10+ messages in thread From: Pierrick Bouvier @ 2024-11-01 18:23 UTC (permalink / raw) To: Peter Maydell, qemu-arm, qemu-devel Cc: Cédric Le Goater, Steven Lee, Troy Lee, Jamin Lin, Andrew Jeffery, Joel Stanley 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> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] hw/arm/aspeed_ast27x0: Avoid hardcoded '256' in IRQ calculation 2024-11-01 16:11 ` [PATCH 2/2] hw/arm/aspeed_ast27x0: Avoid hardcoded '256' in IRQ calculation Peter Maydell 2024-11-01 18:23 ` Pierrick Bouvier @ 2024-11-04 10:24 ` Philippe Mathieu-Daudé 2025-01-30 15:02 ` Philippe Mathieu-Daudé 2 siblings, 0 replies; 10+ messages in thread From: Philippe Mathieu-Daudé @ 2024-11-04 10:24 UTC (permalink / raw) To: Peter Maydell, qemu-arm, qemu-devel Cc: Cédric Le Goater, Steven Lee, Troy Lee, Jamin Lin, Andrew Jeffery, Joel Stanley 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> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] hw/arm/aspeed_ast27x0: Avoid hardcoded '256' in IRQ calculation 2024-11-01 16:11 ` [PATCH 2/2] hw/arm/aspeed_ast27x0: Avoid hardcoded '256' in IRQ calculation Peter Maydell 2024-11-01 18:23 ` Pierrick Bouvier 2024-11-04 10:24 ` Philippe Mathieu-Daudé @ 2025-01-30 15:02 ` Philippe Mathieu-Daudé 2025-02-03 5:10 ` Jamin Lin 2 siblings, 1 reply; 10+ messages in thread From: Philippe Mathieu-Daudé @ 2025-01-30 15:02 UTC (permalink / raw) To: qemu-arm, qemu-devel Cc: Cédric Le Goater, Peter Maydell, Steven Lee, Troy Lee, Jamin Lin, Andrew Jeffery, Joel Stanley 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, ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH 2/2] hw/arm/aspeed_ast27x0: Avoid hardcoded '256' in IRQ calculation 2025-01-30 15:02 ` Philippe Mathieu-Daudé @ 2025-02-03 5:10 ` Jamin Lin 0 siblings, 0 replies; 10+ messages in thread From: Jamin Lin @ 2025-02-03 5:10 UTC (permalink / raw) To: Philippe Mathieu-Daudé, qemu-arm@nongnu.org, qemu-devel@nongnu.org Cc: Cédric Le Goater, Peter Maydell, Steven Lee, Troy Lee, Andrew Jeffery, Joel Stanley 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, ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] hw/arm/aspeed_ast27x0: minor IRQ number cleanup 2024-11-01 16:11 [PATCH 0/2] hw/arm/aspeed_ast27x0: minor IRQ number cleanup Peter Maydell 2024-11-01 16:11 ` [PATCH 1/2] hw/arm/aspeed_ast27x0: Use bsa.h for PPI definitions Peter Maydell 2024-11-01 16:11 ` [PATCH 2/2] hw/arm/aspeed_ast27x0: Avoid hardcoded '256' in IRQ calculation Peter Maydell @ 2024-11-02 14:59 ` Cédric Le Goater 2 siblings, 0 replies; 10+ messages in thread From: Cédric Le Goater @ 2024-11-02 14:59 UTC (permalink / raw) To: Peter Maydell, qemu-arm, qemu-devel Cc: Steven Lee, Troy Lee, Jamin Lin, Andrew Jeffery, Joel Stanley On 11/1/24 17:11, Peter Maydell wrote: > In the course of a conversation on IRC with somebody who was using > the ast27x0 code as a model for a new board, I noticed that the > code currently defines a local ARCH_GIC_MAINT_IRQ with a different > value from the constant of the same name that we define in the > include/hw/arm/bsa.h header. This patchset cleans that up and > also another minor awkwardness that I spotted in the process. > > thanks > -- PMM > > Peter Maydell (2): > hw/arm/aspeed_ast27x0: Use bsa.h for PPI definitions > hw/arm/aspeed_ast27x0: Avoid hardcoded '256' in IRQ calculation > > hw/arm/aspeed_ast27x0.c | 22 +++++++++++----------- > 1 file changed, 11 insertions(+), 11 deletions(-) > Applied to aspeed-next. Thanks, C. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2025-02-03 5:32 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-11-01 16:11 [PATCH 0/2] hw/arm/aspeed_ast27x0: minor IRQ number cleanup Peter Maydell 2024-11-01 16:11 ` [PATCH 1/2] hw/arm/aspeed_ast27x0: Use bsa.h for PPI definitions Peter Maydell 2024-11-01 18:22 ` Pierrick Bouvier 2024-11-04 10:23 ` Philippe Mathieu-Daudé 2024-11-01 16:11 ` [PATCH 2/2] hw/arm/aspeed_ast27x0: Avoid hardcoded '256' in IRQ calculation Peter Maydell 2024-11-01 18:23 ` Pierrick Bouvier 2024-11-04 10:24 ` Philippe Mathieu-Daudé 2025-01-30 15:02 ` Philippe Mathieu-Daudé 2025-02-03 5:10 ` Jamin Lin 2024-11-02 14:59 ` [PATCH 0/2] hw/arm/aspeed_ast27x0: minor IRQ number cleanup Cédric Le Goater
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).