Skip to content

Conversation

@joerchan
Copy link
Contributor

@joerchan joerchan commented Nov 19, 2021

Remove outdated nrf21540 FEM pins. The configuration for these are
outdated as more pins have been added later.
The module that uses these document the addition of this to be added
as overlays from the application, and the mcu selection is handled by
the MPSL FEM module.

The current setup would not work with TFM enabled since the mcu select
code would not be included in the secure image.
Since this is the responsebility of the MPSL FEM module remove it so
that there is only one place that needs to be updated for TFM.

Copy FEM pin configuration for Thingy:53 board from cpunet device tree
configuration to the application device tree configuration so that it is
added for both application and network core as documented in user guide
for FEM in GPIO mode.

Add nrf_radio_fem_spi node to application device tree configuration.

This removes the GPIO_PULL_DOWN configuration which was not correct.

@mbolivar-nordic
Copy link
Contributor

mbolivar-nordic commented Nov 20, 2021

Please remove our internal bug tracker references from the commit log.

(But otherwise LGTM)

@joerchan joerchan force-pushed the nrf-remove-thing53-fem-pins branch from 1090ea6 to 65bf1ef Compare November 21, 2021 09:52
Copy link

@jciupis jciupis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering: with these changes, only boards/arm/thingy53_nrf5340/thingy53_nrf5340_cpunet.dts file defines a nRF21540 FEM node. In order for the nRF53 application core to correctly forward the FEM pins to the network core (so that it's capable of controlling the FEM), the application also needs to have the FEM pins specified through devicetree. If the default devicetree description of this board doesn't contain a nRF21540 node for the application core, then in order to enable FEM support the user must provide an overlay that matches the network's core FEM node. That's rather inconvenient. Do you think it'd make sense to add a relevant, matching entry in application's core .dts file?

nrf_radio_fem: nrf21540_fem {
		compatible = "nordic,nrf21540-fem";
		rx-en-gpios = <&gpio1 11 (GPIO_PULL_DOWN | GPIO_ACTIVE_HIGH)>;
		mode-gpios = <&gpio1 12 (GPIO_PULL_DOWN | GPIO_ACTIVE_HIGH)>;
		pdn-gpios = <&gpio1 10 (GPIO_PULL_DOWN | GPIO_ACTIVE_HIGH)>;
		tx-en-gpios = <&gpio0 30 (GPIO_PULL_DOWN | GPIO_ACTIVE_HIGH)>;
	};

@joerchan
Copy link
Contributor Author

I'm wondering: with these changes, only boards/arm/thingy53_nrf5340/thingy53_nrf5340_cpunet.dts file defines a nRF21540 FEM node. In order for the nRF53 application core to correctly forward the FEM pins to the network core (so that it's capable of controlling the FEM), the application also needs to have the FEM pins specified through devicetree. If the default devicetree description of this board doesn't contain a nRF21540 node for the application core, then in order to enable FEM support the user must provide an overlay that matches the network's core FEM node. That's rather inconvenient. Do you think it'd make sense to add a relevant, matching entry in application's core .dts file?

nrf_radio_fem: nrf21540_fem {
		compatible = "nordic,nrf21540-fem";
		rx-en-gpios = <&gpio1 11 (GPIO_PULL_DOWN | GPIO_ACTIVE_HIGH)>;
		mode-gpios = <&gpio1 12 (GPIO_PULL_DOWN | GPIO_ACTIVE_HIGH)>;
		pdn-gpios = <&gpio1 10 (GPIO_PULL_DOWN | GPIO_ACTIVE_HIGH)>;
		tx-en-gpios = <&gpio0 30 (GPIO_PULL_DOWN | GPIO_ACTIVE_HIGH)>;
	};

It is documented how the application is supposed to do this in ug_radio_fem.rst.

On nRF53 devices, you must also apply the same devicetree node mentioned in step 1 to the network core.
To do so, apply the overlay to the correct network core child image by creating an overlay file named :file:child_image/*childImageName*.overlay in your application directory, for example :file:child_image/multiprotocol_rpmsg.overlay.
It is documented how the application is supposed to enable

I don't think there is anything else to do. This just removes something that is broken and not according to the user-guide.

@eobalski
Copy link

AFAIK FEM should be enabled by default for Thingy:53, pins should always be specified in DTS. The only thing i find inconsistent is having same pins in NET core as @jciupis suggested.

Why can't we have this enabled by default as intended?

I agree to remove pins assignment.

@jciupis
Copy link

jciupis commented Nov 23, 2021

The only thing i find inconsistent is having same pins in NET core as @jciupis suggested.

@emob-nordic could you elaborate, please?

Comment on lines 86 to 93

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only thing i find inconsistent is having same pins in NET core as @jciupis suggested.

@emob-nordic could you elaborate, please?

Sorry I introduced some chaos. I think FEM pins should remain here and in lines 182-189 as this is the default configuration for Thingy:53, i was refering to this:

Do you think it'd make sense to add a relevant, matching entry in application's core .dts file?

@jciupis
Copy link

jciupis commented Nov 23, 2021

@emob-nordic the chunk of code you attached is legacy and should not be used anymore. The new approach is based on nrf_radio_fem: nrf21540 node as e.g. in the nRF21540 DK devicetree description:

nrf_radio_fem: nrf21540_fem {
compatible = "nordic,nrf21540-fem";
tx-en-gpios = <&gpio0 22 GPIO_ACTIVE_HIGH>;
rx-en-gpios = <&gpio0 19 GPIO_ACTIVE_HIGH>;
pdn-gpios = <&gpio0 23 GPIO_ACTIVE_HIGH>;
ant-sel-gpios = <&gpio0 20 GPIO_ACTIVE_HIGH>;
mode-gpios = <&gpio0 17 GPIO_ACTIVE_HIGH>;
spi-if = <&nrf_radio_fem_spi>;
};

The tricky part with nRF21540 support for nRF53 series is that both the network core and the application core need to have such a node specified in their devicetree files. The former must know the pins configuration so that it can control them; the latter must know the pins so that it can forward them to the network core, as by default all pins are assigned to the application core. The code this PR removes is deprecated and indeed should be removed. The question is whether some code should be added so that nRF21540 is enabled by default on Thingy53, just as you suggested. The way I understand it, the current solution will require the user to provide an additional overlay for the application core. As @joerchan pointed out, it is documented, but can be made easier.

@eobalski
Copy link

It is documented how the application is supposed to do this in ug_radio_fem.rst.

On nRF53 devices, you must also apply the same devicetree node mentioned in step 1 to the network core.
To do so, apply the overlay to the correct network core child image by creating an overlay file named :file:child_image/*childImageName*.overlay in your application directory, for example :file:child_image/multiprotocol_rpmsg.overlay.
It is documented how the application is supposed to enable

I don't think there is anything else to do. This just removes something that is broken and not according to the user-guide.

@joerchan i think there should be a note saying that for Thingy:53 FEM is enabled by default. In this/or following PR matching pins should be added to the APP core dts file.

@joerchan joerchan force-pushed the nrf-remove-thing53-fem-pins branch from 65bf1ef to 6aec063 Compare November 24, 2021 14:21
@joerchan
Copy link
Contributor Author

@emob-nordic I've added the MPSL FEM configuration. Can you please verify that this is correct and working.

@joerchan joerchan force-pushed the nrf-remove-thing53-fem-pins branch 2 times, most recently from c658a0c to f9e6f9c Compare November 24, 2021 14:31
Comment on lines 9 to 12

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there also be GPIO_PULL_DOWN as in net core DTS?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could also make a note in ug_radio_fem.rst file saying that for Thingy:53 FEM is defined by default in DTS (to avoid confusion).
I will check if it works for me asap.

Copy link
Contributor Author

@joerchan joerchan Nov 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there also be GPIO_PULL_DOWN as in net core DTS?

@jciupis Do you know? I've just tried to keep the same GPIO configuration from where I removed it.

You could also make a note in ug_radio_fem.rst file saying that for Thingy:53 FEM is defined by default in DTS (to avoid confusion).
I will check if it works for me asap.

We would need this change downstream first (since the UG is downstream). Because it is currently broken downstream.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we're talking about downstream, at some point we will want to make the MPSL layer respect the fem property now introduced upstream, instead of just relying on the magic nrf_radio_fem node label. This will requires some sort of transition period. Who do we need to coordinate this with?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joerchan I'm not sure what the correct configuration for Thingy53 should be. I think it should be the same for both cores though, so maybe @emob-nordic could shed some light on whether GPIO_PULL_DOWN is indeed needed in this case.

As for @mbolivar-nordic's comment - @czeslawmakarski I think changes related to FEM representation in devicetree affect us too, right?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other way around, afaik GPIO_PULL_DOWN is correct configuration, so GPIO_PULL_DOWN should be removed from thingy53_nrf5340_cpunet.dts but i would appreciate if @anangl could have a look also. Sorry for the noise.

@joerchan joerchan force-pushed the nrf-remove-thing53-fem-pins branch from f9e6f9c to 31f51b2 Compare November 24, 2021 15:09
@joerchan joerchan force-pushed the nrf-remove-thing53-fem-pins branch from 31f51b2 to 5eee45f Compare November 26, 2021 15:21
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please remove CONFIG_THINGY53_MISO_WORKAROUND form thingy's Kconfig?

@joerchan joerchan force-pushed the nrf-remove-thing53-fem-pins branch from 5eee45f to 25ccca6 Compare December 2, 2021 16:11
@eobalski eobalski self-requested a review December 3, 2021 14:58
Copy link

@eobalski eobalski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for iterate review. I believe you should also remove this: https://github.com/zephyrproject-rtos/zephyr/blob/main/boards/arm/thingy53_nrf5340/thingy53_nrf5340_cpunet.dts#L53:L59
when u include thingy53_nrf5340_fem.dts in thingy53_nrf5340_cpunet.dts.
I'm yet to find some time to test that with our application. Tossing this one out so i don't forget it.

@joerchan
Copy link
Contributor Author

joerchan commented Dec 8, 2021

Sorry for iterate review. I believe you should also remove this: https://github.com/zephyrproject-rtos/zephyr/blob/main/boards/arm/thingy53_nrf5340/thingy53_nrf5340_cpunet.dts#L53:L59 when u include thingy53_nrf5340_fem.dts in thingy53_nrf5340_cpunet.dts. I'm yet to find some time to test that with our application. Tossing this one out so i don't forget it.

Other way around, afaik GPIO_PULL_DOWN is correct configuration, so GPIO_PULL_DOWN should be removed from thingy53_nrf5340_cpunet.dts but i would appreciate if @anangl could have a look also. Sorry for the noise.

So based on the two above statements I've removed the nrf_radio_fem node from thingy53_nrf5340_cpunet.dts and added GPIO_PULL_DOWN in the thingy53_nrf5340_fem.dts to the pins.

Was this correct? @emob-nordic @jciupis
I haven't seen GPIO_PULL_DOWN in the other definitions of the nrf21540_fem, as well as the user guide for nRF2150 in downstream.

@eobalski
Copy link

eobalski commented Dec 8, 2021

Sorry for iterate review. I believe you should also remove this: https://github.com/zephyrproject-rtos/zephyr/blob/main/boards/arm/thingy53_nrf5340/thingy53_nrf5340_cpunet.dts#L53:L59 when u include thingy53_nrf5340_fem.dts in thingy53_nrf5340_cpunet.dts. I'm yet to find some time to test that with our application. Tossing this one out so i don't forget it.

Other way around, afaik GPIO_PULL_DOWN is correct configuration, so GPIO_PULL_DOWN should be removed from thingy53_nrf5340_cpunet.dts but i would appreciate if @anangl could have a look also. Sorry for the noise.

So based on the two above statements I've removed the nrf_radio_fem node from thingy53_nrf5340_cpunet.dts and added GPIO_PULL_DOWN in the thingy53_nrf5340_fem.dts to the pins.

Was this correct? @emob-nordic @jciupis I haven't seen GPIO_PULL_DOWN in the other definitions of the nrf21540_fem, as well as the user guide for nRF2150 in downstream.

I misspelled that comment. It should be:

... afaik GPIO_ACTIVE_HIGH is correct configuration, so GPIO_PULL_DOWN should be removed

I haven't seen GPIO_PULL_DOWN in the other definitions of the nrf21540_fem, as well as the user guide for nRF2150 in downstream.

Exactly.. only GPIO_ACTIVE_HIGH flag should be added to FEM pins.

@joerchan joerchan force-pushed the nrf-remove-thing53-fem-pins branch from 25ccca6 to 5a5e071 Compare December 8, 2021 12:18
@joerchan
Copy link
Contributor Author

joerchan commented Dec 8, 2021

@emob-nordic Thank you for clearing that up, that makes much more sense. I thought that statement looked contradictory.

I've updated the commit to move the thingy53 nrf_radio_fem to be common for both app and net core, and that removing the GPIO_PULL_DOWN configuration is an intentional change.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just realized we cannot do it like that. This file (*_fem.dts) is included form net core too, but net core does not have spi3 at all.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to find another way to do it, im taking a look into this.

@eobalski
Copy link

diff --git a/boards/arm/thingy53_nrf5340/thingy53_nrf5340_common.dts b/boards/arm/thingy53_nrf5340/thingy53_nrf5340_common.dts
index 35a3d6c048..efd136c866 100644
--- a/boards/arm/thingy53_nrf5340/thingy53_nrf5340_common.dts
+++ b/boards/arm/thingy53_nrf5340/thingy53_nrf5340_common.dts
@@ -83,6 +83,14 @@
 		regulator-boot-on;
 	};
 
+	nrf_radio_fem: fem {
+		compatible = "nordic,nrf21540-fem";
+		rx-en-gpios = <&gpio1 11 GPIO_ACTIVE_HIGH>;
+		mode-gpios  = <&gpio1 12 GPIO_ACTIVE_HIGH>;
+		pdn-gpios   = <&gpio1 10 GPIO_ACTIVE_HIGH>;
+		tx-en-gpios = <&gpio0 30 GPIO_ACTIVE_HIGH>;
+	};
+
 	aliases {
 		sw0 = &button0;
 		sw1 = &button1;
@@ -171,6 +179,14 @@
 		reg = <1>;
 		int1-gpios = <&gpio0 23 0>;
 	};
+
+	nrf_radio_fem_spi: fem_spi@2 {
+		compatible = "nordic,nrf21540-fem-spi";
+		status = "okay";
+		reg = <2>;
+		label = "FEM_SPI_IF";
+		spi-max-frequency = <8000000>;
+	};
 };
 
 /* UART0 GPIOs can be configured for other use-cases */
diff --git a/boards/arm/thingy53_nrf5340/thingy53_nrf5340_cpuapp.dts b/boards/arm/thingy53_nrf5340/thingy53_nrf5340_cpuapp.dts
index 8041163534..eabf1ba81c 100644
--- a/boards/arm/thingy53_nrf5340/thingy53_nrf5340_cpuapp.dts
+++ b/boards/arm/thingy53_nrf5340/thingy53_nrf5340_cpuapp.dts
@@ -7,7 +7,6 @@
 /dts-v1/;
 #include <nordic/nrf5340_cpuapp_qkaa.dtsi>
 #include "thingy53_nrf5340_common.dts"
-#include "thingy53_nrf5340_fem.dts"
 
 / {
 	model = "Nordic Thingy53 NRF5340 Application";
diff --git a/boards/arm/thingy53_nrf5340/thingy53_nrf5340_cpuapp_ns.dts b/boards/arm/thingy53_nrf5340/thingy53_nrf5340_cpuapp_ns.dts
index 427a9af53c..8e0377672a 100644
--- a/boards/arm/thingy53_nrf5340/thingy53_nrf5340_cpuapp_ns.dts
+++ b/boards/arm/thingy53_nrf5340/thingy53_nrf5340_cpuapp_ns.dts
@@ -7,7 +7,6 @@
 /dts-v1/;
 #include <nordic/nrf5340_cpuappns_qkaa.dtsi>
 #include "thingy53_nrf5340_common.dts"
-#include "thingy53_nrf5340_fem.dts"
 
 / {
 	model = "Nordic Thingy53 NRF5340 Application";
diff --git a/boards/arm/thingy53_nrf5340/thingy53_nrf5340_cpunet.dts b/boards/arm/thingy53_nrf5340/thingy53_nrf5340_cpunet.dts
index c4caf2c417..da48526095 100644
--- a/boards/arm/thingy53_nrf5340/thingy53_nrf5340_cpunet.dts
+++ b/boards/arm/thingy53_nrf5340/thingy53_nrf5340_cpunet.dts
@@ -49,6 +49,15 @@
 			label = "RGB blue LED";
 		};
 	};
+
+	nrf_radio_fem: fem {
+		compatible = "nordic,nrf21540-fem";
+		rx-en-gpios = <&gpio1 11 GPIO_ACTIVE_HIGH>;
+		mode-gpios  = <&gpio1 12 GPIO_ACTIVE_HIGH>;
+		pdn-gpios   = <&gpio1 10 GPIO_ACTIVE_HIGH>;
+		tx-en-gpios = <&gpio0 30 GPIO_ACTIVE_HIGH>;
+	};
+
 	aliases {
 		sw0 = &button0;
 		sw1 = &button1;
@@ -130,6 +139,5 @@
 	};
 };
 
-#include "thingy53_nrf5340_fem.dts"
 /* Include shared RAM configuration file */
 #include "thingy53_nrf5340_shared_sram_planning_conf.dts"
diff --git a/boards/arm/thingy53_nrf5340/thingy53_nrf5340_fem.dts b/boards/arm/thingy53_nrf5340/thingy53_nrf5340_fem.dts
deleted file mode 100644
index 6cca49499b..0000000000
--- a/boards/arm/thingy53_nrf5340/thingy53_nrf5340_fem.dts
+++ /dev/null
@@ -1,27 +0,0 @@
-/*
- * Copyright (c) 2021 Nordic Semiconductor ASA
- *
- * SPDX-License-Identifier: Apache-2.0
- */
-/ {
-	nrf_radio_fem: nrf21540_fem {
-		compatible = "nordic,nrf21540-fem";
-		rx-en-gpios = <&gpio1 11 GPIO_ACTIVE_HIGH>;
-		mode-gpios  = <&gpio1 12 GPIO_ACTIVE_HIGH>;
-		pdn-gpios   = <&gpio1 10 GPIO_ACTIVE_HIGH>;
-		tx-en-gpios = <&gpio0 30 GPIO_ACTIVE_HIGH>;
-		spi-if = <&nrf_radio_fem_spi>;
-	};
-};
-
-fem_spi: &spi3 {
-	status = "okay";
-
-	nrf_radio_fem_spi: nrf21540_fem_spi@2 {
-		compatible = "nordic,nrf21540-fem-spi";
-		status = "okay";
-		reg = <2>;
-		label = "FEM_SPI_IF";
-		spi-max-frequency = <8000000>;
-	};
-};

I couldn't push to your branch so im including the patch here. Use git apply, it should work well.
Squash to one commit as this change will be fairly simple after that.
In the commit message i would suggest to write something like:

* Remove deprecated code from Thigny:53 startup code.
* Update and align FEM pins to match the description from
  ug_radio_fem.rst

Above changes are required so that mpsl_fem driver could correctly
reasign pins to network core and so that pins could be visible from
network core.

@eobalski
Copy link

Summon @anangl @mbolivar-nordic @galak @MaureenHelm as my review is not enough to get this in, someone from maintainers should have a look too.

@joerchan joerchan force-pushed the nrf-remove-thing53-fem-pins branch from 5a5e071 to d702739 Compare December 10, 2021 15:18
@joerchan
Copy link
Contributor Author

@emob-nordic Thank you. Updated with your change now.

@joerchan joerchan force-pushed the nrf-remove-thing53-fem-pins branch from d702739 to 2f60e45 Compare December 10, 2021 15:20
@eobalski
Copy link

I forgot about this, its important for mpsl_fem driver as i can see. @jciupis would that be enough?

diff --git a/boards/arm/thingy53_nrf5340/thingy53_nrf5340_common.dts b/boards/arm/thingy53_nrf5340/thingy53_nrf5340_common.dts
index efd136c866..7409631e07 100644
--- a/boards/arm/thingy53_nrf5340/thingy53_nrf5340_common.dts
+++ b/boards/arm/thingy53_nrf5340/thingy53_nrf5340_common.dts
@@ -89,6 +89,7 @@
 		mode-gpios  = <&gpio1 12 GPIO_ACTIVE_HIGH>;
 		pdn-gpios   = <&gpio1 10 GPIO_ACTIVE_HIGH>;
 		tx-en-gpios = <&gpio0 30 GPIO_ACTIVE_HIGH>;
+		spi-if = <&nrf_radio_fem_spi>;
 	};
 
 	aliases {

@joerchan joerchan force-pushed the nrf-remove-thing53-fem-pins branch from 2f60e45 to bc09d12 Compare December 10, 2021 15:29
@eobalski eobalski self-requested a review December 10, 2021 15:30
Update outdated nrf21540 FEM pins configuration.
New pins have been added and the mcu selection is handled by the MPSL
FEM module.

The current setup would not work with TFM enabled since the mcu select
code would not be included in the secure image.
Since this is the responsebility of the MPSL FEM module remove it so
that there is only one place that needs to be updated for TFM.

Copy FEM pin configuration for Thingy:53 board from cpunet device tree
configuration to the application device tree configuration so that it is
added for both application and network core as documented in user guide
for FEM in GPIO mode.

Add nrf_radio_fem_spi node to application device tree configuration.

This removes the GPIO_PULL_DOWN configuration which was not correct.

Signed-off-by: Joakim Andersson <[email protected]>
@joerchan joerchan force-pushed the nrf-remove-thing53-fem-pins branch from bc09d12 to b7acc70 Compare December 10, 2021 15:49
@joerchan
Copy link
Contributor Author

@galak @MaureenHelm Need a second approver

Copy link
Member

@carlescufi carlescufi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@carlescufi carlescufi merged commit b57ed25 into zephyrproject-rtos:main Jan 4, 2022
@joerchan joerchan deleted the nrf-remove-thing53-fem-pins branch January 4, 2022 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants