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.

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

Conversation

ankuns
Copy link
Contributor

@ankuns ankuns commented Apr 14, 2025

The default value of the tx-en-settle-time-us property is set to 26 to avoid spurious emission issue.

Cherry-pick of commit that went to NSC 3.0 release with #2753
Requires MPSL: nrfconnect/sdk-nrfxlib#1725

The default value of the `tx-en-settle-time-us` property is set
to 26 to avoid spurious emission issue.

Signed-off-by: Andrzej Kuros <[email protected]>
@ankuns ankuns marked this pull request as ready for review April 14, 2025 13:39
@@ -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

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

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

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).

@ankuns
Copy link
Contributor Author

ankuns commented May 6, 2025

Replaced by upstream modification approach and with [nrf fromlist] cherry-picked commit #2833

@ankuns ankuns closed this May 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants