Re: [PATCH v2 12/20] ppc/ppc405: QOM'ify EBC

[Cédric Le Goater]

On 8/4/22 01:36, Daniel Henrique Barboza wrote:
Cedric,
On 8/3/22 10:28, Cédric Le Goater wrote:
Reviewed-by: Daniel Henrique Barboza <danielhb...@gmail.com>
Signed-off-by: Cédric Le Goater <c...@kaod.org>
---
  hw/ppc/ppc405.h    | 16 +++++++++++
  hw/ppc/ppc405_uc.c | 71 +++++++++++++++++++++++++++++++---------------
  2 files changed, 64 insertions(+), 23 deletions(-)

diff --git a/hw/ppc/ppc405.h b/hw/ppc/ppc405.h
index 1da34a7f10f3..1c7fe07b8084 100644
--- a/hw/ppc/ppc405.h
+++ b/hw/ppc/ppc405.h
@@ -65,7 +65,22 @@ struct ppc4xx_bd_info_t {
  typedef struct Ppc405SoCState Ppc405SoCState;
+/* Peripheral controller */
+#define TYPE_PPC405_EBC "ppc405-ebc"
+OBJECT_DECLARE_SIMPLE_TYPE(Ppc405EbcState, PPC405_EBC);
+struct Ppc405EbcState {
+    DeviceState parent_obj;
+
+    PowerPCCPU *cpu;
+    uint32_t addr;
+    uint32_t bcr[8];
+    uint32_t bap[8];
+    uint32_t bear;
+    uint32_t besr0;
+    uint32_t besr1;
+    uint32_t cfg;
+};
  /* DMA controller */
  #define TYPE_PPC405_DMA "ppc405-dma"
@@ -203,6 +218,7 @@ struct Ppc405SoCState {
      Ppc405OcmState ocm;
      Ppc405GpioState gpio;
      Ppc405DmaState dma;
+    Ppc405EbcState ebc;
  };
  /* PowerPC 405 core */
diff --git a/hw/ppc/ppc405_uc.c b/hw/ppc/ppc405_uc.c
index 6bd93c1cb90c..0166f3fc36da 100644
--- a/hw/ppc/ppc405_uc.c
+++ b/hw/ppc/ppc405_uc.c
@@ -393,17 +393,6 @@ static void ppc4xx_opba_init(hwaddr base)
  
/*****************************************************************************/
  /* Peripheral controller */
-typedef struct ppc4xx_ebc_t ppc4xx_ebc_t;
-struct ppc4xx_ebc_t {
-    uint32_t addr;
-    uint32_t bcr[8];
-    uint32_t bap[8];
-    uint32_t bear;
-    uint32_t besr0;
-    uint32_t besr1;
-    uint32_t cfg;
-};
-
  enum {
      EBC0_CFGADDR = 0x012,
      EBC0_CFGDATA = 0x013,
@@ -411,10 +400,9 @@ enum {
  static uint32_t dcr_read_ebc (void *opaque, int dcrn)
  {
-    ppc4xx_ebc_t *ebc;
+    Ppc405EbcState *ebc = PPC405_EBC(opaque);
      uint32_t ret;
-    ebc = opaque;
      switch (dcrn) {
      case EBC0_CFGADDR:
          ret = ebc->addr;
@@ -496,9 +484,8 @@ static uint32_t dcr_read_ebc (void *opaque, int dcrn)
  static void dcr_write_ebc (void *opaque, int dcrn, uint32_t val)
  {
-    ppc4xx_ebc_t *ebc;
+    Ppc405EbcState *ebc = PPC405_EBC(opaque);
-    ebc = opaque;
      switch (dcrn) {
      case EBC0_CFGADDR:
          ebc->addr = val;
@@ -554,12 +541,11 @@ static void dcr_write_ebc (void *opaque, int dcrn, 
uint32_t val)
      }
  }
-static void ebc_reset (void *opaque)
+static void ppc405_ebc_reset(DeviceState *dev)
  {
-    ppc4xx_ebc_t *ebc;
+    Ppc405EbcState *ebc = PPC405_EBC(dev);
      int i;
-    ebc = opaque;
      ebc->addr = 0x00000000;
      ebc->bap[0] = 0x7F8FFE80;
      ebc->bcr[0] = 0xFFE28000;
@@ -572,18 +558,46 @@ static void ebc_reset (void *opaque)
      ebc->cfg = 0x80400000;
  }
-void ppc405_ebc_init(CPUPPCState *env)
+static void ppc405_ebc_realize(DeviceState *dev, Error **errp)
  {
-    ppc4xx_ebc_t *ebc;
+    Ppc405EbcState *ebc = PPC405_EBC(dev);
+    CPUPPCState *env;
+
+    assert(ebc->cpu);
+
+    env = &ebc->cpu->env;
-    ebc = g_new0(ppc4xx_ebc_t, 1);
-    qemu_register_reset(&ebc_reset, ebc);
      ppc_dcr_register(env, EBC0_CFGADDR,
                       ebc, &dcr_read_ebc, &dcr_write_ebc);
      ppc_dcr_register(env, EBC0_CFGDATA,
                       ebc, &dcr_read_ebc, &dcr_write_ebc);
  }
+static Property ppc405_ebc_properties[] = {
+    DEFINE_PROP_LINK("cpu", Ppc405EbcState, cpu, TYPE_POWERPC_CPU,
+                     PowerPCCPU *),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void ppc405_ebc_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+
+    dc->realize = ppc405_ebc_realize;
+    dc->user_creatable = false;
+    dc->reset = ppc405_ebc_reset;
+    device_class_set_props(dc, ppc405_ebc_properties);
+}
+
+void ppc405_ebc_init(CPUPPCState *env)
+{
+    PowerPCCPU *cpu = env_archcpu(env);
+    DeviceState *dev = qdev_new(TYPE_PPC405_EBC);
+
+    object_property_set_link(OBJECT(cpu), "cpu", OBJECT(dev), &error_abort);

This line is breaking the boot of sam460ex:


  ./qemu-system-ppc64 -display none -M sam460ex
Unexpected error in object_property_find_err() at ../qom/object.c:1304:
qemu-system-ppc64: Property '460exb-powerpc64-cpu.cpu' not found
Aborted (core dumped)


I think you meant to link the cpu prop of the EBC obj to the CPU object,
not the cpu prop of the CPU obj to the EBC dev.

Yes. ppc405_ebc_init() has only one user left, the sam460ex, which I didn't
test :/

Thanks,

C.
 

This fixes the issue:


$ git diff
diff --git a/hw/ppc/ppc405_uc.c b/hw/ppc/ppc405_uc.c
index 0166f3fc36..aac3a3f761 100644
--- a/hw/ppc/ppc405_uc.c
+++ b/hw/ppc/ppc405_uc.c
@@ -594,7 +594,7 @@ void ppc405_ebc_init(CPUPPCState *env)
      PowerPCCPU *cpu = env_archcpu(env);
      DeviceState *dev = qdev_new(TYPE_PPC405_EBC);

-    object_property_set_link(OBJECT(cpu), "cpu", OBJECT(dev), &error_abort);
+    object_property_set_link(OBJECT(dev), "cpu", OBJECT(cpu), &error_abort);
      qdev_realize_and_unref(dev, NULL, &error_fatal);
  }


Daniel


+    qdev_realize_and_unref(dev, NULL, &error_fatal);
+}
+
  
/*****************************************************************************/
  /* DMA controller */
  enum {
@@ -1418,6 +1432,8 @@ static void ppc405_soc_instance_init(Object *obj)
      object_initialize_child(obj, "gpio", &s->gpio, TYPE_PPC405_GPIO);
      object_initialize_child(obj, "dma", &s->dma, TYPE_PPC405_DMA);
+
+    object_initialize_child(obj, "ebc", &s->ebc, TYPE_PPC405_EBC);
  }
  static void ppc405_soc_realize(DeviceState *dev, Error **errp)
@@ -1490,7 +1506,11 @@ static void ppc405_soc_realize(DeviceState *dev, Error 
**errp)
                        s->ram_bases, s->ram_sizes, s->do_dram_init);
      /* External bus controller */
-    ppc405_ebc_init(env);
+    object_property_set_link(OBJECT(&s->ebc), "cpu", OBJECT(&s->cpu),
+                             &error_abort);
+    if (!qdev_realize(DEVICE(&s->ebc), NULL, errp)) {
+        return;
+    }
      /* DMA controller */
      object_property_set_link(OBJECT(&s->dma), "cpu", OBJECT(&s->cpu),
@@ -1576,6 +1596,11 @@ static void ppc405_soc_class_init(ObjectClass *oc, void 
*data)
  static const TypeInfo ppc405_types[] = {
      {
+        .name           = TYPE_PPC405_EBC,
+        .parent         = TYPE_DEVICE,
+        .instance_size  = sizeof(Ppc405EbcState),
+        .class_init     = ppc405_ebc_class_init,
+    }, {
          .name           = TYPE_PPC405_DMA,
          .parent         = TYPE_SYS_BUS_DEVICE,
          .instance_size  = sizeof(Ppc405DmaState),