[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 07/11] aspeed/smc: add a 'sdram_base' and 'ma
From: |
Cédric Le Goater |
Subject: |
Re: [Qemu-devel] [PATCH v2 07/11] aspeed/smc: add a 'sdram_base' and 'max-ram-size' properties |
Date: |
Tue, 25 Sep 2018 16:06:22 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.0 |
On 9/25/18 2:26 PM, Peter Maydell wrote:
> On 21 September 2018 at 17:19, Cédric Le Goater <address@hidden> 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 <address@hidden>
>> ---
>> 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
>
- [Qemu-devel] [PATCH v2 05/11] aspeed/smc: fix some alignment issues, (continued)
- [Qemu-devel] [PATCH v2 05/11] aspeed/smc: fix some alignment issues, Cédric Le Goater, 2018/09/21
- [Qemu-devel] [PATCH v2 06/11] aspeed/smc: fix default read value, Cédric Le Goater, 2018/09/21
- [Qemu-devel] [PATCH v2 11/11] aspeed/smc: Add dummy data register, Cédric Le Goater, 2018/09/21
- [Qemu-devel] [PATCH v2 10/11] aspeed/smc: inject errors in DMA checksum, Cédric Le Goater, 2018/09/21
- [Qemu-devel] [PATCH v2 08/11] aspeed/smc: add support for DMAs, Cédric Le Goater, 2018/09/21
- [Qemu-devel] [PATCH v2 09/11] aspeed/smc: add DMA calibration settings, Cédric Le Goater, 2018/09/21
- [Qemu-devel] [PATCH v2 07/11] aspeed/smc: add a 'sdram_base' and 'max-ram-size' properties, Cédric Le Goater, 2018/09/21
- Re: [Qemu-devel] [PATCH v2 00/11] aspeed: misc fixes and enhancements (SMC), Peter Maydell, 2018/09/25