diff mbox

[PULL,01/14] ast2400: add a memory controller device model

Message ID 424cf1dd-3286-190f-7640-5c260c8827a3@kaod.org (mailing list archive)
State New, archived
Headers show

Commit Message

Cédric Le Goater Sept. 6, 2016, 3:52 p.m. UTC 
On 09/06/2016 04:59 PM, Peter Maydell wrote:
> On 6 September 2016 at 14:07, Peter Maydell <peter.maydell@linaro.org> wrote:
>> From: Cédric Le Goater <clg@kaod.org>
>>
>> The uboot in the previous release of the SDK was using a hardcoded
>> value for memory size. This is not true anymore, the value is now
>> retrieved from the memory controller.
>>
>> Below is a model for this device, only supporting unlock and
>> configuration. Without it, we endup running a guest with 64MB, which
>> is a bit low nowdays. It uses a 'silicon-rev' property and ram_size to
>> build a default value. Some bits should be linked to SCU strapping
>> registers but it seems a bit complex to add for the current need.
>>
>> The model is ready for the AST2500 SOC.
> 
>> +static int ast2400_rambits(void)
>> +{
>> +    switch (ram_size >> 20) {
>> +    case 64:
>> +        return ASPEED_SDMC_DRAM_64MB;
>> +    case 128:
>> +        return ASPEED_SDMC_DRAM_128MB;
>> +    case 256:
>> +        return ASPEED_SDMC_DRAM_256MB;
>> +    case 512:
>> +        return ASPEED_SDMC_DRAM_512MB;
>> +    default:
>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid RAM size: 0x%"
>> +                      HWADDR_PRIx "\n", __func__, ram_size);
>> +        break;
> 
> This doesn't compile on 32-bit systems, because ram_size
> is not a hwaddr, it's a ram_addr_t, and it needs a different
> format specifier.
> 
> I'll fix this up locally and resend the pullreq.
> 
> However, I notice looking at this code more closely that it's
> using the global ram_size to determine the behaviour of the
> device. That seems a bit dubious to me, we don't have other
> devices that look at global config settings like that.
> Can you look at doing a followup patch where the board level
> code tells the memory controller how much RAM it has directly,
> please?
> 
> (Also it seems wrong to call this a GUEST_ERROR, because the
> thing setting the ram_size is the user running QEMU (possibly
> in conjunction with any restrictions imposed by the board
> model code), not the guest code.)

How's that ? See below. To apply on top of the ast2500 patchset.

Thanks,

C.


From: Cédric Le Goater <clg@kaod.org>
Subject: [PATCH] aspeed: add a ram_size property to the memory controller
Date: Tue, 06 Sep 2016 17:42:54 +0200
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Configure the size of the RAM of the SOC using a property to propagate 
the value down to the memory controller from the board level.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/arm/aspeed.c               |    2 ++
 hw/arm/aspeed_soc.c           |    2 ++
 hw/misc/aspeed_sdmc.c         |   22 ++++++++++++----------
 include/hw/misc/aspeed_sdmc.h |    1 +
 4 files changed, 17 insertions(+), 10 deletions(-)

Comments

Peter Maydell Sept. 6, 2016, 4:07 p.m. UTC | #1 
On 6 September 2016 at 16:52, Cédric Le Goater <clg@kaod.org> wrote:
> How's that ? See below. To apply on top of the ast2500 patchset.

Looks basically OK, but:

> From: Cédric Le Goater <clg@kaod.org>
> Subject: [PATCH] aspeed: add a ram_size property to the memory controller
> Date: Tue, 06 Sep 2016 17:42:54 +0200
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> Configure the size of the RAM of the SOC using a property to propagate
> the value down to the memory controller from the board level.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  hw/arm/aspeed.c               |    2 ++
>  hw/arm/aspeed_soc.c           |    2 ++
>  hw/misc/aspeed_sdmc.c         |   22 ++++++++++++----------
>  include/hw/misc/aspeed_sdmc.h |    1 +
>  4 files changed, 17 insertions(+), 10 deletions(-)
>
> Index: qemu-aspeed.git/hw/misc/aspeed_sdmc.c
> ===================================================================
> --- qemu-aspeed.git.orig/hw/misc/aspeed_sdmc.c
> +++ qemu-aspeed.git/hw/misc/aspeed_sdmc.c
> @@ -9,6 +9,7 @@
>
>  #include "qemu/osdep.h"
>  #include "qemu/log.h"
> +#include "qemu/error-report.h"
>  #include "hw/misc/aspeed_sdmc.h"
>  #include "hw/misc/aspeed_scu.h"
>  #include "hw/qdev-properties.h"
> @@ -139,9 +140,9 @@ static const MemoryRegionOps aspeed_sdmc
>      .valid.max_access_size = 4,
>  };
>
> -static int ast2400_rambits(void)
> +static int ast2400_rambits(AspeedSDMCState *s)
>  {
> -    switch (ram_size >> 20) {
> +    switch (s->ram_size >> 20) {
>      case 64:
>          return ASPEED_SDMC_DRAM_64MB;
>      case 128:
> @@ -151,8 +152,8 @@ static int ast2400_rambits(void)
>      case 512:
>          return ASPEED_SDMC_DRAM_512MB;
>      default:
> -        qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid RAM size: 0x%"
> -                      HWADDR_PRIx "\n", __func__, ram_size);
> +        error_report("warning: Invalid RAM size %dM. Using default 64M",
> +                     s->ram_size >> 20);

You should do this sanitizing when the property is set
on the device (ie at realize), not every time the device
is reset.

Or you could make it actually a realize error if you want to
force the user to use a valid ram size, though that's a bit
harsher than we usually are.

>          break;
>      }
>
> @@ -160,9 +161,9 @@ static int ast2400_rambits(void)
>      return ASPEED_SDMC_DRAM_64MB;
>  }
>
> -static int ast2500_rambits(void)
> +static int ast2500_rambits(AspeedSDMCState *s)
>  {
> -    switch (ram_size >> 20) {
> +    switch (s->ram_size >> 20) {
>      case 128:
>          return ASPEED_SDMC_AST2500_128MB;
>      case 256:
> @@ -172,8 +173,8 @@ static int ast2500_rambits(void)
>      case 1024:
>          return ASPEED_SDMC_AST2500_1024MB;
>      default:
> -        qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid RAM size: 0x%"
> -                      HWADDR_PRIx "\n", __func__, ram_size);
> +        error_report("warning: Invalid RAM size %dM. Using default 128M",
> +                     s->ram_size >> 20);
>          break;
>      }
>
> @@ -192,7 +193,7 @@ static void aspeed_sdmc_reset(DeviceStat
>      case AST2400_A0_SILICON_REV:
>          s->regs[R_CONF] |=
>              ASPEED_SDMC_VGA_COMPAT |
> -            ASPEED_SDMC_DRAM_SIZE(ast2400_rambits());
> +            ASPEED_SDMC_DRAM_SIZE(ast2400_rambits(s));
>          break;
>
>      case AST2500_A0_SILICON_REV:
> @@ -200,7 +201,7 @@ static void aspeed_sdmc_reset(DeviceStat
>          s->regs[R_CONF] |=
>              ASPEED_SDMC_HW_VERSION(1) |
>              ASPEED_SDMC_VGA_APERTURE(ASPEED_SDMC_VGA_64MB) |
> -            ASPEED_SDMC_DRAM_SIZE(ast2500_rambits());
> +            ASPEED_SDMC_DRAM_SIZE(ast2500_rambits(s));
>          break;
>
>      default:
> @@ -236,6 +237,7 @@ static const VMStateDescription vmstate_
>
>  static Property aspeed_sdmc_properties[] = {
>      DEFINE_PROP_UINT32("silicon-rev", AspeedSDMCState, silicon_rev, 0),
> +    DEFINE_PROP_UINT32("ram-size", AspeedSDMCState, ram_size, 0),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>
> Index: qemu-aspeed.git/include/hw/misc/aspeed_sdmc.h
> ===================================================================
> --- qemu-aspeed.git.orig/include/hw/misc/aspeed_sdmc.h
> +++ qemu-aspeed.git/include/hw/misc/aspeed_sdmc.h
> @@ -25,6 +25,7 @@ typedef struct AspeedSDMCState {
>
>      uint32_t regs[ASPEED_SDMC_NR_REGS];
>      uint32_t silicon_rev;
> +    uint32_t ram_size;
>
>  } AspeedSDMCState;
>
> Index: qemu-aspeed.git/hw/arm/aspeed.c
> ===================================================================
> --- qemu-aspeed.git.orig/hw/arm/aspeed.c
> +++ qemu-aspeed.git/hw/arm/aspeed.c
> @@ -121,6 +121,8 @@ static void aspeed_board_init(MachineSta
>                                     &error_abort);
>      object_property_set_int(OBJECT(&bmc->soc), cfg->hw_strap1, "hw-strap1",
>                              &error_abort);
> +    object_property_set_int(OBJECT(&bmc->soc), ram_size, "ram-size",
> +                            &error_abort);

Having the property be 32 bit means that here you're silently
throwing away the top half of ram_size on a 64-bit host, and
there's nothing in the board model that's restricting the
user-provided ram size.

thanks
-- PMM
Cédric Le Goater Sept. 6, 2016, 4:21 p.m. UTC | #2 
On 09/06/2016 06:07 PM, Peter Maydell wrote:
> On 6 September 2016 at 16:52, Cédric Le Goater <clg@kaod.org> wrote:
>> How's that ? See below. To apply on top of the ast2500 patchset.
> 
> Looks basically OK, but:
> 
>> From: Cédric Le Goater <clg@kaod.org>
>> Subject: [PATCH] aspeed: add a ram_size property to the memory controller
>> Date: Tue, 06 Sep 2016 17:42:54 +0200
>> MIME-Version: 1.0
>> Content-Type: text/plain; charset=UTF-8
>> Content-Transfer-Encoding: 8bit
>>
>> Configure the size of the RAM of the SOC using a property to propagate
>> the value down to the memory controller from the board level.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  hw/arm/aspeed.c               |    2 ++
>>  hw/arm/aspeed_soc.c           |    2 ++
>>  hw/misc/aspeed_sdmc.c         |   22 ++++++++++++----------
>>  include/hw/misc/aspeed_sdmc.h |    1 +
>>  4 files changed, 17 insertions(+), 10 deletions(-)
>>
>> Index: qemu-aspeed.git/hw/misc/aspeed_sdmc.c
>> ===================================================================
>> --- qemu-aspeed.git.orig/hw/misc/aspeed_sdmc.c
>> +++ qemu-aspeed.git/hw/misc/aspeed_sdmc.c
>> @@ -9,6 +9,7 @@
>>
>>  #include "qemu/osdep.h"
>>  #include "qemu/log.h"
>> +#include "qemu/error-report.h"
>>  #include "hw/misc/aspeed_sdmc.h"
>>  #include "hw/misc/aspeed_scu.h"
>>  #include "hw/qdev-properties.h"
>> @@ -139,9 +140,9 @@ static const MemoryRegionOps aspeed_sdmc
>>      .valid.max_access_size = 4,
>>  };
>>
>> -static int ast2400_rambits(void)
>> +static int ast2400_rambits(AspeedSDMCState *s)
>>  {
>> -    switch (ram_size >> 20) {
>> +    switch (s->ram_size >> 20) {
>>      case 64:
>>          return ASPEED_SDMC_DRAM_64MB;
>>      case 128:
>> @@ -151,8 +152,8 @@ static int ast2400_rambits(void)
>>      case 512:
>>          return ASPEED_SDMC_DRAM_512MB;
>>      default:
>> -        qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid RAM size: 0x%"
>> -                      HWADDR_PRIx "\n", __func__, ram_size);
>> +        error_report("warning: Invalid RAM size %dM. Using default 64M",
>> +                     s->ram_size >> 20);
> 
> You should do this sanitizing when the property is set
> on the device (ie at realize), not every time the device
> is reset.

yes.

> Or you could make it actually a realize error if you want to
> force the user to use a valid ram size, though that's a bit
> harsher than we usually are.

Well, I guess the warning is enough. But, maybe using the lowest 
value is a bit severe. Linux really has a hard time on 64M.

>>          break;
>>      }
>>
>> @@ -160,9 +161,9 @@ static int ast2400_rambits(void)
>>      return ASPEED_SDMC_DRAM_64MB;
>>  }
>>
>> -static int ast2500_rambits(void)
>> +static int ast2500_rambits(AspeedSDMCState *s)
>>  {
>> -    switch (ram_size >> 20) {
>> +    switch (s->ram_size >> 20) {
>>      case 128:
>>          return ASPEED_SDMC_AST2500_128MB;
>>      case 256:
>> @@ -172,8 +173,8 @@ static int ast2500_rambits(void)
>>      case 1024:
>>          return ASPEED_SDMC_AST2500_1024MB;
>>      default:
>> -        qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid RAM size: 0x%"
>> -                      HWADDR_PRIx "\n", __func__, ram_size);
>> +        error_report("warning: Invalid RAM size %dM. Using default 128M",
>> +                     s->ram_size >> 20);
>>          break;
>>      }
>>
>> @@ -192,7 +193,7 @@ static void aspeed_sdmc_reset(DeviceStat
>>      case AST2400_A0_SILICON_REV:
>>          s->regs[R_CONF] |=
>>              ASPEED_SDMC_VGA_COMPAT |
>> -            ASPEED_SDMC_DRAM_SIZE(ast2400_rambits());
>> +            ASPEED_SDMC_DRAM_SIZE(ast2400_rambits(s));
>>          break;
>>
>>      case AST2500_A0_SILICON_REV:
>> @@ -200,7 +201,7 @@ static void aspeed_sdmc_reset(DeviceStat
>>          s->regs[R_CONF] |=
>>              ASPEED_SDMC_HW_VERSION(1) |
>>              ASPEED_SDMC_VGA_APERTURE(ASPEED_SDMC_VGA_64MB) |
>> -            ASPEED_SDMC_DRAM_SIZE(ast2500_rambits());
>> +            ASPEED_SDMC_DRAM_SIZE(ast2500_rambits(s));
>>          break;
>>
>>      default:
>> @@ -236,6 +237,7 @@ static const VMStateDescription vmstate_
>>
>>  static Property aspeed_sdmc_properties[] = {
>>      DEFINE_PROP_UINT32("silicon-rev", AspeedSDMCState, silicon_rev, 0),
>> +    DEFINE_PROP_UINT32("ram-size", AspeedSDMCState, ram_size, 0),
>>      DEFINE_PROP_END_OF_LIST(),
>>  };
>>
>> Index: qemu-aspeed.git/include/hw/misc/aspeed_sdmc.h
>> ===================================================================
>> --- qemu-aspeed.git.orig/include/hw/misc/aspeed_sdmc.h
>> +++ qemu-aspeed.git/include/hw/misc/aspeed_sdmc.h
>> @@ -25,6 +25,7 @@ typedef struct AspeedSDMCState {
>>
>>      uint32_t regs[ASPEED_SDMC_NR_REGS];
>>      uint32_t silicon_rev;
>> +    uint32_t ram_size;
>>
>>  } AspeedSDMCState;
>>
>> Index: qemu-aspeed.git/hw/arm/aspeed.c
>> ===================================================================
>> --- qemu-aspeed.git.orig/hw/arm/aspeed.c
>> +++ qemu-aspeed.git/hw/arm/aspeed.c
>> @@ -121,6 +121,8 @@ static void aspeed_board_init(MachineSta
>>                                     &error_abort);
>>      object_property_set_int(OBJECT(&bmc->soc), cfg->hw_strap1, "hw-strap1",
>>                              &error_abort);
>> +    object_property_set_int(OBJECT(&bmc->soc), ram_size, "ram-size",
>> +                            &error_abort);
> 
> Having the property be 32 bit means that here you're silently
> throwing away the top half of ram_size on a 64-bit host, and
> there's nothing in the board model that's restricting the
> user-provided ram size.

true. I can fix that.

I will wait for the next pullreq for the v2.

Thanks for the quick review,

C.
diff mbox

Patch

Index: qemu-aspeed.git/hw/misc/aspeed_sdmc.c
===================================================================
--- qemu-aspeed.git.orig/hw/misc/aspeed_sdmc.c
+++ qemu-aspeed.git/hw/misc/aspeed_sdmc.c
@@ -9,6 +9,7 @@ 
 
 #include "qemu/osdep.h"
 #include "qemu/log.h"
+#include "qemu/error-report.h"
 #include "hw/misc/aspeed_sdmc.h"
 #include "hw/misc/aspeed_scu.h"
 #include "hw/qdev-properties.h"
@@ -139,9 +140,9 @@  static const MemoryRegionOps aspeed_sdmc
     .valid.max_access_size = 4,
 };
 
-static int ast2400_rambits(void)
+static int ast2400_rambits(AspeedSDMCState *s)
 {
-    switch (ram_size >> 20) {
+    switch (s->ram_size >> 20) {
     case 64:
         return ASPEED_SDMC_DRAM_64MB;
     case 128:
@@ -151,8 +152,8 @@  static int ast2400_rambits(void)
     case 512:
         return ASPEED_SDMC_DRAM_512MB;
     default:
-        qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid RAM size: 0x%"
-                      HWADDR_PRIx "\n", __func__, ram_size);
+        error_report("warning: Invalid RAM size %dM. Using default 64M",
+                     s->ram_size >> 20);
         break;
     }
 
@@ -160,9 +161,9 @@  static int ast2400_rambits(void)
     return ASPEED_SDMC_DRAM_64MB;
 }
 
-static int ast2500_rambits(void)
+static int ast2500_rambits(AspeedSDMCState *s)
 {
-    switch (ram_size >> 20) {
+    switch (s->ram_size >> 20) {
     case 128:
         return ASPEED_SDMC_AST2500_128MB;
     case 256:
@@ -172,8 +173,8 @@  static int ast2500_rambits(void)
     case 1024:
         return ASPEED_SDMC_AST2500_1024MB;
     default:
-        qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid RAM size: 0x%"
-                      HWADDR_PRIx "\n", __func__, ram_size);
+        error_report("warning: Invalid RAM size %dM. Using default 128M",
+                     s->ram_size >> 20);
         break;
     }
 
@@ -192,7 +193,7 @@  static void aspeed_sdmc_reset(DeviceStat
     case AST2400_A0_SILICON_REV:
         s->regs[R_CONF] |=
             ASPEED_SDMC_VGA_COMPAT |
-            ASPEED_SDMC_DRAM_SIZE(ast2400_rambits());
+            ASPEED_SDMC_DRAM_SIZE(ast2400_rambits(s));
         break;
 
     case AST2500_A0_SILICON_REV:
@@ -200,7 +201,7 @@  static void aspeed_sdmc_reset(DeviceStat
         s->regs[R_CONF] |=
             ASPEED_SDMC_HW_VERSION(1) |
             ASPEED_SDMC_VGA_APERTURE(ASPEED_SDMC_VGA_64MB) |
-            ASPEED_SDMC_DRAM_SIZE(ast2500_rambits());
+            ASPEED_SDMC_DRAM_SIZE(ast2500_rambits(s));
         break;
 
     default:
@@ -236,6 +237,7 @@  static const VMStateDescription vmstate_
 
 static Property aspeed_sdmc_properties[] = {
     DEFINE_PROP_UINT32("silicon-rev", AspeedSDMCState, silicon_rev, 0),
+    DEFINE_PROP_UINT32("ram-size", AspeedSDMCState, ram_size, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
 
Index: qemu-aspeed.git/include/hw/misc/aspeed_sdmc.h
===================================================================
--- qemu-aspeed.git.orig/include/hw/misc/aspeed_sdmc.h
+++ qemu-aspeed.git/include/hw/misc/aspeed_sdmc.h
@@ -25,6 +25,7 @@  typedef struct AspeedSDMCState {
 
     uint32_t regs[ASPEED_SDMC_NR_REGS];
     uint32_t silicon_rev;
+    uint32_t ram_size;
 
 } AspeedSDMCState;
 
Index: qemu-aspeed.git/hw/arm/aspeed.c
===================================================================
--- qemu-aspeed.git.orig/hw/arm/aspeed.c
+++ qemu-aspeed.git/hw/arm/aspeed.c
@@ -121,6 +121,8 @@  static void aspeed_board_init(MachineSta
                                    &error_abort);
     object_property_set_int(OBJECT(&bmc->soc), cfg->hw_strap1, "hw-strap1",
                             &error_abort);
+    object_property_set_int(OBJECT(&bmc->soc), ram_size, "ram-size",
+                            &error_abort);
     object_property_set_bool(OBJECT(&bmc->soc), true, "realized",
                              &error_abort);
 
Index: qemu-aspeed.git/hw/arm/aspeed_soc.c
===================================================================
--- qemu-aspeed.git.orig/hw/arm/aspeed_soc.c
+++ qemu-aspeed.git/hw/arm/aspeed_soc.c
@@ -117,6 +117,8 @@  static void aspeed_soc_init(Object *obj)
     qdev_set_parent_bus(DEVICE(&s->sdmc), sysbus_get_default());
     qdev_prop_set_uint32(DEVICE(&s->sdmc), "silicon-rev",
                          sc->info->silicon_rev);
+    object_property_add_alias(obj, "ram-size", OBJECT(&s->sdmc),
+                              "ram-size", &error_abort);
 
     object_initialize(&s->ftgmac100, sizeof(s->ftgmac100), TYPE_FTGMAC100);
     object_property_add_child(obj, "ftgmac100", OBJECT(&s->ftgmac100), NULL);