Messages in this thread | | | Date | Mon, 19 Feb 2024 00:48:37 +0100 | Subject | Re: [PATCH v1 1/2] clocksource/drivers/arm_global_timer: Fix maximum prescaler value | From | 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
| |