boards: shields: nrf2240ek: 500mA pmic current limit on bootup #17140

ankuns · 2024-09-03T08:33:32Z

Bringing objects close to the PCB antenna of the nRF2240EK might
cause voltage drops which could in effect reset the nRF2240 what is
invisible to the remaining application.
To counteract this phenomenon the current limit of the PMIC
present on the nRF2240EK is increased to 500mA.

NordicBuilder · 2024-09-03T08:34:07Z

The following west manifest projects have been modified in this Pull Request:

Name	Old Revision	New Revision	Diff
zephyr	nrfconnect/sdk-zephyr@`b53d93c`	nrfconnect/sdk-zephyr#1992	nrfconnect/sdk-zephyr#1992/files

Note: This message is automatically posted and updated by the Manifest GitHub Action.

NordicBuilder · 2024-09-03T08:34:26Z

CI Information

^{To view the history of this post, clich the 'edited' button above}
Build number: 9

Inputs:

Sources:

sdk-nrf: PR head: fee8112ff1d8d3773686f83afaa343e9cff2bf27

more details

sdk-nrf:

PR head: fee8112ff1d8d3773686f83afaa343e9cff2bf27
merge base: 9c4ed0d8d0fc09dd48efcb37aff2033bfcbefa10
target head (main): 9c4ed0d8d0fc09dd48efcb37aff2033bfcbefa10
Diff

Github labels

Enabled	Name	Description
	ci-disabled	Disable the ci execution
	ci-all-test	Run all of ci, no test spec filtering will be done
	ci-force-downstream	Force execution of downstream even if twister fails
	ci-run-twister	Force run twister
	ci-run-zephyr-twister	Force run zephyr twister

List of changed files detected by CI (2)

boards
│  ├── shields
│  │  ├── nrf2240ek
│  │  │  ├── nrf2240ek.conf
│  │  │  │ nrf2240ek.overlay

Outputs:

Toolchain

Version: 9338b70c26
Build docker image: docker-dtr.nordicsemi.no/sw-production/ncs-build:9338b70c26_81ed5a52d6

Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped; ⚠️ Quarantine

◻️ Toolchain - Skipped: existing toolchain is used
✅ Build twister
- sdk-nrf test count: 76
✅ Integration tests
- ✅ test-fw-nrfconnect-fem

Disabled integration tests

- desktop52_verification
- doc-internal
- test_ble_nrf_config
- test-fw-nrfconnect-apps
- test-fw-nrfconnect-ble_mesh
- test-fw-nrfconnect-ble_samples
- test-fw-nrfconnect-boot
- test-fw-nrfconnect-chip
- test-fw-nrfconnect-nfc
- test-fw-nrfconnect-nrf-iot_cloud
- test-fw-nrfconnect-nrf-iot_libmodem-nrf
- test-fw-nrfconnect-nrf-iot_lwm2m
- test-fw-nrfconnect-nrf-iot_mosh
- test-fw-nrfconnect-nrf-iot_nrf_provisioning
- test-fw-nrfconnect-nrf-iot_positioning
- test-fw-nrfconnect-nrf-iot_samples
- test-fw-nrfconnect-nrf-iot_serial_lte_modem
- test-fw-nrfconnect-nrf-iot_thingy91
- test-fw-nrfconnect-nrf-iot_zephyr_lwm2m
- test-fw-nrfconnect-nrf_crypto
- test-fw-nrfconnect-proprietary_esb
- test-fw-nrfconnect-ps
- test-fw-nrfconnect-rpc
- test-fw-nrfconnect-rs
- test-fw-nrfconnect-tfm
- test-fw-nrfconnect-thread
- test-fw-nrfconnect-zigbee
- test-low-level
- test-sdk-audio
- test-sdk-dfu
- test-sdk-find-my
- test-sdk-mcuboot
- test-sdk-pmic-samples
- test-sdk-sidewalk
- test-sdk-wifi

Note: This message is automatically posted and updated by the CI

NordicBuilder · 2024-09-03T09:14:16Z

You can find the documentation preview for this PR at this link. It will be updated about 10 minutes after the documentation build succeeds.

Note: This comment is automatically posted by the Documentation Publishing GitHub Action.

ankuns · 2024-09-04T07:12:15Z

boards/shields/nrf2240ek/src/shield.c

+{
+	int err;
+	struct i2c_dt_spec bus = I2C_DT_SPEC_GET(DT_NODELABEL(nrf2240ek_pmic));
+


Rationale for direct i2c writes.

The regulator.h API promises the function regulator_set_current_limit
https://github.com/nrfconnect/sdk-zephyr/blob/fb6ca20a06ba31a3dc1b86d6986d48ad4a5760b0/include/zephyr/drivers/regulator.h#L604

but it is not implemented
https://github.com/nrfconnect/sdk-zephyr/blob/fb6ca20a06ba31a3dc1b86d6986d48ad4a5760b0/drivers/regulator/regulator_npm1300.c#L637

Why not add an implementation of regulator_set_current_limit() there?

regulator_set_current_limit for VBUS is not implemented because USB interface of PMIC is not considered a regulator. BUCK1, BUCK2, LDO1 and LDO2 are regulators, and they indeed don't support setting of a current limit.

There is charger interface you could use, I'll post a more detailed answer below in a moment.

anangl · 2024-09-04T08:58:50Z

boards/shields/nrf2240ek/src/shield.c

+{
+	int err;
+	struct i2c_dt_spec bus = I2C_DT_SPEC_GET(DT_NODELABEL(nrf2240ek_pmic));
+


Why not add an implementation of regulator_set_current_limit() there?

anangl · 2024-09-04T09:01:03Z

boards/shields/nrf2240ek/src/shield.c

+	return nrf2240ek_pmic_init();
+}
+
+SYS_INIT(shield_init, POST_KERNEL, CONFIG_I2C_INIT_PRIORITY);


This does not guarantee that the I2C driver will be initialized first. The priority value should be greater.

addrssed in commit ece601a

WalkingTalkingPotato · 2024-09-04T10:34:16Z

west.yml

@@ -72,7 +72,7 @@ manifest:
    # https://developer.nordicsemi.com/nRF_Connect_SDK/doc/latest/zephyr/guides/modules.html
    - name: zephyr
      repo-path: sdk-zephyr
-      revision: fb6ca20a06ba31a3dc1b86d6986d48ad4a5760b0


Is this related to the rest of the changes?

To setup current limit on bootup I need to put C code within shield. It was impossible in current sdk-zephyr, but in upstream zephyr there is a commit that makes it possible. Cherry-pick from upstream zephyr and sha update is in separate commit. It could be done in a separate PR, but if I did it in a separate PR one could ask in that PR why is this cherry-pick necessary.
So this change is necessary here, but not core of the change.

WalkingTalkingPotato

I believe you could implement the workaround you need using existing functions. The driver with the interface you need is "npm1300 charger":

https://github.com/nrfconnect/sdk-zephyr/blob/fb6ca20a06ba31a3dc1b86d6986d48ad4a5760b0/dts/bindings/sensor/nordic%2Cnpm1300-charger.yaml
https://github.com/nrfconnect/sdk-zephyr/blob/fb6ca20a06ba31a3dc1b86d6986d48ad4a5760b0/drivers/sensor/nordic/npm1300_charger/npm1300_charger.c

Most of its settings come from device tree and it implements sensor interface.

I believe the device tree setting vbus-limit-microamp is what you would want to use, see example:
https://github.com/nrfconnect/sdk-zephyr/blob/fb6ca20a06ba31a3dc1b86d6986d48ad4a5760b0/boards/shields/npm1300_ek/npm1300_ek.overlay#L52

It sets a different register than the one you have used. I believe the difference is that the one you used is intended for usage for when USB host tells the device (in the process of USB enumeration if I'm not mistaken) what is the actual maximal current it supports, and the one that is set by this dts setting is intended for default initial value that is used before host tells what is the limit. nPM1300 does not handle this conversation with a host, that's why it is implemented this way.

If you're sure that you want to use the registers you've used, you'll have to call sensor_attr_set():

const struct sensor_value currentLimit = { .val1 = 0, val2 = 500000 };
sensor_attr_set(charger_device, SENSOR_CHAN_CURRENT, SENSOR_ATTR_CONFIGURATION, &currentLimit);

Please note that you must be sure that the source is indeed able to provide the current you need. I believe that in some cases just drawing 500 mA would be an issue.

ankuns · 2024-09-06T13:55:25Z

Regarding #17140 (review) #17140 (comment) #17140 (comment) and taking into account the F2F discussion, would it be okay to merge it as is.
It is mentioned that this is a workaround, it is intentionally not mentioned what type of pmic is in use.

Bringing objects close to the PCB antenna of the nRF2240EK might cause voltage drops which could in effect reset the nRF2240 what is invisible to the remaining application. To counteract this phenomenon the current limit of the nPM1300 pmic present on the nRF2240EK is increased to 500mA. Signed-off-by: Andrzej Kuros <[email protected]>

ankuns · 2024-09-09T08:34:10Z

@anangl @WalkingTalkingPotato I changed the approach. The workaround uses now the advised npm1300_charger. No more direct i2c writes. Please review.

github-actions bot added manifest changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. labels Sep 3, 2024

NordicBuilder added manifest-zephyr DNM labels Sep 3, 2024

ankuns force-pushed the nrf2240ek_set_pmic_current_limit_on_bootup branch from e0394f4 to 2eb329b Compare September 3, 2024 13:20

ankuns marked this pull request as ready for review September 3, 2024 13:28

ankuns requested review from anangl, carlescufi and tejlmand as code owners September 3, 2024 13:28

ankuns requested a review from piotrkoziar September 3, 2024 13:28

ankuns force-pushed the nrf2240ek_set_pmic_current_limit_on_bootup branch from 2eb329b to d3c6bca Compare September 4, 2024 06:34

ankuns commented Sep 4, 2024

View reviewed changes

anangl reviewed Sep 4, 2024

View reviewed changes

WalkingTalkingPotato reviewed Sep 4, 2024

View reviewed changes

WalkingTalkingPotato requested changes Sep 4, 2024

View reviewed changes

ankuns force-pushed the nrf2240ek_set_pmic_current_limit_on_bootup branch 2 times, most recently from b5a7803 to ece601a Compare September 6, 2024 13:49

anangl self-requested a review September 6, 2024 13:53

ankuns requested a review from WalkingTalkingPotato September 6, 2024 13:57

ankuns force-pushed the nrf2240ek_set_pmic_current_limit_on_bootup branch from ece601a to 77b0376 Compare September 9, 2024 08:29

ankuns force-pushed the nrf2240ek_set_pmic_current_limit_on_bootup branch from 77b0376 to fee8112 Compare September 9, 2024 08:31

github-actions bot removed the manifest label Sep 9, 2024

ankuns removed the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Sep 9, 2024

ankuns removed the DNM label Sep 9, 2024

ankuns removed the manifest-zephyr label Sep 9, 2024

anangl approved these changes Sep 9, 2024

View reviewed changes

WalkingTalkingPotato approved these changes Sep 9, 2024

View reviewed changes

rlubos merged commit 78748c2 into nrfconnect:main Sep 9, 2024
13 of 15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

boards: shields: nrf2240ek: 500mA pmic current limit on bootup #17140

boards: shields: nrf2240ek: 500mA pmic current limit on bootup #17140

ankuns commented Sep 3, 2024 •

edited

Loading

NordicBuilder commented Sep 3, 2024 •

edited

Loading

NordicBuilder commented Sep 3, 2024 •

edited

Loading

NordicBuilder commented Sep 3, 2024

ankuns Sep 4, 2024

anangl Sep 4, 2024

WalkingTalkingPotato Sep 4, 2024

anangl Sep 4, 2024

anangl Sep 4, 2024

ankuns Sep 6, 2024

WalkingTalkingPotato Sep 4, 2024

ankuns Sep 4, 2024

WalkingTalkingPotato left a comment

ankuns commented Sep 6, 2024

ankuns commented Sep 9, 2024

boards: shields: nrf2240ek: 500mA pmic current limit on bootup #17140

boards: shields: nrf2240ek: 500mA pmic current limit on bootup #17140

Conversation

ankuns commented Sep 3, 2024 • edited Loading

NordicBuilder commented Sep 3, 2024 • edited Loading

NordicBuilder commented Sep 3, 2024 • edited Loading

CI Information

Inputs:

Sources:

Github labels

Outputs:

Toolchain

Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped; ⚠️ Quarantine

NordicBuilder commented Sep 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WalkingTalkingPotato left a comment

Choose a reason for hiding this comment

ankuns commented Sep 6, 2024

ankuns commented Sep 9, 2024

ankuns commented Sep 3, 2024 •

edited

Loading

NordicBuilder commented Sep 3, 2024 •

edited

Loading

NordicBuilder commented Sep 3, 2024 •

edited

Loading