Skip to content

Conversation

@soburi
Copy link
Member

@soburi soburi commented Feb 4, 2023

add support for rpi_pico regulator.

Signed-off-by: TOKITA Hiroshi [email protected]


Since the RaspberryPi Pico does not expose the output wiring of the Core Supply Regulator, I scraped the back of the board as shown in the picture, connected it to the ADC, and run the 'tests/drivers/regulator/voltage/' test.
unnamed

In RaspberryPi Pico, the Core voltage is supplied from the Core Supply Regulator, and if you disable the Core Supply Regulator or set the output to Hi-Z, the MCU will stop.
To fully test the behavior would have required designing a special board to supply the core voltage externally. So which is not tested.
From this reason, made the default settings prevent running it.

Copy link
Contributor

Choose a reason for hiding this comment

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

The description and help string of this Kconfig need to be updated

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for reviewing.
I updated the description.

Copy link
Contributor

Choose a reason for hiding this comment

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

Description and help string need to be updated, unless the RPI is using an NXP regulator (from the datasheet, this seems to be an internal regulator)

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, this is embarrassing...
Fixed it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've not familiar with this chip, but it seems to me that lower voltages could be useful if the chip was underclocked or entering a low power mode. Is there no situation where the 800000uV output would be useful?

Copy link
Member Author

Choose a reason for hiding this comment

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

I noticed these values are usable when externally supplying the core voltage. (RP2040 usually drives by voltage from the built-in regulator, but can configure not use the regulator.)
Thus, I changed to make all values available.
However, this does not pass the test with this board.
So I made it possible to set upper and lower test voltage limits in the test so that we could exclude unstable settings from the test.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a good solution, thank you for enabling this feature

Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nit- I would like to see a mode defined for "HI Z disabled", something like REGULATOR_RPI_PICO_MODE_RUN. Thi way, the allowed-modes and initial-mode properties don't have to simply be set to 0, and a user looking to change the regulator mode will have an easier time understanding that their options are RUN mode or HI Z mode

Copy link
Member Author

@soburi soburi Feb 5, 2023

Choose a reason for hiding this comment

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

It's make a sense.
I added REGULATOR_RPI_PICO_MODE_NORMAL.
(I think the terminology of the RUN cause a complex situation in case if not enable regulator-boot-on property. So I chose NORMAL for it.)

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have a board to test on, but won't you need to subtract VREG_VSEL_IDX_OFFSET here to get the correct voltage? As an aside, the linear range API also supports multiple values mapping to the same voltage (see here for an example: https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/regulator/regulator_pca9420.c#L127), which might be the best way to handle this regulator

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for suggestion.
I rewrite with correctly linear range API usage, and removed VREG_VSEL_IDX_OFFSET.

danieldegrasse
danieldegrasse previously approved these changes Feb 6, 2023
Copy link
Member

@gmarull gmarull left a comment

Choose a reason for hiding this comment

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

thanks for this change, a few questions/change requests

Comment on lines 196 to 201
Copy link
Member

Choose a reason for hiding this comment

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

note: these flags are used to limit regulator operating range, not to describe hardware capabilities. So in general, they should be put at board level. Since this is put at the SoC DTS files, it looks like for example the always-on/boot-on is something that can't be configured, so driver doesn't need to implement enable/disable.

Copy link
Member Author

@soburi soburi Feb 11, 2023

Choose a reason for hiding this comment

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

Thank you for reviewing.

I moved these properties into boards/.
And it left properties that need to prevent the regulator from turning off only.

Comment on lines 162 to 164
Copy link
Member

Choose a reason for hiding this comment

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

note: parent device is not required, just drop it if not needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Delete the parent device and make a flattened structure.

Comment on lines 23 to 24
Copy link
Member

Choose a reason for hiding this comment

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

note: it looks like you only have one regulator (CORE), so there's no need to store ranges in the instance config struct. Just use core_ranges directly.

Copy link
Member Author

@soburi soburi Feb 11, 2023

Choose a reason for hiding this comment

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

Make it use core_ranges and num_core_ranges directly.
Removed ranges and num_ranges from struct.

Copy link
Member

Choose a reason for hiding this comment

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

nit: just make config->reg a vreg_and_chip_reset_hw_t * pointer

Copy link
Member Author

Choose a reason for hiding this comment

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

Applied it.

Copy link
Member

Choose a reason for hiding this comment

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

volts can take int32_t range (includes negatives)

Copy link
Member Author

@soburi soburi Feb 11, 2023

Choose a reason for hiding this comment

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

Changing default to INT32_MIN.

Copy link
Member

Choose a reason for hiding this comment

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

maybe add a comment on how to wire this up (seems you need to solder a wire)

Copy link
Member Author

Choose a reason for hiding this comment

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

I can't attach a picture here, so I described it in the comments.

gmarull
gmarull previously approved these changes Feb 23, 2023
Add support for rpi_pico regulator.

Signed-off-by: TOKITA Hiroshi <[email protected]>
Add a regulator node to keep it always on.

Signed-off-by: TOKITA Hiroshi <[email protected]>
Add a regulator node to keep it always on.

Signed-off-by: TOKITA Hiroshi <[email protected]>
Add a regulator node to keep it always on.

Signed-off-by: TOKITA Hiroshi <[email protected]>
Add min-microvolt/max-microvolt to set the range for voltages for testing.
It filter out ranges that cannot test correctly in the configuration.

Signed-off-by: TOKITA Hiroshi <[email protected]>
Add overlay for RaspberryPi Pico's core-supply regulator test.

Signed-off-by: TOKITA Hiroshi <[email protected]>
@soburi
Copy link
Member Author

soburi commented Mar 2, 2023

@danieldegrasse
Could you please re-approve this PR if there are no problems?

@carlescufi carlescufi merged commit 875b0c7 into zephyrproject-rtos:main Mar 2, 2023
@soburi soburi deleted the pico_regulator branch May 28, 2023 11:51
soburi added a commit to soburi/zephyr that referenced this pull request Aug 2, 2023
Add myself as codeowner of previously committed driver.

- zephyrproject-rtos#54456

Signed-off-by: TOKITA Hiroshi <[email protected]>
fabiobaltieri pushed a commit that referenced this pull request Aug 3, 2023
Add myself as codeowner of previously committed driver.

- #54456

Signed-off-by: TOKITA Hiroshi <[email protected]>
kunoh pushed a commit to kunoh/zephyr that referenced this pull request Aug 7, 2023
Add myself as codeowner of previously committed driver.

- zephyrproject-rtos#54456

Signed-off-by: TOKITA Hiroshi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Devicetree Binding PR modifies or adds a Device Tree binding area: Regulators platform: Raspberry Pi Pico Raspberry Pi Pico (RPi Pico)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants