From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39144) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bMeDH-0006DY-K0 for qemu-devel@nongnu.org; Mon, 11 Jul 2016 12:37:48 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bMeDG-0007nq-3o for qemu-devel@nongnu.org; Mon, 11 Jul 2016 12:37:43 -0400 Received: from 3.mo68.mail-out.ovh.net ([46.105.58.60]:38838) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bMeDE-0007nS-CH for qemu-devel@nongnu.org; Mon, 11 Jul 2016 12:37:42 -0400 Received: from player788.ha.ovh.net (b7.ovh.net [213.186.33.57]) by mo68.mail-out.ovh.net (Postfix) with ESMTP id 6B556FF8FE0 for ; Mon, 11 Jul 2016 18:37:35 +0200 (CEST) References: <1467634738-28642-1-git-send-email-clg@kaod.org> <1467634738-28642-5-git-send-email-clg@kaod.org> <577AA388.3060100@gmail.com> From: =?UTF-8?Q?C=c3=a9dric_Le_Goater?= Message-ID: Date: Mon, 11 Jul 2016 18:37:16 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 4/7] m25p80: add a m25p80_set_rom_storage() routine List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Crosthwaite , "mar.krzeminski" Cc: Peter Maydell , Peter Crosthwaite , "qemu-devel@nongnu.org Developers" , qemu-arm , Andrew Jeffery , Qemu-block , Alistair Francis Hello, On 07/10/2016 01:38 AM, Peter Crosthwaite wrote: > On Mon, Jul 4, 2016 at 10:57 AM, mar.krzeminski > wrote: >> >> >> W dniu 04.07.2016 o 14:18, C=C3=A9dric Le Goater pisze: >>> >>> Some SPI controllers, such as the Aspeed AST2400, have a mode in whic= h >>> accesses to the flash content are no different than doing MMIOs. The >>> controller generates all the necessary commands to load (or store) >>> data in memory. >>> >>> To emulate such a behavior, we need to have access to the underlying >>> storage of the flash object. The purpose of the routine is to set the >>> storage of a preinitialized SPI flash object, which can then be used >>> by the object itself but also by a SPI controller to handled the >>> MMIOs. >> >> Hi, >> >> I am not sure if this approach is correct. I can not find any datashee= t >> to this SPI controller, but as you mentioned in first paragraph, contr= oller >> generates all commands (probably ones are somehow configurable). >> In this series you hack this behaviour and you do direct access to fil= e. >> IMHO you should emulate sending such commands in SPI controller >> model. >> >=20 > Actually I think this is a good approach for two reasons. >=20 > Firstly there is more than one SPI controller that does this, the > Xilinx Zynq QSPI controller does this for its LQSPI mode. See > lqspi_read() in hw/ssi/xilinx_spips.c, which already works the way > Marcin proposes. That one is semi-automatic, in that there are > software configurable registers that hold the actual command to do the > read etc. But once setup, the interface is linear-read-only and > software transparent. A lot of assumptions where made in writing that > code (the buffering scheme is completely made up) and I think the > direct approach might be cleaner. This approach can go beyond SPI, in > that it can work for other protocols and external storage devices. ok. The Aspeed SPI controller could follow the SPI approach and initiate=20 SPI tranfers instead of accessing the underlying storage of the flash=20 module. This is not strictly necessary for that case.=20 But, being able to use the SPI flash module as a boot ROM (as you point out below) brings in extra needs. A rom memory region is required for=20 that as you need to share the storage with the flash object. Sharing=20 gives you direct access to the storage and then, generating SPI transfers= =20 becomes a bit superfluous.=20 I tried a few other approaches, adding aliases, adding a region without ops behind the flash object but they were not very convincing.=20 > The more interesting application of this approach though, is emulating > boot ROMs. Multiple SoCs in tree have pty bootroms that read SD cards > (or even SPI), mount file-systems and then blob+boot images. Rather > than emulators having to talk SDHCI through the controller to get > images, we should be able to go direct, and implement the boot-logic > off the raw block device. This means that there should be a general > approach to a machine fishing the backing block-dev out from an > external storage device, and linear SPI is another good application if > it. So if I read you well, the m25p80_set_rom_storage() approach is not the=20 most hideous thing to do but it needs to be generalized. Currently, it boils down to : * create a rom memory region for each flash module needing one : memory_region_init_rom_device(&fl->mmio, OBJECT(s), &flash_ops, fl, name, fl->size, &err); * if needed, storage can be saved for later usage with : flash->storage =3D memory_region_get_ram_ptr(&fl->mmio); * the memory region storage should be passed on to the flash object=20 using m25p80_set_rom_storage() before realization of the object : fl->flash =3D ssi_create_slave_no_init(s->spi, flashtype); if (dinfo) { qdev_prop_set_drive(fl->flash, "drive", blk_by_legacy_dinfo(d= info), errp); } m25p80_set_rom_storage(fl->flash, &fl->mmio); qdev_init_nofail(fl->flash); This is very m25p80 centric. A common solution could be an object=20 gathering the properties of a memory region rom device and a blk=20 device. This is a bit what the pflash_cfi* objects are proposing=20 but it is all bundled in a big specific object. Thanks, C.=20 > Regards, > Peter >=20 >> Thanks, >> Marcin >> >> >>> This is sufficient to support read only accesses. For writes, we woul= d >>> certainly need to externalize flash_write8() routine. >>> >>> Signed-off-by: C=C3=A9dric Le Goater >>> --- >>> hw/block/m25p80.c | 14 +++++++++++++- >>> include/hw/block/flash.h | 2 ++ >>> 2 files changed, 15 insertions(+), 1 deletion(-) >>> >>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c >>> index aa964280beab..fec0fd4c2228 100644 >>> --- a/hw/block/m25p80.c >>> +++ b/hw/block/m25p80.c >>> @@ -29,6 +29,7 @@ >>> #include "qemu/bitops.h" >>> #include "qemu/log.h" >>> #include "qapi/error.h" >>> +#include "hw/block/flash.h" >>> #ifndef M25P80_ERR_DEBUG >>> #define M25P80_ERR_DEBUG 0 >>> @@ -1152,7 +1153,11 @@ static void m25p80_realize(SSISlave *ss, Error >>> **errp) >>> if (s->blk) { >>> DB_PRINT_L(0, "Binding to IF_MTD drive\n"); >>> - s->storage =3D blk_blockalign(s->blk, s->size); >>> + >>> + /* using an external storage. see m25p80_create_rom() */ >>> + if (!s->storage) { >>> + s->storage =3D blk_blockalign(s->blk, s->size); >>> + } >>> if (blk_pread(s->blk, 0, s->storage, s->size) !=3D s->siz= e) { >>> error_setg(errp, "failed to read the initial flash >>> content"); >>> @@ -1259,3 +1264,10 @@ static void m25p80_register_types(void) >>> } >>> type_init(m25p80_register_types) >>> + >>> +void m25p80_set_rom_storage(DeviceState *dev, MemoryRegion *rom) >>> +{ >>> + Flash *s =3D M25P80(dev); >>> + >>> + s->storage =3D memory_region_get_ram_ptr(rom); >>> +} >>> diff --git a/include/hw/block/flash.h b/include/hw/block/flash.h >>> index 50ccbbcf1352..9d780f4ae0c9 100644 >>> --- a/include/hw/block/flash.h >>> +++ b/include/hw/block/flash.h >>> @@ -61,4 +61,6 @@ uint8_t ecc_digest(ECCState *s, uint8_t sample); >>> void ecc_reset(ECCState *s); >>> extern VMStateDescription vmstate_ecc_state; >>> +void m25p80_set_rom_storage(DeviceState *dev, MemoryRegion *rom); >>> + >>> #endif >> >>