You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mynewt.apache.org by GitBox <gi...@apache.org> on 2020/12/16 04:01:55 UTC
[GitHub] [mynewt-core] ncasaril opened a new pull request #2441: hw/mcu/nordic/nrf52xxx: Using hfxo_request and release. Addresses #2393.
ncasaril opened a new pull request #2441:
URL: https://github.com/apache/mynewt-core/pull/2441
hal_timer and hal_system_clock_start both enabled the HFXO directly.
Now they use the nrf52_clock_hfxo_request, and where suitable also nrf52_clock_hfxo_release.
----------------------------------------------------------------
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] [mynewt-core] danielkucera commented on a change in pull request #2441: hw/mcu/nordic/nrf52xxx: Using hfxo_request and release. Addresses #2393.
Posted by GitBox <gi...@apache.org>.
danielkucera commented on a change in pull request #2441:
URL: https://github.com/apache/mynewt-core/pull/2441#discussion_r548972602
##########
File path: hw/mcu/nordic/nrf52xxx/src/nrf52_clock.c
##########
@@ -44,7 +44,18 @@ nrf52_clock_hfxo_request(void)
__HAL_DISABLE_INTERRUPTS(ctx);
assert(nrf52_clock_hfxo_refcnt < 0xff);
if (nrf52_clock_hfxo_refcnt == 0) {
- NRF_CLOCK->TASKS_HFCLKSTART = 1;
+ /* Check the current STATE and SRC of HFCLK */
+ if ((NRF_CLOCK->HFCLKSTAT &
+ (CLOCK_HFCLKSTAT_SRC_Msk | CLOCK_HFCLKSTAT_STATE_Msk)) !=
+ (CLOCK_HFCLKSTAT_SRC_Msk | CLOCK_HFCLKSTAT_STATE_Msk)) {
Review comment:
Okay, then this is not very obvious... I would suggest comparing to `CLOCK_HFCLKSTAT_SRC_Xtal` and `CLOCK_HFCLKSTAT_STATE_Running` defines, something like:
```
if ((NRF_CLOCK->HFCLKSTAT &
(CLOCK_HFCLKSTAT_SRC_Msk | CLOCK_HFCLKSTAT_STATE_Msk)) !=
(CLOCK_HFCLKSTAT_SRC_Xtal << CLOCK_HFCLKSTAT_SRC_Pos |
CLOCK_HFCLKSTAT_STATE_Running << CLOCK_HFCLKSTAT_STATE_Pos)) {
```
----------------------------------------------------------------
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] [mynewt-core] danielkucera commented on a change in pull request #2441: hw/mcu/nordic/nrf52xxx: Using hfxo_request and release. Addresses #2393.
Posted by GitBox <gi...@apache.org>.
danielkucera commented on a change in pull request #2441:
URL: https://github.com/apache/mynewt-core/pull/2441#discussion_r553298941
##########
File path: hw/mcu/nordic/nrf52xxx/src/nrf52_clock.c
##########
@@ -44,7 +44,16 @@ nrf52_clock_hfxo_request(void)
__HAL_DISABLE_INTERRUPTS(ctx);
assert(nrf52_clock_hfxo_refcnt < 0xff);
if (nrf52_clock_hfxo_refcnt == 0) {
- NRF_CLOCK->TASKS_HFCLKSTART = 1;
+ /* Check the current STATE and SRC of HFCLK */
+ if ((NRF_CLOCK->HFCLKSTAT &
+ (CLOCK_HFCLKSTAT_SRC_Msk | CLOCK_HFCLKSTAT_STATE_Msk)) !=
+ (CLOCK_HFCLKSTAT_SRC_Xtal << CLOCK_HFCLKSTAT_SRC_Pos |
+ CLOCK_HFCLKSTAT_STATE_Running << CLOCK_HFCLKSTAT_STATE_Pos)) {
+ NRF_CLOCK->EVENTS_HFCLKSTARTED = 0;
+ NRF_CLOCK->TASKS_HFCLKSTART = 1;
+ while ((NRF_CLOCK->EVENTS_HFCLKSTARTED) != 0) {
Review comment:
this logic is reversed - `NRF_CLOCK->EVENTS_HFCLKSTARTED` has to be non-zero to escape the 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] [mynewt-core] apache-mynewt-bot commented on pull request #2441: hw/mcu/nordic/nrf52xxx: Using hfxo_request and release. Addresses #2393.
Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot commented on pull request #2441:
URL: https://github.com/apache/mynewt-core/pull/2441#issuecomment-754951266
<!-- style-bot -->
## Style check summary
#### No suggestions at this time!
----------------------------------------------------------------
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] [mynewt-core] ncasaril commented on pull request #2441: hw/mcu/nordic/nrf52xxx: Using hfxo_request and release. Addresses #2393.
Posted by GitBox <gi...@apache.org>.
ncasaril commented on pull request #2441:
URL: https://github.com/apache/mynewt-core/pull/2441#issuecomment-747801152
Sorry guys for all the back and forth on this one and thanks for the input so far.
I'm now checking both the STATE and the SRC of the HFCLK before starting it in hfxo_request.
Note that I've not covered the case of using LFCLK_SYNT + HFINT as that combination is restricted by syscfg.
If you feel this is needed I can add the ifdefs needed for this is hal_system_clock_start.
@andrzej-kaczmarek I think we need your input to see that we're not breaking anything 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] [mynewt-core] ncasaril commented on a change in pull request #2441: hw/mcu/nordic/nrf52xxx: Using hfxo_request and release. Addresses #2393.
Posted by GitBox <gi...@apache.org>.
ncasaril commented on a change in pull request #2441:
URL: https://github.com/apache/mynewt-core/pull/2441#discussion_r552244043
##########
File path: hw/mcu/nordic/nrf52xxx/src/nrf52_clock.c
##########
@@ -44,7 +44,18 @@ nrf52_clock_hfxo_request(void)
__HAL_DISABLE_INTERRUPTS(ctx);
assert(nrf52_clock_hfxo_refcnt < 0xff);
if (nrf52_clock_hfxo_refcnt == 0) {
- NRF_CLOCK->TASKS_HFCLKSTART = 1;
+ /* Check the current STATE and SRC of HFCLK */
+ if ((NRF_CLOCK->HFCLKSTAT &
+ (CLOCK_HFCLKSTAT_SRC_Msk | CLOCK_HFCLKSTAT_STATE_Msk)) !=
+ (CLOCK_HFCLKSTAT_SRC_Msk | CLOCK_HFCLKSTAT_STATE_Msk)) {
Review comment:
I agree that this is clearer. The reason we used the other if-statment was that this was the original statement used and I wanted to keep the changes to a minimum. I'll change so it's clearer.
----------------------------------------------------------------
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] [mynewt-core] kasjer commented on a change in pull request #2441: hw/mcu/nordic/nrf52xxx: Using hfxo_request and release. Addresses #2393.
Posted by GitBox <gi...@apache.org>.
kasjer commented on a change in pull request #2441:
URL: https://github.com/apache/mynewt-core/pull/2441#discussion_r544299696
##########
File path: hw/mcu/nordic/nrf52xxx/src/hal_timer.c
##########
@@ -652,18 +653,7 @@ hal_timer_config(int timer_num, uint32_t freq_hz)
#if MYNEWT_VAL_CHOICE(MCU_HFCLK_SOURCE, HFXO)
/* Make sure HFXO is started */
- if ((NRF_CLOCK->HFCLKSTAT &
- (CLOCK_HFCLKSTAT_SRC_Msk | CLOCK_HFCLKSTAT_STATE_Msk)) !=
- (CLOCK_HFCLKSTAT_SRC_Msk | CLOCK_HFCLKSTAT_STATE_Msk)) {
- NRF_CLOCK->EVENTS_HFCLKSTARTED = 0;
- NRF_CLOCK->TASKS_HFCLKSTART = 1;
- while (1) {
- if ((NRF_CLOCK->EVENTS_HFCLKSTARTED) != 0) {
- break;
- }
- }
- }
-#endif
Review comment:
**#endif** was lost in the process
----------------------------------------------------------------
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] [mynewt-core] apache-mynewt-bot removed a comment on pull request #2441: hw/mcu/nordic/nrf52xxx: Using hfxo_request and release. Addresses #2393.
Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot removed a comment on pull request #2441:
URL: https://github.com/apache/mynewt-core/pull/2441#issuecomment-747798562
<!-- style-bot -->
## Style check summary
#### No suggestions at this time!
----------------------------------------------------------------
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] [mynewt-core] apache-mynewt-bot commented on pull request #2441: hw/mcu/nordic/nrf52xxx: Using hfxo_request and release. Addresses #2393.
Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot commented on pull request #2441:
URL: https://github.com/apache/mynewt-core/pull/2441#issuecomment-747756655
<!-- style-bot -->
## Style check summary
#### No suggestions at this time!
----------------------------------------------------------------
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] [mynewt-core] apache-mynewt-bot removed a comment on pull request #2441: hw/mcu/nordic/nrf52xxx: Using hfxo_request and release. Addresses #2393.
Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot removed a comment on pull request #2441:
URL: https://github.com/apache/mynewt-core/pull/2441#issuecomment-747728549
<!-- style-bot -->
## Style check summary
#### No suggestions at this time!
----------------------------------------------------------------
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] [mynewt-core] kasjer commented on pull request #2441: hw/mcu/nordic/nrf52xxx: Using hfxo_request and release. Addresses #2393.
Posted by GitBox <gi...@apache.org>.
kasjer commented on pull request #2441:
URL: https://github.com/apache/mynewt-core/pull/2441#issuecomment-747295534
@ncasaril this patch slightly changes how code used to work.
Before it waited for XTAL to settle now it just starts it without waiting.
I don't know the implication of this. I guess that when XTAL will start settle time is short enough and it does not matter
when change form HFINT to HFXO will take place.
On the other hand if XTAL fails to start (I'm not sure what are the odds for this provided that hardware is correct) code will ignore this and will continue to run on HFINT, while before the change, it would stuck in loop and probably watchdog would cause the reboot.
That's why I added @wes3 as the original author who may have some insight.
For me it looks 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] [mynewt-core] wes3 commented on a change in pull request #2441: hw/mcu/nordic/nrf52xxx: Using hfxo_request and release. Addresses #2393.
Posted by GitBox <gi...@apache.org>.
wes3 commented on a change in pull request #2441:
URL: https://github.com/apache/mynewt-core/pull/2441#discussion_r547980384
##########
File path: hw/mcu/nordic/nrf52xxx/src/nrf52_clock.c
##########
@@ -44,7 +44,18 @@ nrf52_clock_hfxo_request(void)
__HAL_DISABLE_INTERRUPTS(ctx);
assert(nrf52_clock_hfxo_refcnt < 0xff);
if (nrf52_clock_hfxo_refcnt == 0) {
- NRF_CLOCK->TASKS_HFCLKSTART = 1;
+ /* Check the current STATE and SRC of HFCLK */
+ if ((NRF_CLOCK->HFCLKSTAT &
+ (CLOCK_HFCLKSTAT_SRC_Msk | CLOCK_HFCLKSTAT_STATE_Msk)) !=
+ (CLOCK_HFCLKSTAT_SRC_Msk | CLOCK_HFCLKSTAT_STATE_Msk)) {
Review comment:
I was speaking to @ncasaril about this. The specification shows two bits in that register. SRC is bit 0 and is 0 for HFINT and 1 for HFXO. The STATE bit is bit 16 and is 0 if HFCLK not running and 1 if running. So it seemed best to check both bits being 1. It might be possible to just check SRC and if 1 all is good. Looking at one of the versions of the SDK from nordic it does appear the check both bits.
----------------------------------------------------------------
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] [mynewt-core] apache-mynewt-bot commented on pull request #2441: hw/mcu/nordic/nrf52xxx: Using hfxo_request and release. Addresses #2393.
Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot commented on pull request #2441:
URL: https://github.com/apache/mynewt-core/pull/2441#issuecomment-745749354
<!-- style-bot -->
## Style check summary
#### No suggestions at this time!
----------------------------------------------------------------
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] [mynewt-core] apache-mynewt-bot removed a comment on pull request #2441: hw/mcu/nordic/nrf52xxx: Using hfxo_request and release. Addresses #2393.
Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot removed a comment on pull request #2441:
URL: https://github.com/apache/mynewt-core/pull/2441#issuecomment-747076831
<!-- style-bot -->
## Style check summary
#### No suggestions at this time!
----------------------------------------------------------------
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] [mynewt-core] ncasaril commented on a change in pull request #2441: hw/mcu/nordic/nrf52xxx: Using hfxo_request and release. Addresses #2393.
Posted by GitBox <gi...@apache.org>.
ncasaril commented on a change in pull request #2441:
URL: https://github.com/apache/mynewt-core/pull/2441#discussion_r544672780
##########
File path: hw/mcu/nordic/nrf52xxx/src/hal_timer.c
##########
@@ -652,18 +653,7 @@ hal_timer_config(int timer_num, uint32_t freq_hz)
#if MYNEWT_VAL_CHOICE(MCU_HFCLK_SOURCE, HFXO)
/* Make sure HFXO is started */
- if ((NRF_CLOCK->HFCLKSTAT &
- (CLOCK_HFCLKSTAT_SRC_Msk | CLOCK_HFCLKSTAT_STATE_Msk)) !=
- (CLOCK_HFCLKSTAT_SRC_Msk | CLOCK_HFCLKSTAT_STATE_Msk)) {
- NRF_CLOCK->EVENTS_HFCLKSTARTED = 0;
- NRF_CLOCK->TASKS_HFCLKSTART = 1;
- while (1) {
- if ((NRF_CLOCK->EVENTS_HFCLKSTARTED) != 0) {
- break;
- }
- }
- }
-#endif
Review comment:
Thanks @kasjer.
In the hal_timer_deinit I've added the same ifdef around the release and also checking that it's not timer 5 to match hal_timer_config.
In the deinit, I realise that the if-statement checks for tmr_rtc which is only enabled for timer_num=5 and so the release could go into the else section there. However using the timer_num as in hal_timer_config may be easier to understand?
----------------------------------------------------------------
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] [mynewt-core] apache-mynewt-bot commented on pull request #2441: hw/mcu/nordic/nrf52xxx: Using hfxo_request and release. Addresses #2393.
Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot commented on pull request #2441:
URL: https://github.com/apache/mynewt-core/pull/2441#issuecomment-747081201
<!-- style-bot -->
## Style check summary
#### No suggestions at this time!
----------------------------------------------------------------
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] [mynewt-core] ncasaril commented on pull request #2441: hw/mcu/nordic/nrf52xxx: Using hfxo_request and release. Addresses #2393.
Posted by GitBox <gi...@apache.org>.
ncasaril commented on pull request #2441:
URL: https://github.com/apache/mynewt-core/pull/2441#issuecomment-747744572
@wes3 and @kasjer Thanks for your input.
I've tried to accomodate your suggestions with moving the wait code into hfxo_request.
It feels like a good solution and that's now the only place that I can find that enables the HFCLK.
That said, I think the NRF_CLOCK->HFCLKSTAT isn't used correctly to detect if the HFXO is used. It only checks if the task has been started, not if we're actually running off the HFXO.
Leave me with it for a bit and I'll see if I can get clarity into how it's meant to work.
So, not ready for merge just yet.
----------------------------------------------------------------
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] [mynewt-core] kasjer commented on a change in pull request #2441: hw/mcu/nordic/nrf52xxx: Using hfxo_request and release. Addresses #2393.
Posted by GitBox <gi...@apache.org>.
kasjer commented on a change in pull request #2441:
URL: https://github.com/apache/mynewt-core/pull/2441#discussion_r553303353
##########
File path: hw/mcu/nordic/nrf52xxx/src/nrf52_clock.c
##########
@@ -44,7 +44,16 @@ nrf52_clock_hfxo_request(void)
__HAL_DISABLE_INTERRUPTS(ctx);
assert(nrf52_clock_hfxo_refcnt < 0xff);
if (nrf52_clock_hfxo_refcnt == 0) {
- NRF_CLOCK->TASKS_HFCLKSTART = 1;
+ /* Check the current STATE and SRC of HFCLK */
+ if ((NRF_CLOCK->HFCLKSTAT &
+ (CLOCK_HFCLKSTAT_SRC_Msk | CLOCK_HFCLKSTAT_STATE_Msk)) !=
+ (CLOCK_HFCLKSTAT_SRC_Xtal << CLOCK_HFCLKSTAT_SRC_Pos |
+ CLOCK_HFCLKSTAT_STATE_Running << CLOCK_HFCLKSTAT_STATE_Pos)) {
+ NRF_CLOCK->EVENTS_HFCLKSTARTED = 0;
+ NRF_CLOCK->TASKS_HFCLKSTART = 1;
+ while ((NRF_CLOCK->EVENTS_HFCLKSTARTED) != 0) {
Review comment:
thanks @danielkucera
----------------------------------------------------------------
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] [mynewt-core] ncasaril commented on a change in pull request #2441: hw/mcu/nordic/nrf52xxx: Using hfxo_request and release. Addresses #2393.
Posted by GitBox <gi...@apache.org>.
ncasaril commented on a change in pull request #2441:
URL: https://github.com/apache/mynewt-core/pull/2441#discussion_r553603137
##########
File path: hw/mcu/nordic/nrf52xxx/src/nrf52_clock.c
##########
@@ -44,7 +44,16 @@ nrf52_clock_hfxo_request(void)
__HAL_DISABLE_INTERRUPTS(ctx);
assert(nrf52_clock_hfxo_refcnt < 0xff);
if (nrf52_clock_hfxo_refcnt == 0) {
- NRF_CLOCK->TASKS_HFCLKSTART = 1;
+ /* Check the current STATE and SRC of HFCLK */
+ if ((NRF_CLOCK->HFCLKSTAT &
+ (CLOCK_HFCLKSTAT_SRC_Msk | CLOCK_HFCLKSTAT_STATE_Msk)) !=
+ (CLOCK_HFCLKSTAT_SRC_Xtal << CLOCK_HFCLKSTAT_SRC_Pos |
+ CLOCK_HFCLKSTAT_STATE_Running << CLOCK_HFCLKSTAT_STATE_Pos)) {
+ NRF_CLOCK->EVENTS_HFCLKSTARTED = 0;
+ NRF_CLOCK->TASKS_HFCLKSTART = 1;
+ while ((NRF_CLOCK->EVENTS_HFCLKSTARTED) != 0) {
Review comment:
Dang, too quick. Thanks guys.
----------------------------------------------------------------
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] [mynewt-core] danielkucera commented on a change in pull request #2441: hw/mcu/nordic/nrf52xxx: Using hfxo_request and release. Addresses #2393.
Posted by GitBox <gi...@apache.org>.
danielkucera commented on a change in pull request #2441:
URL: https://github.com/apache/mynewt-core/pull/2441#discussion_r547961577
##########
File path: hw/mcu/nordic/nrf52xxx/src/nrf52_clock.c
##########
@@ -44,7 +44,18 @@ nrf52_clock_hfxo_request(void)
__HAL_DISABLE_INTERRUPTS(ctx);
assert(nrf52_clock_hfxo_refcnt < 0xff);
if (nrf52_clock_hfxo_refcnt == 0) {
- NRF_CLOCK->TASKS_HFCLKSTART = 1;
+ /* Check the current STATE and SRC of HFCLK */
+ if ((NRF_CLOCK->HFCLKSTAT &
+ (CLOCK_HFCLKSTAT_SRC_Msk | CLOCK_HFCLKSTAT_STATE_Msk)) !=
+ (CLOCK_HFCLKSTAT_SRC_Msk | CLOCK_HFCLKSTAT_STATE_Msk)) {
+ NRF_CLOCK->EVENTS_HFCLKSTARTED = 0;
+ NRF_CLOCK->TASKS_HFCLKSTART = 1;
+ while (1) {
+ if ((NRF_CLOCK->EVENTS_HFCLKSTARTED) != 0) {
+ break;
+ }
Review comment:
what about using this here?
```
while (!NRF_CLOCK->EVENTS_HFCLKSTARTED) {
}
```
----------------------------------------------------------------
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] [mynewt-core] ncasaril commented on a change in pull request #2441: hw/mcu/nordic/nrf52xxx: Using hfxo_request and release. Addresses #2393.
Posted by GitBox <gi...@apache.org>.
ncasaril commented on a change in pull request #2441:
URL: https://github.com/apache/mynewt-core/pull/2441#discussion_r552244360
##########
File path: hw/mcu/nordic/nrf52xxx/src/nrf52_clock.c
##########
@@ -44,7 +44,18 @@ nrf52_clock_hfxo_request(void)
__HAL_DISABLE_INTERRUPTS(ctx);
assert(nrf52_clock_hfxo_refcnt < 0xff);
if (nrf52_clock_hfxo_refcnt == 0) {
- NRF_CLOCK->TASKS_HFCLKSTART = 1;
+ /* Check the current STATE and SRC of HFCLK */
+ if ((NRF_CLOCK->HFCLKSTAT &
+ (CLOCK_HFCLKSTAT_SRC_Msk | CLOCK_HFCLKSTAT_STATE_Msk)) !=
+ (CLOCK_HFCLKSTAT_SRC_Msk | CLOCK_HFCLKSTAT_STATE_Msk)) {
+ NRF_CLOCK->EVENTS_HFCLKSTARTED = 0;
+ NRF_CLOCK->TASKS_HFCLKSTART = 1;
+ while (1) {
+ if ((NRF_CLOCK->EVENTS_HFCLKSTARTED) != 0) {
+ break;
+ }
Review comment:
Again, same reason - minimum changes left this one in there. Very happy to simplify 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] [mynewt-core] apache-mynewt-bot removed a comment on pull request #2441: hw/mcu/nordic/nrf52xxx: Using hfxo_request and release. Addresses #2393.
Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot removed a comment on pull request #2441:
URL: https://github.com/apache/mynewt-core/pull/2441#issuecomment-754951266
<!-- style-bot -->
## Style check summary
#### No suggestions at this time!
----------------------------------------------------------------
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] [mynewt-core] wes3 commented on pull request #2441: hw/mcu/nordic/nrf52xxx: Using hfxo_request and release. Addresses #2393.
Posted by GitBox <gi...@apache.org>.
wes3 commented on pull request #2441:
URL: https://github.com/apache/mynewt-core/pull/2441#issuecomment-747480588
@ncasaril and @kasjer I was looking over the changes and I was thinking the same thing regarding settling time. The code to request the hfxo just turns it on and leaves without waiting for it to settle as you say and I was thinking of the implications of that. I think I modeled this code after some code I saw in an SDK from nordic. The idea was to check to see if the clock was started; if so do nothing. If not, clear the started event, turn on the clock, and wait for it to settle. I figured it would be pretty catastrophic if we never see EVENTS_HFCLKSTARTED so that is why it just loops forever and lets the watchdog fire. Yes, that assumes there is a watchdog running :-) There is an errata for the chip that states you could see that event erroneously if the clock takes longer than 400 usecs to settle btw so that would have to be handled differently.
So I guess we just need to decide if we care about using a timer or the radio before the clock settles. I would suspect most settling times for most boards to be in the hundreds of usecs so probably not an issue but who knows? I guess if we were being very cautious and "more correct" (bad term) we would move the code that waits for settling into hfxo_request if the clock is not on. So some thoughts on what we could do if we wanted:
1) Make sure on startup that the HFXO is off. This is to deal with any bootloaders that may have turned it on.
2) In HFXO request, wait to settle if the reference count is zero.
3) Related to 2, if all things are working properly, the HFXO should be off if the reference count is zero. Not sure if we care about that case and want to add an assert there for debug (a debug assert that can get compiled out) but this is not critical.
If both of you feel that there really is no need to wait for it to settle I am fine with the code the way it is.
----------------------------------------------------------------
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] [mynewt-core] apache-mynewt-bot removed a comment on pull request #2441: hw/mcu/nordic/nrf52xxx: Using hfxo_request and release. Addresses #2393.
Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot removed a comment on pull request #2441:
URL: https://github.com/apache/mynewt-core/pull/2441#issuecomment-747081201
<!-- style-bot -->
## Style check summary
#### No suggestions at this time!
----------------------------------------------------------------
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] [mynewt-core] kasjer merged pull request #2441: hw/mcu/nordic/nrf52xxx: Using hfxo_request and release. Addresses #2393.
Posted by GitBox <gi...@apache.org>.
kasjer merged pull request #2441:
URL: https://github.com/apache/mynewt-core/pull/2441
----------------------------------------------------------------
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] [mynewt-core] apache-mynewt-bot commented on pull request #2441: hw/mcu/nordic/nrf52xxx: Using hfxo_request and release. Addresses #2393.
Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot commented on pull request #2441:
URL: https://github.com/apache/mynewt-core/pull/2441#issuecomment-756401046
<!-- style-bot -->
## Style check summary
#### No suggestions at this time!
----------------------------------------------------------------
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] [mynewt-core] ncasaril commented on pull request #2441: hw/mcu/nordic/nrf52xxx: Using hfxo_request and release. Addresses #2393.
Posted by GitBox <gi...@apache.org>.
ncasaril commented on pull request #2441:
URL: https://github.com/apache/mynewt-core/pull/2441#issuecomment-747763547
@wes3 In the hfxo_request I'm waiting for confirmation that the HFCLK has started. I removed the check on the HFCLKSTAT as it was only checking if the task had been triggered (bit 16) and not if the HFXO was used (bit 0) thus not actually starting the HFXO.
If you're happy with how it looks now I'll clean up the commits.
----------------------------------------------------------------
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] [mynewt-core] kasjer merged pull request #2441: hw/mcu/nordic/nrf52xxx: Using hfxo_request and release. Addresses #2393.
Posted by GitBox <gi...@apache.org>.
kasjer merged pull request #2441:
URL: https://github.com/apache/mynewt-core/pull/2441
----------------------------------------------------------------
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] [mynewt-core] wes3 commented on a change in pull request #2441: hw/mcu/nordic/nrf52xxx: Using hfxo_request and release. Addresses #2393.
Posted by GitBox <gi...@apache.org>.
wes3 commented on a change in pull request #2441:
URL: https://github.com/apache/mynewt-core/pull/2441#discussion_r547980384
##########
File path: hw/mcu/nordic/nrf52xxx/src/nrf52_clock.c
##########
@@ -44,7 +44,18 @@ nrf52_clock_hfxo_request(void)
__HAL_DISABLE_INTERRUPTS(ctx);
assert(nrf52_clock_hfxo_refcnt < 0xff);
if (nrf52_clock_hfxo_refcnt == 0) {
- NRF_CLOCK->TASKS_HFCLKSTART = 1;
+ /* Check the current STATE and SRC of HFCLK */
+ if ((NRF_CLOCK->HFCLKSTAT &
+ (CLOCK_HFCLKSTAT_SRC_Msk | CLOCK_HFCLKSTAT_STATE_Msk)) !=
+ (CLOCK_HFCLKSTAT_SRC_Msk | CLOCK_HFCLKSTAT_STATE_Msk)) {
Review comment:
I was speaking to @ncasaril about this. The specification shows two bits in that register. SRC is bit 0 and is 0 for HFINT and 1 for HFXO. The STATE bit is bit 16 and is 0 if HFCLK not running and 1 if running. So it seemed best to check both bits being 1. It might be possible to just check SRC and if 1 all is good. Looking at one of the versions of the SDK from nordic it does appear they check both bits.
----------------------------------------------------------------
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] [mynewt-core] apache-mynewt-bot commented on pull request #2441: hw/mcu/nordic/nrf52xxx: Using hfxo_request and release. Addresses #2393.
Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot commented on pull request #2441:
URL: https://github.com/apache/mynewt-core/pull/2441#issuecomment-747076831
<!-- style-bot -->
## Style check summary
#### No suggestions at this time!
----------------------------------------------------------------
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] [mynewt-core] apache-mynewt-bot commented on pull request #2441: hw/mcu/nordic/nrf52xxx: Using hfxo_request and release. Addresses #2393.
Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot commented on pull request #2441:
URL: https://github.com/apache/mynewt-core/pull/2441#issuecomment-747783422
<!-- style-bot -->
## Style check summary
#### No suggestions at this time!
----------------------------------------------------------------
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] [mynewt-core] apache-mynewt-bot commented on pull request #2441: hw/mcu/nordic/nrf52xxx: Using hfxo_request and release. Addresses #2393.
Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot commented on pull request #2441:
URL: https://github.com/apache/mynewt-core/pull/2441#issuecomment-747798562
<!-- style-bot -->
## Style check summary
#### No suggestions at this time!
----------------------------------------------------------------
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] [mynewt-core] danielkucera commented on a change in pull request #2441: hw/mcu/nordic/nrf52xxx: Using hfxo_request and release. Addresses #2393.
Posted by GitBox <gi...@apache.org>.
danielkucera commented on a change in pull request #2441:
URL: https://github.com/apache/mynewt-core/pull/2441#discussion_r547956552
##########
File path: hw/mcu/nordic/nrf52xxx/src/nrf52_clock.c
##########
@@ -44,7 +44,18 @@ nrf52_clock_hfxo_request(void)
__HAL_DISABLE_INTERRUPTS(ctx);
assert(nrf52_clock_hfxo_refcnt < 0xff);
if (nrf52_clock_hfxo_refcnt == 0) {
- NRF_CLOCK->TASKS_HFCLKSTART = 1;
+ /* Check the current STATE and SRC of HFCLK */
+ if ((NRF_CLOCK->HFCLKSTAT &
+ (CLOCK_HFCLKSTAT_SRC_Msk | CLOCK_HFCLKSTAT_STATE_Msk)) !=
+ (CLOCK_HFCLKSTAT_SRC_Msk | CLOCK_HFCLKSTAT_STATE_Msk)) {
Review comment:
Just out of curiosity... What is this condition supposed to test?
----------------------------------------------------------------
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] [mynewt-core] apache-mynewt-bot removed a comment on pull request #2441: hw/mcu/nordic/nrf52xxx: Using hfxo_request and release. Addresses #2393.
Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot removed a comment on pull request #2441:
URL: https://github.com/apache/mynewt-core/pull/2441#issuecomment-747756655
<!-- style-bot -->
## Style check summary
#### No suggestions at this time!
----------------------------------------------------------------
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] [mynewt-core] apache-mynewt-bot removed a comment on pull request #2441: hw/mcu/nordic/nrf52xxx: Using hfxo_request and release. Addresses #2393.
Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot removed a comment on pull request #2441:
URL: https://github.com/apache/mynewt-core/pull/2441#issuecomment-747783422
<!-- style-bot -->
## Style check summary
#### No suggestions at this time!
----------------------------------------------------------------
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] [mynewt-core] apache-mynewt-bot removed a comment on pull request #2441: hw/mcu/nordic/nrf52xxx: Using hfxo_request and release. Addresses #2393.
Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot removed a comment on pull request #2441:
URL: https://github.com/apache/mynewt-core/pull/2441#issuecomment-745749354
<!-- style-bot -->
## Style check summary
#### No suggestions at this time!
----------------------------------------------------------------
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] [mynewt-core] apache-mynewt-bot commented on pull request #2441: hw/mcu/nordic/nrf52xxx: Using hfxo_request and release. Addresses #2393.
Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot commented on pull request #2441:
URL: https://github.com/apache/mynewt-core/pull/2441#issuecomment-747728549
<!-- style-bot -->
## Style check summary
#### No suggestions at this time!
----------------------------------------------------------------
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