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 2021/03/01 17:24:56 UTC
[GitHub] [incubator-nuttx] davids5 opened a new pull request #2943: stm32x7:lse esure the
davids5 opened a new pull request #2943:
URL: https://github.com/apache/incubator-nuttx/pull/2943
## Summary
On The STM32F7 the RCC LSE code was hanging on waiting for OSC ready ~15% of a a board run. The root cause was determined to be the the variation in crystal response to the drive.
On The STM32H7 the RCC LSE code was hanging on waiting for OSC ready on Rev V silicon.
This PR fixes these issues.
When eanbled STM32x7_RTC_AUTO_LSECLOCK_START_DRV_CAPABILITY will cycle through the correct* values from low to high. To avoid damaging the crystal. We want to use the lowest setting that gets the OSC running. See app note AN2867
On the H7 Rev V slicon
*It will take into account the rev of the silicon and use the correct code points to achieve the drive
strength. See Eratta ES0392 Rev 7 2.2.14 LSE oscillator driving capability selection bits are swapped.
## Impact
None if the Knob is not set.
It will start the LSE or time out. In the latter case higher level SW can determine the RTC is not running, and can report it. Before the code would hang so early in boot, that a system would appear to be bricked.
## Testing
15 F7 boards were tested, All Bards that failed to start the LSE on master work with the F7 commit. The values set were determined to be medium-low.
H7 - boards with Rev V silicon that failed to start the LSE on master work with the H7 commit. The values set were determined to be medium-low.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] juniskane commented on pull request #2943: stm32x7:lse ensure it is started
Posted by GitBox <gi...@apache.org>.
juniskane commented on pull request #2943:
URL: https://github.com/apache/incubator-nuttx/pull/2943#issuecomment-789754798
Yes the static part needs to be corrected/reverted as it silently breaks existing configs and contradicts Kconfig help:
```
config STM32F7_RTC_LSECLOCK_START_DRV_CAPABILITY
int "LSE oscillator drive capability level at LSE start-up"
default 0
range 0 3
---help---
0 = Low drive capability (default)
1 = Medium high drive capability
2 = Medium low drive capability
3 = High drive capability
config STM32F7_RTC_LSECLOCK_RUN_DRV_CAPABILITY
int "LSE oscillator drive capability level after LSE start-up"
default 0
range 0 3
---help---
0 = Low drive capability (default)
1 = Medium high drive capability
2 = Medium low drive capability
3 = High drive capability
```
We have mass produced F7 devices a lot and are aware of the device not starting on manufacturing line due to LSE drive strength issue. Fixed it years ago so cannot remember every detail any more.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on pull request #2943: stm32x7:lse ensure it is started
Posted by GitBox <gi...@apache.org>.
acassis commented on pull request #2943:
URL: https://github.com/apache/incubator-nuttx/pull/2943#issuecomment-789082085
> Sorry @davids5 I have been meaning to test this. Can I merge this tonight? I have hit what I think is this issue on one of my boards and it would be nice to give it a pass.
>
> It's fairly low risk and you have already done some testing so if someone else is feeling like merging that's fine.
Hi @btashton no problem! I'll merge 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on a change in pull request #2943: stm32x7:lse ensure it is started
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #2943:
URL: https://github.com/apache/incubator-nuttx/pull/2943#discussion_r584965863
##########
File path: arch/arm/src/stm32f7/stm32_lse.c
##########
@@ -1,9 +1,9 @@
/****************************************************************************
* arch/arm/src/stm32f7/stm32_lse.c
*
- * Copyright (C) 2017 Gregory Nutt. All rights reserved.
+ * Copyright (C) 2017, 2021 Gregory Nutt. All rights reserved.
* Authors: Gregory Nutt <gn...@nuttx.org>
- * David Sidrane <da...@nscdg.com>
+ * David Sidrane <da...@nscdg.com>
Review comment:
We can in a separate PR
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] juniskane commented on a change in pull request #2943: stm32x7:lse ensure it is started
Posted by GitBox <gi...@apache.org>.
juniskane commented on a change in pull request #2943:
URL: https://github.com/apache/incubator-nuttx/pull/2943#discussion_r586213818
##########
File path: arch/arm/src/stm32f7/Kconfig
##########
@@ -2582,10 +2582,24 @@ endchoice #"RTC clock source"
if STM32F7_RTC_LSECLOCK
+config STM32F7_RTC_AUTO_LSECLOCK_START_DRV_CAPABILITY
+ bool "Automaticaly boost the LSE oscillator drive capability level until it starts-up"
+ default n
+ ---help---
+ This will cycle through the values from low to high. To avoid
+ damaging the the crystal. We want to use the lowest setting that gets
+ the OSC running. See app note AN2867
+
+ 0 = Low drive capability (default)
+ 1 = Medium high drive capability
+ 2 = Medium low drive capability
+ 3 = High drive capability
Review comment:
Yes, these are correct for F7, haven't checked H7. L4 and all new ones have the obvious correction in silicon.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on a change in pull request #2943: stm32x7:lse ensure it is started
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #2943:
URL: https://github.com/apache/incubator-nuttx/pull/2943#discussion_r585531047
##########
File path: arch/arm/src/stm32f7/stm32_lse.c
##########
@@ -1,9 +1,9 @@
/****************************************************************************
* arch/arm/src/stm32f7/stm32_lse.c
*
- * Copyright (C) 2017 Gregory Nutt. All rights reserved.
+ * Copyright (C) 2017, 2021 Gregory Nutt. All rights reserved.
* Authors: Gregory Nutt <gn...@nuttx.org>
- * David Sidrane <da...@nscdg.com>
+ * David Sidrane <da...@nscdg.com>
Review comment:
@acassis - Was your OK to merger this?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on pull request #2943: stm32x7:lse ensure it is started
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #2943:
URL: https://github.com/apache/incubator-nuttx/pull/2943#issuecomment-789086997
@acassis - I think we are all good. No Risk: It is behind a Kconfig. It has been cross tested and more is just better. @btashton feedback is always appreciated! So please let us know, how it works for you.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] juniskane commented on pull request #2943: stm32x7:lse ensure it is started
Posted by GitBox <gi...@apache.org>.
juniskane commented on pull request #2943:
URL: https://github.com/apache/incubator-nuttx/pull/2943#issuecomment-789541296
Please revert, this silently breaks existing configs. David you just need a HW engineer to select your crystal properly and then just put the correct strength settings, that the HW guy tells you to, to your code. No need to add useless run-time loop.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on pull request #2943: stm32x7:lse ensure it is started
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #2943:
URL: https://github.com/apache/incubator-nuttx/pull/2943#issuecomment-789074133
@xiaoxiang781216, @acassis - Can this be merged?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton edited a comment on pull request #2943: stm32x7:lse ensure it is started
Posted by GitBox <gi...@apache.org>.
btashton edited a comment on pull request #2943:
URL: https://github.com/apache/incubator-nuttx/pull/2943#issuecomment-789079047
Sorry @davids5 I have been meaning to test this. Can I merge this tonight? I have hit what I think is this issue on one of my boards and it would be nice to give it a pass.
It's fairly low risk and you have already done some testing so if someone else is feeling like merging that's fine.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 edited a comment on pull request #2943: stm32x7:lse ensure it is started
Posted by GitBox <gi...@apache.org>.
davids5 edited a comment on pull request #2943:
URL: https://github.com/apache/incubator-nuttx/pull/2943#issuecomment-789734279
@juniskane You can turn off the auto if you want to.
If the index that documented do not match the CONFIG that part can be reverted. and you can use the CONFIG in the code.
But on the ~~F7~~ H7 the second stanza (RUN value) does not work. The value can not be changed with the OCS running on the ~~F7~~ H7.
Do you have any concern of over driving?
> HW guy tells you to, to your code. No need to add useless run-time loop
Did you read the part where 15 out of 100 F7 boards failed all with the same components and varied on silicon version on the H7?
This loop only happens once per boot AFTER Vbat is removed.
At it was written the board are bricked in MFG, with good xatal and caps.
Let me know if you want me to revert the static setting section to used the Kconfig directly and not as an index .
3/4/21 - Correction: I misspoke "The value can not be changed with the OCS running on the F7." It is the H7, that the update can not happen after running.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis merged pull request #2943: stm32x7:lse ensure it is started
Posted by GitBox <gi...@apache.org>.
acassis merged pull request #2943:
URL: https://github.com/apache/incubator-nuttx/pull/2943
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on a change in pull request #2943: stm32x7:lse ensure it is started
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #2943:
URL: https://github.com/apache/incubator-nuttx/pull/2943#discussion_r584970609
##########
File path: arch/arm/src/stm32f7/stm32_lse.c
##########
@@ -1,9 +1,9 @@
/****************************************************************************
* arch/arm/src/stm32f7/stm32_lse.c
*
- * Copyright (C) 2017 Gregory Nutt. All rights reserved.
+ * Copyright (C) 2017, 2021 Gregory Nutt. All rights reserved.
* Authors: Gregory Nutt <gn...@nuttx.org>
- * David Sidrane <da...@nscdg.com>
+ * David Sidrane <da...@nscdg.com>
Review comment:
Yes but It is a critical code change and all the files in F7/HS need the license changed. That should be a pass with NO code changes. .... You will see this in the next PR
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on pull request #2943: stm32x7:lse ensure it is started
Posted by GitBox <gi...@apache.org>.
acassis commented on pull request #2943:
URL: https://github.com/apache/incubator-nuttx/pull/2943#issuecomment-789083440
> Sorry @davids5 I have been meaning to test this. Can I merge this tonight? I have hit what I think is this issue on one of my boards and it would be nice to give it a pass.
>
> It's fairly low risk and you have already done some testing so if someone else is feeling like merging that's fine.
Sorry @btashton I read it "too fast" I just realized you were going to test it tonight, I thought you was asking me to merge it.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on a change in pull request #2943: stm32x7:lse ensure it is started
Posted by GitBox <gi...@apache.org>.
acassis commented on a change in pull request #2943:
URL: https://github.com/apache/incubator-nuttx/pull/2943#discussion_r584972655
##########
File path: arch/arm/src/stm32f7/stm32_lse.c
##########
@@ -1,9 +1,9 @@
/****************************************************************************
* arch/arm/src/stm32f7/stm32_lse.c
*
- * Copyright (C) 2017 Gregory Nutt. All rights reserved.
+ * Copyright (C) 2017, 2021 Gregory Nutt. All rights reserved.
* Authors: Gregory Nutt <gn...@nuttx.org>
- * David Sidrane <da...@nscdg.com>
+ * David Sidrane <da...@nscdg.com>
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on pull request #2943: stm32x7:lse ensure it is started
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #2943:
URL: https://github.com/apache/incubator-nuttx/pull/2943#issuecomment-789734279
@juniskane You can turn off the auto if you want to.
If the index that documented do not match the CONFIG that part can be reverted. and you can use the CONFIG in the code.
But on the F7 the second stanza (RUN value) does not work. The value can not be changed with the OCS running on the F7.
Do you have any concern of over driving?
> HW guy tells you to, to your code. No need to add useless run-time loop
Did you read the part where 15 out of 100 F7 boards failed all with the same components and varied on silicon version on the H7?
This loop only happens once per boot AFTER Vbat is removed.
At it was written the board are bricked in MFG, with good xatal and caps.
Let me know if you want me to revert the static setting section to used the Kconfig directly and not as an index .
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on pull request #2943: stm32x7:lse ensure it is started
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #2943:
URL: https://github.com/apache/incubator-nuttx/pull/2943#issuecomment-789081991
> Sorry @davids5 I have been meaning to test this. Can I merge this tonight?
Works for me. Thank you @btashton and @acassis !
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on a change in pull request #2943: stm32x7:lse ensure it is started
Posted by GitBox <gi...@apache.org>.
acassis commented on a change in pull request #2943:
URL: https://github.com/apache/incubator-nuttx/pull/2943#discussion_r584968270
##########
File path: arch/arm/src/stm32f7/stm32_lse.c
##########
@@ -1,9 +1,9 @@
/****************************************************************************
* arch/arm/src/stm32f7/stm32_lse.c
*
- * Copyright (C) 2017 Gregory Nutt. All rights reserved.
+ * Copyright (C) 2017, 2021 Gregory Nutt. All rights reserved.
* Authors: Gregory Nutt <gn...@nuttx.org>
- * David Sidrane <da...@nscdg.com>
+ * David Sidrane <da...@nscdg.com>
Review comment:
It is a good practice to submit the patches with the coding style fixes and license changes, this is the way we are doing it. It doesn't need to be a new PR, you can just add a new commit to this PR.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on a change in pull request #2943: stm32x7:lse ensure it is started
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #2943:
URL: https://github.com/apache/incubator-nuttx/pull/2943#discussion_r584973002
##########
File path: arch/arm/src/stm32f7/stm32_lse.c
##########
@@ -1,9 +1,9 @@
/****************************************************************************
* arch/arm/src/stm32f7/stm32_lse.c
*
- * Copyright (C) 2017 Gregory Nutt. All rights reserved.
+ * Copyright (C) 2017, 2021 Gregory Nutt. All rights reserved.
* Authors: Gregory Nutt <gn...@nuttx.org>
- * David Sidrane <da...@nscdg.com>
+ * David Sidrane <da...@nscdg.com>
Review comment:
Have a look at https://github.com/apache/incubator-nuttx/pull/2945
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on pull request #2943: stm32x7:lse ensure it is started
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #2943:
URL: https://github.com/apache/incubator-nuttx/pull/2943#issuecomment-789079047
Sorry @davids5 I have been meaning to test this. Can I merge this tonight? I have hit what I think is this issue on one of my boards and it would be nice to give it a pass.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] juniskane edited a comment on pull request #2943: stm32x7:lse ensure it is started
Posted by GitBox <gi...@apache.org>.
juniskane edited a comment on pull request #2943:
URL: https://github.com/apache/incubator-nuttx/pull/2943#issuecomment-789541296
Please revert, this silently breaks existing configs. David you just need a HW engineer to select your crystal properly and then put the correct strength settings, that the HW guy tells you to, to your code. No need to add useless run-time loop.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] juniskane commented on a change in pull request #2943: stm32x7:lse ensure it is started
Posted by GitBox <gi...@apache.org>.
juniskane commented on a change in pull request #2943:
URL: https://github.com/apache/incubator-nuttx/pull/2943#discussion_r586213160
##########
File path: arch/arm/src/stm32f7/stm32_lse.c
##########
@@ -100,27 +118,55 @@ void stm32_rcc_enablelse(void)
regval |= RCC_BDCR_LSEON;
#ifdef CONFIG_STM32F7_RTC_LSECLOCK_START_DRV_CAPABILITY
- /* Set start-up drive capability for LSE oscillator. */
+ /* Set start-up drive capability for LSE oscillator. With the
+ * enable on.
+ */
- regval &= ~RCC_BDCR_LSEDRV_MASK;
- regval |= CONFIG_STM32F7_RTC_LSECLOCK_START_DRV_CAPABILITY <<
- RCC_BDCR_LSEDRV_SHIFT;
+ regval &= ~(RCC_BDCR_LSEDRV_MASK);
+ regval |= drives[CONFIG_STM32F7_RTC_LSECLOCK_START_DRV_CAPABILITY];
Review comment:
Suppose CONFIG_STM32F7_RTC_LSECLOCK_START_DRV_CAPABILITY is 1 aka MEDHI, but now drives[1] is MEDLO, so selects wrong capability! Stupid ST silicon designer borked the values used 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on a change in pull request #2943: stm32x7:lse ensure it is started
Posted by GitBox <gi...@apache.org>.
acassis commented on a change in pull request #2943:
URL: https://github.com/apache/incubator-nuttx/pull/2943#discussion_r584964423
##########
File path: arch/arm/src/stm32f7/stm32_lse.c
##########
@@ -1,9 +1,9 @@
/****************************************************************************
* arch/arm/src/stm32f7/stm32_lse.c
*
- * Copyright (C) 2017 Gregory Nutt. All rights reserved.
+ * Copyright (C) 2017, 2021 Gregory Nutt. All rights reserved.
* Authors: Gregory Nutt <gn...@nuttx.org>
- * David Sidrane <da...@nscdg.com>
+ * David Sidrane <da...@nscdg.com>
Review comment:
Could you please change this file license to Apache? You and Greg already assigned the ICLA so no reason to keep it as BSD
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org