STM32: Replace HAL register operations #97329

gautierg-st · 2025-10-10T08:22:30Z

This PR replaces the STM32 HAL register operations macros (such as WRITE_REG or MODIFY_REG) and functions (such as LL_I2C_WriteReg) by Zephyr equivalents in all STM32 drivers and SoC.

gautierg-st · 2025-10-10T08:29:04Z

Waiting for test results to unblock.

etienne-lms

I wonder if all the casts the mem_addr_t could be hidden some way with helper macros or inline functions. Outside of the scope of this P-R.

include/zephyr/sys/sys_io.h

include/zephyr/arch/common/sys_bitops.h

drivers/adc/adc_stm32.c

drivers/adc/adc_stm32wb0.c

soc/st/stm32/stm32h7x/soc_m7.c

drivers/hwinfo/hwinfo_stm32.c

gautierg-st · 2025-10-21T09:01:51Z

New version of the PR: I removed the new functions in sys_bitops.h and instead created a new file in soc/stm32 to add all bitops functions. These new functions use Zephyr bitops functions when possible and hides the cast to mem_addr_t that was otherwise required everywhere. If new system functions are added in the future, they can easily be used by just updating this new file.

drivers/adc/adc_stm32.c

mathieuchopstm · 2025-10-21T09:03:24Z

drivers/adc/adc_stm32wb0.c

 	const uint32_t shift = 4 * (Conversion & 7);

-	MODIFY_REG((&ADCx->SEQ_1)[reg], ADC_SEQ_1_SEQ0 << shift, Channel << shift);
+	stm32_modify_reg(&(&ADCx->SEQ_1)[reg], ADC_SEQ_1_SEQ0 << shift, Channel << shift);


This should be equivalent but more readable:

Suggested change

stm32_modify_reg(&(&ADCx->SEQ_1)[reg], ADC_SEQ_1_SEQ0 << shift, Channel << shift);

stm32_modify_reg((&ADCx->SEQ_1) + reg, ADC_SEQ_1_SEQ0 << shift, Channel << shift);

Outside PR scope and I'd have to check ASM but it might just be simpler to do:

const volatile uint32_t *reg = (Conversion & 8) ? &ADCx->SEQ_2 : &ADCx->SEQ_1;

Edit: no it isn't (-2/4 cycles due to jumps)

mathieuchopstm · 2025-10-21T09:07:24Z

soc/st/stm32/common/stm32_bitops.h

+#include <zephyr/arch/common/sys_bitops.h>
+#include <zephyr/arch/common/sys_io.h>
+
+static ALWAYS_INLINE void stm32_set_mask(volatile uint32_t *addr, uint32_t mask)


I would suggest stm32_set_bits for consistency.

Since this is dedicated to 32bit SoC registers (pointers to 32bit cells):
stm32_reg_set_bits() / stm32_reg_clear_bits() / stm32_reg_read_masked() / stm32_reg_read() / ... ?

I chose stm32_reg_write / stm32_reg_read / stm32_reg_set_bits / stm32_reg_clear_bits / stm32_reg_read_bits / stm32_reg_modify_bits

mathieuchopstm · 2025-10-21T09:07:39Z

soc/st/stm32/common/stm32_bitops.h

+	sys_set_bits((mem_addr_t)addr, mask);
+}
+
+static ALWAYS_INLINE void stm32_clear_mask(volatile uint32_t *addr, uint32_t mask)


I would suggest stm32_clear_bits for consistency.

mathieuchopstm · 2025-10-21T09:08:05Z

soc/st/stm32/common/stm32_bitops.h

+
+static ALWAYS_INLINE uint32_t stm32_read_mask(volatile uint32_t *addr, uint32_t mask)
+{
+	return (*addr) & mask;


Use the sys_read32 primitive:

Suggested change

return (*addr) & mask;

return sys_read32((mem_addr_t)addr) & mask;

mathieuchopstm · 2025-10-21T09:12:27Z

soc/st/stm32/common/stm32_bitops.h

+	sys_clear_bits((mem_addr_t)addr, mask);
+}
+
+static ALWAYS_INLINE uint32_t stm32_read_mask(volatile uint32_t *addr, uint32_t mask)


I would suggest stm32_read_bits/stm32_read_masked or something that says the read value is masked, rather than "we are reading a mask".

mathieuchopstm · 2025-10-21T09:18:54Z

soc/st/stm32/common/stm32_bitops.h

+static ALWAYS_INLINE bool stm32_test_mask(volatile uint32_t *addr, uint32_t mask)
+{
+	return stm32_read_mask(addr, mask) == mask;
+}


This works fine for single-bit fields but is a disaster waiting to happen for multi-bit fields.

Proposal:

Suggested change

static ALWAYS_INLINE bool stm32_test_mask(volatile uint32_t *addr, uint32_t mask)

{

return stm32_read_mask(addr, mask) == mask;

}

/**

* Test if one bit in the value at @p addr is set.

*

* @param addr MMIO address to read

* @param bit_mask Mask with bit to test set and all others clear

*/

static ALWAYS_INLINE bool stm32_test_bit(volatile uint32_t *addr, uint32_t bit_mask)

{

return stm32_read_bits(addr, bit_mask) == bit_mask;

}

For multi-bit fields, another helper should be introduced that consumed the mask too, i.e.:

bool stm32_test_bits(vu32 *addr, u32 field_mask, u32 value) { return stm32_read_bits(addr, field_mask) == value; }

A function named stm32_test_bit should IMO take a bit argument rather than a bit_mask. After all, that's what all sys_xxx_bit functions do.
Agree about taking a mask and a value to compare to for multi-bit fields.

I think there is no real consensus on such a test-multi-bits-are-set helper function.
In the end, I one is to call if (stm32_test_maskedxxx(ptr, mask, expected_masked_value)),
I think one would prefer an explicit if (stm32_read_masked(ptr, mask) == expected_masked_value)).
(at least I would :-)

Removed the test function, and fixed the code everywhere it is needed with an explicit boolean test.

mathieuchopstm · 2025-10-21T09:20:07Z

soc/st/stm32/common/stm32_bitops.h

+	uint32_t temp = *addr;
+
+	*addr = (temp & ~mask) | value;


Use sys_{read,write}32():

Suggested change

uint32_t temp = *addr;

*addr = (temp & ~mask) | value;

const mem_addr_t memaddr = (mem_addr_t)addr;

uint32_t temp = sys_read32(mem_addr);

sys_write32((temp & ~mask) | value, mem_addr);

soc/st/stm32/common/stm32_bitops.h

drivers/flash/flash_stm32h7x.c

drivers/usb_c/tcpc/ucpd_stm32.c

drivers/hwinfo/hwinfo_stm32.c

drivers/i2c/i2c_ll_stm32_v2.c

drivers/usb_c/tcpc/ucpd_stm32.c

drivers/adc/adc_stm32.c

drivers/entropy/entropy_stm32.c

soc/st/stm32/stm32h7x/soc_m7.c

etienne-lms

A few review comments that may be addressed later but an issue to fix in drivers/adc/adc_stm32.c set_reg_value().

soc/st/stm32/common/stm32_bitops.h

drivers/clock_control/clock_stm32_ll_mp13.c

drivers/flash/flash_stm32_ospi.c

drivers/usb_c/tcpc/ucpd_stm32.c

drivers/adc/adc_stm32.c

gautierg-st · 2025-10-24T07:17:22Z

Rebased to fix conflicts.

etienne-lms

LAGTM

mathieuchopstm · 2025-10-24T11:23:18Z

drivers/adc/adc_stm32.c

-
-	uintptr_t addr = (uintptr_t)adc + reg;
+	size_t reg32_offset = reg / sizeof(uint32_t);
+	uint32_t *addr = (uint32_t *)config->base + reg32_offset;


Suggested change

uint32_t *addr = (uint32_t *)config->base + reg32_offset;

volatile uint32_t *addr = (volatile uint32_t *)config->base + reg32_offset;

This should have raised a compiler warning (I didn't check if it did or not)

+ outside PR scope but now that the STM32 helper functions exist, maybe we don't need these {get,set}_reg_value() helpers?

This should have raised a compiler warning (I didn't check if it did or not)

The many places where stm32_reg_xxx() are used use a pointer argument that is not an explicit volatile data pointer. I don't think it's needed here. Actaully, I don't think the address argument of these helper functions need to be volatile. The cast is done in the sys_xxx() functions and that should be enough.

mathieuchopstm · 2025-10-24T11:27:35Z

soc/st/stm32/common/stm32_bitops.h

+#include <zephyr/arch/common/sys_bitops.h>
+#ifdef CONFIG_CPU_CORTEX_M
+#include <zephyr/arch/common/sys_io.h>
+#elif defined(CONFIG_CPU_AARCH32_CORTEX_R) || defined(CONFIG_CPU_AARCH32_CORTEX_A)
+#include <zephyr/arch/arm/cortex_a_r/sys_io.h>
+#endif


Use the generic header:

Suggested change

#include <zephyr/arch/common/sys_bitops.h>

#ifdef CONFIG_CPU_CORTEX_M

#include <zephyr/arch/common/sys_io.h>

#elif defined(CONFIG_CPU_AARCH32_CORTEX_R) || defined(CONFIG_CPU_AARCH32_CORTEX_A)

#include <zephyr/arch/arm/cortex_a_r/sys_io.h>

#endif

#include <zephyr/arch/cpu.h>

soc/st/stm32/common/stm32_bitops.h

Add a set of bitops functions in order to replace the STM32 HAL bitops macros throughout the drivers. Signed-off-by: Guillaume Gautier <[email protected]>

For all STM32 drivers, replace the SET_BIT macro (defined in the STM32 HAL) by stm32_reg_set_bits defined in Zephyr. Signed-off-by: Guillaume Gautier <[email protected]>

For all STM32 drivers, replace the CLEAR_BIT macro (defined in the STM32 HAL) by stm32_reg_clear_bits defined in Zephyr. Signed-off-by: Guillaume Gautier <[email protected]>

For all STM32 drivers and SoC, replace the READ_BIT macro (defined in the STM32 HAL) by stm32_reg_read_bits. Fixes some cases where the return value was tested like a boolean despite not being one. Signed-off-by: Guillaume Gautier <[email protected]>

For all STM32 drivers and SoC, replace the WRITE_REG macro and the LL_xxx_WriteReg functions (defined in the STM32 HAL) by stm32_reg_write defined in Zephyr. Signed-off-by: Guillaume Gautier <[email protected]>

For all STM32 drivers and SoC, replace the READ_REG macro and the LL_xxx_ReadReg functions (defined in the STM32 HAL) by stm32_reg_read defined in Zephyr. Signed-off-by: Guillaume Gautier <[email protected]>

For all STM32 drivers and SoC, replace the MODIFY_REG macro (defined in the STM32 HAL) by stm32_reg_modify_bits defined in Zephyr. Signed-off-by: Guillaume Gautier <[email protected]>

Call stm32_reg_set_bits instead of stm32_reg_read then stm32_reg_write. Signed-off-by: Guillaume Gautier <[email protected]>

Use dedicated set/clear bits function instead of reading/masking/writing manually. Signed-off-by: Guillaume Gautier <[email protected]>

etienne-lms

Still LGTM.

sonarqubecloud · 2025-10-24T12:44:40Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

gautierg-st changed the title ~~Remove hal reg op~~ STM32: Replace HAL register operations Oct 10, 2025

zephyrbot requested review from FRASTM, GeorgeCGV, MaureenHelm, avisconti, djiatsaf-st, erwango, etienne-lms, jeppenodgaard, marwaiehm-st, mathieuchopstm, nordic-krch, teburd, tristan-google, ubieda and yperess October 10, 2025 08:24

zephyrbot assigned nordic-krch Oct 10, 2025

gautierg-st added the block: HW Test Testing on hardware required before merging label Oct 10, 2025

etienne-lms reviewed Oct 10, 2025

View reviewed changes

teburd assigned erwango and unassigned nordic-krch Oct 10, 2025

teburd added the platform: STM32 ST Micro STM32 label Oct 10, 2025

gautierg-st force-pushed the remove_hal_reg_op branch from 7b6ab32 to c3ab6e2 Compare October 10, 2025 13:38

zephyrbot requested review from alexanderwachter, andyross, bjarki-andreasen, ceolin, danieldegrasse and dcpleung October 10, 2025 13:40

gautierg-st force-pushed the remove_hal_reg_op branch 2 times, most recently from 627e39b to a19ec9e Compare October 21, 2025 08:58

gautierg-st force-pushed the remove_hal_reg_op branch from a19ec9e to 9b9e650 Compare October 21, 2025 09:27

mathieuchopstm reviewed Oct 21, 2025

View reviewed changes

gautierg-st force-pushed the remove_hal_reg_op branch 2 times, most recently from 62a4992 to 3658a6a Compare October 22, 2025 11:48

etienne-lms reviewed Oct 22, 2025

View reviewed changes

gautierg-st force-pushed the remove_hal_reg_op branch from 3658a6a to f97903d Compare October 23, 2025 09:35

etienne-lms requested changes Oct 23, 2025

View reviewed changes

gautierg-st force-pushed the remove_hal_reg_op branch 2 times, most recently from a5f466b to ea37e79 Compare October 24, 2025 07:17

etienne-lms previously approved these changes Oct 24, 2025

View reviewed changes

erwango requested review from mathieuchopstm and peter-mitsis October 24, 2025 08:53

mathieuchopstm reviewed Oct 24, 2025

View reviewed changes

gautierg-st added 9 commits October 24, 2025 14:27

soc: st: stm32: common: add common bitops functions

8838493

Add a set of bitops functions in order to replace the STM32 HAL bitops macros throughout the drivers. Signed-off-by: Guillaume Gautier <[email protected]>

drivers: stm32: replace SET_BIT HAL macro by stm32_reg_set_bits

76f7f0a

For all STM32 drivers, replace the SET_BIT macro (defined in the STM32 HAL) by stm32_reg_set_bits defined in Zephyr. Signed-off-by: Guillaume Gautier <[email protected]>

drivers: stm32: replace CLEAR_BIT HAL macro by stm32_reg_clear_bits

08c9ecc

For all STM32 drivers, replace the CLEAR_BIT macro (defined in the STM32 HAL) by stm32_reg_clear_bits defined in Zephyr. Signed-off-by: Guillaume Gautier <[email protected]>

drivers: stm32: replace WRITE_REG HAL macro by stm32_reg_write

17f39f2

For all STM32 drivers and SoC, replace the WRITE_REG macro and the LL_xxx_WriteReg functions (defined in the STM32 HAL) by stm32_reg_write defined in Zephyr. Signed-off-by: Guillaume Gautier <[email protected]>

drivers: stm32: replace READ_REG HAL macro by stm32_reg_read

b410706

For all STM32 drivers and SoC, replace the READ_REG macro and the LL_xxx_ReadReg functions (defined in the STM32 HAL) by stm32_reg_read defined in Zephyr. Signed-off-by: Guillaume Gautier <[email protected]>

drivers: stm32: replace MODIFY_REG HAL macro by stm32_reg_modify_bits

5a5a89a

For all STM32 drivers and SoC, replace the MODIFY_REG macro (defined in the STM32 HAL) by stm32_reg_modify_bits defined in Zephyr. Signed-off-by: Guillaume Gautier <[email protected]>

drivers: i2c: stm32: v2: simplify reg operation

eb9ad57

Call stm32_reg_set_bits instead of stm32_reg_read then stm32_reg_write. Signed-off-by: Guillaume Gautier <[email protected]>

drivers: usb_c: tcpc: stm32: use dedicated bitops functions

b18ff3c

Use dedicated set/clear bits function instead of reading/masking/writing manually. Signed-off-by: Guillaume Gautier <[email protected]>

gautierg-st dismissed etienne-lms’s stale review via b18ff3c October 24, 2025 12:28

gautierg-st force-pushed the remove_hal_reg_op branch from ea37e79 to b18ff3c Compare October 24, 2025 12:28

etienne-lms approved these changes Oct 24, 2025

View reviewed changes

	stm32_modify_reg(&(&ADCx->SEQ_1)[reg], ADC_SEQ_1_SEQ0 << shift, Channel << shift);
	stm32_modify_reg((&ADCx->SEQ_1) + reg, ADC_SEQ_1_SEQ0 << shift, Channel << shift);

	return (*addr) & mask;
	return sys_read32((mem_addr_t)addr) & mask;

-static ALWAYS_INLINE bool stm32_test_mask(volatile uint32_t *addr, uint32_t mask)
-{
-	return stm32_read_mask(addr, mask) == mask;
-}
+/**
+ * Test if one bit in the value at @p addr is set.
+ *
+ * @param addr MMIO address to read
+ * @param bit_mask Mask with bit to test set and all others clear
+ */
+static ALWAYS_INLINE bool stm32_test_bit(volatile uint32_t *addr, uint32_t bit_mask)
+{
+	return stm32_read_bits(addr, bit_mask) == bit_mask;
+}

-	uint32_t temp = *addr;
-	*addr = (temp & ~mask) | value;
+        const mem_addr_t memaddr = (mem_addr_t)addr;
+	uint32_t temp = sys_read32(mem_addr);
+	sys_write32((temp & ~mask) | value, mem_addr);

	uint32_t addr = (uint32_t )config->base + reg32_offset;
	volatile uint32_t addr = (volatile uint32_t )config->base + reg32_offset;

STM32: Replace HAL register operations #97329

Are you sure you want to change the base?

STM32: Replace HAL register operations #97329

Conversation

gautierg-st commented Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gautierg-st commented Oct 10, 2025

Uh oh!

etienne-lms left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gautierg-st commented Oct 21, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mathieuchopstm Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

etienne-lms left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gautierg-st commented Oct 24, 2025

Uh oh!

gautierg-st commented Oct 10, 2025 •

edited

Loading

mathieuchopstm Oct 21, 2025 •

edited

Loading