diff mbox series

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

Message ID 20240218174138.1942418-2-martin.blumenstingl@googlemail.com
State New
Headers
Series clocksource/drivers/arm_global_timer: Two small fixes |

Commit Message

Martin Blumenstingl Feb. 18, 2024, 5:41 p.m. UTC 
  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(-)

Comments

Daniel Lezcano Feb. 18, 2024, 10:59 p.m. UTC | #1 
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
Martin Blumenstingl Feb. 18, 2024, 11:18 p.m. UTC | #2 
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
Daniel Lezcano Feb. 18, 2024, 11:48 p.m. UTC | #3 
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
diff mbox series

Patch

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)