diff mbox series

[1/3] clocksource: davinci: work around a clocksource problem on dm365 SoC

Message ID 20191213162453.15691-2-brgl@bgdev.pl (mailing list archive)
State New, archived
Headers show
Series ARM: davinci: convert dm365 to using the new clocksource driver | expand

Commit Message

Bartosz Golaszewski Dec. 13, 2019, 4:24 p.m. UTC 
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

The DM365 platform has a strange quirk (only present when using ancient
u-boot - mainline u-boot v2013.01 and later works fine) where if we
enable the second half of the timer in periodic mode before we do its
initialization - the time won't start flowing and we can't boot.

When using more recent u-boot, we can enable the timer, then reinitialize
it and all works fine.

I've been unable to figure out why that is, but a workaround for this
is straightforward - just cache the enable bits for tim34.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/clocksource/timer-davinci.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Comments

David Lechner Dec. 16, 2019, 5:58 p.m. UTC | #1 
On 12/13/19 10:24 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> The DM365 platform has a strange quirk (only present when using ancient
> u-boot - mainline u-boot v2013.01 and later works fine) where if we
> enable the second half of the timer in periodic mode before we do its
> initialization - the time won't start flowing and we can't boot.
> 
> When using more recent u-boot, we can enable the timer, then reinitialize
> it and all works fine.
> 
> I've been unable to figure out why that is, but a workaround for this
> is straightforward - just cache the enable bits for tim34.


I had a hard time groking this code. See suggested changes below for
something that would make the intention more clear (to me at least).


> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>   drivers/clocksource/timer-davinci.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clocksource/timer-davinci.c b/drivers/clocksource/timer-davinci.c
> index 62745c962049..1c22443acaeb 100644
> --- a/drivers/clocksource/timer-davinci.c
> +++ b/drivers/clocksource/timer-davinci.c
> @@ -64,6 +64,8 @@ static struct {
>   	unsigned int tim_off;
>   } davinci_clocksource;
>   
> +static unsigned int davinci_clocksource_tim32_mode;

static unsigned bool davinci_clocksource_initialized;

> +
>   static struct davinci_clockevent *
>   to_davinci_clockevent(struct clock_event_device *clockevent)
>   {
> @@ -94,7 +96,7 @@ static void davinci_tim12_shutdown(void __iomem *base)
>   	 * halves. In this case TIM34 runs in periodic mode and we must
>   	 * not modify it.
>   	 */
> -	tcr |= DAVINCI_TIMER_ENAMODE_PERIODIC <<
> +	tcr |= davinci_clocksource_tim32_mode <<
>   		DAVINCI_TIMER_ENAMODE_SHIFT_TIM34;

	if (davinci_clocksource_initialized)
		tcr |= DAVINCI_TIMER_ENAMODE_PERIODIC <<
			DAVINCI_TIMER_ENAMODE_SHIFT_TIM34;

>   
>   	writel_relaxed(tcr, base + DAVINCI_TIMER_REG_TCR);
> @@ -107,7 +109,7 @@ static void davinci_tim12_set_oneshot(void __iomem *base)
>   	tcr = DAVINCI_TIMER_ENAMODE_ONESHOT <<
>   		DAVINCI_TIMER_ENAMODE_SHIFT_TIM12;
>   	/* Same as above. */
> -	tcr |= DAVINCI_TIMER_ENAMODE_PERIODIC <<
> +	tcr |= davinci_clocksource_tim32_mode <<
>   		DAVINCI_TIMER_ENAMODE_SHIFT_TIM34;

	if (davinci_clocksource_initialized)
		tcr |= DAVINCI_TIMER_ENAMODE_PERIODIC <<
			DAVINCI_TIMER_ENAMODE_SHIFT_TIM34;

>   
>   	writel_relaxed(tcr, base + DAVINCI_TIMER_REG_TCR);
> @@ -206,6 +208,8 @@ static void davinci_clocksource_init_tim34(void __iomem *base)
>   	writel_relaxed(0x0, base + DAVINCI_TIMER_REG_TIM34);
>   	writel_relaxed(UINT_MAX, base + DAVINCI_TIMER_REG_PRD34);
>   	writel_relaxed(tcr, base + DAVINCI_TIMER_REG_TCR);
> +
> +	davinci_clocksource_tim32_mode = DAVINCI_TIMER_ENAMODE_PERIODIC;


	davinci_clocksource_initialized  = true;

>   }
>   
>   /*
>
Bartosz Golaszewski Dec. 17, 2019, 8 a.m. UTC | #2 
pon., 16 gru 2019 o 18:58 David Lechner <david@lechnology.com> napisał(a):
>
> >
> > +static unsigned int davinci_clocksource_tim32_mode;
>
> static unsigned bool davinci_clocksource_initialized;
>
> > +
> >   static struct davinci_clockevent *
> >   to_davinci_clockevent(struct clock_event_device *clockevent)
> >   {
> > @@ -94,7 +96,7 @@ static void davinci_tim12_shutdown(void __iomem *base)
> >        * halves. In this case TIM34 runs in periodic mode and we must
> >        * not modify it.
> >        */
> > -     tcr |= DAVINCI_TIMER_ENAMODE_PERIODIC <<
> > +     tcr |= davinci_clocksource_tim32_mode <<
> >               DAVINCI_TIMER_ENAMODE_SHIFT_TIM34;
>
>         if (davinci_clocksource_initialized)
>                 tcr |= DAVINCI_TIMER_ENAMODE_PERIODIC <<
>                         DAVINCI_TIMER_ENAMODE_SHIFT_TIM34;

I thought about doing this initially, but then figured this code would
be called a lot, so why not avoid branching and just store the right
value? Alternatively we can do:

    if (likely(davinci_clocksource_initialized)
        ....

Bart
David Lechner Dec. 17, 2019, 5:01 p.m. UTC | #3 
On 12/17/19 2:00 AM, Bartosz Golaszewski wrote:
> pon., 16 gru 2019 o 18:58 David Lechner <david@lechnology.com> napisał(a):
>>
>>>
>>> +static unsigned int davinci_clocksource_tim32_mode;
>>
>> static unsigned bool davinci_clocksource_initialized;
>>
>>> +
>>>    static struct davinci_clockevent *
>>>    to_davinci_clockevent(struct clock_event_device *clockevent)
>>>    {
>>> @@ -94,7 +96,7 @@ static void davinci_tim12_shutdown(void __iomem *base)
>>>         * halves. In this case TIM34 runs in periodic mode and we must
>>>         * not modify it.
>>>         */
>>> -     tcr |= DAVINCI_TIMER_ENAMODE_PERIODIC <<
>>> +     tcr |= davinci_clocksource_tim32_mode <<
>>>                DAVINCI_TIMER_ENAMODE_SHIFT_TIM34;
>>
>>          if (davinci_clocksource_initialized)
>>                  tcr |= DAVINCI_TIMER_ENAMODE_PERIODIC <<
>>                          DAVINCI_TIMER_ENAMODE_SHIFT_TIM34;
> 
> I thought about doing this initially, but then figured this code would
> be called a lot, so why not avoid branching and just store the right
> value? Alternatively we can do:
> 
>      if (likely(davinci_clocksource_initialized)
>          ....
> 

Unless there is a measurable performance difference, I think it
would be better with if/likely.
Sekhar Nori Dec. 18, 2019, 9:28 a.m. UTC | #4 
Hi Bart,

On 13/12/19 9:54 PM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> The DM365 platform has a strange quirk (only present when using ancient
> u-boot - mainline u-boot v2013.01 and later works fine) where if we
> enable the second half of the timer in periodic mode before we do its
> initialization - the time won't start flowing and we can't boot.
> 
> When using more recent u-boot, we can enable the timer, then reinitialize
> it and all works fine.
> 
> I've been unable to figure out why that is, but a workaround for this
> is straightforward - just cache the enable bits for tim34.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Timer Global Control Register (TGCR) has bits to reset both halves of
timer. Does placing both halves in reset, waiting a bit (say 10ms) and
then taking them out of reset help solve the this problem?

Also, there are LPSCs controlling the timers. As an experiment, can you
see if using LPSC_STATE_SWRSTDISABLE instead of LPSC_STATE_DISABLE in
davinci_lpsc_clk_disable() and then doing a clk_disable() + clk_enable()
on timer can get the timer out of this bad state.

We need some way for Linux to start on a clean state after bootloader is
done. And trying to reset the timer before use seems to be a better way
to accomplish it.

I assume the original code was just lucky in not hitting this case?

Thanks,
Sekhar
Bartosz Golaszewski Dec. 19, 2019, 8:59 a.m. UTC | #5 
śr., 18 gru 2019 o 10:28 Sekhar Nori <nsekhar@ti.com> napisał(a):
>
> Hi Bart,
>
> On 13/12/19 9:54 PM, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > The DM365 platform has a strange quirk (only present when using ancient
> > u-boot - mainline u-boot v2013.01 and later works fine) where if we
> > enable the second half of the timer in periodic mode before we do its
> > initialization - the time won't start flowing and we can't boot.
> >
> > When using more recent u-boot, we can enable the timer, then reinitialize
> > it and all works fine.
> >
> > I've been unable to figure out why that is, but a workaround for this
> > is straightforward - just cache the enable bits for tim34.
> >
> > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> Timer Global Control Register (TGCR) has bits to reset both halves of
> timer. Does placing both halves in reset, waiting a bit (say 10ms) and
> then taking them out of reset help solve the this problem?
>

No, it doesn't change anything. On u-boot present on my dm365-evm,
tim34 is not in reset when we get to linux, while tim12 is in reset,
but putting tim34 in reset in linux doesn't seem to change anything.

> Also, there are LPSCs controlling the timers. As an experiment, can you
> see if using LPSC_STATE_SWRSTDISABLE instead of LPSC_STATE_DISABLE in
> davinci_lpsc_clk_disable() and then doing a clk_disable() + clk_enable()
> on timer can get the timer out of this bad state.

I tried several combinations of this e.g. normal prepare_enable ->
disable -> enable, disable -> enable, disable -> delay -> enable etc.
and neither worked.

>
> We need some way for Linux to start on a clean state after bootloader is
> done. And trying to reset the timer before use seems to be a better way
> to accomplish it.
>
> I assume the original code was just lucky in not hitting this case?
>

I guess so. It used to re-read the registers instead of assuming
certain values. When I did it too, there was no problem, it's only
when we dropped re-reading that this must have appeared.

Bart
diff mbox series

Patch

diff --git a/drivers/clocksource/timer-davinci.c b/drivers/clocksource/timer-davinci.c
index 62745c962049..1c22443acaeb 100644
--- a/drivers/clocksource/timer-davinci.c
+++ b/drivers/clocksource/timer-davinci.c
@@ -64,6 +64,8 @@  static struct {
 	unsigned int tim_off;
 } davinci_clocksource;
 
+static unsigned int davinci_clocksource_tim32_mode;
+
 static struct davinci_clockevent *
 to_davinci_clockevent(struct clock_event_device *clockevent)
 {
@@ -94,7 +96,7 @@  static void davinci_tim12_shutdown(void __iomem *base)
 	 * halves. In this case TIM34 runs in periodic mode and we must
 	 * not modify it.
 	 */
-	tcr |= DAVINCI_TIMER_ENAMODE_PERIODIC <<
+	tcr |= davinci_clocksource_tim32_mode <<
 		DAVINCI_TIMER_ENAMODE_SHIFT_TIM34;
 
 	writel_relaxed(tcr, base + DAVINCI_TIMER_REG_TCR);
@@ -107,7 +109,7 @@  static void davinci_tim12_set_oneshot(void __iomem *base)
 	tcr = DAVINCI_TIMER_ENAMODE_ONESHOT <<
 		DAVINCI_TIMER_ENAMODE_SHIFT_TIM12;
 	/* Same as above. */
-	tcr |= DAVINCI_TIMER_ENAMODE_PERIODIC <<
+	tcr |= davinci_clocksource_tim32_mode <<
 		DAVINCI_TIMER_ENAMODE_SHIFT_TIM34;
 
 	writel_relaxed(tcr, base + DAVINCI_TIMER_REG_TCR);
@@ -206,6 +208,8 @@  static void davinci_clocksource_init_tim34(void __iomem *base)
 	writel_relaxed(0x0, base + DAVINCI_TIMER_REG_TIM34);
 	writel_relaxed(UINT_MAX, base + DAVINCI_TIMER_REG_PRD34);
 	writel_relaxed(tcr, base + DAVINCI_TIMER_REG_TCR);
+
+	davinci_clocksource_tim32_mode = DAVINCI_TIMER_ENAMODE_PERIODIC;
 }
 
 /*