musicpal: Fix flash mapping

Submitted by Jan Kiszka on Sept. 6, 2012, 11:03 p.m.

Details

Message ID 50492BC5.4090107@web.de
State New
Headers show

Commit Message

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(-)

Comments

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 hide | download patch | download mbox

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,