1. Patchew
  2. linux
  3. [PATCH v1 0/9] Move RAPL defaults to interface drivers
  4. View patch

[PATCH v1 3/9] powercap: intel_rapl: Use GENMASK() and BIT() macros

Kuppuswamy Sathyanarayanan posted 9 patches 1 week, 3 days ago
[PATCH v1 3/9] powercap: intel_rapl: Use GENMASK() and BIT() macros
Posted by Kuppuswamy Sathyanarayanan 1 week, 3 days ago 
Replace hardcoded bitmasks and bit shift operations with standard
GENMASK(), GENMASK_ULL(), BIT(), and BIT_ULL() macros for better
readability and to follow kernel coding conventions.

No functional changes.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 drivers/powercap/intel_rapl_common.c | 78 ++++++++++++++--------------
 1 file changed, 39 insertions(+), 39 deletions(-)

diff --git a/drivers/powercap/intel_rapl_common.c b/drivers/powercap/intel_rapl_common.c
index 74a74af8f0ec..0faafba8cc7c 100644
--- a/drivers/powercap/intel_rapl_common.c
+++ b/drivers/powercap/intel_rapl_common.c
@@ -31,62 +31,62 @@
 #include <asm/msr.h>
 
 /* bitmasks for RAPL MSRs, used by primitive access functions */
-#define ENERGY_STATUS_MASK		0xffffffff
+#define ENERGY_STATUS_MASK		GENMASK(31, 0)
 
-#define POWER_LIMIT1_MASK		0x7FFF
+#define POWER_LIMIT1_MASK		GENMASK(14, 0)
 #define POWER_LIMIT1_ENABLE		BIT(15)
 #define POWER_LIMIT1_CLAMP		BIT(16)
 
-#define POWER_LIMIT2_MASK		(0x7FFFULL<<32)
+#define POWER_LIMIT2_MASK		GENMASK_ULL(46, 32)
 #define POWER_LIMIT2_ENABLE		BIT_ULL(47)
 #define POWER_LIMIT2_CLAMP		BIT_ULL(48)
 #define POWER_HIGH_LOCK			BIT_ULL(63)
 #define POWER_LOW_LOCK			BIT(31)
 
-#define POWER_LIMIT4_MASK		0x1FFF
+#define POWER_LIMIT4_MASK		GENMASK(12, 0)
 
-#define TIME_WINDOW1_MASK		(0x7FULL<<17)
-#define TIME_WINDOW2_MASK		(0x7FULL<<49)
+#define TIME_WINDOW1_MASK		GENMASK_ULL(23, 17)
+#define TIME_WINDOW2_MASK		GENMASK_ULL(55, 49)
 
 #define POWER_UNIT_OFFSET		0x00
-#define POWER_UNIT_MASK			0x0F
+#define POWER_UNIT_MASK			GENMASK(3, 0)
 
 #define ENERGY_UNIT_OFFSET		0x08
-#define ENERGY_UNIT_MASK		0x1F00
+#define ENERGY_UNIT_MASK		GENMASK(12, 8)
 
 #define TIME_UNIT_OFFSET		0x10
-#define TIME_UNIT_MASK			0xF0000
+#define TIME_UNIT_MASK			GENMASK(19, 16)
 
-#define POWER_INFO_MAX_MASK		(0x7fffULL<<32)
-#define POWER_INFO_MIN_MASK		(0x7fffULL<<16)
-#define POWER_INFO_MAX_TIME_WIN_MASK	(0x3fULL<<48)
-#define POWER_INFO_THERMAL_SPEC_MASK	0x7fff
+#define POWER_INFO_MAX_MASK		GENMASK_ULL(46, 32)
+#define POWER_INFO_MIN_MASK		GENMASK_ULL(30, 16)
+#define POWER_INFO_MAX_TIME_WIN_MASK	GENMASK_ULL(53, 48)
+#define POWER_INFO_THERMAL_SPEC_MASK	GENMASK(14, 0)
 
-#define PERF_STATUS_THROTTLE_TIME_MASK	0xffffffff
-#define PP_POLICY_MASK			0x1F
+#define PERF_STATUS_THROTTLE_TIME_MASK	GENMASK(31, 0)
+#define PP_POLICY_MASK			GENMASK(4, 0)
 
 /*
  * SPR has different layout for Psys Domain PowerLimit registers.
  * There are 17 bits of PL1 and PL2 instead of 15 bits.
  * The Enable bits and TimeWindow bits are also shifted as a result.
  */
-#define PSYS_POWER_LIMIT1_MASK		0x1FFFF
+#define PSYS_POWER_LIMIT1_MASK		GENMASK_ULL(16, 0)
 #define PSYS_POWER_LIMIT1_ENABLE	BIT(17)
 
-#define PSYS_POWER_LIMIT2_MASK		(0x1FFFFULL<<32)
+#define PSYS_POWER_LIMIT2_MASK		GENMASK_ULL(48, 32)
 #define PSYS_POWER_LIMIT2_ENABLE	BIT_ULL(49)
 
-#define PSYS_TIME_WINDOW1_MASK		(0x7FULL<<19)
-#define PSYS_TIME_WINDOW2_MASK		(0x7FULL<<51)
+#define PSYS_TIME_WINDOW1_MASK		GENMASK_ULL(25, 19)
+#define PSYS_TIME_WINDOW2_MASK		GENMASK_ULL(57, 51)
 
 /* bitmasks for RAPL TPMI, used by primitive access functions */
-#define TPMI_POWER_LIMIT_MASK		0x3FFFF
+#define TPMI_POWER_LIMIT_MASK		GENMASK_ULL(17, 0)
 #define TPMI_POWER_LIMIT_ENABLE		BIT_ULL(62)
-#define TPMI_TIME_WINDOW_MASK		(0x7FULL<<18)
-#define TPMI_INFO_SPEC_MASK		0x3FFFF
-#define TPMI_INFO_MIN_MASK		(0x3FFFFULL << 18)
-#define TPMI_INFO_MAX_MASK		(0x3FFFFULL << 36)
-#define TPMI_INFO_MAX_TIME_WIN_MASK	(0x7FULL << 54)
+#define TPMI_TIME_WINDOW_MASK		GENMASK_ULL(24, 18)
+#define TPMI_INFO_SPEC_MASK		GENMASK_ULL(17, 0)
+#define TPMI_INFO_MIN_MASK		GENMASK_ULL(35, 18)
+#define TPMI_INFO_MAX_MASK		GENMASK_ULL(53, 36)
+#define TPMI_INFO_MAX_TIME_WIN_MASK	GENMASK_ULL(60, 54)
 
 /* Non HW constants */
 #define RAPL_PRIMITIVE_DERIVED		BIT(1)	/* not from raw data */
@@ -111,9 +111,9 @@
 #define TPMI_POWER_UNIT_OFFSET		POWER_UNIT_OFFSET
 #define TPMI_POWER_UNIT_MASK		POWER_UNIT_MASK
 #define TPMI_ENERGY_UNIT_OFFSET		0x06
-#define TPMI_ENERGY_UNIT_MASK		0x7C0
+#define TPMI_ENERGY_UNIT_MASK		GENMASK_ULL(10, 6)
 #define TPMI_TIME_UNIT_OFFSET		0x0C
-#define TPMI_TIME_UNIT_MASK		0xF000
+#define TPMI_TIME_UNIT_MASK		GENMASK_ULL(15, 12)
 
 #define RAPL_EVENT_MASK			GENMASK(7, 0)
 
@@ -964,13 +964,13 @@ static int rapl_check_unit_core(struct rapl_domain *rd)
 	}
 
 	value = (ra.value & ENERGY_UNIT_MASK) >> ENERGY_UNIT_OFFSET;
-	rd->energy_unit = ENERGY_UNIT_SCALE * 1000000 / (1 << value);
+	rd->energy_unit = ENERGY_UNIT_SCALE * 1000000 / BIT(value);
 
 	value = (ra.value & POWER_UNIT_MASK) >> POWER_UNIT_OFFSET;
-	rd->power_unit = 1000000 / (1 << value);
+	rd->power_unit = 1000000 / BIT(value);
 
 	value = (ra.value & TIME_UNIT_MASK) >> TIME_UNIT_OFFSET;
-	rd->time_unit = 1000000 / (1 << value);
+	rd->time_unit = 1000000 / BIT(value);
 
 	pr_debug("Core CPU %s:%s energy=%dpJ, time=%dus, power=%duW\n",
 		 rd->rp->name, rd->name, rd->energy_unit, rd->time_unit, rd->power_unit);
@@ -992,13 +992,13 @@ static int rapl_check_unit_atom(struct rapl_domain *rd)
 	}
 
 	value = (ra.value & ENERGY_UNIT_MASK) >> ENERGY_UNIT_OFFSET;
-	rd->energy_unit = ENERGY_UNIT_SCALE * 1 << value;
+	rd->energy_unit = ENERGY_UNIT_SCALE * BIT(value);
 
 	value = (ra.value & POWER_UNIT_MASK) >> POWER_UNIT_OFFSET;
-	rd->power_unit = (1 << value) * 1000;
+	rd->power_unit = BIT(value) * 1000;
 
 	value = (ra.value & TIME_UNIT_MASK) >> TIME_UNIT_OFFSET;
-	rd->time_unit = 1000000 / (1 << value);
+	rd->time_unit = 1000000 / BIT(value);
 
 	pr_debug("Atom %s:%s energy=%dpJ, time=%dus, power=%duW\n",
 		 rd->rp->name, rd->name, rd->energy_unit, rd->time_unit, rd->power_unit);
@@ -1102,8 +1102,8 @@ static void set_floor_freq_atom(struct rapl_domain *rd, bool enable)
 			      &power_ctrl_orig_val);
 	mdata = power_ctrl_orig_val;
 	if (enable) {
-		mdata &= ~(0x7f << 8);
-		mdata |= 1 << 8;
+		mdata &= ~GENMASK(14, 8);
+		mdata |= BIT(8);
 	}
 	iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_CR_WRITE,
 		       defaults->floor_freq_reg_addr, mdata);
@@ -1136,7 +1136,7 @@ static u64 rapl_compute_time_window_core(struct rapl_domain *rd, u64 value,
 		if (y > 0x1f)
 			return 0x7f;
 
-		f = div64_u64(4 * (value - (1ULL << y)), 1ULL << y);
+		f = div64_u64(4 * (value - BIT_ULL(y)), BIT_ULL(y));
 		value = (y & 0x1f) | ((f & 0x3) << 5);
 	}
 	return value;
@@ -1169,13 +1169,13 @@ static int rapl_check_unit_tpmi(struct rapl_domain *rd)
 	}
 
 	value = (ra.value & TPMI_ENERGY_UNIT_MASK) >> TPMI_ENERGY_UNIT_OFFSET;
-	rd->energy_unit = ENERGY_UNIT_SCALE * 1000000 / (1 << value);
+	rd->energy_unit = ENERGY_UNIT_SCALE * 1000000 / BIT(value);
 
 	value = (ra.value & TPMI_POWER_UNIT_MASK) >> TPMI_POWER_UNIT_OFFSET;
-	rd->power_unit = 1000000 / (1 << value);
+	rd->power_unit = 1000000 / BIT(value);
 
 	value = (ra.value & TPMI_TIME_UNIT_MASK) >> TPMI_TIME_UNIT_OFFSET;
-	rd->time_unit = 1000000 / (1 << value);
+	rd->time_unit = 1000000 / BIT(value);
 
 	pr_debug("Core CPU %s:%s energy=%dpJ, time=%dus, power=%duW\n",
 		 rd->rp->name, rd->name, rd->energy_unit, rd->time_unit, rd->power_unit);
-- 
2.43.0
Re: [PATCH v1 3/9] powercap: intel_rapl: Use GENMASK() and BIT() macros
Posted by David Laight 1 week, 3 days ago 
On Thu, 29 Jan 2026 10:36:40 -0800
Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:

> Replace hardcoded bitmasks and bit shift operations with standard
> GENMASK(), GENMASK_ULL(), BIT(), and BIT_ULL() macros for better
> readability and to follow kernel coding conventions.
> 
> No functional changes.

Assuming that changing values to 'unsigned long' doesn't have any
subtle side effects.

...
>  	value = (ra.value & ENERGY_UNIT_MASK) >> ENERGY_UNIT_OFFSET;
> -	rd->energy_unit = ENERGY_UNIT_SCALE * 1000000 / (1 << value);
> +	rd->energy_unit = ENERGY_UNIT_SCALE * 1000000 / BIT(value);

That should really be:
	rd->energy_unit = ENERGY_UNIT_SCALE * 1000000 >> value;

While using BIT() for bit patterns is resonable, wholesale substition
isn't really right - and that isn't a bit pattern.

	David
Re: [PATCH v1 3/9] powercap: intel_rapl: Use GENMASK() and BIT() macros
Posted by Kuppuswamy Sathyanarayanan 1 week, 3 days ago 

On 1/29/2026 1:52 PM, David Laight wrote:
> On Thu, 29 Jan 2026 10:36:40 -0800
> Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> 
>> Replace hardcoded bitmasks and bit shift operations with standard
>> GENMASK(), GENMASK_ULL(), BIT(), and BIT_ULL() macros for better
>> readability and to follow kernel coding conventions.
>>
>> No functional changes.
> 
> Assuming that changing values to 'unsigned long' doesn't have any
> subtle side effects.
> 
> ...
>>  	value = (ra.value & ENERGY_UNIT_MASK) >> ENERGY_UNIT_OFFSET;
>> -	rd->energy_unit = ENERGY_UNIT_SCALE * 1000000 / (1 << value);
>> +	rd->energy_unit = ENERGY_UNIT_SCALE * 1000000 / BIT(value);
> 
> That should really be:
> 	rd->energy_unit = ENERGY_UNIT_SCALE * 1000000 >> value;

Type change to unsigned long will not have any impact for this use case. 
However, as you noted, using right shift (>>) instead of division by 
BIT() is the correct approach here anyway.

> 
> While using BIT() for bit patterns is resonable, wholesale substition
> isn't really right - and that isn't a bit pattern.

Agreed. I will use right shift for 1 / (1 << value) use cases.


> 
> 	David

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.