Re: [Qemu-devel] [PATCH v13 13/19] i.MX: KZM now uses the standalone i.MX31 SOC support

[Jean-Christophe DUBOIS]

Le 07/08/2015 15:45, Peter Maydell a écrit :

On 16 July 2015 at 22:21, Jean-Christophe Dubois <address@hidden> wrote:

Tested by booting a minimal Linux system on the emulated platform

Signed-off-by: Jean-Christophe Dubois <address@hidden>
---

This said:

-     * 0x80000000-0x87ffffff RAM                  EMULATED
-     * 0x88000000-0x8fffffff RAM Aliasing         EMULATED

but your patch changes it:

+ * 0x00000000-0x7fffffff See i.MX31 SOC for support
+ * 0x80000000-0x8fffffff RAM                  EMULATED
+ * 0x90000000-0x9fffffff RAM                  EMULATED

The i.MX31 has 2 SDRAM controllers:

• 0x80000000-0x8fffffff:First SDRAM controller. There is memory aliasing if we have less than 256 MB of memory in this slot.

• 0x90000000-0x9fffffff:Second SDRAM controller. There is memory aliasing if we have less than 256 MB of memory in this slot.
  

+    /* initialize our memory */
+    for (i=0, ram_size = machine->ram_size; (i<2) && ram_size; i++) {
+        unsigned int size;
+        char ram_name[20];
+        static const struct {
+            hwaddr addr;
+            unsigned int size;
+        } ram[2] = {
+            { FSL_IMX31_SDRAM0_ADDR, FSL_IMX31_SDRAM0_SIZE },
+            { FSL_IMX31_SDRAM1_ADDR, FSL_IMX31_SDRAM1_SIZE },
+        };
+
+       if (ram_size > ram[i].size) {
+            size = ram[i].size;
+       } else {
+            size = ram_size;
+       }
+
+        sprintf(ram_name, "kzm.ram%d", i);
+
+        ram_size -= size;
+
+        memory_region_allocate_system_memory(&s->ram[i], NULL, ram_name, size);

memory_region_allocate_system_memory() needs to be called once and
only once by a board init. You're going to end up calling it twice here.

Actually, I may be calling it up 3 time because there is also an internal (16KB) memory range that is allocated during the SOC init (fsl-imx31.c file).

Now as my memory (the 3 memory banks) is  (partially) discontiguous how am I suppose to init all of it in one call?

And if memory_region_allocate_system_memory() should really be called only once, why is it not enforced in this function? After all, memory_region_allocate_system_memory() only calls memory_region_init_ram() and vmstate_register_ram_global() in one step ...

+        memory_region_add_subregion(get_system_memory(), ram[i].addr,
+                                    &s->ram[i]);
+        if (size < ram[i].size) {
+            memory_region_init_alias(&s->ram_alias, NULL, "ram.alias",
+                                     &s->ram[i], 0, ram[i].size - size);
+            memory_region_add_subregion(get_system_memory(),
+                                        ram[i].addr + size, &s->ram_alias);
+        }
+    }

What is this code trying to do with that alias? It looks very odd.
(Are we trying to model under-decoded address bits?)

Yes in case the RAM size does not take all of the SDRAM controller window size (this is more or less what was done in the previous kzm code but for only one SDRAM controller).

Actually I am trying to allow a KZM board with a memory that can vary from 0MB to 512MB (instead of 0MB to 256 MB as before). This might be more than what the real board allow (I am not sure) but why not if the user needs more memory?

My instinct is to say that rather than modelling two separate lumps
of RAM we should just have one region -- they're contiguous, after
all.

If the requested RAM is smaller than the SDRAM controller window, the function is only called once. It is called twice here if the requested RAM exceed the SDRAM first controller window.

And I still have this 16 KB internal memory to allocate/initialize.

thanks
-- PMM