Patchwork musicpal: Fix flash mapping

login
register
mail settings
Submitter Jan Kiszka
Date Sept. 6, 2012, 11:03 p.m.
Message ID <50492BC5.4090107@web.de>
Download mbox | patch
Permalink /patch/182299/
State New
Headers show

Comments

Jan Kiszka - Sept. 6, 2012, 11:03 p.m.
The old arithmetic assumed 32 physical address bits which is no longer
true for ARM since 3cc0cd61f4.

Signed-off-by: Jan Kiszka <jan.kiszka@web.de>
---
 hw/musicpal.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
Peter Maydell - Sept. 7, 2012, 2:41 p.m.
On 7 September 2012 00:03, Jan Kiszka <jan.kiszka@web.de> wrote:
> The old arithmetic assumed 32 physical address bits which is no longer
> true for ARM since 3cc0cd61f4.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@web.de>
> ---
>  hw/musicpal.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/musicpal.c b/hw/musicpal.c
> index ad725b5..10c2c16 100644
> --- a/hw/musicpal.c
> +++ b/hw/musicpal.c
> @@ -1583,7 +1583,7 @@ static void musicpal_init(ram_addr_t ram_size,
>           * image is smaller than 32 MB.
>           */
>  #ifdef TARGET_WORDS_BIGENDIAN
> -        pflash_cfi02_register(0-MP_FLASH_SIZE_MAX, NULL,
> +        pflash_cfi02_register(0x100000000-MP_FLASH_SIZE_MAX, NULL,

I don't think this will compile on a 32 bit system, will it?
You probably want an ULL suffix.

-- PMM
Jan Kiszka - Sept. 7, 2012, 2:53 p.m.
On 2012-09-07 16:41, Peter Maydell wrote:
> On 7 September 2012 00:03, Jan Kiszka <jan.kiszka@web.de> wrote:
>> The old arithmetic assumed 32 physical address bits which is no longer
>> true for ARM since 3cc0cd61f4.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@web.de>
>> ---
>>  hw/musicpal.c |    4 ++--
>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/musicpal.c b/hw/musicpal.c
>> index ad725b5..10c2c16 100644
>> --- a/hw/musicpal.c
>> +++ b/hw/musicpal.c
>> @@ -1583,7 +1583,7 @@ static void musicpal_init(ram_addr_t ram_size,
>>           * image is smaller than 32 MB.
>>           */
>>  #ifdef TARGET_WORDS_BIGENDIAN
>> -        pflash_cfi02_register(0-MP_FLASH_SIZE_MAX, NULL,
>> +        pflash_cfi02_register(0x100000000-MP_FLASH_SIZE_MAX, NULL,
> 
> I don't think this will compile on a 32 bit system, will it?
> You probably want an ULL suffix.

It does as the result always fits in 32 bits. But I can add that if you
prefer.

Jan
Peter Maydell - Sept. 7, 2012, 3:25 p.m.
On 7 September 2012 15:53, Jan Kiszka <jan.kiszka@web.de> wrote:
> On 2012-09-07 16:41, Peter Maydell wrote:
>> On 7 September 2012 00:03, Jan Kiszka <jan.kiszka@web.de> wrote:
>>> +        pflash_cfi02_register(0x100000000-MP_FLASH_SIZE_MAX, NULL,
>>
>> I don't think this will compile on a 32 bit system, will it?
>> You probably want an ULL suffix.
>
> It does as the result always fits in 32 bits. But I can add that if you
> prefer.

I think I had a misconception of this bit of the C standard.
C will pick a type big enough to fit the constant value (which
will in this case be a 64 bit type of some kind), even without
an ULL suffix. So you're right, it's OK.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM
Blue Swirl - Sept. 8, 2012, 8:44 a.m.
On Fri, Sep 7, 2012 at 3:25 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 7 September 2012 15:53, Jan Kiszka <jan.kiszka@web.de> wrote:
>> On 2012-09-07 16:41, Peter Maydell wrote:
>>> On 7 September 2012 00:03, Jan Kiszka <jan.kiszka@web.de> wrote:
>>>> +        pflash_cfi02_register(0x100000000-MP_FLASH_SIZE_MAX, NULL,
>>>
>>> I don't think this will compile on a 32 bit system, will it?
>>> You probably want an ULL suffix.
>>
>> It does as the result always fits in 32 bits. But I can add that if you
>> prefer.
>
> I think I had a misconception of this bit of the C standard.
> C will pick a type big enough to fit the constant value (which
> will in this case be a 64 bit type of some kind), even without
> an ULL suffix. So you're right, it's OK.

GCC disagrees:
$ cat u64.c
unsigned int i = 0x100000000 - 1;
$ gcc -m32 -Wall -c u64.c
u64.c:1: warning: integer constant is too large for 'long' type

Clang doesn't care even with --all-warnings:
$ clang -m32 -Wall --all-warnings -c u64.c

>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
> -- PMM
>
Jan Kiszka - Sept. 8, 2012, 8:50 a.m.
On 2012-09-08 10:44, Blue Swirl wrote:
> On Fri, Sep 7, 2012 at 3:25 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 7 September 2012 15:53, Jan Kiszka <jan.kiszka@web.de> wrote:
>>> On 2012-09-07 16:41, Peter Maydell wrote:
>>>> On 7 September 2012 00:03, Jan Kiszka <jan.kiszka@web.de> wrote:
>>>>> +        pflash_cfi02_register(0x100000000-MP_FLASH_SIZE_MAX, NULL,
>>>>
>>>> I don't think this will compile on a 32 bit system, will it?
>>>> You probably want an ULL suffix.
>>>
>>> It does as the result always fits in 32 bits. But I can add that if you
>>> prefer.
>>
>> I think I had a misconception of this bit of the C standard.
>> C will pick a type big enough to fit the constant value (which
>> will in this case be a 64 bit type of some kind), even without
>> an ULL suffix. So you're right, it's OK.
> 
> GCC disagrees:
> $ cat u64.c
> unsigned int i = 0x100000000 - 1;
> $ gcc -m32 -Wall -c u64.c
> u64.c:1: warning: integer constant is too large for 'long' type

Obviously depends on the compiler version or configuration, mine (4.5
still) does not. I'll send v2 to make them all happy.

Jan

Patch

diff --git a/hw/musicpal.c b/hw/musicpal.c
index ad725b5..10c2c16 100644
--- a/hw/musicpal.c
+++ b/hw/musicpal.c
@@ -1583,7 +1583,7 @@  static void musicpal_init(ram_addr_t ram_size,
          * image is smaller than 32 MB.
          */
 #ifdef TARGET_WORDS_BIGENDIAN
-        pflash_cfi02_register(0-MP_FLASH_SIZE_MAX, NULL,
+        pflash_cfi02_register(0x100000000-MP_FLASH_SIZE_MAX, NULL,
                               "musicpal.flash", flash_size,
                               dinfo->bdrv, 0x10000,
                               (flash_size + 0xffff) >> 16,
@@ -1591,7 +1591,7 @@  static void musicpal_init(ram_addr_t ram_size,
                               2, 0x00BF, 0x236D, 0x0000, 0x0000,
                               0x5555, 0x2AAA, 1);
 #else
-        pflash_cfi02_register(0-MP_FLASH_SIZE_MAX, NULL,
+        pflash_cfi02_register(0x100000000-MP_FLASH_SIZE_MAX, NULL,
                               "musicpal.flash", flash_size,
                               dinfo->bdrv, 0x10000,
                               (flash_size + 0xffff) >> 16,