From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57058) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZOpjE-0005uF-L9 for qemu-devel@nongnu.org; Mon, 10 Aug 2015 12:15:18 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZOpj9-00085k-Qv for qemu-devel@nongnu.org; Mon, 10 Aug 2015 12:15:12 -0400 Received: from zose-mta03.web4all.fr ([185.49.20.44]:34912) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZOpj9-00080i-I6 for qemu-devel@nongnu.org; Mon, 10 Aug 2015 12:15:07 -0400 Message-ID: <55C8CE07.2060705@tribudubois.net> Date: Mon, 10 Aug 2015 18:15:03 +0200 From: Jean-Christophe DUBOIS MIME-Version: 1.0 References: <84eb5a74233f317b36f41963e465851328972355.1437080501.git.jcd@tribudubois.net> In-Reply-To: Content-Type: multipart/alternative; boundary="------------040409030005070904060406" Subject: Re: [Qemu-devel] [PATCH v13 13/19] i.MX: KZM now uses the standalone i.MX31 SOC support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: QEMU Developers This is a multi-part message in MIME format. --------------040409030005070904060406 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: quoted-printable Le 07/08/2015 15:45, Peter Maydell a =C3=A9crit : > On 16 July 2015 at 22:21, Jean-Christophe Dubois = wrote: >> Tested by booting a minimal Linux system on the emulated platform >> >> Signed-off-by: Jean-Christophe Dubois >> --- > 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 aliasin= g if we have less than 256 MB of memory in this slot. * 0x90000000-0x9fffffff:Second SDRAM controller. There is memory aliasi= ng if we have less than 256 MB of memory in this slot. >> + /* initialize our memory */ >> + for (i=3D0, ram_size =3D 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] =3D { >> + { FSL_IMX31_SDRAM0_ADDR, FSL_IMX31_SDRAM0_SIZE }, >> + { FSL_IMX31_SDRAM1_ADDR, FSL_IMX31_SDRAM1_SIZE }, >> + }; >> + >> + if (ram_size > ram[i].size) { >> + size =3D ram[i].size; >> + } else { >> + size =3D ram_size; >> + } >> + >> + sprintf(ram_name, "kzm.ram%d", i); >> + >> + ram_size -=3D size; >> + >> + memory_region_allocate_system_memory(&s->ram[i], NULL, ram_na= me, 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=20 internal (16KB) memory range that is allocated during the SOC init=20 (fsl-imx31.c file). Now as my memory (the 3 memory banks) is (partially) discontiguous how=20 am I suppose to init all of it in one call? And if memory_region_allocate_system_memory() should really be called=20 only once, why is it not enforced in this function? After all,=20 memory_region_allocate_system_memory() only calls=20 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 - siz= e); >> + memory_region_add_subregion(get_system_memory(), >> + ram[i].addr + size, &s->ram_a= lias); >> + } >> + } > 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=20 window size (this is more or less what was done in the previous kzm code=20 but for only one SDRAM controller). Actually I am trying to allow a KZM board with a memory that can vary=20 from 0MB to 512MB (instead of 0MB to 256 MB as before). This might be=20 more than what the real board allow (I am not sure) but why not if the=20 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=20 function is only called once. It is called twice here if the requested=20 RAM exceed the SDRAM first controller window. And I still have this 16 KB internal memory to allocate/initialize. > > thanks > -- PMM > --------------040409030005070904060406 Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: quoted-printable
Le 07/08/2015 15:45, Peter Maydell a =C3=A9crit=C2=A0:
On 16 July 2015 at 22:21, Jean-Christophe Dubois <j=
cd@tribudubois.net> wrote:

Tested by booting a minimal Linux system on the em=
ulated platform

Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net>
---

This said:


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

but your patch changes it:


+ * 0x00000000-0x7fffffff See i.MX31 SOC for suppo=
rt
+ * 0x80000000-0x8fffffff RAM                  EMULATED
+ * 0x90000000-0x9fffffff RAM                  EMULATED
The i.MX31 has 2 SDRAM controllers:
  • 0x80000000-0x8fffffff:First SDRAM controller. Ther=
e is memory aliasing if we have less than 256 MB of memory in this slot.<=
/pre>
      
  • 0x90000000-0x9fffffff:Second SDRAM controller. The=
re is memory aliasing if we have less than 256 MB of memory in this slot.


      

        
+    /* initialize our memory */
+    for (i=3D0, ram_size =3D machine->ram_size; (i<2) && r=
am_size; i++) {
+        unsigned int size;
+        char ram_name[20];
+        static const struct {
+            hwaddr addr;
+            unsigned int size;
+        } ram[2] =3D {
+            { FSL_IMX31_SDRAM0_ADDR, FSL_IMX31_SDRAM0_SIZE },
+            { FSL_IMX31_SDRAM1_ADDR, FSL_IMX31_SDRAM1_SIZE },
+        };
+
+       if (ram_size > ram[i].size) {
+            size =3D ram[i].size;
+       } else {
+            size =3D ram_size;
+       }
+
+        sprintf(ram_name, "kzm.ram%d", i);
+
+        ram_size -=3D size;
+
+        memory_region_allocate_system_memory(&s->ram[i], NULL, ra=
m_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.<=
/pre>
    

    

    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=C2=A0 (partially) discontigu=
ous
    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_me=
mory(), ram[i].addr,
+                                    &s->ram[i]);
+        if (size < ram[i].size) {
+            memory_region_init_alias(&s->ram_alias, NULL, "ram.al=
ias",
+                                     &s->ram[i], 0, ram[i].size -=
 size);
+            memory_region_add_subregion(get_system_memory(),
+                                        ram[i].addr + size, &s->r=
am_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



    

    

  


--------------040409030005070904060406--