soc: nordic: nrf53: assign pin xl1,xl2 to app core if lfxo disabled #92664

ivynya · 2025-07-03T22:08:41Z

I opened an issue #92663 where disabling LFXO for the nRF53 via devicetree or kconfig does not properly allow the pins from the LFXO (0.00 and 0.01) to be used as GPIO. This adds a few guards against those devicetree and kconfig properties, and if LFXO is disabled by the build configuration, will assign those pins to the nRF53 app core instead.

github-actions · 2025-07-03T22:09:19Z

Hello @ivynya, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

bjarki-andreasen

I believe a better option than riding on the LFXO_CAP, would be to just use DT_NODE_HAS_STATUS_OKAY(LFXO_NODE), and if it is, then BUILD_ASSERT(DT_NODE_HAS_PROP(LFXO_NODE), load_capacitors) and lastly, clean up the#ifdef like

#ifdef CONFIG_SOC_NRF5340_CPUAPP
#if DT_NODE_HAS_STATUS_OKAY(LFXO_NODE)
	nrf_oscillators_lfxo_cap_set(NRF_OSCILLATORS, LFXO_CAP);
#if !defined(CONFIG_BUILD_WITH_TFM)
	/* This can only be done from secure code.
	 * This is handled by the TF-M platform so we skip it when TF-M is
	 * enabled.
	 */
	nrf_gpio_pin_control_select(PIN_XL1, NRF_GPIO_PIN_SEL_PERIPHERAL);
	nrf_gpio_pin_control_select(PIN_XL2, NRF_GPIO_PIN_SEL_PERIPHERAL);
#endif /* !defined(CONFIG_BUILD_WITH_TFM) */
#elif !defined(CONFIG_BUILD_WITH_TFM)
	nrf_gpio_pin_control_select(PIN_XL1, NRF_GPIO_PIN_SEL_APP);
	nrf_gpio_pin_control_select(PIN_XL2, NRF_GPIO_PIN_SEL_APP);
#endif

This aligns everything to use the devicetree, and is is clearer as DT_NODE_HAS_STATUS_OKAY(LFXO_NODE) means what it says, where LFXO_CAP is being used to mean the same, but IMO does not read the same :)

If maintaining the HWM1 module is of interest, then a bespoke PR for it based on the deprecated Kconfig symbol would be needed.

bjarki-andreasen · 2025-07-04T13:47:41Z

soc/nordic/nrf53/soc.c

@@ -54,11 +54,13 @@
 #define HFXO_NODE DT_NODELABEL(hfxo)

 /* LFXO config from DT */
+#if DT_NODE_HAS_STATUS_OKAY(LFXO_NODE) && defined(CONFIG_SOC_ENABLE_LFXO)


CONFIG_SOC_ENABLE_LFXO is deprecated, and is not used upstream. It is only used in the nordic specific HWM1 module, which defines it itself anyway, so really the config should just be removed from upstream entirely :)

With this change as is, the new and correct behavior added in this PR when LFXO is disabled in devicetree is only applied if the deprecated CONFIG_SOC_ENABLE_LFXO is also selected, which is an unnecessary additional constraint, especially given the downstream HWM1 module entirely ignores the contents of this file so its not making the change any more backwards compatible.

ivynya · 2025-07-07T16:47:49Z

Thank you for the feedback! I believe I applied the suggestions correctly, checking directly and exclusively for the devicetree node instead. I also removed the legacy LFXO Kconfig if block (L62-72) as it sounds like after these changes it will not affect anything anymore. I tested quickly with my project and it seems to work. Please let me know if I might have misinterpreted anything or further changes are needed.

bjarki-andreasen

This looks just about perfect :) Nice work!

bjarki-andreasen · 2025-07-08T11:51:51Z

The Signed-off-by: entry must use the full name of your github profile, so Signed-off-by: Ivynya Lu <[email protected]>

anangl · 2025-07-09T07:30:22Z

soc/nordic/nrf53/soc.c

-#else
-/* LFXO config from legacy Kconfig */
-#if defined(CONFIG_SOC_LFXO_CAP_INT_6PF)
-#define LFXO_CAP NRF_OSCILLATORS_LFXO_CAP_6PF
-#elif defined(CONFIG_SOC_LFXO_CAP_INT_7PF)
-#define LFXO_CAP NRF_OSCILLATORS_LFXO_CAP_7PF
-#elif defined(CONFIG_SOC_LFXO_CAP_INT_9PF)
-#define LFXO_CAP NRF_OSCILLATORS_LFXO_CAP_9PF
-#else
-#define LFXO_CAP NRF_OSCILLATORS_LFXO_CAP_EXTERNAL
-#endif
-#endif


This part cannot be removed yet. These options are deprecated but they are still present in Kconfig, so they still need to be supported.

Then remove them in Kconfig? The have been deprecated for a year now

Yes, they were deprecated in 4.0, so they can be removed now.

anangl · 2025-07-09T07:42:34Z

soc/nordic/nrf53/soc.c

@@ -54,22 +54,14 @@
 #define HFXO_NODE DT_NODELABEL(hfxo)

 /* LFXO config from DT */
+#if DT_NODE_HAS_STATUS_OKAY(LFXO_NODE)
+BUILD_ASSERT(DT_NODE_HAS_PROP(LFXO_NODE, load_capacitors));


It's probably better to mark this property as required in the devicetree binding. But anyway, as long as the legacy Kconfig options are available, we cannot require this property to be present.

Marking it as required is an issue since the lfxo is not neccesarily present, which I believe is noted by the lfxo node having status = "disabled". In this case, if the prop is required, some random value has to be set even though it can not be correct given there is no lfxo on the board :) The same issue is present for the comparator where some properties are required only if the comp is actually enabled :)

I don't think I understand the issue. Properties with required: true are required by dtc only for nodes with status "okay", so for disabled nodes you don't need to provide any value for them.

ah, I though they where always required, given what "disabled" means according to devicetree spec, seems we have our own rule which matches zephyrs bindings better :)

anangl · 2025-07-09T07:55:49Z

soc/nordic/nrf53/soc.c

+#elif !defined(CONFIG_BUILD_WITH_TFM)
+	nrf_gpio_pin_control_select(PIN_XL1, NRF_GPIO_PIN_SEL_APP);
+	nrf_gpio_pin_control_select(PIN_XL2, NRF_GPIO_PIN_SEL_APP);
+#endif /* DT_NODE_HAS_STATUS_OKAY(LFXO_NODE) */


How about defining something like LFXO_PIN_SEL depending on the lfxo node status and using here just:

#if defined(LFXO_CAP) nrf_oscillators_lfxo_cap_set(NRF_OSCILLATORS, LFXO_CAP); #endif #if !defined(CONFIG_BUILD_WITH_TFM) /* This can only be done from secure code. * This is handled by the TF-M platform so we skip it when TF-M is * enabled. */ nrf_gpio_pin_control_select(PIN_XL1, LFXO_PIN_SEL); nrf_gpio_pin_control_select(PIN_XL2, LFXO_PIN_SEL); #endif /* !defined(CONFIG_BUILD_WITH_TFM) */

Wouldn't that be clearer?

Fixes zephyrproject-rtos#92663 Disabled LFXO via devicetree allows pin 0.00 and 0.01 to work correctly as gpio by assigning it to the app core instead of peripheral. Removed deprecated Kconfig options so DT is the only config path now. Signed-off-by: Ivynya Lu <[email protected]>

ivynya · 2025-07-14T16:45:44Z

Sorry for the delay in update. I added a define LFXO_PIN_SEL and used it in the CPUAPP ifdef, as suggested, and also removed the Kconfig options for SOC_LFXO_ENABLE and related as it sounds like this would be acceptable due to them being deprecated.

However, I'm a little confused regarding the conversation about marking the DT property for load capacitors as required - would this be a preferred change or is it okay as-is? Again, for these changes I ran a quick test on my project and it seems to be working as intended, but please let me know if there are further changes needed. Thanks!

sonarqubecloud · 2025-07-14T17:03:55Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

bjarki-andreasen

Looking into the build failure, some boards depend on the previous default behavior, which automatically sets load capacitors to "external" if load-capacitors is not specified in the devicetree.

To avoid having to look through and update all boards to check if they set load-capacitors to manually set them to external, let's keep the default behavior. That is, don't assert

BUILD_ASSERT(DT_NODE_HAS_PROP(LFXO_NODE, load_capacitors))

and instead default to external if the prop does not exist:

#if DT_NODE_HAS_PROP(LFXO_NODE, load_capacitors)
#if DT_ENUM_HAS_VALUE(LFXO_NODE, load_capacitors, external)
#define LFXO_CAP NRF_OSCILLATORS_LFXO_CAP_EXTERNAL
#elif DT_ENUM_HAS_VALUE(LFXO_NODE, load_capacitors, internal)
#define LFXO_CAP (DT_ENUM_IDX(LFXO_NODE, load_capacitance_picofarad) + 1U)
#endif
#else
#define LFXO_CAP NRF_OSCILLATORS_LFXO_CAP_EXTERNAL
#endif

and leave the binding, so load-capacitors stays not required :)

ivynya mentioned this pull request Jul 3, 2025

nRF53 disrespects LFXO disabled configuration to use P0.00/P0.01 as GPIO #92663

Open

1 task

github-actions bot added the platform: nRF Nordic nRFx label Jul 3, 2025

github-actions bot requested review from anangl, jaz1-nordic, kl-cruz, magp-nordic, masz-nordic and nika-nordic July 3, 2025 22:09

github-actions bot assigned anangl and masz-nordic Jul 3, 2025

carlescufi requested a review from bjarki-andreasen July 4, 2025 13:32

bjarki-andreasen requested changes Jul 4, 2025

View reviewed changes

ivynya force-pushed the nrf53-lfxo-config branch from 877bc15 to 1b70a1c Compare July 7, 2025 16:37

ivynya requested a review from bjarki-andreasen July 7, 2025 16:48

bjarki-andreasen previously approved these changes Jul 8, 2025

View reviewed changes

masz-nordic previously approved these changes Jul 8, 2025

View reviewed changes

ivynya force-pushed the nrf53-lfxo-config branch from 1b70a1c to 31edb33 Compare July 9, 2025 03:05

anangl requested changes Jul 9, 2025

View reviewed changes

ivynya dismissed stale reviews from masz-nordic and bjarki-andreasen via 21ea969 July 14, 2025 16:37

ivynya force-pushed the nrf53-lfxo-config branch from 31edb33 to 21ea969 Compare July 14, 2025 16:37

ivynya requested a review from anangl July 14, 2025 16:46

bjarki-andreasen approved these changes Jul 14, 2025

View reviewed changes

bjarki-andreasen requested changes Jul 25, 2025

View reviewed changes

soc: nordic: nrf53: assign pin xl1,xl2 to app core if lfxo disabled #92664

Are you sure you want to change the base?

soc: nordic: nrf53: assign pin xl1,xl2 to app core if lfxo disabled #92664

Uh oh!

Conversation

ivynya commented Jul 3, 2025

Uh oh!

github-actions bot commented Jul 3, 2025

Uh oh!

bjarki-andreasen left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjarki-andreasen Jul 4, 2025

Choose a reason for hiding this comment

Uh oh!

ivynya commented Jul 7, 2025

Uh oh!

bjarki-andreasen left a comment

Choose a reason for hiding this comment

Uh oh!

bjarki-andreasen commented Jul 8, 2025

Uh oh!

anangl Jul 9, 2025

Choose a reason for hiding this comment

Uh oh!

bjarki-andreasen Jul 9, 2025

Choose a reason for hiding this comment

Uh oh!

anangl Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anangl Jul 9, 2025

Choose a reason for hiding this comment

Uh oh!

bjarki-andreasen Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anangl Jul 9, 2025

Choose a reason for hiding this comment

Uh oh!

bjarki-andreasen Jul 9, 2025

Choose a reason for hiding this comment

Uh oh!

anangl Jul 9, 2025

Choose a reason for hiding this comment

Uh oh!

ivynya commented Jul 14, 2025

Uh oh!

sonarqubecloud bot commented Jul 14, 2025

Quality Gate passed

Uh oh!

bjarki-andreasen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bjarki-andreasen left a comment •

edited

Loading

anangl Jul 9, 2025 •

edited

Loading

bjarki-andreasen Jul 9, 2025 •

edited

Loading