From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:57727) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T9Kvs-0007lO-It for qemu-devel@nongnu.org; Wed, 05 Sep 2012 15:06:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T9Kvr-0004xt-1x for qemu-devel@nongnu.org; Wed, 05 Sep 2012 15:06:36 -0400 Received: from mail-bk0-f45.google.com ([209.85.214.45]:62380) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T9Kvq-0004xU-Qt for qemu-devel@nongnu.org; Wed, 05 Sep 2012 15:06:34 -0400 Received: by bkcji1 with SMTP id ji1so373002bkc.4 for ; Wed, 05 Sep 2012 12:06:33 -0700 (PDT) Message-ID: <5047A2EB.3070005@gmail.com> Date: Wed, 05 Sep 2012 21:07:23 +0200 From: Francesco Lavra MIME-Version: 1.0 References: <5046357B.1010101@gmail.com> <5046E022.5020009@weilnetz.de> In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] Versatile Express: add modelling of NOR flash List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: Stefan Weil , qemu-devel@nongnu.org, paul@codesourcery.com Hi, On 09/05/2012 10:47 AM, Peter Maydell wrote: > On 5 September 2012 06:16, Stefan Weil 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