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(-)
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);
--
2.34.1On 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);
> >
Hello Jamin,
Sorry for the later answer. I think I missed the question below.
On 10/23/24 04:46, Jamin Lin wrote:
> 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?
defaults_enabled() should be used to avoid creating the devices indeed.
Please send a patch for it. Or did you already ?
Thanks,
C.
>
> 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);
>>>
>
Hi Cedric,
> Subject: Re: [PATCH v2 07/18] aspeed: Fix hardcode attach flash model of spi
> controllers
>
> Hello Jamin,
>
> Sorry for the later answer. I think I missed the question below.
>
> On 10/23/24 04:46, Jamin Lin wrote:
> > 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?
>
>
> defaults_enabled() should be used to avoid creating the devices indeed.
> Please send a patch for it. Or did you already ?
>
This patch had been applied,
https://github.com/qemu/qemu/commit/22b3c557ffd4fb9c6c034a839b3e5b82d131123a
Thanks-Jamin
> Thanks,
>
> C.
>
>
>
> >
> > 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);
> >>>
> >
© 2016 - 2026 Red Hat, Inc.