The SDRAM is incorrectly created in the OMAP2420 SoC.
Move its creation in the board code, this will later allow the
board to have the QOM ownership of the RAM.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/arm/nseries.c      | 10 +++++++---
 hw/arm/omap2.c        | 13 +++++--------
 include/hw/arm/omap.h |  4 +---
 3 files changed, 13 insertions(+), 14 deletions(-)

diff --git a/hw/arm/nseries.c b/hw/arm/nseries.c
index a36971d39a..11f2c193f3 100644
--- a/hw/arm/nseries.c
+++ b/hw/arm/nseries.c
@@ -1311,11 +1311,15 @@ static int n810_atag_setup(const struct arm_boot_info *info, void *p)
 static void n8x0_init(MachineState *machine,
                       struct arm_boot_info *binfo, int model)
 {
-    MemoryRegion *sysmem = get_system_memory();
+    MemoryRegion *sdram = g_new(MemoryRegion, 1);
     struct n800_s *s = (struct n800_s *) g_malloc0(sizeof(*s));
-    int sdram_size = binfo->ram_size;
+    uint64_t sdram_size = binfo->ram_size;
 
-    s->mpu = omap2420_mpu_init(sysmem, sdram_size, machine->cpu_type);
+    memory_region_allocate_system_memory(sdram, NULL, "omap2.dram",
+                                         sdram_size);
+    memory_region_add_subregion(get_system_memory(), OMAP2_Q2_BASE, sdram);
+
+    s->mpu = omap2420_mpu_init(sdram, machine->cpu_type);
 
     /* Setup peripherals
      *
diff --git a/hw/arm/omap2.c b/hw/arm/omap2.c
index bd7ddff983..457f152bac 100644
--- a/hw/arm/omap2.c
+++ b/hw/arm/omap2.c
@@ -22,6 +22,7 @@
 #include "qemu/error-report.h"
 #include "qapi/error.h"
 #include "cpu.h"
+#include "exec/address-spaces.h"
 #include "sysemu/blockdev.h"
 #include "sysemu/qtest.h"
 #include "sysemu/reset.h"
@@ -2276,8 +2277,7 @@ static const struct dma_irq_map omap2_dma_irq_map[] = {
     { 0, OMAP_INT_24XX_SDMA_IRQ3 },
 };
 
-struct omap_mpu_state_s *omap2420_mpu_init(MemoryRegion *sysmem,
-                unsigned long sdram_size,
+struct omap_mpu_state_s *omap2420_mpu_init(MemoryRegion *sdram,
                 const char *cpu_type)
 {
     struct omap_mpu_state_s *s = g_new0(struct omap_mpu_state_s, 1);
@@ -2286,11 +2286,11 @@ struct omap_mpu_state_s *omap2420_mpu_init(MemoryRegion *sysmem,
     int i;
     SysBusDevice *busdev;
     struct omap_target_agent_s *ta;
+    MemoryRegion *sysmem = get_system_memory();
 
     /* Core */
     s->mpu_model = omap2420;
     s->cpu = ARM_CPU(cpu_create(cpu_type));
-    s->sdram_size = sdram_size;
     s->sram_size = OMAP242X_SRAM_SIZE;
 
     s->wakeup = qemu_allocate_irq(omap_mpu_wakeup, s, 0);
@@ -2299,9 +2299,6 @@ struct omap_mpu_state_s *omap2420_mpu_init(MemoryRegion *sysmem,
     omap_clk_init(s);
 
     /* Memory-mapped stuff */
-    memory_region_allocate_system_memory(&s->sdram, NULL, "omap2.dram",
-                                         s->sdram_size);
-    memory_region_add_subregion(sysmem, OMAP2_Q2_BASE, &s->sdram);
     memory_region_init_ram(&s->sram, NULL, "omap2.sram", s->sram_size,
                            &error_fatal);
     memory_region_add_subregion(sysmem, OMAP2_SRAM_BASE, &s->sram);
@@ -2338,8 +2335,8 @@ struct omap_mpu_state_s *omap2420_mpu_init(MemoryRegion *sysmem,
     s->port->addr_valid = omap2_validate_addr;
 
     /* Register SDRAM and SRAM ports for fast DMA transfers.  */
-    soc_dma_port_add_mem(s->dma, memory_region_get_ram_ptr(&s->sdram),
-                         OMAP2_Q2_BASE, s->sdram_size);
+    soc_dma_port_add_mem(s->dma, memory_region_get_ram_ptr(sdram),
+                         OMAP2_Q2_BASE, memory_region_size(sdram));
     soc_dma_port_add_mem(s->dma, memory_region_get_ram_ptr(&s->sram),
                          OMAP2_SRAM_BASE, s->sram_size);
 
diff --git a/include/hw/arm/omap.h b/include/hw/arm/omap.h
index 2fda996648..763d8eab4f 100644
--- a/include/hw/arm/omap.h
+++ b/include/hw/arm/omap.h
@@ -824,7 +824,6 @@ struct omap_mpu_state_s {
     MemoryRegion tap_iomem;
     MemoryRegion imif_ram;
     MemoryRegion emiff_ram;
-    MemoryRegion sdram;
     MemoryRegion sram;
 
     struct omap_dma_port_if_s {
@@ -938,8 +937,7 @@ struct omap_mpu_state_s *omap310_mpu_init(MemoryRegion *system_memory,
                 const char *core);
 
 /* omap2.c */
-struct omap_mpu_state_s *omap2420_mpu_init(MemoryRegion *sysmem,
-                unsigned long sdram_size,
+struct omap_mpu_state_s *omap2420_mpu_init(MemoryRegion *sdram,
                 const char *core);
 
 uint32_t omap_badwidth_read8(void *opaque, hwaddr addr);
-- 
2.21.0

On 10/20/19 3:56 PM, Philippe Mathieu-Daudé wrote:
>  static void n8x0_init(MachineState *machine,
>                        struct arm_boot_info *binfo, int model)
>  {
> -    MemoryRegion *sysmem = get_system_memory();
> +    MemoryRegion *sdram = g_new(MemoryRegion, 1);
>      struct n800_s *s = (struct n800_s *) g_malloc0(sizeof(*s));
> -    int sdram_size = binfo->ram_size;
> +    uint64_t sdram_size = binfo->ram_size;
>  
> -    s->mpu = omap2420_mpu_init(sysmem, sdram_size, machine->cpu_type);
> +    memory_region_allocate_system_memory(sdram, NULL, "omap2.dram",
> +                                         sdram_size);
> +    memory_region_add_subregion(get_system_memory(), OMAP2_Q2_BASE, sdram);
> +
> +    s->mpu = omap2420_mpu_init(sdram, machine->cpu_type);

Any reason not to put the MemoryRegion in to the n800_s structure?

Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

On 10/21/19 6:59 PM, Richard Henderson wrote:
> On 10/20/19 3:56 PM, Philippe Mathieu-Daudé wrote:
>>   static void n8x0_init(MachineState *machine,
>>                         struct arm_boot_info *binfo, int model)
>>   {
>> -    MemoryRegion *sysmem = get_system_memory();
>> +    MemoryRegion *sdram = g_new(MemoryRegion, 1);
>>       struct n800_s *s = (struct n800_s *) g_malloc0(sizeof(*s));
>> -    int sdram_size = binfo->ram_size;
>> +    uint64_t sdram_size = binfo->ram_size;
>>   
>> -    s->mpu = omap2420_mpu_init(sysmem, sdram_size, machine->cpu_type);
>> +    memory_region_allocate_system_memory(sdram, NULL, "omap2.dram",
>> +                                         sdram_size);
>> +    memory_region_add_subregion(get_system_memory(), OMAP2_Q2_BASE, sdram);
>> +
>> +    s->mpu = omap2420_mpu_init(sdram, machine->cpu_type);
> 
> Any reason not to put the MemoryRegion in to the n800_s structure?

Ah yes, better to keep it in n800_s in case we ever QOM'ify this SoC.
I'll do.

> Otherwise,
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Thanks :)