All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] time: Fix get_ticks being non-monotonic
@ 2020-09-09 20:24 Sean Anderson
  2020-09-10 13:38 ` Simon Glass
  2020-10-14 17:42 ` Tom Rini
  0 siblings, 2 replies; 7+ messages in thread
From: Sean Anderson @ 2020-09-09 20:24 UTC (permalink / raw)
  To: u-boot

get_ticks does not always succeed. Sometimes it can be called before the
timer has been initialized. If it does, it returns a negative errno.
This causes the timer to appear non-monotonic, because the value will
become much smaller after the timer is initialized.

No users of get_ticks which I checked handle errors of this kind. Further,
functions like tick_to_time mangle the result of get_ticks, making it very
unlikely that one could check for an error without suggesting a patch such
as this one.

This patch panics if we ever get an error. There are two cases in which
this can occur. The first is if we couldn't find/probe the timer for some
reason. One reason for this is if the timer is not available so early. This
likely indicates misconfiguration. Another reason is that the timer has an
invalid/missing device tree binding. In this case, panicing is also
correct. The second case covers errors calling get_count. This can only
occur if the timer is missing a get_count function (or on RISC-V, but that
should be fixed soon).

Fixes: c8a7ba9e6a5
Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

Changes in v2:
- Panic on error

 lib/time.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/time.c b/lib/time.c
index 47f8c84327..88bc50405f 100644
--- a/lib/time.c
+++ b/lib/time.c
@@ -91,13 +91,13 @@ uint64_t notrace get_ticks(void)
 
 		ret = dm_timer_init();
 		if (ret)
-			return ret;
+			panic("Could not initialize timer (err %d)\n", ret);
 #endif
 	}
 
 	ret = timer_get_count(gd->timer, &count);
 	if (ret)
-		return ret;
+		panic("Could not read count from timer (err %d)\n", ret);
 
 	return count;
 }
-- 
2.28.0

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v2] time: Fix get_ticks being non-monotonic
  2020-09-09 20:24 [PATCH v2] time: Fix get_ticks being non-monotonic Sean Anderson
@ 2020-09-10 13:38 ` Simon Glass
  2020-10-14 17:42 ` Tom Rini
  1 sibling, 0 replies; 7+ messages in thread
From: Simon Glass @ 2020-09-10 13:38 UTC (permalink / raw)
  To: u-boot

On Wed, 9 Sep 2020 at 14:25, Sean Anderson <seanga2@gmail.com> wrote:
>
> get_ticks does not always succeed. Sometimes it can be called before the
> timer has been initialized. If it does, it returns a negative errno.
> This causes the timer to appear non-monotonic, because the value will
> become much smaller after the timer is initialized.
>
> No users of get_ticks which I checked handle errors of this kind. Further,
> functions like tick_to_time mangle the result of get_ticks, making it very
> unlikely that one could check for an error without suggesting a patch such
> as this one.
>
> This patch panics if we ever get an error. There are two cases in which
> this can occur. The first is if we couldn't find/probe the timer for some
> reason. One reason for this is if the timer is not available so early. This
> likely indicates misconfiguration. Another reason is that the timer has an
> invalid/missing device tree binding. In this case, panicing is also
> correct. The second case covers errors calling get_count. This can only
> occur if the timer is missing a get_count function (or on RISC-V, but that
> should be fixed soon).
>
> Fixes: c8a7ba9e6a5
> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> ---
>
> Changes in v2:
> - Panic on error
>
>  lib/time.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v2] time: Fix get_ticks being non-monotonic
  2020-09-09 20:24 [PATCH v2] time: Fix get_ticks being non-monotonic Sean Anderson
  2020-09-10 13:38 ` Simon Glass
@ 2020-10-14 17:42 ` Tom Rini
  2020-11-19 10:23   ` Michael Opdenacker
  1 sibling, 1 reply; 7+ messages in thread
From: Tom Rini @ 2020-10-14 17:42 UTC (permalink / raw)
  To: u-boot

On Wed, Sep 09, 2020 at 04:24:56PM -0400, Sean Anderson wrote:

> get_ticks does not always succeed. Sometimes it can be called before the
> timer has been initialized. If it does, it returns a negative errno.
> This causes the timer to appear non-monotonic, because the value will
> become much smaller after the timer is initialized.
> 
> No users of get_ticks which I checked handle errors of this kind. Further,
> functions like tick_to_time mangle the result of get_ticks, making it very
> unlikely that one could check for an error without suggesting a patch such
> as this one.
> 
> This patch panics if we ever get an error. There are two cases in which
> this can occur. The first is if we couldn't find/probe the timer for some
> reason. One reason for this is if the timer is not available so early. This
> likely indicates misconfiguration. Another reason is that the timer has an
> invalid/missing device tree binding. In this case, panicing is also
> correct. The second case covers errors calling get_count. This can only
> occur if the timer is missing a get_count function (or on RISC-V, but that
> should be fixed soon).
> 
> Fixes: c8a7ba9e6a5
> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20201014/8c0d9833/attachment.sig>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v2] time: Fix get_ticks being non-monotonic
  2020-10-14 17:42 ` Tom Rini
@ 2020-11-19 10:23   ` Michael Opdenacker
  2020-11-21 23:07     ` Simon Glass
  0 siblings, 1 reply; 7+ messages in thread
From: Michael Opdenacker @ 2020-11-19 10:23 UTC (permalink / raw)
  To: u-boot

Hi,

Sorry, no messaging quoting, I was not subscribed to the list at that time.

Merging this change into master actually broke the SPL on
Atmel/Microchip SAMA5D3, at least booting from MMC:

RomBOOT

<debug_uart>
Could not initialize timer (err -11)

Could not initialize timer (err -11)

Could not initialize timer (err -11)
...

I'll look for a fix, but suggestions are welcome!

Cheers,

Michael.

-- 
Michael Opdenacker, CEO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v2] time: Fix get_ticks being non-monotonic
  2020-11-19 10:23   ` Michael Opdenacker
@ 2020-11-21 23:07     ` Simon Glass
  2021-03-01 16:40       ` Michael Opdenacker
  0 siblings, 1 reply; 7+ messages in thread
From: Simon Glass @ 2020-11-21 23:07 UTC (permalink / raw)
  To: u-boot

Hi Michael,

On Thu, 19 Nov 2020 at 04:23, Michael Opdenacker
<michael.opdenacker@bootlin.com> wrote:
>
> Hi,
>
> Sorry, no messaging quoting, I was not subscribed to the list at that time.
>
> Merging this change into master actually broke the SPL on
> Atmel/Microchip SAMA5D3, at least booting from MMC:
>
> RomBOOT
>
> <debug_uart>
> Could not initialize timer (err -11)
>
> Could not initialize timer (err -11)
>
> Could not initialize timer (err -11)
> ...
>
> I'll look for a fix, but suggestions are welcome!

The board might need CONFIG_TIMER_EARLY, but otherwise I think it is a
case of figuring out why the timer is used before it is available.

You can use dm_dump_all() to print out available devices and whether
they are probed.

Regards,
Simon

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v2] time: Fix get_ticks being non-monotonic
  2020-11-21 23:07     ` Simon Glass
@ 2021-03-01 16:40       ` Michael Opdenacker
  2021-03-01 23:02         ` Sean Anderson
  0 siblings, 1 reply; 7+ messages in thread
From: Michael Opdenacker @ 2021-03-01 16:40 UTC (permalink / raw)
  To: u-boot

Hi Simon,

I'm back working on this old issue. Thanks for your help with this!

So, SPL support for Atmel/Microchip SAMA5D3 is still broken in the
latest mainline, at least for the Xplained board with the MMC configuration.

My comments and further questions below...

On 11/22/20 12:07 AM, Simon Glass wrote:
> Hi Michael,
>
> On Thu, 19 Nov 2020 at 04:23, Michael Opdenacker
> <michael.opdenacker@bootlin.com> wrote:
>> Hi,
>>
>> Sorry, no messaging quoting, I was not subscribed to the list at that time.
>>
>> Merging this change into master actually broke the SPL on
>> Atmel/Microchip SAMA5D3, at least booting from MMC:
>>
>> RomBOOT
>>
>> <debug_uart>
>> Could not initialize timer (err -11)
>>
>> Could not initialize timer (err -11)
>>
>> Could not initialize timer (err -11)
>> ...
>>
>> I'll look for a fix, but suggestions are welcome!


Now, it's:

Could not initialize timer (err -19)
(-19 is -ENODEV? /* No such device */)

> The board might need CONFIG_TIMER_EARLY, but otherwise I think it is a
> case of figuring out why the timer is used before it is available.


I tried to enable CONFIG_TIMER_EARLY but it fails at link time:

? LD????? u-boot
arm-linux-ld.bfd: lib/built-in.o: in function `get_tbclk':
/home/mike/work/git/git.denx.de/u-boot/lib/time.c:70: undefined
reference to `timer_early_get_rate'
arm-linux-ld.bfd: lib/built-in.o: in function `get_ticks':
/home/mike/work/git/git.denx.de/u-boot/lib/time.c:90: undefined
reference to `timer_early_get_count'
make: *** [Makefile:1765: u-boot] Error 1


This is not a surprise, as according to
https://elixir.bootlin.com/u-boot/latest/C/ident/timer_early_get_rate,?
timer_early_get_rate() is not implemented on ARM, only on sandbox
(drivers/timer/sandbox_timer.c) and on x86 (drivers/timer/tsc_timer.c).

So, I'm moving to your second suggestion...


>
> You can use dm_dump_all() to print out available devices and whether
> they are probed.


Done, I added dm_dump_all to lib/time.c right before the panic() message:

?Class???? Index? Probed? Driver??????????????? Name
-----------------------------------------------------------
?root? 0? [ + ]?? root_driver? root_driver
?simple_bus? 0? [?? ]?? simple_bus? `-- ahb
?simple_bus? 1? [?? ]?? simple_bus????? `-- apb
?mmc? 0? [?? ]?? atmel-mci????????? |-- mmc at f0000000
?blk? 0? [?? ]?? mmc_blk????????? |?? `-- mmc at f0000000.blk
?mmc? 1? [?? ]?? atmel-mci????????? |-- mmc at f8000000
?blk? 1? [?? ]?? mmc_blk????????? |?? `-- mmc at f8000000.blk
?serial? 0? [?? ]?? serial_atmel????????? |-- serial at ffffee00
?pinctrl? 0? [?? ]?? atmel_sama5d3_pinctrl????????? |-- pinctrl at fffff200
?pinconfig? 0? [?? ]?? pinconfig????????? |?? |-- dbgu
?pinconfig? 1? [?? ]?? pinconfig????????? |?? |?? `-- dbgu-0
?pinconfig? 2? [?? ]?? pinconfig????????? |?? |-- mmc0
?pinconfig? 3? [?? ]?? pinconfig????????? |?? |?? |-- mmc0_clk_cmd_dat0
?pinconfig? 4? [?? ]?? pinconfig????????? |?? |?? |-- mmc0_dat1_3
?pinconfig? 5? [?? ]?? pinconfig????????? |?? |?? `-- mmc0_dat4_7
?pinconfig? 6? [?? ]?? pinconfig????????? |?? |-- mmc1
?pinconfig? 7? [?? ]?? pinconfig????????? |?? |?? |-- mmc1_clk_cmd_dat0
?pinconfig? 8? [?? ]?? pinconfig????????? |?? |?? `-- mmc1_dat1_3
?pinconfig? 9? [?? ]?? pinconfig????????? |?? |-- spi0
?pinconfig? 10? [?? ]?? pinconfig????????? |?? |?? `-- spi0-0
?pinconfig? 11? [?? ]?? pinconfig????????? |?? |-- spi1
?pinconfig? 12? [?? ]?? pinconfig????????? |?? |?? `-- spi1-0
?pinconfig? 13? [?? ]?? pinconfig????????? |?? `-- board
?pinconfig? 14? [?? ]?? pinconfig????????? |?????? |-- mmc0_cd
?pinconfig? 15? [?? ]?? pinconfig????????? |?????? `-- mmc1_cd
?gpio? 0? [?? ]?? atmel_at91rm9200_gpio????????? |-- gpio at fffff200
?gpio? 1? [?? ]?? atmel_at91rm9200_gpio????????? |-- gpio at fffff400
?gpio? 2? [?? ]?? atmel_at91rm9200_gpio????????? |-- gpio at fffff600
?gpio? 3? [?? ]?? atmel_at91rm9200_gpio????????? |-- gpio at fffff800
?gpio? 4? [?? ]?? atmel_at91rm9200_gpio????????? |-- gpio at fffffa00
?simple_bus? 2? [?? ]?? at91-pmc????????? `-- pmc at fffffc00
?clk? 0? [?? ]?? at91sam9x5-utmi-clk????????????? |-- utmick
?clk? 1? [?? ]?? at91-master-clk????????????? |-- masterck
?misc? 0? [?? ]?? sam9x5-periph-clk????????????? `-- periphck
?clk? 2? [?? ]?? periph-clk????????????????? |-- dbgu_clk at 2
?clk? 3? [?? ]?? periph-clk????????????????? |-- pioA_clk at 6
?clk? 4? [?? ]?? periph-clk????????????????? |-- pioB_clk at 7
?clk? 5? [?? ]?? periph-clk????????????????? |-- pioC_clk at 8
?clk? 6? [?? ]?? periph-clk????????????????? |-- pioD_clk at 9
?clk? 7? [?? ]?? periph-clk????????????????? |-- pioE_clk at 10
?clk? 8? [?? ]?? periph-clk????????????????? |-- mci0_clk at 21
?clk? 9? [?? ]?? periph-clk????????????????? |-- mci1_clk at 22
?clk? 10? [?? ]?? periph-clk????????????????? |-- spi0_clk at 24
?clk? 11? [?? ]?? periph-clk????????????????? `-- spi1_clk at 25
Could not initialize timer (err -19)

I'm not familiar enough with U-Boot yet (and with SAMA5D3 support
either) to see why the timer device is missing here, but I hope our
Microchip friends can give us further clues...

Thanks again,

Cheers,

Michael.

-- 
Michael Opdenacker, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v2] time: Fix get_ticks being non-monotonic
  2021-03-01 16:40       ` Michael Opdenacker
@ 2021-03-01 23:02         ` Sean Anderson
  0 siblings, 0 replies; 7+ messages in thread
From: Sean Anderson @ 2021-03-01 23:02 UTC (permalink / raw)
  To: u-boot

On 3/1/21 11:40 AM, Michael Opdenacker wrote:
> Hi Simon,
> 
> I'm back working on this old issue. Thanks for your help with this!
> 
> So, SPL support for Atmel/Microchip SAMA5D3 is still broken in the
> latest mainline, at least for the Xplained board with the MMC configuration.
> 
> My comments and further questions below...
> 
> On 11/22/20 12:07 AM, Simon Glass wrote:
>> Hi Michael,
>>
>> On Thu, 19 Nov 2020 at 04:23, Michael Opdenacker
>> <michael.opdenacker@bootlin.com> wrote:
>>> Hi,
>>>
>>> Sorry, no messaging quoting, I was not subscribed to the list at that time.
>>>
>>> Merging this change into master actually broke the SPL on
>>> Atmel/Microchip SAMA5D3, at least booting from MMC:
>>>
>>> RomBOOT
>>>
>>> <debug_uart>
>>> Could not initialize timer (err -11)
>>>
>>> Could not initialize timer (err -11)
>>>
>>> Could not initialize timer (err -11)
>>> ...
>>>
>>> I'll look for a fix, but suggestions are welcome!
> 
> 
> Now, it's:
> 
> Could not initialize timer (err -19)
> (-19 is -ENODEV  /* No such device */)
> 
>> The board might need CONFIG_TIMER_EARLY, but otherwise I think it is a
>> case of figuring out why the timer is used before it is available.
> 
> 
> I tried to enable CONFIG_TIMER_EARLY but it fails at link time:
> 
>    LD      u-boot
> arm-linux-ld.bfd: lib/built-in.o: in function `get_tbclk':
> /home/mike/work/git/git.denx.de/u-boot/lib/time.c:70: undefined
> reference to `timer_early_get_rate'
> arm-linux-ld.bfd: lib/built-in.o: in function `get_ticks':
> /home/mike/work/git/git.denx.de/u-boot/lib/time.c:90: undefined
> reference to `timer_early_get_count'
> make: *** [Makefile:1765: u-boot] Error 1
> 
> 
> This is not a surprise, as according to
> https://elixir.bootlin.com/u-boot/latest/C/ident/timer_early_get_rate,
> timer_early_get_rate() is not implemented on ARM, only on sandbox
> (drivers/timer/sandbox_timer.c) and on x86 (drivers/timer/tsc_timer.c).

It is also available on most RISC-V boards, not that it helps :)

> 
> So, I'm moving to your second suggestion...
> 
> 
>>
>> You can use dm_dump_all() to print out available devices and whether
>> they are probed.
> 
> 
> Done, I added dm_dump_all to lib/time.c right before the panic() message:
> 
>   Class     Index  Probed  Driver                Name
> -----------------------------------------------------------
>   root  0  [ + ]   root_driver  root_driver
>   simple_bus  0  [   ]   simple_bus  `-- ahb
>   simple_bus  1  [   ]   simple_bus      `-- apb
>   mmc  0  [   ]   atmel-mci          |-- mmc at f0000000
>   blk  0  [   ]   mmc_blk          |   `-- mmc at f0000000.blk
>   mmc  1  [   ]   atmel-mci          |-- mmc at f8000000
>   blk  1  [   ]   mmc_blk          |   `-- mmc at f8000000.blk
>   serial  0  [   ]   serial_atmel          |-- serial at ffffee00
>   pinctrl  0  [   ]   atmel_sama5d3_pinctrl          |-- pinctrl at fffff200
>   pinconfig  0  [   ]   pinconfig          |   |-- dbgu
>   pinconfig  1  [   ]   pinconfig          |   |   `-- dbgu-0
>   pinconfig  2  [   ]   pinconfig          |   |-- mmc0
>   pinconfig  3  [   ]   pinconfig          |   |   |-- mmc0_clk_cmd_dat0
>   pinconfig  4  [   ]   pinconfig          |   |   |-- mmc0_dat1_3
>   pinconfig  5  [   ]   pinconfig          |   |   `-- mmc0_dat4_7
>   pinconfig  6  [   ]   pinconfig          |   |-- mmc1
>   pinconfig  7  [   ]   pinconfig          |   |   |-- mmc1_clk_cmd_dat0
>   pinconfig  8  [   ]   pinconfig          |   |   `-- mmc1_dat1_3
>   pinconfig  9  [   ]   pinconfig          |   |-- spi0
>   pinconfig  10  [   ]   pinconfig          |   |   `-- spi0-0
>   pinconfig  11  [   ]   pinconfig          |   |-- spi1
>   pinconfig  12  [   ]   pinconfig          |   |   `-- spi1-0
>   pinconfig  13  [   ]   pinconfig          |   `-- board
>   pinconfig  14  [   ]   pinconfig          |       |-- mmc0_cd
>   pinconfig  15  [   ]   pinconfig          |       `-- mmc1_cd
>   gpio  0  [   ]   atmel_at91rm9200_gpio          |-- gpio at fffff200
>   gpio  1  [   ]   atmel_at91rm9200_gpio          |-- gpio at fffff400
>   gpio  2  [   ]   atmel_at91rm9200_gpio          |-- gpio at fffff600
>   gpio  3  [   ]   atmel_at91rm9200_gpio          |-- gpio at fffff800
>   gpio  4  [   ]   atmel_at91rm9200_gpio          |-- gpio at fffffa00
>   simple_bus  2  [   ]   at91-pmc          `-- pmc at fffffc00
>   clk  0  [   ]   at91sam9x5-utmi-clk              |-- utmick
>   clk  1  [   ]   at91-master-clk              |-- masterck
>   misc  0  [   ]   sam9x5-periph-clk              `-- periphck
>   clk  2  [   ]   periph-clk                  |-- dbgu_clk at 2
>   clk  3  [   ]   periph-clk                  |-- pioA_clk at 6
>   clk  4  [   ]   periph-clk                  |-- pioB_clk at 7
>   clk  5  [   ]   periph-clk                  |-- pioC_clk at 8
>   clk  6  [   ]   periph-clk                  |-- pioD_clk at 9
>   clk  7  [   ]   periph-clk                  |-- pioE_clk at 10
>   clk  8  [   ]   periph-clk                  |-- mci0_clk at 21
>   clk  9  [   ]   periph-clk                  |-- mci1_clk at 22
>   clk  10  [   ]   periph-clk                  |-- spi0_clk at 24
>   clk  11  [   ]   periph-clk                  `-- spi1_clk at 25
> Could not initialize timer (err -19)

So nothing here is probed, but additionally nothing has UCLASS_TIMER.
What do you expect the timer device to be?

--Sean

> 
> I'm not familiar enough with U-Boot yet (and with SAMA5D3 support
> either) to see why the timer device is missing here, but I hope our
> Microchip friends can give us further clues...
> 
> Thanks again,
> 
> Cheers,
> 
> Michael.
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-03-01 23:02 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-09 20:24 [PATCH v2] time: Fix get_ticks being non-monotonic Sean Anderson
2020-09-10 13:38 ` Simon Glass
2020-10-14 17:42 ` Tom Rini
2020-11-19 10:23   ` Michael Opdenacker
2020-11-21 23:07     ` Simon Glass
2021-03-01 16:40       ` Michael Opdenacker
2021-03-01 23:02         ` Sean Anderson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.