STMicroelectronics / STM32CubeF7

STM32Cube MCU Full Package for the STM32F7 series - (HAL + LL Drivers, CMSIS Core, CMSIS Device, MW libraries plus a set of Projects running on all boards provided by ST (Nucleo, Evaluation and Discovery Kits))
Other
322 stars 191 forks source link

Questionable logic when setting the number of flash wait states #24

Closed hjuul closed 3 years ago

hjuul commented 3 years ago

When setting the number of flash wait states, the following condition must be true to be able arrive at the highest wait states:

https://github.com/STMicroelectronics/STM32CubeF7/blob/08376dce1b404687e4a86b73077f396bccfc9cb5/Drivers/STM32F7xx_HAL_Driver/Src/stm32f7xx_ll_utils.c#L481

So if this function is called with e.g. 216MHz as input but overdrive mode disabled, it will end up with LL_FLASH_LATENCY_5.

Although the input conditions would be invalid, both of the following solutions would be better, IMHO:

The same reasoning could be applied to the voltage scaling condition in the same function.

ASELSTM commented 3 years ago

Hi @hjuul,

First thank you for your contribution.

The over-drive mode allows the CPU and the core logic to operate at a higher frequency than the normal mode for the voltage scaling scale 1 and scale 2.

Indeed, in order to reach an HCLK frequancy equal to 216MHz witch is the maximum frequency, the over driver mode must be activated. This is described in the Table 18. General operating conditions within the datasheet.

Thus, since the input of the function UTILS_SetFlashLatency is the HCLK_Frequency and for a frequency equal to 216MHz, the function will end up with LL_FLASH_LATENCY_7 since the over drive mode is imperatively activated to reach this frequency.

Therefore, the following part of the code is accessible only when the drive mode is active given that these HCLK frequencies are only reached when this mode is activated. https://github.com/STMicroelectronics/STM32CubeF7/blob/08376dce1b404687e4a86b73077f396bccfc9cb5/Drivers/STM32F7xx_HAL_Driver/Src/stm32f7xx_ll_utils.c#L481-L493

Hope that this will answer your question. With regards,

hjuul commented 3 years ago

I understand that LATENCY_6 and LATENCY_7 should only be used when overdrive mode is enabled. But I don't think this function should assume that the caller actually did that.

In my opinion, if the caller wants 216MHz but hasn't enabled overdrive mode (and the correct voltage scale) he should get an error, not LATENCY_5.

Have a look at this related Zephyr issue. Zephyr calls UTILS_SetFlashLatency() with 216MHz without having enabled overdrive mode. If this function had returned an error on that condition, that bug would have been detected during development.

ASELSTM commented 3 years ago

Hi @hjuul,

To ensure a proper use of the UTILS_SetFlashLatency function, correct parameters must be passed. Indeed, you have to pass the real value of HCLK_Frequency used by your system. In order to calculate the exact value of HCL_Frequancy, you may use the macro __LL_RCC_CALC_HCLK_FREQ.

Would you please try to use this macro to calculate the appropriate value of the HCLK_Frequancy and test back LL_SetFlashLatency function with the HCLK_Frequancy returned by the macro.

With regards,

hjuul commented 3 years ago

Never mind. I know it will work if I provide the correct frequency, I was just suggesting you raise an error if I don't. Feel free to close the issue if you don't want this function to handle such errors.