[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
RE: [PATCH v2 07/18] aspeed: Fix hardcode attach flash model of spi cont
|
From: |
Jamin Lin |
|
Subject: |
RE: [PATCH v2 07/18] aspeed: Fix hardcode attach flash model of spi controllers |
|
Date: |
Wed, 27 Nov 2024 01:13:26 +0000 |
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);
> >>>
> >