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

[Cédric Le Goater]

Hello,

On 07/04/2016 07:57 PM, mar.krzeminski 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.

I took some time to implement what you proposed, that is to emulate all 
the SPI commands needed to handle the read access. This is easily tested 
through the monitor. Looks good.

But the goal is to boot from the device, so I added a memory region alias 
at 0 to trigger the flash module mmios at boot time, as this is where 
u-boot expects to be. 

and I fell in this trap :/

        aspeed_smc_flash_read: To 0x0 of size 1: 0xbe mode:0
        Bad ram pointer (nil)
        Aborted (core dumped)

There is a failure in get_page_addr_code(), possibly because qemu uses
byte per byte reads of the code (cpu_ldub_code). But this is beyond my 
understanding of qemu's internal.


It seems that we do need a ROM. This is how firmwares are handled, with 
a load_image_targphys(), and also the pflash devices, through a ROM device 
region.


The proposal below allows to modify the backing region of the flash device
model for this purpose. A ROM device region can be used to enable booting 
from the device. It also allows some shortcuts in the model, like reading 
directly from the storage for the benefit of speed. This is what Peter 
Crosthwaite mentioned in a previous email. But this is not the primary 
use. Being able to use a ROM memory region is.

Shall I resend ? or are there strong objections ? 

Thanks,

C. 



> 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
>