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

[Marcin Krzemiński]

2016-12-05 10:36 GMT+01:00 Cédric Le Goater <address@hidden>:

> 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 ?
>
It could be that I mess up everything because my understanding how the real
HW
work and boot is wrong. Please correct my assumptions:
1. You are setting boot source to spi-nor (jumper, resistor whatever)
2. There is a small bootrom in SoC (not u-boot) that set eg. reset vector,
configure SMC
and start execude code from flash.
3. Memory mapped flash content is at address 0x1c000000.


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

I think not, if you have fs on flash (eg. UBI) and smaller image fs will
not mount,
or in worse case you can issue writes outside the file.

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

This is exactly what i meant, I was thinking that fl->size is the size of
the whole flash,
not the size of memory mapped part.

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

I do not think there is a reason for doing that yet :)

Thanks,
Marcin

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