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

[BALATON Zoltan]

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