regulator: enable regulator support for i.MX23/i.MX28

This patch adds a minimal driver for the Freescale i.MX23, i.MX28 power subsystem. It's required to trigger the probing of the underlying drivers like on-chip regulators.

Hi,

Quoted 3 lines · Stefan Wahren

This patch adds a minimal driver for the Freescale i.MX23, i.MX28 power subsystem. It's required to trigger the probing of the underlying drivers like on-chip regulators.

I'm not sure what you need the power supply class for. I see only one property (ac connected), which is constant. Maybe just drop the power supply stuff?

This would leave an almost empty driver, which just sets a single register at probe time and probes more drivers. That would probably be more fitting for the MFD subsystem.

-- Sebastian

Am 23.05.2015 um 19:41 schrieb Sebastian Reichel:

Quoted 7 lines · Stefan Wahren

Quoted 3 lines

This patch adds a minimal driver for the Freescale i.MX23, i.MX28 power subsystem. It's required to trigger the probing of the underlying drivers like on-chip regulators.

I'm not sure what you need the power supply class for. I see only one property (ac connected), which is constant. Maybe just drop the power supply stuff?

those SoCs could be powered by 3 different sources:
- 5V wall
- USB
- battery

I choose the first one as a base, because it's the simplest one. Unfortunately i don't have the time to implement all of them.

From what I can see the driver does not implement the 5V wall

The Freescale downstream kernel (based on 2.6.35) [1] has a driver to handle all MXS power sources. But i think a clean implementation from the scratch would be necessary for mainline.

Quoted 3 lines

This would leave an almost empty driver, which just sets a single register at probe time and probes more drivers. That would probably be more fitting for the MFD subsystem.

I don't have any problems with fitting the driver to MFD.

Quoted 1 line

-- Sebastian

[1] -
http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/tree/drivers/power/mxs/linux.c?h=imx_2.6.35_maintain

Hi Stefan,

Quoted 13 lines · Stefan Wahren

Am 23.05.2015 um 19:41 schrieb Sebastian Reichel:

Quoted 7 lines · Stefan Wahren

Quoted 3 lines

This patch adds a minimal driver for the Freescale i.MX23, i.MX28 power subsystem. It's required to trigger the probing of the underlying drivers like on-chip regulators.

I'm not sure what you need the power supply class for. I see only one property (ac connected), which is constant. Maybe just drop the power supply stuff?

those SoCs could be powered by 3 different sources:
- 5V wall
- USB
- battery

Quoted 2 lines

I choose the first one as a base, because it's the simplest one. Unfortunately i don't have the time to implement all of them.

From what I can see the driver does not implement the 5V wall charger handling. It simply assumes, that the 5V wall charger is there.

From my understanding it's not possible to determine where the 5V

While you can skip implementation of battery connection check you should make sure, that AC is actually connected.

the driver consists of multiple files in the mxs sub directory. I linked to the main file to reduce search effort. The battery handling incl. charging should be implemented in ddi_power_battery.c

From my understanding it's not possible to determine where the 5V exactly come from (either 5V wall plug or USB). The driver simply uses the USB PHY state.

Quoted 2 lines

While you can skip implementation of battery connection check you should make sure, that AC is actually connected.

Do you refer to the right state of property
POWER_SUPPLY_PROP_ONLINE or driver probing?

J?rgen Borleis <[email protected]> hat am 4. Mai 2015 um 15:51 geschrieben:

Hi Stefan,

for the i.MX23 the regulator daisy chain looks a little bit different:

+------------+     VDD5V  +---------+  VDDIO
| DC-DC      |   +--------> LINREG  +--------->+
|            |            | VDDIO   |          |
|            |            +---------+          |
| DCDC_VDDIO +->-+-----------------------------+
|            |   |
|            |   | VDDIO  +---------+ VDDMEM
|            |   +--------> LINREG  +--------->
|            |   |        | VDDMEM  |
|            |   |        +---------+
|            |   | VDDIO  +---------+  VDDA
|            |   +--------> LINREG  +--------->+
|            |            | VDDA    |          |
|            |            +---------+          |
| DCDC_VDDA  +->-+-----------------------------+
|            |   | VDDA   +---------+  VDDD
|            |   +--------> LINREG  +--------->+
|            |            | VDDD    |          |
|            |            +---------+          |
| DCDC_VDDD  +->-------------------------------+
+------------+                                 |

v

From my understanding VDDMEM and VDD1P5 describe the same regulator function

Regards,
Juergen

[...]

Quoted 24 lines

+------------+     VDD5V  +---------+  VDDIO
| DC-DC      |   +--------> LINREG  +--------->+
|            |            | VDDIO   |          |
|            |            +---------+          |
|            |                                 |
| DCDC_VDDIO +->-+-----------------------------+
|            |   |
|            |   | VDDIO  +---------+ VDDMEM
|            |   +--------> LINREG  +--------->
|            |   |        | VDDMEM  |
|            |   |        +---------+
|            |   | VDDIO  +---------+  VDDA
|            |   +--------> LINREG  +--------->+
|            |            | VDDA    |          |
|            |            +---------+          |
| DCDC_VDDA  +->-+-----------------------------+
|            |   |
|            |   | VDDA   +---------+  VDDD
|            |   +--------> LINREG  +--------->+
|            |            | VDDD    |          |
|            |            +---------+          |
| DCDC_VDDD  +->-------------------------------+
+------------+                                 |

Quoted 1 line

v

damn! I didn't noticed that VDDMEM is powered by VDDIO and VDD1P5 is powered by VDDA. Thank you very much.

From my understanding VDDMEM and VDD1P5 describe the same regulator function (provides power to external memory such as SDRAM and mDDR which doesn't fit to VDDD, VDDIO or VDDA). But the usage of
the name is not consistent across the documentation:

                  | i.MX23 | i.MX28
------------------------------------
Application note  | VDDMEM | VDD1P5
Reference manual  | VDDMEM | VDDMEM
Data sheet        | -      | VDD1P5

I would suggest to use VDDMEM, because the registers are always called HW_VDDMEM (for both SoCs).

I think we have 3 options here:

a) add the regulator daisy chain for the i.MX23 too
b) remove VDD1P5 from the i.MX28 diagram so it would fit for both again
c) remove the diagram complete

I would prefer option b. What's your opinion?

J?rgen Borleis <[email protected]> hat am 5. Mai 2015 um 10:21 geschrieben:

Hi Stefan,

Quoted 4 lines · On Monday 04 May 2015 22:20:50 Stefan Wahren wrote:

[...]

I think we have 3 options here:

Quoted 5 lines

a) add the regulator daisy chain for the i.MX23 too
b) remove VDD1P5 from the i.MX28 diagram so it would fit for both again
c) remove the diagram complete

I would prefer option b. What's your opinion?

I prefer a) because once the details are known, we should keep them.

Regards,
Juergen

Mark Brown <[email protected]> hat am 4. Mai 2015 um 14:36 geschrieben:

drivers/regulator/mxs-regulator.c line 346

Quoted 3 lines · Stefan Wahren

drivers/regulator/mxs-regulator.c new file +3 -0

+static struct regulator_ops mxs_dcdc_ops = {
+ .is_enabled = regulator_is_enabled_regmap,
+};

Why do we have an is_enabled() operation but no enable or disable operation?

I'm also not 100% clear what code the DCDCs and LDOs are sharing here...

Stefan Wahren <[email protected]> hat am 4. Mai 2015 um 22:34 geschrieben:

Hi Mark,

Quoted 11 lines

Mark Brown <[email protected]> hat am 4. Mai 2015 um 14:36 geschrieben:

drivers/regulator/mxs-regulator.c line 346

Quoted 3 lines · Stefan Wahren

drivers/regulator/mxs-regulator.c new file +3 -0

+static struct regulator_ops mxs_dcdc_ops = {
+ .is_enabled = regulator_is_enabled_regmap,
+};

Why do we have an is_enabled() operation but no enable or disable operation?

i'm sorry, i forgot the following point in the changelog:

- move dcdc handling from mxs_power to mxs-regulator

I hope the change is okay. I implemented only is_enabled() to have at least 1 operation.
But it's not really necessary.

Quoted 3 lines

I'm also not 100% clear what code the DCDCs and LDOs are sharing here...

Actually only probing and the regulator info structure.

Best regards
Stefan

Quoted 5 lines

Stefan Wahren <[email protected]> hat am 4. Mai 2015 um 22:34

i'm sorry, i forgot the following point in the changelog:

- move dcdc handling from mxs_power to mxs-regulator

Quoted 3 lines

I hope the change is okay. I implemented only is_enabled() to have at least 1 operation.
But it's not really necessary.

do we need enable / disable operation?

In the bootloader code enabling the DC-DC is critical. There is a known issue which falsely triggers the brownout detection.

Quoted 4 lines

Quoted 2 lines

I'm also not 100% clear what code the DCDCs and LDOs are sharing here...

Actually only probing and the regulator info structure.

How can i improve that?

Mark Brown <[email protected]> hat am 7. Mai 2015 um 20:52 geschrieben:

Quoted 10 lines · Stefan Wahren

Quoted 10 lines

Stefan Wahren <[email protected]> hat am 4. Mai 2015 um 22:34

i'm sorry, i forgot the following point in the changelog:

- move dcdc handling from mxs_power to mxs-regulator

I hope the change is okay. I implemented only is_enabled() to have at least 1
operation.
But it's not really necessary.

Quoted 5 lines

do we need enable / disable operation?

In the bootloader code enabling the DC-DC is critical. There is a known issue
which falsely triggers the brownout detection.

Why would the driver not provide these operations?

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

I'm also not 100% clear what code the DCDCs and LDOs are sharing here...

Actually only probing and the regulator info structure.

How can i improve that?

If the devices aren't sharing code then they should be separate drivers.

Quoted 5 lines

Mark Brown <[email protected]> hat am 7. Mai 2015 um 20:52 geschrieben:

Quoted 3 lines

In the bootloader code enabling the DC-DC is critical. There is a known issue
which falsely triggers the brownout detection.

Quoted 1 line

Why would the driver not provide these operations?

I didn't implement this operation because the suggested workaround for the hardware issue isn't very nice
(disable brownout detection during enabling DC-DC). I was afraid of the implementation.

Quoted 1 line

If the devices aren't sharing code then they should be separate drivers.

Okay.

mxs-regulator-ldo.c
mxs-regulator-dcdc.c

What do you think of?

Mark Brown <[email protected]> hat am 7. Mai 2015 um 21:32 geschrieben:

Quoted 5 lines · Stefan Wahren

Quoted 5 lines

Mark Brown <[email protected]> hat am 7. Mai 2015 um 20:52 geschrieben:

Quoted 3 lines

In the bootloader code enabling the DC-DC is critical. There is a known issue
which falsely triggers the brownout detection.

Quoted 6 lines

Quoted 1 line

Why would the driver not provide these operations?

I didn't implement this operation because the suggested workaround for the hardware issue isn't very nice
(disable brownout detection during enabling DC-DC). I was afraid of the implementation.

That doesn't sound too complicated?

Quoted 6 lines

Mark Brown <[email protected]> hat am 7. Mai 2015 um 21:32 geschrieben:

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

In the bootloader code enabling the DC-DC is critical. There is a known issue
which falsely triggers the brownout detection.

Quoted 8 lines

Quoted 6 lines

Quoted 1 line

Why would the driver not provide these operations?

I didn't implement this operation because the suggested workaround for the hardware issue isn't very nice
(disable brownout detection during enabling DC-DC). I was afraid of the implementation.

That doesn't sound too complicated?

Okay, i think i managed the brownout handling.

Unfortunately i'm stuck at the next step. The DC-DC (3 way output) can supply 3 LDOs vddd, vdda, vddio. In case of a DC-DC state change the 3 regulators needs to be reconfigured before.

I tought of REGULATOR_EVENT_PRE_DISABLE and REGULATOR_EVENT_PRE_ENABLE events, but i'm not sure that's the right way to achieve a synchronization between DC-DC and consuming LDOs.

Mark Brown <[email protected]> hat am 27. Mai 2015 um 19:33 geschrieben:

Quoted 25 lines · Stefan Wahren

Quoted 9 lines

Mark Brown <[email protected]> hat am 7. Mai 2015 um 21:32 geschrieben:

Quoted 6 lines

Quoted 6 lines

Quoted 4 lines

In the bootloader code enabling the DC-DC is critical. There is a known
issue
which falsely triggers the brownout detection.

Why would the driver not provide these operations?

Quoted 7 lines

Quoted 5 lines

I didn't implement this operation because the suggested workaround for the
hardware issue isn't very nice
(disable brownout detection during enabling DC-DC). I was afraid of the implementation.

That doesn't sound too complicated?

Okay, i think i managed the brownout handling.

Unfortunately i'm stuck at the next step. The DC-DC (3 way output) can supply 3
LDOs vddd, vdda, vddio. In case of a DC-DC state change the 3 regulators needs
to be reconfigured before.

Quoted 5 lines

I tought of REGULATOR_EVENT_PRE_DISABLE and REGULATOR_EVENT_PRE_ENABLE events,
but i'm not sure that's the right way to achieve a synchronization between DC-DC
and consuming LDOs.

OK, that's possibly sounding a bit complicated. Is it just a case of writing some value, doing a reconfiguration and rewriting the original value or is it something more?

If it's much more complicated than that
then I think putting a comment in the driver explaining what the issue is would be the right step, that would address the issue with the driver looking strange.

In case of a single driver instance instead of 4 ones (vddd, vddio, vdda, dc-dc), things would be much easier.

Here are the steps for disabling DC-DC:

1. configure vddio to work without DC-DC
2. configure vdda to work without DC-DC
3. configure vddd to work without DC-DC
4. backup DC-DC settings
5. disable brownout detection
6. disable DC-DC
7. restore DC-DC settings

My problem is that step 1 to 3 must be done step by step through the other regulator instances.

Quoted 4 lines

If it's much more complicated than that
then I think putting a comment in the driver explaining what the issue is would be the right step, that would address the issue with the driver looking strange.

Enabling or disabling DC-DC is only needed internally by the power subsystem (e.g. in case of handoff AC to battery), because the DC-DC only supplys the regulator daisy. From my understanding hardware components could use only the regulator outputs. In the end a state change is necessary, but it shouldn't be a public.

This patch series adds on-chip regulator support to Freescale i.MX23 and

It's the result of the last discussion about cpufreq-dt support [1]. This

doesn't contain cpufreq-dt support anymore because it violated a constraint between VDDD voltage and EMI clock frequency (Thanks to Juergen Borleis for pointing out).

The cpufreq-dt support would be a further patch series.