[U-Boot] [Resend RFC PATCH v2] mips: Use unsigned int when reading c0 registers

[Chris Packham]

On Wed, Jul 15, 2015 at 9:12 AM, Chris Packham <judge.packham at gmail.com> wrote:
> On Wed, Jul 15, 2015 at 7:01 AM, Daniel Schwierzeck
> <daniel.schwierzeck at gmail.com> wrote:
>> Hi Chris,
>>
>> sorry for the delay.
>
> No problem. It only just occurred to me that it's probably peak
> holiday season for people in the northern hemisphere.
>
>> Am 14.07.2015 um 12:54 schrieb Chris Packham:
>>> In commit a18a477 (MIPS: use common code from lib/time.c) MIPS platforms
>>> started using common the common timer functions which are based around
>>> the fact that many platforms have a 32-bit free running counter register
>>> that can be used see commit 8dfafdd (Introduce common timer functions).
>>>
>>> Even MIPS64 has such a 32-bit register (some have an additional 64-bit free
>>> running counter, but that's something for another time).
>>>
>>> The problem is that in __read_32bit_c0_register() we read the value from
>>> this register into an _signed_ int and as it's returned up the call
>>> chain to timer_read_counter() it gets assigned to an unsigned long. On a
>>> 32-bit system there is no problem. On a 64-bit system odd things happen,
>>> sign extension seems to kick in and all of a sudden if the counter
>>> register happens to have the MSb (i.e. the sign bit) set the negative
>>> int gets sign extended into a very large unsigned long value. This in
>>> turn throws out things from get_ticks() up.
>>>
>>> Update __read_32bit_c0_register() and __read_32bit_c0_ctrl_register() to
>>> use "unsigned int res;" instead of "int res;". There seems to be little
>>> reason to treat these register values as signed. They are either
>>> counters (which by definition are unsigned) or are made up of various
>>> bit fields to be interpreted as per the CPU datasheet.
>>
>> I agree that those macros should always use unsigned int's. Also some
>> similar but newer macros use unsigned int's. But that header file is
>> imported from Linux kernel and I'd like to keep it in sync. Could you
>> post a similar patch to Linux MIPS mailing list? Maybe someone there
>> know why signed int's are used and if a change would have side-effects.
>> Thanks.
>
> OK I'll go looking there, they may have already fixed it.
>

Linux patch is working it's way through
http://www.linux-mips.org/archives/linux-mips/2015-07/msg00262.html.
Looks like I've missed the merge window for 4.2 so this is queued up
for 4.3. It doesn't appear to be in any public repo yet. If I wait for
the Linux 4.3 merge window to open before syncing this change to
u-boot I'll miss the 2015.10 merge window.

How do you want to proceed? Would it be possible to apply this patch
to u-boot now with the knowledge that it could be sync'd in the
future.

>>
>>>
>>> Reported-by: Sachin Surendran <sachin.surendran at alliedtelesis.co.nz>
>>> Signed-off-by: Chris Packham <judge.packham at gmail.com>
>>>
>>> ---
>>>
>>> Changes in v2:
>>> - Use Rob's current email address
>>>
>>>  arch/mips/include/asm/mipsregs.h | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/mips/include/asm/mipsregs.h b/arch/mips/include/asm/mipsregs.h
>>> index 3571e4f..c7a0849 100644
>>> --- a/arch/mips/include/asm/mipsregs.h
>>> +++ b/arch/mips/include/asm/mipsregs.h
>>> @@ -594,7 +594,7 @@ do {                                                              \
>>>   */
>>>
>>>  #define __read_32bit_c0_register(source, sel)                                \
>>> -({ int __res;                                                                \
>>> +({ unsigned int __res;                                                       \
>>>       if (sel == 0)                                                   \
>>>               __asm__ __volatile__(                                   \
>>>                       "mfc0\t%0, " #source "\n\t"                     \
>>> @@ -676,7 +676,7 @@ do {                                                                      \
>>>   * On RM7000/RM9000 these are uses to access cop0 set 1 registers
>>>   */
>>>  #define __read_32bit_c0_ctrl_register(source)                                \
>>> -({ int __res;                                                                \
>>> +({ unsigned int __res;                                                       \
>>>       __asm__ __volatile__(                                           \
>>>               "cfc0\t%0, " #source "\n\t"                             \
>>>               : "=r" (__res));                                        \
>>>
>>
>> --
>> - Daniel