| Message ID | 20240218174138.1942418-2-martin.blumenstingl@googlemail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | clocksource/drivers/arm_global_timer: Two small fixes | expand |
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
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
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 --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)
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(-)