This series consists of two small fixes:
1) incorrect max width (and therefore mask calculation) of the prescaler
field in the Global Timer Control Register. The field is actually 8
bit wide according to the Cortex-A9 MPCore Technical Reference Manual
r3p0 [0]
2) non-function: a stray tab which is inconsistent with to the coding
style in the driver
I don't have any ARM/STI hardware to verify the first fix. However, I
tested this on an Amlogic Meson8b board with Cortex-A5 which uses the
same Global Timer implementation as Cortex-A9 SoCs and the documentation
confirms the same register layout: [1]
[0] https://developer.arm.com/documentation/ddi0407/g/Global-timer--private-timers--and-watchdog-registers/Global-timer-registers/Global-Timer-Control-Register
[1] https://developer.arm.com/documentation/ddi0434/b/CIHJGFEA
Martin Blumenstingl (2):
clocksource/drivers/arm_global_timer: Fix maximum prescaler value
clocksource/drivers/arm_global_timer: Remove stray tab
drivers/clocksource/arm_global_timer.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
--
2.43.2
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 <[email protected]>
---
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
Remove a stray tab in global_timer_of_register() which is different from
the coding style in the rest of the driver.
Signed-off-by: Martin Blumenstingl <[email protected]>
---
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 e1c773bb5535..8dd1e46b7176 100644
--- a/drivers/clocksource/arm_global_timer.c
+++ b/drivers/clocksource/arm_global_timer.c
@@ -411,7 +411,7 @@ static int __init global_timer_of_register(struct device_node *np)
err = gt_clocksource_init();
if (err)
goto out_irq;
-
+
err = cpuhp_setup_state(CPUHP_AP_ARM_GLOBAL_TIMER_STARTING,
"clockevents/arm/global_timer:starting",
gt_starting_cpu, gt_dying_cpu);
--
2.43.2
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 <[email protected]>
> ---
> 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
Hi Daniel,
On Sun, Feb 18, 2024 at 11:59 PM Daniel Lezcano
<[email protected]> 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
On 19/02/2024 00:18, Martin Blumenstingl wrote:
> Hi Daniel,
>
> On Sun, Feb 18, 2024 at 11:59 PM Daniel Lezcano
> <[email protected]> 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
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 <[email protected]>
AuthorDate: Sun, 18 Feb 2024 18:41:37 +01:00
Committer: Daniel Lezcano <[email protected]>
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 <[email protected]>
Signed-off-by: Daniel Lezcano <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
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)
The following commit has been merged into the timers/core branch of tip:
Commit-ID: 9256cec7b4f3293c11585326401325b1f81670e1
Gitweb: https://git.kernel.org/tip/9256cec7b4f3293c11585326401325b1f81670e1
Author: Martin Blumenstingl <[email protected]>
AuthorDate: Sun, 18 Feb 2024 18:41:38 +01:00
Committer: Daniel Lezcano <[email protected]>
CommitterDate: Mon, 19 Feb 2024 00:48:54 +01:00
clocksource/drivers/arm_global_timer: Remove stray tab
Remove a stray tab in global_timer_of_register() which is different from
the coding style in the rest of the driver.
Signed-off-by: Martin Blumenstingl <[email protected]>
Signed-off-by: Daniel Lezcano <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
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 e1c773b..8dd1e46 100644
--- a/drivers/clocksource/arm_global_timer.c
+++ b/drivers/clocksource/arm_global_timer.c
@@ -411,7 +411,7 @@ static int __init global_timer_of_register(struct device_node *np)
err = gt_clocksource_init();
if (err)
goto out_irq;
-
+
err = cpuhp_setup_state(CPUHP_AP_ARM_GLOBAL_TIMER_STARTING,
"clockevents/arm/global_timer:starting",
gt_starting_cpu, gt_dying_cpu);