ARM: dt: tegra: seaboard: add regulators
Subsystems (2)
PATCH 1/3 ARM: dt: tegra: seaboard: add regulators
From: Stephen Warren <[email protected]>
Seaboard uses a TPS6586x regulator. Instantiate this, and hook up a couple of fixed GPIO-controlled regulators too.
The regulator configurations were mostly taken from the ChromeOS 3.2 kernel. Exceptions are:
* The schematic lists a fixed voltage for each rail, whereas the ChromeOS kernel lists a range for many rails. I used the values from the ChromeOS kernel in all cases, since I know the board file there is the most complete available for this hardware.
* The vdd_1v2 fixed regulator is present only in the schematic. So, I added this based on the schematic.
* A 3.3v fixed regulator using GPIO3 of the TPS6586x is present in the ChromeOS kernel, but not in the schematic. So, I dropped this based on the schematic.
--- arch/arm/boot/dts/tegra20-seaboard.dts | 164 ++++++++++++++++++++++++++++++++ 1 files changed, 164 insertions(+), 0 deletions(-)
arch/arm/boot/dts/tegra20-seaboard.dts +164 -0
@@ -374,6 +374,141 @@ status = "okay"; clock-frequency = <400000>; + pmic: tps6586x at 34 { + compatible = "ti,tps6586x"; + reg = <0x34>; + interrupts = <0 86 0x4>; + + #gpio-cells = <2>; + gpio-controller; + + regulators { + #address-cells = <1>; + #size-cells = <0>; + + regulator at 0 { + reg = <0>; + regulator-compatible = "sm0"; + regulator-name = "vdd_sm0"; + regulator-min-microvolt = < 950000>; + regulator-max-microvolt = <1300000>; + regulator-always-on; + }; + + regulator at 1 { + reg = <1>; + regulator-compatible = "sm1"; + regulator-name = "vdd_sm1"; + regulator-min-microvolt = < 750000>; + regulator-max-microvolt = <1275000>; + regulator-always-on; + }; + + sm2_reg: regulator at 2 { + reg = <2>; + regulator-compatible = "sm2"; + regulator-name = "vdd_sm2"; + regulator-min-microvolt = <3000000>; + regulator-max-microvolt = <4550000>; + regulator-always-on; + }; + + regulator at 3 { + reg = <3>; + regulator-compatible = "ldo0"; + regulator-name = "vdd_ldo0"; + regulator-min-microvolt = <1250000>; + regulator-max-microvolt = <3300000>; + vin-supply = <&sm2_reg>; + }; + + regulator at 4 { + reg = <4>; + regulator-compatible = "ldo1"; + regulator-name = "vdd_ldo1"; + regulator-min-microvolt = <1100000>; + regulator-max-microvolt = <1100000>; + regulator-always-on; + vin-supply = <&sm2_reg>; + }; + + regulator at 5 { + reg = <5>; + regulator-compatible = "ldo2"; + regulator-name = "vdd_ldo2"; + regulator-min-microvolt = < 900000>; + regulator-max-microvolt = <1300000>; + vin-supply = <&sm2_reg>; + }; + + regulator at 6 { + reg = <6>; + regulator-compatible = "ldo3"; + regulator-name = "vdd_ldo3"; + regulator-min-microvolt = <3300000>; + regulator-max-microvolt = <3300000>; + regulator-always-on; + vin-supply = <&sm2_reg>; + }; + + regulator at 7 { + reg = <7>; + regulator-compatible = "ldo4"; + regulator-name = "vdd_ldo4"; + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <1800000>; + regulator-always-on; + vin-supply = <&sm2_reg>; + }; + + regulator at 8 { + reg = <8>; + regulator-compatible = "ldo5"; + regulator-name = "vdd_ldo5"; + regulator-min-microvolt = <2850000>; + regulator-max-microvolt = <3300000>; + regulator-always-on; + }; + + regulator at 9 { + reg = <9>; + regulator-compatible = "ldo6"; + regulator-name = "vdd_ldo6"; + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <1800000>; + vin-supply = <&sm2_reg>; + }; + + regulator at 10 { + reg = <10>; + regulator-compatible = "ldo7"; + regulator-name = "vdd_ldo7"; + regulator-min-microvolt = <3300000>; + regulator-max-microvolt = <3300000>; + vin-supply = <&sm2_reg>; + }; + + regulator at 11 { + reg = <11>; + regulator-compatible = "ldo8"; + regulator-name = "vdd_ldo8"; + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <1800000>; + vin-supply = <&sm2_reg>; + }; + + regulator at 12 { + reg = <12>; + regulator-compatible = "ldo9"; + regulator-name = "vdd_ldo9"; + regulator-min-microvolt = <2850000>; + regulator-max-microvolt = <2850000>; + regulator-always-on; + vin-supply = <&sm2_reg>; + }; + }; + }; + temperature-sensor at 4c { compatible = "nct1008"; reg = <0x4c>;
@@ -387,6 +522,10 @@ }; }; + pmc { + nvidia,invert-interrupt; + }; + memory-controller at 0x7000f400 { emc-table at 190000 { reg = <190000>;
@@ -473,6 +612,31 @@ }; }; + regulators { + compatible = "simple-bus"; + #address-cells = <1>; + #size-cells = <0>; + + regulator at 0 { + compatible = "regulator-fixed"; + reg = <0>; + regulator-name = "vdd_1v5"; + regulator-min-microvolt = <1500000>; + regulator-max-microvolt = <1500000>; + gpio = <&pmic 0 0>; + }; + + regulator at 1 { + compatible = "regulator-fixed"; + reg = <1>; + regulator-name = "vdd_1v2"; + regulator-min-microvolt = <1200000>; + regulator-max-microvolt = <1200000>; + gpio = <&pmic 1 0>; + enable-active-high; + }; + }; + sound { compatible = "nvidia,tegra-audio-wm8903-seaboard", "nvidia,tegra-audio-wm8903";
Signature · 1 line
1.7.0.4
PATCH 2/3 ARM: dt: tegra: ventana: add regulators
From: Stephen Warren <[email protected]>
Ventana uses a TPS6586x regulator. Instantiate this, and hook up a couple of fixed GPIO-controlled regulators too.
The regulator configurations were all taken from NVIDIA's downstream android-tegra-nv-3.1 kernel.
--- arch/arm/boot/dts/tegra20-ventana.dts | 173 +++++++++++++++++++++++++++++++++ 1 files changed, 173 insertions(+), 0 deletions(-)
arch/arm/boot/dts/tegra20-ventana.dts +173 -0
@@ -289,6 +289,144 @@ i2c at 7000d000 { status = "okay"; clock-frequency = <400000>; + + pmic: tps6586x at 34 { + compatible = "ti,tps6586x"; + reg = <0x34>; + interrupts = <0 86 0x4>; + + #gpio-cells = <2>; + gpio-controller; + + regulators { + #address-cells = <1>; + #size-cells = <0>; + + regulator at 0 { + reg = <0>; + regulator-compatible = "sm0"; + regulator-name = "vdd_sm0"; + regulator-min-microvolt = < 725000>; + regulator-max-microvolt = <1500000>; + regulator-always-on; + }; + + regulator at 1 { + reg = <1>; + regulator-compatible = "sm1"; + regulator-name = "vdd_sm1"; + regulator-min-microvolt = < 725000>; + regulator-max-microvolt = <1500000>; + regulator-always-on; + }; + + sm2_reg: regulator at 2 { + reg = <2>; + regulator-compatible = "sm2"; + regulator-name = "vdd_sm2"; + regulator-min-microvolt = <3000000>; + regulator-max-microvolt = <4550000>; + regulator-always-on; + }; + + regulator at 3 { + reg = <3>; + regulator-compatible = "ldo0"; + regulator-name = "vdd_ldo0"; + regulator-min-microvolt = <1250000>; + regulator-max-microvolt = <3300000>; + vin-supply = <&sm2_reg>; + }; + + regulator at 4 { + reg = <4>; + regulator-compatible = "ldo1"; + regulator-name = "vdd_ldo1"; + regulator-min-microvolt = < 725000>; + regulator-max-microvolt = <1500000>; + regulator-always-on; + vin-supply = <&sm2_reg>; + }; + + regulator at 5 { + reg = <5>; + regulator-compatible = "ldo2"; + regulator-name = "vdd_ldo2"; + regulator-min-microvolt = < 725000>; + regulator-max-microvolt = <1500000>; + vin-supply = <&sm2_reg>; + }; + + regulator at 6 { + reg = <6>; + regulator-compatible = "ldo3"; + regulator-name = "vdd_ldo3"; + regulator-min-microvolt = <1250000>; + regulator-max-microvolt = <3300000>; + vin-supply = <&sm2_reg>; + }; + + regulator at 7 { + reg = <7>; + regulator-compatible = "ldo4"; + regulator-name = "vdd_ldo4"; + regulator-min-microvolt = <1700000>; + regulator-max-microvolt = <2475000>; + regulator-always-on; + vin-supply = <&sm2_reg>; + }; + + regulator at 8 { + reg = <8>; + regulator-compatible = "ldo5"; + regulator-name = "vdd_ldo5"; + regulator-min-microvolt = <1250000>; + regulator-max-microvolt = <3300000>; + regulator-always-on; + }; + + regulator at 9 { + reg = <9>; + regulator-compatible = "ldo6"; + regulator-name = "vdd_ldo6"; + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <1800000>; + vin-supply = <&sm2_reg>; + }; + + regulator at 10 { + reg = <10>; + regulator-compatible = "ldo7"; + regulator-name = "vdd_ldo7"; + regulator-min-microvolt = <1250000>; + regulator-max-microvolt = <3300000>; + vin-supply = <&sm2_reg>; + }; + + regulator at 11 { + reg = <11>; + regulator-compatible = "ldo8"; + regulator-name = "vdd_ldo8"; + regulator-min-microvolt = <1250000>; + regulator-max-microvolt = <3300000>; + vin-supply = <&sm2_reg>; + }; + + regulator at 12 { + reg = <12>; + regulator-compatible = "ldo9"; + regulator-name = "vdd_ldo9"; + regulator-min-microvolt = <1250000>; + regulator-max-microvolt = <3300000>; + regulator-always-on; + vin-supply = <&sm2_reg>; + }; + }; + }; + }; + + pmc { + nvidia,invert-interrupt; }; usb at c5000000 {
@@ -317,6 +455,41 @@ bus-width = <8>; }; + regulators { + compatible = "simple-bus"; + #address-cells = <1>; + #size-cells = <0>; + + regulator at 0 { + compatible = "regulator-fixed"; + reg = <0>; + regulator-name = "vdd_1v5"; + regulator-min-microvolt = <1500000>; + regulator-max-microvolt = <1500000>; + gpio = <&pmic 0 0>; + }; + + regulator at 1 { + compatible = "regulator-fixed"; + reg = <1>; + regulator-name = "vdd_1v2"; + regulator-min-microvolt = <1200000>; + regulator-max-microvolt = <1200000>; + gpio = <&pmic 1 0>; + enable-active-high; + }; + + regulator at 2 { + compatible = "regulator-fixed"; + reg = <2>; + regulator-name = "pnl_pwr"; + regulator-min-microvolt = <2800000>; + regulator-max-microvolt = <2800000>; + gpio = <&gpio 22 0>; /* gpio PC6 */ + enable-active-high; + }; + }; + sound { compatible = "nvidia,tegra-audio-wm8903-ventana", "nvidia,tegra-audio-wm8903";
Signature · 1 line
1.7.0.4
PATCH 3/3 ARM: dt: tegra: paz00: add regulators
In reply to Stephen WarrenFrom: Stephen Warren <[email protected]>
The Toshiba AC100/PAZ00 uses a TPS6586x regulator. Instantiate this.
The regulator configurations were all taken from the AC100 kernel used by the Ubuntu port, specifically:
git://gitorious.org/~marvin24/ac100/marvin24s-kernel.git chromeos-ac100-3.0-exp
--- arch/arm/boot/dts/tegra20-paz00.dts | 138 +++++++++++++++++++++++++++++++++++ 1 files changed, 138 insertions(+), 0 deletions(-)
arch/arm/boot/dts/tegra20-paz00.dts +138 -0
@@ -272,12 +272,150 @@ status = "okay"; clock-frequency = <400000>; + pmic: tps6586x at 34 { + compatible = "ti,tps6586x"; + reg = <0x34>; + interrupts = <0 86 0x4>; + + #gpio-cells = <2>; + gpio-controller; + + regulators { + #address-cells = <1>; + #size-cells = <0>; + + regulator at 0 { + reg = <0>; + regulator-compatible = "sm0"; + regulator-name = "+1.2vs_sm0"; + regulator-min-microvolt = < 725000>; + regulator-max-microvolt = <1300000>; + regulator-always-on; + }; + + regulator at 1 { + reg = <1>; + regulator-compatible = "sm1"; + regulator-name = "+1.0vs_sm1"; + regulator-min-microvolt = < 725000>; + regulator-max-microvolt = <1125000>; + regulator-always-on; + }; + + sm2_reg: regulator at 2 { + reg = <2>; + regulator-compatible = "sm2"; + regulator-name = "+3.7vs_sm2"; + regulator-min-microvolt = <3000000>; + regulator-max-microvolt = <3700000>; + regulator-always-on; + }; + + regulator at 3 { + reg = <3>; + regulator-compatible = "ldo0"; + regulator-name = "+3.3vs_ldo0"; + regulator-min-microvolt = <1250000>; + regulator-max-microvolt = <3300000>; + vin-supply = <&sm2_reg>; + }; + + regulator at 4 { + reg = <4>; + regulator-compatible = "ldo1"; + regulator-name = "+1.1vs_ldo1"; + regulator-min-microvolt = < 725000>; + regulator-max-microvolt = <1100000>; + regulator-always-on; + vin-supply = <&sm2_reg>; + }; + + regulator at 5 { + reg = <5>; + regulator-compatible = "ldo2"; + regulator-name = "+1.2vs_ldo2"; + regulator-min-microvolt = < 725000>; + regulator-max-microvolt = <1275000>; + vin-supply = <&sm2_reg>; + }; + + regulator at 6 { + reg = <6>; + regulator-compatible = "ldo3"; + regulator-name = "+3.3vs_ldo3"; + regulator-min-microvolt = <1250000>; + regulator-max-microvolt = <3300000>; + vin-supply = <&sm2_reg>; + }; + + regulator at 7 { + reg = <7>; + regulator-compatible = "ldo4"; + regulator-name = "+1.8vs_ldo4"; + regulator-min-microvolt = <1700000>; + regulator-max-microvolt = <1800000>; + regulator-always-on; + vin-supply = <&sm2_reg>; + }; + + regulator at 8 { + reg = <8>; + regulator-compatible = "ldo5"; + regulator-name = "+2.85vs_ldo5"; + regulator-min-microvolt = <1250000>; + regulator-max-microvolt = <2850000>; + regulator-always-on; + }; + + regulator at 9 { + reg = <9>; + regulator-compatible = "ldo6"; + regulator-name = "+2.85vs_ldo6"; + regulator-min-microvolt = <1250000>; + regulator-max-microvolt = <2850000>; + vin-supply = <&sm2_reg>; + }; + + regulator at 10 { + reg = <10>; + regulator-compatible = "ldo7"; + regulator-name = "+3.3vs_ldo7"; + regulator-min-microvolt = <1250000>; + regulator-max-microvolt = <3300000>; + vin-supply = <&sm2_reg>; + }; + + regulator at 11 { + reg = <11>; + regulator-compatible = "ldo8"; + regulator-name = "+1.8vs_ldo8"; + regulator-min-microvolt = <1250000>; + regulator-max-microvolt = <1800000>; + vin-supply = <&sm2_reg>; + }; + + regulator at 12 { + reg = <12>; + regulator-compatible = "ldo9"; + regulator-name = "+2.85vs_ldo9"; + regulator-min-microvolt = <1250000>; + regulator-max-microvolt = <2850000>; + regulator-always-on; + vin-supply = <&sm2_reg>; + }; + }; + }; + adt7461 at 4c { compatible = "adi,adt7461"; reg = <0x4c>; }; }; + pmc { + nvidia,invert-interrupt; + }; + usb at c5000000 { status = "okay"; };
Signature · 1 line
1.7.0.4
PATCH 3/3 ARM: dt: tegra: paz00: add regulators
Hi Stephen,
Quoted 59 lines · On Friday 22 June 2012 17:14:02 Stephen Warren wrote:
From: Stephen Warren <[email protected]>
The Toshiba AC100/PAZ00 uses a TPS6586x regulator. Instantiate this.
The regulator configurations were all taken from the AC100 kernel used by the Ubuntu port, specifically:
git://gitorious.org/~marvin24/ac100/marvin24s-kernel.git
chromeos-ac100-3.0-expCc Marc Dietrich <[email protected]>Signed-off-by Stephen Warren <[email protected]>--- arch/arm/boot/dts/tegra20-paz00.dts | 138+++++++++++++++++++++++++++++++++++ 1 files changed, 138 insertions(+), 0 deletions(-)
(quoted diff) +0
-0
b/arch/arm/boot/dts/tegra20-paz00.dtsindex 684a9e1..55ca34c 100644arch/arm/boot/dts/tegra20-paz00.dts +34
-0@@ -272,12 +272,150 @@ status = "okay"; clock-frequency = <400000>; + pmic: tps6586x at 34 { + compatible = "ti,tps6586x"; + reg = <0x34>; + interrupts = <0 86 0x4>; + + #gpio-cells = <2>; + gpio-controller; + + regulators { + #address-cells = <1>; + #size-cells = <0>; + + regulator at 0 { + reg = <0>; + regulator-compatible = "sm0"; + regulator-name = "+1.2vs_sm0"; + regulator-min-microvolt = < 725000>; + regulator-max-microvolt = <1300000>; + regulator-always-on; + }; + + regulator at 1 { + reg = <1>; + regulator-compatible = "sm1"; + regulator-name = "+1.0vs_sm1"; + regulator-min-microvolt = < 725000>; + regulator-max-microvolt = <1125000>; + regulator-always-on; + }; + + sm2_reg: regulator at 2 { + reg = <2>; + regulator-compatible = "sm2"; + regulator-name = "+3.7vs_sm2";
arch/arm/boot/dts/tegra20-paz00.dts line 318
Quoted 10 lines
arch/arm/boot/dts/tegra20-paz00.dts +10
-0+ regulator-min-microvolt = <3000000>; + regulator-max-microvolt = <3700000>; + regulator-always-on; + }; + + regulator at 3 { + reg = <3>; + regulator-compatible = "ldo0"; + regulator-name = "+3.3vs_ldo0"; + regulator-min-microvolt = <1250000>;
I think the common sense was that this should also be 3.3 V as it is the pex clock (which is not used at all on this board). I guess it doesn't matter much. So ...
Quoted 103 lines
arch/arm/boot/dts/tegra20-paz00.dts +94
-0+ regulator-max-microvolt = <3300000>; + vin-supply = <&sm2_reg>; + }; + + regulator at 4 { + reg = <4>; + regulator-compatible = "ldo1"; + regulator-name = "+1.1vs_ldo1"; + regulator-min-microvolt = < 725000>; + regulator-max-microvolt = <1100000>; + regulator-always-on; + vin-supply = <&sm2_reg>; + }; + + regulator at 5 { + reg = <5>; + regulator-compatible = "ldo2"; + regulator-name = "+1.2vs_ldo2"; + regulator-min-microvolt = < 725000>; + regulator-max-microvolt = <1275000>; + vin-supply = <&sm2_reg>; + }; + + regulator at 6 { + reg = <6>; + regulator-compatible = "ldo3"; + regulator-name = "+3.3vs_ldo3"; + regulator-min-microvolt = <1250000>; + regulator-max-microvolt = <3300000>; + vin-supply = <&sm2_reg>; + }; + + regulator at 7 { + reg = <7>; + regulator-compatible = "ldo4"; + regulator-name = "+1.8vs_ldo4"; + regulator-min-microvolt = <1700000>; + regulator-max-microvolt = <1800000>; + regulator-always-on; + vin-supply = <&sm2_reg>; + }; + + regulator at 8 { + reg = <8>; + regulator-compatible = "ldo5"; + regulator-name = "+2.85vs_ldo5"; + regulator-min-microvolt = <1250000>; + regulator-max-microvolt = <2850000>; + regulator-always-on; + }; + + regulator at 9 { + reg = <9>; + regulator-compatible = "ldo6"; + regulator-name = "+2.85vs_ldo6"; + regulator-min-microvolt = <1250000>; + regulator-max-microvolt = <2850000>; + vin-supply = <&sm2_reg>; + }; + + regulator at 10 { + reg = <10>; + regulator-compatible = "ldo7"; + regulator-name = "+3.3vs_ldo7"; + regulator-min-microvolt = <1250000>; + regulator-max-microvolt = <3300000>; + vin-supply = <&sm2_reg>; + }; + + regulator at 11 { + reg = <11>; + regulator-compatible = "ldo8"; + regulator-name = "+1.8vs_ldo8"; + regulator-min-microvolt = <1250000>; + regulator-max-microvolt = <1800000>; + vin-supply = <&sm2_reg>; + }; + + regulator at 12 { + reg = <12>; + regulator-compatible = "ldo9"; + regulator-name = "+2.85vs_ldo9"; + regulator-min-microvolt = <1250000>; + regulator-max-microvolt = <2850000>; + regulator-always-on; + vin-supply = <&sm2_reg>; + }; + }; + }; + adt7461 at 4c { compatible = "adi,adt7461"; reg = <0x4c>; }; }; + pmc { + nvidia,invert-interrupt; + }; + usb at c5000000 { status = "okay"; };
PATCH 3/3 ARM: dt: tegra: paz00: add regulators
Quoted 2 lines · Marc Dietrich
Quoted 2 lines
The regulator configurations were all taken from the AC100 kernel used by the Ubuntu port, specifically:
These generally all look pretty broken...
Quoted 6 lines
arch/arm/boot/dts/tegra20-paz00.dts line 292
Quoted 6 lines
arch/arm/boot/dts/tegra20-paz00.dts +6
-0+ regulator at 0 { + reg = <0>; + regulator-compatible = "sm0"; + regulator-name = "+1.2vs_sm0"; + regulator-min-microvolt = < 725000>; + regulator-max-microvolt = <1300000>;
Most of these ranges look suspiciously like the maximum possible variation the regulator has, not what the board actually requires (which is a depressingly common error, I've no idea why people seem to think they're supposed to cut'n'paste the physical limits of the regualtor out of the driver). If something decides to take advantage of the variation this could be problematic.
Quoted 10 lines
arch/arm/boot/dts/tegra20-paz00.dts line 318
Quoted 5 lines
arch/arm/boot/dts/tegra20-paz00.dts +5
-0+ regulator at 3 { + reg = <3>; + regulator-compatible = "ldo0"; + regulator-name = "+3.3vs_ldo0"; + regulator-min-microvolt = <1250000>;I think the common sense was that this should also be 3.3 V as it is the pex clock (which is not used at all on this board). I guess it doesn't matter much. So ...
Quoted 3 lines
Acked-by Marc Dietrich <[email protected]>Review
Quoted 1 line
(quoted diff) +1
-0+ regulator-max-microvolt = <3300000>;
This is one example, it looks like the rail needs to be fixed to 3.3V.
PATCH 3/3 ARM: dt: tegra: paz00: add regulators
Quoted 7 lines · On Sunday 24 June 2012 12:03:06 Mark Brown wrote:
Quoted 4 lines · Marc Dietrich
Quoted 4 lines
The regulator configurations were all taken from the AC100 kernel used by
the Ubuntu port, specifically:
These generally all look pretty broken...
Quoted 13 lines
Quoted 6 lines
arch/arm/boot/dts/tegra20-paz00.dts line 292
Quoted 6 lines
arch/arm/boot/dts/tegra20-paz00.dts +6
-0+ regulator at 0 { + reg = <0>; + regulator-compatible = "sm0"; + regulator-name = "+1.2vs_sm0"; + regulator-min-microvolt = < 725000>; + regulator-max-microvolt = <1300000>;Most of these ranges look suspiciously like the maximum possible variation the regulator has, not what the board actually requires (which is a depressingly common error, I've no idea why people seem to think they're supposed to cut'n'paste the physical limits of the regualtor out of the driver). If something decides to take advantage of the variation this could be problematic.
AFAIR we saw some instabilities with 1.2 V here, but looking back, that could also be related to something else. Finding these "undervolt" bugs is really hard to do. I indeed just copied the values from the original source (
http://gitorious.org/ac100/kernel/blobs/2.6.32/arch/arm/mach-
tegra/odm_kit/adaptations/pmu/tps6586x/nvodm_pmu_tps6586x.c#line136 ) and bumped up sm0 by 100 mV for the said stabilty reasons.
According to the datasheet, sm0 can go up to 1.5 V if I read it correctly, so 1.3 V is still inside the spec and not the maximum the regulator can provide.
Quoted 11 lines
Quoted 11 lines
arch/arm/boot/dts/tegra20-paz00.dts line 318
Quoted 6 lines
arch/arm/boot/dts/tegra20-paz00.dts +5
-0+ regulator at 3 { + reg = <3>; + regulator-compatible = "ldo0"; + regulator-name = "+3.3vs_ldo0"; + regulator-min-microvolt = <1250000>;I think the common sense was that this should also be 3.3 V as it is the pex clock (which is not used at all on this board). I guess it doesn't matter much. So ...
Quoted 5 lines
Quoted 3 lines
Acked-by Marc Dietrich <[email protected]>Review
Quoted 1 line
(quoted diff) +1
-0+ regulator-max-microvolt = <3300000>;This is one example, it looks like the rail needs to be fixed to 3.3V.
I think nowhere in the code a regulator (beside sm*) is programmed to some different value that the maximum given here (this is not the maximum the regulator can provide). I never understood why the kernel code always sets the regulator to the maximum value if no other value was specified. IMHO, there should be some initial value, e.g. regulator-default-microvolt, as the original driver (from 2.6.32 ages) did. This way the maximum value can be set to the hw limits, but maybe this is a bit dangerous.
Marc
PATCH 3/3 ARM: dt: tegra: paz00: add regulators
Quoted 7 lines · Marc Dietrich
Quoted 4 lines · On Sunday 24 June 2012 12:03:06 Mark Brown wrote:
Quoted 2 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ regulator-name = "+3.3vs_ldo0"; + regulator-max-microvolt = <3300000>;This is one example, it looks like the rail needs to be fixed to 3.3V.
Quoted 6 lines
I think nowhere in the code a regulator (beside sm*) is programmed to some different value that the maximum given here (this is not the maximum the regulator can provide). I never understood why the kernel code always sets the regulator to the maximum value if no other value was specified. IMHO, there should be some initial value, e.g. regulator-default-microvolt, as the original driver (from 2.6.32 ages) did. This way the maximum value can be set
That's *never* been in mainline, and nobody even bothered trying to submit it.
Quoted 1 line
to the hw limits, but maybe this is a bit dangerous.
One of two things should be happening. Either a single voltage is specified (in which case that voltage will be configured in the hardware and consumer drivers can't change anything) or a voltage range is specified (in which case the consumers are expected to manage the voltage and the most the API should do is bring the voltage within the limits given, though I don't think that's actually implemented yet). Specifying an initial value within the range should at best be redundant as the drivers that are actively managing their voltages will be overriding it anyway.
We certainly shouldn't be specifying the limits of the regulator itself as normally the board design will be much more constrained than the regulator itself and like I said it's stupid to have to cut'n'paste the numbers out of the driver into the machine constraints. We should instead be specifying the constraints the system is designed to operate in. Chances are that if nothing is able to actively manage the voltage it's not in fact safe to change the voltage at all and therefore the constraints should specify only one voltage.
In the above case the fact that the supply is named "+3.3vs_ldo0" seems
like a fairly clear sign that the board has been designed for this to
operate at 3.3V which makes the fact that the constraints go down to
1.25V seem at best odd.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120624/c1d854e4/attachment.sig>
PATCH 3/3 ARM: dt: tegra: paz00: add regulators
Quoted 14 lines · On Sunday 24 June 2012 13:31:51 Mark Brown wrote:
Quoted 6 lines · Marc Dietrich
Quoted 4 lines · On Sunday 24 June 2012 12:03:06 Mark Brown wrote:
Quoted 2 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ regulator-name = "+3.3vs_ldo0"; + regulator-max-microvolt = <3300000>;This is one example, it looks like the rail needs to be fixed to 3.3V.
Quoted 7 lines
I think nowhere in the code a regulator (beside sm*) is programmed to some different value that the maximum given here (this is not the maximum the regulator can provide). I never understood why the kernel code always sets the regulator to the maximum value if no other value was specified. IMHO, there should be some initial value, e.g. regulator-default-microvolt, as the original driver (from 2.6.32 ages) did. This way the maximum value can be set
Quoted 2 lines
That's *never* been in mainline, and nobody even bothered trying to submit it.
which was the best thing to do ;-)
Quoted 4 lines
Quoted 1 line
to the hw limits, but maybe this is a bit dangerous.
One of two things should be happening. Either a single voltage is specified (in which case that voltage will be configured in the
I'm not an expert on this, but it seems to me that only sm0 and sm1 should be changeable (and some rail called vdd_aon, which seems to be ldo2 in case of paz00 connected to the rtc). So, all others can be constant voltage. Maybe Stephen can comment on the actual requirements (also for the other boards which may have similar layout).
Marc
Quoted 21 lines
hardware and consumer drivers can't change anything) or a voltage range is specified (in which case the consumers are expected to manage the voltage and the most the API should do is bring the voltage within the limits given, though I don't think that's actually implemented yet). Specifying an initial value within the range should at best be redundant as the drivers that are actively managing their voltages will be overriding it anyway.
We certainly shouldn't be specifying the limits of the regulator itself as normally the board design will be much more constrained than the regulator itself and like I said it's stupid to have to cut'n'paste the numbers out of the driver into the machine constraints. We should instead be specifying the constraints the system is designed to operate in. Chances are that if nothing is able to actively manage the voltage it's not in fact safe to change the voltage at all and therefore the constraints should specify only one voltage.
In the above case the fact that the supply is named "+3.3vs_ldo0" seems like a fairly clear sign that the board has been designed for this to operate at 3.3V which makes the fact that the constraints go down to 1.25V seem at best odd.
PATCH 3/3 ARM: dt: tegra: paz00: add regulators
Quoted 10 lines · Marc Dietrich
Quoted 6 lines · On Sunday 24 June 2012 13:31:51 Mark Brown wrote:
Quoted 3 lines
there should be some initial value, e.g. regulator-default-microvolt, as the original driver (from 2.6.32 ages) did. This way the maximum value can be set
That's *never* been in mainline, and nobody even bothered trying to submit it.
which was the best thing to do ;-)
Well, no - had there been some attempt to submit it it might've helped
stop people writing constaints like this.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120625/8a8ff01d/attachment-0001.sig>
PATCH 3/3 ARM: dt: tegra: paz00: add regulators
Am Montag, 25. Juni 2012, 09:46:56 schrieb Mark Brown:
Quoted 7 lines · Marc Dietrich
Quoted 6 lines · On Sunday 24 June 2012 13:31:51 Mark Brown wrote:
Quoted 5 lines
Quoted 4 lines
there should be some initial value, e.g. regulator-default-microvolt, as
the original driver (from 2.6.32 ages) did. This way the maximum value can be set
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
That's *never* been in mainline, and nobody even bothered trying to submit it.
which was the best thing to do ;-)
Well, no - had there been some attempt to submit it it might've helped stop people writing constaints like this.
by saying "best thing to do" I meant that the driver isn't mainline compatible as it is based on some cross OS HAL. On the other hand, I think the current tps6586x could be enhanced to include such a field. I don't know if the regulator api supports this, though.
Marc
PATCH 3/3 ARM: dt: tegra: paz00: add regulators
In reply to Marc DietrichQuoted 22 lines · Marc Dietrich
Quoted 14 lines · On Sunday 24 June 2012 13:31:51 Mark Brown wrote:
Quoted 6 lines · Marc Dietrich
Quoted 4 lines · On Sunday 24 June 2012 12:03:06 Mark Brown wrote:
Quoted 2 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ regulator-name = "+3.3vs_ldo0"; + regulator-max-microvolt = <3300000>;This is one example, it looks like the rail needs to be fixed to 3.3V.
Quoted 7 lines
I think nowhere in the code a regulator (beside sm*) is programmed to some different value that the maximum given here (this is not the maximum the regulator can provide). I never understood why the kernel code always sets the regulator to the maximum value if no other value was specified. IMHO, there should be some initial value, e.g. regulator-default-microvolt, as the original driver (from 2.6.32 ages) did. This way the maximum value can be set
Quoted 2 lines
That's *never* been in mainline, and nobody even bothered trying to submit it.
which was the best thing to do ;-)
Quoted 2 lines
Quoted 1 line
to the hw limits, but maybe this is a bit dangerous.
Quoted 8 lines
Quoted 2 lines
One of two things should be happening. Either a single voltage is specified (in which case that voltage will be configured in the
I'm not an expert on this, but it seems to me that only sm0 and sm1 should be changeable (and some rail called
vdd_aon, which seems to be ldo2 in case of paz00 connected to the rtc). So, all others can be constant voltage. Maybe Stephen can comment on the actual requirements (also for the other boards which may have similar layout).
I can confirm that at least for ldo0 the value needs to be fixed. I did in fact post a patch back in February that was needed to fix PCIe on Harmony. I also sat down with one of our hardware engineers and worked through the list and wrote down the requirements for Harmony. I need to check where our notes have gone, though.
Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120625/954f8773/attachment.sig>
PATCH 3/3 ARM: dt: tegra: paz00: add regulators
In reply to Mark BrownQuoted 7 lines · Mark Brown
Quoted 2 lines · Marc Dietrich
Quoted 2 lines
The regulator configurations were all taken from the AC100 kernel used by the Ubuntu port, specifically:
These generally all look pretty broken...
Quoted 13 lines
Quoted 6 lines
arch/arm/boot/dts/tegra20-paz00.dts line 292
Quoted 6 lines
arch/arm/boot/dts/tegra20-paz00.dts +6
-0+ regulator at 0 { + reg = <0>; + regulator-compatible = "sm0"; + regulator-name = "+1.2vs_sm0"; + regulator-min-microvolt = < 725000>; + regulator-max-microvolt = <1300000>;Most of these ranges look suspiciously like the maximum possible variation the regulator has, not what the board actually requires (which is a depressingly common error, I've no idea why people seem to think they're supposed to cut'n'paste the physical limits of the regualtor out of the driver). If something decides to take advantage of the variation this could be problematic.
So I think this sm0 (and the sm1) entry might actually be correct; sm0 feeds vdd_core and sm1 feeds vdd_cpu. These rails have DVFS and hence the voltage can vary. I guess I should change the regulator-name in these cases to something more useful than the very first signal name on the schematics too.
That said, we don't actually have DVFS in the mainline kernel yet, and correlating the regulator limits in our downstream kernels against the DVFS tables we use is ... challenging; I'm not sure they're consistent anyway:-(
However, it probably doesn't make sense to vary sm2, since all that is used for is to feed the TPS6586x's input pins for the the LDO regulators.
Likewise, all the LDO regulators are used for various peripherals in general, and in the main it probably makes no sense for those rails to vary.
So, what I think I'll do for any regulators where the downstream board files specify a voltage range, is boot the kernel and find out what voltage is selected by default, and program both min-/max-microvolt to that specific value. That way, there will be no behaviour change. We can expand the ranges on the regulators later if/when we add DVFS etc. Does that sound like a reasonable approach?
PATCH 3/3 ARM: dt: tegra: paz00: add regulators
Quoted 5 lines · Stephen Warren
So I think this sm0 (and the sm1) entry might actually be correct; sm0 feeds
vdd_coreand sm1 feedsvdd_cpu. These rails have DVFS and hence the voltage can vary. I guess I should change the regulator-name in these cases to something more useful than the very first signal name on the schematics too.
Even if they feed these supplies are they capable of varying on this board if they're shared with other things? This is one of the common issues with constraints... But generally if the supply covers more than one thing the idiomatic thing is to list them all separated by / or something.
Quoted 4 lines
That said, we don't actually have DVFS in the mainline kernel yet, and correlating the regulator limits in our downstream kernels against the DVFS tables we use is ... challenging; I'm not sure they're consistent anyway:-(
Well, if you're using the regulator API the regulator API limits will win if they're more constrained. But this is half the problem with these silly constraints that just copy the regulator limits, you've no idea what the board is actually supposed to do.
Quoted 2 lines
However, it probably doesn't make sense to vary sm2, since all that is used for is to feed the TPS6586x's input pins for the the LDO regulators.
Actually if we get clever then there's some fun to be had there. If we can have all the LDOs specify the headroom they need then we should be able to arrange to vary the DCDC depending on what the needs of the currently active LDOs are which would improve power efficiency as the goal here is to drop the voltage as much as possible using the more efficient DCDC then regulate on from there with the LDOs.
I keep considering doing this but don't have any real need for it myself, it's just amusing. Might fall out of (or be similar to) some work I'm going to be doing soon for bypass modes though.
Quoted 2 lines
Likewise, all the LDO regulators are used for various peripherals in general, and in the main it probably makes no sense for those rails to vary.
This is typically very unusual, the devices on the rails are normally heavily constrained when active.
Quoted 6 lines
So, what I think I'll do for any regulators where the downstream board files specify a voltage range, is boot the kernel and find out what voltage is selected by default, and program both min-/max-microvolt to that specific value. That way, there will be no behaviour change. We can expand the ranges on the regulators later if/when we add DVFS etc. Does that sound like a reasonable approach?
Yes.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120627/a89ae1f2/attachment-0001.sig>
PATCH 3/3 ARM: dt: tegra: paz00: add regulators
Quoted 8 lines · Mark Brown
Quoted 5 lines · Stephen Warren
So I think this sm0 (and the sm1) entry might actually be correct; sm0 feeds
vdd_coreand sm1 feedsvdd_cpu. These rails have DVFS and hence the voltage can vary. I guess I should change the regulator-name in these cases to something more useful than the very first signal name on the schematics too.
Quoted 5 lines
Even if they feed these supplies are they capable of varying on this board if they're shared with other things? This is one of the common issues with constraints... But generally if the supply covers more than one thing the idiomatic thing is to list them all separated by / or something.
These two supplies aren't shared, it's just that the signal on the schematic ends up being named different things in different places; on the regulator page it's named vdd_sm0, but on some other page it gets renamed vdd_core.
PATCH 3/3 ARM: dt: tegra: paz00: add regulators
In reply to Mark BrownQuoted 8 lines · Mark Brown
Quoted 5 lines · Stephen Warren
So I think this sm0 (and the sm1) entry might actually be correct; sm0 feeds
vdd_coreand sm1 feedsvdd_cpu. These rails have DVFS and hence the voltage can vary. I guess I should change the regulator-name in these cases to something more useful than the very first signal name on the schematics too.
Quoted 5 lines
Even if they feed these supplies are they capable of varying on this board if they're shared with other things? This is one of the common issues with constraints... But generally if the supply covers more than one thing the idiomatic thing is to list them all separated by / or something.
Just FYI, the particular choice of "/" as a separator doesn't work well, because then /sys/kernel/debug/regulator/${regulator-name} can't be created. I guess I'll use a comma. I assume it's not worth fixing the regulator core to s@/@,@ when generating the debugfs filenames?
PATCH 3/3 ARM: dt: tegra: paz00: add regulators
Quoted 4 lines · Stephen Warren
Just FYI, the particular choice of "/" as a separator doesn't work well, because then
/sys/kernel/debug/regulator/${regulator-name}can't be created. I guess I'll use a comma. I assume it's not worth fixing the regulator core to s@/@,@ when generating the debugfs filenames?
I don't think that's sensible, no.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120630/9ea5b02b/attachment.sig>
PATCH 1/3 ARM: dt: tegra: seaboard: add regulators
In reply to Stephen WarrenQuoted 16 lines · Stephen Warren
From: Stephen Warren<[email protected]>
Seaboard uses a TPS6586x regulator. Instantiate this, and hook up a couple of fixed GPIO-controlled regulators too.
The regulator configurations were mostly taken from the ChromeOS 3.2 kernel. Exceptions are:
* The schematic lists a fixed voltage for each rail, whereas the ChromeOS kernel lists a range for many rails. I used the values from the ChromeOS kernel in all cases, since I know the board file there is the most complete available for this hardware.
* The
vdd_1v2fixed regulator is present only in the schematic. So, I added this based on the schematic.
Quoted 6 lines
* A 3.3v fixed regulator using GPIO3 of the TPS6586x is present in the ChromeOS kernel, but not in the schematic. So, I dropped this based on the schematic.
Signed-off-by Stephen Warren<[email protected]>---
I think this series very much depends on the series
[PATCH V3 0/3] regulator: dt: add policy to match regulator with prop "regulator-compatible"
Review
Quoted 7 lines
(quoted diff) +7
-0+ regulator at 3 { + reg =<3>; + regulator-compatible = "ldo0"; + regulator-name = "vdd_ldo0"; + regulator-min-microvolt =<1250000>; + regulator-max-microvolt =<3300000>; + vin-supply =<&sm2_reg>;
I think support for vin-supply is still not there for this regulator in driver.
PATCH 1/3 ARM: dt: tegra: seaboard: add regulators
Quoted 23 lines · Laxman Dewangan
Quoted 22 lines · Stephen Warren
From: Stephen Warren<[email protected]>
Seaboard uses a TPS6586x regulator. Instantiate this, and hook up a couple of fixed GPIO-controlled regulators too.
The regulator configurations were mostly taken from the ChromeOS 3.2 kernel. Exceptions are:
* The schematic lists a fixed voltage for each rail, whereas the ChromeOS kernel lists a range for many rails. I used the values from the
ChromeOS
kernel in all cases, since I know the board file there is the most complete available for this hardware.* The
vdd_1v2fixed regulator is present only in the schematic. So, I added
this based on the schematic.* A 3.3v fixed regulator using GPIO3 of the TPS6586x is present in the ChromeOS kernel, but not in the schematic. So, I dropped this based on the schematic.
Quoted 8 lines
Quoted 2 lines
Signed-off-by Stephen Warren<[email protected]>---
Acked-by Laxman Dewangan <[email protected]>I think this series very much depends on the series
[PATCH V3 0/3] regulator: dt: add policy to match regulator with prop "regulator-compatible"
That's certainly true. However, it's a runtime dependency to enable a new feature, and shouldn't cause any breakage without that patch, so we don't need to be explicit about the dependency in git.
Quoted 10 lines
Review
Quoted 7 lines
(quoted diff) +7
-0+ regulator at 3 { + reg =<3>; + regulator-compatible = "ldo0"; + regulator-name = "vdd_ldo0"; + regulator-min-microvolt =<1250000>; + regulator-max-microvolt =<3300000>; + vin-supply =<&sm2_reg>;I think support for vin-supply is still not there for this regulator in driver.
That's also true. I wonder if we shouldn't support this in the regulator core bindings instead, since the existence of a parent regulator seems likely to be common. Either way, I'd like to include the property to document it for now.
PATCH 1/3 ARM: dt: tegra: seaboard: add regulators
Quoted 10 lines · Stephen Warren
Quoted 9 lines · Laxman Dewangan
Quoted 8 lines · Stephen Warren
From: Stephen Warren<[email protected]>
Seaboard uses a TPS6586x regulator. Instantiate this, and hook up a couple of fixed GPIO-controlled regulators too.
Signed-off-by Stephen Warren<[email protected]>---
Quoted 9 lines
Quoted 5 lines
Acked-by Laxman Dewangan<[email protected]>I think this series very much depends on the series
[PATCH V3 0/3] regulator: dt: add policy to match regulator with prop "regulator-compatible"That's certainly true. However, it's a runtime dependency to enable a new feature, and shouldn't cause any breakage without that patch, so we don't need to be explicit about the dependency in git.
Here, wanted to say that although we enable it but it will not work. The functionality depends on the above patch.
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
(quoted diff) +7
-0+ regulator at 3 { + reg =<3>; + regulator-compatible = "ldo0"; + regulator-name = "vdd_ldo0"; + regulator-min-microvolt =<1250000>; + regulator-max-microvolt =<3300000>; + vin-supply =<&sm2_reg>;
Quoted 6 lines
Quoted 2 lines
I think support for vin-supply is still not there for this regulator in driver.
That's also true. I wonder if we shouldn't support this in the regulator core bindings instead, since the existence of a parent regulator seems likely to be common. Either way, I'd like to include the property to document it for now.
I had detailed discussion with Mark on this support and as per him (based on my understanding), the input to different regulator is from the pin of the chips and so the name should be the <pin-name>-supply which should be part of chip-dt binding, not to the particular rail.
PATCH 1/3 ARM: dt: tegra: seaboard: add regulators
Quoted 7 lines · Laxman Dewangan
Quoted 6 lines · Stephen Warren
Quoted 5 lines · Laxman Dewangan
Quoted 4 lines · Stephen Warren
From: Stephen Warren<[email protected]>
Seaboard uses a TPS6586x regulator. Instantiate this, and hook up a couple of fixed GPIO-controlled regulators too.
...
Quoted 14 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
(quoted diff) +7
-0+ regulator at 3 { + reg =<3>; + regulator-compatible = "ldo0"; + regulator-name = "vdd_ldo0"; + regulator-min-microvolt =<1250000>; + regulator-max-microvolt =<3300000>; + vin-supply =<&sm2_reg>;Quoted 6 lines
Quoted 2 lines
I think support for vin-supply is still not there for this regulator in driver.
That's also true. I wonder if we shouldn't support this in the regulator core bindings instead, since the existence of a parent regulator seems likely to be common. Either way, I'd like to include the property to document it for now.
Quoted 4 lines
I had detailed discussion with Mark on this support and as per him (based on my understanding), the input to different regulator is from the pin of the chips and so the name should be the <pin-name>-supply which should be part of chip-dt binding, not to the particular rail.
OK, that's fine. Can you please update the TPS6586x binding and driver to allow this to be represented correctly then. Thanks.
PATCH 1/3 ARM: dt: tegra: seaboard: add regulators
In reply to Laxman DewanganQuoted 5 lines · Laxman Dewangan
I had detailed discussion with Mark on this support and as per him (based on my understanding), the input to different regulator is from the pin of the chips and so the name should be the <pin-name>-supply which should be part of chip-dt binding, not to the particular rail.
More specifically, all the supplies for a device (including those that
happen to be inputs for regulators) should be specified in exactly the
same fashion. This makes the binding more regular and means that users
can just go through the schematic adding the mappings without worrying
about what what the supply happens to be.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120625/342fb195/attachment.sig>
PATCH 1/3 ARM: dt: tegra: seaboard: add regulators
Quoted 8 lines · Mark Brown
Quoted 5 lines · Laxman Dewangan
I had detailed discussion with Mark on this support and as per him (based on my understanding), the input to different regulator is from the pin of the chips and so the name should be the <pin-name>-supply which should be part of chip-dt binding, not to the particular rail.
Quoted 6 lines
More specifically, all the supplies for a device (including those that happen to be inputs for regulators) should be specified in exactly the same fashion. This makes the binding more regular and means that users can just go through the schematic adding the mappings without worrying about what what the supply happens to be.
Just making sure I parsed that right. I think what you're saying is that the device itself should represent its input pins, e.g.:
tps6586x {
vin-ldo01-supply = <&some_regulator>;
vin-ldo23-supply = <...>;
vin-ldo4-supply = <...>;
vin-ldo678-supply = <...>;
vin-ldo9-supply = <...>;
regulators {
regulator at 0 {
regulator-compatible = "ldo0";
...
};
regulator at 1 {
regulator-compatible = "ldo1";
...
};
};
};(and then the driver internally uses the *-supply to set up the parents of each of its own regulators)
... rather than each regulator specifying its parent, which might result in some duplication, since in this case both ldo0/1 are supplied from the same input pin:
tps6586x {
regulators {
regulator at 0 {
regulator-compatible = "ldo0";
vin-supply = <&some_regulator>;
...
};
regulator at 1 {
regulator-compatible = "ldo1";
vin-supply = <&some_regulator>;
...
};
};
};PATCH 1/3 ARM: dt: tegra: seaboard: add regulators
Quoted 14 lines · Stephen Warren
Quoted 7 lines · Mark Brown
Quoted 5 lines · Laxman Dewangan
I had detailed discussion with Mark on this support and as per him (based on my understanding), the input to different regulator is from the pin of the chips and so the name should be the <pin-name>-supply which should be part of chip-dt binding, not to the particular rail.
Quoted 6 lines
More specifically, all the supplies for a device (including those that happen to be inputs for regulators) should be specified in exactly the same fashion. This makes the binding more regular and means that users can just go through the schematic adding the mappings without worrying about what what the supply happens to be.
Quoted 5 lines
Just making sure I parsed that right. I think what you're saying is that the device itself should represent its input pins, e.g.:
tps6586x { vin-ldo01-supply =<&some_regulator>;
I think it is fine. The pin name as per data sheet is VINLDO01 and so we can have vin-ldo01.
Quoted 38 lines
vin-ldo23-supply =<...>; vin-ldo4-supply =<...>; vin-ldo678-supply =<...>; vin-ldo9-supply =<...>; regulators { regulator at 0 { regulator-compatible = "ldo0"; ... }; regulator at 1 { regulator-compatible = "ldo1"; ... }; }; };(and then the driver internally uses the *-supply to set up the parents of each of its own regulators)
... rather than each regulator specifying its parent, which might result in some duplication, since in this case both ldo0/1 are supplied from the same input pin:
tps6586x { regulators { regulator at 0 { regulator-compatible = "ldo0"; vin-supply =<&some_regulator>; ... }; regulator at 1 { regulator-compatible = "ldo1"; vin-supply =<&some_regulator>; ... }; }; };
PATCH 1/3 ARM: dt: tegra: seaboard: add regulators
In reply to Stephen WarrenQuoted 5 lines · Stephen Warren
Just making sure I parsed that right. I think what you're saying is that the device itself should represent its input pins, e.g.:
tps6586x { vin-ldo01-supply = <&some_regulator>;
Yes, that's what I'd expect here.
Quoted 3 lines
... rather than each regulator specifying its parent, which might result in some duplication, since in this case both ldo0/1 are supplied from the same input pin:
Plus you then get back to the problem of users having to figure out
where to put the references which makes everything harder to use.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120626/01d6d3ed/attachment.sig>
PATCH 1/3 ARM: dt: tegra: seaboard: add regulators
In reply to Stephen WarrenHi Mark,
I require your input on supporting the vin-supply for tps6586x.
Quoted 11 lines · Stephen Warren
Quoted 7 lines · Mark Brown
More specifically, all the supplies for a device (including those that happen to be inputs for regulators) should be specified in exactly the same fashion. This makes the binding more regular and means that users can just go through the schematic adding the mappings without worrying about what what the supply happens to be.
Just making sure I parsed that right. I think what you're saying is that the device itself should represent its input pins, e.g.:
Quoted 8 lines
tps6586x { vin-ldo01-supply =<&some_regulator>; vin-ldo23-supply =<...>; vin-ldo4-supply =<...>; vin-ldo678-supply =<...>; vin-ldo9-supply =<...>;:::::::::::
};
Looked tps6586x-regulator driver and it has the platform data which is regulator_init_data.
So for adding the vin-supply similar to what we have in fixed or tps6591x regulator to pass through the desc.supply_name, we are not having option here in platform data which can work for DT and non-DT case.
So if still want to have the DT and non-DT case similar, we can add one tps6586x_regulator_platform_data as
struct tps6586x_regulator_platform_data
{
const char *input_supply;
struct regulator_init_data *reg_init_data;
}and then pass this when registering the regulator.
Or,
second option is to support the input supply name for DT case through desc.supply_name and for non-DT let it be there through regulator_init_data.
Please let me know your opinion.
Thanks,
Laxman
PATCH 1/3 ARM: dt: tegra: seaboard: add regulators
Quoted 9 lines · Laxman Dewangan
Looked tps6586x-regulator driver and it has the platform data which is
regulator_init_data.
So for adding the vin-supply similar to what we have in fixed or tps6591x regulator to pass through the desc.supply_name, we are not having option here in platform data which can work for DT and non-DT case.So if still want to have the DT and non-DT case similar, we can add one
tps6586x_regulator_platform_dataas
The supply names are not supposed to be conditional (I realise they are
for your current DT patch, I let that in because I just want to see an
end to all this DT stuff).
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120710/258f9afe/attachment.sig>
PATCH 1/3 ARM: dt: tegra: seaboard: add regulators
Quoted 12 lines · Mark Brown
* PGP Signed by an unknown key
Quoted 8 lines · Laxman Dewangan
Looked tps6586x-regulator driver and it has the platform data which is
regulator_init_data.
So for adding the vin-supply similar to what we have in fixed or tps6591x regulator to pass through the desc.supply_name, we are not having option here in platform data which can work for DT and non-DT case.
So if still want to have the DT and non-DT case similar, we can add onetps6586x_regulator_platform_dataas
Quoted 4 lines
The supply names are not supposed to be conditional (I realise they are for your current DT patch, I let that in because I just want to see an end to all this DT stuff).
Sorry, I did not get it fully. I had two patches, one for fixed one and other for tps65910. On which patch, you are seeing issue and which part of code?
I like to fix it if it does not conform the DT or your expectation or any future enhancement which you are planning.
Little bit more details will help so that I will not repeat the same and fix it in my future patches.
Thanks,
Laxman
PATCH 1/3 ARM: dt: tegra: seaboard: add regulators
Quoted 3 lines · Laxman Dewangan
Sorry, I did not get it fully. I had two patches, one for fixed one and other for tps65910. On which patch, you are seeing issue and which part of code?
Mainly tps65910. The fixed voltage regulator does have the same issue
but it is somewhat special here.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120710/c0c51521/attachment.sig>
PATCH 1/3 ARM: dt: tegra: seaboard: add regulators
Quoted 9 lines · Mark Brown
* PGP Signed by an unknown key
Quoted 3 lines · Laxman Dewangan
Sorry, I did not get it fully. I had two patches, one for fixed one and other for tps65910. On which patch, you are seeing issue and which part of code?
Mainly tps65910. The fixed voltage regulator does have the same issue but it is somewhat special here.
Oops, I follow the same approach for tps65910 what it was with fixed regulator, was thinking that this is good approach.
The sample code for tps65910 is as follows. So where do you think that it is wrong and what should be correct approach.
I need to follow the same for tps6586x also.
@@ -1023,6 +1051,13 @@ static struct tps65910_board *tps65910_parse_dt_reg_data(
"ti,regulator-ext-sleep-control", &prop);
if (!ret)
pmic_plat_data->regulator_ext_sleep_control[idx] = prop;+
+
+ if (info->vin_name) {
+ snprintf(in_supply, 32, "%s-supply", info->vin_name);
+ if (of_find_property(np, in_supply, 0))
+ pmic_plat_data->input_supply[idx] =
+ info->vin_name;
+ }
}
return pmic_plat_data;
@@ -1126,6 +1161,7 @@ static __devinit int tps65910_probe(struct
platform_device *pdev)
pmic->info[i] = info;
pmic->desc[i].name = info->name;
+ pmic->desc[i].supply_name = pmic_plat_data->input_supply[i];
pmic->desc[i].id = i;PATCH 1/3 ARM: dt: tegra: seaboard: add regulators
Quoted 3 lines · Laxman Dewangan
The sample code for tps65910 is as follows. So where do you think that it is wrong and what should be correct approach.
I need to follow the same for tps6586x also.
Like I said the use of a supply name should be unconditional, you should
just set the supply name in the descriptor.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120710/75a398e8/attachment-0001.sig>
PATCH 1/3 ARM: dt: tegra: seaboard: add regulators
Quoted 10 lines · Mark Brown
* PGP Signed by an unknown key
Quoted 3 lines · Laxman Dewangan
The sample code for tps65910 is as follows. So where do you think that it is wrong and what should be correct approach.
I need to follow the same for tps6586x also.Like I said the use of a supply name should be unconditional, you should just set the supply name in the descriptor.
if I set the desc.supply_name unconditional then if the entry of that supply name is not there in the DT entry (on device node) then that regulator will not get registered because the search for supply_regulator will fail during registration.
I am setting the desc.supply only when there is supply entry in DT or if it is from platform.
Do we need to make such supply pin entry as required inplace of optional?
PATCH 1/3 ARM: dt: tegra: seaboard: add regulators
Quoted 1 line · Laxman Dewangan
Do we need to make such supply pin entry as required inplace of optional?
That's the idea.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120710/fc726e9b/attachment.sig>
PATCH 1/3 ARM: dt: tegra: seaboard: add regulators
Quoted 6 lines · Mark Brown
* PGP Signed by an unknown key
Quoted 1 line · Laxman Dewangan
Do we need to make such supply pin entry as required inplace of optional?
That's the idea.
Then I will have 2 question:
1. If that pin is connected to battery then what should be entry?
vcc1-supply = <>>>;
Do we need to register battery supply as fixed (non-gpio based) and then refer here for phandle?
2. what about non-dt case? The regulator registration will also fail here because of there is no regulator saying vcc1 in this case. Do we need to register the vcc1 regualtor as fixed, battery supplied regulator in board files so that the ldo's registration will success?
PATCH 1/3 ARM: dt: tegra: seaboard: add regulators
Quoted 5 lines · Laxman Dewangan
Then I will have 2 question:
1. If that pin is connected to battery then what should be entry? vcc1-supply = <>>>;
Do we need to register battery supply as fixed (non-gpio based) and then refer here for phandle?
Use a fixed voltage regulator to represent the battery.
Quoted 5 lines
2. what about non-dt case? The regulator registration will also fail here because of there is no regulator saying vcc1 in this case. Do we need to register the vcc1 regualtor as fixed, battery supplied regulator in board files so that the ldo's registration will success?
Same thing, use a fixed voltage regulator.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120710/428c71d0/attachment.sig>
PATCH 1/3 ARM: dt: tegra: seaboard: add regulators
Quoted 9 lines · Mark Brown
* PGP Signed by an unknown key
Quoted 5 lines · Laxman Dewangan
Then I will have 2 question:
1. If that pin is connected to battery then what should be entry?vcc1-supply =<>>>;Do we need to register battery supply as fixed (non-gpio based) and then refer here for phandle?
Quoted 8 lines
Use a fixed voltage regulator to represent the battery.
Quoted 5 lines
2. what about non-dt case? The regulator registration will also fail here because of there is no regulator saying vcc1 in this case. Do we need to register the vcc1 regualtor as fixed, battery supplied regulator in board files so that the ldo's registration will success?
Same thing, use a fixed voltage regulator.
Understood completely and it is so simple to support vin-supply. Will implement the same. Thanks for clearing doubts.