[Qemu-devel] [PATCH] Versatile Express: add modelling of NOR flash

[Qemu-devel] [PATCH] Versatile Express: add modelling of NOR flash

[Francesco Lavra]

This patch adds modelling of the two NOR flash banks found in the
Versatile Express motherboard. Tested with U-Boot running on an emulated
Versatile Express A9. The alias of the first NOR flash in the Cortex-A
Series memory map is not modelled.

Signed-off-by: Francesco Lavra <francescolavra.fl@gmail.com>
---
 hw/vexpress.c |   25 +++++++++++++++++++++++--
 1 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/hw/vexpress.c b/hw/vexpress.c
index b615844..695892b 100644
--- a/hw/vexpress.c
+++ b/hw/vexpress.c
@@ -29,8 +29,12 @@
 #include "sysemu.h"
 #include "boards.h"
 #include "exec-memory.h"
+#include "blockdev.h"
+#include "flash.h"

 #define VEXPRESS_BOARD_ID 0x8e0
+#define VEXPRESS_FLASH_SIZE (64 * 1024 * 1024)
+#define VEXPRESS_FLASH_SECT_SIZE (256 * 1024)

 static struct arm_boot_info vexpress_binfo;

@@ -357,6 +361,7 @@ static void vexpress_common_init(const VEDBoardInfo
*daughterboard,
     qemu_irq pic[64];
     uint32_t proc_id;
     uint32_t sys_id;
+    DriveInfo *dinfo;
     ram_addr_t vram_size, sram_size;
     MemoryRegion *sysmem = get_system_memory();
     MemoryRegion *vram = g_new(MemoryRegion, 1);
@@ -412,9 +417,25 @@ static void vexpress_common_init(const VEDBoardInfo
*daughterboard,

     sysbus_create_simple("pl111", map[VE_CLCD], pic[14]);

-    /* VE_NORFLASH0: not modelled */
+    dinfo = drive_get(IF_PFLASH, 0, 0);
+    if (!pflash_cfi01_register(map[VE_NORFLASH0], NULL, "vexpress.flash0",
+            VEXPRESS_FLASH_SIZE, dinfo ? dinfo->bdrv : NULL,
+            VEXPRESS_FLASH_SECT_SIZE,
+            VEXPRESS_FLASH_SIZE / VEXPRESS_FLASH_SECT_SIZE, 4,
+            0x00, 0x89, 0x00, 0x18, 0)) {
+        fprintf(stderr, "vexpress: error registering flash 0.\n");
+    }
+
     /* VE_NORFLASH0ALIAS: not modelled */
-    /* VE_NORFLASH1: not modelled */
+
+    dinfo = drive_get(IF_PFLASH, 0, 1);
+    if (!pflash_cfi01_register(map[VE_NORFLASH1], NULL, "vexpress.flash1",
+            VEXPRESS_FLASH_SIZE, dinfo ? dinfo->bdrv : NULL,
+            VEXPRESS_FLASH_SECT_SIZE,
+            VEXPRESS_FLASH_SIZE / VEXPRESS_FLASH_SECT_SIZE, 4,
+            0x00, 0x89, 0x00, 0x18, 0)) {
+        fprintf(stderr, "vexpress: error registering flash 1.\n");
+    }

     sram_size = 0x2000000;
     memory_region_init_ram(sram, "vexpress.sram", sram_size);
-- 
1.7.5.4

Re: [Qemu-devel] [PATCH] Versatile Express: add modelling of NOR flash

[Stefan Weil]

Am 04.09.2012 19:08, schrieb Francesco Lavra:
> This patch adds modelling of the two NOR flash banks found in the
> Versatile Express motherboard. Tested with U-Boot running on an emulated
> Versatile Express A9. The alias of the first NOR flash in the Cortex-A
> Series memory map is not modelled.
>
> Signed-off-by: Francesco Lavra <francescolavra.fl@gmail.com>
> ---
>   hw/vexpress.c |   25 +++++++++++++++++++++++--
>   1 files changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/hw/vexpress.c b/hw/vexpress.c
> index b615844..695892b 100644
> --- a/hw/vexpress.c
> +++ b/hw/vexpress.c
> @@ -29,8 +29,12 @@
>   #include "sysemu.h"
>   #include "boards.h"
>   #include "exec-memory.h"
> +#include "blockdev.h"
> +#include "flash.h"
>
>   #define VEXPRESS_BOARD_ID 0x8e0
> +#define VEXPRESS_FLASH_SIZE (64 * 1024 * 1024)
> +#define VEXPRESS_FLASH_SECT_SIZE (256 * 1024)
>
>   static struct arm_boot_info vexpress_binfo;
>
> @@ -357,6 +361,7 @@ static void vexpress_common_init(const VEDBoardInfo
> *daughterboard,
>       qemu_irq pic[64];
>       uint32_t proc_id;
>       uint32_t sys_id;
> +    DriveInfo *dinfo;
>       ram_addr_t vram_size, sram_size;
>       MemoryRegion *sysmem = get_system_memory();
>       MemoryRegion *vram = g_new(MemoryRegion, 1);
> @@ -412,9 +417,25 @@ static void vexpress_common_init(const VEDBoardInfo
> *daughterboard,
>
>       sysbus_create_simple("pl111", map[VE_CLCD], pic[14]);
>
> -    /* VE_NORFLASH0: not modelled */
> +    dinfo = drive_get(IF_PFLASH, 0, 0);
> +    if (!pflash_cfi01_register(map[VE_NORFLASH0], NULL, "vexpress.flash0",
> +            VEXPRESS_FLASH_SIZE, dinfo ? dinfo->bdrv : NULL,
> +            VEXPRESS_FLASH_SECT_SIZE,
> +            VEXPRESS_FLASH_SIZE / VEXPRESS_FLASH_SECT_SIZE, 4,
> +            0x00, 0x89, 0x00, 0x18, 0)) {
> +        fprintf(stderr, "vexpress: error registering flash 0.\n");
> +    }
> +
>       /* VE_NORFLASH0ALIAS: not modelled */

What about that alias? It's not difficult to add it, too.
Just look for memory_region_init_alias in the code to
see how it is done (hw/mips_malta.c has an alias region
for flash).

Regards,


>
> -    /* VE_NORFLASH1: not modelled */
> +
> +    dinfo = drive_get(IF_PFLASH, 0, 1);
> +    if (!pflash_cfi01_register(map[VE_NORFLASH1], NULL, "vexpress.flash1",
> +            VEXPRESS_FLASH_SIZE, dinfo ? dinfo->bdrv : NULL,
> +            VEXPRESS_FLASH_SECT_SIZE,
> +            VEXPRESS_FLASH_SIZE / VEXPRESS_FLASH_SECT_SIZE, 4,
> +            0x00, 0x89, 0x00, 0x18, 0)) {
> +        fprintf(stderr, "vexpress: error registering flash 1.\n");
> +    }
>
>       sram_size = 0x2000000;
>       memory_region_init_ram(sram, "vexpress.sram", sram_size);

Re: [Qemu-devel] [PATCH] Versatile Express: add modelling of NOR flash

[Peter Maydell]

On 5 September 2012 06:16, Stefan Weil <sw@weilnetz.de> wrote:
> Am 04.09.2012 19:08, schrieb Francesco Lavra:
>>       /* VE_NORFLASH0ALIAS: not modelled */
>
>
> What about that alias? It's not difficult to add it, too.
> Just look for memory_region_init_alias in the code to
> see how it is done (hw/mips_malta.c has an alias region
> for flash).

It's painful because you might also have to add the logic for
letting the guest map and unmap the alias (which implies
implementing a whole section of the A15 board we don't currently
bother with, the SCC registers). I'd need to check the board
documentation more carefully to see if we can get away with
always mapping that area as the flash alias.

(Also we'd need to fix the current problem with the
motherboard address map arrays that there's no way to
distinguish "peripheral not present on this board" from
"peripheral at address 0", since the A9 board doesn't have
the flash alias.)

More to the point, this is the third attempt at doing this.
Previously Liming Wang sent a patch:
 http://patchwork.ozlabs.org/patch/147905/
and Jagan sent a two-patch set:
 http://patchwork.ozlabs.org/patch/171812/
 http://patchwork.ozlabs.org/patch/171814/

both of which failed in the code review stage. Francesco,
can you check that you haven't fallen into any of the
same problems they did, please?

-- PMM

Re: [Qemu-devel] [PATCH] Versatile Express: add modelling of NOR flash

[Francesco Lavra]

Hi,

On 09/05/2012 10:47 AM, Peter Maydell wrote:
> On 5 September 2012 06:16, Stefan Weil <sw@weilnetz.de> wrote:
>> Am 04.09.2012 19:08, schrieb Francesco Lavra:
>>>       /* VE_NORFLASH0ALIAS: not modelled */
>>
>>
>> What about that alias? It's not difficult to add it, too.
>> Just look for memory_region_init_alias in the code to
>> see how it is done (hw/mips_malta.c has an alias region
>> for flash).
> 
> It's painful because you might also have to add the logic for
> letting the guest map and unmap the alias (which implies
> implementing a whole section of the A15 board we don't currently
> bother with, the SCC registers). I'd need to check the board
> documentation more carefully to see if we can get away with
> always mapping that area as the flash alias.

Documentation at
http://infocenter.arm.com/help/topic/com.arm.doc.ddi0503c/CHDEFDJF.html
says that the entire first 512 MB can be mapped to either SMC (which is
the default) or AXI, so if AXI is selected neither of the 2 flash banks
is visible. Also, the same doc says that it's possible to map either
NOR0 (default) or NOR1 to the address 0x00000000. This implies that in
the A Series memory map VE_NORFLASH0 should be at 0x08000000 and
VE_NORFLASH0ALIAS at 0x00000000, not the other way around (by the way,
this is also how U-Boot defines the memory for the A5 CoreTile). Maybe
worth a patch?

If we can get way with always aliasing to flash 0, the actual
implementation of the alias is made difficult by the fact that
memory_region_init_alias() needs the MemoryRegion of the aliased memory,
and the daughterboard-specific initialization is done in a function
which doesn't have access to that MemoryRegion. So we can either:
1. move initialization of common flash modelling before
daughterboard-specific initialization and pass the relevant MemoryRegion
to the daughterboard-specific init function
2. add another field to VEDBoardInfo which tells if the alias capability
is implemented, and use this info in vexpress_common_init() to define
the alias if appropriate
Or we can simply deem this alias not worth the trouble, which is what I
thought before sending the patch... Let me know your thoughts.

> 
> (Also we'd need to fix the current problem with the
> motherboard address map arrays that there's no way to
> distinguish "peripheral not present on this board" from
> "peripheral at address 0", since the A9 board doesn't have
> the flash alias.)
> 
> More to the point, this is the third attempt at doing this.
> Previously Liming Wang sent a patch:
>  http://patchwork.ozlabs.org/patch/147905/
> and Jagan sent a two-patch set:
>  http://patchwork.ozlabs.org/patch/171812/
>  http://patchwork.ozlabs.org/patch/171814/
> 
> both of which failed in the code review stage. Francesco,
> can you check that you haven't fallen into any of the
> same problems they did, please?

I read the reviews of previous attempts, and in fact there is a fix
which can be easily done, i.e. replacing the calls to drive_get() with
drive_get_next(). Will do that in v2, but first the above points need to
be addressed.

Thanks,
Francesco

Re: [Qemu-devel] [PATCH] Versatile Express: add modelling of NOR flash

[Francesco Lavra]

On 09/05/2012 09:07 PM, Francesco Lavra wrote:
> Hi,
> 
> On 09/05/2012 10:47 AM, Peter Maydell wrote:
>> On 5 September 2012 06:16, Stefan Weil <sw@weilnetz.de> wrote:
>>> Am 04.09.2012 19:08, schrieb Francesco Lavra:
>>>>       /* VE_NORFLASH0ALIAS: not modelled */
>>>
>>>
>>> What about that alias? It's not difficult to add it, too.
>>> Just look for memory_region_init_alias in the code to
>>> see how it is done (hw/mips_malta.c has an alias region
>>> for flash).
>>
>> It's painful because you might also have to add the logic for
>> letting the guest map and unmap the alias (which implies
>> implementing a whole section of the A15 board we don't currently
>> bother with, the SCC registers). I'd need to check the board
>> documentation more carefully to see if we can get away with
>> always mapping that area as the flash alias.
> 
> Documentation at
> http://infocenter.arm.com/help/topic/com.arm.doc.ddi0503c/CHDEFDJF.html
> says that the entire first 512 MB can be mapped to either SMC (which is
> the default) or AXI, so if AXI is selected neither of the 2 flash banks
> is visible. Also, the same doc says that it's possible to map either
> NOR0 (default) or NOR1 to the address 0x00000000. This implies that in
> the A Series memory map VE_NORFLASH0 should be at 0x08000000 and
> VE_NORFLASH0ALIAS at 0x00000000, not the other way around (by the way,
> this is also how U-Boot defines the memory for the A5 CoreTile). Maybe
> worth a patch?
> 
> If we can get way with always aliasing to flash 0, the actual
> implementation of the alias is made difficult by the fact that
> memory_region_init_alias() needs the MemoryRegion of the aliased memory,
> and the daughterboard-specific initialization is done in a function
> which doesn't have access to that MemoryRegion. So we can either:
> 1. move initialization of common flash modelling before
> daughterboard-specific initialization and pass the relevant MemoryRegion
> to the daughterboard-specific init function
> 2. add another field to VEDBoardInfo which tells if the alias capability
> is implemented, and use this info in vexpress_common_init() to define
> the alias if appropriate
> Or we can simply deem this alias not worth the trouble, which is what I
> thought before sending the patch... Let me know your thoughts.
> 
>>
>> (Also we'd need to fix the current problem with the
>> motherboard address map arrays that there's no way to
>> distinguish "peripheral not present on this board" from
>> "peripheral at address 0", since the A9 board doesn't have
>> the flash alias.)
>>
>> More to the point, this is the third attempt at doing this.
>> Previously Liming Wang sent a patch:
>>  http://patchwork.ozlabs.org/patch/147905/
>> and Jagan sent a two-patch set:
>>  http://patchwork.ozlabs.org/patch/171812/
>>  http://patchwork.ozlabs.org/patch/171814/
>>
>> both of which failed in the code review stage. Francesco,
>> can you check that you haven't fallen into any of the
>> same problems they did, please?
> 
> I read the reviews of previous attempts, and in fact there is a fix
> which can be easily done, i.e. replacing the calls to drive_get() with
> drive_get_next(). Will do that in v2, but first the above points need to
> be addressed.
> 
> Thanks,
> Francesco

Ping?
http://thread.gmane.org/gmane.comp.emulators.qemu/168461

Re: [Qemu-devel] [PATCH] Versatile Express: add modelling of NOR flash

[Peter Maydell]

On 5 September 2012 20:07, Francesco Lavra <francescolavra.fl@gmail.com> wrote:
> Documentation at
> http://infocenter.arm.com/help/topic/com.arm.doc.ddi0503c/CHDEFDJF.html
> says that the entire first 512 MB can be mapped to either SMC (which is
> the default) or AXI, so if AXI is selected neither of the 2 flash banks
> is visible. Also, the same doc says that it's possible to map either
> NOR0 (default) or NOR1 to the address 0x00000000. This implies that in
> the A Series memory map VE_NORFLASH0 should be at 0x08000000 and
> VE_NORFLASH0ALIAS at 0x00000000, not the other way around (by the way,
> this is also how U-Boot defines the memory for the A5 CoreTile). Maybe
> worth a patch?

Agreed, NORFLASH0 should be at 0x08000000 in the new memory map. Unless
anybody actually needs the aliasing of the flash at address zero, I
suggest just deleting all references to VE_NORFLASH0ALIAS.

-- PMM

Re: [Qemu-devel] [PATCH] Versatile Express: add modelling of NOR flash

[Francesco Lavra]

On 09/17/2012 03:21 PM, Peter Maydell wrote:
> On 5 September 2012 20:07, Francesco Lavra <francescolavra.fl@gmail.com> wrote:
>> Documentation at
>> http://infocenter.arm.com/help/topic/com.arm.doc.ddi0503c/CHDEFDJF.html
>> says that the entire first 512 MB can be mapped to either SMC (which is
>> the default) or AXI, so if AXI is selected neither of the 2 flash banks
>> is visible. Also, the same doc says that it's possible to map either
>> NOR0 (default) or NOR1 to the address 0x00000000. This implies that in
>> the A Series memory map VE_NORFLASH0 should be at 0x08000000 and
>> VE_NORFLASH0ALIAS at 0x00000000, not the other way around (by the way,
>> this is also how U-Boot defines the memory for the A5 CoreTile). Maybe
>> worth a patch?
> 
> Agreed, NORFLASH0 should be at 0x08000000 in the new memory map. Unless
> anybody actually needs the aliasing of the flash at address zero, I
> suggest just deleting all references to VE_NORFLASH0ALIAS.

Ok, thanks, then I'm going to send a 2 patch series, which first will
fix NORFLASH0 and remove NORFLASH0ALIAS, and then will actually add the
flash modelling.