| Message ID | 20240208181245.96617-12-philmd@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hw: Strengthen SysBus & QBus API | expand |
On Thu, 8 Feb 2024 at 18:14, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > Inline cpu_create() in order to call > qdev_init_gpio_in_named_with_opaque() > before the CPU is realized. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > hw/sparc64/sparc64.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/hw/sparc64/sparc64.c b/hw/sparc64/sparc64.c > index 72f0849f50..3091cde586 100644 > --- a/hw/sparc64/sparc64.c > +++ b/hw/sparc64/sparc64.c > @@ -24,6 +24,7 @@ > > > #include "qemu/osdep.h" > +#include "qapi/error.h" > #include "cpu.h" > #include "hw/boards.h" > #include "hw/sparc/sparc64.h" > @@ -271,9 +272,10 @@ SPARCCPU *sparc64_cpu_devinit(const char *cpu_type, uint64_t prom_addr) > uint32_t stick_frequency = 100 * 1000000; > uint32_t hstick_frequency = 100 * 1000000; > > - cpu = SPARC_CPU(cpu_create(cpu_type)); > + cpu = SPARC_CPU(object_new(cpu_type)); > qdev_init_gpio_in_named(DEVICE(cpu), sparc64_cpu_set_ivec_irq, > "ivec-irq", IVEC_MAX); > + qdev_realize(DEVICE(cpu), NULL, &error_fatal); > env = &cpu->env; > > env->tick = cpu_timer_create("tick", cpu, tick_irq, > -- > 2.41.0 For the purposes of letting us enforce the "init GPIOs before realize, not after" rule, Reviewed-by: Peter Maydell <peter.maydell@linaro.org> but it looks like this code is adding a GPIO to a device from code that's not actually part of the implementation of the device. Ideally most of the code in this file should be rolled into the CPU itself in target/sparc. thanks -- PMM
On 08/02/2024 18:12, Philippe Mathieu-Daudé wrote: > Inline cpu_create() in order to call > qdev_init_gpio_in_named_with_opaque() > before the CPU is realized. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > hw/sparc64/sparc64.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/hw/sparc64/sparc64.c b/hw/sparc64/sparc64.c > index 72f0849f50..3091cde586 100644 > --- a/hw/sparc64/sparc64.c > +++ b/hw/sparc64/sparc64.c > @@ -24,6 +24,7 @@ > > > #include "qemu/osdep.h" > +#include "qapi/error.h" > #include "cpu.h" > #include "hw/boards.h" > #include "hw/sparc/sparc64.h" > @@ -271,9 +272,10 @@ SPARCCPU *sparc64_cpu_devinit(const char *cpu_type, uint64_t prom_addr) > uint32_t stick_frequency = 100 * 1000000; > uint32_t hstick_frequency = 100 * 1000000; > > - cpu = SPARC_CPU(cpu_create(cpu_type)); > + cpu = SPARC_CPU(object_new(cpu_type)); > qdev_init_gpio_in_named(DEVICE(cpu), sparc64_cpu_set_ivec_irq, > "ivec-irq", IVEC_MAX); > + qdev_realize(DEVICE(cpu), NULL, &error_fatal); > env = &cpu->env; > > env->tick = cpu_timer_create("tick", cpu, tick_irq, Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> ATB, Mark.
On 09/02/2024 11:34, Peter Maydell wrote: > On Thu, 8 Feb 2024 at 18:14, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: >> >> Inline cpu_create() in order to call >> qdev_init_gpio_in_named_with_opaque() >> before the CPU is realized. >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> --- >> hw/sparc64/sparc64.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/hw/sparc64/sparc64.c b/hw/sparc64/sparc64.c >> index 72f0849f50..3091cde586 100644 >> --- a/hw/sparc64/sparc64.c >> +++ b/hw/sparc64/sparc64.c >> @@ -24,6 +24,7 @@ >> >> >> #include "qemu/osdep.h" >> +#include "qapi/error.h" >> #include "cpu.h" >> #include "hw/boards.h" >> #include "hw/sparc/sparc64.h" >> @@ -271,9 +272,10 @@ SPARCCPU *sparc64_cpu_devinit(const char *cpu_type, uint64_t prom_addr) >> uint32_t stick_frequency = 100 * 1000000; >> uint32_t hstick_frequency = 100 * 1000000; >> >> - cpu = SPARC_CPU(cpu_create(cpu_type)); >> + cpu = SPARC_CPU(object_new(cpu_type)); >> qdev_init_gpio_in_named(DEVICE(cpu), sparc64_cpu_set_ivec_irq, >> "ivec-irq", IVEC_MAX); >> + qdev_realize(DEVICE(cpu), NULL, &error_fatal); >> env = &cpu->env; >> >> env->tick = cpu_timer_create("tick", cpu, tick_irq, >> -- >> 2.41.0 > > For the purposes of letting us enforce the "init GPIOs > before realize, not after" rule, > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> > > but it looks like this code is adding a GPIO to a > device from code that's not actually part of the > implementation of the device. Ideally most of the code in > this file should be rolled into the CPU itself in target/sparc. I suspect the reason the code is arranged like this is because IVECs aren't part of the core SPARC 64-bit architecture specification, although they happen to be implemented by the CPUs used by QEMU. Perhaps this would be better be handled on a CPU model basis, but I agree this shouldn't be a blocker for this patch. ATB, Mark.
On 9/2/24 23:01, Mark Cave-Ayland wrote: > On 09/02/2024 11:34, Peter Maydell wrote: > >> On Thu, 8 Feb 2024 at 18:14, Philippe Mathieu-Daudé >> <philmd@linaro.org> wrote: >>> >>> Inline cpu_create() in order to call >>> qdev_init_gpio_in_named_with_opaque() >>> before the CPU is realized. >>> >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>> --- >>> hw/sparc64/sparc64.c | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/hw/sparc64/sparc64.c b/hw/sparc64/sparc64.c >>> index 72f0849f50..3091cde586 100644 >>> --- a/hw/sparc64/sparc64.c >>> +++ b/hw/sparc64/sparc64.c >>> @@ -24,6 +24,7 @@ >>> >>> >>> #include "qemu/osdep.h" >>> +#include "qapi/error.h" >>> #include "cpu.h" >>> #include "hw/boards.h" >>> #include "hw/sparc/sparc64.h" >>> @@ -271,9 +272,10 @@ SPARCCPU *sparc64_cpu_devinit(const char >>> *cpu_type, uint64_t prom_addr) >>> uint32_t stick_frequency = 100 * 1000000; >>> uint32_t hstick_frequency = 100 * 1000000; >>> >>> - cpu = SPARC_CPU(cpu_create(cpu_type)); >>> + cpu = SPARC_CPU(object_new(cpu_type)); >>> qdev_init_gpio_in_named(DEVICE(cpu), sparc64_cpu_set_ivec_irq, >>> "ivec-irq", IVEC_MAX); >>> + qdev_realize(DEVICE(cpu), NULL, &error_fatal); >>> env = &cpu->env; >>> >>> env->tick = cpu_timer_create("tick", cpu, tick_irq, >>> -- >>> 2.41.0 >> >> For the purposes of letting us enforce the "init GPIOs >> before realize, not after" rule, >> Reviewed-by: Peter Maydell <peter.maydell@linaro.org> >> >> but it looks like this code is adding a GPIO to a >> device from code that's not actually part of the >> implementation of the device. Ideally most of the code in >> this file should be rolled into the CPU itself in target/sparc. > > I suspect the reason the code is arranged like this is because IVECs > aren't part of the core SPARC 64-bit architecture specification, > although they happen to be implemented by the CPUs used by QEMU. Perhaps > this would be better be handled on a CPU model basis, but I agree this > shouldn't be a blocker for this patch. Suggestion recorded as https://gitlab.com/qemu-project/qemu/-/issues/2163. Thanks both! Phil.
diff --git a/hw/sparc64/sparc64.c b/hw/sparc64/sparc64.c index 72f0849f50..3091cde586 100644 --- a/hw/sparc64/sparc64.c +++ b/hw/sparc64/sparc64.c @@ -24,6 +24,7 @@ #include "qemu/osdep.h" +#include "qapi/error.h" #include "cpu.h" #include "hw/boards.h" #include "hw/sparc/sparc64.h" @@ -271,9 +272,10 @@ SPARCCPU *sparc64_cpu_devinit(const char *cpu_type, uint64_t prom_addr) uint32_t stick_frequency = 100 * 1000000; uint32_t hstick_frequency = 100 * 1000000; - cpu = SPARC_CPU(cpu_create(cpu_type)); + cpu = SPARC_CPU(object_new(cpu_type)); qdev_init_gpio_in_named(DEVICE(cpu), sparc64_cpu_set_ivec_irq, "ivec-irq", IVEC_MAX); + qdev_realize(DEVICE(cpu), NULL, &error_fatal); env = &cpu->env; env->tick = cpu_timer_create("tick", cpu, tick_irq,
Inline cpu_create() in order to call qdev_init_gpio_in_named_with_opaque() before the CPU is realized. Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- hw/sparc64/sparc64.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)