On 8 July 2016 at 17:06, Cédric Le Goater <c...@kaod.org> 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 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.
>
> Signed-off-by: Cédric Le Goater <c...@kaod.org>
> ---
Just a few nits below.
> +/*
> + * Configuration register Ox4 (for Aspeed AST2400 SOC)
> + *
> + * These are for the record and future use. ASPEED_SDMC_DRAM_SIZE is
> + * what we care about right now as it is checked by U-Boot to
> + * determine the RAM size.
> + */
> +
> +#define ASPEED_SDMC_AST2300_COMPAT (1 << 10)
> +#define ASPEED_SDMC_SCRAMBLE_PATTERN (1 << 9)
> +#define ASPEED_SDMC_DATA_SCRAMBLE (1 << 8)
> +#define ASPEED_SDMC_ECC_ENABLE (1 << 7)
> +#define ASPEED_SDMC_VGA_COMPAT (1 << 6) /* readonly */
> +#define ASPEED_SDMC_DRAM_BANK (1 << 5)
> +#define ASPEED_SDMC_DRAM_BURST (1 << 4)
> +#define ASPEED_SDMC_VGA_APERTURE(x) ((x & 0x3) << 2) /* readonly */
> +#define ASPEED_SDMC_VGA_8MB 0x0
> +#define ASPEED_SDMC_VGA_16MB 0x1
> +#define ASPEED_SDMC_VGA_32MB 0x2
> +#define ASPEED_SDMC_VGA_64MB 0x3
> +#define ASPEED_SDMC_DRAM_SIZE(x) (x & 0x3)
> +#define ASPEED_SDMC_DRAM_64MB 0x0
> +#define ASPEED_SDMC_DRAM_128MB 0x1
> +#define ASPEED_SDMC_DRAM_256MB 0x2
> +#define ASPEED_SDMC_DRAM_512MB 0x3
> +
> +/*
> + * Configuration register Ox4 (for Aspeed AST2500 SOC and higher)
> + *
> + * Incompatibilities are annoted in the list. ASPEED_SDMC_HW_VERSION
"noted" or "annotated".
> + * should be set to 1 for the AST2500 SOC.
> + */
> +#define ASPEED_SDMC_HW_VERSION(x) ((x & 0xf) << 28) /* readonly */
> +#define ASPEED_SDMC_SW_VERSION ((x & 0xff) << 20)
> +#define ASPEED_SDMC_CACHE_INITIAL_DONE (1 << 19) /* readonly */
> +#define ASPEED_SDMC_CACHE_DDR4_CONF (1 << 13)
> +#define ASPEED_SDMC_CACHE_INITIAL (1 << 12)
> +#define ASPEED_SDMC_CACHE_RANGE_CTRL (1 << 11)
> +#define ASPEED_SDMC_CACHE_ENABLE (1 << 10) /* differs from AST2400 */
> +#define ASPEED_SDMC_DRAM_TYPE (1 << 4) /* differs from AST2400 */
> +
> +/* DRAM size definitions differs */
> +#define ASPEED_SDMC_AST2500_128MB 0x0
> +#define ASPEED_SDMC_AST2500_256MB 0x1
> +#define ASPEED_SDMC_AST2500_512MB 0x2
> +#define ASPEED_SDMC_AST2500_1024MB 0x3
> +
> + if (addr == R_CONF) {
> + /* Make sure readonly bits are kept */
> + switch (s->silicon_rev) {
> + case AST2400_A0_SILICON_REV:
> + data &= 0x000007B3;
> + break;
> + case AST2500_A0_SILICON_REV:
> + data &= 0x0FF03FB3;
> + break;
Maybe these would be more readable using the symbolic constant
names you #defined above? (Or maybe not; your choice.)
> + default:
> + g_assert_not_reached();
> + }
> + }
> +
> + 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,
.valid.unaligned = false is the default, you don't need to
specify it explicitly.
> +};
> +
> +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;
These should have newlines between the case line and the code.
> + default:
> + qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid RAM size: 0x%"
> + HWADDR_PRIx "\n", __func__, ram_size);
> + break;
> + }
> +
> + /* set a minimum default */
> + return ASPEED_SDMC_DRAM_64MB;
> +}
> +
> +static int ast2500_rambits(void)
> +{
> + switch (ram_size >> 20) {
> + case 128: return ASPEED_SDMC_AST2500_128MB;
> + case 256: return ASPEED_SDMC_AST2500_256MB;
> + case 512: return ASPEED_SDMC_AST2500_512MB;
> + case 1024: return ASPEED_SDMC_AST2500_1024MB;
Ditto.
> + default:
> + qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid RAM size: 0x%"
> + HWADDR_PRIx "\n", __func__, ram_size);
> + break;
> + }
> +
> + /* set a minimum default */
> + return ASPEED_SDMC_AST2500_128MB;
> +}
> +
> +static void aspeed_sdmc_reset(DeviceState *dev)
> +{
> + AspeedSDMCState *s = ASPEED_SDMC(dev);
> +
> + memset(s->regs, 0, sizeof(s->regs));
> +
> + /* Set ram size bit and defaults values */
> + switch (s->silicon_rev) {
> + case AST2400_A0_SILICON_REV:
> + s->regs[R_CONF] |=
> + ASPEED_SDMC_VGA_COMPAT |
> + ASPEED_SDMC_DRAM_SIZE(ast2400_rambits());
> + break;
> +
> + case AST2500_A0_SILICON_REV:
> + s->regs[R_CONF] |=
> + ASPEED_SDMC_HW_VERSION(1) |
> + ASPEED_SDMC_VGA_APERTURE(ASPEED_SDMC_VGA_64MB) |
> + ASPEED_SDMC_DRAM_SIZE(ast2500_rambits());
Missing "break;".
> + default:
> + g_assert_not_reached();
> + }
> +}
Otherwise
Reviewed-by: Peter Maydell <peter.mayd...@linaro.org>
thanks
-- PMM
