Add regulator driver infrastructure #27360

pabigot · 2020-08-03T19:06:22Z

To support runtime power management we need to be able to control voltage/current regulators. Although there are boards in-tree that use these, there is no devicetree support for describing them. This PR provides support based on Linux bindings.

The API is based on the Linux API but adapted for Zephyr. The enable operation is asynchronous as it may take time to turn a regulator on. The disable operation is synchronous as the caller doesn't need to know when the regulator transitions to off. (Power management infrastructure will, but that's not part of this API.)

On the theory that user space never needs to control system power the API requires supervisor privileges. This may be changed based on feedback. The API is also isr-ok and pre-kernel-ok.

A significant gap is the lack of any ability to configure the kernel init level and priority on a per-device level. Without that this infrastructure is fragile.

The thingy52_nrf52832 board has been updated to provide its two external power rails, but the pseudo-devices that currently control them cannot be replaced until device-specific init priority is supported.

Commit Contents

Introduction of standard devicetree properties for power supply control

d306458c15 devicetree: add properties for power supply control

Primary regulator commits

0afd91e3bd drivers: add infrastructure for regulator devices
d34246f19e drivers: regulator: add GPIO-controlled regulator driver
c1bc4515f6 doc: introduce the regulator driver API
1724be2c7b tests: drivers: regulator: add test for fixed regulator

Add Thingy:52 on-board regulator devices

85c792590c boards: thingy52_nrf52832: add nodes for on-board voltage regulators

Blockers for merge:

merge sys: onoff: support clients using synchronous transitions #27443
update to actually do asynchronous work
tests
merge build: support passing preprocessor flags to DTS processing #27719
determine whether this is usable without device-specific init priorities
documentation on tests and how to select regulator implementation: onoff, sync, or dedicated power-supply

pabigot · 2020-08-03T19:07:36Z

My current thought is to add a power-supply optional property to the SPI and I2C device base binding definitions, in lieu of having to add those to every driver. To be discussed.

pabigot · 2020-08-03T22:47:47Z

The latest push reveals another gap in dependency information: all regulators are included in the image even if only one (or no) device that depends on them is enabled.

Yes, that could be addressed by marking some or all of them status = "disabled"; but that's pretty painful from an application perspective.

drivers/regulator/regulator_gpio.c

include/drivers/regulator.h

galak · 2020-08-04T16:27:17Z

Should we be using regulator-fixed binding/compat instead of regulator-gpio?

carlescufi · 2020-08-04T16:30:46Z

API meeting 4th August 2020:

Initial draft takes the smallest subset possible from the Linux functionality (enable/disable for GPIO-controlled regulators only)
Can be later extended to support things like PMICs and other types of regulators
This would be combined later with a future PMIC driver [RFC] drivers: Add PMIC driver #12097

galak · 2020-08-04T16:41:16Z

Looks like regulator-name is a required property. So thinking we should as well, and try and have label and regulator-name match for Zephyr's purposes at this point.

include/drivers/regulator.h

drivers/regulator/regulator_gpio.c

pabigot · 2020-08-04T17:15:10Z

Looks like regulator-name is a required property. So thinking we should as well, and try and have label and regulator-name match for Zephyr's purposes at this point.

I'm going to add regulator-name (~~though it'll be ignored in the driver~~), but don't agree about documenting that the Zephyr-specific label must match the regulator name. That would limit proper use of the label property in the future, when we are free to make this a human-readable description of the node rather than the globally-unique string used in device_get_binding().

Edit: The documented purpose for regulator-name suggests this might be used in log messages, so maybe it'll be used. More review necessary.

chrta · 2020-08-06T08:43:18Z

Thank you, this is a good addition also for all SiLabs boards, since nearly every peripheral on the board must be enabled with a dedicated io pin.

My current thought is to add a power-supply optional property to the SPI and I2C device base binding definitions, in lieu of having to add those to every driver. To be discussed.

I would prefer if there would be a more general approach, but i don't know how to implement it properly, maybe move power-supply optional property to some base binding.

Use cases:

For (nearly all) SiLabs dev boards, there is a virtual serial port (via on-board Segger J-Link, named Board controller in source code) that must be enabled with a gpio pin, see https://github.com/zephyrproject-rtos/zephyr/blob/master/boards/arm/efm32gg_stk3701a/board.c#L20

Also the ethernet phy must be enabled via gpio pin a few lines further down, https://github.com/zephyrproject-rtos/zephyr/blob/master/boards/arm/efm32gg_stk3701a/board.c#L38

pabigot · 2020-08-06T13:02:30Z

Yes, I have the sltb004a. It motivates a slightly different approach: there are many cases, these included, where we want a more efficient solution that doesn't involve a full on-off service that supports delayed startup and shutdown. Just controlling a GPIO based on whether a counter is zero or positive handles it. I'm thinking of having another compatible regulator-fixed-direct(?) to handle that case in the same driver. (Alternatively that optimized variant could be selected for an instance based on other properties, like all delays being zero and the referenced pin being directly controlled by a SOC GPIO peripheral.) I'm going to take a look at that next.

If there's only one thing that will ever control the signal, and it already knows the state without a counter, I don't think a full regulator API is appropriate. For that application using power-gpios instead of power-supply might be better. Which is probably the case for the sltb004a peripherals.

It should be possible to eliminate boards/arm/efm32gg_stk3701a/board.h by putting that information into a board node in the devicetree, e.g. eth-power-enable-gpios, eth-ref-clk-gpios and eth-reset-gpios properties. Not sure how to handle the clock, but it should be possible. Whether it's worth doing is a maintainer decision.

include/drivers/regulator.h

sjg20 · 2020-08-04T16:17:00Z

include/drivers/regulator.h

+ *
+ * @param reg a regulator device
+ *
+ * @return non-negative on successful completion of the release


Does it wait for the regulator to actually be fully off?

No. This was a design trade-off in the context of developing the on-off service. If it's necessary to detect when the transition completes that can be done in other ways.

I suspect it will be necessary, so this may have to change.

In general you can only know that you have started to turn it off. Of course you can look at datasheets and some of them will have a 'ready' bit which tells you that the power is stable (or fully off).

IMO it is probably enough to know that we have completed 'asking' for it to turn off. A client could add a udelay if needed.

For turning on, I think we need to know that the power is fully stable, since until it is we cannot advance.

include/drivers/regulator.h

sjg20 · 2020-09-29T14:17:26Z

@pabigot Sorry I seem to have missed a step so that I could see them but not you. Can you see them now?

sjg20 · 2020-09-29T20:32:24Z

The latest push reveals another gap in dependency information: all regulators are included in the image even if only one (or no) device that depends on them is enabled.

Yes, that could be addressed by marking some or all of them status = "disabled"; but that's pretty painful from an application perspective.

That seems OK to me for now

sjg20 · 2020-09-29T20:40:48Z

drivers/regulator/regulator_fixed.c

+	return rc;
+}
+
+struct driver_data_sync {


struct comment?

I suppose we can start requiring that for internal implementation data structures that aren't processed to provide documentation. Do we want to do that?

Yes I think we still want comments

sjg20 · 2020-09-29T20:41:48Z

drivers/regulator/regulator_fixed.c

+	const struct driver_config *cfg = dev->config;
+	int rc = common_init(dev, &data->gpio);
+
+	(void)regulator_fixed_init_onoff;


What are these three lines for?

They eliminated "function defined but not referenced" in cases where only _sync regulators are present.

I may be able to remove them with preprocessor conditional that prevent them from being defined, but I think I tried that and it got ugly.

How about __maybe_unused ?

Possibly; no precedent in zephyr.

doc/reference/peripherals/regulators.rst

tests/drivers/regulator/fixed/README.txt

tests/drivers/regulator/fixed/src/main.c

boards/arm/thingy52_nrf52832/thingy52_nrf52832.dts

dts/bindings/regulator/regulator-fixed.yaml

galak · 2020-10-20T16:42:25Z

dts/bindings/regulator/regulator-fixed.yaml

+#    compatible = "regulator-fixed-sync", "regulator-fixed";
+#
+
+compatible: "regulator-fixed"


Are we trying to be in sync with the linux binding for regulator-fixed?

To a reasonable degree, yes. We don't support external properties for the active level and open drain, for reasons described in the binding.

galak · 2020-10-20T17:08:41Z

dts/bindings/regulator/regulator-fixed.yaml

+    required: true
+    description: Descriptive name for regulator outputs.
+
+  enable-gpios:


Looks like upstream just calls this gpio for this property.

Yes, that's true. upstream uses gpio, enable-active-high, and gpio-open-drain. This uses enable-gpios to handle all three of those.

I though we'd discussed this and you agreed we shouldn't attempt to support the extra properties when proper use of a GPIO selector makes them unnecessary. Given that, I think we should use a different name, because gpio would suggest the flags are ignored.

If we need to name this zephyr,regulator-fixed to avoid confusion I'm fine with that.

I think we can leave things as is, but can we add a comment in the binding to explain how/why enable-gpios differs from gpio?

@galak Rebased (since this hasn't been tested directly on master since August), and then updated the text.

I went with documentation text rather than comment, because if we're going to generate human-readable documentation for these things the comments are less visible than when we expect people to look at the binding file directly.

Provide a common set of properties for various ways of controlling power: * supply-gpios for a GPIO specifier acting like a switch * vin-supply for a reference to a regulator device Document the behavior expected when these properties are present. Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>

This PR follows Linux in defining devicetree content for generic voltage and current regulators, and an initial driver API for controlling them. A regulator itself may depend on a power source, so it needs to support the properties that enable that power source. Signed-off-by: Peter A. Bigot <pab@pabigot.com>

This provides structure for the regulator device hierarchy and a driver for GPIO-controlled regulators along with its binding. Signed-off-by: Peter A. Bigot <pab@pabigot.com>

Add a page for the regulator API and introduce it as an experimental API. Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>

Basic checks on core regulator functions. Signed-off-by: Peter A. Bigot <pab@pabigot.com>

This board has a multi-level power domain hierarchy where a sensor has its own power control that is accessed through an external GPIO peripheral that itself has a power control. Signed-off-by: Peter A. Bigot <pab@pabigot.com>

pabigot added area: Power Management area: API Changes to public APIs labels Aug 3, 2020

pabigot requested review from carlescufi and wentongwu August 3, 2020 19:06

pabigot requested review from mbolivar-nordic, galak and henrikbrixandersen August 3, 2020 19:07

github-actions bot added area: Boards area: Devicetree area: Devicetree Binding PR modifies or adds a Device Tree binding labels Aug 3, 2020

pabigot force-pushed the pabigot/20200803a branch from db783dc to 4763e45 Compare August 3, 2020 22:05

wentongwu requested a review from nashif August 4, 2020 02:12

wentongwu reviewed Aug 4, 2020

View reviewed changes

drivers/regulator/regulator_gpio.c Outdated Show resolved Hide resolved

wentongwu reviewed Aug 4, 2020

View reviewed changes

include/drivers/regulator.h Show resolved Hide resolved

pabigot force-pushed the pabigot/20200803a branch from d520b90 to 8b3da4f Compare August 4, 2020 15:41

carlescufi requested review from MaureenHelm, dcpleung and gmarull August 4, 2020 16:24

dcpleung reviewed Aug 4, 2020

View reviewed changes

include/drivers/regulator.h Outdated Show resolved Hide resolved

include/drivers/regulator.h Outdated Show resolved Hide resolved

drivers/regulator/regulator_gpio.c Outdated Show resolved Hide resolved

drivers/regulator/regulator_gpio.c Outdated Show resolved Hide resolved

pabigot force-pushed the pabigot/20200803a branch from 8b3da4f to 5bb9337 Compare August 4, 2020 19:57

pabigot mentioned this pull request Aug 4, 2020

device power management irq lock #24230

Open

pabigot force-pushed the pabigot/20200803a branch from 5bb9337 to 10187e1 Compare August 4, 2020 20:33

sjg20 reviewed Sep 29, 2020

View reviewed changes

pabigot force-pushed the pabigot/20200803a branch from c7aa4f9 to 23896bf Compare September 29, 2020 16:42

sjg20 approved these changes Sep 29, 2020

View reviewed changes

pabigot force-pushed the pabigot/20200803a branch from 23896bf to 85c7925 Compare September 30, 2020 15:47

carlescufi requested review from nordic-krch, dcpleung, wentongwu and gmarull October 15, 2020 14:36

dcpleung approved these changes Oct 15, 2020

View reviewed changes

wentongwu approved these changes Oct 16, 2020

View reviewed changes

galak reviewed Oct 20, 2020

View reviewed changes

dts/bindings/regulator/regulator-fixed.yaml Show resolved Hide resolved

galak reviewed Oct 20, 2020

View reviewed changes

pabigot added the dev-review To be discussed in dev-review meeting label Oct 22, 2020

galak removed the dev-review To be discussed in dev-review meeting label Oct 22, 2020

pabigot and others added 2 commits October 22, 2020 08:35

pabigot force-pushed the pabigot/20200803a branch from 85c7925 to 577031a Compare October 22, 2020 13:35

pabigot and others added 4 commits October 22, 2020 08:39

drivers: regulator: add GPIO-controlled regulator driver

a9dc970

This provides structure for the regulator device hierarchy and a driver for GPIO-controlled regulators along with its binding. Signed-off-by: Peter A. Bigot <pab@pabigot.com>

doc: introduce the regulator driver API

37e4203

Add a page for the regulator API and introduce it as an experimental API. Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>

tests: drivers: regulator: add test for fixed regulator

a07254f

Basic checks on core regulator functions. Signed-off-by: Peter A. Bigot <pab@pabigot.com>

pabigot force-pushed the pabigot/20200803a branch from 577031a to 1056280 Compare October 22, 2020 13:39

henrikbrixandersen removed their request for review October 27, 2020 19:25

carlescufi approved these changes Oct 28, 2020

View reviewed changes

carlescufi merged commit c2bac29 into zephyrproject-rtos:master Oct 28, 2020

Architecture Project automation moved this from In Progress to Done Oct 28, 2020

pabigot deleted the pabigot/20200803a branch October 31, 2020 11:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add regulator driver infrastructure #27360

Add regulator driver infrastructure #27360

pabigot commented Aug 3, 2020 •

edited

pabigot commented Aug 3, 2020

pabigot commented Aug 3, 2020

galak commented Aug 4, 2020

carlescufi commented Aug 4, 2020 •

edited

galak commented Aug 4, 2020

pabigot commented Aug 4, 2020 •

edited

chrta commented Aug 6, 2020

pabigot commented Aug 6, 2020

sjg20 Aug 4, 2020

pabigot Sep 29, 2020

sjg20 Sep 29, 2020

sjg20 commented Sep 29, 2020

sjg20 commented Sep 29, 2020

sjg20 Sep 29, 2020

pabigot Sep 30, 2020

sjg20 Oct 1, 2020

sjg20 Sep 29, 2020

pabigot Sep 30, 2020

sjg20 Oct 1, 2020

pabigot Oct 1, 2020

galak Oct 20, 2020

pabigot Oct 20, 2020

galak Oct 20, 2020

pabigot Oct 20, 2020 •

edited

pabigot Oct 20, 2020

galak Oct 22, 2020

pabigot Oct 22, 2020

pabigot Oct 22, 2020

Add regulator driver infrastructure #27360

Add regulator driver infrastructure #27360

Conversation

pabigot commented Aug 3, 2020 • edited

Commit Contents

Blockers for merge:

pabigot commented Aug 3, 2020

pabigot commented Aug 3, 2020

galak commented Aug 4, 2020

carlescufi commented Aug 4, 2020 • edited

galak commented Aug 4, 2020

pabigot commented Aug 4, 2020 • edited

chrta commented Aug 6, 2020

pabigot commented Aug 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjg20 commented Sep 29, 2020

sjg20 commented Sep 29, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pabigot Oct 20, 2020 • edited

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

pabigot commented Aug 3, 2020 •

edited

carlescufi commented Aug 4, 2020 •

edited

pabigot commented Aug 4, 2020 •

edited

pabigot Oct 20, 2020 •

edited