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 <address@hidden> 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