Re: [PATCH v3 07/22] ppc/ppc405: QOM'ify CPU

[Cédric Le Goater]

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