[PATCH v1 1/2] clocksource/drivers/tegra: Cycles can't be 0

[PATCH v1 1/2] clocksource/drivers/tegra: Cycles can't be 0

[Dmitry Osipenko]

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

[PATCH v1 2/2] clocksource/drivers/tegra: Set up maximum limit properly

[Dmitry Osipenko]

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

Re: [PATCH v1 1/2] clocksource/drivers/tegra: Cycles can't be 0

[Thierry Reding]

[-- 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 --]

Re: [PATCH v1 1/2] clocksource/drivers/tegra: Cycles can't be 0

[Dmitry Osipenko]

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.