Re: [Qemu-block] [PATCH 4/7] m25p80: add a m25p80_set_rom_storage() routine

[Cédric Le Goater]

Hello,

On 07/10/2016 01:38 AM, Peter Crosthwaite wrote:
> On Mon, Jul 4, 2016 at 10:57 AM, mar.krzeminski
> <address@hidden> wrote:
>>
>>
>> W dniu 04.07.2016 o 14:18, Cédric Le Goater pisze:
>>>
>>> Some SPI controllers, such as the Aspeed AST2400, have a mode in which
>>> 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 datasheet
>> to this SPI controller, but as you mentioned in first paragraph, controller
>> generates all commands (probably ones are somehow configurable).
>> In this series you hack this behaviour and you do direct access to file.
>> IMHO you should emulate sending such commands in SPI controller
>> model.
>>
> 
> Actually I think this is a good approach for two reasons.
> 
> 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 
SPI tranfers instead of accessing the underlying storage of the flash 
module. This is not strictly necessary for that case. 

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 
that as you need to share the storage with the flash object. Sharing 
gives you direct access to the storage and then, generating SPI transfers 
becomes a bit superfluous. 

I tried a few other approaches, adding aliases, adding a region without
ops behind the flash object but they were not very convincing. 


> 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 
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 = memory_region_get_ram_ptr(&fl->mmio);


 * the memory region storage should be passed on to the flash object 
   using m25p80_set_rom_storage() before realization of the object :

        fl->flash = ssi_create_slave_no_init(s->spi, flashtype);
        if (dinfo) {
            qdev_prop_set_drive(fl->flash, "drive", blk_by_legacy_dinfo(dinfo),
                                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 
gathering the properties of a memory region rom device and a blk 
device. This is a bit what the pflash_cfi* objects are proposing 
but it is all bundled in a big specific object.


Thanks,

C. 

> Regards,
> Peter
> 
>> Thanks,
>> Marcin
>>
>>
>>> This is sufficient to support read only accesses. For writes, we would
>>> certainly need to externalize flash_write8() routine.
>>>
>>> Signed-off-by: Cédric Le Goater <address@hidden>
>>> ---
>>>   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 = blk_blockalign(s->blk, s->size);
>>> +
>>> +        /* using an external storage. see m25p80_create_rom() */
>>> +        if (!s->storage) {
>>> +            s->storage = blk_blockalign(s->blk, s->size);
>>> +        }
>>>             if (blk_pread(s->blk, 0, s->storage, s->size) != s->size) {
>>>               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 = M25P80(dev);
>>> +
>>> +    s->storage = 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
>>
>>