stm32h7: allow definition of HSI divider in board config

[slorquet]

Summary

nucleo-h743zi is the only instanciation of stm32h7, and this board uses the HSE oscillator provided by the stlink interface.
If the integrated stlink debugger is disabled (this can happen in certain power configurations) then the CPU has no more clock reference. The HSI can be used instead, but definitions to do so easily are currently lacking.

When the HSI is used, the HSI divider is hardwired to 4, which results in a 16 MHz reference clock, and this requires a full recomputation of all PLL constants to match the complex clock tree speeds.

This commit adds the ability to choose the HSI divider, which must be indicated in board.h .
This is useful to match the HSI frequency to the existing HSE config, eg 8 MHz.
This allows to use the exact same PLL configuration and minimizes code changes.

Impact

None expected if projects based on STM32H7 continue to use HSE.
This is new code that only applies to users of STM32H7 that want to use the HSI oscillator. To my knowledge there is none but you never know.

Testing

In theory, all projects using STM32H7 should be retested. No regression is expected.

[davids5]

See inline

[slorquet]

Excuse me, how is that a breaking change?

[davids5]

It will break non-intree builds - Breaking change says:This change requires a mitigation entry in the release notes.

[slorquet]

But what do you understand will break? This is a sincere question here. I dont understand the issue.

[davids5]

If STM32_BOARD_USEHSI is declared in an out of tree build, the build will fail. Yes it has a message - that is great, but there have been very stern complaints about undocumented changes that break the build. This tag make sure the release notes have a mention.

[slorquet]

With the current code, if any board external or internal defines STM32_BOARD_USEHSI, the board will hang as soon as the code to configure the PLL is executed. It just does not work.

Thats why I believe that no external project is actually defining USEHSI. Otherwise, the bug would have been found already.

I think a build error is preferable to a runtime hang with no obvious explanation.

[davids5]

@slorquet Please fix the CI failures

[slorquet]

It seems that some jobs were cancelled when you requested changes. will these restart automatically?

[davids5]

That is not how it works.... The cancel is caused because of the real build failure https://github.com/apache/incubator-nuttx/runs/6472207049?check_suite_focus=true

Fixed it, do a fixup (rebase -i master and either squash (s) it or do fixup (f) and force push - that will start CI again.

Need Fixup

[slorquet]

Sorry! Did not see this. looking into it now.

[slorquet]

I think I have managed to execute the workflow you suggested. I changed my patch to only use this define when the clock actually uses HSI. I have verified that this compiles and execute both for HSE and HSI.

[xiaoxiang781216]

@slorquet please rebase your patch to the last mainline, Ci issue is fixed here: #6281

[slorquet]

nothing in the last commits have a relation to this pull request.

Note: I've run out of spoons on this issue.

[davids5]

@slorquet You are much closer to the "Problem" then I am at the moment. But I am wondering why @jlaitine placed the code where he did. Can you think of any reason that init was added where it was?

[slorquet]

When the CI first failed, I was surprised to encounter an attempt to set the HSI divider in a board that did not use HSI.

I investigated and found that code in the rcc_reset() function, which was surprising
https://github.com/apache/incubator-nuttx/blob/4f31c89963ebdf51a41705c508de8d0891fbaca3/arch/arm/src/stm32h7/stm32h7x3xx_rcc.c#L149

That is why I moved the code into stm32_clockconfig() where the HSI configuration is applied:
https://github.com/apache/incubator-nuttx/blob/4f31c89963ebdf51a41705c508de8d0891fbaca3/arch/arm/src/stm32h7/stm32h7x3xx_rcc.c#L599

The STM32 chips boot with a default clock that just works, so there is no need to set a HSI divider in the rcc_reset code.

Since my board works like this (a breakage would hang) I am confident that my change is correct.

Thats just my opinion and I agree that someone could add more info here if you dont have enough trust in my change.

[davids5]

@slorquet I have pinged Jukka on Slack, I looking at this in detail now: I just has a quick look at the Clock tree and the code more carefully. Thing make less sense now :(

The HSI is the default clock on reset:

The original code

 regval  = getreg32(STM32_RCC_CR);
  regval &= ~(RCC_CR_HSEON | RCC_CR_HSI48ON |
              RCC_CR_CSION | RCC_CR_PLL1ON |
              RCC_CR_PLL2ON | RCC_CR_PLL3ON |
              RCC_CR_HSIDIV_MASK);

  /* Set HSI predivider to default (4, 16MHz) */

  regval |= RCC_CR_HSIDIV_4;

So this code is :

RCC_CR_HSIDIV_MASK is dropping the default.
and the regval |= RCC_CR_HSIDIV_4; is adding it back

So the comments say....But the RM0433 Rev 7 says:

Bits 4:3 HSIDIV[1:0]: HSI clock divider
Set and reset by software.
These bits allow selecting a division ratio in order to configure the wanted HSI clock frequency. The
HSIDIV cannot be changed if the HSI is selected as reference clock for at least one enabled PLL
(PLLxON bit set to ‘1’). In that case, the new HSIDIV value is ignored.
00: Division by 1, hsi(_ker)_ck = 64 MHz (default after reset)
01: Division by 2, hsi(_ker)_ck = 32 MHz
10: Division by 4, hsi(_ker)_ck = 16 MHz
11: Division by 8, hsi(_ker)_ck = 8 MHz

So it is really slowing down the system by 4X at boot.

I wonder if the HW he was debugging was an issue. Let's see what he says.

My take is: It s it better to boot faster and make it work with 00: Division by 1, hsi(_ker)_ck = 64 MHz (default after reset), then set up the clock tree and the the final setting can be done with the divisor once the clock tree is configured.

Which is the direction you have taken. I know you ran it as a test, but did you review and verify the code path's order of operations of modifying the clock tree without the drop and ensure there is not an issue with going out of spec on the PLL set up?

Are you up for that? If not the safe thing to do is leave the original code with the /4 and your final change.

[slorquet]

I did not and I have no issue to restore the default setting in rcc_reset. Shall I proceed to restore this just now?

[davids5]

Yes. Better safe then sorry.

[hartmannathan]

I think there is an error in the log message:

"This commit adds the ability to choose the HSE divider, which must be indicated in board.h ."

(Emphasis mine.)

I think you meant HSI divider here.

[slorquet]

Sure, will update that.

[davids5]

Thank you @slorquet!