You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2022/05/17 13:58:03 UTC

[GitHub] [incubator-nuttx] slorquet opened a new pull request, #6287: stm32h7: allow definition of HSI divider in board config

slorquet opened a new pull request, #6287:
URL: https://github.com/apache/incubator-nuttx/pull/6287

   ## Summary
   nucleo-h743zi is the only instanciation of stm32h7, and this board uses the HSE oscillator provided by the stlink interface.
   Definitions are missing to use HSI instead. This commit adds the ability to choose the HSE divider instead of the default value 4.
   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.
   
   ## 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.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [incubator-nuttx] slorquet commented on pull request #6287: stm32h7: allow definition of HSI divider in board config

Posted by GitBox <gi...@apache.org>.
slorquet commented on PR #6287:
URL: https://github.com/apache/incubator-nuttx/pull/6287#issuecomment-1129157379

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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [incubator-nuttx] davids5 commented on a diff in pull request #6287: stm32h7: allow definition of HSI divider in board config

Posted by GitBox <gi...@apache.org>.
davids5 commented on code in PR #6287:
URL: https://github.com/apache/incubator-nuttx/pull/6287#discussion_r874899601


##########
arch/arm/src/stm32h7/stm32h7x3xx_rcc.c:
##########
@@ -98,6 +98,12 @@
 #  define USE_PLL3
 #endif
 
+#ifdef STM32_BOARD_USEHSI
+#ifndef STM32_BOARD_HSIDIV
+#error When HSI is used, you have to define STM32_BOARD_HSIDIV in board/include/board.h

Review Comment:
   Ok
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [incubator-nuttx] slorquet commented on pull request #6287: stm32h7: allow definition of HSI divider in board config

Posted by GitBox <gi...@apache.org>.
slorquet commented on PR #6287:
URL: https://github.com/apache/incubator-nuttx/pull/6287#issuecomment-1129867992

   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.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6287: stm32h7: allow definition of HSI divider in board config

Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6287:
URL: https://github.com/apache/incubator-nuttx/pull/6287#discussion_r875083300


##########
arch/arm/src/stm32h7/stm32h7x3xx_rcc.c:
##########
@@ -98,6 +98,12 @@
 #  define USE_PLL3
 #endif
 
+#ifdef STM32_BOARD_USEHSI

Review Comment:
   ```
   #if defined(STM32_BOARD_USEHSI) && !defined(STM32_BOARD_HSIDIV)
   #  error When HSI is used, you have to define STM32_BOARD_HSIDIV in board/include/board.h
   #endif
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [incubator-nuttx] slorquet commented on a diff in pull request #6287: stm32h7: allow definition of HSI divider in board config

Posted by GitBox <gi...@apache.org>.
slorquet commented on code in PR #6287:
URL: https://github.com/apache/incubator-nuttx/pull/6287#discussion_r874882882


##########
arch/arm/src/stm32h7/stm32h7x3xx_rcc.c:
##########
@@ -98,6 +98,12 @@
 #  define USE_PLL3
 #endif
 
+#ifdef STM32_BOARD_USEHSI
+#ifndef STM32_BOARD_HSIDIV
+#error When HSI is used, you have to define STM32_BOARD_HSIDIV in board/include/board.h

Review Comment:
   I do not recommend this
   
   default case is not broken, by default the nucleo-h7 boards use HSE (external clock provided by stlink) not HSI
   
   The error will only trigger if the user modifies the board.h config to use HSI and she does not define this divider.
   
   Having a silent default value will only bring confusion if the user really forgot to set the divider.
   
   So I recommend keeping the original code instead. if you cant accept it, I will add the default value and change the error to warn.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [incubator-nuttx] davids5 commented on a diff in pull request #6287: stm32h7: allow definition of HSI divider in board config

Posted by GitBox <gi...@apache.org>.
davids5 commented on code in PR #6287:
URL: https://github.com/apache/incubator-nuttx/pull/6287#discussion_r875254268


##########
arch/arm/src/stm32h7/stm32h7x3xx_rcc.c:
##########
@@ -144,10 +148,6 @@ static inline void rcc_reset(void)
               RCC_CR_PLL2ON | RCC_CR_PLL3ON |
               RCC_CR_HSIDIV_MASK);
 
-  /* Set HSI predivider to default (4, 16MHz) */

Review Comment:
   @jlaitine do you see any issue with the change of order of operation here?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [incubator-nuttx] jlaitine commented on a diff in pull request #6287: stm32h7: allow definition of HSI divider in board config

Posted by GitBox <gi...@apache.org>.
jlaitine commented on code in PR #6287:
URL: https://github.com/apache/incubator-nuttx/pull/6287#discussion_r876253507


##########
arch/arm/src/stm32h7/stm32h7x3xx_rcc.c:
##########
@@ -144,10 +148,6 @@ static inline void rcc_reset(void)
               RCC_CR_PLL2ON | RCC_CR_PLL3ON |
               RCC_CR_HSIDIV_MASK);
 
-  /* Set HSI predivider to default (4, 16MHz) */

Review Comment:
   No problem. I don't have access to that (proprietary) HW any longer, and I don't remember any more why it was done like that. Anyhow, now the original code looks wrong and the patch looks correct to me.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [incubator-nuttx] slorquet commented on pull request #6287: stm32h7: allow definition of HSI divider in board config

Posted by GitBox <gi...@apache.org>.
slorquet commented on PR #6287:
URL: https://github.com/apache/incubator-nuttx/pull/6287#issuecomment-1129920124

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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [incubator-nuttx] davids5 merged pull request #6287: stm32h7: allow definition of HSI divider in board config

Posted by GitBox <gi...@apache.org>.
davids5 merged PR #6287:
URL: https://github.com/apache/incubator-nuttx/pull/6287


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [incubator-nuttx] davids5 commented on pull request #6287: stm32h7: allow definition of HSI divider in board config

Posted by GitBox <gi...@apache.org>.
davids5 commented on PR #6287:
URL: https://github.com/apache/incubator-nuttx/pull/6287#issuecomment-1129923090

   Yes. Better safe then sorry.  


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [incubator-nuttx] davids5 commented on pull request #6287: stm32h7: allow definition of HSI divider in board config

Posted by GitBox <gi...@apache.org>.
davids5 commented on PR #6287:
URL: https://github.com/apache/incubator-nuttx/pull/6287#issuecomment-1129040530

   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.
   
   
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [incubator-nuttx] slorquet commented on pull request #6287: stm32h7: allow definition of HSI divider in board config

Posted by GitBox <gi...@apache.org>.
slorquet commented on PR #6287:
URL: https://github.com/apache/incubator-nuttx/pull/6287#issuecomment-1129019372

   With the current code, if STM32_BOARD_USEHSI, the board will hang as soon as the code to configure the PLL is executed. It just does not work.
   
   I think a build error is preferable to a runtime hang with no obvious explanation.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [incubator-nuttx] slorquet commented on pull request #6287: stm32h7: allow definition of HSI divider in board config

Posted by GitBox <gi...@apache.org>.
slorquet commented on PR #6287:
URL: https://github.com/apache/incubator-nuttx/pull/6287#issuecomment-1129002197

   Excuse me, how is that a breaking change?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [incubator-nuttx] davids5 commented on pull request #6287: stm32h7: allow definition of HSI divider in board config

Posted by GitBox <gi...@apache.org>.
davids5 commented on PR #6287:
URL: https://github.com/apache/incubator-nuttx/pull/6287#issuecomment-1129013809

   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.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [incubator-nuttx] slorquet commented on pull request #6287: stm32h7: allow definition of HSI divider in board config

Posted by GitBox <gi...@apache.org>.
slorquet commented on PR #6287:
URL: https://github.com/apache/incubator-nuttx/pull/6287#issuecomment-1129068046

   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.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [incubator-nuttx] davids5 commented on a diff in pull request #6287: stm32h7: allow definition of HSI divider in board config

Posted by GitBox <gi...@apache.org>.
davids5 commented on code in PR #6287:
URL: https://github.com/apache/incubator-nuttx/pull/6287#discussion_r874867847


##########
arch/arm/src/stm32h7/stm32h7x3xx_rcc.c:
##########
@@ -98,6 +98,12 @@
 #  define USE_PLL3
 #endif
 
+#ifdef STM32_BOARD_USEHSI
+#ifndef STM32_BOARD_HSIDIV
+#error When HSI is used, you have to define STM32_BOARD_HSIDIV in board/include/board.h

Review Comment:
   To not make this NOT a breaking change, how about this?
   
   ```suggestion
   #define STM32_BOARD_HSIDIV RCC_CR_HSIDIV_4
   #warn default STM32_BOARD_HSIDIV set to RCC_CR_HSIDIV_4 to override define STM32_BOARD_HSIDIV in board/include/board.h
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [incubator-nuttx] davids5 commented on pull request #6287: stm32h7: allow definition of HSI divider in board config

Posted by GitBox <gi...@apache.org>.
davids5 commented on PR #6287:
URL: https://github.com/apache/incubator-nuttx/pull/6287#issuecomment-1129005765

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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [incubator-nuttx] slorquet commented on pull request #6287: stm32h7: allow definition of HSI divider in board config

Posted by GitBox <gi...@apache.org>.
slorquet commented on PR #6287:
URL: https://github.com/apache/incubator-nuttx/pull/6287#issuecomment-1129007207

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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #6287: stm32h7: allow definition of HSI divider in board config

Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6287:
URL: https://github.com/apache/incubator-nuttx/pull/6287#issuecomment-1129132454

   @slorquet please rebase your patch to the last mainline, Ci issue is fixed here: https://github.com/apache/incubator-nuttx/pull/6281


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [incubator-nuttx] davids5 commented on pull request #6287: stm32h7: allow definition of HSI divider in board config

Posted by GitBox <gi...@apache.org>.
davids5 commented on PR #6287:
URL: https://github.com/apache/incubator-nuttx/pull/6287#issuecomment-1129811800

   @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? 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [incubator-nuttx] slorquet commented on pull request #6287: stm32h7: allow definition of HSI divider in board config

Posted by GitBox <gi...@apache.org>.
slorquet commented on PR #6287:
URL: https://github.com/apache/incubator-nuttx/pull/6287#issuecomment-1129958319

   Sure, will update that.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [incubator-nuttx] davids5 commented on a diff in pull request #6287: stm32h7: allow definition of HSI divider in board config

Posted by GitBox <gi...@apache.org>.
davids5 commented on code in PR #6287:
URL: https://github.com/apache/incubator-nuttx/pull/6287#discussion_r874867847


##########
arch/arm/src/stm32h7/stm32h7x3xx_rcc.c:
##########
@@ -98,6 +98,12 @@
 #  define USE_PLL3
 #endif
 
+#ifdef STM32_BOARD_USEHSI
+#ifndef STM32_BOARD_HSIDIV
+#error When HSI is used, you have to define STM32_BOARD_HSIDIV in board/include/board.h

Review Comment:
   To not make this a breaking change, how about this?
   
   ```suggestion
   #define STM32_BOARD_HSIDIV RCC_CR_HSIDIV_4
   #warn default STM32_BOARD_HSIDIV set to RCC_CR_HSIDIV_4 to override define STM32_BOARD_HSIDIV in board/include/board.h
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [incubator-nuttx] slorquet commented on pull request #6287: stm32h7: allow definition of HSI divider in board config

Posted by GitBox <gi...@apache.org>.
slorquet commented on PR #6287:
URL: https://github.com/apache/incubator-nuttx/pull/6287#issuecomment-1129046439

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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [incubator-nuttx] hartmannathan commented on pull request #6287: stm32h7: allow definition of HSI divider in board config

Posted by GitBox <gi...@apache.org>.
hartmannathan commented on PR #6287:
URL: https://github.com/apache/incubator-nuttx/pull/6287#issuecomment-1129951492

   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.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [incubator-nuttx] davids5 commented on a diff in pull request #6287: stm32h7: allow definition of HSI divider in board config

Posted by GitBox <gi...@apache.org>.
davids5 commented on code in PR #6287:
URL: https://github.com/apache/incubator-nuttx/pull/6287#discussion_r874867847


##########
arch/arm/src/stm32h7/stm32h7x3xx_rcc.c:
##########
@@ -98,6 +98,12 @@
 #  define USE_PLL3
 #endif
 
+#ifdef STM32_BOARD_USEHSI
+#ifndef STM32_BOARD_HSIDIV
+#error When HSI is used, you have to define STM32_BOARD_HSIDIV in board/include/board.h

Review Comment:
   To not make this a breaking change, how about this?
   
   ```suggestion
   #define STM32_BOARD_HSIDIV RCC_CR_HSIDIV_4
   #warn default STM32_BOARD_HSIDIV set to RCC_CR_HSIDIV_4 to override define STM32_BOARD_HSIDIV in board/include/board.h
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [incubator-nuttx] slorquet commented on pull request #6287: stm32h7: allow definition of HSI divider in board config

Posted by GitBox <gi...@apache.org>.
slorquet commented on PR #6287:
URL: https://github.com/apache/incubator-nuttx/pull/6287#issuecomment-1129023616

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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [incubator-nuttx] davids5 commented on pull request #6287: stm32h7: allow definition of HSI divider in board config

Posted by GitBox <gi...@apache.org>.
davids5 commented on PR #6287:
URL: https://github.com/apache/incubator-nuttx/pull/6287#issuecomment-1129022734

   @slorquet Please fix the CI failures


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [incubator-nuttx] davids5 commented on pull request #6287: stm32h7: allow definition of HSI divider in board config

Posted by GitBox <gi...@apache.org>.
davids5 commented on PR #6287:
URL: https://github.com/apache/incubator-nuttx/pull/6287#issuecomment-1129916671

   @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. 
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [incubator-nuttx] slorquet commented on a diff in pull request #6287: stm32h7: allow definition of HSI divider in board config

Posted by GitBox <gi...@apache.org>.
slorquet commented on code in PR #6287:
URL: https://github.com/apache/incubator-nuttx/pull/6287#discussion_r874886187


##########
arch/arm/src/stm32h7/stm32h7x3xx_rcc.c:
##########
@@ -98,6 +98,12 @@
 #  define USE_PLL3
 #endif
 
+#ifdef STM32_BOARD_USEHSI
+#ifndef STM32_BOARD_HSIDIV
+#error When HSI is used, you have to define STM32_BOARD_HSIDIV in board/include/board.h

Review Comment:
   Also the board will not start with a default of 4, because the clock will be too high. if you mandate a default it should be 8.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org