On 07/25/2016 05:12 PM, Peter Maydell wrote:
> 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.)
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 <peter.mayd...@linaro.org>
Thanks for the review, I will send an updated version.
C.
> thanks
> -- PMM
>
