Hi Cédric
> Subject: Re: [SPAM] [PATCH v1 01/21] hw/arm/aspeed_ast27x0-fc: Support
> VBootRom
>
> On 7/17/25 05:40, Jamin Lin wrote:
> > Introduces support for loading a vbootrom image into the dedicated
> > vbootrom memory region in the AST2700 Full Core machine.
> >
> > Additionally, it implements a mechanism to extract the content of
> > fmc_cs0 flash data(backend file) and copy it into the memory-mapped
> > region corresponding to ASPEED_DEV_SPI_BOOT.
> >
> > Signed-off-by: Jamin Lin <[email protected]>
> > ---
> > hw/arm/aspeed_ast27x0-fc.c | 75
> ++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 75 insertions(+)
> >
> > diff --git a/hw/arm/aspeed_ast27x0-fc.c b/hw/arm/aspeed_ast27x0-fc.c
> > index 7087be4288..e2eee6183f 100644
> > --- a/hw/arm/aspeed_ast27x0-fc.c
> > +++ b/hw/arm/aspeed_ast27x0-fc.c
> > @@ -11,6 +11,7 @@
> >
> > #include "qemu/osdep.h"
> > #include "qemu/units.h"
> > +#include "qemu/datadir.h"
> > #include "qapi/error.h"
> > #include "system/block-backend.h"
> > #include "system/system.h"
> > @@ -35,6 +36,7 @@ struct Ast2700FCState {
> >
> > MemoryRegion ca35_memory;
> > MemoryRegion ca35_dram;
> > + MemoryRegion ca35_boot_rom;
> > MemoryRegion ssp_memory;
> > MemoryRegion tsp_memory;
> >
> > @@ -55,12 +57,65 @@ struct Ast2700FCState {
> > #define AST2700FC_HW_STRAP2 0x00000003
> > #define AST2700FC_FMC_MODEL "w25q01jvq"
> > #define AST2700FC_SPI_MODEL "w25q512jv"
> > +#define VBOOTROM_FILE_NAME "ast27x0_bootrom.bin"
> > +
> > +static void ast2700fc_ca35_load_vbootrom(AspeedSoCState *soc,
> > + const char *bios_name,
> Error
> > +**errp) {
> > + g_autofree char *filename = NULL;
> > + int ret;
> > +
> > + filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
> > + if (!filename) {
> > + error_setg(errp, "Could not find vbootrom image '%s'",
> bios_name);
> > + return;
> > + }
> > +
> > + ret = load_image_mr(filename, &soc->vbootrom);
> > + if (ret < 0) {
> > + error_setg(errp, "Failed to load vbootrom image '%s'",
> bios_name);
> > + return;
> > + }
> > +}
> > +
> > +static void ast2700fc_ca35_write_boot_rom(DriveInfo *dinfo, hwaddr addr,
> > + size_t rom_size, Error
> > +**errp) {
> > + BlockBackend *blk = blk_by_legacy_dinfo(dinfo);
> > + g_autofree void *storage = NULL;
> > + int64_t size;
> > +
> > + /*
> > + * The block backend size should have already been 'validated' by
> > + * the creation of the m25p80 object.
> > + */
> > + size = blk_getlength(blk);
> > + if (size <= 0) {
> > + error_setg(errp, "failed to get flash size");
> > + return;
> > + }
> > +
> > + if (rom_size > size) {
> > + rom_size = size;
> > + }
> > +
> > + storage = g_malloc0(rom_size);
> > + if (blk_pread(blk, 0, rom_size, storage, 0) < 0) {
> > + error_setg(errp, "failed to read the initial flash content");
> > + return;
> > + }
> > +
> > + rom_add_blob_fixed("aspeed.boot_rom", storage, rom_size, addr); }
>
> The above is duplicated code. Could we try to have common routines instead ?
Thanks for your review and suggestion.
Per our earlier discussion, we plan to refactor hw/arm/aspeed.c. As a first
step, I can move the vbootrom helpers into a common source file so they can be
reused by other boards.
Do you have a preference for the filename?
hw/arm/aspeed_utils.c (with a small header in include/hw/arm/aspeed_utils.h),
Once that’s in place, aspeed_ast27x0f.c can reuse these helpers to support
vbootrom with coprocessors.
Thanks-Jamin
#define VBOOTROM_FILE_NAME "ast27x0_bootrom.bin"
static void aspeed_load_vbootrom(AspeedMachineState *bmc, const char *bios_name,
Error **errp)
static void aspeed_install_boot_rom(AspeedMachineState *bmc, BlockBackend *blk,
uint64_t rom_size)
static void write_boot_rom(BlockBackend *blk, hwaddr addr, size_t rom_size,
Error **errp)
>
> > static void ast2700fc_ca35_init(MachineState *machine)
> > {
> > Ast2700FCState *s = AST2700A1FC(machine);
> > + const char *bios_name = NULL;
> > AspeedSoCState *soc;
> > AspeedSoCClass *sc;
> > + uint64_t rom_size;
> > + DriveInfo *mtd0;
> >
> > object_initialize_child(OBJECT(s), "ca35", &s->ca35, "ast2700-a1");
> > soc = ASPEED_SOC(&s->ca35);
> > @@ -118,6 +173,26 @@ static void ast2700fc_ca35_init(MachineState
> *machine)
> > ast2700fc_board_info.ram_size = machine->ram_size;
> > ast2700fc_board_info.loader_start =
> > sc->memmap[ASPEED_DEV_SDRAM];
> >
> > + /* Install first FMC flash content as a boot rom. */
>
> This is a first addition for the ast2700fc machine and ...
>
> > + if (!s->mmio_exec) {
> > + mtd0 = drive_get(IF_MTD, 0, 0);
> > +
> > + if (mtd0) {
> > + rom_size = memory_region_size(&soc->spi_boot);
> > + memory_region_init_rom(&s->ca35_boot_rom, NULL,
> "aspeed.boot_rom",
> > + rom_size, &error_abort);
> > +
> memory_region_add_subregion_overlap(&soc->spi_boot_container, 0,
> > +
> &s->ca35_boot_rom, 1);
> > + ast2700fc_ca35_write_boot_rom(mtd0,
> > +
> sc->memmap[ASPEED_DEV_SPI_BOOT],
> > + rom_size,
> &error_abort);
> > + }
> > + }
> > +
> > + /* VBOOTROM */
>
> ... this is a second. Could you please split the changes ?
>
Will do
>
> Thanks,
>
> C.
>
>
>
>
> > + bios_name = machine->firmware ?: VBOOTROM_FILE_NAME;
> > + ast2700fc_ca35_load_vbootrom(soc, bios_name, &error_abort);
> > +
> > arm_load_kernel(ARM_CPU(first_cpu), machine,
> &ast2700fc_board_info);
> > }
> >
