Skip to contentSkip to conversation

Versatile Express: add modelling of NOR flash

PATCH Superseded Francesco Lavra 2 participants 8 messages Sep 17, 2012 – Sep 19, 2012 Arm/versatile express platform

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Francesco Lavra

author
#0 lore raw

v2 cover Versatile Express: add modelling of NOR flash

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

Changes in v2:
Fix definition of NOR flash 0 address and remove flash alias in Cortex-A Series memory map.
Use drive_get_next() instead of drive_get() to get a backing storage for each flash bank.

Francesco Lavra (2):
Versatile Express: Fix NOR flash 0 address and remove flash alias Versatile Express: Add modelling of NOR flash

 hw/vexpress.c |   31 ++++++++++++++++++++++++-------
 1 files changed, 24 insertions(+), 7 deletions(-)
Signature · 1 line
1.7.5.4
author
#1 lore raw

PATCH v2 1/2 Versatile Express: Fix NOR flash 0 address and remove flash alias

In the A series memory map (implemented in the Cortex A15 CoreTile), the first NOR flash bank (flash 0) is mapped to address 0x08000000, while address 0x00000000 can be configured as alias to either the first or the second flash bank. This patch fixes the definition of flash 0 address, and for simplicity removes the alias definition.

Signed-off-by Francesco Lavra <[email protected]>
---
 hw/vexpress.c |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)
hw/vexpress.c +2 -4
index b615844..454c2bb 100644
@@ -62,7 +62,6 @@ enum {
     VE_COMPACTFLASH,
     VE_CLCD,
     VE_NORFLASH0,
-    VE_NORFLASH0ALIAS,
     VE_NORFLASH1,
     VE_SRAM,
     VE_VIDEORAM,

@@ -104,9 +103,8 @@ static target_phys_addr_t motherboard_legacy_map[] = {
 };
 
 static target_phys_addr_t motherboard_aseries_map[] = {
-    /* CS0: 0x00000000 .. 0x0c000000 */
-    [VE_NORFLASH0] = 0x00000000,
-    [VE_NORFLASH0ALIAS] = 0x08000000,
+    /* CS0: 0x08000000 .. 0x0c000000 */
+    [VE_NORFLASH0] = 0x08000000,
     /* CS4: 0x0c000000 .. 0x10000000 */
     [VE_NORFLASH1] = 0x0c000000,
     /* CS5: 0x10000000 .. 0x14000000 */

@@ -413,7 +411,6 @@ static void vexpress_common_init(const VEDBoardInfo

*daughterboard,

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

     /* VE_NORFLASH0: not modelled */
-    /* VE_NORFLASH0ALIAS: not modelled */
     /* VE_NORFLASH1: not modelled */

     sram_size = 0x2000000;
Signature · 1 line
1.7.5.4
Reviewed-by on v2 1/2
#2 lore raw

Re: [Qemu-devel] [PATCH v2 1/2] Versatile Express: Fix NOR flash 0 address and remove flash alias

Quoted 7 lines · Francesco Lavra

In the A series memory map (implemented in the Cortex A15 CoreTile), the first NOR flash bank (flash 0) is mapped to address 0x08000000, while address 0x00000000 can be configured as alias to either the first or the second flash bank. This patch fixes the definition of flash 0 address, and for simplicity removes the alias definition.

Signed-off-by Francesco Lavra <[email protected]>
Reviewed-by Peter Maydell <[email protected]>

-- PMM

author
#3 lore raw

PATCH v2 2/2 Versatile Express: add modelling of NOR flash

In reply to Francesco Lavra

This patch adds modelling of the two NOR flash banks found on the Versatile Express motherboard. Tested with U-Boot running on an emulated Versatile Express, with either A9 or A15 CoreTile.

Signed-off-by Francesco Lavra <[email protected]>

---
Changes in v2:
Use drive_get_next() instead of drive_get() to get a backing storage for each flash bank.

 hw/vexpress.c |   24 ++++++++++++++++++++++--
 1 files changed, 22 insertions(+), 2 deletions(-)
hw/vexpress.c +4 -0
index 454c2bb..2ffeab1 100644
@@ -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;
 

@@ -355,6 +359,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);

@@ -410,8 +415,23 @@ static void vexpress_common_init(const VEDBoardInfo *daughterboard,

     sysbus_create_simple("pl111", map[VE_CLCD], pic[14]);
-    /* VE_NORFLASH0: not modelled */
-    /* VE_NORFLASH1: not modelled */
+    dinfo = drive_get_next(IF_PFLASH);
+    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");
+ }
+
+ dinfo = drive_get_next(IF_PFLASH);
+ 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);
Signature · 1 line
1.7.5.4
#4 lore raw
Quoted 31 lines · Francesco Lavra

This patch adds modelling of the two NOR flash banks found on the Versatile Express motherboard. Tested with U-Boot running on an emulated Versatile Express, with either A9 or A15 CoreTile.

Signed-off-by Francesco Lavra <[email protected]>

---
Changes in v2:
Use drive_get_next() instead of drive_get() to get a backing storage for each flash bank.

 hw/vexpress.c |   24 ++++++++++++++++++++++--
 1 files changed, 22 insertions(+), 2 deletions(-)
hw/vexpress.c +4 -0
index 454c2bb..2ffeab1 100644
@@ -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;

hw/vexpress.c static void vexpress_common_init(const VEDBoardInfo · line 359

Quoted 1 line
hw/vexpress.c +0 -0
@@ -355,6 +359,7 @@ static void vexpress_common_init(const VEDBoardInfo
 *daughterboard,

Something in your email send path is wrapping long lines, which means 'git am' doesn't work cleanly. If you're planning on sending more QEMU patches you might want to look into getting this fixed.

Quoted 11 lines
hw/vexpress.c +1 -0
     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);
hw/vexpress.c +0 -0
@@ -410,8 +415,23 @@ static void vexpress_common_init(const VEDBoardInfo
 *daughterboard,
 
     sysbus_create_simple("pl111", map[VE_CLCD], pic[14]);

hw/vexpress.c static void vexpress_common_init(const VEDBoardInfo · line 425

Quoted 10 lines
hw/vexpress.c +7 -2
 
-    /* VE_NORFLASH0: not modelled */
-    /* VE_NORFLASH1: not modelled */
+    dinfo = drive_get_next(IF_PFLASH);
+    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");

Shouldn't these errors be fatal?

Quoted 13 lines
hw/vexpress.c +10 -0
+    }
+
+    dinfo = drive_get_next(IF_PFLASH);
+    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);
Quoted 2 lines

--
1.7.5.4

Otherwise looks OK.

-- PMM

author
#5 lore raw
Quoted 29 lines · Peter Maydell
Quoted 28 lines · Francesco Lavra

This patch adds modelling of the two NOR flash banks found on the Versatile Express motherboard. Tested with U-Boot running on an emulated Versatile Express, with either A9 or A15 CoreTile.

Signed-off-by Francesco Lavra <[email protected]>

---
Changes in v2:
Use drive_get_next() instead of drive_get() to get a backing storage for each flash bank.

 hw/vexpress.c |   24 ++++++++++++++++++++++--
 1 files changed, 22 insertions(+), 2 deletions(-)
hw/vexpress.c +4 -0
index 454c2bb..2ffeab1 100644
@@ -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)
Quoted 8 lines

hw/vexpress.c static void vexpress_common_init(const VEDBoardInfo · line 359

Quoted 4 lines
hw/vexpress.c +0 -0
 static struct arm_boot_info vexpress_binfo;
 

@@ -355,6 +359,7 @@ static void vexpress_common_init(const VEDBoardInfo
 *daughterboard,

Something in your email send path is wrapping long lines, which means 'git am' doesn't work cleanly. If you're planning on sending more QEMU patches you might want to look into getting this fixed.

Oops, sorry about that, if you want me to re-send the patch with these artifacts fixed, just let me know.

Quoted 12 lines
Quoted 12 lines
hw/vexpress.c +1 -0
     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);
hw/vexpress.c +0 -0
@@ -410,8 +415,23 @@ static void vexpress_common_init(const VEDBoardInfo
 *daughterboard,
 
     sysbus_create_simple("pl111", map[VE_CLCD], pic[14]);
Quoted 11 lines

hw/vexpress.c static void vexpress_common_init(const VEDBoardInfo · line 425

Quoted 9 lines
hw/vexpress.c +7 -2
-    /* VE_NORFLASH0: not modelled */
-    /* VE_NORFLASH1: not modelled */
+    dinfo = drive_get_next(IF_PFLASH);
+    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");

Shouldn't these errors be fatal?

I checked the existing uses of pflash_cfi_0[1,2]_register() in the code, and if I'm not mistaken only in 5 out of 19 devices these errors are fatal, in the other 14 cases initialization continues even after flash registration failure, with or without an error message. Let me know if you still prefer these errors to be fatal.

Quoted 11 lines
Quoted 11 lines
hw/vexpress.c +10 -0
+    }
+
+    dinfo = drive_get_next(IF_PFLASH);
+    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");
+    }
Quoted 8 lines
Quoted 4 lines
hw/vexpress.c +0 -0
     sram_size = 0x2000000;

     memory_region_init_ram(sram, "vexpress.sram", sram_size);

--
1.7.5.4

Otherwise looks OK.

-- PMM

Thanks,
Francesco

#6 lore raw
Quoted 66 lines · Francesco Lavra
Quoted 29 lines · Peter Maydell
Quoted 28 lines · Francesco Lavra

This patch adds modelling of the two NOR flash banks found on the Versatile Express motherboard. Tested with U-Boot running on an emulated Versatile Express, with either A9 or A15 CoreTile.

Signed-off-by Francesco Lavra <[email protected]>

---
Changes in v2:
Use drive_get_next() instead of drive_get() to get a backing storage for each flash bank.

 hw/vexpress.c |   24 ++++++++++++++++++++++--
 1 files changed, 22 insertions(+), 2 deletions(-)
hw/vexpress.c +4 -0
index 454c2bb..2ffeab1 100644
@@ -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)
Quoted 8 lines

hw/vexpress.c static void vexpress_common_init(const VEDBoardInfo · line 359

Quoted 4 lines
hw/vexpress.c +0 -0
 static struct arm_boot_info vexpress_binfo;
 

@@ -355,6 +359,7 @@ static void vexpress_common_init(const VEDBoardInfo
 *daughterboard,

Something in your email send path is wrapping long lines, which means 'git am' doesn't work cleanly. If you're planning on sending more QEMU patches you might want to look into getting this fixed.

Oops, sorry about that, if you want me to re-send the patch with these artifacts fixed, just let me know.

Quoted 12 lines
Quoted 12 lines
hw/vexpress.c +1 -0
     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);
hw/vexpress.c +0 -0
@@ -410,8 +415,23 @@ static void vexpress_common_init(const VEDBoardInfo
 *daughterboard,
 
     sysbus_create_simple("pl111", map[VE_CLCD], pic[14]);
Quoted 11 lines

hw/vexpress.c static void vexpress_common_init(const VEDBoardInfo · line 425

Quoted 9 lines
hw/vexpress.c +7 -2
-    /* VE_NORFLASH0: not modelled */
-    /* VE_NORFLASH1: not modelled */
+    dinfo = drive_get_next(IF_PFLASH);
+    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");

Shouldn't these errors be fatal?

Quoted 5 lines

I checked the existing uses of pflash_cfi_0[1,2]_register() in the code, and if I'm not mistaken only in 5 out of 19 devices these errors are fatal, in the other 14 cases initialization continues even after flash registration failure, with or without an error message. Let me know if you still prefer these errors to be fatal.

So the only reason this can fail is if the user specified a file to back the flash but trying to read it failed (ie, bad filename or file not the same size as the flash). I think that merits actually stopping on error.

Ideally in the long term the flash devices should be converted to proper QOM devices and we could push the error handling back into the device itself (which is better positioned to distinguish "bad filename" from "wrong length" I suspect).

-- PMM

author
#7 lore raw
Quoted 38 lines · Peter Maydell
Quoted 26 lines · Francesco Lavra
Quoted 13 lines · Peter Maydell
Quoted 12 lines · Francesco Lavra
     qemu_irq pic[64];
     uint32_t proc_id;
     uint32_t sys_id;
(quoted diff) +1 -0
+    DriveInfo *dinfo;
     ram_addr_t vram_size, sram_size;
     MemoryRegion *sysmem = get_system_memory();
     MemoryRegion *vram = g_new(MemoryRegion, 1);

@@ -410,8 +415,23 @@ static void vexpress_common_init(const VEDBoardInfo
 *daughterboard,
 
     sysbus_create_simple("pl111", map[VE_CLCD], pic[14]);
Quoted 11 lines

Review static void vexpress_common_init(const VEDBoardInfo · line 425

Quoted 9 lines
(quoted diff) +7 -2
-    /* VE_NORFLASH0: not modelled */
-    /* VE_NORFLASH1: not modelled */
+    dinfo = drive_get_next(IF_PFLASH);
+    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");

Shouldn't these errors be fatal?

Quoted 5 lines

I checked the existing uses of pflash_cfi_0[1,2]_register() in the code, and if I'm not mistaken only in 5 out of 19 devices these errors are fatal, in the other 14 cases initialization continues even after flash registration failure, with or without an error message. Let me know if you still prefer these errors to be fatal.

So the only reason this can fail is if the user specified a file to back the flash but trying to read it failed (ie, bad filename or file not the same size as the flash). I think that merits actually stopping on error.

Quoted 4 lines

Ideally in the long term the flash devices should be converted to proper QOM devices and we could push the error handling back into the device itself (which is better positioned to distinguish "bad filename" from "wrong length" I suspect).

Ok, I will make these errors fatal in v3.

--
Francesco