On 09/06/2016 06:07 PM, Peter Maydell wrote:
> On 6 September 2016 at 16:52, Cédric Le Goater <c...@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 <c...@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 <c...@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.
