| Message ID | 20220808102734.133084-8-clg@kaod.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ppc: QOM'ify 405 board | expand |
Patch title is wrong. It should be Embed CPU object in SoC as it's not QOMifies the CPU just moves it from dinamically allocated to embedded. On Mon, 8 Aug 2022, Cédric Le Goater wrote: > Drop the use of ppc4xx_init() and duplicate a bit of code related to > clocks in the SoC realize routine. We will clean that up in the > following patches. Could this be split off into a separate patch? Maybe it would be clearer that way what's related to stop using ppc4xx_init() (which is needed because it dinamically allocates CPU) and what's the embedding it in the soc object. > ppc_dcr_init() simply allocates default DCR handlers for the CPU. Maybe > this could be done in model initializer of the CPU families needing it. > > Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> > Signed-off-by: Cédric Le Goater <clg@kaod.org> > --- > hw/ppc/ppc405.h | 2 +- > include/hw/ppc/ppc4xx.h | 1 + > hw/ppc/ppc405_boards.c | 2 +- > hw/ppc/ppc405_uc.c | 35 +++++++++++++++++++++++++---------- > hw/ppc/ppc4xx_devs.c | 2 +- > 5 files changed, 29 insertions(+), 13 deletions(-) > > diff --git a/hw/ppc/ppc405.h b/hw/ppc/ppc405.h > index dc862bc8614c..8cc76cc8b3fe 100644 > --- a/hw/ppc/ppc405.h > +++ b/hw/ppc/ppc405.h > @@ -79,7 +79,7 @@ struct Ppc405SoCState { > hwaddr ram_size; > > uint32_t sysclk; > - PowerPCCPU *cpu; > + PowerPCCPU cpu; > DeviceState *uic; > }; > > diff --git a/include/hw/ppc/ppc4xx.h b/include/hw/ppc/ppc4xx.h > index 980f964b5a91..021376c2d260 100644 > --- a/include/hw/ppc/ppc4xx.h > +++ b/include/hw/ppc/ppc4xx.h > @@ -29,6 +29,7 @@ > #include "exec/memory.h" > > /* PowerPC 4xx core initialization */ > +void ppc4xx_reset(void *opaque); > PowerPCCPU *ppc4xx_init(const char *cpu_model, > clk_setup_t *cpu_clk, clk_setup_t *tb_clk, > uint32_t sysclk); > diff --git a/hw/ppc/ppc405_boards.c b/hw/ppc/ppc405_boards.c > index 0b39ff08bd65..5ba12d60bc00 100644 > --- a/hw/ppc/ppc405_boards.c > +++ b/hw/ppc/ppc405_boards.c > @@ -313,7 +313,7 @@ static void ppc405_init(MachineState *machine) > > /* Load ELF kernel and rootfs.cpio */ > } else if (kernel_filename && !machine->firmware) { > - boot_from_kernel(machine, ppc405->soc.cpu); > + boot_from_kernel(machine, &ppc405->soc.cpu); > } > } > > diff --git a/hw/ppc/ppc405_uc.c b/hw/ppc/ppc405_uc.c > index abcc2537140c..fa3853df2233 100644 > --- a/hw/ppc/ppc405_uc.c > +++ b/hw/ppc/ppc405_uc.c > @@ -1432,22 +1432,36 @@ static void ppc405ep_cpc_init (CPUPPCState *env, clk_setup_t clk_setup[8], > #endif > } > > +static void ppc405_soc_instance_init(Object *obj) > +{ > + Ppc405SoCState *s = PPC405_SOC(obj); > + > + object_initialize_child(obj, "cpu", &s->cpu, > + POWERPC_CPU_TYPE_NAME("405ep")); > +} > + > static void ppc405_soc_realize(DeviceState *dev, Error **errp) > { > Ppc405SoCState *s = PPC405_SOC(dev); > - clk_setup_t clk_setup[PPC405EP_CLK_NB], tlb_clk_setup; > + clk_setup_t clk_setup[PPC405EP_CLK_NB]; > qemu_irq dma_irqs[4], gpt_irqs[5], mal_irqs[4]; > CPUPPCState *env; > > memset(clk_setup, 0, sizeof(clk_setup)); > > /* init CPUs */ > - s->cpu = ppc4xx_init(POWERPC_CPU_TYPE_NAME("405ep"), > - &clk_setup[PPC405EP_CPU_CLK], > - &tlb_clk_setup, s->sysclk); > - env = &s->cpu->env; > - clk_setup[PPC405EP_CPU_CLK].cb = tlb_clk_setup.cb; > - clk_setup[PPC405EP_CPU_CLK].opaque = tlb_clk_setup.opaque; > + if (!qdev_realize(DEVICE(&s->cpu), NULL, errp)) { > + return; > + } > + qemu_register_reset(ppc4xx_reset, &s->cpu); > + > + env = &s->cpu.env; > + > + clk_setup[PPC405EP_CPU_CLK].cb = > + ppc_40x_timers_init(env, s->sysclk, PPC_INTERRUPT_PIT); > + clk_setup[PPC405EP_CPU_CLK].opaque = env; > + > + ppc_dcr_init(env, NULL, NULL); > > /* CPU control */ > ppc405ep_cpc_init(env, clk_setup, s->sysclk); > @@ -1464,16 +1478,16 @@ static void ppc405_soc_realize(DeviceState *dev, Error **errp) > /* Universal interrupt controller */ > s->uic = qdev_new(TYPE_PPC_UIC); > > - object_property_set_link(OBJECT(s->uic), "cpu", OBJECT(s->cpu), > + object_property_set_link(OBJECT(s->uic), "cpu", OBJECT(&s->cpu), > &error_fatal); > if (!sysbus_realize(SYS_BUS_DEVICE(s->uic), errp)) { > return; > } > > sysbus_connect_irq(SYS_BUS_DEVICE(s->uic), PPCUIC_OUTPUT_INT, > - qdev_get_gpio_in(DEVICE(s->cpu), PPC40x_INPUT_INT)); > + qdev_get_gpio_in(DEVICE(&s->cpu), PPC40x_INPUT_INT)); > sysbus_connect_irq(SYS_BUS_DEVICE(s->uic), PPCUIC_OUTPUT_CINT, > - qdev_get_gpio_in(DEVICE(s->cpu), PPC40x_INPUT_CINT)); > + qdev_get_gpio_in(DEVICE(&s->cpu), PPC40x_INPUT_CINT)); > > /* SDRAM controller */ > /* XXX 405EP has no ECC interrupt */ > @@ -1562,6 +1576,7 @@ static const TypeInfo ppc405_types[] = { > .name = TYPE_PPC405_SOC, > .parent = TYPE_DEVICE, > .instance_size = sizeof(Ppc405SoCState), > + .instance_init = ppc405_soc_instance_init, > .class_init = ppc405_soc_class_init, > } > }; > diff --git a/hw/ppc/ppc4xx_devs.c b/hw/ppc/ppc4xx_devs.c > index 737c0896b4f8..f20098cf417c 100644 > --- a/hw/ppc/ppc4xx_devs.c > +++ b/hw/ppc/ppc4xx_devs.c > @@ -37,7 +37,7 @@ > #include "qapi/error.h" > #include "trace.h" > > -static void ppc4xx_reset(void *opaque) > +void ppc4xx_reset(void *opaque) > { > PowerPCCPU *cpu = opaque; This just calls cpu_reset() and does nothing else. Can't that be registered directly so this could be kept static to this file? Why do we need this at all? Isn't the cpu object reset automatically? Why do we need to register it separately? Regards, BALATON Zoltan
On 8/8/22 15:17, BALATON Zoltan wrote: > > Patch title is wrong. It should be Embed CPU object in SoC as it's not QOMifies the CPU just moves it from dinamically allocated to embedded. > > On Mon, 8 Aug 2022, Cédric Le Goater wrote: >> Drop the use of ppc4xx_init() and duplicate a bit of code related to >> clocks in the SoC realize routine. We will clean that up in the >> following patches. > > Could this be split off into a separate patch? Maybe it would be clearer that way what's related to stop using ppc4xx_init() (which is needed because it dinamically allocates CPU) and what's the embedding it in the soc object. I'd rather not. It has been painful enough to untangle. Let's keep it that way. And, this part is further changed in the CPC patch. >> ppc_dcr_init() simply allocates default DCR handlers for the CPU. Maybe >> this could be done in model initializer of the CPU families needing it. >> >> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> >> Signed-off-by: Cédric Le Goater <clg@kaod.org> >> --- >> hw/ppc/ppc405.h | 2 +- >> include/hw/ppc/ppc4xx.h | 1 + >> hw/ppc/ppc405_boards.c | 2 +- >> hw/ppc/ppc405_uc.c | 35 +++++++++++++++++++++++++---------- >> hw/ppc/ppc4xx_devs.c | 2 +- >> 5 files changed, 29 insertions(+), 13 deletions(-) >> >> diff --git a/hw/ppc/ppc405.h b/hw/ppc/ppc405.h >> index dc862bc8614c..8cc76cc8b3fe 100644 >> --- a/hw/ppc/ppc405.h >> +++ b/hw/ppc/ppc405.h >> @@ -79,7 +79,7 @@ struct Ppc405SoCState { >> hwaddr ram_size; >> >> uint32_t sysclk; >> - PowerPCCPU *cpu; >> + PowerPCCPU cpu; >> DeviceState *uic; >> }; >> >> diff --git a/include/hw/ppc/ppc4xx.h b/include/hw/ppc/ppc4xx.h >> index 980f964b5a91..021376c2d260 100644 >> --- a/include/hw/ppc/ppc4xx.h >> +++ b/include/hw/ppc/ppc4xx.h >> @@ -29,6 +29,7 @@ >> #include "exec/memory.h" >> >> /* PowerPC 4xx core initialization */ >> +void ppc4xx_reset(void *opaque); >> PowerPCCPU *ppc4xx_init(const char *cpu_model, >> clk_setup_t *cpu_clk, clk_setup_t *tb_clk, >> uint32_t sysclk); >> diff --git a/hw/ppc/ppc405_boards.c b/hw/ppc/ppc405_boards.c >> index 0b39ff08bd65..5ba12d60bc00 100644 >> --- a/hw/ppc/ppc405_boards.c >> +++ b/hw/ppc/ppc405_boards.c >> @@ -313,7 +313,7 @@ static void ppc405_init(MachineState *machine) >> >> /* Load ELF kernel and rootfs.cpio */ >> } else if (kernel_filename && !machine->firmware) { >> - boot_from_kernel(machine, ppc405->soc.cpu); >> + boot_from_kernel(machine, &ppc405->soc.cpu); >> } >> } >> >> diff --git a/hw/ppc/ppc405_uc.c b/hw/ppc/ppc405_uc.c >> index abcc2537140c..fa3853df2233 100644 >> --- a/hw/ppc/ppc405_uc.c >> +++ b/hw/ppc/ppc405_uc.c >> @@ -1432,22 +1432,36 @@ static void ppc405ep_cpc_init (CPUPPCState *env, clk_setup_t clk_setup[8], >> #endif >> } >> >> +static void ppc405_soc_instance_init(Object *obj) >> +{ >> + Ppc405SoCState *s = PPC405_SOC(obj); >> + >> + object_initialize_child(obj, "cpu", &s->cpu, >> + POWERPC_CPU_TYPE_NAME("405ep")); >> +} >> + >> static void ppc405_soc_realize(DeviceState *dev, Error **errp) >> { >> Ppc405SoCState *s = PPC405_SOC(dev); >> - clk_setup_t clk_setup[PPC405EP_CLK_NB], tlb_clk_setup; >> + clk_setup_t clk_setup[PPC405EP_CLK_NB]; >> qemu_irq dma_irqs[4], gpt_irqs[5], mal_irqs[4]; >> CPUPPCState *env; >> >> memset(clk_setup, 0, sizeof(clk_setup)); >> >> /* init CPUs */ >> - s->cpu = ppc4xx_init(POWERPC_CPU_TYPE_NAME("405ep"), >> - &clk_setup[PPC405EP_CPU_CLK], >> - &tlb_clk_setup, s->sysclk); >> - env = &s->cpu->env; >> - clk_setup[PPC405EP_CPU_CLK].cb = tlb_clk_setup.cb; >> - clk_setup[PPC405EP_CPU_CLK].opaque = tlb_clk_setup.opaque; >> + if (!qdev_realize(DEVICE(&s->cpu), NULL, errp)) { >> + return; >> + } >> + qemu_register_reset(ppc4xx_reset, &s->cpu); >> + >> + env = &s->cpu.env; >> + >> + clk_setup[PPC405EP_CPU_CLK].cb = >> + ppc_40x_timers_init(env, s->sysclk, PPC_INTERRUPT_PIT); >> + clk_setup[PPC405EP_CPU_CLK].opaque = env; >> + >> + ppc_dcr_init(env, NULL, NULL); >> >> /* CPU control */ >> ppc405ep_cpc_init(env, clk_setup, s->sysclk); >> @@ -1464,16 +1478,16 @@ static void ppc405_soc_realize(DeviceState *dev, Error **errp) >> /* Universal interrupt controller */ >> s->uic = qdev_new(TYPE_PPC_UIC); >> >> - object_property_set_link(OBJECT(s->uic), "cpu", OBJECT(s->cpu), >> + object_property_set_link(OBJECT(s->uic), "cpu", OBJECT(&s->cpu), >> &error_fatal); >> if (!sysbus_realize(SYS_BUS_DEVICE(s->uic), errp)) { >> return; >> } >> >> sysbus_connect_irq(SYS_BUS_DEVICE(s->uic), PPCUIC_OUTPUT_INT, >> - qdev_get_gpio_in(DEVICE(s->cpu), PPC40x_INPUT_INT)); >> + qdev_get_gpio_in(DEVICE(&s->cpu), PPC40x_INPUT_INT)); >> sysbus_connect_irq(SYS_BUS_DEVICE(s->uic), PPCUIC_OUTPUT_CINT, >> - qdev_get_gpio_in(DEVICE(s->cpu), PPC40x_INPUT_CINT)); >> + qdev_get_gpio_in(DEVICE(&s->cpu), PPC40x_INPUT_CINT)); >> >> /* SDRAM controller */ >> /* XXX 405EP has no ECC interrupt */ >> @@ -1562,6 +1576,7 @@ static const TypeInfo ppc405_types[] = { >> .name = TYPE_PPC405_SOC, >> .parent = TYPE_DEVICE, >> .instance_size = sizeof(Ppc405SoCState), >> + .instance_init = ppc405_soc_instance_init, >> .class_init = ppc405_soc_class_init, >> } >> }; >> diff --git a/hw/ppc/ppc4xx_devs.c b/hw/ppc/ppc4xx_devs.c >> index 737c0896b4f8..f20098cf417c 100644 >> --- a/hw/ppc/ppc4xx_devs.c >> +++ b/hw/ppc/ppc4xx_devs.c >> @@ -37,7 +37,7 @@ >> #include "qapi/error.h" >> #include "trace.h" >> >> -static void ppc4xx_reset(void *opaque) >> +void ppc4xx_reset(void *opaque) >> { >> PowerPCCPU *cpu = opaque; > > This just calls cpu_reset() and does nothing else. Can't that be registered directly so this could be kept static to this file? what do you mean ? > Why do we need this at all? Oh yes. We need the CPU to start in a reset state, for U-boot at least. > Isn't the cpu object reset automatically? Why do we need to register it separately? All devices need a reset handler. The handler of the PPC405 CPU is registered in ppc405_soc_realize() : qemu_register_reset(ppc4xx_reset, &s->cpu); If we had a more complex model, like pseries or PowerNV, we would install the reset handler in the realize routine of the CPU model. But we don't. Thanks, C. > > Regards, > BALATON Zoltan
On Mon, 8 Aug 2022, Cédric Le Goater wrote: > On 8/8/22 15:17, BALATON Zoltan wrote: >> >> Patch title is wrong. It should be Embed CPU object in SoC as it's not >> QOMifies the CPU just moves it from dinamically allocated to embedded. >> >> On Mon, 8 Aug 2022, Cédric Le Goater wrote: >>> Drop the use of ppc4xx_init() and duplicate a bit of code related to >>> clocks in the SoC realize routine. We will clean that up in the >>> following patches. >> >> Could this be split off into a separate patch? Maybe it would be clearer >> that way what's related to stop using ppc4xx_init() (which is needed >> because it dinamically allocates CPU) and what's the embedding it in the >> soc object. > > I'd rather not. It has been painful enough to untangle. Let's keep it that > way. And, this part is further changed in the CPC patch. OK, if nobody else thinks this should be split I'm OK with it. >>> ppc_dcr_init() simply allocates default DCR handlers for the CPU. Maybe >>> this could be done in model initializer of the CPU families needing it. >>> >>> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> >>> Signed-off-by: Cédric Le Goater <clg@kaod.org> >>> --- >>> hw/ppc/ppc405.h | 2 +- >>> include/hw/ppc/ppc4xx.h | 1 + >>> hw/ppc/ppc405_boards.c | 2 +- >>> hw/ppc/ppc405_uc.c | 35 +++++++++++++++++++++++++---------- >>> hw/ppc/ppc4xx_devs.c | 2 +- >>> 5 files changed, 29 insertions(+), 13 deletions(-) >>> >>> diff --git a/hw/ppc/ppc405.h b/hw/ppc/ppc405.h >>> index dc862bc8614c..8cc76cc8b3fe 100644 >>> --- a/hw/ppc/ppc405.h >>> +++ b/hw/ppc/ppc405.h >>> @@ -79,7 +79,7 @@ struct Ppc405SoCState { >>> hwaddr ram_size; >>> >>> uint32_t sysclk; >>> - PowerPCCPU *cpu; >>> + PowerPCCPU cpu; >>> DeviceState *uic; >>> }; >>> >>> diff --git a/include/hw/ppc/ppc4xx.h b/include/hw/ppc/ppc4xx.h >>> index 980f964b5a91..021376c2d260 100644 >>> --- a/include/hw/ppc/ppc4xx.h >>> +++ b/include/hw/ppc/ppc4xx.h >>> @@ -29,6 +29,7 @@ >>> #include "exec/memory.h" >>> >>> /* PowerPC 4xx core initialization */ >>> +void ppc4xx_reset(void *opaque); >>> PowerPCCPU *ppc4xx_init(const char *cpu_model, >>> clk_setup_t *cpu_clk, clk_setup_t *tb_clk, >>> uint32_t sysclk); >>> diff --git a/hw/ppc/ppc405_boards.c b/hw/ppc/ppc405_boards.c >>> index 0b39ff08bd65..5ba12d60bc00 100644 >>> --- a/hw/ppc/ppc405_boards.c >>> +++ b/hw/ppc/ppc405_boards.c >>> @@ -313,7 +313,7 @@ static void ppc405_init(MachineState *machine) >>> >>> /* Load ELF kernel and rootfs.cpio */ >>> } else if (kernel_filename && !machine->firmware) { >>> - boot_from_kernel(machine, ppc405->soc.cpu); >>> + boot_from_kernel(machine, &ppc405->soc.cpu); >>> } >>> } >>> >>> diff --git a/hw/ppc/ppc405_uc.c b/hw/ppc/ppc405_uc.c >>> index abcc2537140c..fa3853df2233 100644 >>> --- a/hw/ppc/ppc405_uc.c >>> +++ b/hw/ppc/ppc405_uc.c >>> @@ -1432,22 +1432,36 @@ static void ppc405ep_cpc_init (CPUPPCState *env, >>> clk_setup_t clk_setup[8], >>> #endif >>> } >>> >>> +static void ppc405_soc_instance_init(Object *obj) >>> +{ >>> + Ppc405SoCState *s = PPC405_SOC(obj); >>> + >>> + object_initialize_child(obj, "cpu", &s->cpu, >>> + POWERPC_CPU_TYPE_NAME("405ep")); >>> +} >>> + >>> static void ppc405_soc_realize(DeviceState *dev, Error **errp) >>> { >>> Ppc405SoCState *s = PPC405_SOC(dev); >>> - clk_setup_t clk_setup[PPC405EP_CLK_NB], tlb_clk_setup; >>> + clk_setup_t clk_setup[PPC405EP_CLK_NB]; >>> qemu_irq dma_irqs[4], gpt_irqs[5], mal_irqs[4]; >>> CPUPPCState *env; >>> >>> memset(clk_setup, 0, sizeof(clk_setup)); >>> >>> /* init CPUs */ >>> - s->cpu = ppc4xx_init(POWERPC_CPU_TYPE_NAME("405ep"), >>> - &clk_setup[PPC405EP_CPU_CLK], >>> - &tlb_clk_setup, s->sysclk); >>> - env = &s->cpu->env; >>> - clk_setup[PPC405EP_CPU_CLK].cb = tlb_clk_setup.cb; >>> - clk_setup[PPC405EP_CPU_CLK].opaque = tlb_clk_setup.opaque; >>> + if (!qdev_realize(DEVICE(&s->cpu), NULL, errp)) { >>> + return; >>> + } >>> + qemu_register_reset(ppc4xx_reset, &s->cpu); >>> + >>> + env = &s->cpu.env; >>> + >>> + clk_setup[PPC405EP_CPU_CLK].cb = >>> + ppc_40x_timers_init(env, s->sysclk, PPC_INTERRUPT_PIT); >>> + clk_setup[PPC405EP_CPU_CLK].opaque = env; >>> + >>> + ppc_dcr_init(env, NULL, NULL); >>> >>> /* CPU control */ >>> ppc405ep_cpc_init(env, clk_setup, s->sysclk); >>> @@ -1464,16 +1478,16 @@ static void ppc405_soc_realize(DeviceState *dev, >>> Error **errp) >>> /* Universal interrupt controller */ >>> s->uic = qdev_new(TYPE_PPC_UIC); >>> >>> - object_property_set_link(OBJECT(s->uic), "cpu", OBJECT(s->cpu), >>> + object_property_set_link(OBJECT(s->uic), "cpu", OBJECT(&s->cpu), >>> &error_fatal); >>> if (!sysbus_realize(SYS_BUS_DEVICE(s->uic), errp)) { >>> return; >>> } >>> >>> sysbus_connect_irq(SYS_BUS_DEVICE(s->uic), PPCUIC_OUTPUT_INT, >>> - qdev_get_gpio_in(DEVICE(s->cpu), >>> PPC40x_INPUT_INT)); >>> + qdev_get_gpio_in(DEVICE(&s->cpu), >>> PPC40x_INPUT_INT)); >>> sysbus_connect_irq(SYS_BUS_DEVICE(s->uic), PPCUIC_OUTPUT_CINT, >>> - qdev_get_gpio_in(DEVICE(s->cpu), >>> PPC40x_INPUT_CINT)); >>> + qdev_get_gpio_in(DEVICE(&s->cpu), >>> PPC40x_INPUT_CINT)); >>> >>> /* SDRAM controller */ >>> /* XXX 405EP has no ECC interrupt */ >>> @@ -1562,6 +1576,7 @@ static const TypeInfo ppc405_types[] = { >>> .name = TYPE_PPC405_SOC, >>> .parent = TYPE_DEVICE, >>> .instance_size = sizeof(Ppc405SoCState), >>> + .instance_init = ppc405_soc_instance_init, >>> .class_init = ppc405_soc_class_init, >>> } >>> }; >>> diff --git a/hw/ppc/ppc4xx_devs.c b/hw/ppc/ppc4xx_devs.c >>> index 737c0896b4f8..f20098cf417c 100644 >>> --- a/hw/ppc/ppc4xx_devs.c >>> +++ b/hw/ppc/ppc4xx_devs.c >>> @@ -37,7 +37,7 @@ >>> #include "qapi/error.h" >>> #include "trace.h" >>> >>> -static void ppc4xx_reset(void *opaque) >>> +void ppc4xx_reset(void *opaque) >>> { >>> PowerPCCPU *cpu = opaque; >> >> This just calls cpu_reset() and does nothing else. Can't that be registered >> directly so this could be kept static to this file? > > what do you mean ? Something like qemu_register_reset(cpu_reset, &s->cpu); but I'm not sure this is even needed. >> Why do we need this at all? > > Oh yes. We need the CPU to start in a reset state, for U-boot at least. > >> Isn't the cpu object reset automatically? Why do we need to register it >> separately? > > All devices need a reset handler. The handler of the PPC405 CPU is registered > in ppc405_soc_realize() : > > qemu_register_reset(ppc4xx_reset, &s->cpu); But the handler we register here just calls cpu_reset which seems to just call the reset method of the CPU object. If we have nothing else to do here do we need to explicitly call cpi_reset like this? Wouldn't the CPU object be reset by qdev when resetting the machine or the soc its in? If we have our own reset method we may call cpu_reset from there to make sure the CPU is in a known state but is this needed when we don't want to do anything else? I don't know how reset handling works but some machines seems to do this and others don't. > If we had a more complex model, like pseries or PowerNV, we would install the > reset handler in the realize routine of the CPU model. But we don't. If this is needed maybe you should add a soc_reset method and call cpu_reset() from there and get rid of ppc4xx_reset or move it to ppc405_uc.c if it's not used anywhere else, so it can be kept static and does not need to be added to ppc4xx.h. Regards, BALATON Zoltan
On Mon, 8 Aug 2022 at 18:05, BALATON Zoltan <balaton@eik.bme.hu> wrote: > But the handler we register here just calls cpu_reset which seems to just > call the reset method of the CPU object. If we have nothing else to do > here do we need to explicitly call cpi_reset like this? Wouldn't the CPU > object be reset by qdev when resetting the machine or the soc its in? If > we have our own reset method we may call cpu_reset from there to make sure > the CPU is in a known state but is this needed when we don't want to do > anything else? I don't know how reset handling works but some machines > seems to do this and others don't. You do unfortunately need to manually reset the CPU object. This is because the 'automatic' qdev reset only works for devices that hang off a bus (including sysbus devices). This is because it works by having qdev_machine_creation_done() register a reset function which does "reset the sysbus". Resetting a bus resets every device on it. Resetting a device resets every bus it owns. (This means that for instance PCI devices get reset because the PCI controller is on the sysbus, so it gets reset, and it owns the PCI bus, so the PCI bus resets when the controller is reset, and when the PCI bus resets then all the devices on it are reset.) So reset propagates down the bus tree, but it won't reach devices which aren't on that bus tree at all. The most common case of "device which isn't on a bus" is the CPU objects. This is, clearly, a complete mess. I would strongly like to sort it out, but to do that we need (a) a plan for what reset handling ought to look like and (b) a transition plan for getting from here to there without breaking everything in the process. I haven't had the time to focus on that, though it's been nominally on my todo list for years. In the meantime, it's the responsibility of something in architecture specific code to arrange for the CPU objects to be reset -- whether that's done directly by the board, or hidden in some other layer of code, depends on the target arch. For instance for arm it's hidden inside arm_load_kernel() and board models must therefore call that function even if they have no intention of supporting loading Linux kernels. For sparc it's done directly in the board code. thanks -- PMM
On Mon, 8 Aug 2022, Peter Maydell wrote: > On Mon, 8 Aug 2022 at 18:05, BALATON Zoltan <balaton@eik.bme.hu> wrote: >> But the handler we register here just calls cpu_reset which seems to just >> call the reset method of the CPU object. If we have nothing else to do >> here do we need to explicitly call cpi_reset like this? Wouldn't the CPU >> object be reset by qdev when resetting the machine or the soc its in? If >> we have our own reset method we may call cpu_reset from there to make sure >> the CPU is in a known state but is this needed when we don't want to do >> anything else? I don't know how reset handling works but some machines >> seems to do this and others don't. > > You do unfortunately need to manually reset the CPU object. This is > because the 'automatic' qdev reset only works for devices that hang > off a bus (including sysbus devices). This is because it works by > having qdev_machine_creation_done() register a reset function which > does "reset the sysbus". Resetting a bus resets every device on it. > Resetting a device resets every bus it owns. (This means that for > instance PCI devices get reset because the PCI controller is on the > sysbus, so it gets reset, and it owns the PCI bus, so the PCI bus > resets when the controller is reset, and when the PCI bus resets > then all the devices on it are reset.) So reset propagates down > the bus tree, but it won't reach devices which aren't on that bus > tree at all. The most common case of "device which isn't on a bus" > is the CPU objects. > > This is, clearly, a complete mess. I would strongly like to sort it > out, but to do that we need (a) a plan for what reset handling ought > to look like and (b) a transition plan for getting from here to there > without breaking everything in the process. I haven't had the time > to focus on that, though it's been nominally on my todo list for years. > > In the meantime, it's the responsibility of something in architecture > specific code to arrange for the CPU objects to be reset -- whether > that's done directly by the board, or hidden in some other layer > of code, depends on the target arch. For instance for arm it's > hidden inside arm_load_kernel() and board models must therefore > call that function even if they have no intention of supporting > loading Linux kernels. For sparc it's done directly in the board code. OK thanks for the clarification. Both bamboo and sam460ex has a main_cpu_reset function that is registered from the machine init func which calls cpu_reset. Maybe 405 boards could do the same for consistency? It could also be added to the soc object's reset method as that owns the cpu so maybe it may make more sense that way. This is probably also OK as 440 machines don't have soc object yet so it wouldn't cause any problems. I think the ppc4xx_reset func was kept around because ppc4xx_init was also kept but nothing else seems to use it so maybe these should become the soc object's realize and reset methods and drop the current ppc4xx_init and reset funcs? Regards, BALATON Zoltan
On 8/8/22 19:25, BALATON Zoltan wrote: > On Mon, 8 Aug 2022, Peter Maydell wrote: >> On Mon, 8 Aug 2022 at 18:05, BALATON Zoltan <balaton@eik.bme.hu> wrote: >>> But the handler we register here just calls cpu_reset which seems to just >>> call the reset method of the CPU object. If we have nothing else to do >>> here do we need to explicitly call cpi_reset like this? Wouldn't the CPU >>> object be reset by qdev when resetting the machine or the soc its in? If >>> we have our own reset method we may call cpu_reset from there to make sure >>> the CPU is in a known state but is this needed when we don't want to do >>> anything else? I don't know how reset handling works but some machines >>> seems to do this and others don't. >> >> You do unfortunately need to manually reset the CPU object. This is >> because the 'automatic' qdev reset only works for devices that hang >> off a bus (including sysbus devices). This is because it works by >> having qdev_machine_creation_done() register a reset function which >> does "reset the sysbus". Resetting a bus resets every device on it. >> Resetting a device resets every bus it owns. (This means that for >> instance PCI devices get reset because the PCI controller is on the >> sysbus, so it gets reset, and it owns the PCI bus, so the PCI bus >> resets when the controller is reset, and when the PCI bus resets >> then all the devices on it are reset.) So reset propagates down >> the bus tree, but it won't reach devices which aren't on that bus >> tree at all. The most common case of "device which isn't on a bus" >> is the CPU objects. >> >> This is, clearly, a complete mess. I would strongly like to sort it >> out, but to do that we need (a) a plan for what reset handling ought >> to look like and (b) a transition plan for getting from here to there >> without breaking everything in the process. I haven't had the time >> to focus on that, though it's been nominally on my todo list for years. >> >> In the meantime, it's the responsibility of something in architecture >> specific code to arrange for the CPU objects to be reset -- whether >> that's done directly by the board, or hidden in some other layer >> of code, depends on the target arch. For instance for arm it's >> hidden inside arm_load_kernel() and board models must therefore >> call that function even if they have no intention of supporting >> loading Linux kernels. For sparc it's done directly in the board code. > > OK thanks for the clarification. Both bamboo and sam460ex has a main_cpu_reset function that is registered from the machine init func which calls cpu_reset. Maybe 405 boards could do the same for consistency? It could also be added to the soc object's reset method as that owns the cpu so maybe it may make more sense that way. This is probably also OK as 440 machines don't have soc object yet so it wouldn't cause any problems. > > I think the ppc4xx_reset func was kept around because ppc4xx_init was also kept but nothing else seems to use it so maybe these should become the soc object's realize and reset methods and drop the current ppc4xx_init and reset funcs? Yes. I have moved the CPU reset routine under ppc405_uc.c and removed ppc4xx_init() which is now unused. Thanks, C.
diff --git a/hw/ppc/ppc405.h b/hw/ppc/ppc405.h index dc862bc8614c..8cc76cc8b3fe 100644 --- a/hw/ppc/ppc405.h +++ b/hw/ppc/ppc405.h @@ -79,7 +79,7 @@ struct Ppc405SoCState { hwaddr ram_size; uint32_t sysclk; - PowerPCCPU *cpu; + PowerPCCPU cpu; DeviceState *uic; }; diff --git a/include/hw/ppc/ppc4xx.h b/include/hw/ppc/ppc4xx.h index 980f964b5a91..021376c2d260 100644 --- a/include/hw/ppc/ppc4xx.h +++ b/include/hw/ppc/ppc4xx.h @@ -29,6 +29,7 @@ #include "exec/memory.h" /* PowerPC 4xx core initialization */ +void ppc4xx_reset(void *opaque); PowerPCCPU *ppc4xx_init(const char *cpu_model, clk_setup_t *cpu_clk, clk_setup_t *tb_clk, uint32_t sysclk); diff --git a/hw/ppc/ppc405_boards.c b/hw/ppc/ppc405_boards.c index 0b39ff08bd65..5ba12d60bc00 100644 --- a/hw/ppc/ppc405_boards.c +++ b/hw/ppc/ppc405_boards.c @@ -313,7 +313,7 @@ static void ppc405_init(MachineState *machine) /* Load ELF kernel and rootfs.cpio */ } else if (kernel_filename && !machine->firmware) { - boot_from_kernel(machine, ppc405->soc.cpu); + boot_from_kernel(machine, &ppc405->soc.cpu); } } diff --git a/hw/ppc/ppc405_uc.c b/hw/ppc/ppc405_uc.c index abcc2537140c..fa3853df2233 100644 --- a/hw/ppc/ppc405_uc.c +++ b/hw/ppc/ppc405_uc.c @@ -1432,22 +1432,36 @@ static void ppc405ep_cpc_init (CPUPPCState *env, clk_setup_t clk_setup[8], #endif } +static void ppc405_soc_instance_init(Object *obj) +{ + Ppc405SoCState *s = PPC405_SOC(obj); + + object_initialize_child(obj, "cpu", &s->cpu, + POWERPC_CPU_TYPE_NAME("405ep")); +} + static void ppc405_soc_realize(DeviceState *dev, Error **errp) { Ppc405SoCState *s = PPC405_SOC(dev); - clk_setup_t clk_setup[PPC405EP_CLK_NB], tlb_clk_setup; + clk_setup_t clk_setup[PPC405EP_CLK_NB]; qemu_irq dma_irqs[4], gpt_irqs[5], mal_irqs[4]; CPUPPCState *env; memset(clk_setup, 0, sizeof(clk_setup)); /* init CPUs */ - s->cpu = ppc4xx_init(POWERPC_CPU_TYPE_NAME("405ep"), - &clk_setup[PPC405EP_CPU_CLK], - &tlb_clk_setup, s->sysclk); - env = &s->cpu->env; - clk_setup[PPC405EP_CPU_CLK].cb = tlb_clk_setup.cb; - clk_setup[PPC405EP_CPU_CLK].opaque = tlb_clk_setup.opaque; + if (!qdev_realize(DEVICE(&s->cpu), NULL, errp)) { + return; + } + qemu_register_reset(ppc4xx_reset, &s->cpu); + + env = &s->cpu.env; + + clk_setup[PPC405EP_CPU_CLK].cb = + ppc_40x_timers_init(env, s->sysclk, PPC_INTERRUPT_PIT); + clk_setup[PPC405EP_CPU_CLK].opaque = env; + + ppc_dcr_init(env, NULL, NULL); /* CPU control */ ppc405ep_cpc_init(env, clk_setup, s->sysclk); @@ -1464,16 +1478,16 @@ static void ppc405_soc_realize(DeviceState *dev, Error **errp) /* Universal interrupt controller */ s->uic = qdev_new(TYPE_PPC_UIC); - object_property_set_link(OBJECT(s->uic), "cpu", OBJECT(s->cpu), + object_property_set_link(OBJECT(s->uic), "cpu", OBJECT(&s->cpu), &error_fatal); if (!sysbus_realize(SYS_BUS_DEVICE(s->uic), errp)) { return; } sysbus_connect_irq(SYS_BUS_DEVICE(s->uic), PPCUIC_OUTPUT_INT, - qdev_get_gpio_in(DEVICE(s->cpu), PPC40x_INPUT_INT)); + qdev_get_gpio_in(DEVICE(&s->cpu), PPC40x_INPUT_INT)); sysbus_connect_irq(SYS_BUS_DEVICE(s->uic), PPCUIC_OUTPUT_CINT, - qdev_get_gpio_in(DEVICE(s->cpu), PPC40x_INPUT_CINT)); + qdev_get_gpio_in(DEVICE(&s->cpu), PPC40x_INPUT_CINT)); /* SDRAM controller */ /* XXX 405EP has no ECC interrupt */ @@ -1562,6 +1576,7 @@ static const TypeInfo ppc405_types[] = { .name = TYPE_PPC405_SOC, .parent = TYPE_DEVICE, .instance_size = sizeof(Ppc405SoCState), + .instance_init = ppc405_soc_instance_init, .class_init = ppc405_soc_class_init, } }; diff --git a/hw/ppc/ppc4xx_devs.c b/hw/ppc/ppc4xx_devs.c index 737c0896b4f8..f20098cf417c 100644 --- a/hw/ppc/ppc4xx_devs.c +++ b/hw/ppc/ppc4xx_devs.c @@ -37,7 +37,7 @@ #include "qapi/error.h" #include "trace.h" -static void ppc4xx_reset(void *opaque) +void ppc4xx_reset(void *opaque) { PowerPCCPU *cpu = opaque;