1. Patchew
  2. linux
  3. [PATCH v1 0/2] clocksource/drivers/arm_global_timer: Two small fixes
  4. View patch

[PATCH v1 1/2] clocksource/drivers/arm_global_timer: Fix maximum prescaler value

Martin Blumenstingl posted 2 patches 1 year, 11 months ago
[PATCH v1 1/2] clocksource/drivers/arm_global_timer: Fix maximum prescaler value
Posted by Martin Blumenstingl 1 year, 11 months ago 
The prescaler in the "Global Timer Control Register bit assignments" is
documented to use bits [15:8], which means that the maximum prescaler
register value is 0xff.

Fixes: 171b45a4a70e ("clocksource/drivers/arm_global_timer: Implement rate compensation whenever source clock changes")
Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/clocksource/arm_global_timer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c
index 44a61dc6f932..e1c773bb5535 100644
--- a/drivers/clocksource/arm_global_timer.c
+++ b/drivers/clocksource/arm_global_timer.c
@@ -32,7 +32,7 @@
 #define GT_CONTROL_IRQ_ENABLE		BIT(2)	/* banked */
 #define GT_CONTROL_AUTO_INC		BIT(3)	/* banked */
 #define GT_CONTROL_PRESCALER_SHIFT      8
-#define GT_CONTROL_PRESCALER_MAX        0xF
+#define GT_CONTROL_PRESCALER_MAX        0xFF
 #define GT_CONTROL_PRESCALER_MASK       (GT_CONTROL_PRESCALER_MAX << \
 					 GT_CONTROL_PRESCALER_SHIFT)
 
-- 
2.43.2
Re: [PATCH v1 1/2] clocksource/drivers/arm_global_timer: Fix maximum prescaler value
Posted by Daniel Lezcano 1 year, 11 months ago 
On 18/02/2024 18:41, Martin Blumenstingl wrote:
> The prescaler in the "Global Timer Control Register bit assignments" is
> documented to use bits [15:8], which means that the maximum prescaler
> register value is 0xff.
> 
> Fixes: 171b45a4a70e ("clocksource/drivers/arm_global_timer: Implement rate compensation whenever source clock changes")
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>   drivers/clocksource/arm_global_timer.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c
> index 44a61dc6f932..e1c773bb5535 100644
> --- a/drivers/clocksource/arm_global_timer.c
> +++ b/drivers/clocksource/arm_global_timer.c
> @@ -32,7 +32,7 @@
>   #define GT_CONTROL_IRQ_ENABLE		BIT(2)	/* banked */
>   #define GT_CONTROL_AUTO_INC		BIT(3)	/* banked */
>   #define GT_CONTROL_PRESCALER_SHIFT      8
> -#define GT_CONTROL_PRESCALER_MAX        0xF
> +#define GT_CONTROL_PRESCALER_MAX        0xFF
>   #define GT_CONTROL_PRESCALER_MASK       (GT_CONTROL_PRESCALER_MAX << \
>   					 GT_CONTROL_PRESCALER_SHIFT

Good catch!

IMO the initial confusion is coming from the shift and the mask size.

But should GT_CONTROL_PRESCALER_MAX be 256 ? so (0xFF + 1)

The following may be less confusing:

#define GT_CONTROL_PRESCALER_SHIFT	8
#define GT_CONTROL_PRESCALER_MASK	GENMASK(15,8)
#define GT_CONTROL_PRESCALER_MAX	(GT_CONTROL_PRESCALER_MASK >> \
					 GT_CONTROL_PRESCALER_SHIFT) + 1


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
Re: [PATCH v1 1/2] clocksource/drivers/arm_global_timer: Fix maximum prescaler value
Posted by Martin Blumenstingl 1 year, 11 months ago 
Hi Daniel,

On Sun, Feb 18, 2024 at 11:59 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
[...]
> >   #define GT_CONTROL_PRESCALER_SHIFT      8
> > -#define GT_CONTROL_PRESCALER_MAX        0xF
> > +#define GT_CONTROL_PRESCALER_MAX        0xFF
> >   #define GT_CONTROL_PRESCALER_MASK       (GT_CONTROL_PRESCALER_MAX << \
> >                                        GT_CONTROL_PRESCALER_SHIFT
>
> Good catch!
>
> IMO the initial confusion is coming from the shift and the mask size.
>
> But should GT_CONTROL_PRESCALER_MAX be 256 ? so (0xFF + 1)
It depends on what we consider "max" to be:
- the register value
- the actual number that's used in the calculation formula

If we ignore the usage of GT_CONTROL_PRESCALER_MAX within
GT_CONTROL_PRESCALER_MASK then there's only one occurrence left, which
decrements the calculated value right before comparing it against
GT_CONTROL_PRESCALER_MAX.
This means: the remaining driver currently considers
GT_CONTROL_PRESCALER_MAX to be the maximum value that can be written
to the register, having converted the value from the calculation
formula to register value beforehand.

> The following may be less confusing:
>
> #define GT_CONTROL_PRESCALER_SHIFT      8
> #define GT_CONTROL_PRESCALER_MASK       GENMASK(15,8)
> #define GT_CONTROL_PRESCALER_MAX        (GT_CONTROL_PRESCALER_MASK >> \
>                                          GT_CONTROL_PRESCALER_SHIFT) + 1
If you're interested then I'll work on a follow-up patch to clean up
the prescaler macros (using FIELD_PREP, FIELD_GET and GENMASK would
simplify things IMO).
I think that this patch is still good as-is since it's small and can
be backported easily (if someone wants to do that).


Best regards,
Martin
Re: [PATCH v1 1/2] clocksource/drivers/arm_global_timer: Fix maximum prescaler value
Posted by Daniel Lezcano 1 year, 11 months ago 
On 19/02/2024 00:18, Martin Blumenstingl wrote:
> Hi Daniel,
> 
> On Sun, Feb 18, 2024 at 11:59 PM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
> [...]
>>>    #define GT_CONTROL_PRESCALER_SHIFT      8
>>> -#define GT_CONTROL_PRESCALER_MAX        0xF
>>> +#define GT_CONTROL_PRESCALER_MAX        0xFF
>>>    #define GT_CONTROL_PRESCALER_MASK       (GT_CONTROL_PRESCALER_MAX << \
>>>                                         GT_CONTROL_PRESCALER_SHIFT
>>
>> Good catch!
>>
>> IMO the initial confusion is coming from the shift and the mask size.
>>
>> But should GT_CONTROL_PRESCALER_MAX be 256 ? so (0xFF + 1)
> It depends on what we consider "max" to be:
> - the register value
> - the actual number that's used in the calculation formula
> 
> If we ignore the usage of GT_CONTROL_PRESCALER_MAX within
> GT_CONTROL_PRESCALER_MASK then there's only one occurrence left, which
> decrements the calculated value right before comparing it against
> GT_CONTROL_PRESCALER_MAX.
> This means: the remaining driver currently considers
> GT_CONTROL_PRESCALER_MAX to be the maximum value that can be written
> to the register, having converted the value from the calculation
> formula to register value beforehand.
> 
>> The following may be less confusing:
>>
>> #define GT_CONTROL_PRESCALER_SHIFT      8
>> #define GT_CONTROL_PRESCALER_MASK       GENMASK(15,8)
>> #define GT_CONTROL_PRESCALER_MAX        (GT_CONTROL_PRESCALER_MASK >> \
>>                                           GT_CONTROL_PRESCALER_SHIFT) + 1
> If you're interested then I'll work on a follow-up patch to clean up
> the prescaler macros (using FIELD_PREP, FIELD_GET and GENMASK would
> simplify things IMO).

Yes, cleanups are welcome

> I think that this patch is still good as-is since it's small and can
> be backported easily (if someone wants to do that).

Ok, I'm fine with that


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
[tip: timers/core] clocksource/drivers/arm_global_timer: Fix maximum prescaler value
Posted by tip-bot2 for Martin Blumenstingl 1 year, 10 months ago 
The following commit has been merged into the timers/core branch of tip:

Commit-ID:     b34b9547cee41575a4fddf390f615570759dc999
Gitweb:        https://git.kernel.org/tip/b34b9547cee41575a4fddf390f615570759dc999
Author:        Martin Blumenstingl <martin.blumenstingl@googlemail.com>
AuthorDate:    Sun, 18 Feb 2024 18:41:37 +01:00
Committer:     Daniel Lezcano <daniel.lezcano@linaro.org>
CommitterDate: Mon, 19 Feb 2024 00:48:54 +01:00

clocksource/drivers/arm_global_timer: Fix maximum prescaler value

The prescaler in the "Global Timer Control Register bit assignments" is
documented to use bits [15:8], which means that the maximum prescaler
register value is 0xff.

Fixes: 171b45a4a70e ("clocksource/drivers/arm_global_timer: Implement rate compensation whenever source clock changes")
Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Link: https://lore.kernel.org/r/20240218174138.1942418-2-martin.blumenstingl@googlemail.com
---
 drivers/clocksource/arm_global_timer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c
index 44a61dc..e1c773b 100644
--- a/drivers/clocksource/arm_global_timer.c
+++ b/drivers/clocksource/arm_global_timer.c
@@ -32,7 +32,7 @@
 #define GT_CONTROL_IRQ_ENABLE		BIT(2)	/* banked */
 #define GT_CONTROL_AUTO_INC		BIT(3)	/* banked */
 #define GT_CONTROL_PRESCALER_SHIFT      8
-#define GT_CONTROL_PRESCALER_MAX        0xF
+#define GT_CONTROL_PRESCALER_MAX        0xFF
 #define GT_CONTROL_PRESCALER_MASK       (GT_CONTROL_PRESCALER_MAX << \
 					 GT_CONTROL_PRESCALER_SHIFT)

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.