[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PULL 01/14] ast2400: add a memory controller device mo
From: |
Cédric Le Goater |
Subject: |
Re: [Qemu-devel] [PULL 01/14] ast2400: add a memory controller device model |
Date: |
Tue, 6 Sep 2016 17:52:15 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 |
On 09/06/2016 04:59 PM, Peter Maydell wrote:
> On 6 September 2016 at 14:07, Peter Maydell <address@hidden> wrote:
>> From: Cédric Le Goater <address@hidden>
>>
>> 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 <address@hidden>
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 <address@hidden>
---
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);
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);
[Qemu-devel] [PULL 08/14] palmetto-bmc: replace palmetto_bmc with aspeed, Peter Maydell, 2016/09/06
[Qemu-devel] [PULL 04/14] ast2400: rename the Aspeed SoC files to aspeed_soc, Peter Maydell, 2016/09/06
[Qemu-devel] [PULL 09/14] palmetto-bmc: add board specific configuration, Peter Maydell, 2016/09/06
[Qemu-devel] [PULL 10/14] hw/misc: use macros to define hw-strap1 register on the AST2400 Aspeed SoC, Peter Maydell, 2016/09/06
[Qemu-devel] [PULL 06/14] aspeed-soc: provide a framework to add new SoCs, Peter Maydell, 2016/09/06
[Qemu-devel] [PULL 07/14] palmetto-bmc: rename the Aspeed board file to aspeed.c, Peter Maydell, 2016/09/06
[Qemu-devel] [PULL 14/14] block: m25p80: Fix vmstate structure name, Peter Maydell, 2016/09/06