From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chris Packham Date: Thu, 16 Jul 2015 12:14:15 +1200 Subject: [U-Boot] [Resend RFC PATCH v2] mips: Use unsigned int when reading c0 registers In-Reply-To: References: <1436871282-5602-1-git-send-email-judge.packham@gmail.com> <1436871282-5602-2-git-send-email-judge.packham@gmail.com> <55A55C89.3030708@gmail.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Wed, Jul 15, 2015 at 9:12 AM, Chris Packham wrote: > On Wed, Jul 15, 2015 at 7:01 AM, Daniel Schwierzeck > 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 >>> Signed-off-by: Chris Packham >>> >>> --- >>> >>> 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