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

[Francesco Lavra]

On 09/19/2012 12:26 PM, Peter Maydell wrote:
> On 18 September 2012 21:59, Francesco Lavra <address@hidden> wrote:
>> On 09/18/2012 03:46 PM, Peter Maydell wrote:
>>> On 17 September 2012 21:08, Francesco Lavra <address@hidden> 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