On 07/06/2016 06:34 AM, Andrew Jeffery wrote:
> On Mon, 2016-07-04 at 14:18 +0200, Cédric Le Goater wrote:
>> 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 for Linux nowdays.
>>
>> Signed-off-by: Cédric Le Goater <c...@kaod.org>
>> ---
>> hw/arm/ast2400.c | 13 +++++
>> hw/misc/Makefile.objs | 2 +-
>> hw/misc/aspeed_sdmc.c | 126
>> ++++++++++++++++++++++++++++++++++++++++++
>> include/hw/arm/ast2400.h | 1 +
>> include/hw/misc/aspeed_sdmc.h | 49 ++++++++++++++++
>> 5 files changed, 190 insertions(+), 1 deletion(-)
>> create mode 100644 hw/misc/aspeed_sdmc.c
>> create mode 100644 include/hw/misc/aspeed_sdmc.h
>>
>> diff --git a/hw/arm/ast2400.c b/hw/arm/ast2400.c
>> index 9c30b45f87a9..fb0156ba8bc1 100644
>> --- a/hw/arm/ast2400.c
>> +++ b/hw/arm/ast2400.c
>> @@ -27,6 +27,7 @@
>> #define AST2400_FMC_BASE 0X1E620000
>> #define AST2400_SPI_BASE 0X1E630000
>> #define AST2400_VIC_BASE 0x1E6C0000
>> +#define AST2400_SDMC_BASE 0x1E6E0000
>> #define AST2400_SCU_BASE 0x1E6E2000
>> #define AST2400_TIMER_BASE 0x1E782000
>> #define AST2400_I2C_BASE 0x1E78A000
>> @@ -99,6 +100,10 @@ static void ast2400_init(Object *obj)
>> object_initialize(&s->spi, sizeof(s->spi), "aspeed.smc.spi");
>> object_property_add_child(obj, "spi", OBJECT(&s->spi), NULL);
>> qdev_set_parent_bus(DEVICE(&s->spi), sysbus_get_default());
>> +
>> + object_initialize(&s->sdmc, sizeof(s->sdmc), TYPE_ASPEED_SDMC);
>> + object_property_add_child(obj, "sdmc", OBJECT(&s->sdmc), NULL);
>> + qdev_set_parent_bus(DEVICE(&s->sdmc), sysbus_get_default());
>> }
>>
>> static void ast2400_realize(DeviceState *dev, Error **errp)
>> @@ -184,6 +189,14 @@ static void ast2400_realize(DeviceState *dev, Error
>> **errp)
>> }
>> sysbus_mmio_map(SYS_BUS_DEVICE(&s->spi), 0, AST2400_SPI_BASE);
>> sysbus_mmio_map(SYS_BUS_DEVICE(&s->spi), 1, AST2400_SPI_FLASH_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, AST2400_SDMC_BASE);
>> }
>>
>> static void ast2400_class_init(ObjectClass *oc, void *data)
>> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
>> index 54020aa06c00..d488b748c789 100644
>> --- a/hw/misc/Makefile.objs
>> +++ b/hw/misc/Makefile.objs
>> @@ -52,4 +52,4 @@ obj-$(CONFIG_PVPANIC) += pvpanic.o
>> obj-$(CONFIG_EDU) += edu.o
>> obj-$(CONFIG_HYPERV_TESTDEV) += hyperv_testdev.o
>> obj-$(CONFIG_AUX) += aux.o
>> -obj-$(CONFIG_ASPEED_SOC) += aspeed_scu.o
>> +obj-$(CONFIG_ASPEED_SOC) += aspeed_scu.o aspeed_sdmc.o
>> diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
>> new file mode 100644
>> index 000000000000..52b6393b3f0b
>> --- /dev/null
>> +++ b/hw/misc/aspeed_sdmc.c
>> @@ -0,0 +1,126 @@
>> +/*
>> + * ASPEED SDRAM Memory Controller
>> + *
>> + * Copyright 2016 IBM Corp.
>> + *
>> + * This code is licensed under the GPL version 2 or later. See
>> + * the COPYING file in the top-level directory.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "hw/misc/aspeed_sdmc.h"
>> +#include "hw/qdev-properties.h"
>> +#include "qapi/error.h"
>> +#include "trace.h"
>
> We need to #include "qemu/log.h" here to avoid build failures when
> alternative tracing backends are enabled. I have forgotten it a couple
> of times recently.
yes. Thanks.
>> +
>> +/* Protection Key Register */
>> +#define R_PROT (0x00 / 4)
>> +#define PROT_KEY_UNLOCK 0xFC600309
>> +
>> +/* Configuration Register */
>> +#define R_CONF (0x04 / 4)
>> +
>> +static uint64_t aspeed_sdmc_read(void *opaque, hwaddr addr, unsigned size)
>> +{
>> + AspeedSDMCState *s = ASPEED_SDMC(opaque);
>> +
>> + addr >>= 2;
>> +
>> + if (addr >= ARRAY_SIZE(s->regs)) {
>> + qemu_log_mask(LOG_GUEST_ERROR,
>> + "%s: Out-of-bounds read at offset 0x%" HWADDR_PRIx
>> "\n",
>> + __func__, addr);
>> + return 0;
>> + }
>> +
>> + return s->regs[addr];
>> +}
>> +
>> +static void aspeed_sdmc_write(void *opaque, hwaddr addr, uint64_t data,
>> + unsigned int size)
>> +{
>> + AspeedSDMCState *s = ASPEED_SDMC(opaque);
>> +
>> + addr >>= 2;
>> +
>> + if (addr >= ARRAY_SIZE(s->regs)) {
>> + qemu_log_mask(LOG_GUEST_ERROR,
>> + "%s: Out-of-bounds write at offset 0x%" HWADDR_PRIx
>> "\n",
>> + __func__, addr);
>> + return;
>> + }
>> +
>> + if (addr != R_PROT && s->regs[R_PROT] != PROT_KEY_UNLOCK) {
>> + qemu_log_mask(LOG_GUEST_ERROR, "%s: SDMC is locked!\n", __func__);
>> + return;
>> + }
>> +
>> + s->regs[addr] = data;
>> +}
>> +
>> +static const MemoryRegionOps aspeed_sdmc_ops = {
>> + .read = aspeed_sdmc_read,
>> + .write = aspeed_sdmc_write,
>> + .endianness = DEVICE_LITTLE_ENDIAN,
>> + .valid.min_access_size = 4,
>> + .valid.max_access_size = 4,
>> + .valid.unaligned = false,
>> +};
>> +
>> +static void aspeed_sdmc_reset(DeviceState *dev)
>> +{
>> + AspeedSDMCState *s = ASPEED_SDMC(dev);
>> +
>> + memset(s->regs, 0, sizeof(s->regs));
>> + s->regs[R_CONF] = s->config;
>> +}
>> +
>> +static void aspeed_sdmc_realize(DeviceState *dev, Error **errp)
>> +{
>> + SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>> + AspeedSDMCState *s = ASPEED_SDMC(dev);
>> +
>> + memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_sdmc_ops, s,
>> + TYPE_ASPEED_SDMC, 0x1000);
>> + sysbus_init_mmio(sbd, &s->iomem);
>> +}
>> +
>> +static const VMStateDescription vmstate_aspeed_sdmc = {
>> + .name = "aspeed.sdmc",
>> + .version_id = 1,
>> + .minimum_version_id = 1,
>> + .fields = (VMStateField[]) {
>> + VMSTATE_UINT32_ARRAY(regs, AspeedSDMCState, ASPEED_SDMC_NR_REGS),
>> + VMSTATE_END_OF_LIST()
>> + }
>> +};
>> +
>> +static Property aspeed_sdmc_properties[] = {
>> + DEFINE_PROP_UINT32("configuration", AspeedSDMCState, config,
>> + ASPEED_SDMC_256MB),
>
> The datasheet's default value for configuration is 0x40, which
> configures a read-only, backwards-compatible 16-bit VGA mode bit.
> Should we also set this bit? Do we care?
I think we should add the definitions of all the bits of the
registers we use. I will add the 16-bit VGA mode bit in the
next version.
>> + DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>> +static void aspeed_sdmc_class_init(ObjectClass *klass, void *data)
>> +{
>> + DeviceClass *dc = DEVICE_CLASS(klass);
>> + dc->realize = aspeed_sdmc_realize;
>> + dc->reset = aspeed_sdmc_reset;
>> + dc->desc = "ASPEED SDRAM Memory Controller";
>> + dc->vmsd = &vmstate_aspeed_sdmc;
>> + dc->props = aspeed_sdmc_properties;
>> +}
>> +
>> +static const TypeInfo aspeed_sdmc_info = {
>> + .name = TYPE_ASPEED_SDMC,
>> + .parent = TYPE_SYS_BUS_DEVICE,
>> + .instance_size = sizeof(AspeedSDMCState),
>> + .class_init = aspeed_sdmc_class_init,
>> +};
>> +
>> +static void aspeed_sdmc_register_types(void)
>> +{
>> + type_register_static(&aspeed_sdmc_info);
>> +}
>> +
>> +type_init(aspeed_sdmc_register_types);
>> diff --git a/include/hw/arm/ast2400.h b/include/hw/arm/ast2400.h
>> index 7833bc716cd8..d549cceb121c 100644
>> --- a/include/hw/arm/ast2400.h
>> +++ b/include/hw/arm/ast2400.h
>> @@ -32,6 +32,7 @@ typedef struct AST2400State {
>> AspeedSCUState scu;
>> AspeedSMCState smc;
>> AspeedSMCState spi;
>> + AspeedSDMCState sdmc;
>
> You need to #include "hw/misc/aspeed_sdmc.h" for the AspeedSDMCState
> type.
yes ... that was a bad refresh of the patch before sending.
>> } AST2400State;
>>
>> #define TYPE_AST2400 "ast2400"
>> diff --git a/include/hw/misc/aspeed_sdmc.h b/include/hw/misc/aspeed_sdmc.h
>> new file mode 100644
>> index 000000000000..65fefebdb2cd
>> --- /dev/null
>> +++ b/include/hw/misc/aspeed_sdmc.h
>> @@ -0,0 +1,49 @@
>> +/*
>> + * ASPEED SDRAM Memory Controller
>> + *
>> + * Copyright 2016 IBM Corp.
>> + *
>> + * This code is licensed under the GPL version 2 or later. See
>> + * the COPYING file in the top-level directory.
>> + */
>> +#ifndef ASPEED_SDMC_H
>> +#define ASPEED_SDMC_H
>> +
>> +#include "hw/sysbus.h"
>> +
>> +#define TYPE_ASPEED_SDMC "aspeed.sdmc"
>> +#define ASPEED_SDMC(obj) OBJECT_CHECK(AspeedSDMCState, (obj),
>> TYPE_ASPEED_SDMC)
>> +
>> +#define ASPEED_SDMC_NR_REGS (0x8 >> 2)
>> +
>> +typedef struct AspeedSDMCState {
>> + /*< private >*/
>> + SysBusDevice parent_obj;
>> +
>> + /*< public >*/
>> + MemoryRegion iomem;
>> +
>> + uint32_t regs[ASPEED_SDMC_NR_REGS];
>> + uint32_t config;
>> +
>> +} AspeedSDMCState;
>> +
>> +/*
>> + * For Aspeed AST2400 SOC
>> + */
>> +#define ASPEED_SDMC_64MB 0x0
>> +#define ASPEED_SDMC_128MB 0x1
>> +#define ASPEED_SDMC_256MB 0x2
>> +#define ASPEED_SDMC_512MB 0x3
>> +
>> +/*
>> + * For Aspeed AST2500 SOC and higher revision number
>> + */
>
> It might be worth noting somewhere in a comment that the AST2500
> datasheet explicitly states the DRAM controller for the AST2500 is not
> backwards compatible with the AST2400, but given the current
> implementation it doesn't matter.
yes. I will add that.
> If we start caring about the values
> in any of the registers MCR10-MCR24, MCR34, MCR38, MCR54, MCR5C-MCR6C,
> MCR80-MCR8C, then we need to be careful, i.e. test DRAM Controller
> Hardware Version field in R_CONF (MCR04) to dispatch. That check needs
> care as well, as the version field is 4 bits but only 2 values are
> defined in the AST2500 datasheet.
>
>> +#define ASPEED_SDMC_NEW_VERSION (1 << 28)
>> +
>> +#define ASPEED_SDMC_NEW_128MB (0x0 | ASPEED_SDMC_NEW_VERSION)
>> +#define ASPEED_SDMC_NEW_256MB (0x1 | ASPEED_SDMC_NEW_VERSION)
>> +#define ASPEED_SDMC_NEW_512MB (0x2 | ASPEED_SDMC_NEW_VERSION)
>> +#define ASPEED_SDMC_NEW_1024MB (0x3 | ASPEED_SDMC_NEW_VERSION)
>> +
>> +#endif /* ASPEED_SDMC_H */
>
> Cheers,
Thanks for the review, this patch needs another spin.
C.
