Re: [Qemu-arm] [Qemu-devel] [PATCH for-2.9 24/30] aspeed: use first SPI flash as a boot ROM

[Cédric Le Goater]

Hello Marcin,

On 12/04/2016 06:00 PM, mar.krzeminski wrote:
> Hi Cedric,
> 
> it looks like good idea for now to handle boot from flash.
> As I understand you are trying to omit bootrom code in Qemu model?

I suppose you mean handling a romd memory region under the m25p80 
object ? 

> This could lead you to some hacks in device models (eg SMC).

I haven't had to, yet. 
 
> W dniu 29.11.2016 o 16:44, Cédric Le Goater pisze:
>> Fill a ROM region with the flash content to support U-Boot. This is a
>> little hacky but until we can boot from a MMIO region, it seems
>> difficult to do anything else.
>>
>> Signed-off-by: Cédric Le Goater <address@hidden>
>> Reviewed-by: Joel Stanley <address@hidden>
>> Reviewed-by: Andrew Jeffery <address@hidden>
>> ---
>>  hw/arm/aspeed.c | 41 +++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 41 insertions(+)
>>
>> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
>> index 40c13838fb2d..a92c2f1c362b 100644
>> --- a/hw/arm/aspeed.c
>> +++ b/hw/arm/aspeed.c
>> @@ -20,6 +20,8 @@
>>  #include "qemu/log.h"
>>  #include "sysemu/block-backend.h"
>>  #include "sysemu/blockdev.h"
>> +#include "hw/loader.h"
>> +#include "qemu/error-report.h"
>>  
>>  static struct arm_boot_info aspeed_board_binfo = {
>>      .board_id = -1, /* device-tree-only board */
>> @@ -104,6 +106,28 @@ static const AspeedBoardConfig aspeed_boards[] = {
>>      },
>>  };
>>  
>> +#define FIRMWARE_ADDR 0x0
>> +
>> +static void write_boot_rom(DriveInfo *dinfo, hwaddr addr, size_t rom_size,
>> +                           Error **errp)
>> +{
>> +    BlockBackend *blk = blk_by_legacy_dinfo(dinfo);
>> +    uint8_t *storage;
>> +
>> +    if (rom_size > blk_getlength(blk)) {
>> +        rom_size = blk_getlength(blk);
>> +    }
> I was not able to attach smaller file as m25p80 storage.

yes that's most probably because m25p80_realize() does : 

       if (blk_pread(s->blk, 0, s->storage, s->size) != s->size) {
            error_setg(errp, "failed to read the initial flash content");
            return;
        }

my bad. May be, we could relax a bit the test and allow smaller block 
backends ? 


>> +
>> +    storage = g_new0(uint8_t, rom_size);
>> +    if (blk_pread(blk, 0, storage, rom_size) < 0) {
>> +        error_setg(errp, "failed to read the initial flash content");
>> +        return;
>> +    }
>> +
>> +    rom_add_blob_fixed("aspeed.boot_rom", storage, rom_size, addr);
>> +    g_free(storage);
>> +}
>> +
>>  static void aspeed_board_init_flashes(AspeedSMCState *s, const char 
>> *flashtype,
>>                                        Error **errp)
>>  {
>> @@ -135,6 +159,7 @@ static void aspeed_board_init(MachineState *machine,
>>  {
>>      AspeedBoardState *bmc;
>>      AspeedSoCClass *sc;
>> +    DriveInfo *drive0 = drive_get(IF_MTD, 0, 0);
>>  
>>      bmc = g_new0(AspeedBoardState, 1);
>>      object_initialize(&bmc->soc, (sizeof(bmc->soc)), cfg->soc_name);
>> @@ -168,6 +193,22 @@ static void aspeed_board_init(MachineState *machine,
>>      aspeed_board_init_flashes(&bmc->soc.fmc, cfg->fmc_model, &error_abort);
>>      aspeed_board_init_flashes(&bmc->soc.spi[0], cfg->spi_model, 
>> &error_abort);
>>  
>> +    /* Install first FMC flash content as a boot rom. */
>> +    if (drive0) {
>> +        AspeedSMCFlash *fl = &bmc->soc.fmc.flashes[0];
>> +        MemoryRegion *boot_rom = g_new(MemoryRegion, 1);
>> +
>> +        /*
>> +         * create a ROM region using the default mapping window size of
>> +         * the flash module.
>> +         */
>> +        memory_region_init_rom(boot_rom, OBJECT(bmc), "aspeed.boot_rom",
>> +                               fl->size, &error_abort);
>> +        memory_region_add_subregion(get_system_memory(), FIRMWARE_ADDR,
>> +                                    boot_rom);
>> +        write_boot_rom(drive0, FIRMWARE_ADDR, fl->size, &error_abort);
>
> Is it possible that fl->size will be bigger than segment size?
>
> I think max_size here should be segment size in smc.

I am not sure what you mean by "segment" ? 

fl->size holds the default mapping window size on the AHB bus of the 
flash chip CS0. The size can be changed by the guest using the segment 
address registers but for CS0 this should be "autoconfigured" by HW.

Here, we are just using the default from the specs but we could go a 
little further in the model and setup the mapping window size of CS0 
using the block backend size, and set up the segment registers accordingly.
There are some checks to be done. It might be a little complex.

Thanks,

C. 


> Thanks,
> Marcin
>> +    }
>> +
>>      aspeed_board_binfo.kernel_filename = machine->kernel_filename;
>>      aspeed_board_binfo.initrd_filename = machine->initrd_filename;
>>      aspeed_board_binfo.kernel_cmdline = machine->kernel_cmdline;
>