[U-Boot] [PATCH 1/3] dm: timer: refuse timers with zero clock_rate

[U-Boot] [PATCH 1/3] dm: timer: refuse timers with zero clock_rate

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

If a timer has a zero clock_rate, get_tbclk() will return zero for it,
which will cause tick_to_time() to perform a division-by-zero, which will
crash U-Boot.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 drivers/timer/timer-uclass.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/timer/timer-uclass.c b/drivers/timer/timer-uclass.c
index aca421bdea33..0771562c600d 100644
--- a/drivers/timer/timer-uclass.c
+++ b/drivers/timer/timer-uclass.c
@@ -47,6 +47,16 @@ static int timer_pre_probe(struct udevice *dev)
 	return 0;
 }
 
+static int timer_post_probe(struct udevice *dev)
+{
+	struct timer_dev_priv *uc_priv = dev_get_uclass_priv(dev);
+
+	if (!uc_priv->clock_rate)
+		return -EINVAL;
+
+	return 0;
+}
+
 u64 timer_conv_64(u32 count)
 {
 	/* increment tbh if tbl has rolled over */
@@ -60,5 +70,6 @@ UCLASS_DRIVER(timer) = {
 	.id		= UCLASS_TIMER,
 	.name		= "timer",
 	.pre_probe	= timer_pre_probe,
+	.post_probe	= timer_post_probe,
 	.per_device_auto_alloc_size = sizeof(struct timer_dev_priv),
 };
-- 
2.6.4

[U-Boot] [PATCH 2/3] timer: sandbox: work without device tree

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

A default invocation of sandbox U-Boot apparently uses no device tree,
which means that no timer is registers, which in turn means that the
sleep shell command hangs.

Fix the sandbox timer code to register a device when there's no DT, just
like e.g. the sandbox reset driver does. When there's no DT, the DM uclass
can't initialize clock_rate from DT, so set a default value in the
timer code instead.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 drivers/timer/sandbox_timer.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/timer/sandbox_timer.c b/drivers/timer/sandbox_timer.c
index 00a9944f78e6..a8da93634949 100644
--- a/drivers/timer/sandbox_timer.c
+++ b/drivers/timer/sandbox_timer.c
@@ -27,6 +27,11 @@ static int sandbox_timer_get_count(struct udevice *dev, u64 *count)
 
 static int sandbox_timer_probe(struct udevice *dev)
 {
+	struct timer_dev_priv *uc_priv = dev_get_uclass_priv(dev);
+
+	if (!uc_priv->clock_rate)
+		uc_priv->clock_rate = 1000000;
+
 	return 0;
 }
 
@@ -47,3 +52,8 @@ U_BOOT_DRIVER(sandbox_timer) = {
 	.ops	= &sandbox_timer_ops,
 	.flags = DM_FLAG_PRE_RELOC,
 };
+
+/* This is here in case we don't have a device tree */
+U_BOOT_DEVICE(sandbox_timer_non_fdt) = {
+	.name = "sandbox_timer",
+};
-- 
2.6.4

[U-Boot] [PATCH 2/3] timer: sandbox: work without device tree

[Bin Meng]

On Thu, Jan 7, 2016 at 1:33 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> From: Stephen Warren <swarren@nvidia.com>
>
> A default invocation of sandbox U-Boot apparently uses no device tree,
> which means that no timer is registers, which in turn means that the
> sleep shell command hangs.
>
> Fix the sandbox timer code to register a device when there's no DT, just
> like e.g. the sandbox reset driver does. When there's no DT, the DM uclass
> can't initialize clock_rate from DT, so set a default value in the
> timer code instead.
>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
>  drivers/timer/sandbox_timer.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[U-Boot] [PATCH 2/3] timer: sandbox: work without device tree

[Simon Glass]

On 6 January 2016 at 10:33, Stephen Warren <swarren@wwwdotorg.org> wrote:
> From: Stephen Warren <swarren@nvidia.com>
>
> A default invocation of sandbox U-Boot apparently uses no device tree,
> which means that no timer is registers, which in turn means that the
> sleep shell command hangs.
>
> Fix the sandbox timer code to register a device when there's no DT, just
> like e.g. the sandbox reset driver does. When there's no DT, the DM uclass
> can't initialize clock_rate from DT, so set a default value in the
> timer code instead.
>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
>  drivers/timer/sandbox_timer.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)

Acked-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH 2/3] timer: sandbox: work without device tree

[Simon Glass]

On 11 January 2016 at 09:58, Simon Glass <sjg@chromium.org> wrote:
> On 6 January 2016 at 10:33, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> A default invocation of sandbox U-Boot apparently uses no device tree,
>> which means that no timer is registers, which in turn means that the
>> sleep shell command hangs.
>>
>> Fix the sandbox timer code to register a device when there's no DT, just
>> like e.g. the sandbox reset driver does. When there's no DT, the DM uclass
>> can't initialize clock_rate from DT, so set a default value in the
>> timer code instead.
>>
>> Signed-off-by: Stephen Warren <swarren@nvidia.com>
>> ---
>>  drivers/timer/sandbox_timer.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>
> Acked-by: Simon Glass <sjg@chromium.org>

Applied to u-boot-dm, thanks!

[U-Boot] [PATCH 3/3] test/py: add a test for the sleep command

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

Execute "sleep", and validate that it sleeps for approximately the correct
amount of time.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
This patch depends on both the two previous patches, and on my unrelated
series which adds the test/py code. If I resend that series, I can add
this patch into it.
---
 test/py/tests/test_sleep.py | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)
 create mode 100644 test/py/tests/test_sleep.py

diff --git a/test/py/tests/test_sleep.py b/test/py/tests/test_sleep.py
new file mode 100644
index 000000000000..64f1ddf9a09f
--- /dev/null
+++ b/test/py/tests/test_sleep.py
@@ -0,0 +1,24 @@
+# Copyright (c) 2016, NVIDIA CORPORATION. All rights reserved.
+#
+# SPDX-License-Identifier: GPL-2.0
+
+import pytest
+import time
+
+def test_sleep(u_boot_console):
+    '''Test the sleep command, and validate that it sleeps for approximately
+    the correct amount of time.'''
+
+    # Do this before we time anything, to make sure U-Boot is already running.
+    # Otherwise, the system boot time is included in the time measurement.
+    u_boot_console.ensure_spawned()
+
+    # 3s isn't too long, but is enough to cross a few second boundaries.
+    sleep_time = 3
+    tstart = time.time()
+    u_boot_console.run_command('sleep %d' % sleep_time)
+    tend = time.time()
+    elapsed = tend - tstart
+    delta_to_expected = abs(elapsed - sleep_time)
+    # 0.25s margin is hopefully enough to account for any system overhead.
+    assert delta_to_expected < 0.25
-- 
2.6.4

[U-Boot] [PATCH 3/3] test/py: add a test for the sleep command

[Simon Glass]

On 6 January 2016 at 10:33, Stephen Warren <swarren@wwwdotorg.org> wrote:
> From: Stephen Warren <swarren@nvidia.com>
>
> Execute "sleep", and validate that it sleeps for approximately the correct
> amount of time.
>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
> This patch depends on both the two previous patches, and on my unrelated
> series which adds the test/py code. If I resend that series, I can add
> this patch into it.
> ---
>  test/py/tests/test_sleep.py | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
>  create mode 100644 test/py/tests/test_sleep.py

Acked-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH 3/3] test/py: add a test for the sleep command

[Stephen Warren]

On 01/06/2016 10:33 AM, Stephen Warren wrote:
> From: Stephen Warren <swarren@nvidia.com>
>
> Execute "sleep", and validate that it sleeps for approximately the correct
> amount of time.

FYI, I've reposted the series that implements the test/py code, and 
bundled this patch 3/3 in this series into that series.

As such, I believe patches 1/3 and 2/3 in this series can be applied 
through whatever path sandbox (or DM or timer) patches would normally go 
through.

[U-Boot] [PATCH 3/3] test/py: add a test for the sleep command

[Simon Glass]

Hi Stephen,

On 15 January 2016 at 11:24, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 01/06/2016 10:33 AM, Stephen Warren wrote:
>>
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> Execute "sleep", and validate that it sleeps for approximately the correct
>> amount of time.
>
>
> FYI, I've reposted the series that implements the test/py code, and bundled
> this patch 3/3 in this series into that series.
>
> As such, I believe patches 1/3 and 2/3 in this series can be applied through
> whatever path sandbox (or DM or timer) patches would normally go through.

I've applied them to u-boot-dm.

Regards,
Simon

[U-Boot] [PATCH 1/3] dm: timer: refuse timers with zero clock_rate

[Bin Meng]

On Thu, Jan 7, 2016 at 1:33 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> From: Stephen Warren <swarren@nvidia.com>
>
> If a timer has a zero clock_rate, get_tbclk() will return zero for it,
> which will cause tick_to_time() to perform a division-by-zero, which will
> crash U-Boot.
>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
>  drivers/timer/timer-uclass.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/timer/timer-uclass.c b/drivers/timer/timer-uclass.c
> index aca421bdea33..0771562c600d 100644
> --- a/drivers/timer/timer-uclass.c
> +++ b/drivers/timer/timer-uclass.c
> @@ -47,6 +47,16 @@ static int timer_pre_probe(struct udevice *dev)
>         return 0;
>  }
>
> +static int timer_post_probe(struct udevice *dev)
> +{
> +       struct timer_dev_priv *uc_priv = dev_get_uclass_priv(dev);
> +
> +       if (!uc_priv->clock_rate)
> +               return -EINVAL;

Should we just panic here?

> +
> +       return 0;
> +}
> +
>  u64 timer_conv_64(u32 count)
>  {
>         /* increment tbh if tbl has rolled over */
> @@ -60,5 +70,6 @@ UCLASS_DRIVER(timer) = {
>         .id             = UCLASS_TIMER,
>         .name           = "timer",
>         .pre_probe      = timer_pre_probe,
> +       .post_probe     = timer_post_probe,
>         .per_device_auto_alloc_size = sizeof(struct timer_dev_priv),
>  };
> --

Regards,
Bin

[U-Boot] [PATCH 1/3] dm: timer: refuse timers with zero clock_rate

[Stephen Warren]

On 01/06/2016 10:29 PM, Bin Meng wrote:
> On Thu, Jan 7, 2016 at 1:33 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> If a timer has a zero clock_rate, get_tbclk() will return zero for it,
>> which will cause tick_to_time() to perform a division-by-zero, which will
>> crash U-Boot.
>>
>> Signed-off-by: Stephen Warren <swarren@nvidia.com>
>> ---
>>   drivers/timer/timer-uclass.c | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/timer/timer-uclass.c b/drivers/timer/timer-uclass.c
>> index aca421bdea33..0771562c600d 100644
>> --- a/drivers/timer/timer-uclass.c
>> +++ b/drivers/timer/timer-uclass.c
>> @@ -47,6 +47,16 @@ static int timer_pre_probe(struct udevice *dev)
>>          return 0;
>>   }
>>
>> +static int timer_post_probe(struct udevice *dev)
>> +{
>> +       struct timer_dev_priv *uc_priv = dev_get_uclass_priv(dev);
>> +
>> +       if (!uc_priv->clock_rate)
>> +               return -EINVAL;
>
> Should we just panic here?

That would prevent the system operating correctly if multiple timers 
happened to be registered and the other one didn't have this issue. 
Still, it does seem reasonable to highlight this error. Simon, what do 
you think here?

[U-Boot] [PATCH 1/3] dm: timer: refuse timers with zero clock_rate

[Simon Glass]

Hi,

On 6 January 2016 at 22:29, Bin Meng <bmeng.cn@gmail.com> wrote:
> On Thu, Jan 7, 2016 at 1:33 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> If a timer has a zero clock_rate, get_tbclk() will return zero for it,
>> which will cause tick_to_time() to perform a division-by-zero, which will
>> crash U-Boot.
>>
>> Signed-off-by: Stephen Warren <swarren@nvidia.com>
>> ---
>>  drivers/timer/timer-uclass.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)

Acked-by: Simon Glass <sjg@chromium.org>

>>
>> diff --git a/drivers/timer/timer-uclass.c b/drivers/timer/timer-uclass.c
>> index aca421bdea33..0771562c600d 100644
>> --- a/drivers/timer/timer-uclass.c
>> +++ b/drivers/timer/timer-uclass.c
>> @@ -47,6 +47,16 @@ static int timer_pre_probe(struct udevice *dev)
>>         return 0;
>>  }
>>
>> +static int timer_post_probe(struct udevice *dev)
>> +{
>> +       struct timer_dev_priv *uc_priv = dev_get_uclass_priv(dev);
>> +
>> +       if (!uc_priv->clock_rate)
>> +               return -EINVAL;
>
> Should we just panic here?

I don't think so - there may be multiple timers, and this error should
be reported. If you like you could use a strange error number so it is
more obvious (e.g. -ETIME).

>
>> +
>> +       return 0;
>> +}
>> +
>>  u64 timer_conv_64(u32 count)
>>  {
>>         /* increment tbh if tbl has rolled over */
>> @@ -60,5 +70,6 @@ UCLASS_DRIVER(timer) = {
>>         .id             = UCLASS_TIMER,
>>         .name           = "timer",
>>         .pre_probe      = timer_pre_probe,
>> +       .post_probe     = timer_post_probe,
>>         .per_device_auto_alloc_size = sizeof(struct timer_dev_priv),
>>  };
>> --
>
> Regards,
> Bin

Regards,
Simon

[U-Boot] [PATCH 1/3] dm: timer: refuse timers with zero clock_rate

[Simon Glass]

On 11 January 2016 at 09:56, Simon Glass <sjg@chromium.org> wrote:
> Hi,
>
> On 6 January 2016 at 22:29, Bin Meng <bmeng.cn@gmail.com> wrote:
>> On Thu, Jan 7, 2016 at 1:33 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>> From: Stephen Warren <swarren@nvidia.com>
>>>
>>> If a timer has a zero clock_rate, get_tbclk() will return zero for it,
>>> which will cause tick_to_time() to perform a division-by-zero, which will
>>> crash U-Boot.
>>>
>>> Signed-off-by: Stephen Warren <swarren@nvidia.com>
>>> ---
>>>  drivers/timer/timer-uclass.c | 11 +++++++++++
>>>  1 file changed, 11 insertions(+)
>
> Acked-by: Simon Glass <sjg@chromium.org>
>
>>>
>>> diff --git a/drivers/timer/timer-uclass.c b/drivers/timer/timer-uclass.c
>>> index aca421bdea33..0771562c600d 100644
>>> --- a/drivers/timer/timer-uclass.c
>>> +++ b/drivers/timer/timer-uclass.c
>>> @@ -47,6 +47,16 @@ static int timer_pre_probe(struct udevice *dev)
>>>         return 0;
>>>  }
>>>
>>> +static int timer_post_probe(struct udevice *dev)
>>> +{
>>> +       struct timer_dev_priv *uc_priv = dev_get_uclass_priv(dev);
>>> +
>>> +       if (!uc_priv->clock_rate)
>>> +               return -EINVAL;
>>
>> Should we just panic here?
>
> I don't think so - there may be multiple timers, and this error should
> be reported. If you like you could use a strange error number so it is
> more obvious (e.g. -ETIME).
>
>>
>>> +
>>> +       return 0;
>>> +}
>>> +
>>>  u64 timer_conv_64(u32 count)
>>>  {
>>>         /* increment tbh if tbl has rolled over */
>>> @@ -60,5 +70,6 @@ UCLASS_DRIVER(timer) = {
>>>         .id             = UCLASS_TIMER,
>>>         .name           = "timer",
>>>         .pre_probe      = timer_pre_probe,
>>> +       .post_probe     = timer_post_probe,
>>>         .per_device_auto_alloc_size = sizeof(struct timer_dev_priv),
>>>  };
>>> --

Applied to u-boot-dm, thanks!