Re: [PATCH v3 5/7] aspeed: Create flash devices only when defaults are enabled

[Joel Stanley]

On Thu, 31 Aug 2023 at 13:22, Cédric Le Goater <clg@kaod.org> wrote:
>
> On 8/31/23 15:00, Joel Stanley wrote:
> > On Thu, 31 Aug 2023 at 12:39, Cédric Le Goater <clg@kaod.org> wrote:
> >>
> >> When the -nodefaults option is set, flash devices should be created
> >> with :
> >>
> >>      -blockdev node-name=fmc0,driver=file,filename=./flash.img \
> >>      -device mx66u51235f,cs=0x0,bus=ssi.0,drive=fmc0 \
> >>
> >> To be noted that in this case, the ROM will not be installed and the
> >> initial boot sequence (U-Boot loading) will fetch instructions using
> >> SPI transactions which is significantly slower. That's exactly how HW
> >> operates though.
> >>
> >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >
> > I think this is the first foray for the aspeed machines into
> > nodefaults removing things that previously would have just worked.
>
> This is true. It is change from the previous behavior.
>
> QEMU should probably complain if no fmc0 are found to boot from.
> Would that be ok ? And yes, documentation needs some update.

I didn't consider warning. That would help users who blindly added
-nodefaults and wondered why nothing was happening.

This is what happens if you add -nodefaults to an "old" command line
with your patch applied:

$ ./build/qemu-system-arm -M rainier-bmc -nographic -nodefaults
-serial stdio -drive
file=obmc-phosphor-image-rainier.static.mtd,if=mtd,format=raw
qemu-system-arm: -drive
file=obmc-phosphor-image-rainier.static.mtd,if=mtd,format=raw: machine
type does not support if=mtd,bus=0,unit=0

Which at least isn't sitting there spinning, as I was worried. I'll
leave it to you as to whether it needs a helpful message.

Cheers,

Joel



>
> Thanks,
>
> C.
>
> > I
> > know we haven't had it in our recommended command lines for a long
> > time, so that's fine.
> >
> > Reviewed-by: Joel Stanley <joel@jms.id.au>
> >
> > Should the content of your commit message go in the docs?
> >
> >> ---
> >>   hw/arm/aspeed.c | 6 ++++--
> >>   1 file changed, 4 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> >> index cd92cf9ce0bb..271512ce5ced 100644
> >> --- a/hw/arm/aspeed.c
> >> +++ b/hw/arm/aspeed.c
> >> @@ -396,12 +396,14 @@ static void aspeed_machine_init(MachineState 
> >> *machine)
> >>       connect_serial_hds_to_uarts(bmc);
> >>       qdev_realize(DEVICE(&bmc->soc), NULL, &error_abort);
> >>
> >> -    aspeed_board_init_flashes(&bmc->soc.fmc,
> >> +    if (defaults_enabled()) {
> >> +        aspeed_board_init_flashes(&bmc->soc.fmc,
> >>                                 bmc->fmc_model ? bmc->fmc_model : 
> >> amc->fmc_model,
> >>                                 amc->num_cs, 0);
> >> -    aspeed_board_init_flashes(&bmc->soc.spi[0],
> >> +        aspeed_board_init_flashes(&bmc->soc.spi[0],
> >>                                 bmc->spi_model ? bmc->spi_model : 
> >> amc->spi_model,
> >>                                 1, amc->num_cs);
> >> +    }
> >>
> >>       if (machine->kernel_filename && sc->num_cpus > 1) {
> >>           /* With no u-boot we must set up a boot stub for the secondary 
> >> CPU */
> >> --
> >> 2.41.0
> >>
>