Re: [PATCH v2 07/20] ppc/ppc405: QOM'ify CPC

[BALATON Zoltan <balaton@eik.bme.hu>]

[-- Attachment #1: Type: text/plain, Size: 10126 bytes --]

On Wed, 3 Aug 2022, Cédric Le Goater wrote:
> Introduce a QOM property "cpu" to initialize the DCR handlers. This is
> a pattern that we will reuse for the all other 405 devices needing it.
>
> Now that all clock settings are handled at the CPC level, change the
> SoC "sys-clk" property to be an alias on the same property in the CPC
> model.
>
> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> hw/ppc/ppc405.h    |  39 +++++++++++++++-
> hw/ppc/ppc405_uc.c | 109 +++++++++++++++++++--------------------------
> 2 files changed, 85 insertions(+), 63 deletions(-)
>
> diff --git a/hw/ppc/ppc405.h b/hw/ppc/ppc405.h
> index ae64549537c6..88c63774d9ba 100644
> --- a/hw/ppc/ppc405.h
> +++ b/hw/ppc/ppc405.h
> @@ -63,6 +63,43 @@ struct ppc4xx_bd_info_t {
>     uint32_t bi_iic_fast[2];
> };
>
> +typedef struct Ppc405SoCState Ppc405SoCState;

This typedef is already done by the OBJECT_DECLARE_SIMPLE_TYPE macro 
below. Could some compilers complain about double typedef? There may be 
some circular dependencies here so to avoid a separate typedef you may 
need to bring the OBJECT_DECLARE_SIMPLE_TYPE(Ppc405SoCState, PPC405_SOC); 
line up here to the front while keeping the actual declaration of the 
state struct and rest of the object later which separates them but adding 
a comment may explain that. I'm not sure if it's better to do that or 
repeating the typedef in advance as done here is better but declaring the 
object in advance is probably a bit cleaner than repeating part of its 
internals just in case this implementation detail ever changes.

Regards,
BALATON Zoltan

> +
> +#define TYPE_PPC405_CPC "ppc405-cpc"
> +OBJECT_DECLARE_SIMPLE_TYPE(Ppc405CpcState, PPC405_CPC);
> +
> +enum {
> +    PPC405EP_CPU_CLK   = 0,
> +    PPC405EP_PLB_CLK   = 1,
> +    PPC405EP_OPB_CLK   = 2,
> +    PPC405EP_EBC_CLK   = 3,
> +    PPC405EP_MAL_CLK   = 4,
> +    PPC405EP_PCI_CLK   = 5,
> +    PPC405EP_UART0_CLK = 6,
> +    PPC405EP_UART1_CLK = 7,
> +    PPC405EP_CLK_NB    = 8,
> +};
> +
> +struct Ppc405CpcState {
> +    DeviceState parent_obj;
> +
> +    PowerPCCPU *cpu;
> +
> +    uint32_t sysclk;
> +    clk_setup_t clk_setup[PPC405EP_CLK_NB];
> +    uint32_t boot;
> +    uint32_t epctl;
> +    uint32_t pllmr[2];
> +    uint32_t ucr;
> +    uint32_t srr;
> +    uint32_t jtagid;
> +    uint32_t pci;
> +    /* Clock and power management */
> +    uint32_t er;
> +    uint32_t fr;
> +    uint32_t sr;
> +};
> +
> #define TYPE_PPC405_SOC "ppc405-soc"
> OBJECT_DECLARE_SIMPLE_TYPE(Ppc405SoCState, PPC405_SOC);
>
> @@ -79,9 +116,9 @@ struct Ppc405SoCState {
>     MemoryRegion *dram_mr;
>     hwaddr ram_size;
>
> -    uint32_t sysclk;
>     PowerPCCPU cpu;
>     DeviceState *uic;
> +    Ppc405CpcState cpc;
> };
>
> /* PowerPC 405 core */
> diff --git a/hw/ppc/ppc405_uc.c b/hw/ppc/ppc405_uc.c
> index 013dccee898b..32bfc9480bc6 100644
> --- a/hw/ppc/ppc405_uc.c
> +++ b/hw/ppc/ppc405_uc.c
> @@ -1178,36 +1178,7 @@ enum {
> #endif
> };
>
> -enum {
> -    PPC405EP_CPU_CLK   = 0,
> -    PPC405EP_PLB_CLK   = 1,
> -    PPC405EP_OPB_CLK   = 2,
> -    PPC405EP_EBC_CLK   = 3,
> -    PPC405EP_MAL_CLK   = 4,
> -    PPC405EP_PCI_CLK   = 5,
> -    PPC405EP_UART0_CLK = 6,
> -    PPC405EP_UART1_CLK = 7,
> -    PPC405EP_CLK_NB    = 8,
> -};
> -
> -typedef struct ppc405ep_cpc_t ppc405ep_cpc_t;
> -struct ppc405ep_cpc_t {
> -    uint32_t sysclk;
> -    clk_setup_t clk_setup[PPC405EP_CLK_NB];
> -    uint32_t boot;
> -    uint32_t epctl;
> -    uint32_t pllmr[2];
> -    uint32_t ucr;
> -    uint32_t srr;
> -    uint32_t jtagid;
> -    uint32_t pci;
> -    /* Clock and power management */
> -    uint32_t er;
> -    uint32_t fr;
> -    uint32_t sr;
> -};
> -
> -static void ppc405ep_compute_clocks (ppc405ep_cpc_t *cpc)
> +static void ppc405ep_compute_clocks(Ppc405CpcState *cpc)
> {
>     uint32_t CPU_clk, PLB_clk, OPB_clk, EBC_clk, MAL_clk, PCI_clk;
>     uint32_t UART0_clk, UART1_clk;
> @@ -1302,10 +1273,9 @@ static void ppc405ep_compute_clocks (ppc405ep_cpc_t *cpc)
>
> static uint32_t dcr_read_epcpc (void *opaque, int dcrn)
> {
> -    ppc405ep_cpc_t *cpc;
> +    Ppc405CpcState *cpc = PPC405_CPC(opaque);
>     uint32_t ret;
>
> -    cpc = opaque;
>     switch (dcrn) {
>     case PPC405EP_CPC0_BOOT:
>         ret = cpc->boot;
> @@ -1342,9 +1312,8 @@ static uint32_t dcr_read_epcpc (void *opaque, int dcrn)
>
> static void dcr_write_epcpc (void *opaque, int dcrn, uint32_t val)
> {
> -    ppc405ep_cpc_t *cpc;
> +    Ppc405CpcState *cpc = PPC405_CPC(opaque);
>
> -    cpc = opaque;
>     switch (dcrn) {
>     case PPC405EP_CPC0_BOOT:
>         /* Read-only register */
> @@ -1377,9 +1346,9 @@ static void dcr_write_epcpc (void *opaque, int dcrn, uint32_t val)
>     }
> }
>
> -static void ppc405ep_cpc_reset (void *opaque)
> +static void ppc405_cpc_reset(DeviceState *dev)
> {
> -    ppc405ep_cpc_t *cpc = opaque;
> +    Ppc405CpcState *cpc = PPC405_CPC(dev);
>
>     cpc->boot = 0x00000010;     /* Boot from PCI - IIC EEPROM disabled */
>     cpc->epctl = 0x00000000;
> @@ -1391,21 +1360,24 @@ static void ppc405ep_cpc_reset (void *opaque)
>     cpc->er = 0x00000000;
>     cpc->fr = 0x00000000;
>     cpc->sr = 0x00000000;
> +    cpc->jtagid = 0x20267049;
>     ppc405ep_compute_clocks(cpc);
> }
>
> /* XXX: sysclk should be between 25 and 100 MHz */
> -static void ppc405ep_cpc_init (CPUPPCState *env, clk_setup_t clk_setup[8],
> -                               uint32_t sysclk)
> +static void ppc405_cpc_realize(DeviceState *dev, Error **errp)
> {
> -    ppc405ep_cpc_t *cpc;
> +    Ppc405CpcState *cpc = PPC405_CPC(dev);
> +    CPUPPCState *env;
> +
> +    assert(cpc->cpu);
> +
> +    env = &cpc->cpu->env;
> +
> +    cpc->clk_setup[PPC405EP_CPU_CLK].cb =
> +        ppc_40x_timers_init(env, cpc->sysclk, PPC_INTERRUPT_PIT);
> +    cpc->clk_setup[PPC405EP_CPU_CLK].opaque = env;
>
> -    cpc = g_new0(ppc405ep_cpc_t, 1);
> -    memcpy(cpc->clk_setup, clk_setup,
> -           PPC405EP_CLK_NB * sizeof(clk_setup_t));
> -    cpc->jtagid = 0x20267049;
> -    cpc->sysclk = sysclk;
> -    qemu_register_reset(&ppc405ep_cpc_reset, cpc);
>     ppc_dcr_register(env, PPC405EP_CPC0_BOOT, cpc,
>                      &dcr_read_epcpc, &dcr_write_epcpc);
>     ppc_dcr_register(env, PPC405EP_CPC0_EPCTL, cpc,
> @@ -1422,14 +1394,23 @@ static void ppc405ep_cpc_init (CPUPPCState *env, clk_setup_t clk_setup[8],
>                      &dcr_read_epcpc, &dcr_write_epcpc);
>     ppc_dcr_register(env, PPC405EP_CPC0_PCI, cpc,
>                      &dcr_read_epcpc, &dcr_write_epcpc);
> -#if 0
> -    ppc_dcr_register(env, PPC405EP_CPC0_ER, cpc,
> -                     &dcr_read_epcpc, &dcr_write_epcpc);
> -    ppc_dcr_register(env, PPC405EP_CPC0_FR, cpc,
> -                     &dcr_read_epcpc, &dcr_write_epcpc);
> -    ppc_dcr_register(env, PPC405EP_CPC0_SR, cpc,
> -                     &dcr_read_epcpc, &dcr_write_epcpc);
> -#endif
> +}
> +
> +static Property ppc405_cpc_properties[] = {
> +    DEFINE_PROP_LINK("cpu", Ppc405CpcState, cpu, TYPE_POWERPC_CPU,
> +                     PowerPCCPU *),
> +    DEFINE_PROP_UINT32("sys-clk", Ppc405CpcState, sysclk, 0),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void ppc405_cpc_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +
> +    dc->realize = ppc405_cpc_realize;
> +    dc->user_creatable = false;
> +    dc->reset = ppc405_cpc_reset;
> +    device_class_set_props(dc, ppc405_cpc_properties);
> }
>
> static void ppc405_soc_instance_init(Object *obj)
> @@ -1438,12 +1419,14 @@ static void ppc405_soc_instance_init(Object *obj)
>
>     object_initialize_child(obj, "cpu", &s->cpu,
>                             POWERPC_CPU_TYPE_NAME("405ep"));
> +
> +    object_initialize_child(obj, "cpc", &s->cpc, TYPE_PPC405_CPC);
> +    object_property_add_alias(obj, "sys-clk", OBJECT(&s->cpc), "sys-clk");
> }
>
> static void ppc405_soc_realize(DeviceState *dev, Error **errp)
> {
>     Ppc405SoCState *s = PPC405_SOC(dev);
> -    clk_setup_t clk_setup[PPC405EP_CLK_NB];
>     qemu_irq dma_irqs[4], gpt_irqs[5], mal_irqs[4];
>     CPUPPCState *env;
>     Error *err = NULL;
> @@ -1458,8 +1441,6 @@ static void ppc405_soc_realize(DeviceState *dev, Error **errp)
>     memory_region_add_subregion(get_system_memory(), PPC405EP_SRAM_BASE,
>                                 &s->sram);
>
> -    memset(clk_setup, 0, sizeof(clk_setup));
> -
>     /* init CPUs */
>     if (!qdev_realize(DEVICE(&s->cpu), NULL, errp)) {
>         return;
> @@ -1468,14 +1449,14 @@ static void ppc405_soc_realize(DeviceState *dev, Error **errp)
>
>     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);
> +    object_property_set_link(OBJECT(&s->cpc), "cpu", OBJECT(&s->cpu),
> +                             &error_abort);
> +    if (!qdev_realize(DEVICE(&s->cpc), NULL, errp)) {
> +        return;
> +    }
>
>     /* PLB arbitrer */
>     ppc4xx_plb_init(env);
> @@ -1566,7 +1547,6 @@ static void ppc405_soc_realize(DeviceState *dev, Error **errp)
> static Property ppc405_soc_properties[] = {
>     DEFINE_PROP_LINK("dram", Ppc405SoCState, dram_mr, TYPE_MEMORY_REGION,
>                      MemoryRegion *),
> -    DEFINE_PROP_UINT32("sys-clk", Ppc405SoCState, sysclk, 0),
>     DEFINE_PROP_BOOL("dram-init", Ppc405SoCState, do_dram_init, 0),
>     DEFINE_PROP_UINT64("ram-size", Ppc405SoCState, ram_size, 0),
>     DEFINE_PROP_END_OF_LIST(),
> @@ -1583,6 +1563,11 @@ static void ppc405_soc_class_init(ObjectClass *oc, void *data)
>
> static const TypeInfo ppc405_types[] = {
>     {
> +        .name           = TYPE_PPC405_CPC,
> +        .parent         = TYPE_DEVICE,
> +        .instance_size  = sizeof(Ppc405CpcState),
> +        .class_init     = ppc405_cpc_class_init,
> +    }, {
>         .name           = TYPE_PPC405_SOC,
>         .parent         = TYPE_DEVICE,
>         .instance_size  = sizeof(Ppc405SoCState),
>