soc: nxp: kinetis: ke1xz: add LPFLL clock support to SCG #93509
+105
−23
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hello @mgalda82, and thank you very much for your first pull request to the Zephyr project! |
zephyrbot
added
platform: NXP Drivers
zephyrbot
requested review from
butok,
dbaluta,
decsny,
DerekSnell,
dleach02,
EmilioCBen,
manuargue,
mmahadevan108 and
nordic-krch
mgalda82
force-pushed
the
fix/ke1xz_lpfll
branch
2 times, most recently
from
4f18ff4 to
608fd4b
Compare
DerekSnell
requested changes
Jul 23, 2025
There was a problem hiding this comment.
Thank you @mgalda82 for contributing this. Here are some changes I would like to see. There are also some compliance issues to clean up. Thanks
| core_clk: core_clk { | ||
| compatible = "fixed-factor-clock"; | ||
| clocks = <&firc_clk>; | ||
| #clocks = <&firc_clk>; |
There was a problem hiding this comment.
Why leave this comment in here?
|
Hi Derek,
Thanks for reviewing this.
All of the boards can run by default from Internal reference:
FIRC
LPFLL, using FIRC
But only some of the boards have the external Crystal Oscillator.
-This is not yet supported.
1.) It would be good if I add the External Oscillator support as well
2.) So what is the correct approach for this SOC family?
>
These clock config settings are specific to the board/app, and should be easy for a custom board or app to configure without modifying this soc.c file. On newer boards, NXP has been configuring the clocks in a board source file, that can be customized for a custom board. But for this SOC family, clk_init() is declared as __weak so a different board/app can override this function and configure as needed.
<<
From: Derek Snell ***@***.***>
Sent: Wednesday, July 23, 2025 1:55 PM
To: zephyrproject-rtos/zephyr ***@***.***>
Cc: Michael Galda ***@***.***>; Mention ***@***.***>
Subject: [EXT] Re: [zephyrproject-rtos/zephyr] soc: nxp: kinetis: ke1xz: add LPFLL clock support to SCG (PR #93509)
Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button
@DerekSnell requested changes on this pull request.
Thank you @mgalda82<https://github.com/mgalda82> for contributing this. Here are some changes I would like to see. There are also some compliance issues to clean up. Thanks
________________________________
In dts/arm/nxp/nxp_ke1xz.dtsi<#93509 (comment)>:
core_clk: core_clk {
compatible = "fixed-factor-clock";
- clocks = <&firc_clk>;
+ #clocks = <&firc_clk>;
Why leave this comment in here?
________________________________
On soc/nxp/kinetis/ke1xz/soc.c<#93509 (comment)>:
We should add the NXP copyright to this file
Copyright 2025 NXP
________________________________
In soc/nxp/kinetis/ke1xz/soc.c<#93509 (comment)>:
#endif
};
/* Slow Internal Reference Clock (SIRC) configuration */
ASSERT_ASYNC_CLK_DIV_VALID(SCG_CLOCK_DIV(sircdiv2_clk),
"Invalid SCG SIRC divider 2 value");
-static const scg_sirc_config_t scg_sirc_config = {
+static scg_sirc_config_t scg_sirc_config = {
Where is scg_sirc_config used now? Can we remove it?
________________________________
In soc/nxp/kinetis/ke1xz/soc.c<#93509 (comment)>:
@@ -81,31 +85,82 @@ static const scg_firc_config_t scg_firc_config = {
.trimConfig = NULL
};
-__weak void clk_init(void)
+const scg_sosc_config_t scgSysOscConfig = {
Where is scgSysOscConfig used? Can we remove it?
________________________________
In soc/nxp/kinetis/ke1xz/soc.c<#93509 (comment)>:
@@ -66,7 +70,7 @@ ASSERT_ASYNC_CLK_DIV_VALID(SCG_CLOCK_DIV(fircdiv2_clk),
"Invalid SCG FIRC divider 2 value");
static const scg_firc_config_t scg_firc_config = {
.enableMode = kSCG_FircEnable,
- .div2 = TO_ASYNC_CLK_DIV(SCG_CLOCK_DIV(fircdiv2_clk)), /* b20253 */
+ .div2 = TO_ASYNC_CLK_DIV(SCG_CLOCK_DIV(fircdiv2_clk)),
You replaced scg_firc_config with scgFircConfig, and now scg_firc_config is no longer used.
________________________________
In soc/nxp/kinetis/ke1xz/soc.c<#93509 (comment)>:
+const scg_sirc_config_t scgSircConfig = {
+ .enableMode = kSCG_SircEnable, /* Enable SIRC clock */
+ .div2 = kSCG_AsyncClkDivBy2, /* Slow IRC Clock Divider 2: divided by 2 */
+ .range = kSCG_SircRangeHigh, /* Slow IRC high range clock (8 MHz) */
+};
+const scg_firc_config_t scgFircConfig = {
+ .enableMode = kSCG_FircEnable, /* Enable FIRC clock */
+ .div2 = kSCG_AsyncClkDivBy1, /* Fast IRC Clock Divider 2: divided by 1 */
+ .range = kSCG_FircRange48M, /* Fast IRC is trimmed to 48MHz */
+ .trimConfig = NULL, /* Fast IRC Trim disabled */
+};
Previously these settings were configured by devicetree properties, and now you have them hard-coded in this source file. Is there a reason the DT properties are no longer used here?
________________________________
In soc/nxp/kinetis/ke1xz/soc.c<#93509 (comment)>:
+const scg_lpfll_config_t scgLpFllConfig = {
+ .enableMode = kSCG_LpFllEnable, /* Enable LPFLL clock */
+ .div2 = kSCG_AsyncClkDivBy2, /* Low Power FLL Clock Divider 2: divided by 2 */
+ .range = kSCG_LpFllRange72M, /* LPFLL is trimmed to 72MHz */
+ .trimConfig = NULL,
+};
These clock config settings are specific to the board/app, and should be easy for a custom board or app to configure without modifying this soc.c file. On newer boards, NXP has been configuring the clocks in a board source file, that can be customized for a custom board. But for this SOC family, clk_init() is declared as __weak so a different board/app can override this function and configure as needed.
Therefore, if these clock config structs are hard-coded in this file, they should be included in a __weak function like they were before. That also includes scgFircConfig and scgSircConfig.
-
Reply to this email directly, view it on GitHub<#93509 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/BH3WM3BXLAN2ZXLIIKDPFP33J5ZYDAVCNFSM6AAAAACCCZXSMWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTANBWHAZDCMRYG4>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
LPFLL (soc: scg: "lpfll_clk") clock source can be now selected in KE1xz Device tree (nxp_ke1xz.dtsi., board.dts or overlay) Tested on boards: frdm_ke15z, frdm_ke17z, frdm_ke17z512 Signed-off-by: Michael Galda <michael.galda@nxp.com>
mgalda82
force-pushed
the
fix/ke1xz_lpfll
branch
from
608fd4b to
807cce3
Compare
|
DerekSnell
requested changes
Jul 28, 2025
There was a problem hiding this comment.
Hi @mgalda82,
Thank you for addressing most of my feedback. I have a few more requests now. Plus, there are some compliance issues to clean up. Thanks
| @@ -1,5 +1,5 @@ | |||
| /* | |||
| * Copyright 2024 NXP | |||
| * Copyright 2024, 2025 NXP | |||
There was a problem hiding this comment.
Suggested change
| * Copyright 2024, 2025 NXP | |
| * Copyright 2024-2025 NXP |
| #endif | ||
| }; | ||
|
|
||
| /* Slow Internal Reference Clock (SIRC) configuration */ | ||
| ASSERT_ASYNC_CLK_DIV_VALID(SCG_CLOCK_DIV(sircdiv2_clk), | ||
| "Invalid SCG SIRC divider 2 value"); | ||
| static const scg_sirc_config_t scg_sirc_config = { | ||
| static scg_sirc_config_t scg_sirc_config = { |
There was a problem hiding this comment.
Suggested change
| static scg_sirc_config_t scg_sirc_config = { | |
| static const scg_sirc_config_t scg_sirc_config = { |
I think this can still be a const struct, right? No need to consume RAM.
Comment on lines
+158
to
+159
| CLOCK_InitSysOsc(&scg_sysosc_config); | ||
| CLOCK_SetXtal0Freq(scg_sysosc_config.freq); |
There was a problem hiding this comment.
Suggested change
| CLOCK_InitSysOsc(&scg_sysosc_config); | |
| CLOCK_SetXtal0Freq(scg_sysosc_config.freq); | |
| CLOCK_InitSysOsc(&scg_sosc_config); | |
| CLOCK_SetXtal0Freq(scg_sosc_config.freq); |
| .div2 = TO_ASYNC_CLK_DIV(SCG_CLOCK_DIV(soscdiv2_clk)), | ||
| .workMode = DT_PROP(DT_INST(0, nxp_kinetis_scg), sosc_mode) | ||
| }; | ||
| #endif /* DT_NODE_HAS_PROP(DT_INST(0, nxp_kinetis_scg), sosc_freq) */ |
There was a problem hiding this comment.
Suggested change
| #endif /* DT_NODE_HAS_PROP(DT_INST(0, nxp_kinetis_scg), sosc_freq) */ | |
| #endif |
This comment does not look accurate
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
LPFLL (soc: scg: "lpfll_clk") clock source can be now selected in KE1xz Device tree (nxp_ke1xz.dtsi., board.dts or overlay) .
Tested on boards: frdm_ke15z, frdm_ke17z, frdm_ke17z512