-
Notifications
You must be signed in to change notification settings - Fork 370
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DA1469x: Clock time shifts on reboots #3321
base: master
Are you sure you want to change the base?
Conversation
9367986
to
d85d3fc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Number of functions are moved around that makes it harder to see what is new and what was really modified/added.
hw/hal/include/hal/hal_os_tick.h
Outdated
* | ||
* @param os_ticks_per_sec The number of OS ticks per second | ||
*/ | ||
void hal_os_tick_calc_params(uint32_t os_ticks_per_sec); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear why this function is in this header.
It it used only in da1469x code so maybe it should be part of da1469x code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the other services in hal_os_tick.c, I still believe this function belongs here. This is a new addition to the API, for the OS tick dividers to be re-tuned (case in point, after an oscillator re-calibration). It can be implemented for the other MCU's as well, on a need-to-do basis.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not convinced.
There is no code (in public repository) that requires this prototype. Even file in this commit does not need this.
If I was to implement this function for other MCU I would not know what is the purpose, why OS needs it and when it should be called.
To make your case it would be nice to have other code that utilizes this functionality. For now this function could be static in your hal_os_tick.c without any prototype.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a matter of fact, the reason I went thru the trouble of carving this out into a function & adding it to the API is that I wish to use it from our code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really see why external code would need to call this. This calculates parameters based on current lpclk frequency so it just has to be done after lpclk frequency has changed e.g. after calibration so it can be called from that routine directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually calling this from da1469x_lpclk_updated
should be enough.
d85d3fc
to
0d38224
Compare
I see your concern; I separated the original commit into two, one only rearranges the functions with no changes whatsoever, and the second one adds the missing services to the API. |
static void | ||
da1469x_clock_lp_set_rtc_divs(void) | ||
{ | ||
/* Please see section 2.20.4 for details */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you be more specific what section 2.20.4 is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure.
If your first commit does not change functionality is it really needed? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't move code around in .c
files - this breaks git annotate
da1469x_clock_lp_xtal32k_calibrate(void) | ||
{ | ||
g_mcu_clock_xtal32k_freq = | ||
da1469x_clock_calibrate(2, MYNEWT_VAL(MCU_CLOCK_RCX_CAL_REF_CNT)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ref cnt should have separate syscfg
@@ -38,6 +38,7 @@ static void | |||
da1469x_lpclk_settle_tmr_cb(void *arg) | |||
{ | |||
da1469x_clock_lp_xtal32k_switch(); | |||
da1469x_clock_lp_xtal32k_calibrate(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calibration of XTAL32K should be optional and disabled by default - we should assume after settling it's accurate and has its native frequency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the XTALK32K calib for full symmetry, but I see your point about making it optional & default off, to reduce latency.
} | ||
|
||
void | ||
da1469x_clock_sys_rc32m_calibrate(void) | ||
{ | ||
g_mcu_clock_rc32m_freq = da1469x_clock_calibrate(1, 100); | ||
g_mcu_clock_rc32m_freq = da1469x_clock_calibrate(DA1469X_CALIB_RC32M, RC32M_CAL_REF_CNT); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ref cnt should be configurable via syscfg for consistency
#elif MYNEWT_VAL_CHOICE(MCU_LPCLK_SOURCE, RC32K) | ||
da1469x_clock_lp_rc32k_calibrate(); | ||
#elif MYNEWT_VAL_CHOICE(MCU_LPCLK_SOURCE, XTAL32K) | ||
da1469x_clock_lp_xtal32k_calibrate(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as in other commit, this should be optional
hw/hal/include/hal/hal_os_tick.h
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code should be optional as it only applies to RCX. For devices which use XTAL32K and e.g. 128 OS ticks per seconds there's really no need to have extra overhead for ticks calculation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a good way to handle this would be for the caller to either call hal_os_tick_calc_params() or not. (In other words, this API itself could be agnostic about which oscillator is being used - it just cares about the frequency.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this comment to .h
by mistake - it should apply to .c
file (i.e. ticks calculation). Sorry for confusion.
Yeah, keeping the git annotate may indeed outweigh the benefits of having the functions in a neat order. To that extent, should I keep the "da1469x/clock: Eliminate magic numbers" commit, or should I leave those lines unchanged as well? |
Keep it, removing magic numbers is fine. |
This commit adds the missing services to the driver API. It also makes the actual calibration of the XTAL32K clock a syscfg option, defaulting to 0.
This commit replaces the magic numbers with meaningfully named constants.
This commit streamlines the LP clock API, by offering clean access to the LP clock selected via sysconfig.
This commit adds the function that calculates the RTC divider registers, and invokes it when the LP clock frequency changes.
If the LP clock's frequency is not an integer multiple of the specified OS tick frequency, the period of OS ticks will be incorrect due to an integer division truncation error (to the tune of almost 1% for RCX), resulting in OS Time gradually creeping away from the actual time. This commit fixes the issue, by maintaining the fractional OS tick value and advance OS Time accordingly.
reg |= CRG_XTAL_XTALRDY_CTRL_REG_XTALRDY_CLK_SEL_Msk; | ||
reg |= xtalrdy_cnt; | ||
CRG_XTAL->XTALRDY_CTRL_REG = reg; | ||
CRG_XTAL->XTALRDY_CTRL_REG = (xtalrdy_cnt << CRG_XTAL_XTALRDY_CTRL_REG_XTALRDY_CNT_Pos) | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit-pick: nothing major, I think we should use the masks here specially because values are coming from a syscfg.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point; thanks!
This commit adds masking to register bitfields, where a wrong syscfg value may result in the inadvertent overrides of neighboring bitfields.
0d38224
to
561a02b
Compare
Addressed all current reviewer comments. |
If the LP clock's frequency is not an integer multiple of the specified OS tick frequency, the period of OS ticks will be incorrect due to integer division truncation errors (to the tune of almost 1% for RCX), resulting in OS Time
gradually creeping away from the actual time. This PR fixes the issue, by maintaining the fractional OS tick value and advance OS Time accordingly.
This PR also improves on the implementation of the DA1469x clock driver and updates the RTC divider registers after the selected LP clock is calibrated.