Re: [Qemu-devel] [PING][PATCH] Added the -m flag feature to stellaris boards

[Peter Maydell]

On 28 March 2016 at 19:15, Aurelio Remonda
<address@hidden> wrote:
>>>  #define NUM_IRQ_LINES 64
>>> +#define LM3S811EVB_DEFAULT_DC0 0x00001f00 /* Default value for dc0 
>>> sram_size half */
>>> +#define LM3S6965EVB_DEFAULT_DC0 0x0000ff00 /* Default value for dc0 
>>> sram_size half */
>>
>> These don't seem to be the same as the default values we had previously ?
>
> I thought it will be easier to see in hexadecimal rather than decimal,
> these are ram_size just like
> user-given are.

Your patch should not be changing default behaviour -- the default value
before your patch should be the same as after.

>>> +    /* RAM size should be divided by 256 in order to get a valid 16 bits 
>>> dc0 value */
>>> +    ram_size = (ram_size >> 8) - 1;
>>> +
>>> +    if (ram_size > DC0_MAX_SRAM) {
>>> +        error_report("Requested RAM size is too big for this board. The 
>>> maximum allowed is 16M.");
>>> +        exit(EXIT_FAILURE);
>>> +    }
>>> +
>>> +    board->dc0 |= ram_size << DC0_SRAM_SHIFT;
>>>      flash_size = (((board->dc0 & 0xffff) + 1) << 1) * 1024;
>>>      sram_size = ((board->dc0 >> 18) + 1) * 1024;
>>
>> Do you know why your DC0_SRAM_SHIFT is 16 but this line which
>> calculates sram_size from board->dc0 is doing a shift by 18 ?
>
> DC0_SRAM_SHIFT will just place the ram_size, prevously calculated
> based on the decimal ram_size
> value, in the correct dc0 half. Then the sram_size will be calculated as 
> always.

It does look odd though, because in principle we should be taking
a size in bytes (that's the input ram_size) and then writing it
into the dc0 value somehow, and then if we extract sram_size from
dc0 that should be the exact reverse transformation to get a
size in bytes. I suspect that this is the result of that "divide
by 256" step you have earlier (so "divide by 256 then shift by 16"
vs "shift by 18 then multiply by 1024" kind of cancel each other
out. I think you should write this code so that it's clear that the
two transformations are inverses of each other, though. (Or alternatively,
don't extract the sram_size from the dc0 value, because we already
know it.)

>>> @@ -1391,6 +1405,7 @@ static void lm3s811evb_class_init(ObjectClass *oc, 
>>> void *data)
>>>
>>>      mc->desc = "Stellaris LM3S811EVB";
>>>      mc->init = lm3s811evb_init;
>>> +    mc->default_ram_size = LM3S811EVB_DEFAULT_DC0;
>>
>> The default_ram_size should be a size in bytes, not a DC0 value.
>
> As I said before, I thought it was easier to see ff00 rather than
> 65280, or 1f00 instead of 7936.
> These values are fixed as default values to match the dc0 default
> value on each board.

The default_ram_size field *must* be a size in bytes (this is
how the rest of QEMU interprets it). The DC0 value is not a size in
bytes.

thanks
-- PMM