Skip to content

[nrf noup] dts: nrf21540-fem default tx-en-settle-time-us=26 #2762

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

[nrf noup] dts: nrf21540-fem default tx-en-settle-time-us=26 #2762

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion dts/bindings/net/wireless/nordic,nrf21540-fem.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ properties:
This must be present to support SPI control of the FEM.
tx-en-settle-time-us:
type: int
default: 11
default: 26
Copy link
Contributor

Choose a reason for hiding this comment

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

why can't this be submitted upstream and brought in as fromlist/fromtree?

Copy link
Contributor Author

@ankuns ankuns Apr 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@nordicjm As you see description of this property below

Settling time in microseconds from state PG to TX.
Default value is based on Table 6 of the nRF21540 Product
Specification (v1.0), and can be overridden for tuned
configurations.

This is exactly this case, tuned configuration for use in NCS with MPSL and protocol drivers we deliver in NCS. The value 11 is perfect from Product Specification point of view. But due the way protocol drivers and use the FEM API the value is increased by 15us to 26 to avoid spurious emission issue.
Eventually, if we find a better solution, we could revert it, but now we would like to have main aligned with v3.0-branch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nordicjm is such justification enough?

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not make this the default upstream though, is there a downside if we did that?

Copy link
Contributor Author

@ankuns ankuns Apr 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@carlescufi @nordicjm I can make the change upstream, but I deliberately chose to do it just in downstream sdk-zephyr, because the requirement to enable TX_EN pin for the nrf21540 earlier comes from the way the SDC, 802.15.4 Radio Driver and other protocols/samples work. The TX_EN pin must be enabled is such a moment that the FEM'a PA is ready when the RF power starts to rise (no such parameter in public and even internal product specifications for the nRF SoCs and no such parameter in nordic,nrf-radio compatible that I could add and pass the sum to the MPSL configuration API). All protocols in NCS use target time when the FEM should be ready as the RADIO's READY event which with default tx-en-settle-time-us is too late (the RF power already is emitted in this moment).

On the other hand in upstream Zephyr there is no MPSL, SDC, FEM support and the samples that use FEM.
So the justification applies only to downstream. In upstream, the change does not have justification at all. How would I justify it in upstream, should I write "the change is needed in some downstream fork?", that will not pass.

This change is very small, does the job, fixes an issue important to customers at very low cost.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@carlescufi @nordicjm Is the above justification enough?

Copy link
Contributor

Choose a reason for hiding this comment

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

so why not fix it in boards? This is a default value, it can be overridden, if you need a different value, set it where it is needed

Copy link
Contributor Author

@ankuns ankuns Apr 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

so why not fix it in boards? This is a default value, it can be overridden, if you need a different value, set it where it is needed

"don't repeat yourself" rule in action... No.
It is needed just here where I propose. There are for sure boards made by our customers in their own repos which I cannot access and fix for them. The fix I propose fix the issue for all of them (unless they have overridden the default but that's not the case).

description: |
Settling time in microseconds from state PG to TX.

Expand Down