Skip to content

drivers: entropy: Fix non-stop RNG ISR firing for STM32WB09 #98151

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+21 −4
Open

drivers: entropy: Fix non-stop RNG ISR firing for STM32WB09 #98151

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5beef82
drivers: entropy: Fix non-stop RNG ISR firing for STM32WB09
HoZHel Oct 23, 2025
cd96302
drivers: bluetooth: hci: Fix the issue about the TRNG peripheral inst…
HoZHel Oct 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion drivers/bluetooth/hci/hci_stm32wb0.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -343,7 +343,7 @@ static int ble_pm_action(const struct device *dev,

static void rng_get_random(void *num, size_t size)
{
const struct device *dev = DEVICE_DT_GET(DT_DRV_INST(0));
const struct device *dev = DEVICE_DT_GET(DT_CHOSEN(zephyr_entropy));
int res;

/* try to allocate from pool */
Expand Down
16 changes: 14 additions & 2 deletions drivers/entropy/entropy_stm32.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,12 @@
#define STM32_CONDRST_SUPPORT
#endif

#if IRQLESS_TRNG
#define MAX_AVAIL_RND_NUM 1
#else
#define MAX_AVAIL_RND_NUM 4 /* Internal four-word FIFO */
Copy link
Contributor

Choose a reason for hiding this comment

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

When an error is triggered only 3 samples are to be removed from the 4-word FIFO.
If there is no FIFO, there is nothing to evacuate since upon error RNG produces no random sample.

That said, the change shows something that was not update by commit d65f8e3: indeed value 12 should have been updated to 3 at that time and no-FIFO instance addressed.

Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't really matter, reading empty FIFO will just return 0.

Copy link
Contributor

@mathieuchopstm mathieuchopstm Oct 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

And FWIW other IPs do also have a FIFO but no corresponding "FIFO full" interrupt so I'm not sure that lowering this value is "correct"

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think lowering the value will fix anything. Just a matter of consistency.

#endif /* IRQLESS_TRNG */

/*
* This driver need to take into account all STM32 family:
* - simple rng without hardware fifo and no DMA.
Expand Down Expand Up @@ -327,10 +333,16 @@ static int recover_seed_error(RNG_TypeDef *rng)
{
ll_rng_clear_seis(rng);

for (int i = 0; i < 12; ++i) {
for (int i = 0; i < MAX_AVAIL_RND_NUM; ++i) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this should be replaced with:

Suggested change
for (int i = 0; i < MAX_AVAIL_RND_NUM; ++i) {
while (ll_rng_is_active_drdy(rng)) {

to be more solid

(void)ll_rng_read_rand_data(rng);
}

#if defined(CONFIG_SOC_STM32WB09XX)
if (LL_RNG_GetErrorIrq(rng)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 on Etienne's comment: unless there's a reason for it, let's not bother checking for the flags.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean clearing a flag without checking if it is raised?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. Per RefMan, this should have no effect

SET_BIT(rng->IRQ_SR, RNG_IRQ_SR_ERROR_IRQ);
Copy link
Contributor

Choose a reason for hiding this comment

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

The flag should have already been cleared by ll_rng_clear_sei() above.
Using SET_BIT() here may also clear the FF_FULL flag.

Copy link
Contributor

Choose a reason for hiding this comment

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

ll_rng_clear_seis() doesn't clear IRQ_SR_ERROR 🤔

Copy link
Contributor Author

@HoZHel HoZHel Oct 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@etienne-lms , ll_rng_clear_seis() clears RNG_CR_RST_HEALTH_FLAGS flag.
For your second sentence, please refer to the previous comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

My mistake. Indeed it is an error, we should clear that bit.
Could you rather fix ll_rng_clear_sei() adding their clear of IRQ_SR_ERROR?

I think we need to revisit a bit WB09xx sequences in this driver.

Copy link
Member

@erwango erwango Oct 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Please use new bits handling functions: #97329

}
#endif /* CONFIG_SOC_STM32WB09XX */

if (ll_rng_is_active_seis(rng) != 0) {
return -EIO;
}
Expand Down Expand Up @@ -445,7 +457,7 @@ static uint16_t generate_from_isr(uint8_t *buf, uint16_t len)
ret = random_sample_get(&rnd_sample);
#if !IRQLESS_TRNG
NVIC_ClearPendingIRQ(IRQN);
#endif /* IRQLESS_TRNG */
#endif /* !IRQLESS_TRNG */

if (ret < 0) {
continue;
Expand Down
4 changes: 3 additions & 1 deletion drivers/entropy/entropy_stm32.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,9 @@ static inline rng_sample_t ll_rng_read_rand_data(RNG_TypeDef *RNGx)
* Raw register access is performed because STM32CubeWB0 v1.0.0
* package is lacking the LL function to clear IRQ flags.
*/
WRITE_REG(RNG->IRQ_SR, RNG_IRQ_SR_FF_FULL_IRQ);
if (LL_RNG_GetFfFullIrq(RNGx)) {
SET_BIT(RNGx->IRQ_SR, RNG_IRQ_SR_FF_FULL_IRQ);
Copy link
Contributor

Choose a reason for hiding this comment

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

Using SET_BIT(), you may clear also a pending ERROR flag. I think WRITE_REG() was the way to go.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1: WRITE_REG is the correct operation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What you're saying is the opposite. As the name suggests, SET_BIT() is for setting one bit of the register, while WRITE_REG() manipulates the whole register. Here you can find these two macros as follows:

#define SET_BIT(REG, BIT) ((REG) |= (BIT))
#define WRITE_REG(REG, VAL) ((REG) = (VAL))

Copy link
Contributor

Choose a reason for hiding this comment

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

I know what the macros do. SET_BIT() is not correct here because it does a read then write, and the flags are cleared by writing 1 to their bit:
image

If ERROR_IRQ was 1, SET_BIT(IRQ_SR, FF_FULL_IRQ) will read the register (= ERROR_IRQ), then |= FF_FULL_IRQ to the read value and write back the result (= ERROR_IRQ | FF_FULL_IRQ) which will clear both interrupt flags.

Using WRITE_REG (or really, just RNG->IRQ_SR = RNG_IRQ_SR_FF_FULL_IRQ) will write a value with only bit FF_FULL_IRQ set which ensures that only this specific interrupt flag is cleared.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now I understand, thanks for the clarification.

}

return rnd;
#elif defined(CONFIG_SOC_SERIES_STM32WB0X)
Expand Down
3 changes: 3 additions & 0 deletions soc/st/stm32/stm32wb0x/Kconfig.defconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@ endif # STM32WB0_RADIO_TIMER

if BT

config SYSTEM_WORKQUEUE_STACK_SIZE
default 1152
Comment on lines +23 to +24
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
config SYSTEM_WORKQUEUE_STACK_SIZE
default 1152
configdefault SYSTEM_WORKQUEUE_STACK_SIZE
default 1152


config BT_AUTO_PHY_UPDATE
default n

Expand Down