[U-Boot] [PATCH 1/4] ARM: bcm283x: Repair wdog.h

[U-Boot] [PATCH 1/4] ARM: bcm283x: Repair wdog.h

[Marek Vasut]

Trivially fix the include check in wdog.h.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: Tyler Baker <tyler.baker@linaro.org>
---
 arch/arm/mach-bcm283x/include/mach/wdog.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-bcm283x/include/mach/wdog.h b/arch/arm/mach-bcm283x/include/mach/wdog.h
index beb6a08..cbc350e 100644
--- a/arch/arm/mach-bcm283x/include/mach/wdog.h
+++ b/arch/arm/mach-bcm283x/include/mach/wdog.h
@@ -4,8 +4,8 @@
  * SPDX-License-Identifier:	GPL-2.0
  */
 
-#ifndef _BCM2835_TIMER_H
-#define _BCM2835_TIMER_H
+#ifndef _BCM2835_WDOG_H
+#define _BCM2835_WDOG_H
 
 #ifdef CONFIG_BCM2836
 #define BCM2835_WDOG_PHYSADDR			0x3f100000
-- 
2.1.4

[U-Boot] [PATCH 2/4] ARM: bcm283x: Reorder timer.h

[Marek Vasut]

Reorder the timer.h file so it can be included from board config file.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: Tyler Baker <tyler.baker@linaro.org>
---
 arch/arm/mach-bcm283x/include/mach/timer.h | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mach-bcm283x/include/mach/timer.h b/arch/arm/mach-bcm283x/include/mach/timer.h
index fc7aec7..004c24b 100644
--- a/arch/arm/mach-bcm283x/include/mach/timer.h
+++ b/arch/arm/mach-bcm283x/include/mach/timer.h
@@ -13,6 +13,14 @@
 #define BCM2835_TIMER_PHYSADDR	0x20003000
 #endif
 
+#define BCM2835_TIMER_CS_M3	(1 << 3)
+#define BCM2835_TIMER_CS_M2	(1 << 2)
+#define BCM2835_TIMER_CS_M1	(1 << 1)
+#define BCM2835_TIMER_CS_M0	(1 << 0)
+
+#ifndef __ASSEMBLY__
+#include <linux/types.h>
+
 struct bcm2835_timer_regs {
 	u32 cs;
 	u32 clo;
@@ -23,11 +31,8 @@ struct bcm2835_timer_regs {
 	u32 c3;
 };
 
-#define BCM2835_TIMER_CS_M3	(1 << 3)
-#define BCM2835_TIMER_CS_M2	(1 << 2)
-#define BCM2835_TIMER_CS_M1	(1 << 1)
-#define BCM2835_TIMER_CS_M0	(1 << 0)
-
 extern ulong get_timer_us(ulong base);
 
 #endif
+
+#endif
-- 
2.1.4

[U-Boot] [PATCH 3/4] ARM: mmc: bcm283x: Remove get_timer_us() from mmc driver

[Marek Vasut]

The get_timer_us() function is something which is no longer
existing in case we use generic timer framework, so replace
it with get_timer().

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: Tyler Baker <tyler.baker@linaro.org>
---
 drivers/mmc/bcm2835_sdhci.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/bcm2835_sdhci.c b/drivers/mmc/bcm2835_sdhci.c
index 4ec2968..0f81add 100644
--- a/drivers/mmc/bcm2835_sdhci.c
+++ b/drivers/mmc/bcm2835_sdhci.c
@@ -69,11 +69,11 @@ static inline void bcm2835_sdhci_raw_writel(struct sdhci_host *host, u32 val,
 	 * (Which is just as well - otherwise we'd have to nobble the DMA engine
 	 * too)
 	 */
-	while (get_timer_us(bcm_host->last_write) < bcm_host->twoticks_delay)
+	while (get_timer(bcm_host->last_write) < bcm_host->twoticks_delay)
 		;
 
 	writel(val, host->ioaddr + reg);
-	bcm_host->last_write = get_timer_us(0);
+	bcm_host->last_write = get_timer(0);
 }
 
 static inline u32 bcm2835_sdhci_raw_readl(struct sdhci_host *host, int reg)
-- 
2.1.4

[U-Boot] [PATCH 4/4] ARM: bcm283x: Switch to generic timer

[Marek Vasut]

Switch to generic timer implementation from lib/time.c .
This also fixes a signed overflow which was in __udelay()
implementation.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: Tyler Baker <tyler.baker@linaro.org>
---
 arch/arm/mach-bcm283x/Makefile |  2 +-
 arch/arm/mach-bcm283x/timer.c  | 58 ------------------------------------------
 include/configs/rpi-common.h   |  6 +++++
 3 files changed, 7 insertions(+), 59 deletions(-)
 delete mode 100644 arch/arm/mach-bcm283x/timer.c

diff --git a/arch/arm/mach-bcm283x/Makefile b/arch/arm/mach-bcm283x/Makefile
index ac27d00..f0dadd0 100644
--- a/arch/arm/mach-bcm283x/Makefile
+++ b/arch/arm/mach-bcm283x/Makefile
@@ -5,4 +5,4 @@
 #
 
 obj-$(CONFIG_TARGET_RPI) += lowlevel_init.o
-obj-y	+= init.o reset.o timer.o mbox.o phys2bus.o
+obj-y	+= init.o reset.o mbox.o phys2bus.o
diff --git a/arch/arm/mach-bcm283x/timer.c b/arch/arm/mach-bcm283x/timer.c
deleted file mode 100644
index 017907c..0000000
--- a/arch/arm/mach-bcm283x/timer.c
+++ /dev/null
@@ -1,58 +0,0 @@
-/*
- * (C) Copyright 2012 Stephen Warren
- *
- * See file CREDITS for list of people who contributed to this
- * project.
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public License
- * version 2 as published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful, but
- * WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- */
-
-#include <common.h>
-#include <asm/io.h>
-#include <asm/arch/timer.h>
-
-ulong get_timer_us(ulong base)
-{
-	struct bcm2835_timer_regs *regs =
-		(struct bcm2835_timer_regs *)BCM2835_TIMER_PHYSADDR;
-
-	return readl(&regs->clo) - base;
-}
-
-ulong get_timer(ulong base)
-{
-	ulong us = get_timer_us(0);
-	us /= (1000000 / CONFIG_SYS_HZ);
-	us -= base;
-	return us;
-}
-
-unsigned long long get_ticks(void)
-{
-	return get_timer(0);
-}
-
-ulong get_tbclk(void)
-{
-	return CONFIG_SYS_HZ;
-}
-
-void __udelay(unsigned long usec)
-{
-	ulong endtime;
-	signed long diff;
-
-	endtime = get_timer_us(0) + usec;
-
-	do {
-		ulong now = get_timer_us(0);
-		diff = endtime - now;
-	} while (diff >= 0);
-}
diff --git a/include/configs/rpi-common.h b/include/configs/rpi-common.h
index 3121ac9..b54cf8b 100644
--- a/include/configs/rpi-common.h
+++ b/include/configs/rpi-common.h
@@ -8,12 +8,18 @@
 #define _RPI_COMMON_H_
 
 #include <linux/sizes.h>
+#include <asm/arch/timer.h>
 
 /* Architecture, CPU, etc.*/
 #define CONFIG_SYS_GENERIC_BOARD
 #define CONFIG_BCM2835
 #define CONFIG_ARCH_CPU_INIT
 #define CONFIG_SYS_DCACHE_OFF
+
+#define CONFIG_SYS_TIMER_RATE		1000000
+#define CONFIG_SYS_TIMER_COUNTER	\
+	(&((struct bcm2835_timer_regs *)BCM2835_TIMER_PHYSADDR)->clo)
+
 /*
  * 2835 is a SKU in a series for which the 2708 is the first or primary SoC,
  * so 2708 has historically been used rather than a dedicated 2835 ID.
-- 
2.1.4

[U-Boot] [PATCH 3/4] ARM: mmc: bcm283x: Remove get_timer_us() from mmc driver

[Pantelis Antoniou]

Hi Marek,

> On May 4, 2015, at 23:54 , Marek Vasut <marex@denx.de> wrote:
> 
> The get_timer_us() function is something which is no longer
> existing in case we use generic timer framework, so replace
> it with get_timer().
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Stephen Warren <swarren@wwwdotorg.org>
> Cc: Tyler Baker <tyler.baker@linaro.org>
> ---
> drivers/mmc/bcm2835_sdhci.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/bcm2835_sdhci.c b/drivers/mmc/bcm2835_sdhci.c
> index 4ec2968..0f81add 100644
> --- a/drivers/mmc/bcm2835_sdhci.c
> +++ b/drivers/mmc/bcm2835_sdhci.c
> @@ -69,11 +69,11 @@ static inline void bcm2835_sdhci_raw_writel(struct sdhci_host *host, u32 val,
> 	 * (Which is just as well - otherwise we'd have to nobble the DMA engine
> 	 * too)
> 	 */
> -	while (get_timer_us(bcm_host->last_write) < bcm_host->twoticks_delay)
> +	while (get_timer(bcm_host->last_write) < bcm_host->twoticks_delay)
> 		;
> 
> 	writel(val, host->ioaddr + reg);
> -	bcm_host->last_write = get_timer_us(0);
> +	bcm_host->last_write = get_timer(0);
> }
> 
> static inline u32 bcm2835_sdhci_raw_readl(struct sdhci_host *host, int reg)
> -- 
> 2.1.4

Applied, thanks.

? Pantelis

[U-Boot] [PATCH 4/4] ARM: bcm283x: Switch to generic timer

[Stephen Warren]

On 05/04/2015 02:54 PM, Marek Vasut wrote:
> Switch to generic timer implementation from lib/time.c .
> This also fixes a signed overflow which was in __udelay()
> implementation.

Can you explain that a bit more?

> -void __udelay(unsigned long usec)
> -{
> -	ulong endtime;
> -	signed long diff;
> -
> -	endtime = get_timer_us(0) + usec;
> -
> -	do {
> -		ulong now = get_timer_us(0);
> -		diff = endtime - now;
> -	} while (diff >= 0);
> -}

I believe since endtime and now hold micro seconds, there shouldn't be 
any overflow so long as the microsecond difference fits into 31 bits, 
i.e. so long as usec is less than ~36 minutes. I doubt anything is 
calling __udelay() with that large of a value. Perhaps the issue this 
patch fixes is in get_timer_us(0) instead, or something else changed as 
a side-effect?

The other 3 patches in this series,
Acked-by: Stephen Warren <swarren@wwwdotorg.org>

[U-Boot] [PATCH 4/4] ARM: bcm283x: Switch to generic timer

[Marek Vasut]

On Tuesday, May 05, 2015 at 11:46:56 PM, Stephen Warren wrote:
> On 05/04/2015 02:54 PM, Marek Vasut wrote:
> > Switch to generic timer implementation from lib/time.c .
> > This also fixes a signed overflow which was in __udelay()
> > implementation.
> 
> Can you explain that a bit more?
> 
> > -void __udelay(unsigned long usec)
> > -{
> > -	ulong endtime;
> > -	signed long diff;
> > -
> > -	endtime = get_timer_us(0) + usec;
> > -
> > -	do {
> > -		ulong now = get_timer_us(0);
> > -		diff = endtime - now;
> > -	} while (diff >= 0);
> > -}
> 
> I believe since endtime and now hold micro seconds, there shouldn't be
> any overflow so long as the microsecond difference fits into 31 bits,
> i.e. so long as usec is less than ~36 minutes. I doubt anything is
> calling __udelay() with that large of a value. Perhaps the issue this
> patch fixes is in get_timer_us(0) instead, or something else changed as
> a side-effect?

The generic implementation caters for full 32-bit range, that's all.
Since the argument of this function is unsigned, it can overflow if
you use argument which is bigger than 31 bits. OK like that ?

Best regards,
Marek Vasut

[U-Boot] [PATCH 4/4] ARM: bcm283x: Switch to generic timer

[Stephen Warren]

On 05/05/2015 04:17 PM, Marek Vasut wrote:
> On Tuesday, May 05, 2015 at 11:46:56 PM, Stephen Warren wrote:
>> On 05/04/2015 02:54 PM, Marek Vasut wrote:
>>> Switch to generic timer implementation from lib/time.c .
>>> This also fixes a signed overflow which was in __udelay()
>>> implementation.
>>
>> Can you explain that a bit more?
>>
>>> -void __udelay(unsigned long usec)
>>> -{
>>> -	ulong endtime;
>>> -	signed long diff;
>>> -
>>> -	endtime = get_timer_us(0) + usec;
>>> -
>>> -	do {
>>> -		ulong now = get_timer_us(0);
>>> -		diff = endtime - now;
>>> -	} while (diff >= 0);
>>> -}
>>
>> I believe since endtime and now hold micro seconds, there shouldn't be
>> any overflow so long as the microsecond difference fits into 31 bits,
>> i.e. so long as usec is less than ~36 minutes. I doubt anything is
>> calling __udelay() with that large of a value. Perhaps the issue this
>> patch fixes is in get_timer_us(0) instead, or something else changed as
>> a side-effect?
>
> The generic implementation caters for full 32-bit range, that's all.
> Since the argument of this function is unsigned, it can overflow if
> you use argument which is bigger than 31 bits. OK like that ?

Sorry, I still don't understand. Both the __udelay() here and in 
lib/time.c take an unsigned long argument. I don't see how switching one 
out for the other can affect anything if the argument type is the issue. 
Besides, what's passing a value >~36 minutes to udelay()?

[U-Boot] [PATCH 4/4] ARM: bcm283x: Switch to generic timer

[Marek Vasut]

On Wednesday, May 06, 2015 at 12:37:38 AM, Stephen Warren wrote:
> On 05/05/2015 04:17 PM, Marek Vasut wrote:
> > On Tuesday, May 05, 2015 at 11:46:56 PM, Stephen Warren wrote:
> >> On 05/04/2015 02:54 PM, Marek Vasut wrote:
> >>> Switch to generic timer implementation from lib/time.c .
> >>> This also fixes a signed overflow which was in __udelay()
> >>> implementation.
> >> 
> >> Can you explain that a bit more?
> >> 
> >>> -void __udelay(unsigned long usec)
> >>> -{
> >>> -	ulong endtime;
> >>> -	signed long diff;
> >>> -
> >>> -	endtime = get_timer_us(0) + usec;
> >>> -
> >>> -	do {
> >>> -		ulong now = get_timer_us(0);
> >>> -		diff = endtime - now;
> >>> -	} while (diff >= 0);
> >>> -}
> >> 
> >> I believe since endtime and now hold micro seconds, there shouldn't be
> >> any overflow so long as the microsecond difference fits into 31 bits,
> >> i.e. so long as usec is less than ~36 minutes. I doubt anything is
> >> calling __udelay() with that large of a value. Perhaps the issue this
> >> patch fixes is in get_timer_us(0) instead, or something else changed as
> >> a side-effect?
> > 
> > The generic implementation caters for full 32-bit range, that's all.
> > Since the argument of this function is unsigned, it can overflow if
> > you use argument which is bigger than 31 bits. OK like that ?
> 
> Sorry, I still don't understand. Both the __udelay() here and in
> lib/time.c take an unsigned long argument. I don't see how switching one
> out for the other can affect anything if the argument type is the issue.

So, if now is close to 0x7fffffff (which it can), then if endtime is big-ish,
diff will become negative and this udelay() will not perform the correct delay,
right ?

> Besides, what's passing a value >~36 minutes to udelay()?

Nothing, but that doesn't mean we can have a possibly broken implementation, 
right ?

Best regards,
Marek Vasut

[U-Boot] [PATCH 4/4] ARM: bcm283x: Switch to generic timer

[Stephen Warren]

On 05/05/2015 04:42 PM, Marek Vasut wrote:
> On Wednesday, May 06, 2015 at 12:37:38 AM, Stephen Warren wrote:
>> On 05/05/2015 04:17 PM, Marek Vasut wrote:
>>> On Tuesday, May 05, 2015 at 11:46:56 PM, Stephen Warren wrote:
>>>> On 05/04/2015 02:54 PM, Marek Vasut wrote:
>>>>> Switch to generic timer implementation from lib/time.c .
>>>>> This also fixes a signed overflow which was in __udelay()
>>>>> implementation.
>>>>
>>>> Can you explain that a bit more?
>>>>
>>>>> -void __udelay(unsigned long usec)
>>>>> -{
>>>>> -	ulong endtime;
>>>>> -	signed long diff;
>>>>> -
>>>>> -	endtime = get_timer_us(0) + usec;
>>>>> -
>>>>> -	do {
>>>>> -		ulong now = get_timer_us(0);
>>>>> -		diff = endtime - now;
>>>>> -	} while (diff >= 0);
>>>>> -}
>>>>
>>>> I believe since endtime and now hold micro seconds, there shouldn't be
>>>> any overflow so long as the microsecond difference fits into 31 bits,
>>>> i.e. so long as usec is less than ~36 minutes. I doubt anything is
>>>> calling __udelay() with that large of a value. Perhaps the issue this
>>>> patch fixes is in get_timer_us(0) instead, or something else changed as
>>>> a side-effect?
>>>
>>> The generic implementation caters for full 32-bit range, that's all.
>>> Since the argument of this function is unsigned, it can overflow if
>>> you use argument which is bigger than 31 bits. OK like that ?
>>
>> Sorry, I still don't understand. Both the __udelay() here and in
>> lib/time.c take an unsigned long argument. I don't see how switching one
>> out for the other can affect anything if the argument type is the issue.
>
> So, if now is close to 0x7fffffff (which it can), then if endtime is big-ish,
> diff will become negative and this udelay() will not perform the correct delay,
> right ?

I don't believe so, no.

endtime and now are both unsigned. My (admittedly intuitive rather than 
well-researched) understanding of C math promotion rules means that 
"endtime - now" will be calculated as an unsigned value, then converted 
into a signed value to be stored in the signed diff. As such, I would 
expect the value of diff to be a small value in this case. I wrote a 
test program to validate this; endtime = 0x80000002, now = 0x7ffffffe, 
yields diff=4 as expected.

Perhaps you meant a much larger endtime value than 0x80000002; perhaps 
0xffffffff? This doesn't cause issues either. All that's relevant is the 
difference between endtime and now, not their absolute values, and not 
whether endtime has wrapped but now has or hasn't. For example, endtime 
= 0x00000002, now = 0xfffffff0 yields diff=18 as expected.

>> Besides, what's passing a value >~36 minutes to udelay()?
>
> Nothing, but that doesn't mean we can have a possibly broken implementation,
> right ?

True. However, I'd expect that any specification for udelay would 
disallow such large parameter values, and hence its behaviour wouldn't 
be relevant if such values were passed.

[U-Boot] [PATCH 4/4] ARM: bcm283x: Switch to generic timer

[Marek Vasut]

On Wednesday, May 06, 2015 at 12:57:54 AM, Stephen Warren wrote:
> On 05/05/2015 04:42 PM, Marek Vasut wrote:
> > On Wednesday, May 06, 2015 at 12:37:38 AM, Stephen Warren wrote:
> >> On 05/05/2015 04:17 PM, Marek Vasut wrote:
> >>> On Tuesday, May 05, 2015 at 11:46:56 PM, Stephen Warren wrote:
> >>>> On 05/04/2015 02:54 PM, Marek Vasut wrote:
> >>>>> Switch to generic timer implementation from lib/time.c .
> >>>>> This also fixes a signed overflow which was in __udelay()
> >>>>> implementation.
> >>>> 
> >>>> Can you explain that a bit more?
> >>>> 
> >>>>> -void __udelay(unsigned long usec)
> >>>>> -{
> >>>>> -	ulong endtime;
> >>>>> -	signed long diff;
> >>>>> -
> >>>>> -	endtime = get_timer_us(0) + usec;
> >>>>> -
> >>>>> -	do {
> >>>>> -		ulong now = get_timer_us(0);
> >>>>> -		diff = endtime - now;
> >>>>> -	} while (diff >= 0);
> >>>>> -}
> >>>> 
> >>>> I believe since endtime and now hold micro seconds, there shouldn't be
> >>>> any overflow so long as the microsecond difference fits into 31 bits,
> >>>> i.e. so long as usec is less than ~36 minutes. I doubt anything is
> >>>> calling __udelay() with that large of a value. Perhaps the issue this
> >>>> patch fixes is in get_timer_us(0) instead, or something else changed
> >>>> as a side-effect?
> >>> 
> >>> The generic implementation caters for full 32-bit range, that's all.
> >>> Since the argument of this function is unsigned, it can overflow if
> >>> you use argument which is bigger than 31 bits. OK like that ?
> >> 
> >> Sorry, I still don't understand. Both the __udelay() here and in
> >> lib/time.c take an unsigned long argument. I don't see how switching one
> >> out for the other can affect anything if the argument type is the issue.
> > 
> > So, if now is close to 0x7fffffff (which it can), then if endtime is
> > big-ish, diff will become negative and this udelay() will not perform
> > the correct delay, right ?
> 
> I don't believe so, no.
> 
> endtime and now are both unsigned. My (admittedly intuitive rather than
> well-researched) understanding of C math promotion rules means that
> "endtime - now" will be calculated as an unsigned value, then converted
> into a signed value to be stored in the signed diff. As such, I would
> expect the value of diff to be a small value in this case. I wrote a
> test program to validate this; endtime = 0x80000002, now = 0x7ffffffe,
> yields diff=4 as expected.
> 
> Perhaps you meant a much larger endtime value than 0x80000002; perhaps
> 0xffffffff? This doesn't cause issues either. All that's relevant is the
> difference between endtime and now, not their absolute values, and not
> whether endtime has wrapped but now has or hasn't. For example, endtime
> = 0x00000002, now = 0xfffffff0 yields diff=18 as expected.

So what if the difference is bigger than 1 << 31 ?

> >> Besides, what's passing a value >~36 minutes to udelay()?
> > 
> > Nothing, but that doesn't mean we can have a possibly broken
> > implementation, right ?
> 
> True. However, I'd expect that any specification for udelay would
> disallow such large parameter values, and hence its behaviour wouldn't
> be relevant if such values were passed.

Do you think you can pick this patch and drop the "fixes overflow" part
or do you need resubmission ?

Best regards,
Marek Vasut

[U-Boot] [PATCH 4/4] ARM: bcm283x: Switch to generic timer

[Stephen Warren]

On 05/05/2015 05:37 PM, Marek Vasut wrote:
> On Wednesday, May 06, 2015 at 12:57:54 AM, Stephen Warren wrote:
>> On 05/05/2015 04:42 PM, Marek Vasut wrote:
>>> On Wednesday, May 06, 2015 at 12:37:38 AM, Stephen Warren wrote:
>>>> On 05/05/2015 04:17 PM, Marek Vasut wrote:
>>>>> On Tuesday, May 05, 2015 at 11:46:56 PM, Stephen Warren wrote:
>>>>>> On 05/04/2015 02:54 PM, Marek Vasut wrote:
>>>>>>> Switch to generic timer implementation from lib/time.c .
>>>>>>> This also fixes a signed overflow which was in __udelay()
>>>>>>> implementation.
>>>>>>
>>>>>> Can you explain that a bit more?
>>>>>>
>>>>>>> -void __udelay(unsigned long usec)
>>>>>>> -{
>>>>>>> -	ulong endtime;
>>>>>>> -	signed long diff;
>>>>>>> -
>>>>>>> -	endtime = get_timer_us(0) + usec;
>>>>>>> -
>>>>>>> -	do {
>>>>>>> -		ulong now = get_timer_us(0);
>>>>>>> -		diff = endtime - now;
>>>>>>> -	} while (diff >= 0);
>>>>>>> -}
>>>>>>
>>>>>> I believe since endtime and now hold micro seconds, there shouldn't be
>>>>>> any overflow so long as the microsecond difference fits into 31 bits,
>>>>>> i.e. so long as usec is less than ~36 minutes. I doubt anything is
>>>>>> calling __udelay() with that large of a value. Perhaps the issue this
>>>>>> patch fixes is in get_timer_us(0) instead, or something else changed
>>>>>> as a side-effect?
>>>>>
>>>>> The generic implementation caters for full 32-bit range, that's all.
>>>>> Since the argument of this function is unsigned, it can overflow if
>>>>> you use argument which is bigger than 31 bits. OK like that ?
>>>>
>>>> Sorry, I still don't understand. Both the __udelay() here and in
>>>> lib/time.c take an unsigned long argument. I don't see how switching one
>>>> out for the other can affect anything if the argument type is the issue.
>>>
>>> So, if now is close to 0x7fffffff (which it can), then if endtime is
>>> big-ish, diff will become negative and this udelay() will not perform
>>> the correct delay, right ?
>>
>> I don't believe so, no.
>>
>> endtime and now are both unsigned. My (admittedly intuitive rather than
>> well-researched) understanding of C math promotion rules means that
>> "endtime - now" will be calculated as an unsigned value, then converted
>> into a signed value to be stored in the signed diff. As such, I would
>> expect the value of diff to be a small value in this case. I wrote a
>> test program to validate this; endtime = 0x80000002, now = 0x7ffffffe,
>> yields diff=4 as expected.
>>
>> Perhaps you meant a much larger endtime value than 0x80000002; perhaps
>> 0xffffffff? This doesn't cause issues either. All that's relevant is the
>> difference between endtime and now, not their absolute values, and not
>> whether endtime has wrapped but now has or hasn't. For example, endtime
>> = 0x00000002, now = 0xfffffff0 yields diff=18 as expected.
>
> So what if the difference is bigger than 1 << 31 ?

As I said, I don't believe that case is relevant; it can only happen if 
passing ridiculously large delay values into __udelay() (i.e. greater 
than the 1<<31value you mention), and I don't believe there's any need 
to support that.

The implementation in lib/time.c probably has exactly the same problem, 
except that since it uses 64-bit math rather than 32-bit math, so the 
issue happens@1<<63 rather than 1<<31. It's probably equally 
problematic for delay values as large as 1<<63:-) In practice, given 
1<<31 us is so large, I don't think there's any practical difference.

>>>> Besides, what's passing a value >~36 minutes to udelay()?
>>>
>>> Nothing, but that doesn't mean we can have a possibly broken
>>> implementation, right ?
>>
>> True. However, I'd expect that any specification for udelay would
>> disallow such large parameter values, and hence its behaviour wouldn't
>> be relevant if such values were passed.
>
> Do you think you can pick this patch and drop the "fixes overflow" part
> or do you need resubmission ?

Tom Rini (or in the past Albert Aribaud) actually apply the patches.

Re: the patch description: I'd certainly be happy if it was re-written 
to say something more like "replace bcm2835-specific timer logic with 
common code to reduce the number of different implementations for the 
same thing".

I think you'd mentioned on IRC that this change fixed something 
USB-related for you, and I still don't understand how that could be 
possible. Perhaps there's some intermittent problem, and it just 
happened not to show up when you tested after this patch?

[U-Boot] [PATCH 4/4] ARM: bcm283x: Switch to generic timer

[Marek Vasut]

On Wednesday, May 06, 2015 at 05:52:37 PM, Stephen Warren wrote:
[...]
> >>> So, if now is close to 0x7fffffff (which it can), then if endtime is
> >>> big-ish, diff will become negative and this udelay() will not perform
> >>> the correct delay, right ?
> >> 
> >> I don't believe so, no.
> >> 
> >> endtime and now are both unsigned. My (admittedly intuitive rather than
> >> well-researched) understanding of C math promotion rules means that
> >> "endtime - now" will be calculated as an unsigned value, then converted
> >> into a signed value to be stored in the signed diff. As such, I would
> >> expect the value of diff to be a small value in this case. I wrote a
> >> test program to validate this; endtime = 0x80000002, now = 0x7ffffffe,
> >> yields diff=4 as expected.
> >> 
> >> Perhaps you meant a much larger endtime value than 0x80000002; perhaps
> >> 0xffffffff? This doesn't cause issues either. All that's relevant is the
> >> difference between endtime and now, not their absolute values, and not
> >> whether endtime has wrapped but now has or hasn't. For example, endtime
> >> = 0x00000002, now = 0xfffffff0 yields diff=18 as expected.
> > 
> > So what if the difference is bigger than 1 << 31 ?
> 
> As I said, I don't believe that case is relevant; it can only happen if
> passing ridiculously large delay values into __udelay() (i.e. greater
> than the 1<<31value you mention), and I don't believe there's any need
> to support that.

So what you say is that it's OK to have a function which is buggy in
corner cases ?

> The implementation in lib/time.c probably has exactly the same problem,
> except that since it uses 64-bit math rather than 32-bit math, so the
> issue happens at 1<<63 rather than 1<<31. It's probably equally
> problematic for delay values as large as 1<<63:-) In practice, given
> 1<<31 us is so large, I don't think there's any practical difference.

The implementation in lib/time.c uses 32bit usec argument though, so
it's not prone to this overflow. Please correct me if I'm wrong.

> >>>> Besides, what's passing a value >~36 minutes to udelay()?
> >>> 
> >>> Nothing, but that doesn't mean we can have a possibly broken
> >>> implementation, right ?
> >> 
> >> True. However, I'd expect that any specification for udelay would
> >> disallow such large parameter values, and hence its behaviour wouldn't
> >> be relevant if such values were passed.
> > 
> > Do you think you can pick this patch and drop the "fixes overflow" part
> > or do you need resubmission ?
> 
> Tom Rini (or in the past Albert Aribaud) actually apply the patches.
> 
> Re: the patch description: I'd certainly be happy if it was re-written
> to say something more like "replace bcm2835-specific timer logic with
> common code to reduce the number of different implementations for the
> same thing".

Tom, do you want a repost ?

> I think you'd mentioned on IRC that this change fixed something
> USB-related for you, and I still don't understand how that could be
> possible. Perhaps there's some intermittent problem, and it just
> happened not to show up when you tested after this patch?

I think Tyler can elaborate on that, but in his test case, he still
triggers the USB issue.

[U-Boot] [PATCH 4/4] ARM: bcm283x: Switch to generic timer

[Tyler Baker]

On 6 May 2015 at 11:13, Marek Vasut <marex@denx.de> wrote:
> On Wednesday, May 06, 2015 at 05:52:37 PM, Stephen Warren wrote:
> [...]
>> >>> So, if now is close to 0x7fffffff (which it can), then if endtime is
>> >>> big-ish, diff will become negative and this udelay() will not perform
>> >>> the correct delay, right ?
>> >>
>> >> I don't believe so, no.
>> >>
>> >> endtime and now are both unsigned. My (admittedly intuitive rather than
>> >> well-researched) understanding of C math promotion rules means that
>> >> "endtime - now" will be calculated as an unsigned value, then converted
>> >> into a signed value to be stored in the signed diff. As such, I would
>> >> expect the value of diff to be a small value in this case. I wrote a
>> >> test program to validate this; endtime = 0x80000002, now = 0x7ffffffe,
>> >> yields diff=4 as expected.
>> >>
>> >> Perhaps you meant a much larger endtime value than 0x80000002; perhaps
>> >> 0xffffffff? This doesn't cause issues either. All that's relevant is the
>> >> difference between endtime and now, not their absolute values, and not
>> >> whether endtime has wrapped but now has or hasn't. For example, endtime
>> >> = 0x00000002, now = 0xfffffff0 yields diff=18 as expected.
>> >
>> > So what if the difference is bigger than 1 << 31 ?
>>
>> As I said, I don't believe that case is relevant; it can only happen if
>> passing ridiculously large delay values into __udelay() (i.e. greater
>> than the 1<<31value you mention), and I don't believe there's any need
>> to support that.
>
> So what you say is that it's OK to have a function which is buggy in
> corner cases ?
>
>> The implementation in lib/time.c probably has exactly the same problem,
>> except that since it uses 64-bit math rather than 32-bit math, so the
>> issue happens at 1<<63 rather than 1<<31. It's probably equally
>> problematic for delay values as large as 1<<63:-) In practice, given
>> 1<<31 us is so large, I don't think there's any practical difference.
>
> The implementation in lib/time.c uses 32bit usec argument though, so
> it's not prone to this overflow. Please correct me if I'm wrong.
>
>> >>>> Besides, what's passing a value >~36 minutes to udelay()?
>> >>>
>> >>> Nothing, but that doesn't mean we can have a possibly broken
>> >>> implementation, right ?
>> >>
>> >> True. However, I'd expect that any specification for udelay would
>> >> disallow such large parameter values, and hence its behaviour wouldn't
>> >> be relevant if such values were passed.
>> >
>> > Do you think you can pick this patch and drop the "fixes overflow" part
>> > or do you need resubmission ?
>>
>> Tom Rini (or in the past Albert Aribaud) actually apply the patches.
>>
>> Re: the patch description: I'd certainly be happy if it was re-written
>> to say something more like "replace bcm2835-specific timer logic with
>> common code to reduce the number of different implementations for the
>> same thing".
>
> Tom, do you want a repost ?
>
>> I think you'd mentioned on IRC that this change fixed something
>> USB-related for you, and I still don't understand how that could be
>> possible. Perhaps there's some intermittent problem, and it just
>> happened not to show up when you tested after this patch?
>
> I think Tyler can elaborate on that, but in his test case, he still
> triggers the USB issue.

I'll provide some context on the issue I'm fighting...

I recently bought a RPi B+ Model, flashed the latest raspbian image[0]
to an sd card, built the master branch (v2015.05+) u-boot and
overwrote kernel.img with u-boot.bin. U-Boot came right up but I was
unable to obtain a DHCP lease after using 'usb start; dhcp'. I ran
tcpdump and saw the DHCP requests being made, but shortly after the
board was seemly ignoring responses from the DHCP server. Now
sometimes if the response from the DHCP server was quick enough, it
could get a lease, but the tftp transfer would stall. I decided it
would be best to set 'ipaddr, gateway, netmask' to see if this was a
DHCP issue. It turned out that I had no network connectivity even when
I configured the ip address statically.

Could not obtain a lease:

 starting USB...
USB0: Core Release: 2.80a
scanning bus 0 for devices... 3 USB Device(s) found
scanning usb for storage devices... 0 Storage Device(s) found
scanning usb for ethernet devices... 1 Ethernet Device(s) found
Waiting for Ethernet connection... done.
BOOTP broadcast 1
BOOTP broadcast 2
BOOTP broadcast 3
BOOTP broadcast 4
BOOTP broadcast 5
BOOTP broadcast 6
BOOTP broadcast 7
BOOTP broadcast 8
Retry time exceeded; starting again
Bad Linux ARM zImage magic!

Obtains a lease, but stalls on transfer:

 starting USB...
USB0: Core Release: 2.80a
scanning bus 0 for devices... 3 USB Device(s) found
scanning usb for storage devices... 0 Storage Device(s) found
scanning usb for ethernet devices... 1 Ethernet Device(s) found
Waiting for Ethernet connection... done.
BOOTP broadcast 1
DHCP client bound to address 192.168.2.55 (1328 ms)
Waiting for Ethernet connection... done.
Using sms0 device
TFTP from server 192.168.2.2; our IP address is 192.168.2.55
Filename 'tmp2rkX_N/.zImage'.
Load address: 0x1000000
Loading: * T T T T T T T T T T
Retry count exceeded; starting again
Bad Linux ARM zImage magic!

At this point I reached out for help on IRC and that is when Marek and
I starting chatting about this. Hacking around, I found that using
v2015.01 I was able to obtain a lease and transfer files 100% of the
time (although it seems very slow). Thinking that we have a good and
bad commit, this would be easy to bisect. Wrong, we both tried and got
different results. It seems that as you move from v2015.01 -> HEAD
master this issue becomes very intermittent and thus hard to pin down.

So my test case for this issue has become...

* Obtain lease
* Transfer kernel, dtb, ramdisk without stalling/timing out
* Do this 10 times in a row with a power cycle in between

Hope this help clarify the situation in some way,

Tyler

[0] https://www.raspberrypi.org/downloads/

[U-Boot] [PATCH 4/4] ARM: bcm283x: Switch to generic timer

[Stephen Warren]

On 05/06/2015 12:13 PM, Marek Vasut wrote:
> On Wednesday, May 06, 2015 at 05:52:37 PM, Stephen Warren wrote:
> [...]
>>>>> So, if now is close to 0x7fffffff (which it can), then if endtime is
>>>>> big-ish, diff will become negative and this udelay() will not perform
>>>>> the correct delay, right ?
>>>>
>>>> I don't believe so, no.
>>>>
>>>> endtime and now are both unsigned. My (admittedly intuitive rather than
>>>> well-researched) understanding of C math promotion rules means that
>>>> "endtime - now" will be calculated as an unsigned value, then converted
>>>> into a signed value to be stored in the signed diff. As such, I would
>>>> expect the value of diff to be a small value in this case. I wrote a
>>>> test program to validate this; endtime = 0x80000002, now = 0x7ffffffe,
>>>> yields diff=4 as expected.
>>>>
>>>> Perhaps you meant a much larger endtime value than 0x80000002; perhaps
>>>> 0xffffffff? This doesn't cause issues either. All that's relevant is the
>>>> difference between endtime and now, not their absolute values, and not
>>>> whether endtime has wrapped but now has or hasn't. For example, endtime
>>>> = 0x00000002, now = 0xfffffff0 yields diff=18 as expected.
>>>
>>> So what if the difference is bigger than 1 << 31 ?
>>
>> As I said, I don't believe that case is relevant; it can only happen if
>> passing ridiculously large delay values into __udelay() (i.e. greater
>> than the 1<<31value you mention), and I don't believe there's any need
>> to support that.
>
> So what you say is that it's OK to have a function which is buggy in
> corner cases ?

A corner case (something that's within spec but perhaps hard/unusual) 
should not be buggy.

The behaviour of something outside spec isn't relevant; it's actively 
not specified.

I suppose there is no specification of what range of values this 
function is supposed to accept. I'd argue we should create one, and that 
spec should likely limit the range to much less than the 32-bit 
parameter can actually hold, since some HW timer implementations may 
have well less than 32-bits of range.

[U-Boot] [PATCH 4/4] ARM: bcm283x: Switch to generic timer

[Stephen Warren]

On 05/06/2015 01:51 PM, Tyler Baker wrote:
> On 6 May 2015 at 11:13, Marek Vasut <marex@denx.de> wrote:
>> On Wednesday, May 06, 2015 at 05:52:37 PM, Stephen Warren wrote:
>> [...]
>>>>>> So, if now is close to 0x7fffffff (which it can), then if endtime is
>>>>>> big-ish, diff will become negative and this udelay() will not perform
>>>>>> the correct delay, right ?
>>>>>
>>>>> I don't believe so, no.
>>>>>
>>>>> endtime and now are both unsigned. My (admittedly intuitive rather than
>>>>> well-researched) understanding of C math promotion rules means that
>>>>> "endtime - now" will be calculated as an unsigned value, then converted
>>>>> into a signed value to be stored in the signed diff. As such, I would
>>>>> expect the value of diff to be a small value in this case. I wrote a
>>>>> test program to validate this; endtime = 0x80000002, now = 0x7ffffffe,
>>>>> yields diff=4 as expected.
>>>>>
>>>>> Perhaps you meant a much larger endtime value than 0x80000002; perhaps
>>>>> 0xffffffff? This doesn't cause issues either. All that's relevant is the
>>>>> difference between endtime and now, not their absolute values, and not
>>>>> whether endtime has wrapped but now has or hasn't. For example, endtime
>>>>> = 0x00000002, now = 0xfffffff0 yields diff=18 as expected.
>>>>
>>>> So what if the difference is bigger than 1 << 31 ?
>>>
>>> As I said, I don't believe that case is relevant; it can only happen if
>>> passing ridiculously large delay values into __udelay() (i.e. greater
>>> than the 1<<31value you mention), and I don't believe there's any need
>>> to support that.
>>
>> So what you say is that it's OK to have a function which is buggy in
>> corner cases ?
>>
>>> The implementation in lib/time.c probably has exactly the same problem,
>>> except that since it uses 64-bit math rather than 32-bit math, so the
>>> issue happens at 1<<63 rather than 1<<31. It's probably equally
>>> problematic for delay values as large as 1<<63:-) In practice, given
>>> 1<<31 us is so large, I don't think there's any practical difference.
>>
>> The implementation in lib/time.c uses 32bit usec argument though, so
>> it's not prone to this overflow. Please correct me if I'm wrong.
>>
>>>>>>> Besides, what's passing a value >~36 minutes to udelay()?
>>>>>>
>>>>>> Nothing, but that doesn't mean we can have a possibly broken
>>>>>> implementation, right ?
>>>>>
>>>>> True. However, I'd expect that any specification for udelay would
>>>>> disallow such large parameter values, and hence its behaviour wouldn't
>>>>> be relevant if such values were passed.
>>>>
>>>> Do you think you can pick this patch and drop the "fixes overflow" part
>>>> or do you need resubmission ?
>>>
>>> Tom Rini (or in the past Albert Aribaud) actually apply the patches.
>>>
>>> Re: the patch description: I'd certainly be happy if it was re-written
>>> to say something more like "replace bcm2835-specific timer logic with
>>> common code to reduce the number of different implementations for the
>>> same thing".
>>
>> Tom, do you want a repost ?
>>
>>> I think you'd mentioned on IRC that this change fixed something
>>> USB-related for you, and I still don't understand how that could be
>>> possible. Perhaps there's some intermittent problem, and it just
>>> happened not to show up when you tested after this patch?
>>
>> I think Tyler can elaborate on that, but in his test case, he still
>> triggers the USB issue.
>
> I'll provide some context on the issue I'm fighting...
>
> I recently bought a RPi B+ Model, flashed the latest raspbian image[0]
> to an sd card, built the master branch (v2015.05+) u-boot and
> overwrote kernel.img with u-boot.bin. U-Boot came right up but I was
> unable to obtain a DHCP lease after using 'usb start; dhcp'. I ran
> tcpdump and saw the DHCP requests being made, but shortly after the
> board was seemly ignoring responses from the DHCP server. Now
> sometimes if the response from the DHCP server was quick enough, it
> could get a lease, but the tftp transfer would stall. I decided it
> would be best to set 'ipaddr, gateway, netmask' to see if this was a
> DHCP issue. It turned out that I had no network connectivity even when
> I configured the ip address statically.
>
> Could not obtain a lease:
>
>   starting USB...
> USB0: Core Release: 2.80a
> scanning bus 0 for devices... 3 USB Device(s) found
> scanning usb for storage devices... 0 Storage Device(s) found
> scanning usb for ethernet devices... 1 Ethernet Device(s) found
> Waiting for Ethernet connection... done.
> BOOTP broadcast 1
> BOOTP broadcast 2
> BOOTP broadcast 3
> BOOTP broadcast 4
> BOOTP broadcast 5
> BOOTP broadcast 6
> BOOTP broadcast 7
> BOOTP broadcast 8
> Retry time exceeded; starting again
> Bad Linux ARM zImage magic!
>
> Obtains a lease, but stalls on transfer:
>
>   starting USB...
> USB0: Core Release: 2.80a
> scanning bus 0 for devices... 3 USB Device(s) found
> scanning usb for storage devices... 0 Storage Device(s) found
> scanning usb for ethernet devices... 1 Ethernet Device(s) found
> Waiting for Ethernet connection... done.
> BOOTP broadcast 1
> DHCP client bound to address 192.168.2.55 (1328 ms)
> Waiting for Ethernet connection... done.
> Using sms0 device
> TFTP from server 192.168.2.2; our IP address is 192.168.2.55
> Filename 'tmp2rkX_N/.zImage'.
> Load address: 0x1000000
> Loading: * T T T T T T T T T T
> Retry count exceeded; starting again
> Bad Linux ARM zImage magic!
>
> At this point I reached out for help on IRC and that is when Marek and
> I starting chatting about this. Hacking around, I found that using
> v2015.01 I was able to obtain a lease and transfer files 100% of the
> time (although it seems very slow). Thinking that we have a good and
> bad commit, this would be easy to bisect. Wrong, we both tried and got
> different results. It seems that as you move from v2015.01 -> HEAD
> master this issue becomes very intermittent and thus hard to pin down.
>
> So my test case for this issue has become...
>
> * Obtain lease
> * Transfer kernel, dtb, ramdisk without stalling/timing out
> * Do this 10 times in a row with a power cycle in between
>
> Hope this help clarify the situation in some way,

OK, but if you apply Marek's change to replace the timer implementation 
with the one in lib/time.c, does that reliably fix the issue when tested 
over a large number of runs with/without that change? With the current 
explanation, I can't see how it possibly could. Equally, I can't see why 
the move from v2015.01 to HEAD would affect the issue if it was caused 
by the timer implementation.

My suspicion is that there's something else entirely going on.

[U-Boot] [PATCH 4/4] ARM: bcm283x: Switch to generic timer

[Marek Vasut]

On Friday, May 08, 2015 at 06:06:41 PM, Stephen Warren wrote:
[...]

> > * Obtain lease
> > * Transfer kernel, dtb, ramdisk without stalling/timing out
> > * Do this 10 times in a row with a power cycle in between
> > 
> > Hope this help clarify the situation in some way,
> 
> OK, but if you apply Marek's change to replace the timer implementation
> with the one in lib/time.c, does that reliably fix the issue when tested
> over a large number of runs with/without that change? With the current
> explanation, I can't see how it possibly could. Equally, I can't see why
> the move from v2015.01 to HEAD would affect the issue if it was caused
> by the timer implementation.
> 
> My suspicion is that there's something else entirely going on.

It doesn't , there's something else going on.

Best regards,
Marek Vasut

[U-Boot] [PATCH 4/4] ARM: bcm283x: Switch to generic timer

[Marek Vasut]

On Friday, May 08, 2015 at 06:03:34 PM, Stephen Warren wrote:
> On 05/06/2015 12:13 PM, Marek Vasut wrote:
> > On Wednesday, May 06, 2015 at 05:52:37 PM, Stephen Warren wrote:
> > [...]
> > 
> >>>>> So, if now is close to 0x7fffffff (which it can), then if endtime is
> >>>>> big-ish, diff will become negative and this udelay() will not perform
> >>>>> the correct delay, right ?
> >>>> 
> >>>> I don't believe so, no.
> >>>> 
> >>>> endtime and now are both unsigned. My (admittedly intuitive rather
> >>>> than well-researched) understanding of C math promotion rules means
> >>>> that "endtime - now" will be calculated as an unsigned value, then
> >>>> converted into a signed value to be stored in the signed diff. As
> >>>> such, I would expect the value of diff to be a small value in this
> >>>> case. I wrote a test program to validate this; endtime = 0x80000002,
> >>>> now = 0x7ffffffe, yields diff=4 as expected.
> >>>> 
> >>>> Perhaps you meant a much larger endtime value than 0x80000002; perhaps
> >>>> 0xffffffff? This doesn't cause issues either. All that's relevant is
> >>>> the difference between endtime and now, not their absolute values,
> >>>> and not whether endtime has wrapped but now has or hasn't. For
> >>>> example, endtime = 0x00000002, now = 0xfffffff0 yields diff=18 as
> >>>> expected.
> >>> 
> >>> So what if the difference is bigger than 1 << 31 ?
> >> 
> >> As I said, I don't believe that case is relevant; it can only happen if
> >> passing ridiculously large delay values into __udelay() (i.e. greater
> >> than the 1<<31value you mention), and I don't believe there's any need
> >> to support that.
> > 
> > So what you say is that it's OK to have a function which is buggy in
> > corner cases ?
> 
> A corner case (something that's within spec but perhaps hard/unusual)
> should not be buggy.
> 
> The behaviour of something outside spec isn't relevant; it's actively
> not specified.
> 
> I suppose there is no specification of what range of values this
> function is supposed to accept. I'd argue we should create one, and that
> spec should likely limit the range to much less than the 32-bit
> parameter can actually hold, since some HW timer implementations may
> have well less than 32-bits of range.

Maybe we should just accept this patch and be done with it? It's clearly
and improvement which migrates away from old timer code to generic timer.

Best regards,
Marek Vasut

[U-Boot] [PATCH 4/4] ARM: bcm283x: Switch to generic timer

[Stephen Warren]

On 05/08/2015 10:31 AM, Marek Vasut wrote:
> On Friday, May 08, 2015 at 06:03:34 PM, Stephen Warren wrote:
>> On 05/06/2015 12:13 PM, Marek Vasut wrote:
>>> On Wednesday, May 06, 2015 at 05:52:37 PM, Stephen Warren wrote:
>>> [...]
>>>
>>>>>>> So, if now is close to 0x7fffffff (which it can), then if endtime is
>>>>>>> big-ish, diff will become negative and this udelay() will not perform
>>>>>>> the correct delay, right ?
>>>>>>
>>>>>> I don't believe so, no.
>>>>>>
>>>>>> endtime and now are both unsigned. My (admittedly intuitive rather
>>>>>> than well-researched) understanding of C math promotion rules means
>>>>>> that "endtime - now" will be calculated as an unsigned value, then
>>>>>> converted into a signed value to be stored in the signed diff. As
>>>>>> such, I would expect the value of diff to be a small value in this
>>>>>> case. I wrote a test program to validate this; endtime = 0x80000002,
>>>>>> now = 0x7ffffffe, yields diff=4 as expected.
>>>>>>
>>>>>> Perhaps you meant a much larger endtime value than 0x80000002; perhaps
>>>>>> 0xffffffff? This doesn't cause issues either. All that's relevant is
>>>>>> the difference between endtime and now, not their absolute values,
>>>>>> and not whether endtime has wrapped but now has or hasn't. For
>>>>>> example, endtime = 0x00000002, now = 0xfffffff0 yields diff=18 as
>>>>>> expected.
>>>>>
>>>>> So what if the difference is bigger than 1 << 31 ?
>>>>
>>>> As I said, I don't believe that case is relevant; it can only happen if
>>>> passing ridiculously large delay values into __udelay() (i.e. greater
>>>> than the 1<<31value you mention), and I don't believe there's any need
>>>> to support that.
>>>
>>> So what you say is that it's OK to have a function which is buggy in
>>> corner cases ?
>>
>> A corner case (something that's within spec but perhaps hard/unusual)
>> should not be buggy.
>>
>> The behaviour of something outside spec isn't relevant; it's actively
>> not specified.
>>
>> I suppose there is no specification of what range of values this
>> function is supposed to accept. I'd argue we should create one, and that
>> spec should likely limit the range to much less than the 32-bit
>> parameter can actually hold, since some HW timer implementations may
>> have well less than 32-bits of range.
>
> Maybe we should just accept this patch and be done with it? It's clearly
> and improvement which migrates away from old timer code to generic timer.

The code change is fine. I have no issues with that.

I just don't think the patch description is appropriate, since the 
version in lib/time.c has exactly the same overflow issue (albeit with a 
64-bit type rather than a 32-bit type).

[U-Boot] [PATCH 4/4] ARM: bcm283x: Switch to generic timer

[Marek Vasut]

On Friday, May 08, 2015 at 06:40:22 PM, Stephen Warren wrote:
> On 05/08/2015 10:31 AM, Marek Vasut wrote:
> > On Friday, May 08, 2015 at 06:03:34 PM, Stephen Warren wrote:
> >> On 05/06/2015 12:13 PM, Marek Vasut wrote:
> >>> On Wednesday, May 06, 2015 at 05:52:37 PM, Stephen Warren wrote:
> >>> [...]
> >>> 
> >>>>>>> So, if now is close to 0x7fffffff (which it can), then if endtime
> >>>>>>> is big-ish, diff will become negative and this udelay() will not
> >>>>>>> perform the correct delay, right ?
> >>>>>> 
> >>>>>> I don't believe so, no.
> >>>>>> 
> >>>>>> endtime and now are both unsigned. My (admittedly intuitive rather
> >>>>>> than well-researched) understanding of C math promotion rules means
> >>>>>> that "endtime - now" will be calculated as an unsigned value, then
> >>>>>> converted into a signed value to be stored in the signed diff. As
> >>>>>> such, I would expect the value of diff to be a small value in this
> >>>>>> case. I wrote a test program to validate this; endtime = 0x80000002,
> >>>>>> now = 0x7ffffffe, yields diff=4 as expected.
> >>>>>> 
> >>>>>> Perhaps you meant a much larger endtime value than 0x80000002;
> >>>>>> perhaps 0xffffffff? This doesn't cause issues either. All that's
> >>>>>> relevant is the difference between endtime and now, not their
> >>>>>> absolute values, and not whether endtime has wrapped but now has or
> >>>>>> hasn't. For example, endtime = 0x00000002, now = 0xfffffff0 yields
> >>>>>> diff=18 as expected.
> >>>>> 
> >>>>> So what if the difference is bigger than 1 << 31 ?
> >>>> 
> >>>> As I said, I don't believe that case is relevant; it can only happen
> >>>> if passing ridiculously large delay values into __udelay() (i.e.
> >>>> greater than the 1<<31value you mention), and I don't believe there's
> >>>> any need to support that.
> >>> 
> >>> So what you say is that it's OK to have a function which is buggy in
> >>> corner cases ?
> >> 
> >> A corner case (something that's within spec but perhaps hard/unusual)
> >> should not be buggy.
> >> 
> >> The behaviour of something outside spec isn't relevant; it's actively
> >> not specified.
> >> 
> >> I suppose there is no specification of what range of values this
> >> function is supposed to accept. I'd argue we should create one, and that
> >> spec should likely limit the range to much less than the 32-bit
> >> parameter can actually hold, since some HW timer implementations may
> >> have well less than 32-bits of range.
> > 
> > Maybe we should just accept this patch and be done with it? It's clearly
> > and improvement which migrates away from old timer code to generic timer.
> 
> The code change is fine. I have no issues with that.
> 
> I just don't think the patch description is appropriate, since the
> version in lib/time.c has exactly the same overflow issue (albeit with a
> 64-bit type rather than a 32-bit type).

Feel free to tweak the commit message.

Best regards,
Marek Vasut

[U-Boot] [U-Boot,1/4] ARM: bcm283x: Repair wdog.h

[Tom Rini]

On Mon, May 04, 2015 at 10:54:34PM +0200, Marek Vasut wrote:

> Trivially fix the include check in wdog.h.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Stephen Warren <swarren@wwwdotorg.org>
> Cc: Tyler Baker <tyler.baker@linaro.org>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150528/3e275cc2/attachment.sig>

[U-Boot] [U-Boot,2/4] ARM: bcm283x: Reorder timer.h

[Tom Rini]

On Mon, May 04, 2015 at 10:54:35PM +0200, Marek Vasut wrote:

> Reorder the timer.h file so it can be included from board config file.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Stephen Warren <swarren@wwwdotorg.org>
> Cc: Tyler Baker <tyler.baker@linaro.org>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150528/a1f0d0cf/attachment.sig>

[U-Boot] [U-Boot,4/4] ARM: bcm283x: Switch to generic timer

[Tom Rini]

On Mon, May 04, 2015 at 10:54:37PM +0200, Marek Vasut wrote:

> Switch to generic timer implementation from lib/time.c .
> This also fixes a signed overflow which was in __udelay()
> implementation.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Stephen Warren <swarren@wwwdotorg.org>
> Cc: Tyler Baker <tyler.baker@linaro.org>
> Acked-by: Stephen Warren <swarren@wwwdotorg.org>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150528/66442811/attachment.sig>

[U-Boot] [PATCH 3/4] ARM: mmc: bcm283x: Remove get_timer_us() from mmc driver

[Stephen Warren]

On 05/04/2015 02:54 PM, Marek Vasut wrote:
> The get_timer_us() function is something which is no longer
> existing in case we use generic timer framework, so replace
> it with get_timer().

Marek,

This patch causes "saveenv" to got from almost no time to nearly 50s on
my RPi model A+. Can you take a look at that please?

[U-Boot] [PATCH 3/4] ARM: mmc: bcm283x: Remove get_timer_us() from mmc driver

[Marek Vasut]

On Tuesday, June 16, 2015 at 05:44:06 AM, Stephen Warren wrote:
> On 05/04/2015 02:54 PM, Marek Vasut wrote:
> > The get_timer_us() function is something which is no longer
> > existing in case we use generic timer framework, so replace
> > it with get_timer().
> 
> Marek,
> 
> This patch causes "saveenv" to got from almost no time to nearly 50s on
> my RPi model A+. Can you take a look at that please?

Can you try the attached diff ? ;-/ I think I mistakenly used get_timer(),
which returns msecs instead of timer_get_us() which reports usecs, sorry.

Best regards,
Marek Vasut
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sdmmc.diff
Type: text/x-patch
Size: 706 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150617/97f9d398/attachment.bin>

[U-Boot] [PATCH 3/4] ARM: mmc: bcm283x: Remove get_timer_us() from mmc driver

[Jakub Kiciński]

On Wed, 17 Jun 2015 12:44:15 +0200, Marek Vasut wrote:
> On Tuesday, June 16, 2015 at 05:44:06 AM, Stephen Warren wrote:
> > On 05/04/2015 02:54 PM, Marek Vasut wrote:
> > > The get_timer_us() function is something which is no longer
> > > existing in case we use generic timer framework, so replace
> > > it with get_timer().
> > 
> > Marek,
> > 
> > This patch causes "saveenv" to got from almost no time to nearly 50s on
> > my RPi model A+. Can you take a look at that please?
> 
> Can you try the attached diff ? ;-/ I think I mistakenly used get_timer(),
> which returns msecs instead of timer_get_us() which reports usecs, sorry.

I can confirm this solves the regression.

[U-Boot] [PATCH 3/4] ARM: mmc: bcm283x: Remove get_timer_us() from mmc driver

[Stephen Warren]

On 06/17/2015 04:44 AM, Marek Vasut wrote:
> On Tuesday, June 16, 2015 at 05:44:06 AM, Stephen Warren wrote:
>> On 05/04/2015 02:54 PM, Marek Vasut wrote:
>>> The get_timer_us() function is something which is no longer
>>> existing in case we use generic timer framework, so replace
>>> it with get_timer().
>>
>> Marek,
>>
>> This patch causes "saveenv" to got from almost no time to nearly 50s on
>> my RPi model A+. Can you take a look at that please?
> 
> Can you try the attached diff ? ;-/ I think I mistakenly used get_timer(),
> which returns msecs instead of timer_get_us() which reports usecs, sorry.

Thanks, that fixes it.

Tested-by: Stephen Warren <swarren@wwwdotorg.org>

[U-Boot] [PATCH 3/4] ARM: mmc: bcm283x: Remove get_timer_us() from mmc driver

[Marek Vasut]

On Wednesday, June 17, 2015 at 06:13:03 PM, Jakub Kici?ski wrote:
> On Wed, 17 Jun 2015 12:44:15 +0200, Marek Vasut wrote:
> > On Tuesday, June 16, 2015 at 05:44:06 AM, Stephen Warren wrote:
> > > On 05/04/2015 02:54 PM, Marek Vasut wrote:
> > > > The get_timer_us() function is something which is no longer
> > > > existing in case we use generic timer framework, so replace
> > > > it with get_timer().
> > > 
> > > Marek,
> > > 
> > > This patch causes "saveenv" to got from almost no time to nearly 50s on
> > > my RPi model A+. Can you take a look at that please?
> > 
> > Can you try the attached diff ? ;-/ I think I mistakenly used
> > get_timer(), which returns msecs instead of timer_get_us() which reports
> > usecs, sorry.
> 
> I can confirm this solves the regression.

That's not a regression, that was a bug ;-)
Thanks for testing :)

Best regards,
Marek Vasut

[U-Boot] [PATCH 3/4] ARM: mmc: bcm283x: Remove get_timer_us() from mmc driver

[Jakub Kiciński]

On Thu, 18 Jun 2015 14:35:27 +0200, Marek Vasut wrote:
> On Wednesday, June 17, 2015 at 06:13:03 PM, Jakub Kici?ski wrote:
> > On Wed, 17 Jun 2015 12:44:15 +0200, Marek Vasut wrote:
> > > On Tuesday, June 16, 2015 at 05:44:06 AM, Stephen Warren wrote:
> > > > On 05/04/2015 02:54 PM, Marek Vasut wrote:
> > > > > The get_timer_us() function is something which is no longer
> > > > > existing in case we use generic timer framework, so replace
> > > > > it with get_timer().
> > > > 
> > > > Marek,
> > > > 
> > > > This patch causes "saveenv" to got from almost no time to nearly 50s on
> > > > my RPi model A+. Can you take a look at that please?
> > > 
> > > Can you try the attached diff ? ;-/ I think I mistakenly used
> > > get_timer(), which returns msecs instead of timer_get_us() which reports
> > > usecs, sorry.
> > 
> > I can confirm this solves the regression.
> 
> That's not a regression, that was a bug ;-)

Ach OK, I thought Stephen said that MMC used to be fast before the
offending patch ;)  Anyway I don't really know what's the difference
and wikipedia says regression is a bug too so we may both be right :P

[U-Boot] [PATCH 3/4] ARM: mmc: bcm283x: Remove get_timer_us() from mmc driver

[Marek Vasut]

On Thursday, June 18, 2015 at 02:51:18 PM, Jakub Kici?ski wrote:
> On Thu, 18 Jun 2015 14:35:27 +0200, Marek Vasut wrote:
> > On Wednesday, June 17, 2015 at 06:13:03 PM, Jakub Kici?ski wrote:
> > > On Wed, 17 Jun 2015 12:44:15 +0200, Marek Vasut wrote:
> > > > On Tuesday, June 16, 2015 at 05:44:06 AM, Stephen Warren wrote:
> > > > > On 05/04/2015 02:54 PM, Marek Vasut wrote:
> > > > > > The get_timer_us() function is something which is no longer
> > > > > > existing in case we use generic timer framework, so replace
> > > > > > it with get_timer().
> > > > > 
> > > > > Marek,
> > > > > 
> > > > > This patch causes "saveenv" to got from almost no time to nearly
> > > > > 50s on my RPi model A+. Can you take a look at that please?
> > > > 
> > > > Can you try the attached diff ? ;-/ I think I mistakenly used
> > > > get_timer(), which returns msecs instead of timer_get_us() which
> > > > reports usecs, sorry.
> > > 
> > > I can confirm this solves the regression.
> > 
> > That's not a regression, that was a bug ;-)
> 
> Ach OK, I thought Stephen said that MMC used to be fast before the
> offending patch ;)  Anyway I don't really know what's the difference
> and wikipedia says regression is a bug too so we may both be right :P

I always considered it to be [1] 2c , maybe I'm wrong though ;-)
Just recently I thought I finally figured out the difference too,
but now you surely made me unsure again :)

"
2 :  a trend or shift toward a lower or less perfect state: as 
  c :  reversion to an earlier mental or behavioral level 
"

[1] http://www.merriam-webster.com/dictionary/regression

Best regards,
Marek Vasut