Re: [Qemu-devel] [PATCH 5/5] ast2400: add a memory controller device model

[Peter Maydell]

On 8 July 2016 at 17:06, Cédric Le Goater <address@hidden> 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 <address@hidden>
> ---

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 <address@hidden>

thanks
-- PMM