clocksource/drivers/tegra: Cycles can't be 0
Subsystems (2)
PATCH 1/2 clocksource/drivers/tegra: Cycles can't be 0
The minimum number of "cycles" is limited to 1 byclockevents_config_and_register().
--- drivers/clocksource/timer-tegra.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/clocksource/timer-tegra.c +1 -3
@@ -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 ; }
Signature · 1 line
2.22.0
PATCH 2/2 clocksource/drivers/tegra: Set up maximum limit properly
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.
--- drivers/clocksource/timer-tegra.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/clocksource/timer-tegra.c +1 -1
@@ -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 ; }
Signature · 1 line
2.22.0
Quoted 2 lines · Dmitry Osipenko
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.
Quoted 10 lines
Signed-off-by Dmitry Osipenko <[email protected]>--- drivers/clocksource/timer-tegra.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)drivers/clocksource/timer-tegra.c +0
-0
drivers/clocksource/timer-tegra.c static int tegra_timer_set_next_event(unsigned long cycles, · line 57
Quoted 7 lines
drivers/clocksource/timer-tegra.c +1
-3@@ -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
17.06.2019 12:21, Thierry Reding пишет:
Quoted 11 lines · Dmitry Osipenko
Quoted 2 lines
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.
Quoted 3 lines
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
Quoted 13 lines
Quoted 13 lines
Signed-off-by Dmitry Osipenko <[email protected]>--- drivers/clocksource/timer-tegra.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)drivers/clocksource/timer-tegra.c +0
-0@@ -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));
Quoted 9 lines
drivers/clocksource/timer-tegra.c static int tegra_timer_set_next_event(unsigned long cycles, · line 57
Quoted 4 lines
drivers/clocksource/timer-tegra.c +1
-3-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.