1. Patchew
  2. QEMU
  3. [Qemu-devel] [PATCH v2 00/11] aspeed: misc fixes and enhancements (SMC)
  4. View patch

[Qemu-devel] [PATCH v2 07/11] aspeed/smc: add a 'sdram_base' and 'max-ram-size' properties

Cédric Le Goater posted 11 patches 7 years, 1 month ago
[Qemu-devel] [PATCH v2 07/11] aspeed/smc: add a 'sdram_base' and 'max-ram-size' properties
Posted by Cédric Le Goater 7 years, 1 month ago 
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(-)

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(),
 };
 
-- 
2.17.1
Re: [Qemu-devel] [PATCH v2 07/11] aspeed/smc: add a 'sdram_base' and 'max-ram-size' properties
Posted by Peter Maydell 7 years, 1 month ago 
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
Re: [Qemu-devel] [PATCH v2 07/11] aspeed/smc: add a 'sdram_base' and 'max-ram-size' properties
Posted by Cédric Le Goater 7 years, 1 month ago 
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
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.