boards: nrf7002dk: Fix SR coexistence GPIOs ownership

[muraliThokala]

In nRF53 SoC, the CPUAPP owns all the GPIOs by default. Therefore, even if the GPIOs are exclusively used by CPUNET, we must redefine them for CPUAPP and then set up GPIO forwarding to CPUNET using SoC registers (this is done as part of the MPSL CX initialization).

Previously, these GPIOs were mistakenly removed from CPUAPP and defined only for CPUNET. This caused the GPIOs to be
available for CPUNET but still owned by CPUAPP, breaking SR coexistence.

[github-actions]

Hello @muraliThokala, 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. 😊

[krish2718]

Can you please elaborate commit log with affects of having/not having these entries?

[krish2718]

Add missing coex node in cpuapp. Without this, coexistence connections are broken and will not function.

it needs more details, here's my stab, please review:

boards: nrf7002dk: Fix SR co-existence GPIOs ownership

In nRF53 SoC, the CPUAPP owns all the GPIOs by default.
So, though the GPIOs are only used by CPUNET, we need to 
redefine them for CPUAPP and then setup GPIO forwarding
to CPUNET using SoC registers (done as part of MPSL CX init).

These GPIOs were mistakenly removed earlier from CPUAPP,
and defined only for CPUNET causing GPIOs to be available
for CPUNET but owned by CPUAPP and breaking SR co-existence.

[bjarki-andreasen]

This is done using GPIO forwarding:

	gpio_fwd: nrf-gpio-forwarder {
		compatible = "nordic,nrf-gpio-forwarder";
		status = "okay";
		uart {
			gpios = <&gpio1 1 0>, <&gpio1 0 0>, <&gpio0 11 0>, <&gpio0 10 0>;
		};
	};

in this case add:

&gpio_fwd {
        coex {
                gpios = <&gpio0 28>, <&gpio0 30>, <&gpio0 24>, <&gpio 29>;
        };
};

or add:

        coex {
                gpios = <&gpio0 28>, <&gpio0 30>, <&gpio0 24>, <&gpio 29>;
        };

directly to gpio_fwd

[krish2718]

Not just GPIO forwarding, we also need this for MPSL CX, https://github.com/nrfconnect/sdk-nrf/blob/main/subsys/mpsl/cx/nrf700x/mpsl_cx_nrf700x.c#L250 in the downstream.

[bjarki-andreasen]

Not just GPIO forwarding, we also need this for MPSL CX, https://github.com/nrfconnect/sdk-nrf/blob/main/subsys/mpsl/cx/nrf700x/mpsl_cx_nrf700x.c#L250 in the downstream.

The commit clearly states that its purpose is to forward the GPIOs, which is done using gpio forwarding upstream. If a downstream app wants to do it differently, add an overlay downstream.

[bjarki-andreasen]

What is the issue with just adding or using

zephyr/soc/nordic/nrf53/soc.c

Lines 565 to 574 in 798122e

	#if defined(CONFIG_SOC_NRF_GPIO_FORWARDER_FOR_NRF5340)
	static const uint8_t forwarded_psels[] = {
	DT_FOREACH_STATUS_OKAY(nordic_nrf_gpio_forwarder, ALL_GPIOS_IN_FORWARDER)
	};
	for (int i = 0; i < ARRAY_SIZE(forwarded_psels); i++) {
	soc_secure_gpio_pin_mcu_select(forwarded_psels[i], NRF_GPIO_PIN_SEL_NETWORK);
	}
	#endif

downstream anyway?

[krish2718]

No issue in using DTS to forward GPIOs, just stating that the commit has multiple purposes. If a downstream purpose isn't acceptable, then we can do the DTS based GPIO forwarding in upstream and handle rest in downstream (cannot use an app overaly as this has to be for all apps using that board).

[bjarki-andreasen]

No issue in using DTS to forward GPIOs, just stating that the commit has multiple purposes. If a downstream purpose isn't acceptable, then we can do the DTS based GPIO forwarding in upstream and handle rest in downstream (cannot use an app overaly as this has to be for all apps using that board).

I don't get it really. A board overlay seems fitting I guess, but still, why is downstream not just using the same forwarding as upstream? Seems we are adding duplicate or even redundant code both in dts and downstream.

It wont't hurt anyone adding the coex interface to the cpuapp I guess, but it feels wrong

[krish2718]

No issue in using DTS to forward GPIOs, just stating that the commit has multiple purposes. If a downstream purpose isn't acceptable, then we can do the DTS based GPIO forwarding in upstream and handle rest in downstream (cannot use an app overaly as this has to be for all apps using that board).

I don't get it really. A board overlay seems fitting I guess, but still, why is downstream not just using the same forwarding as upstream? Seems we are adding duplicate or even redundant code both in dts and downstream.

The downstream code has a conditional forward CONFIG_MPSL_CX_PIN_FORWARDER, I am not aware of the reason, but may be it's there exactly for this purpose to disable manual forward and instead use DTS?
nrfconnect/sdk-nrf@7425b57 adds this Kconfig but 81c48c3 DTS forwarding was added a little bit later (as a better approach?) by the same author, may be we can now remove the downstream code, let me check internally.

[krish2718]

@ankuns @martintv any comments on above MPSL changes?

[ankuns]

Why not to use generic gpio forwarder node for this purpose?

zephyr/boards/nordic/nrf7002dk/nrf5340_cpuapp_common.dtsi

Line 94 in 4272353

gpio_fwd: nrf-gpio-forwarder {

You could add there:

	gpio_fwd: nrf-gpio-forwarder {
		compatible = "nordic,nrf-gpio-forwarder";
		status = "okay";
		uart {
			gpios = <&gpio1 1 0>, <&gpio1 0 0>, <&gpio1 5 0>, <&gpio1 4 0>;
		};
		coex {
			gpios = <&gpio0 28 0>, <&gpio0 30 0>, <&gpio0 24 0>, <&gpio0 29>;
		};
	};

[ankuns]

... as already mentioned by #83028 (review)

[krish2718]

Sure, we agreed to use that, but wanted to get some context about removing the manual forwarding from MPSL in NCS.