From patchwork Tue Sep 6 15:52:15 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?C=C3=A9dric_Le_Goater?= X-Patchwork-Id: 9317423 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id B3139607D3 for ; Tue, 6 Sep 2016 15:55:55 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id A18B028E0F for ; Tue, 6 Sep 2016 15:55:55 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 965AF28E10; Tue, 6 Sep 2016 15:55:55 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id E5CBE28E12 for ; Tue, 6 Sep 2016 15:55:53 +0000 (UTC) Received: from localhost ([::1]:34563 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bhIj1-0002V5-Q1 for patchwork-qemu-devel@patchwork.kernel.org; Tue, 06 Sep 2016 11:55:51 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47988) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bhIff-0008Ql-Cp for qemu-devel@nongnu.org; Tue, 06 Sep 2016 11:52:24 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bhIfb-0002Nj-98 for qemu-devel@nongnu.org; Tue, 06 Sep 2016 11:52:22 -0400 Received: from 7.mo179.mail-out.ovh.net ([46.105.61.94]:48692) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bhIfa-0002NV-Vj for qemu-devel@nongnu.org; Tue, 06 Sep 2016 11:52:19 -0400 Received: from player792.ha.ovh.net (b7.ovh.net [213.186.33.57]) by mo179.mail-out.ovh.net (Postfix) with ESMTP id 138B6FFB3D4 for ; Tue, 6 Sep 2016 17:52:18 +0200 (CEST) Received: from hermes.kaod.org (deibp9eh1--blueice2n7.emea.ibm.com [195.212.29.169]) (Authenticated sender: clg@kaod.org) by player792.ha.ovh.net (Postfix) with ESMTPSA id AABC2A0083; Tue, 6 Sep 2016 17:52:15 +0200 (CEST) To: Peter Maydell , QEMU Developers , Andrew Jeffery References: <1473167287-8326-1-git-send-email-peter.maydell@linaro.org> <1473167287-8326-2-git-send-email-peter.maydell@linaro.org> From: =?UTF-8?Q?C=c3=a9dric_Le_Goater?= Message-ID: <424cf1dd-3286-190f-7640-5c260c8827a3@kaod.org> 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 MIME-Version: 1.0 In-Reply-To: X-Ovh-Tracer-Id: 16587320378366004139 X-VR-SPAMSTATE: OK X-VR-SPAMSCORE: -100 X-VR-SPAMCAUSE: gggruggvucftvghtrhhoucdtuddrfeeluddrheelgdelieculddtuddrfeeltddrtddtmdcutefuodetggdotefrodftvfcurfhrohhfihhlvgemucfqggfjnecuuegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmd X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x (no timestamps) [generic] X-Received-From: 46.105.61.94 Subject: Re: [Qemu-devel] [PULL 01/14] ast2400: add a memory controller device model X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: "Qemu-devel" X-Virus-Scanned: ClamAV using ClamSMTP On 09/06/2016 04:59 PM, Peter Maydell wrote: > On 6 September 2016 at 14:07, Peter Maydell wrote: >> From: Cédric Le Goater >> >> 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 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 --- 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);