Patchwork [v2,2/2] Versatile Express: add modelling of NOR flash

login
register
mail settings
Submitter Francesco Lavra
Date Sept. 17, 2012, 8:08 p.m.
Message ID <50578341.8010308@gmail.com>
Download mbox | patch
Permalink /patch/184530/
State New
Headers show

Comments

Francesco Lavra - Sept. 17, 2012, 8:08 p.m.
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 <francescolavra.fl@gmail.com>
---
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(-)

     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);
Peter Maydell - Sept. 18, 2012, 1:46 p.m.
On 17 September 2012 21:08, Francesco Lavra <francescolavra.fl@gmail.com> wrote:
> 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 <francescolavra.fl@gmail.com>
> ---
> 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(-)
>
> diff --git a/hw/vexpress.c b/hw/vexpress.c
> index 454c2bb..2ffeab1 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;
>
> @@ -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.

>      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");

Shouldn't these errors be fatal?

> +    }
> +
> +    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);
> --
> 1.7.5.4

Otherwise looks OK.

-- PMM
Francesco Lavra - Sept. 18, 2012, 8:59 p.m.
On 09/18/2012 03:46 PM, Peter Maydell wrote:
> On 17 September 2012 21:08, Francesco Lavra <francescolavra.fl@gmail.com> wrote:
>> 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 <francescolavra.fl@gmail.com>
>> ---
>> 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(-)
>>
>> diff --git a/hw/vexpress.c b/hw/vexpress.c
>> index 454c2bb..2ffeab1 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;
>>
>> @@ -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.

>>      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");
> 
> 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.

>> +    }
>> +
>> +    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);
>> --
>> 1.7.5.4
> 
> Otherwise looks OK.
> 
> -- PMM

Thanks,
Francesco
Peter Maydell - Sept. 19, 2012, 10:26 a.m.
On 18 September 2012 21:59, Francesco Lavra <francescolavra.fl@gmail.com> wrote:
> On 09/18/2012 03:46 PM, Peter Maydell wrote:
>> On 17 September 2012 21:08, Francesco Lavra <francescolavra.fl@gmail.com> wrote:
>>> 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 <francescolavra.fl@gmail.com>
>>> ---
>>> 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(-)
>>>
>>> diff --git a/hw/vexpress.c b/hw/vexpress.c
>>> index 454c2bb..2ffeab1 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;
>>>
>>> @@ -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.
>
>>>      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");
>>
>> 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.

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
Francesco Lavra - Sept. 19, 2012, 1:51 p.m.
On 09/19/2012 12:26 PM, Peter Maydell wrote:
> On 18 September 2012 21:59, Francesco Lavra <francescolavra.fl@gmail.com> wrote:
>> On 09/18/2012 03:46 PM, Peter Maydell wrote:
>>> On 17 September 2012 21:08, Francesco Lavra <francescolavra.fl@gmail.com> wrote:
>>>>      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");
>>>
>>> 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.
> 
> 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).

Ok, I will make these errors fatal in v3.

--
Francesco

Patch

diff --git a/hw/vexpress.c b/hw/vexpress.c
index 454c2bb..2ffeab1 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;

@@ -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();