qemu-devel
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v3 09/22] ppc/ppc405: QOM'ify CPC

From: BALATON Zoltan
Subject: Re: [PATCH v3 09/22] ppc/ppc405: QOM'ify CPC
Date: Mon, 8 Aug 2022 16:12:29 +0200 (CEST) 
On Mon, 8 Aug 2022, Cédric Le Goater wrote:

The OCM controller is currently modeled as a DCR device.


What's OCM? Did you mean CPC?


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    |  35 ++++++++++++-
hw/ppc/ppc405_uc.c | 122 +++++++++++++++++++--------------------------
2 files changed, 86 insertions(+), 71 deletions(-)

diff --git a/hw/ppc/ppc405.h b/hw/ppc/ppc405.h
index 8cc76cc8b3fe..2ba829988de2 100644
--- a/hw/ppc/ppc405.h
+++ b/hw/ppc/ppc405.h
@@ -63,6 +63,39 @@ struct ppc4xx_bd_info_t {
    uint32_t bi_iic_fast[2];
};

+#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 {
+    Ppc4xxDcrDeviceState parent_obj;
+
+    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);

@@ -78,9 +111,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 fa3853df2233..e45535b8e52a 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);
I still think that these callbacks that are registered by the realize method with an already checked opaque pointer don't need to check it again here so I'd drop the QOM casts from all these callbacks and assign opaque directly. Only cast where needed (you're passed a DeviceState, wtc.). But now that you've introduced ppc4xx_dcr_register it is passed a DcrDevice as it registers the callbacks with itself. That's wrong, the scr callbacks could have any opaque data so ppc4xx_dcr_register() should have an opaque argument (where you pass cpc uin this patch) then drop these casts. Same for all later patches. 


    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 *opaque)
{
-    ppc405ep_cpc_t *cpc = opaque;
+    Ppc405CpcState *cpc = PPC405_CPC(opaque);

    cpc->boot = 0x00000010;     /* Boot from PCI - IIC EEPROM disabled */
    cpc->epctl = 0x00000000;
@@ -1391,45 +1360,56 @@ 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);
+    Ppc4xxDcrDeviceState *dcr = PPC4xx_DCR_DEVICE(dev);
Only these casts are enough to check the object as it's only passed on from this point. 


+    CPUPPCState *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,
-                     &dcr_read_epcpc, &dcr_write_epcpc);
-    ppc_dcr_register(env, PPC405EP_CPC0_PLLMR0, cpc,
-                     &dcr_read_epcpc, &dcr_write_epcpc);
-    ppc_dcr_register(env, PPC405EP_CPC0_PLLMR1, cpc,
+    assert(dcr->cpu);
So if you move this to the end you can drop this assert. Also env may not be needed any more now that it only apears twice. 

Regards,
BALATON Zoltan


+
+    env = &dcr->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;
+
+    ppc4xx_dcr_register(dcr, PPC405EP_CPC0_BOOT,
                     &dcr_read_epcpc, &dcr_write_epcpc);
-    ppc_dcr_register(env, PPC405EP_CPC0_UCR, cpc,
+    ppc4xx_dcr_register(dcr, PPC405EP_CPC0_EPCTL,
                     &dcr_read_epcpc, &dcr_write_epcpc);
-    ppc_dcr_register(env, PPC405EP_CPC0_SRR, cpc,
+    ppc4xx_dcr_register(dcr, PPC405EP_CPC0_PLLMR0,
                     &dcr_read_epcpc, &dcr_write_epcpc);
-    ppc_dcr_register(env, PPC405EP_CPC0_JTAGID, cpc,
+    ppc4xx_dcr_register(dcr, PPC405EP_CPC0_PLLMR1,
                     &dcr_read_epcpc, &dcr_write_epcpc);
-    ppc_dcr_register(env, PPC405EP_CPC0_PCI, cpc,
+    ppc4xx_dcr_register(dcr, PPC405EP_CPC0_UCR,
                     &dcr_read_epcpc, &dcr_write_epcpc);
-#if 0
-    ppc_dcr_register(env, PPC405EP_CPC0_ER, cpc,
+    ppc4xx_dcr_register(dcr, PPC405EP_CPC0_SRR,
                     &dcr_read_epcpc, &dcr_write_epcpc);
-    ppc_dcr_register(env, PPC405EP_CPC0_FR, cpc,
+    ppc4xx_dcr_register(dcr, PPC405EP_CPC0_JTAGID,
                     &dcr_read_epcpc, &dcr_write_epcpc);
-    ppc_dcr_register(env, PPC405EP_CPC0_SR, cpc,
+    ppc4xx_dcr_register(dcr, PPC405EP_CPC0_PCI,
                     &dcr_read_epcpc, &dcr_write_epcpc);
-#endif
+}
+
+static Property ppc405_cpc_properties[] = {
+    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;
+    device_class_set_props(dc, ppc405_cpc_properties);
+    dc->reset = ppc405_cpc_reset;
}

static void ppc405_soc_instance_init(Object *obj)
@@ -1438,17 +1418,17 @@ 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;

-    memset(clk_setup, 0, sizeof(clk_setup));
-
    /* init CPUs */
    if (!qdev_realize(DEVICE(&s->cpu), NULL, errp)) {
        return;
@@ -1457,14 +1437,12 @@ 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);
+    if (!ppc4xx_dcr_realize(PPC4xx_DCR_DEVICE(&s->cpc), &s->cpu, errp)) {
+        return;
+    }

    /* PLB arbitrer */
    ppc4xx_plb_init(env);
@@ -1556,7 +1534,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(),
@@ -1573,6 +1550,11 @@ static void ppc405_soc_class_init(ObjectClass *oc, void 
*data)

static const TypeInfo ppc405_types[] = {
    {
+        .name           = TYPE_PPC405_CPC,
+        .parent         = TYPE_PPC4xx_DCR_DEVICE,
+        .instance_size  = sizeof(Ppc405CpcState),
+        .class_init     = ppc405_cpc_class_init,
+    }, {
        .name           = TYPE_PPC405_SOC,
        .parent         = TYPE_DEVICE,
        .instance_size  = sizeof(Ppc405SoCState),
reply via email to
[Prev in Thread] Current Thread [Next in Thread]