Re: [Qemu-devel] [PATCH v2] hw/vexpress.c: Add NOR flash model

[Peter Maydell]

On 21 March 2012 03:37, Liming Wang <address@hidden> wrote:
> Vexpress motherboard has two 2x16 NOR flash, but pflash_cfi01
> doesn't support interleaving, so here only models two 1x32 flash.
> Although it's not exactly modeled, it works fine for running linux.

(Sorry for the delay getting round to reviewing this.)

> Signed-off-by: Liming Wang <address@hidden>
> ---
> v2:
>  - swap NORFLASH0 and NORFLASH0ALIAS and reserve NORFLASH0ALIAS if we
>    want to boot from flash0 in the future.
>
>  hw/vexpress.c |   28 ++++++++++++++++++++++------
>  1 files changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/hw/vexpress.c b/hw/vexpress.c
> index b9aafec..6a5bf4e 100644
> --- a/hw/vexpress.c
> +++ b/hw/vexpress.c
> @@ -29,9 +29,13 @@
>  #include "sysemu.h"
>  #include "boards.h"
>  #include "exec-memory.h"
> +#include "flash.h"
> +#include "blockdev.h"
>
>  #define VEXPRESS_BOARD_ID 0x8e0
>
> +#define VEXPRESS_FLASH_SIZE 0x04000000
> +
>  static struct arm_boot_info vexpress_binfo;
>
>  /* Address maps for peripherals:
> @@ -61,8 +65,8 @@ enum {
>     VE_RTC,
>     VE_COMPACTFLASH,
>     VE_CLCD,
> -    VE_NORFLASH0,
>     VE_NORFLASH0ALIAS,
> +    VE_NORFLASH0,
>     VE_NORFLASH1,
>     VE_SRAM,
>     VE_VIDEORAM,
> @@ -92,6 +96,7 @@ static target_phys_addr_t motherboard_legacy_map[] = {
>     [VE_COMPACTFLASH] = 0x1001a000,
>     [VE_CLCD] = 0x1001f000,
>     /* CS0: 0x40000000 .. 0x44000000 */
> +    [VE_NORFLASH0ALIAS] = 0x40000000,

This isn't right: you've ended up with NORFLASH0ALIAS being
the non-remapped address of the flash in the legacy map
but the remapped (0) address in the non-legacy map.

I would suggest just deleting all references to NORFLASH0ALIAS
from the file completely.

>     [VE_NORFLASH0] = 0x40000000,
>     /* CS1: 0x44000000 .. 0x48000000 */
>     [VE_NORFLASH1] = 0x44000000,
> @@ -105,8 +110,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,
> +    [VE_NORFLASH0ALIAS] = 0x00000000,
> +    [VE_NORFLASH0] = 0x08000000,
>     /* CS4: 0x0c000000 .. 0x10000000 */
>     [VE_NORFLASH1] = 0x0c000000,
>     /* CS5: 0x10000000 .. 0x14000000 */
> @@ -355,6 +360,9 @@ static void vexpress_common_init(const VEDBoardInfo 
> *daughterboard,
>     MemoryRegion *vram = g_new(MemoryRegion, 1);
>     MemoryRegion *sram = g_new(MemoryRegion, 1);
>     const target_phys_addr_t *map = daughterboard->motherboard_map;
> +    DriveInfo *dinfo = NULL;
> +    uint32_t sector_len = 256 * 1024;

These two are only used inside your for() loop so you can move
the declarations into that loop.

> +    int i = 0;

Initialiser not needed.

>
>     daughterboard->init(daughterboard, ram_size, cpu_model, pic, &proc_id);
>
> @@ -405,9 +413,17 @@ 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 */
> +    for (i = 0; i < 2; i++) {
> +        dinfo = drive_get(IF_PFLASH, 0, i);
> +        if (dinfo) {

This is a good place to include a brief comment:
 /* Strictly, the board has two 2x16 NOR flash, but pflash_cfi01
  * doesn't support interleaving, so we create two 1x32 flash instead.
  */

> +            pflash_cfi01_register(i ? map[VE_NORFLASH1] : map[VE_NORFLASH0],
> +                                 NULL,
> +                                 i ? "vexpress.flash1" : "vexpress:flash0",

I guess you mean ".flash0" here, not ":flash0" ?

> +                                 VEXPRESS_FLASH_SIZE, dinfo->bdrv, 
> sector_len,
> +                                 VEXPRESS_FLASH_SIZE / sector_len, 4,
> +                                 0, 0x89, 0x89, 0x19, 0);
> +        }
> +    }
>
>     sram_size = 0x2000000;
>     memory_region_init_ram(sram, "vexpress.sram", sram_size);
> --
> 1.7.0.4

-- PMM