2024-02-21 21:44:14

by Martin Blumenstingl

Subject: [PATCH v1] clocksource/drivers/arm_global_timer: Simplify prescaler register access

Use GENMASK() to define the prescaler mask and make the whole driver use
the mask (together with helpers such as FIELD_GET and FIELD_PREP)
instead of needed an additional shift and max value constant.

Signed-off-by: Martin Blumenstingl <[email protected]>
---
drivers/clocksource/arm_global_timer.c | 19 ++++++++-----------
1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c
index 8dd1e46b7176..1eb91fa00657 100644
--- a/drivers/clocksource/arm_global_timer.c
+++ b/drivers/clocksource/arm_global_timer.c
@@ -9,6 +9,7 @@

#include <linux/init.h>
#include <linux/interrupt.h>
+#include <linux/bitfield.h>
#include <linux/clocksource.h>
#include <linux/clockchips.h>
#include <linux/cpu.h>
@@ -31,10 +32,7 @@
#define GT_CONTROL_COMP_ENABLE BIT(1) /* banked */
#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 0xFF
-#define GT_CONTROL_PRESCALER_MASK (GT_CONTROL_PRESCALER_MAX << \
- GT_CONTROL_PRESCALER_SHIFT)
+#define GT_CONTROL_PRESCALER_MASK GENMASK(15, 8)

#define GT_INT_STATUS 0x0c
#define GT_INT_STATUS_EVENT_FLAG BIT(0)
@@ -247,7 +245,7 @@ static void gt_write_presc(u32 psv)

reg = readl(gt_base + GT_CONTROL);
reg &= ~GT_CONTROL_PRESCALER_MASK;
- reg |= psv << GT_CONTROL_PRESCALER_SHIFT;
+ reg |= FIELD_PREP(GT_CONTROL_PRESCALER_MASK, psv);
writel(reg, gt_base + GT_CONTROL);
}

@@ -256,8 +254,7 @@ static u32 gt_read_presc(void)
u32 reg;

reg = readl(gt_base + GT_CONTROL);
- reg &= GT_CONTROL_PRESCALER_MASK;
- return reg >> GT_CONTROL_PRESCALER_SHIFT;
+ return FIELD_GET(GT_CONTROL_PRESCALER_MASK, reg);
}

static void __init gt_delay_timer_init(void)
@@ -272,9 +269,9 @@ static int __init gt_clocksource_init(void)
writel(0, gt_base + GT_COUNTER0);
writel(0, gt_base + GT_COUNTER1);
/* set prescaler and enable timer on all the cores */
- writel(((CONFIG_ARM_GT_INITIAL_PRESCALER_VAL - 1) <<
- GT_CONTROL_PRESCALER_SHIFT)
- | GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL);
+ writel(FIELD_PREP(GT_CONTROL_PRESCALER_MASK,
+ CONFIG_ARM_GT_INITIAL_PRESCALER_VAL - 1) |
+ GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL);

#ifdef CONFIG_CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCK
sched_clock_register(gt_sched_clock_read, 64, gt_target_rate);
@@ -301,7 +298,7 @@ static int gt_clk_rate_change_cb(struct notifier_block *nb,
psv--;

/* prescaler within legal range? */
- if (psv < 0 || psv > GT_CONTROL_PRESCALER_MAX)
+ if (psv < 0 || psv > FIELD_GET(GT_CONTROL_PRESCALER_MASK, ~0))
return NOTIFY_BAD;

/*
--
2.43.2

2024-02-22 10:02:33

by Daniel Lezcano

Subject: Re: [PATCH v1] clocksource/drivers/arm_global_timer: Simplify prescaler register access

Hi Martin,

thank you for providing these cleanups.

One question below:


On 21/02/2024 22:43, Martin Blumenstingl wrote:

[ ... ]

> /* prescaler within legal range? */
> - if (psv < 0 || psv > GT_CONTROL_PRESCALER_MAX)
> + if (psv < 0 || psv > FIELD_GET(GT_CONTROL_PRESCALER_MASK, ~0))

FIELD_MAX() ?

[ ... ]

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

2024-02-22 22:00:15

by Martin Blumenstingl

Subject: Re: [PATCH v1] clocksource/drivers/arm_global_timer: Simplify prescaler register access

Hi Daniel,

On Thu, Feb 22, 2024 at 11:02 AM Daniel Lezcano
<[email protected]> wrote:
[ ... ]
>
> > /* prescaler within legal range? */
> > - if (psv < 0 || psv > GT_CONTROL_PRESCALER_MAX)
> > + if (psv < 0 || psv > FIELD_GET(GT_CONTROL_PRESCALER_MASK, ~0))
>
> FIELD_MAX() ?
Oh, I was not aware of FIELD_MAX() - thank you!
While researching that I found that there's also FIELD_FIT() which I
think is perfect here. What do you think?


Best regards,
Martin

2024-02-22 22:34:33

by Daniel Lezcano

Subject: Re: [PATCH v1] clocksource/drivers/arm_global_timer: Simplify prescaler register access

On 22/02/2024 22:57, Martin Blumenstingl wrote:
> Hi Daniel,
>
> On Thu, Feb 22, 2024 at 11:02 AM Daniel Lezcano
> <[email protected]> wrote:
> [ ... ]
>>
>>> /* prescaler within legal range? */
>>> - if (psv < 0 || psv > GT_CONTROL_PRESCALER_MAX)
>>> + if (psv < 0 || psv > FIELD_GET(GT_CONTROL_PRESCALER_MASK, ~0))
>>
>> FIELD_MAX() ?
> Oh, I was not aware of FIELD_MAX() - thank you!
> While researching that I found that there's also FIELD_FIT() which I
> think is perfect here. What do you think?

Ah yes, even better :)

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