| Message ID | 20230831123922.105200-6-clg@kaod.org |
|---|---|
| State | New |
| Headers | show |
| Series | aspeed: Add blockdev support for flash device definition | expand |
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. 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 >
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. 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 >>
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 > >> >
On 8/31/23 15:42, Joel Stanley wrote: > 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 yes that's a post board init sanity check on unused drives. > 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. It seems difficult since we could be booting the machine from a kernel also. I will update the documentation. Thanks, C.
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 */
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> --- hw/arm/aspeed.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)