| Message ID | 20230508075859.3326566-7-clg@kaod.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | aspeed: fixes and extensions | expand |
On 8/5/23 09:58, Cédric Le Goater wrote: > Currently, a set of default flash devices is created at machine init > and drives defined on the QEMU command line are associated to the FMC > and SPI controllers in sequence : > > -drive file<file>,format=raw,if=mtd > -drive file<file1>,format=raw,if=mtd > > The CS lines are wired in the same creation loop. This makes a strong > assumption on the ordering and is not very flexible since only a > limited set of flash devices can be defined : 1 FMC + 1 or 2 SPI, > which is less than what the SoC really supports. > > A better alternative would be to define the flash devices on the > command line using a blockdev attached to a CS line of a SSI bus : > > -blockdev node-name=fmc0,driver=file,filename=./flash.img > -device mx66u51235f,addr=0x0,bus=ssi.0,drive=fmc0 > > However, user created flash devices are not correctly wired to their > SPI controller and consequently can not be used by the machine. Fix > that and wire the CS lines of all available devices when the SSI bus > is reset. > > Signed-off-by: Cédric Le Goater <clg@kaod.org> > --- > hw/arm/aspeed.c | 5 +---- > hw/ssi/aspeed_smc.c | 8 ++++++++ > 2 files changed, 9 insertions(+), 4 deletions(-) > > diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c > index b654513f35..9a15899cbc 100644 > --- a/hw/arm/aspeed.c > +++ b/hw/arm/aspeed.c > @@ -299,17 +299,14 @@ void aspeed_board_init_flashes(AspeedSMCState *s, const char *flashtype, > > for (i = 0; i < count; ++i) { > DriveInfo *dinfo = drive_get(IF_MTD, 0, unit0 + i); > - qemu_irq cs_line; > DeviceState *dev; > > dev = qdev_new(flashtype); > if (dinfo) { > qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(dinfo)); > } > + qdev_prop_set_uint32(dev, "addr", i); > qdev_realize_and_unref(dev, BUS(s->spi), &error_fatal); > - > - cs_line = qdev_get_gpio_in_named(dev, SSI_GPIO_CS, 0); > - qdev_connect_gpio_out_named(DEVICE(s), "cs", i, cs_line); > } > } > > diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c > index 7281169322..2a4001b774 100644 > --- a/hw/ssi/aspeed_smc.c > +++ b/hw/ssi/aspeed_smc.c > @@ -692,6 +692,14 @@ static void aspeed_smc_reset(DeviceState *d) > memset(s->regs, 0, sizeof s->regs); > } > > + for (i = 0; i < asc->cs_num_max; i++) { > + DeviceState *dev = ssi_get_cs(s->spi, i); > + if (dev) { > + qemu_irq cs_line = qdev_get_gpio_in_named(dev, SSI_GPIO_CS, 0); > + qdev_connect_gpio_out_named(DEVICE(s), "cs", i, cs_line); > + } > + } Why did you chose the DeviceReset handler? Shouldn't this be done in aspeed_smc_realize(), the DeviceRealize handler? > /* Unselect all peripherals */ > for (i = 0; i < asc->cs_num_max; ++i) { > s->regs[s->r_ctrl0 + i] |= CTRL_CE_STOP_ACTIVE;
On 5/30/23 22:56, Philippe Mathieu-Daudé wrote: > On 8/5/23 09:58, Cédric Le Goater wrote: >> Currently, a set of default flash devices is created at machine init >> and drives defined on the QEMU command line are associated to the FMC >> and SPI controllers in sequence : >> >> -drive file<file>,format=raw,if=mtd >> -drive file<file1>,format=raw,if=mtd >> >> The CS lines are wired in the same creation loop. This makes a strong >> assumption on the ordering and is not very flexible since only a >> limited set of flash devices can be defined : 1 FMC + 1 or 2 SPI, >> which is less than what the SoC really supports. >> >> A better alternative would be to define the flash devices on the >> command line using a blockdev attached to a CS line of a SSI bus : >> >> -blockdev node-name=fmc0,driver=file,filename=./flash.img >> -device mx66u51235f,addr=0x0,bus=ssi.0,drive=fmc0 >> >> However, user created flash devices are not correctly wired to their >> SPI controller and consequently can not be used by the machine. Fix >> that and wire the CS lines of all available devices when the SSI bus >> is reset. >> >> Signed-off-by: Cédric Le Goater <clg@kaod.org> >> --- >> hw/arm/aspeed.c | 5 +---- >> hw/ssi/aspeed_smc.c | 8 ++++++++ >> 2 files changed, 9 insertions(+), 4 deletions(-) >> >> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c >> index b654513f35..9a15899cbc 100644 >> --- a/hw/arm/aspeed.c >> +++ b/hw/arm/aspeed.c >> @@ -299,17 +299,14 @@ void aspeed_board_init_flashes(AspeedSMCState *s, const char *flashtype, >> for (i = 0; i < count; ++i) { >> DriveInfo *dinfo = drive_get(IF_MTD, 0, unit0 + i); >> - qemu_irq cs_line; >> DeviceState *dev; >> dev = qdev_new(flashtype); >> if (dinfo) { >> qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(dinfo)); >> } >> + qdev_prop_set_uint32(dev, "addr", i); >> qdev_realize_and_unref(dev, BUS(s->spi), &error_fatal); >> - >> - cs_line = qdev_get_gpio_in_named(dev, SSI_GPIO_CS, 0); >> - qdev_connect_gpio_out_named(DEVICE(s), "cs", i, cs_line); >> } >> } >> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c >> index 7281169322..2a4001b774 100644 >> --- a/hw/ssi/aspeed_smc.c >> +++ b/hw/ssi/aspeed_smc.c >> @@ -692,6 +692,14 @@ static void aspeed_smc_reset(DeviceState *d) >> memset(s->regs, 0, sizeof s->regs); >> } >> + for (i = 0; i < asc->cs_num_max; i++) { >> + DeviceState *dev = ssi_get_cs(s->spi, i); >> + if (dev) { >> + qemu_irq cs_line = qdev_get_gpio_in_named(dev, SSI_GPIO_CS, 0); >> + qdev_connect_gpio_out_named(DEVICE(s), "cs", i, cs_line); >> + } >> + } > > Why did you chose the DeviceReset handler? > Shouldn't this be done in aspeed_smc_realize(), the DeviceRealize handler? This looks weird, I agree, but the wiring needs to be done at reset time to wire the user-created devices. I don't think there is another way. Thanks, C. > >> /* Unselect all peripherals */ >> for (i = 0; i < asc->cs_num_max; ++i) { >> s->regs[s->r_ctrl0 + i] |= CTRL_CE_STOP_ACTIVE; >
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c index b654513f35..9a15899cbc 100644 --- a/hw/arm/aspeed.c +++ b/hw/arm/aspeed.c @@ -299,17 +299,14 @@ void aspeed_board_init_flashes(AspeedSMCState *s, const char *flashtype, for (i = 0; i < count; ++i) { DriveInfo *dinfo = drive_get(IF_MTD, 0, unit0 + i); - qemu_irq cs_line; DeviceState *dev; dev = qdev_new(flashtype); if (dinfo) { qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(dinfo)); } + qdev_prop_set_uint32(dev, "addr", i); qdev_realize_and_unref(dev, BUS(s->spi), &error_fatal); - - cs_line = qdev_get_gpio_in_named(dev, SSI_GPIO_CS, 0); - qdev_connect_gpio_out_named(DEVICE(s), "cs", i, cs_line); } } diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c index 7281169322..2a4001b774 100644 --- a/hw/ssi/aspeed_smc.c +++ b/hw/ssi/aspeed_smc.c @@ -692,6 +692,14 @@ static void aspeed_smc_reset(DeviceState *d) memset(s->regs, 0, sizeof s->regs); } + for (i = 0; i < asc->cs_num_max; i++) { + DeviceState *dev = ssi_get_cs(s->spi, i); + if (dev) { + qemu_irq cs_line = qdev_get_gpio_in_named(dev, SSI_GPIO_CS, 0); + qdev_connect_gpio_out_named(DEVICE(s), "cs", i, cs_line); + } + } + /* Unselect all peripherals */ for (i = 0; i < asc->cs_num_max; ++i) { s->regs[s->r_ctrl0 + i] |= CTRL_CE_STOP_ACTIVE;
Currently, a set of default flash devices is created at machine init and drives defined on the QEMU command line are associated to the FMC and SPI controllers in sequence : -drive file<file>,format=raw,if=mtd -drive file<file1>,format=raw,if=mtd The CS lines are wired in the same creation loop. This makes a strong assumption on the ordering and is not very flexible since only a limited set of flash devices can be defined : 1 FMC + 1 or 2 SPI, which is less than what the SoC really supports. A better alternative would be to define the flash devices on the command line using a blockdev attached to a CS line of a SSI bus : -blockdev node-name=fmc0,driver=file,filename=./flash.img -device mx66u51235f,addr=0x0,bus=ssi.0,drive=fmc0 However, user created flash devices are not correctly wired to their SPI controller and consequently can not be used by the machine. Fix that and wire the CS lines of all available devices when the SSI bus is reset. Signed-off-by: Cédric Le Goater <clg@kaod.org> --- hw/arm/aspeed.c | 5 +---- hw/ssi/aspeed_smc.c | 8 ++++++++ 2 files changed, 9 insertions(+), 4 deletions(-)