-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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
modules: hal_nordic: use CLOCK_CONTROL_NRF2 for HFCLK request/release… #83698
base: main
Are you sure you want to change the base?
modules: hal_nordic: use CLOCK_CONTROL_NRF2 for HFCLK request/release… #83698
Conversation
@ankuns FYI |
e8c11ca
to
ed48612
Compare
modules/hal_nordic/nrf_802154/sl_opensource/platform/nrf_802154_clock_zephyr.c
Outdated
Show resolved
Hide resolved
modules/hal_nordic/nrf_802154/sl_opensource/platform/nrf_802154_clock_zephyr.c
Outdated
Show resolved
Hide resolved
modules/hal_nordic/nrf_802154/sl_opensource/platform/nrf_802154_clock_zephyr.c
Outdated
Show resolved
Hide resolved
#include <zephyr/drivers/clock_control.h> | ||
#elif !defined(NRF54H_SERIES) | ||
#elif (defined(CONFIG_CLOCK_CONTROL_NRF2) && defined(NRF54H_SERIES)) | ||
#define NRF_802154_HFCLK_CLOCK_FREQUENCY 32000000 |
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.
Shouldn't this parameter be taken from devicetree clock-frequency
property?
I wonder why do we have to (do we have to?) invent ourselves here the frequency value we request for?
@anangl any hints?
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.
@anangl is this parameter ok or shall I use devicetree prop?
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.
Why not just use NRF_CLOCK_CONTROL_FREQUENCY_MAX
?
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.
@anangl right now, NRF_CLOCK_CONTROL_FREQUENCY_MAX
is set to UINT32_MAX
. Using it generates invalid clock spec error.
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.
Oh, it's a bug in the clock control driver then. That needs to be fixed.
But as for HFXO there is actually no choice regarding its attributes, you could use just NULL
as spec
:
zephyr/include/zephyr/drivers/clock_control/nrf_clock_control.h
Lines 214 to 219 in 7faafda
* @param spec specification of minimal acceptable attributes, like frequency, | |
* accuracy, and precision, required for the clock. | |
* Value of 0 has the meaning of "default" and can be passed | |
* instead of a given attribute if there is no strict requirement | |
* in this regard. If there is no specific requirement for any of | |
* the attributes, this parameter can be NULL. |
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.
Missed that, thanks!
modules/hal_nordic/nrf_802154/sl_opensource/platform/nrf_802154_clock_zephyr.c
Show resolved
Hide resolved
ed48612
to
6e73809
Compare
#include <zephyr/drivers/clock_control.h> | ||
#elif !defined(NRF54H_SERIES) | ||
#elif (defined(CONFIG_CLOCK_CONTROL_NRF2) && defined(NRF54H_SERIES)) | ||
#define NRF_802154_HFCLK_CLOCK_FREQUENCY 32000000 |
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.
Why not just use NRF_CLOCK_CONTROL_FREQUENCY_MAX
?
@@ -11,10 +11,12 @@ | |||
|
|||
#include <compiler_abstraction.h> | |||
#include <zephyr/kernel.h> | |||
#if defined(CONFIG_CLOCK_CONTROL_NRF) | |||
#include <zephyr/drivers/clock_control/nrf_clock_control.h> |
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.
nrf_clock_control.h
is all you actually need to include here. You could get rid of these conditional directives.
6e73809
to
d03dd44
Compare
Makes 802.15.4 use the API under CONFIG_CLOCK_CONTROL_NRF2 in nrf_clock_control.h to request/release HFCLK. Previous HFCLK requesting/releasing on nRF54H20 was more of a workaround and could produce issues when comes to sharing the resources. Signed-off-by: Piotr Koziar <[email protected]>
d03dd44
to
3c76c01
Compare
… on nRF54H20
Makes use of the API under CONFIG_CLOCK_CONTROL_NRF2 in nrf_clock_control.h.
Previous HFCLK requesting/releasing on nRF54H20 was more of a workaround and could produce issues when comes to sharing the resources.