Re: [PATCH v4 05/12] hw/arm: Add NPCM730 and NPCM750 SoC models

[Havard Skinnemoen]

On Wed, Jul 8, 2020 at 11:13 AM Havard Skinnemoen
<hskinnemoen@google.com> wrote:
>
> On Wed, Jul 8, 2020 at 10:31 AM Philippe Mathieu-Daudé <f4bug@amsat.org> 
> wrote:
> >
> > On 7/7/20 8:47 PM, Havard Skinnemoen wrote:
> > > +    /* System Global Control Registers (GCR) */
> > > +    object_property_set_int(OBJECT(&s->gcr), nc->disabled_modules,
> > > +                            "disabled-modules", &error_abort);
> > > +    object_property_set_link(OBJECT(&s->gcr), OBJECT(s->dram), "dram",
> > > +                             &error_abort);
> >
> > I guess you can simplify using in npcm7xx_init():
> >
> >       object_property_add_const_link(obj, "dram-mr", OBJECT(&s->gcr));
> >
> > And in npcm7xx_gcr_realize()
> >
> >     obj = object_property_get_link(OBJECT(dev), "dram-mr", &err);
> >     if (obj == NULL) {
> >         error_setg(errp, "%s: required dram-mr link not found: %s",
> >                    __func__, error_get_pretty(err));
> >         return;
> >     }
> >     s->dram = MEMORY_REGION(obj);
>
> OK, I'll try that, thanks!

Hmm, I ended up doing

-    object_property_set_link(OBJECT(&s->gcr), OBJECT(s->dram), "dram",
-                             &error_abort);
+    object_property_add_const_link(OBJECT(&s->gcr), "dram-mr",
OBJECT(s->dram));

in realize() because s->dram isn't initialized yet in npcm7xx_init().
Is this what you had in mind?

Here's the diff from all the dram-related changes:

diff --git a/include/hw/arm/npcm7xx.h b/include/hw/arm/npcm7xx.h
index 39a1f28d8e..30e00a514d 100644
--- a/include/hw/arm/npcm7xx.h
+++ b/include/hw/arm/npcm7xx.h
@@ -28,6 +28,10 @@

 #define NPCM7XX_MAX_NUM_CPUS    (2)

+/* The first half of the address space is reserved for DDR4 DRAM. */
+#define NPCM7XX_DRAM_BA         (0x00000000)
+#define NPCM7XX_DRAM_SZ         (2 * GiB)
+
 /* Magic addresses for setting up direct kernel booting and SMP boot stubs. */
 #define NPCM7XX_LOADER_START            (0x00000000)  /* Start of SDRAM */
 #define NPCM7XX_SMP_LOADER_START        (0xffff0000)  /* Boot ROM */
diff --git a/include/hw/misc/npcm7xx_gcr.h b/include/hw/misc/npcm7xx_gcr.h
index 49d699410f..4884676be2 100644
--- a/include/hw/misc/npcm7xx_gcr.h
+++ b/include/hw/misc/npcm7xx_gcr.h
@@ -68,7 +68,6 @@ typedef struct NPCM7xxGCRState {
     uint32_t reset_pwron;
     uint32_t reset_mdlr;
     uint32_t reset_intcr3;
-    MemoryRegion *dram;
 } NPCM7xxGCRState;

 #define TYPE_NPCM7XX_GCR "npcm7xx-gcr"
diff --git a/hw/arm/npcm7xx.c b/hw/arm/npcm7xx.c
index e64cf6a84c..a05a900197 100644
--- a/hw/arm/npcm7xx.c
+++ b/hw/arm/npcm7xx.c
@@ -26,10 +26,6 @@
 #include "qemu/units.h"
 #include "sysemu/sysemu.h"

-/* The first half of the address space is reserved for DDR4 DRAM. */
-#define NPCM7XX_DRAM_BA         (0x00000000)
-#define NPCM7XX_DRAM_SZ         (2 * GiB)
-
 /*
  * This covers the whole MMIO space. We'll use this to catch any MMIO accesses
  * that aren't handled by any device.
@@ -257,8 +253,7 @@ static void npcm7xx_realize(DeviceState *dev, Error **errp)
     /* System Global Control Registers (GCR) */
     object_property_set_int(OBJECT(&s->gcr), nc->disabled_modules,
                             "disabled-modules", &error_abort);
-    object_property_set_link(OBJECT(&s->gcr), OBJECT(s->dram), "dram",
-                             &error_abort);
+    object_property_add_const_link(OBJECT(&s->gcr), "dram-mr",
OBJECT(s->dram));
     sysbus_realize(SYS_BUS_DEVICE(&s->gcr), &error_abort);
     sysbus_mmio_map(SYS_BUS_DEVICE(&s->gcr), 0, NPCM7XX_GCR_BA);

@@ -326,13 +321,10 @@ static void npcm7xx_realize(DeviceState *dev,
Error **errp)
     memory_region_init_rom(&s->irom, OBJECT(dev), "irom", NPCM7XX_ROM_SZ,
                            &error_abort);
     memory_region_add_subregion(get_system_memory(), NPCM7XX_ROM_BA, &s->irom);
-
-    /* External DDR4 SDRAM */
-    memory_region_add_subregion(get_system_memory(), NPCM7XX_DRAM_BA, s->dram);
 }

 static Property npcm7xx_properties[] = {
-    DEFINE_PROP_LINK("dram", NPCM7xxState, dram, TYPE_MEMORY_REGION,
+    DEFINE_PROP_LINK("dram-mr", NPCM7xxState, dram, TYPE_MEMORY_REGION,
                      MemoryRegion *),
     DEFINE_PROP_END_OF_LIST(),
 };
diff --git a/hw/arm/npcm7xx_boards.c b/hw/arm/npcm7xx_boards.c
index 2f66e699b1..cfb31ce6f5 100644
--- a/hw/arm/npcm7xx_boards.c
+++ b/hw/arm/npcm7xx_boards.c
@@ -83,21 +83,25 @@ static void npcm7xx_connect_flash(NPCM7xxFIUState
*fiu, int cs_no,
     sysbus_connect_irq(SYS_BUS_DEVICE(fiu), cs_no, flash_cs);
 }

+static void npcm7xx_connect_dram(NPCM7xxState *soc, MemoryRegion *dram)
+{
+    memory_region_add_subregion(get_system_memory(), NPCM7XX_DRAM_BA, dram);
+
+    object_property_set_link(OBJECT(soc), OBJECT(dram), "dram-mr",
+                             &error_abort);
+}
+
 static NPCM7xxState *npcm7xx_create_soc(MachineState *machine,
                                         uint32_t hw_straps)
 {
     NPCM7xxMachineClass *nmc = NPCM7XX_MACHINE_GET_CLASS(machine);
-    NPCM7xxState *soc;
+    Object *obj;

-    soc = NPCM7XX(object_new_with_props(nmc->soc_type, OBJECT(machine), "soc",
-                                        &error_abort, NULL));
-    object_property_set_link(OBJECT(soc), OBJECT(machine->ram), "dram",
-                             &error_abort);
-    object_property_set_uint(OBJECT(soc), hw_straps, "power-on-straps",
-                             &error_abort);
-    qdev_realize(DEVICE(soc), NULL, &error_abort);
+    obj = object_new_with_props(nmc->soc_type, OBJECT(machine), "soc",
+                                &error_abort, NULL);
+    object_property_set_uint(obj, hw_straps, "power-on-straps", &error_abort);

-    return soc;
+    return NPCM7XX(obj);
 }

 static void npcm750_evb_init(MachineState *machine)
@@ -105,6 +109,9 @@ static void npcm750_evb_init(MachineState *machine)
     NPCM7xxState *soc;

     soc = npcm7xx_create_soc(machine, NPCM750_EVB_POWER_ON_STRAPS);
+    npcm7xx_connect_dram(soc, machine->ram);
+    qdev_realize(DEVICE(soc), NULL, &error_abort);
+
     npcm7xx_load_bootrom(soc);
     npcm7xx_connect_flash(&soc->fiu[0], 0, "w25q256", drive_get(IF_MTD, 0, 0));
     npcm7xx_load_kernel(machine, soc);
@@ -115,6 +122,9 @@ static void quanta_gsj_init(MachineState *machine)
     NPCM7xxState *soc;

     soc = npcm7xx_create_soc(machine, QUANTA_GSJ_POWER_ON_STRAPS);
+    npcm7xx_connect_dram(soc, machine->ram);
+    qdev_realize(DEVICE(soc), NULL, &error_abort);
+
     npcm7xx_load_bootrom(soc);
     npcm7xx_connect_flash(&soc->fiu[0], 0, "mx25l25635e",
                           drive_get(IF_MTD, 0, 0));
diff --git a/hw/misc/npcm7xx_gcr.c b/hw/misc/npcm7xx_gcr.c
index 78a885e265..9934cd238d 100644
--- a/hw/misc/npcm7xx_gcr.c
+++ b/hw/misc/npcm7xx_gcr.c
@@ -127,11 +127,16 @@ static void npcm7xx_gcr_realize(DeviceState
*dev, Error **errp)
 {
     NPCM7xxGCRState *s = NPCM7XX_GCR(dev);
     uint64_t dram_size;
+    Error *err = NULL;
+    Object *obj;

-    if (!s->dram) {
-        error_setg(errp, "npcm7xx_gcr: 'dram' link not set");
+    obj = object_property_get_link(OBJECT(dev), "dram-mr", &err);
+    if (!obj) {
+        error_setg(errp, "%s: required dram-mr link not found: %s",
+                   __func__, error_get_pretty(err));
         return;
     }
+    dram_size = memory_region_size(MEMORY_REGION(obj));

     /* Power-on reset value */
     s->reset_intcr3 = 0x00001002;
@@ -149,7 +154,6 @@ static void npcm7xx_gcr_realize(DeviceState *dev,
Error **errp)
      *
      * 
https://github.com/Nuvoton-Israel/u-boot/blob/2aef993bd2aafeb5408dbaad0f3ce099ee40c4aa/board/nuvoton/poleg/poleg.c#L244
      */
-    dram_size = int128_get64(s->dram->size);
     if (dram_size >= 2 * GiB) {
         s->reset_intcr3 |= 4 << 8;
     } else if (dram_size >= 1 * GiB) {
@@ -191,8 +195,6 @@ static const VMStateDescription vmstate_npcm7xx_gcr = {
 static Property npcm7xx_gcr_properties[] = {
     DEFINE_PROP_UINT32("disabled-modules", NPCM7xxGCRState, reset_mdlr, 0),
     DEFINE_PROP_UINT32("power-on-straps", NPCM7xxGCRState, reset_pwron, 0),
-    DEFINE_PROP_LINK("dram", NPCM7xxGCRState, dram, TYPE_MEMORY_REGION,
-                     MemoryRegion *),
     DEFINE_PROP_END_OF_LIST(),
 };