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

[Cédric Le Goater]

On 07/25/2016 05:12 PM, Peter Maydell wrote:
> 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.)

symbolic constants are better but I was lazy for these. I will see 
if I can build a mask from the above defines.

> 
>> +        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.

OK will do. 

>> +    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;".

Indeed.

>> +    default:
>> +        g_assert_not_reached();
>> +    }
>> +}
> 
> Otherwise
> Reviewed-by: Peter Maydell <address@hidden>

Thanks for the review, I will send an updated version.

C. 


> thanks
> -- PMM
>