LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v1 1/2] clocksource/drivers/tegra: Cycles can't be 0
@ 2019-06-16 23:47 Dmitry Osipenko
  2019-06-16 23:47 ` [PATCH v1 2/2] clocksource/drivers/tegra: Set up maximum limit properly Dmitry Osipenko
  2019-06-17  9:21 ` [PATCH v1 1/2] clocksource/drivers/tegra: Cycles can't be 0 Thierry Reding
  0 siblings, 2 replies; 4+ messages in thread
From: Dmitry Osipenko @ 2019-06-16 23:47 UTC (permalink / raw)
  To: Daniel Lezcano, Thierry Reding, Jonathan Hunter, Peter De Schrijver
  Cc: linux-tegra, linux-kernel

The minimum number of "cycles" is limited to 1 by
clockevents_config_and_register().

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/clocksource/timer-tegra.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/clocksource/timer-tegra.c b/drivers/clocksource/timer-tegra.c
index f6a8eb0d7322..090c85358fe8 100644
--- a/drivers/clocksource/timer-tegra.c
+++ b/drivers/clocksource/timer-tegra.c
@@ -54,9 +54,7 @@ static int tegra_timer_set_next_event(unsigned long cycles,
 {
 	void __iomem *reg_base = timer_of_base(to_timer_of(evt));
 
-	writel_relaxed(TIMER_PTV_EN |
-		       ((cycles > 1) ? (cycles - 1) : 0), /* n+1 scheme */
-		       reg_base + TIMER_PTV);
+	writel_relaxed(TIMER_PTV_EN | (cycles - 1), reg_base + TIMER_PTV);
 
 	return 0;
 }
-- 
2.22.0


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

* [PATCH v1 2/2] clocksource/drivers/tegra: Set up maximum limit properly
  2019-06-16 23:47 [PATCH v1 1/2] clocksource/drivers/tegra: Cycles can't be 0 Dmitry Osipenko
@ 2019-06-16 23:47 ` Dmitry Osipenko
  2019-06-17  9:21 ` [PATCH v1 1/2] clocksource/drivers/tegra: Cycles can't be 0 Thierry Reding
  1 sibling, 0 replies; 4+ messages in thread
From: Dmitry Osipenko @ 2019-06-16 23:47 UTC (permalink / raw)
  To: Daniel Lezcano, Thierry Reding, Jonathan Hunter, Peter De Schrijver
  Cc: linux-tegra, linux-kernel

Tegra's timer has 29 bits for the counter. The counter's value is smaller
than the actual value by 1, hence the maximum equals to 29 bits + 1.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/clocksource/timer-tegra.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clocksource/timer-tegra.c b/drivers/clocksource/timer-tegra.c
index 090c85358fe8..d0a46e10dbca 100644
--- a/drivers/clocksource/timer-tegra.c
+++ b/drivers/clocksource/timer-tegra.c
@@ -130,7 +130,7 @@ static int tegra_timer_setup(unsigned int cpu)
 
 	clockevents_config_and_register(&to->clkevt, timer_of_rate(to),
 					1, /* min */
-					0x1fffffff); /* 29 bits */
+					0x20000000); /* 29 bits + 1 */
 
 	return 0;
 }
-- 
2.22.0


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

* Re: [PATCH v1 1/2] clocksource/drivers/tegra: Cycles can't be 0
  2019-06-16 23:47 [PATCH v1 1/2] clocksource/drivers/tegra: Cycles can't be 0 Dmitry Osipenko
  2019-06-16 23:47 ` [PATCH v1 2/2] clocksource/drivers/tegra: Set up maximum limit properly Dmitry Osipenko
@ 2019-06-17  9:21 ` Thierry Reding
  2019-06-17 14:41   ` Dmitry Osipenko
  1 sibling, 1 reply; 4+ messages in thread
From: Thierry Reding @ 2019-06-17  9:21 UTC (permalink / raw)
  To: Dmitry Osipenko
  Cc: Daniel Lezcano, Jonathan Hunter, Peter De Schrijver, linux-tegra,
	linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1765 bytes --]

On Mon, Jun 17, 2019 at 02:47:43AM +0300, Dmitry Osipenko wrote:
> The minimum number of "cycles" is limited to 1 by
> clockevents_config_and_register().

Looks to me like cycles also depends on the multiplier and shift that
are computed for the clock source. It looks like the multiplier will
never be zero and the shift will always be such that it won't result in
a zero cycles either. But might be worth mentioning this in the commit
message. And it might be nice to also repeate that in a comment close to
the code, just to document this.

It took me a couple of minutes to track this all down, so I think we
should take the same amount of time to document it so that we don't have
to go through it again once we've forgotten why we made this change.

> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/clocksource/timer-tegra.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/clocksource/timer-tegra.c b/drivers/clocksource/timer-tegra.c
> index f6a8eb0d7322..090c85358fe8 100644
> --- a/drivers/clocksource/timer-tegra.c
> +++ b/drivers/clocksource/timer-tegra.c
> @@ -54,9 +54,7 @@ static int tegra_timer_set_next_event(unsigned long cycles,
>  {
>  	void __iomem *reg_base = timer_of_base(to_timer_of(evt));
>  
> -	writel_relaxed(TIMER_PTV_EN |
> -		       ((cycles > 1) ? (cycles - 1) : 0), /* n+1 scheme */
> -		       reg_base + TIMER_PTV);
> +	writel_relaxed(TIMER_PTV_EN | (cycles - 1), reg_base + TIMER_PTV);

Maybe keep the n+1 scheme comment and explain why we don't need to
handle the case where cycles < 1. That way it becomes crystal clear that
we don't need it, so we decrease the chances of somebody coming around
and trying to "fix" this.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

* Re: [PATCH v1 1/2] clocksource/drivers/tegra: Cycles can't be 0
  2019-06-17  9:21 ` [PATCH v1 1/2] clocksource/drivers/tegra: Cycles can't be 0 Thierry Reding
@ 2019-06-17 14:41   ` Dmitry Osipenko
  0 siblings, 0 replies; 4+ messages in thread
From: Dmitry Osipenko @ 2019-06-17 14:41 UTC (permalink / raw)
  To: Thierry Reding, Daniel Lezcano
  Cc: Jonathan Hunter, Peter De Schrijver, linux-tegra, linux-kernel

17.06.2019 12:21, Thierry Reding пишет:
> On Mon, Jun 17, 2019 at 02:47:43AM +0300, Dmitry Osipenko wrote:
>> The minimum number of "cycles" is limited to 1 by
>> clockevents_config_and_register().
> 
> Looks to me like cycles also depends on the multiplier and shift that
> are computed for the clock source. It looks like the multiplier will
> never be zero and the shift will always be such that it won't result in
> a zero cycles either. But might be worth mentioning this in the commit
> message. And it might be nice to also repeate that in a comment close to
> the code, just to document this.
> 
> It took me a couple of minutes to track this all down, so I think we
> should take the same amount of time to document it so that we don't have
> to go through it again once we've forgotten why we made this change.

It's explicitly stated in the comment [1] what's the intent of the min_delta. But it's
also good that you verified the clocksource code itself :)

[1] https://elixir.bootlin.com/linux/v5.2-rc5/source/kernel/time/clockevents.c#L500

>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/clocksource/timer-tegra.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/clocksource/timer-tegra.c b/drivers/clocksource/timer-tegra.c
>> index f6a8eb0d7322..090c85358fe8 100644
>> --- a/drivers/clocksource/timer-tegra.c
>> +++ b/drivers/clocksource/timer-tegra.c
>> @@ -54,9 +54,7 @@ static int tegra_timer_set_next_event(unsigned long cycles,
>>  {
>>  	void __iomem *reg_base = timer_of_base(to_timer_of(evt));
>>  
>> -	writel_relaxed(TIMER_PTV_EN |
>> -		       ((cycles > 1) ? (cycles - 1) : 0), /* n+1 scheme */
>> -		       reg_base + TIMER_PTV);
>> +	writel_relaxed(TIMER_PTV_EN | (cycles - 1), reg_base + TIMER_PTV);
> 
> Maybe keep the n+1 scheme comment and explain why we don't need to
> handle the case where cycles < 1. That way it becomes crystal clear that
> we don't need it, so we decrease the chances of somebody coming around
> and trying to "fix" this.

Okay, I'll extend the commit message and add a clarifying comment to the code in v2.

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

end of thread, other threads:[~2019-06-17 14:41 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-16 23:47 [PATCH v1 1/2] clocksource/drivers/tegra: Cycles can't be 0 Dmitry Osipenko
2019-06-16 23:47 ` [PATCH v1 2/2] clocksource/drivers/tegra: Set up maximum limit properly Dmitry Osipenko
2019-06-17  9:21 ` [PATCH v1 1/2] clocksource/drivers/tegra: Cycles can't be 0 Thierry Reding
2019-06-17 14:41   ` Dmitry Osipenko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).