| Message ID | 20241022094110.1574011-8-jamin_lin@aspeedtech.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Fix write incorrect data into flash in user mode | expand |
On 10/22/24 11:40, Jamin Lin wrote: > It only attached flash model of fmc and spi[0] in aspeed_machine_init function. > However, AST2500 and AST2600 have one fmc and two spi(spi1 and spi2) > controllers; AST2700 have one fmc and 3 spi(spi0, spi1 and spi2) controllers. > > Besides, it used hardcode to attach flash model of fmc, spi[0] and spi[1] in > aspeed_minibmc_machine_init for AST1030. > > To make both functions more flexible and support all ASPEED SOCs spi > controllers, adds a for loop with sc->spis_num to attach flash model of > all supported spi controllers. The sc->spis_num is from AspeedSoCClass. > > Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com> Reviewed-by: Cédric Le Goater <clg@redhat.com> Thanks, C. > --- > hw/arm/aspeed.c | 21 ++++++++++++--------- > 1 file changed, 12 insertions(+), 9 deletions(-) > > diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c > index b4b1ce9efb..7ac01a3562 100644 > --- a/hw/arm/aspeed.c > +++ b/hw/arm/aspeed.c > @@ -419,9 +419,11 @@ static void aspeed_machine_init(MachineState *machine) > 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], > - bmc->spi_model ? bmc->spi_model : amc->spi_model, > - 1, amc->num_cs); > + for (i = 0; i < sc->spis_num; i++) { > + aspeed_board_init_flashes(&bmc->soc->spi[i], > + bmc->spi_model ? bmc->spi_model : amc->spi_model, > + amc->num_cs, amc->num_cs + (amc->num_cs * i)); > + } > } > > if (machine->kernel_filename && sc->num_cpus > 1) { > @@ -1579,7 +1581,9 @@ static void aspeed_minibmc_machine_init(MachineState *machine) > { > AspeedMachineState *bmc = ASPEED_MACHINE(machine); > AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(machine); > + AspeedSoCClass *sc; > Clock *sysclk; > + int i; > > sysclk = clock_new(OBJECT(machine), "SYSCLK"); > clock_set_hz(sysclk, SYSCLK_FRQ); > @@ -1587,6 +1591,7 @@ static void aspeed_minibmc_machine_init(MachineState *machine) > bmc->soc = ASPEED_SOC(object_new(amc->soc_name)); > object_property_add_child(OBJECT(machine), "soc", OBJECT(bmc->soc)); > object_unref(OBJECT(bmc->soc)); > + sc = ASPEED_SOC_GET_CLASS(bmc->soc); > qdev_connect_clock_in(DEVICE(bmc->soc), "sysclk", sysclk); > > object_property_set_link(OBJECT(bmc->soc), "memory", > @@ -1599,13 +1604,11 @@ static void aspeed_minibmc_machine_init(MachineState *machine) > amc->num_cs, > 0); > > - aspeed_board_init_flashes(&bmc->soc->spi[0], > - bmc->spi_model ? bmc->spi_model : amc->spi_model, > - amc->num_cs, amc->num_cs); > - > - aspeed_board_init_flashes(&bmc->soc->spi[1], > + for (i = 0; i < sc->spis_num; i++) { > + aspeed_board_init_flashes(&bmc->soc->spi[i], > bmc->spi_model ? bmc->spi_model : amc->spi_model, > - amc->num_cs, (amc->num_cs * 2)); > + amc->num_cs, amc->num_cs + (amc->num_cs * i)); > + } > > if (amc->i2c_init) { > amc->i2c_init(bmc);
oops. R-b sent on the wrong patch. On 10/22/24 12:48, Cédric Le Goater wrote: > On 10/22/24 11:40, Jamin Lin wrote: >> It only attached flash model of fmc and spi[0] in aspeed_machine_init function. >> However, AST2500 and AST2600 have one fmc and two spi(spi1 and spi2) >> controllers; AST2700 have one fmc and 3 spi(spi0, spi1 and spi2) controllers. >> >> Besides, it used hardcode to attach flash model of fmc, spi[0] and spi[1] in >> aspeed_minibmc_machine_init for AST1030. >> >> To make both functions more flexible and support all ASPEED SOCs spi >> controllers, adds a for loop with sc->spis_num to attach flash model of >> all supported spi controllers. The sc->spis_num is from AspeedSoCClass. To be honest, I am not a big fan of the aspeed_board_init_flashes() routine. See commit 27a2c66c92ec for the reason. I prefer the more flexible approach : $ qemu-system-arm -M ast2600-evb \ -blockdev node-name=fmc0,driver=file,filename=/path/to/fmc0.img \ -device mx66u51235f,bus=ssi.0,cs=0x0,drive=fmc0 \ -blockdev node-name=fmc1,driver=file,filename=/path/to/fmc1.img \ -device mx66u51235f,bus=ssi.0,cs=0x1,drive=fmc1 \ -blockdev node-name=spi1,driver=file,filename=/path/to/spi1.img \ -device mx66u51235f,cs=0x0,bus=ssi.1,drive=spi1 \ -nographic -nodefaults which doesn't use the drive_get() interface and so, doesn't make assumption on the order of the drives defined on the QEMU command line. Also, the number of availabe flash devices is a machine definition, not a SoC definition. Not all CS are wired. I will drop that patch for now. Thanks, C. >> --- >> hw/arm/aspeed.c | 21 ++++++++++++--------- >> 1 file changed, 12 insertions(+), 9 deletions(-) >> >> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c >> index b4b1ce9efb..7ac01a3562 100644 >> --- a/hw/arm/aspeed.c >> +++ b/hw/arm/aspeed.c >> @@ -419,9 +419,11 @@ static void aspeed_machine_init(MachineState *machine) >> 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], >> - bmc->spi_model ? bmc->spi_model : amc->spi_model, >> - 1, amc->num_cs); >> + for (i = 0; i < sc->spis_num; i++) { >> + aspeed_board_init_flashes(&bmc->soc->spi[i], >> + bmc->spi_model ? bmc->spi_model : amc->spi_model, >> + amc->num_cs, amc->num_cs + (amc->num_cs * i)); >> + } >> } >> if (machine->kernel_filename && sc->num_cpus > 1) { >> @@ -1579,7 +1581,9 @@ static void aspeed_minibmc_machine_init(MachineState *machine) >> { >> AspeedMachineState *bmc = ASPEED_MACHINE(machine); >> AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(machine); >> + AspeedSoCClass *sc; >> Clock *sysclk; >> + int i; >> sysclk = clock_new(OBJECT(machine), "SYSCLK"); >> clock_set_hz(sysclk, SYSCLK_FRQ); >> @@ -1587,6 +1591,7 @@ static void aspeed_minibmc_machine_init(MachineState *machine) >> bmc->soc = ASPEED_SOC(object_new(amc->soc_name)); >> object_property_add_child(OBJECT(machine), "soc", OBJECT(bmc->soc)); >> object_unref(OBJECT(bmc->soc)); >> + sc = ASPEED_SOC_GET_CLASS(bmc->soc); >> qdev_connect_clock_in(DEVICE(bmc->soc), "sysclk", sysclk); >> object_property_set_link(OBJECT(bmc->soc), "memory", >> @@ -1599,13 +1604,11 @@ static void aspeed_minibmc_machine_init(MachineState *machine) >> amc->num_cs, >> 0); >> - aspeed_board_init_flashes(&bmc->soc->spi[0], >> - bmc->spi_model ? bmc->spi_model : amc->spi_model, >> - amc->num_cs, amc->num_cs); >> - >> - aspeed_board_init_flashes(&bmc->soc->spi[1], >> + for (i = 0; i < sc->spis_num; i++) { >> + aspeed_board_init_flashes(&bmc->soc->spi[i], >> bmc->spi_model ? bmc->spi_model : amc->spi_model, >> - amc->num_cs, (amc->num_cs * 2)); >> + amc->num_cs, amc->num_cs + (amc->num_cs * i)); >> + } >> if (amc->i2c_init) { >> amc->i2c_init(bmc); >
Hi Cedric, > Subject: Re: [PATCH v2 07/18] aspeed: Fix hardcode attach flash model of spi > controllers > > oops. R-b sent on the wrong patch. > > On 10/22/24 12:48, Cédric Le Goater wrote: > > On 10/22/24 11:40, Jamin Lin wrote: > >> It only attached flash model of fmc and spi[0] in aspeed_machine_init > function. > >> However, AST2500 and AST2600 have one fmc and two spi(spi1 and spi2) > >> controllers; AST2700 have one fmc and 3 spi(spi0, spi1 and spi2) controllers. > >> > >> Besides, it used hardcode to attach flash model of fmc, spi[0] and > >> spi[1] in aspeed_minibmc_machine_init for AST1030. > >> > >> To make both functions more flexible and support all ASPEED SOCs spi > >> controllers, adds a for loop with sc->spis_num to attach flash model > >> of all supported spi controllers. The sc->spis_num is from AspeedSoCClass. > > To be honest, I am not a big fan of the aspeed_board_init_flashes() routine. > See commit 27a2c66c92ec for the reason. > > I prefer the more flexible approach : > > $ qemu-system-arm -M ast2600-evb \ > -blockdev node-name=fmc0,driver=file,filename=/path/to/fmc0.img \ > -device mx66u51235f,bus=ssi.0,cs=0x0,drive=fmc0 \ > -blockdev node-name=fmc1,driver=file,filename=/path/to/fmc1.img \ > -device mx66u51235f,bus=ssi.0,cs=0x1,drive=fmc1 \ > -blockdev node-name=spi1,driver=file,filename=/path/to/spi1.img \ > -device mx66u51235f,cs=0x0,bus=ssi.1,drive=spi1 \ > -nographic -nodefaults > Thanks for notify me this solution. I can successfully attach the default image to supported SPI controllers with different flash model. It seems we need to add "defaults_enabled()" if-statement in aspeed_minibmc_machine_init to support this solution for AST1030. Otherwise, I will get this error. qemu-system-arm: -device w25q80bl,bus=ssi.0,cs=0x0,drive=fmc0: CS index '0x0' in use by a w25q80bl device https://github.com/qemu/qemu/blob/master/hw/arm/aspeed.c 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], bmc->spi_model ? bmc->spi_model : amc->spi_model, amc->num_cs, amc->num_cs); aspeed_board_init_flashes(&bmc->soc->spi[1], bmc->spi_model ? bmc->spi_model : amc->spi_model, amc->num_cs, (amc->num_cs * 2)); } Do I need to send this patch in v3 patch series? Or individually send this patch in the new patch series? AST1030: -blockdev node-name=fmc0,driver=file,filename=./fmc_cs0_img \ -device w25q80bl,bus=ssi.0,cs=0x0,drive=fmc0 \ -blockdev node-name=fmc1,driver=file,filename=./fmc_cs1_img \ -device w25q80bl,bus=ssi.0,cs=0x1,drive=fmc1 \ -blockdev node-name=spi1c0,driver=file,filename=./spi1_cs0_img \ -device w25q256,bus=ssi.1,cs=0x0,drive=spi1c0 \ -blockdev node-name=spi1c1,driver=file,filename=./spi1_cs1_img \ -device w25q256,bus=ssi.1,cs=0x1,drive=spi1c1 \ -blockdev node-name=spi2c0,driver=file,filename=./spi2_cs0_img \ -device w25q256,bus=ssi.2,cs=0x0,drive=spi2c0 \ -blockdev node-name=spi2c1,driver=file,filename=./spi2_cs1_img \ -device w25q256,bus=ssi.2,cs=0x1,drive=spi2c1 \ -nodefaults AST2600: -blockdev node-name=fmc0,driver=file,filename=$1 \ -device mx66u51235f,cs=0x0,bus=ssi.0,drive=fmc0 \ -blockdev node-name=fmc1,driver=file,filename=./fmc_cs1_img \ -device mx66u51235f,cs=0x1,bus=ssi.0,drive=fmc1 \ -blockdev node-name=spi1,driver=file,filename=./spi1_cs0_img \ -device mx66u51235f,cs=0x0,bus=ssi.1,drive=spi1 \ -blockdev node-name=spi2,driver=file,filename=./spi2_cs0_img \ -device mx66u51235f,cs=0x0,bus=ssi.2,drive=spi2 \ -nodefaults > which doesn't use the drive_get() interface and so, doesn't make assumption > on the order of the drives defined on the QEMU command line. > > Also, the number of availabe flash devices is a machine definition, not a SoC > definition. Not all CS are wired. > > I will drop that patch for now. > Understand and thanks for suggestion. Jamin > > Thanks, > > C. > > > > >> --- > >> hw/arm/aspeed.c | 21 ++++++++++++--------- > >> 1 file changed, 12 insertions(+), 9 deletions(-) > >> > >> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c index > >> b4b1ce9efb..7ac01a3562 100644 > >> --- a/hw/arm/aspeed.c > >> +++ b/hw/arm/aspeed.c > >> @@ -419,9 +419,11 @@ static void aspeed_machine_init(MachineState > >> *machine) > >> 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], > >> - bmc->spi_model ? > bmc->spi_model : > >> amc->spi_model, > >> - 1, amc->num_cs); > >> + for (i = 0; i < sc->spis_num; i++) { > >> + aspeed_board_init_flashes(&bmc->soc->spi[i], > >> + bmc->spi_model ? > bmc->spi_model : > >> +amc->spi_model, > >> + amc->num_cs, amc->num_cs + > (amc->num_cs > >> +* i)); > >> + } > >> } > >> if (machine->kernel_filename && sc->num_cpus > 1) { @@ > -1579,7 > >> +1581,9 @@ static void aspeed_minibmc_machine_init(MachineState > >> *machine) > >> { > >> AspeedMachineState *bmc = ASPEED_MACHINE(machine); > >> AspeedMachineClass *amc = > ASPEED_MACHINE_GET_CLASS(machine); > >> + AspeedSoCClass *sc; > >> Clock *sysclk; > >> + int i; > >> sysclk = clock_new(OBJECT(machine), "SYSCLK"); > >> clock_set_hz(sysclk, SYSCLK_FRQ); @@ -1587,6 +1591,7 @@ > static > >> void aspeed_minibmc_machine_init(MachineState *machine) > >> bmc->soc = ASPEED_SOC(object_new(amc->soc_name)); > >> object_property_add_child(OBJECT(machine), "soc", > >> OBJECT(bmc->soc)); > >> object_unref(OBJECT(bmc->soc)); > >> + sc = ASPEED_SOC_GET_CLASS(bmc->soc); > >> qdev_connect_clock_in(DEVICE(bmc->soc), "sysclk", sysclk); > >> object_property_set_link(OBJECT(bmc->soc), "memory", @@ > >> -1599,13 +1604,11 @@ static void > >> aspeed_minibmc_machine_init(MachineState *machine) > >> amc->num_cs, > >> 0); > >> - aspeed_board_init_flashes(&bmc->soc->spi[0], > >> - bmc->spi_model ? > bmc->spi_model : > >> amc->spi_model, > >> - amc->num_cs, > amc->num_cs); > >> - > >> - aspeed_board_init_flashes(&bmc->soc->spi[1], > >> + for (i = 0; i < sc->spis_num; i++) { > >> + aspeed_board_init_flashes(&bmc->soc->spi[i], > >> bmc->spi_model ? > bmc->spi_model : > >> amc->spi_model, > >> - amc->num_cs, > (amc->num_cs * 2)); > >> + amc->num_cs, > amc->num_cs + > >> +(amc->num_cs * i)); > >> + } > >> if (amc->i2c_init) { > >> amc->i2c_init(bmc); > >
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c index b4b1ce9efb..7ac01a3562 100644 --- a/hw/arm/aspeed.c +++ b/hw/arm/aspeed.c @@ -419,9 +419,11 @@ static void aspeed_machine_init(MachineState *machine) 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], - bmc->spi_model ? bmc->spi_model : amc->spi_model, - 1, amc->num_cs); + for (i = 0; i < sc->spis_num; i++) { + aspeed_board_init_flashes(&bmc->soc->spi[i], + bmc->spi_model ? bmc->spi_model : amc->spi_model, + amc->num_cs, amc->num_cs + (amc->num_cs * i)); + } } if (machine->kernel_filename && sc->num_cpus > 1) { @@ -1579,7 +1581,9 @@ static void aspeed_minibmc_machine_init(MachineState *machine) { AspeedMachineState *bmc = ASPEED_MACHINE(machine); AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(machine); + AspeedSoCClass *sc; Clock *sysclk; + int i; sysclk = clock_new(OBJECT(machine), "SYSCLK"); clock_set_hz(sysclk, SYSCLK_FRQ); @@ -1587,6 +1591,7 @@ static void aspeed_minibmc_machine_init(MachineState *machine) bmc->soc = ASPEED_SOC(object_new(amc->soc_name)); object_property_add_child(OBJECT(machine), "soc", OBJECT(bmc->soc)); object_unref(OBJECT(bmc->soc)); + sc = ASPEED_SOC_GET_CLASS(bmc->soc); qdev_connect_clock_in(DEVICE(bmc->soc), "sysclk", sysclk); object_property_set_link(OBJECT(bmc->soc), "memory", @@ -1599,13 +1604,11 @@ static void aspeed_minibmc_machine_init(MachineState *machine) amc->num_cs, 0); - aspeed_board_init_flashes(&bmc->soc->spi[0], - bmc->spi_model ? bmc->spi_model : amc->spi_model, - amc->num_cs, amc->num_cs); - - aspeed_board_init_flashes(&bmc->soc->spi[1], + for (i = 0; i < sc->spis_num; i++) { + aspeed_board_init_flashes(&bmc->soc->spi[i], bmc->spi_model ? bmc->spi_model : amc->spi_model, - amc->num_cs, (amc->num_cs * 2)); + amc->num_cs, amc->num_cs + (amc->num_cs * i)); + } if (amc->i2c_init) { amc->i2c_init(bmc);
It only attached flash model of fmc and spi[0] in aspeed_machine_init function. However, AST2500 and AST2600 have one fmc and two spi(spi1 and spi2) controllers; AST2700 have one fmc and 3 spi(spi0, spi1 and spi2) controllers. Besides, it used hardcode to attach flash model of fmc, spi[0] and spi[1] in aspeed_minibmc_machine_init for AST1030. To make both functions more flexible and support all ASPEED SOCs spi controllers, adds a for loop with sc->spis_num to attach flash model of all supported spi controllers. The sc->spis_num is from AspeedSoCClass. Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com> --- hw/arm/aspeed.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-)