Use sky13317 antenna switch in Beagleconnect freedom

[Ayush1325]

• Use antenna switch sky13317 instead of hack
• Base the board_antenna file on cc1352p1_launchxl/board_antenna.c
• This is a continuation of Add sky13317 antenna switch #72332 , boards: beagle: beagleconnect_freedom: Use sky13317 antenna switch #71403 , but also takes in account that only 2 of the 3 switch pins are connected in beagleconnect freedom.

Fixes #71285

[jadonk]

Please:

• Move formatting changes to a different commit, if not a different PR.
• Add GPIO states to DT as well, not just pinmux.
• Verify pinmux/GPIO state for PA-on mode.

[Ayush1325]

To explain some of the comments regarding formatting and stuff, I actually deleted the old board_antenna and replaced it with the one in launchxl. git tried to make it a diff with the old version and thus some changes look weird. I will make changes like static since it makes sense but I would like to leave formatting as is.

[Ayush1325]

I am going to seperate the seemigly format changes to a separate commit but do not, I am simply recreating a commit that is not originally supposed to exist (since I deleted the original file and replaced it with launchxl board_antenna). So somethings might still end up looking weird.

[cfriedt]

It would be good to get this in for the LTS release. I think the gpio dt logic is sound and it fixes a long-standing hack that IIRC has been delayed for a long time.

@Ayush1325 - can you verify that you've done reliable perf measurements with these changes for long-ish periods? @fgrandel would be a good source for that info (I think he had posted a table a while ago) I.e. is the link stable at <G frequencies?

[Ayush1325]

It would be good to get this in for the LTS release. I think the gpio dt logic is sound and it fixes a long-standing hack that IIRC has been delayed for a long time.

@Ayush1325 - can you verify that you've done reliable perf measurements with these changes for long-ish periods? @fgrandel would be a good source for that info (I think he had posted a table a while ago) I.e. is the link stable at <G frequencies?

I will try to make zperf table as well. Do note that the zperf commands used #58439 (comment) is not usable, at least with the default config. The client runs out of Cache entry and never recovers. This has been the case since before the antenna patch. So let me adjust things.

[Ayush1325]

Ok, so here are the results. The config is just the default subg overlay with the following extra options:

CONFIG_NET_L2_IEEE802154_FRAGMENT_REASS_CACHE_SIZE=8

I also tested removing GPIO toggling and using IOC_PORT_RFC_GPO1 and the results were a bit weird. Here is a table of results

Zephyr Version	Rate server (client)	Jitter	Pkt lost
Antenna + No GPIO + IOC_PORT_RFC_GPO1	40 Kbps (41 Kbps)	7.76 ms	0
Antenna + No GPIO Patch	36 Kbps (37 Kbps)	5.27 ms	0
Antenna + IOC_PORT_RFC_GPO1	47 Kbps (48 Kbps)	3.63 ms	0
Antenna	48 Kbps (49 Kbps)	3.13 ms	0
Mainline	48 Kbps (48 Kbps)	3.64 ms	0

As can be seen, there does not seem to be any regression (or improvement in performance) with the antenna patch, which is pretty much what was expected. But the GPIO toggling is definitely required.

Outputs

Antenna + No GPIO + IOC_PORT_RFC_GPO1

uart:~$ zperf udp upload fe80::212:4b00:29b9:98f7 5001 10 64 200K
Remote port is 5001
Connecting to fe80::212:4b00:29b9:98f7
Duration:       10.00 s
Packet size:    64 bytes
Rate:           200 kbps
Starting...
Rate:           200 Kbps
Packet duration 2 ms
-
Upload completed!
Statistics:             server  (client)
Duration:               10.22 s (10.01 s)
Num packets:            804     (805)
Num packets out order:  0
Num packets lost:       0
Jitter:                 7.76 ms
Rate:                   40 Kbps (41 Kbps)

Antenna + IOC_PORT_RFC_GPO1

uart:~$ zperf udp upload fe80::212:4b00:29b9:98f7 5001 10 64 200K
Remote port is 5001
Connecting to fe80::212:4b00:29b9:98f7
Duration:       10.00 s
Packet size:    64 bytes
Rate:           200 kbps
Starting...
Rate:           200 Kbps
Packet duration 2 ms
-
Upload completed!
Statistics:             server  (client)
Duration:               10.17 s (10.01 s)
Num packets:            952     (953)
Num packets out order:  0
Num packets lost:       0
Jitter:                 3.63 ms
Rate:                   47 Kbps (48 Kbps)

Antenna + No GPIO Patch

uart:~$ zperf udp upload fe80::212:4b00:29b9:98f7 5001 10 64 200K
Remote port is 5001
Connecting to fe80::212:4b00:29b9:98f7
Duration:       10.00 s
Packet size:    64 bytes
Rate:           200 kbps
Starting...
Rate:           200 Kbps
Packet duration 2 ms
-
Upload completed!
Statistics:             server  (client)
Duration:               10.20 s (10.02 s)
Num packets:            724     (725)
Num packets out order:  0
Num packets lost:       0
Jitter:                 5.27 ms
Rate:                   36 Kbps (37 Kbps)

Antenna

uart:~$ zperf udp upload fe80::212:4b00:29b9:98f7 5001 10 64 200K
Remote port is 5001
Connecting to fe80::212:4b00:29b9:98f7
Duration:       10.00 s
Packet size:    64 bytes
Rate:           200 kbps
Starting...
Rate:           200 Kbps
Packet duration 2 ms
-
Upload completed!
Statistics:             server  (client)
Duration:               10.14 s (10.01 s)
Num packets:            967     (968)
Num packets out order:  0
Num packets lost:       0
Jitter:                 3.13 ms
Rate:                   48 Kbps (49 Kbps)

Mainline

uart:~$ zperf udp upload fe80::212:4b00:29b9:98f7 5001 10 64 200K
Remote port is 5001
Connecting to fe80::212:4b00:29b9:98f7
Duration:       10.00 s
Packet size:    64 bytes
Rate:           200 kbps
Starting...
ping fe80::212:4b00:29b9:98f7 timeout
Rate:           200 Kbps
Packet duration 2 ms
-
Upload completed!
Statistics:             server  (client)
Duration:               10.11 s (10.01 s)
Num packets:            949     (953)
Num packets out order:  0
Num packets lost:       0
Jitter:                 3.64 ms
Rate:                   48 Kbps (48 Kbps)

Here is a branch with all the patches: https://github.com/Ayush1325/zephyr/tree/bct

[edited by cfriedt] Comparison of above branch
main...Ayush1325:zephyr:bct

[fgrandel]

Hi @Ayush1325 , thank you very much for removing inconsistencies between the BeagleConnect Freedom and the LaunchXL board. Very useful. My comments therefore focus on this intent, as you do not fully achieve it, yet, IMHO.

I'll ensure to re-review very quickly once you've made your changes so they can be included in the LTS release.

[Ayush1325]

I have minimized the diff between ti/cc1352p1_launchxl/board_antenna.c and beagle/beagleconnect_freedom/board_antenna.c

❯ diff boards/beagle/beagleconnect_freedom/board_antenna.c boards/ti/cc1352p1_launchxl/board_antenna.c
3,4d2
<  * Copyright (c) 2021 Jason Kridner, BeagleBoard.org Foundation
<  * Copyright (c) 2024 Ayush Singh <[email protected]>
26,30c24,31
< #define PINCTRL_STATE_ANT_SUBG    1
< #define PINCTRL_STATE_ANT_SUBG_PA 2
<
< #define BOARD_ANT_GPIO_PA   0
< #define BOARD_ANT_GPIO_SUBG 1
---
> #define PINCTRL_STATE_ANT_24G         1
> #define PINCTRL_STATE_ANT_24G_PA      2
> #define PINCTRL_STATE_ANT_SUBG                3
> #define PINCTRL_STATE_ANT_SUBG_PA     4
>
> #define BOARD_ANT_GPIO_24G   0
> #define BOARD_ANT_GPIO_PA    1
> #define BOARD_ANT_GPIO_SUBG  2
112a114,121
>                       }
>               } else /* 2.4 GHz */ {
>                       if (paType == RF_TxPowerTable_HighPA) {
>                               pinctrl_apply_state(ant_pcfg, PINCTRL_STATE_ANT_24G_PA);
>                       } else {
>                               pinctrl_apply_state(ant_pcfg, PINCTRL_STATE_ANT_24G);
>                               /* Manually set the 2.4GHZ antenna switch DIO */
>                               gpio_pin_configure_dt(&ant_gpios[BOARD_ANT_GPIO_24G], 1);

[fgrandel]

Hi @Ayush1325, just minor stuff, that you may or may not fix. except for that ordering/readability problem, that should really be fixed.

[fgrandel]

LGTM after realizing that there doesn't seem to be an ordering issue.

[fgrandel]

@cfriedt / @jadonk Could you re-consider/re-approve based on the recent changes so this could possibly still make it into the LTS. :-)

[jadonk]

I approve. I think we can make it a bit more readable and flexible, but this is reasonable and it will take some time to refactor for any improvements as I'm not yet sure how to express in states in DT.

[aescolar]

Moving milestone to 4.0 as this is adding a new feature and we are way past rc1

[Ayush1325]

Moving milestone to 4.0 as this is adding a new feature and we are way past rc1

Can you explain what you mean by new feature? The antenna switch was already working before this commit. The PR just moves stuff like GPIO to device tree instead of hardcoding the GPIOs here. It does not cause any impact on performance or well, anything else really. Just makes stuff more Zephyr way.