[PING] Added the -m flag feature to stellaris boards

Commit Message

Aurelio Remonda March 21, 2016, 6:42 p.m. UTC

This patch adds the memory flag to both stellaris LM3S811EVB
and LM3S6965EVB.

The hardcoded dc0 values for both boards still exists but now the higher 16 bits
are calculated based on ram_size which could be either user-given or the default
one. Then the sram_size is calculated as usual, flash_size is not affected by
this.

As the higher part of dc0 has a top value of 0xffff, the boards won't accept a
size value larger than 16M. We'll throw an error if the user tries to make the
RAM larger than that.

The default RAM sizes are now set in the boards' respective class_init
functions.

I tested this on the LM3S6965evb doing a full system emulation.
I couldn't try this on the LM3S811EVB since I am using RTEMS and it does
not support that board.

Signed-off-by: Aurelio Remonda <[e-mail]>

---
 hw/arm/stellaris.c | 38 +++++++++++++++++++++++++++-----------
 1 file changed, 27 insertions(+), 11 deletions(-)

--
2.7.3

Comments

Peter Maydell March 23, 2016, 3:39 p.m. UTC | #1

On 21 March 2016 at 18:42, Aurelio Remonda
<[e-mail]> wrote:
> This patch adds the memory flag to both stellaris LM3S811EVB
> and LM3S6965EVB.
>
> The hardcoded dc0 values for both boards still exists but now the higher 16 bits
> are calculated based on ram_size which could be either user-given or the default
> one. Then the sram_size is calculated as usual, flash_size is not affected by
> this.
>
> As the higher part of dc0 has a top value of 0xffff, the boards won't accept a
> size value larger than 16M. We'll throw an error if the user tries to make the
> RAM larger than that.
>
> The default RAM sizes are now set in the boards' respective class_init
> functions.
>
> I tested this on the LM3S6965evb doing a full system emulation.
> I couldn't try this on the LM3S811EVB since I am using RTEMS and it does
> not support that board.
>
> Signed-off-by: Aurelio Remonda <[e-mail]>
>
> ---
>  hw/arm/stellaris.c | 38 +++++++++++++++++++++++++++-----------
>  1 file changed, 27 insertions(+), 11 deletions(-)
>
> diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
> index 0114e0a..dbbb1c8 100644
> --- a/hw/arm/stellaris.c
> +++ b/hw/arm/stellaris.c
> @@ -17,6 +17,7 @@
>  #include "hw/boards.h"
>  #include "exec/address-spaces.h"
>  #include "sysemu/sysemu.h"
> +#include "qemu/error-report.h"
>
>  #define GPIO_A 0
>  #define GPIO_B 1
> @@ -31,17 +32,21 @@
>  #define BP_GAMEPAD   0x04
>
>  #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 ?

> +#define DC0_MAX_SRAM 0xffff /* Maximum value for sram half in dc0 register */
> +#define DC0_SRAM_SHIFT 16
>
> -typedef const struct {
> +typedef struct {
>      const char *name;
> -    uint32_t did0;
> -    uint32_t did1;
> +    const uint32_t did0;
> +    const uint32_t did1;
>      uint32_t dc0;
> -    uint32_t dc1;
> -    uint32_t dc2;
> -    uint32_t dc3;
> -    uint32_t dc4;
> -    uint32_t peripherals;
> +    const uint32_t dc1;
> +    const uint32_t dc2;
> +    const uint32_t dc3;
> +    const uint32_t dc4;
> +    const uint32_t peripherals;
>  } stellaris_board_info;
>
>  /* General purpose timer module.  */
> @@ -1190,7 +1195,7 @@ static stellaris_board_info stellaris_boards[] = {
>    { "LM3S811EVB",
>      0,
>      0x0032000e,
> -    0x001f001f, /* dc0 */
> +    0x0000001f, /* dc0 */
>      0x001132bf,
>      0x01071013,
>      0x3f0f01ff,
> @@ -1200,7 +1205,7 @@ static stellaris_board_info stellaris_boards[] = {
>    { "LM3S6965EVB",
>      0x10010002,
>      0x1073402e,
> -    0x00ff007f, /* dc0 */
> +    0x0000007f, /* dc0 */
>      0x001133ff,
>      0x030f5317,
>      0x0f0f87ff,
> @@ -1223,7 +1228,7 @@ static void stellaris_init(const char *kernel_filename, const char *cpu_model,
>      qemu_irq gpio_in[7][8];
>      qemu_irq gpio_out[7][8];
>      qemu_irq adc;
> -    int sram_size;
> +    unsigned int sram_size;
>      int flash_size;
>      I2CBus *i2c;
>      DeviceState *dev;
> @@ -1234,6 +1239,15 @@ static void stellaris_init(const char *kernel_filename, const char *cpu_model,
>      MemoryRegion *flash = g_new(MemoryRegion, 1);
>      MemoryRegion *system_memory = get_system_memory();
>
> +    /* 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 ?

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

>  }
>
>  static const TypeInfo lm3s811evb_type = {
> @@ -1405,6 +1420,7 @@ static void lm3s6965evb_class_init(ObjectClass *oc, void *data)
>
>      mc->desc = "Stellaris LM3S6965EVB";
>      mc->init = lm3s6965evb_init;
> +    mc->default_ram_size = LM3S6965EVB_DEFAULT_DC0;
>  }
>
>  static const TypeInfo lm3s6965evb_type = {
> --
> 2.7.3

thanks
-- PMM

Aurelio Remonda March 28, 2016, 6:15 p.m. UTC | #2

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

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

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

In the case of LM3S811EVB the value is aligned up and you get a ram
size of 8192 but your
dc0 will still be 0x001f001f.

In the other board LM3S6965EVB the value is aligned up to a ram size
of 65536 but your dc0
will be the default 0x00ff007f.

Thank you.

Aurelio Remonda April 14, 2016, 12:21 p.m. UTC | #3

Hello Peter, have you had a chance to look at this?

On Mon, Mar 28, 2016 at 3:15 PM, Aurelio Remonda
<[e-mail]> 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.
>
>>> +    /* 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.
>
>>> @@ -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.
>
> In the case of LM3S811EVB the value is aligned up and you get a ram
> size of 8192 but your
> dc0 will still be 0x001f001f.
>
> In the other board LM3S6965EVB the value is aligned up to a ram size
> of 65536 but your dc0
> will be the default 0x00ff007f.
>
> Thank you.
> --
> Aurelio Remonda
>
> Taller Technologies Argentina
>
> Software Engineer
>
> San Lorenzo 47, 3rd Floor, Office 5
> Córdoba, Argentina
> Phone: [phone]

Peter Maydell April 14, 2016, 12:31 p.m. UTC | #4

On 14 April 2016 at 13:21, Aurelio Remonda
<[e-mail]> wrote:
> Hello Peter, have you had a chance to look at this?

I made review comments in my previous mail on this patch, which means
it's now back to you to update the patch and resend.

thanks
-- PMM

Aurelio Remonda April 14, 2016, 1:27 p.m. UTC | #5

Ok, but I explained my decisions on a the mail dated 03/28, responding
on your mail dated 03/23 maybe you missed that mail.
Thanks!

Peter Maydell April 14, 2016, 1:42 p.m. UTC | #6

On 28 March 2016 at 19:15, Aurelio Remonda
<[e-mail]> 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

Patch

diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
index 0114e0a..dbbb1c8 100644
--- a/hw/arm/stellaris.c
+++ b/hw/arm/stellaris.c
@@ -17,6 +17,7 @@ 
 #include "hw/boards.h"
 #include "exec/address-spaces.h"
 #include "sysemu/sysemu.h"
+#include "qemu/error-report.h"

 #define GPIO_A 0
 #define GPIO_B 1
@@ -31,17 +32,21 @@ 
 #define BP_GAMEPAD   0x04

 #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 */
+#define DC0_MAX_SRAM 0xffff /* Maximum value for sram half in dc0 register */
+#define DC0_SRAM_SHIFT 16

-typedef const struct {
+typedef struct {
     const char *name;
-    uint32_t did0;
-    uint32_t did1;
+    const uint32_t did0;
+    const uint32_t did1;
     uint32_t dc0;
-    uint32_t dc1;
-    uint32_t dc2;
-    uint32_t dc3;
-    uint32_t dc4;
-    uint32_t peripherals;
+    const uint32_t dc1;
+    const uint32_t dc2;
+    const uint32_t dc3;
+    const uint32_t dc4;
+    const uint32_t peripherals;
 } stellaris_board_info;

 /* General purpose timer module.  */
@@ -1190,7 +1195,7 @@  static stellaris_board_info stellaris_boards[] = {
   { "LM3S811EVB",
     0,
     0x0032000e,
-    0x001f001f, /* dc0 */
+    0x0000001f, /* dc0 */
     0x001132bf,
     0x01071013,
     0x3f0f01ff,
@@ -1200,7 +1205,7 @@  static stellaris_board_info stellaris_boards[] = {
   { "LM3S6965EVB",
     0x10010002,
     0x1073402e,
-    0x00ff007f, /* dc0 */
+    0x0000007f, /* dc0 */
     0x001133ff,
     0x030f5317,
     0x0f0f87ff,
@@ -1223,7 +1228,7 @@  static void stellaris_init(const char *kernel_filename, const char *cpu_model,
     qemu_irq gpio_in[7][8];
     qemu_irq gpio_out[7][8];
     qemu_irq adc;
-    int sram_size;
+    unsigned int sram_size;
     int flash_size;
     I2CBus *i2c;
     DeviceState *dev;
@@ -1234,6 +1239,15 @@  static void stellaris_init(const char *kernel_filename, const char *cpu_model,
     MemoryRegion *flash = g_new(MemoryRegion, 1);
     MemoryRegion *system_memory = get_system_memory();

+    /* 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;

@@ -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;
 }

 static const TypeInfo lm3s811evb_type = {
@@ -1405,6 +1420,7 @@  static void lm3s6965evb_class_init(ObjectClass *oc, void *data)

     mc->desc = "Stellaris LM3S6965EVB";
     mc->init = lm3s6965evb_init;
+    mc->default_ram_size = LM3S6965EVB_DEFAULT_DC0;
 }

 static const TypeInfo lm3s6965evb_type = {