On 21 September 2018 at 17:19, Cédric Le Goater <[email protected]> wrote: > The setting of the DRAM address of the DMA transaction depends on the > DRAM base address and the maximum DRAM size of the SoC. Let's add a > couple of properties to give this information to the SMC controller > model. > > Also, move the SDRAM Memory controller realization before the other > controllers which need it.
There's that "Also" again :-) > > Signed-off-by: Cédric Le Goater <[email protected]> > --- > include/hw/ssi/aspeed_smc.h | 4 ++++ > hw/arm/aspeed_soc.c | 28 +++++++++++++++++++--------- > hw/ssi/aspeed_smc.c | 2 ++ > 3 files changed, 25 insertions(+), 9 deletions(-) > > diff --git a/include/hw/ssi/aspeed_smc.h b/include/hw/ssi/aspeed_smc.h > index 1f557313fa93..d7090bb5e9b7 100644 > --- a/include/hw/ssi/aspeed_smc.h > +++ b/include/hw/ssi/aspeed_smc.h > @@ -97,6 +97,10 @@ typedef struct AspeedSMCState { > uint8_t r_timings; > uint8_t conf_enable_w0; > > + /* for DMA support */ > + uint64_t sdram_base; > + uint64_t max_ram_size; > + > AspeedSMCFlash *flashes; > } AspeedSMCState; > > diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c > index 2cbacb4430bb..bbc05d172fe1 100644 > --- a/hw/arm/aspeed_soc.c > +++ b/hw/arm/aspeed_soc.c > @@ -209,6 +209,14 @@ static void aspeed_soc_realize(DeviceState *dev, Error > **errp) > } > sysbus_mmio_map(SYS_BUS_DEVICE(&s->scu), 0, ASPEED_SOC_SCU_BASE); > > + /* SDMC - SDRAM Memory Controller */ > + object_property_set_bool(OBJECT(&s->sdmc), true, "realized", &err); > + if (err) { > + error_propagate(errp, err); > + return; > + } > + sysbus_mmio_map(SYS_BUS_DEVICE(&s->sdmc), 0, ASPEED_SOC_SDMC_BASE); > + > /* VIC */ > object_property_set_bool(OBJECT(&s->vic), true, "realized", &err); > if (err) { > @@ -252,7 +260,17 @@ static void aspeed_soc_realize(DeviceState *dev, Error > **errp) > qdev_get_gpio_in(DEVICE(&s->vic), 12)); > > /* FMC, The number of CS is set at the board level */ > - object_property_set_bool(OBJECT(&s->fmc), true, "realized", &err); > + object_property_set_int(OBJECT(&s->fmc), sc->info->sdram_base, > "sdram-base", > + &err); > + object_property_set_int(OBJECT(&s->fmc), s->sdmc.max_ram_size, > + "max-ram-size", &local_err); > + error_propagate(&err, local_err); > + if (err) { > + error_propagate(errp, err); > + return; > + } > + object_property_set_bool(OBJECT(&s->fmc), true, "realized", &local_err); > + error_propagate(&err, local_err); I don't understand why we have several different variations on "do this thing, and if it fails then propagate the error to caller" here. I was expecting all these things to be in the form some_thing(..., &err); if (err) { error_propagate(errp, err); return; } Is there a reason you need to involve the 'local_err' variable in some cases ? > if (err) { > error_propagate(errp, err); > return; > @@ -278,14 +296,6 @@ static void aspeed_soc_realize(DeviceState *dev, Error > **errp) > s->spi[i].ctrl->flash_window_base); > } > > - /* SDMC - SDRAM Memory Controller */ > - object_property_set_bool(OBJECT(&s->sdmc), true, "realized", &err); > - if (err) { > - error_propagate(errp, err); > - return; > - } > - sysbus_mmio_map(SYS_BUS_DEVICE(&s->sdmc), 0, ASPEED_SOC_SDMC_BASE); > - > /* Watch dog */ > for (i = 0; i < sc->info->wdts_num; i++) { > object_property_set_bool(OBJECT(&s->wdt[i]), true, "realized", &err); > diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c > index 6045ca11b969..500de6d16d09 100644 > --- a/hw/ssi/aspeed_smc.c > +++ b/hw/ssi/aspeed_smc.c > @@ -800,6 +800,8 @@ static const VMStateDescription vmstate_aspeed_smc = { > > static Property aspeed_smc_properties[] = { > DEFINE_PROP_UINT32("num-cs", AspeedSMCState, num_cs, 1), > + DEFINE_PROP_UINT64("sdram-base", AspeedSMCState, sdram_base, 0), > + DEFINE_PROP_UINT64("max-ram-size", AspeedSMCState, max_ram_size, 0), > DEFINE_PROP_END_OF_LIST(), > }; thanks -- PMM
