| Message ID | 20180921161939.822-8-clg@kaod.org |
|---|---|
| State | New |
| Headers | show |
| Series | aspeed: misc fixes and enhancements (SMC) | expand |
On 21 September 2018 at 17:19, Cédric Le Goater <clg@kaod.org> 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 <clg@kaod.org> > --- > 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
On 9/25/18 2:26 PM, Peter Maydell wrote: > On 21 September 2018 at 17:19, Cédric Le Goater <clg@kaod.org> 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 <clg@kaod.org> >> --- >> 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 >
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); 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(), };
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. Signed-off-by: Cédric Le Goater <clg@kaod.org> --- 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(-)