[PATCH v1 0/2] clocksource/drivers/arm_global_timer: Two small fixes

[PATCH v1 0/2] clocksource/drivers/arm_global_timer: Two small fixes

[Martin Blumenstingl]

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


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

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

[Martin Blumenstingl]

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


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

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

[Daniel Lezcano]

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


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

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

[Martin Blumenstingl]

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

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

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

[Daniel Lezcano]

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


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH v1 2/2] clocksource/drivers/arm_global_timer: Remove stray tab

[Martin Blumenstingl]

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


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic