On 9/25/18 2:26 PM, Peter Maydell wrote: > 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 :-)
Arg. It seemed convenient at the time. OK OK, I will split :) >> 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 ? I tried to reuse the accumulation pattern : Error *err = NULL, *local_err = NULL; foo(arg, &err); bar(arg, &local_err); error_propagate(&err, local_err); if (err) { handle the error... } but I am doing it wrong. Another reason to split the patch. Thanks, C. >> 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 >
