Re: [PATCH v2 07/18] aspeed: Fix hardcode attach flash model of spi controllers

["Cédric Le Goater" <clg@kaod.org>]

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);
>>>
>