Skip to content

drivers: stm32_temp stm32h5 device must disable icache to access cal #57178

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 2, 2023

Conversation

tu-brian
Copy link
Contributor

@tu-brian tu-brian commented Apr 24, 2023

Reading the temperature calibration data requires disabling the icache of the stm32h5x mcu.
Else a bus fault error occurs reading Address: 0x8fff814-0x8fff818.
Enable afterwards.

Reading the temperature calibration data requires disabling the icache
of the stm32h5x mcu.
Else a bus fault error occurs reading Address: 0x8fff8014-0x8fff818
Enable afterwards.

Signed-off-by: Brian Juel Folkmann <[email protected]>
@tu-brian tu-brian force-pushed the fix_stm32_tempsensor branch from 439b5f6 to 57bbce3 Compare April 24, 2023 10:29
@erwango erwango assigned gautierg-st and unassigned erwango Apr 25, 2023
FRASTM
FRASTM previously approved these changes Apr 26, 2023
#if defined(CONFIG_SOC_SERIES_STM32H5X)
LL_ICACHE_Enable();
#endif /* CONFIG_SOC_SERIES_STM32H5X */

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it should be enabled, but... shouldn't be restored to original state? (i.e. enabled if was enabled, but disable if it was disabled)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I Get your point, but for this fix I simply ported what was done in 8d5ff8c to here. When searching the codebase for LL_CACHE I can't find any other place where the check before enable is implemented

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I Get your point, but for this fix I simply ported what was done in 8d5ff8c to here. When searching the codebase for LL_CACHE I can't find any other place where the check before enable is implemented

Ok I see. Sorry for the noise. By the way, another point... This is instruction cache, so what does it mean reading 0x8fff814-0x8fff818 addresses? Is it some code fetched from that location? Can you comment about it a little bit more in the commit header? Sorry again if the question is pointless...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@FRASTM Can you please explain why the fix you did in 8d5ff8c solved the bus error on reading the read-only area?

Copy link
Collaborator

@FRASTM FRASTM May 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@FRASTM Can you please explain why the fix you did in 8d5ff8c solved the bus error on reading the read-only area?

If I remember, I got the idea from https://github.com/STMicroelectronics/STM32CubeH5/tree/main/Projects/NUCLEO-H563ZI/Examples_LL/UTILS/UTILS_ReadDeviceInfo

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(For future readers)

On STM32H5, the ICACHE block is interposed on C-bus between Cortex-M33 and FLASH. (See RM0492 §2.1.1 Fast C-bus)

The ICACHE determines whether an access is cacheable or not based on the AHB attribute, which depends on MPU configuration. (§8.4.6 Cacheable and non-cacheable traffic).

When a cacheable transaction is received from the Cortex-M33, if the requested data is not present in cache, a cache miss occurs and a cache line refill (128-bit access via burst) is performed (§8.4.7 Cacheable accesses).

However, all accesses to OTP and RO regions must be done with caching disabled (§7.3.2 FLASH signals); indeed, accesses to this region must be 16 or 32-bit sized, or a bus error is raised (§7.5.9 One-time-programmable and read-only memory protections - table 38 OTP/RO access constraints).

By disabling the ICACHE, we ensure the manufacturing (RO) flash region is accessed by 16/32-bit size as it should.

@gautierg-st
Copy link
Collaborator

How did you test that? die_temp is not present (yet) in stm32h5 dtsi. I suppose you have added it to detect this problem but we can't reproduce without it. Could you add it to your PR?

@tu-brian
Copy link
Contributor Author

How did you test that? die_temp is not present (yet) in stm32h5 dtsi. I suppose you have added it to detect this problem but we can't reproduce without it. Could you add it to your PR?

I tested the implementation on our own board, with the required dts entry added.
For the sake of this PR, I can add a support to samples/sensor/die_temp_polling. Will that be sufficient?

@gautierg-st
Copy link
Collaborator

Support for die_temp should be added directly in the stm32h5.dtsi like it is usually done, and the node should be enabled and the alias created on at least one H5 board (I have the stm32h573i_dk to test). If you could add that, I will be able to test on my side.

Copy link
Collaborator

@gautierg-st gautierg-st left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested your PR, and it is indeed necessary to disable the icache to read the calibration values.
Minor change requested, otherwise LGTM.
Thanks


&die_temp {
status = "okay";
};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you move that directly in the stm32h573i_dk.dts?
Ideally create a separate commit to have one commit for the dtsi and another for the dts, but not required.

Add die temp sensor to stm32h5 series.

Signed-off-by: Brian Juel Folkmann <[email protected]>
@tu-brian tu-brian force-pushed the fix_stm32_tempsensor branch from 8eeabfb to 2839da2 Compare April 27, 2023 13:02
Enable die temp sensor on stm32h573i_dk

Signed-off-by: Brian Juel Folkmann <[email protected]>
@tu-brian tu-brian force-pushed the fix_stm32_tempsensor branch from 2839da2 to e36870e Compare April 27, 2023 13:13
@carlescufi carlescufi merged commit 58088f3 into zephyrproject-rtos:main May 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Sensors Sensors platform: STM32 ST Micro STM32
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants