-
Notifications
You must be signed in to change notification settings - Fork 735
Clock separation #3495
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
base: main
Are you sure you want to change the base?
Clock separation #3495
Conversation
26a63f3 to
fea1633
Compare
eba4d57 to
eb621f6
Compare
adeecd2 to
dfa0e89
Compare
35bdaab to
af18d52
Compare
|
The following west manifest projects have changed revision in this Pull Request:
✅ All manifest checks OK Note: This message is automatically posted and updated by the Manifest GitHub Action. |
…nrf clock shim. Separated clock_control_nrf_hfclk shim from clock_control_nrf shim. Upstream PR #: 99290 Signed-off-by: Michal Frankiewicz <[email protected]>
… clock shim. Separated clock_control_nrf_xo shim from clock_control_nrf shim. Upstream PR #: 99290 Signed-off-by: Michal Frankiewicz <[email protected]>
af18d52 to
40acc62
Compare
masz-nordic
left a comment
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.
Not a full review
| - "CLOCK_CONTROL_NRF_K32SRC_RC" | ||
| - "CLOCK_CONTROL_NRF_K32SRC_XTAL" | ||
| - "CLOCK_CONTROL_NRF_K32SRC_SYNTH" | ||
| - "CLOCK_CONTROL_NRF_K32SRC_EXT_LOW_SWING" | ||
| - "CLOCK_CONTROL_NRF_K32SRC_EXT_FULL_SWING" |
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.
| - "CLOCK_CONTROL_NRF_K32SRC_RC" | |
| - "CLOCK_CONTROL_NRF_K32SRC_XTAL" | |
| - "CLOCK_CONTROL_NRF_K32SRC_SYNTH" | |
| - "CLOCK_CONTROL_NRF_K32SRC_EXT_LOW_SWING" | |
| - "CLOCK_CONTROL_NRF_K32SRC_EXT_FULL_SWING" | |
| - "RC" | |
| - "XTAL" | |
| - "SYNTH" | |
| - "EXT_LOW_SWING" | |
| - "EXT_FULL_SWING" |
modules/hal_nordic/nrfx/Kconfig
Outdated
|
|
||
| config NRFX_CLOCK_HFCLK192M | ||
| bool "HFCLK192M driver" | ||
| depends on $(dt_nodelabel_exists,hfclk192m) |
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 will break out of tree boards. We need a temporary compatibility layer.
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.
Whitespace alignment should be done in a separate PR.
| (nordic_nrf_clock_hfclk), | ||
| (nordic_nrf_clock_xo))), |
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 would be better not to depend on HAL symbol and check if node with compatible exists.
| struct device *clk_dev = DEVICE_DT_GET_ONE(COND_CODE_1((NRF_CLOCK_HAS_HFCLK), | ||
| (nordic_nrf_clock_hfclk), | ||
| (nordic_nrf_clock_xo))); |
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 could be described explicitly in DTS with a phandle? Let's discuss alternatives.
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.
Same in other drivers.
| #if CONFIG_SOC_SERIES_NRF54HX | ||
| #define DMIC_NRFX_CLOCK_FREQ MHZ(16) | ||
| #define DMIC_NRFX_AUDIO_CLOCK_FREQ DT_PROP_OR(NODE_AUDIOPLL, frequency, 0) | ||
| #elif DT_NODE_HAS_STATUS_OKAY(NODE_AUDIO_AUXPLL) | ||
| #define AUXPLL_FREQUENCY_SETTING DT_PROP(NODE_AUDIO_AUXPLL, nordic_frequency) | ||
| BUILD_ASSERT((AUXPLL_FREQUENCY_SETTING == NRF_AUXPLL_FREQ_DIV_AUDIO_48K) || | ||
| (AUXPLL_FREQUENCY_SETTING == NRF_AUXPLL_FREQ_DIV_AUDIO_44K1), | ||
| "Unsupported Audio AUXPLL frequency selection for PDM"); | ||
|
|
||
| #define DMIC_NRFX_AUDIO_CLOCK_FREQ CLOCK_CONTROL_NRF_AUXPLL_GET_FREQ(NODE_AUDIO_AUXPLL) | ||
|
|
||
| #define DMIC_NRFX_CLOCK_FREQ MHZ(32) | ||
|
|
||
| #else | ||
| #elif CONFIG_CLOCK_CONTROL_NRF | ||
| #define DMIC_NRFX_CLOCK_FREQ MHZ(32) | ||
| #define DMIC_NRFX_AUDIO_CLOCK_FREQ DT_PROP_OR(DT_NODELABEL(aclk), clock_frequency, \ | ||
| DT_PROP_OR(DT_NODELABEL(clock), hfclkaudio_frequency, 0)) | ||
| #elif defined(CONFIG_CLOCK_CONTROL_NRF_HFCLKAUDIO) | ||
| #define DMIC_NRFX_CLOCK_FREQ MHZ(32) | ||
| #define DMIC_NRFX_AUDIO_CLOCK_FREQ \ | ||
| DT_PROP_OR(DT_NODELABEL(aclk), clock_frequency, \ | ||
| DT_PROP_OR(DT_NODELABEL(hfclkaudio), hfclkaudio_frequency, 0)) | ||
| #endif | ||
|
|
||
| #if defined(CONFIG_CLOCK_CONTORL_NRF) | ||
| #define HFCLKAUDIO_NODE clock | ||
| #define HFCLKAUDIO_NODE_DESC "hfclkaudio-frequency in the nordic,nrf-clock node, " | ||
| #elif defined(CONFIG_CLOCK_CONTROL_NRF_HFCLKAUDIO) | ||
| #define HFCLKAUDIO_NODE hfclkaudio | ||
| #define HFCLKAUDIO_NODE_DESC "hfclkaudio-frequency in the nordic,nrf-clock-hfclkaudio node, " | ||
| #endif |
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 thinking this whole block could be refactored and simplified.
| #if NRF_LFRC_HAS_CALIBRATION | ||
| IRQ_CONNECT(LFRC_IRQn, DT_INST_IRQ(0, priority), nrfx_isr, clock_irq_handler, 0); | ||
| #endif |
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.
Should be a separate DT node.
| #include "clock_control_nrf_common.h" | ||
| #include <nrfx.h> | ||
|
|
||
| #if NRFX_CHECK(NRFX_POWER_ENABLED) |
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 it's a good idea to depend on nrfx config symbols.
| #define CTX_ONOFF BIT(6) | ||
| #define CTX_API BIT(7) | ||
| #define CTX_MASK (CTX_ONOFF | CTX_API) | ||
|
|
||
| #define STATUS_MASK 0x7 | ||
| #define GET_STATUS(flags) (flags & STATUS_MASK) | ||
| #define GET_CTX(flags) (flags & CTX_MASK) | ||
|
|
||
| /* Used only by HF clock */ | ||
| #define HF_USER_BT BIT(0) | ||
| #define HF_USER_GENERIC BIT(1) | ||
|
|
||
| /* Helper logging macros which prepends hfclk name to the log. */ | ||
| #ifdef CONFIG_LOG | ||
| #define CLOCK_LOG(lvl, dev, ...) \ | ||
| LOG_##lvl("%s: " GET_ARG_N(1, __VA_ARGS__), \ | ||
| "hfclk" COND_CODE_0(NUM_VA_ARGS_LESS_1(__VA_ARGS__), \ | ||
| (), (, GET_ARGS_LESS_N(1, __VA_ARGS__)))) | ||
| #else | ||
| #define CLOCK_LOG(...) | ||
| #endif | ||
|
|
||
| #define ERR(dev, ...) CLOCK_LOG(ERR, dev, __VA_ARGS__) | ||
| #define WRN(dev, ...) CLOCK_LOG(WRN, dev, __VA_ARGS__) | ||
| #define INF(dev, ...) CLOCK_LOG(INF, dev, __VA_ARGS__) | ||
| #define DBG(dev, ...) CLOCK_LOG(DBG, dev, __VA_ARGS__) |
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.
Common code between all drivers, should be factored out.
…nrf clock shim. Separated clock_control_nrf_lfclk shim from clock_control_nrf shim. Upstream PR #: 99290 Signed-off-by: Michal Frankiewicz <[email protected]>
…rom nrf clock shim. Separated clock_control_nrf_hfclk192m shim from clock_control_nrf shim. Upstream PR #: 99290 Signed-off-by: Michal Frankiewicz <[email protected]>
…nrf clock shim. Separated clock_control_nrf_xo24m shim from clock_control_nrf shim. Upstream PR #: 99290 Signed-off-by: Michal Frankiewicz <[email protected]>
…from nrf clock shim. Separated clock_control_nrf_hfclkaudio shim from clock_control_nrf shim. Upstream PR #: 99290 Signed-off-by: Michal Frankiewicz <[email protected]>
40acc62 to
7687271
Compare
magp-nordic
left a comment
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.
Reviewed only part of PR for now
| #endif | ||
|
|
||
| #if defined(CONFIG_CLOCK_CONTROL_NRF) | ||
| #ifndef HALTIUM_XXAA |
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 sure it can be used here; it is an nrfx symbol.
| void clock_control_nrf_common_connect_irq(void) | ||
| { | ||
| if (irq_connected) { | ||
| return; | ||
| } | ||
| irq_connected = true; | ||
|
|
||
| IRQ_CONNECT(DT_INST_IRQN(0), DT_INST_IRQ(0, priority), | ||
| nrfx_isr, clock_irq_handler, 0); |
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 connect nrfx_power_clock_irq_handler as in the previous API?
| }; | ||
| compatible: "nordic,nrf-lfclk" | ||
| compatible: "nordic,nrfs-lfclk" |
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.
File name should also be changed. Or, maybe it could be left as it was? There won't be a name clash with nrf-clock-lfclk
7687271 to
eab3e9d
Compare
Added binding parameters for HW related Kconfigs in clock control shim. Added dts configs for boards/tests using these Kconfigs. Upstream PR #: 99290 Signed-off-by: Michal Frankiewicz <[email protected]>
Replaced old clock_control.h api with nrf_clock_control.h api. Old api is still available when CONFIG_CLOCK_CONTROL_NRF is set. Upstream PR #: 99290 Signed-off-by: Michal Frankiewicz <[email protected]>
eab3e9d to
a9e0cf0
Compare
No description provided.