[PATCH 0/6] tps65090 DTS refactoring and improvements

[PATCH 0/6] tps65090 DTS refactoring and improvements

[Javier Martinez Canillas]

This series does a refactoring by creating dtsi files for the
tps65090 PMU that can be included by DT board files that have
this component. This not only allow to remove duplicated code
but also makes it easier to maintain the tps65090 information.

So the series also improve the tps65090 description by adding
regulator constraints information and also the power scheme
for some boards that are using a simplistic model for the
connection between the regulators in the tps65090.

The refactoring was made after Doug Anderson suggestion on a
previous patch that just attempted to add the constraints to
the Peach machines DTS [0].

Patch 01/06 creates a .dtsi fragment for the tps65090 and patch
02/06 removes the duplicated definitions from the Exynos Snow
board. Patch 03/06 creates a fragment that is more specific for
the tps65090 usage in Chrome OS machines and patch 04/06 uses it
for Peach boards. Patch 05/06 model the real regulators relation
instead of the simplistic model that was used before and finally
patch 06/06 adds the tps65090 regulators constraints according to
the data manual.

NOTES: The NVIDIA Tegra114 Dalmore evaluation board also uses this
PMU and can be converted to use the .dtsi but I didn't include in
this series to avoid unnecessary cross subsystem churn. Once this
is included I can post as a follow-up patch.

Maybe the Snow can also use the cros-tps65090.dtsi instead of just
the tps65090.dtsi but someone with the schematics for this board
has to double check and can be done in a follow-up patch as well.

Javier Martinez Canillas (6):
  ARM: dts: Create fragment for tps65090 PMU
  ARM: dts: Use tps65090 fragment in exynos5250-snow
  ARM: dts: Create cros-tps65090 fragment
  ARM: dts: Use cros-tps65090 fragment in Peach boards
  ARM: dts: Improve cros-tps65090 power scheme
  ARM: dts: Add tps65090 FETs constraints

 arch/arm/boot/dts/cros-tps65090.dtsi       |  81 ++++++++++++++++++++++
 arch/arm/boot/dts/exynos5250-snow.dts      | 108 ++++++++++++++---------------
 arch/arm/boot/dts/exynos5420-peach-pit.dts |  95 ++++++-------------------
 arch/arm/boot/dts/exynos5800-peach-pi.dts  |  94 ++++++-------------------
 arch/arm/boot/dts/tps65090.dtsi            |  75 ++++++++++++++++++++
 5 files changed, 251 insertions(+), 202 deletions(-)
 create mode 100644 arch/arm/boot/dts/cros-tps65090.dtsi
 create mode 100644 arch/arm/boot/dts/tps65090.dtsi

Best regards,
Javier

[0]: https://lkml.org/lkml/2014/8/11/202

[PATCH 1/6] ARM: dts: Create fragment for tps65090 PMU

[Javier Martinez Canillas]

The tps65090 is a Power Management Unit (PMU) used in several
boards so the same information is described on different DTS.
It is better to create a .dtsi fragment that can be included.

Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
 arch/arm/boot/dts/tps65090.dtsi | 57 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)
 create mode 100644 arch/arm/boot/dts/tps65090.dtsi

diff --git a/arch/arm/boot/dts/tps65090.dtsi b/arch/arm/boot/dts/tps65090.dtsi
new file mode 100644
index 0000000..a2ff534
--- /dev/null
+++ b/arch/arm/boot/dts/tps65090.dtsi
@@ -0,0 +1,57 @@
+/*
+ * Copyright (C) 2014 Google, Inc
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+/*
+ * Integrated Power Management Chip
+ * http://www.ti.com/lit/ds/symlink/tps65090.pdf
+ */
+&tps {
+	compatible = "ti,tps65090";
+
+	regulators {
+		tps65090_dcdc1: dcdc1 {
+		};
+
+		tps65090_dcdc2: dcdc2 {
+		};
+
+		tps65090_dcdc3: dcdc3 {
+		};
+
+		tps65090_fet1: fet1 {
+		};
+
+		tps65090_fet2: fet2 {
+		};
+
+		tps65090_fet3: fet3 {
+		};
+
+		tps65090_fet4: fet4 {
+		};
+
+		tps65090_fet5: fet5 {
+		};
+
+		tps65090_fet6: fet6 {
+		};
+
+		tps65090_fet7: fet7 {
+		};
+
+		tps65090_ldo1: ldo1 {
+		};
+
+		tps65090_ldo2: ldo2 {
+		};
+	};
+
+	tps65090_charger: charger {
+		compatible = "ti,tps65090-charger";
+	};
+};
-- 
2.0.0.rc2

[PATCH 2/6] ARM: dts: Use tps65090 fragment in exynos5250-snow

[Javier Martinez Canillas]

Now that there is a .dtsi fragment file for the tps65090 PMU,
include it in the Exynos Snow DTS file to reduce duplication.

Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
 arch/arm/boot/dts/exynos5250-snow.dts | 108 +++++++++++++++++-----------------
 1 file changed, 54 insertions(+), 54 deletions(-)

diff --git a/arch/arm/boot/dts/exynos5250-snow.dts b/arch/arm/boot/dts/exynos5250-snow.dts
index f2b8c41..159a497 100644
--- a/arch/arm/boot/dts/exynos5250-snow.dts
+++ b/arch/arm/boot/dts/exynos5250-snow.dts
@@ -147,8 +147,7 @@
 				wakeup-source;
 			};
 
-			power-regulator {
-				compatible = "ti,tps65090";
+			tps: power-regulator {
 				reg = <0x48>;
 
 				/*
@@ -170,58 +169,6 @@
 				infet7-supply = <&vbat>;
 				vsys-l1-supply = <&vbat>;
 				vsys-l2-supply = <&vbat>;
-
-				regulators {
-					dcdc1 {
-						ti,enable-ext-control;
-					};
-					dcdc2 {
-						ti,enable-ext-control;
-					};
-					dcdc3 {
-						ti,enable-ext-control;
-					};
-					fet1 {
-						regulator-name = "vcd_led";
-						ti,overcurrent-wait = <3>;
-					};
-					tps65090_fet2: fet2 {
-						regulator-name = "video_mid";
-						regulator-always-on;
-						ti,overcurrent-wait = <3>;
-					};
-					fet3 {
-						regulator-name = "wwan_r";
-						regulator-always-on;
-						ti,overcurrent-wait = <3>;
-					};
-					fet4 {
-						regulator-name = "sdcard";
-						ti,overcurrent-wait = <3>;
-					};
-					fet5 {
-						regulator-name = "camout";
-						regulator-always-on;
-						ti,overcurrent-wait = <3>;
-					};
-					fet6 {
-						regulator-name = "lcd_vdd";
-						ti,overcurrent-wait = <3>;
-					};
-					tps65090_fet7: fet7 {
-						regulator-name = "video_mid_1a";
-						regulator-always-on;
-						ti,overcurrent-wait = <3>;
-					};
-					ldo1 {
-					};
-					ldo2 {
-					};
-				};
-
-				charger {
-					compatible = "ti,tps65090-charger";
-				};
 			};
 		};
 	};
@@ -344,6 +291,59 @@
 	};
 };
 
+#include "tps65090.dtsi"
+
+&tps65090_dcdc1 {
+	ti,enable-ext-control;
+};
+
+&tps65090_dcdc2 {
+	ti,enable-ext-control;
+};
+
+&tps65090_dcdc3 {
+	ti,enable-ext-control;
+};
+
+&tps65090_fet1 {
+	regulator-name = "vcd_led";
+	ti,overcurrent-wait = <3>;
+};
+
+&tps65090_fet2 {
+	regulator-name = "video_mid";
+	regulator-always-on;
+	ti,overcurrent-wait = <3>;
+};
+
+&tps65090_fet3 {
+	regulator-name = "wwan_r";
+	regulator-always-on;
+	ti,overcurrent-wait = <3>;
+};
+
+&tps65090_fet4 {
+	regulator-name = "sdcard";
+	ti,overcurrent-wait = <3>;
+};
+
+&tps65090_fet5 {
+	regulator-name = "camout";
+	regulator-always-on;
+	ti,overcurrent-wait = <3>;
+};
+
+&tps65090_fet6 {
+	regulator-name = "lcd_vdd";
+	ti,overcurrent-wait = <3>;
+};
+
+&tps65090_fet7 {
+	regulator-name = "video_mid_1a";
+	regulator-always-on;
+	ti,overcurrent-wait = <3>;
+};
+
 &i2c_0 {
 	max77686@09 {
 		compatible = "maxim,max77686";
-- 
2.0.0.rc2

[PATCH 3/6] ARM: dts: Create cros-tps65090 fragment

[Javier Martinez Canillas]

The tps65090 PMU is a component used in many ChromeOS devices
so instead of having the same device tree definitions in many
files, create a .dtsi fragment that can be included in DTS.

This fragment is based on the DT definitions for Peach boards.

Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
 arch/arm/boot/dts/cros-tps65090.dtsi | 81 ++++++++++++++++++++++++++++++++++++
 1 file changed, 81 insertions(+)
 create mode 100644 arch/arm/boot/dts/cros-tps65090.dtsi

diff --git a/arch/arm/boot/dts/cros-tps65090.dtsi b/arch/arm/boot/dts/cros-tps65090.dtsi
new file mode 100644
index 0000000..99df15e
--- /dev/null
+++ b/arch/arm/boot/dts/cros-tps65090.dtsi
@@ -0,0 +1,81 @@
+/*
+ * Copyright (C) 2014 Google, Inc
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+/*
+ * Common file for tps65090 PMU on Chromebooks
+ */
+/ {
+	vbat: fixed-regulator {
+		compatible = "regulator-fixed";
+		regulator-name = "vbat-supply";
+		regulator-boot-on;
+		regulator-always-on;
+	};
+};
+
+&tps {
+	/*
+	 * Config irq to disable internal pulls
+	 * even though we run in polling mode.
+	 */
+	pinctrl-names = "default";
+	pinctrl-0 = <&tps65090_irq>;
+
+	vsys1-supply = <&vbat>;
+	vsys2-supply = <&vbat>;
+	vsys3-supply = <&vbat>;
+	infet1-supply = <&vbat>;
+	infet2-supply = <&vbat>;
+	infet3-supply = <&vbat>;
+	infet4-supply = <&vbat>;
+	infet5-supply = <&vbat>;
+	infet6-supply = <&vbat>;
+	infet7-supply = <&vbat>;
+	vsys-l1-supply = <&vbat>;
+	vsys-l2-supply = <&vbat>;
+};
+
+&tps65090_dcdc1 {
+	ti,enable-ext-control;
+};
+
+&tps65090_dcdc2 {
+	ti,enable-ext-control;
+};
+
+&tps65090_dcdc3 {
+	ti,enable-ext-control;
+};
+
+&tps65090_fet1 {
+	regulator-name = "vcd_led";
+};
+
+&tps65090_fet2 {
+	regulator-name = "video_mid";
+};
+
+&tps65090_fet3 {
+	regulator-name = "wwan_r";
+};
+
+&tps65090_fet4 {
+	regulator-name = "sdcard";
+};
+
+&tps65090_fet5 {
+	regulator-name = "camout";
+};
+
+&tps65090_fet6 {
+	regulator-name = "lcd_vdd";
+};
+
+&tps65090_fet7 {
+	regulator-name = "video_mid_1a";
+};
-- 
2.0.0.rc2

[PATCH 4/6] ARM: dts: Use cros-tps65090 fragment in Peach boards

[Javier Martinez Canillas]

Peach Pit and Pi machines have the same regulators connection
and regulator name so the cros-tps65090 dtsi file can be used
to remove duplicated code.

Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
 arch/arm/boot/dts/exynos5420-peach-pit.dts | 95 +++++++-----------------------
 arch/arm/boot/dts/exynos5800-peach-pi.dts  | 94 +++++++----------------------
 2 files changed, 41 insertions(+), 148 deletions(-)

diff --git a/arch/arm/boot/dts/exynos5420-peach-pit.dts b/arch/arm/boot/dts/exynos5420-peach-pit.dts
index 228a6b1..a1a4410 100644
--- a/arch/arm/boot/dts/exynos5420-peach-pit.dts
+++ b/arch/arm/boot/dts/exynos5420-peach-pit.dts
@@ -93,13 +93,6 @@
 		pinctrl-0 = <&usb301_vbus_en>;
 		enable-active-high;
 	};
-
-	vbat: fixed-regulator {
-		compatible = "regulator-fixed";
-		regulator-name = "vbat-supply";
-		regulator-boot-on;
-		regulator-always-on;
-	};
 };
 
 &dp {
@@ -350,79 +343,33 @@
 				sbs,i2c-retry-count = <2>;
 			};
 
-			power-regulator@48 {
-				compatible = "ti,tps65090";
+			tps: power-regulator@48 {
 				reg = <0x48>;
-
-				/*
-				 * Config irq to disable internal pulls
-				 * even though we run in polling mode.
-				 */
-				pinctrl-names = "default";
-				pinctrl-0 = <&tps65090_irq>;
-
-				vsys1-supply = <&vbat>;
-				vsys2-supply = <&vbat>;
-				vsys3-supply = <&vbat>;
-				infet1-supply = <&vbat>;
-				infet2-supply = <&vbat>;
-				infet3-supply = <&vbat>;
-				infet4-supply = <&vbat>;
-				infet5-supply = <&vbat>;
-				infet6-supply = <&vbat>;
-				infet7-supply = <&vbat>;
-				vsys-l1-supply = <&vbat>;
-				vsys-l2-supply = <&vbat>;
-
-				regulators {
-					tps65090_dcdc1: dcdc1 {
-						ti,enable-ext-control;
-					};
-					tps65090_dcdc2: dcdc2 {
-						ti,enable-ext-control;
-					};
-					tps65090_dcdc3: dcdc3 {
-						ti,enable-ext-control;
-					};
-					tps65090_fet1: fet1 {
-						regulator-name = "vcd_led";
-					};
-					tps65090_fet2: fet2 {
-						regulator-name = "video_mid";
-						regulator-always-on;
-					};
-					tps65090_fet3: fet3 {
-						regulator-name = "wwan_r";
-						regulator-always-on;
-					};
-					tps65090_fet4: fet4 {
-						regulator-name = "sdcard";
-						regulator-always-on;
-					};
-					tps65090_fet5: fet5 {
-						regulator-name = "camout";
-					};
-					tps65090_fet6: fet6 {
-						regulator-name = "lcd_vdd";
-					};
-					tps65090_fet7: fet7 {
-						regulator-name = "video_mid_1a";
-						regulator-always-on;
-					};
-					tps65090_ldo1: ldo1 {
-					};
-					tps65090_ldo2: ldo2 {
-					};
-				};
-
-				charger {
-					compatible = "ti,tps65090-charger";
-				};
 			};
 		};
 	};
 };
 
+#include "tps65090.dtsi"
+#include "cros-tps65090.dtsi"
+
+
+&tps65090_fet2 {
+	regulator-always-on;
+};
+
+&tps65090_fet3 {
+	regulator-always-on;
+};
+
+&tps65090_fet4 {
+	regulator-always-on;
+};
+
+&tps65090_fet7 {
+	regulator-always-on;
+};
+
 &uart_3 {
 	status = "okay";
 };
diff --git a/arch/arm/boot/dts/exynos5800-peach-pi.dts b/arch/arm/boot/dts/exynos5800-peach-pi.dts
index f3ee48b..6760839 100644
--- a/arch/arm/boot/dts/exynos5800-peach-pi.dts
+++ b/arch/arm/boot/dts/exynos5800-peach-pi.dts
@@ -91,13 +91,6 @@
 		pinctrl-0 = <&usb301_vbus_en>;
 		enable-active-high;
 	};
-
-	vbat: fixed-regulator {
-		compatible = "regulator-fixed";
-		regulator-name = "vbat-supply";
-		regulator-boot-on;
-		regulator-always-on;
-	};
 };
 
 &dp {
@@ -348,79 +341,32 @@
 				sbs,i2c-retry-count = <2>;
 			};
 
-			power-regulator@48 {
-				compatible = "ti,tps65090";
+			tps: power-regulator@48 {
 				reg = <0x48>;
-
-				/*
-				 * Config irq to disable internal pulls
-				 * even though we run in polling mode.
-				 */
-				pinctrl-names = "default";
-				pinctrl-0 = <&tps65090_irq>;
-
-				vsys1-supply = <&vbat>;
-				vsys2-supply = <&vbat>;
-				vsys3-supply = <&vbat>;
-				infet1-supply = <&vbat>;
-				infet2-supply = <&vbat>;
-				infet3-supply = <&vbat>;
-				infet4-supply = <&vbat>;
-				infet5-supply = <&vbat>;
-				infet6-supply = <&vbat>;
-				infet7-supply = <&vbat>;
-				vsys-l1-supply = <&vbat>;
-				vsys-l2-supply = <&vbat>;
-
-				regulators {
-					tps65090_dcdc1: dcdc1 {
-						ti,enable-ext-control;
-					};
-					tps65090_dcdc2: dcdc2 {
-						ti,enable-ext-control;
-					};
-					tps65090_dcdc3: dcdc3 {
-						ti,enable-ext-control;
-					};
-					tps65090_fet1: fet1 {
-						regulator-name = "vcd_led";
-					};
-					tps65090_fet2: fet2 {
-						regulator-name = "video_mid";
-						regulator-always-on;
-					};
-					tps65090_fet3: fet3 {
-						regulator-name = "wwan_r";
-						regulator-always-on;
-					};
-					tps65090_fet4: fet4 {
-						regulator-name = "sdcard";
-						regulator-always-on;
-					};
-					tps65090_fet5: fet5 {
-						regulator-name = "camout";
-					};
-					tps65090_fet6: fet6 {
-						regulator-name = "lcd_vdd";
-					};
-					tps65090_fet7: fet7 {
-						regulator-name = "video_mid_1a";
-						regulator-always-on;
-					};
-					tps65090_ldo1: ldo1 {
-					};
-					tps65090_ldo2: ldo2 {
-					};
-				};
-
-				charger {
-					compatible = "ti,tps65090-charger";
-				};
 			};
 		};
 	};
 };
 
+#include "tps65090.dtsi"
+#include "cros-tps65090.dtsi"
+
+&tps65090_fet2 {
+	regulator-always-on;
+};
+
+&tps65090_fet3 {
+	regulator-always-on;
+};
+
+&tps65090_fet4 {
+	regulator-always-on;
+};
+
+&tps65090_fet7 {
+	regulator-always-on;
+};
+
 &uart_3 {
 	status = "okay";
 };
-- 
2.0.0.rc2

[PATCH 5/6] ARM: dts: Improve cros-tps65090 power scheme

[Javier Martinez Canillas]

The DeviceTree files for the Peach Pit and Pi machines have
a simplistic model of the connections between the different
regulators since not all the tps65090 regulators get their
input supply voltage from the VDC. DCDC1-3, LD0-1 and fet7
parent supply is indded VDC but the fet1-6 get their input
supply from the DCDC1 and DCDC2 output voltage rails.

Update the DeviceTree to better reflect the real connections
between tps65090 regulators. Having this information in the
DTS is useful since FETs are switches that don't provide an
output voltage so the regulator core needs to fetch the FET
parent output voltage if the child voltage is queried.

Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
 arch/arm/boot/dts/cros-tps65090.dtsi | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/arm/boot/dts/cros-tps65090.dtsi b/arch/arm/boot/dts/cros-tps65090.dtsi
index 99df15e..14418b0 100644
--- a/arch/arm/boot/dts/cros-tps65090.dtsi
+++ b/arch/arm/boot/dts/cros-tps65090.dtsi
@@ -30,12 +30,12 @@
 	vsys2-supply = <&vbat>;
 	vsys3-supply = <&vbat>;
 	infet1-supply = <&vbat>;
-	infet2-supply = <&vbat>;
-	infet3-supply = <&vbat>;
-	infet4-supply = <&vbat>;
-	infet5-supply = <&vbat>;
-	infet6-supply = <&vbat>;
-	infet7-supply = <&vbat>;
+	infet2-supply = <&tps65090_dcdc1>;
+	infet3-supply = <&tps65090_dcdc2>;
+	infet4-supply = <&tps65090_dcdc2>;
+	infet5-supply = <&tps65090_dcdc2>;
+	infet6-supply = <&tps65090_dcdc2>;
+	infet7-supply = <&tps65090_dcdc1>;
 	vsys-l1-supply = <&vbat>;
 	vsys-l2-supply = <&vbat>;
 };
-- 
2.0.0.rc2

[PATCH 6/6] ARM: dts: Add tps65090 FETs constraints

[Javier Martinez Canillas]

The tps65090 PMU data manual [0] has a table that list the
"Recommended operating conditions" for each regulator. Add
the information about the FET constraints to its dtsi file.

[0]: http://www.ti.com/lit/ds/symlink/tps65090.pdf

Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
 arch/arm/boot/dts/tps65090.dtsi | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/arch/arm/boot/dts/tps65090.dtsi b/arch/arm/boot/dts/tps65090.dtsi
index a2ff534..60c56da 100644
--- a/arch/arm/boot/dts/tps65090.dtsi
+++ b/arch/arm/boot/dts/tps65090.dtsi
@@ -24,30 +24,48 @@
 		};
 
 		tps65090_fet1: fet1 {
+			regulator-min-microvolt = <5000000>;
+			regulator-max-microvolt = <17000000>;
 		};
 
 		tps65090_fet2: fet2 {
+			regulator-min-microvolt = <4500000>;
+			regulator-max-microvolt = <5500000>;
 		};
 
 		tps65090_fet3: fet3 {
+			regulator-min-microvolt = <3000000>;
+			regulator-max-microvolt = <5500000>;
 		};
 
 		tps65090_fet4: fet4 {
+			regulator-min-microvolt = <3000000>;
+			regulator-max-microvolt = <5500000>;
 		};
 
 		tps65090_fet5: fet5 {
+			regulator-min-microvolt = <3000000>;
+			regulator-max-microvolt = <5500000>;
 		};
 
 		tps65090_fet6: fet6 {
+			regulator-min-microvolt = <3000000>;
+			regulator-max-microvolt = <5500000>;
 		};
 
 		tps65090_fet7: fet7 {
+			regulator-min-microvolt = <3000000>;
+			regulator-max-microvolt = <5500000>;
 		};
 
 		tps65090_ldo1: ldo1 {
+			regulator-min-microvolt = <6000000>;
+			regulator-max-microvolt = <17000000>;
 		};
 
 		tps65090_ldo2: ldo2 {
+			regulator-min-microvolt = <6000000>;
+			regulator-max-microvolt = <17000000>;
 		};
 	};
 
-- 
2.0.0.rc2

Re: [PATCH 1/6] ARM: dts: Create fragment for tps65090 PMU

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 478 bytes --]

On Tue, Aug 12, 2014 at 06:44:23PM +0200, Javier Martinez Canillas wrote:
> The tps65090 is a Power Management Unit (PMU) used in several
> boards so the same information is described on different DTS.
> It is better to create a .dtsi fragment that can be included.

Why is it better to do this?

> +	regulators {
> +		tps65090_dcdc1: dcdc1 {
> +		};
> +

It appears to be largely content free, exactly the same effect should be
achieved by removing the entire regulators node.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/6] ARM: dts: Create fragment for tps65090 PMU

[Javier Martinez Canillas]

Hello Mark,

On 08/12/2014 06:58 PM, Mark Brown wrote:
> On Tue, Aug 12, 2014 at 06:44:23PM +0200, Javier Martinez Canillas wrote:
>> The tps65090 is a Power Management Unit (PMU) used in several
>> boards so the same information is described on different DTS.
>> It is better to create a .dtsi fragment that can be included.
> 
> Why is it better to do this?
>

Is better IMHO because we have a single place where the tps65090 information can
be updated instead of duplicating the same definition on each DTS.

This appears to be the current trend to better manage shared DTS snippet across
different boards. Others examples are arch/arm/boot/dts/omap-gpmc-smsc911x.dtsi
and arch/arm/boot/dts/twl6030.dtsi.

>> +	regulators {
>> +		tps65090_dcdc1: dcdc1 {
>> +		};
>> +
> 
> It appears to be largely content free, exactly the same effect should be
> achieved by removing the entire regulators node.
> 

Yes it's content free but later "[PATCH 6/6] ARM: dts: Add tps65090 FETs
constraints" [0] fills the FETs constraints [0]. This is a preparatory patch.

Also, having all the regulators allows DTS files to reference the node by the
label if they want to add other properties. I can squash this patch and 06/06 if
you think that is better but I thought that the split makes it easier to review.

Best regards,
Javier

[0]: https://lkml.org/lkml/2014/8/12/377

Re: [PATCH 6/6] ARM: dts: Add tps65090 FETs constraints

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 702 bytes --]

On Tue, Aug 12, 2014 at 06:44:28PM +0200, Javier Martinez Canillas wrote:
> The tps65090 PMU data manual [0] has a table that list the
> "Recommended operating conditions" for each regulator. Add
> the information about the FET constraints to its dtsi file.

>  		tps65090_fet1: fet1 {
> +			regulator-min-microvolt = <5000000>;
> +			regulator-max-microvolt = <17000000>;
>  		};

No, this is completely broken and exactly the sort of thing that makes
doing generic device .dtsi a bad idea.  We have absolutely no idea if
these voltage ranges are suitable for use on a given board using the
device, these are the design limits for the device but tell us nothing
about the system they are deployed in.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 3/6] ARM: dts: Create cros-tps65090 fragment

[Doug Anderson]

Javier,

On Tue, Aug 12, 2014 at 9:44 AM, Javier Martinez Canillas
<javier.martinez@collabora.co.uk> wrote:
> The tps65090 PMU is a component used in many ChromeOS devices
> so instead of having the same device tree definitions in many
> files, create a .dtsi fragment that can be included in DTS.
>
> This fragment is based on the DT definitions for Peach boards.
>
> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> ---
>  arch/arm/boot/dts/cros-tps65090.dtsi | 81 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 81 insertions(+)
>  create mode 100644 arch/arm/boot/dts/cros-tps65090.dtsi

I'd probably skip this patch (just have duplication in pit vs. pi), or
make it "peach" specific (like exynos-peach-tps65090.dtsi?).  This is
really board-specific info and trying to guess what the various FETs
are going to be for for all peach variants doesn't seem great.

-Doug

Re: [PATCH 1/6] ARM: dts: Create fragment for tps65090 PMU

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1897 bytes --]

On Tue, Aug 12, 2014 at 07:21:35PM +0200, Javier Martinez Canillas wrote:
> On 08/12/2014 06:58 PM, Mark Brown wrote:
> > On Tue, Aug 12, 2014 at 06:44:23PM +0200, Javier Martinez Canillas wrote:

> >> The tps65090 is a Power Management Unit (PMU) used in several
> >> boards so the same information is described on different DTS.
> >> It is better to create a .dtsi fragment that can be included.

> > Why is it better to do this?

> Is better IMHO because we have a single place where the tps65090 information can
> be updated instead of duplicating the same definition on each DTS.

But there is no real information in this file.

> This appears to be the current trend to better manage shared DTS snippet across
> different boards. Others examples are arch/arm/boot/dts/omap-gpmc-smsc911x.dtsi
> and arch/arm/boot/dts/twl6030.dtsi.

In the smsc911x case that's a block from a reference design that's
commonly repeated over multiple systems and is therefore similar to the
reference design elements that have been factored out for Chromebooks.

The twl6030 fragment is just broken - the regulator section is actively
harmful and should be removed.

> 
> >> +	regulators {
> >> +		tps65090_dcdc1: dcdc1 {
> >> +		};
> >> +
> > 
> > It appears to be largely content free, exactly the same effect should be
> > achieved by removing the entire regulators node.
> > 
> 
> Yes it's content free but later "[PATCH 6/6] ARM: dts: Add tps65090 FETs
> constraints" [0] fills the FETs constraints [0]. This is a preparatory patch.
> 
> Also, having all the regulators allows DTS files to reference the node by the
> label if they want to add other properties. I can squash this patch and 06/06 if
> you think that is better but I thought that the split makes it easier to review.
> 
> Best regards,
> Javier
> 
> [0]: https://lkml.org/lkml/2014/8/12/377
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 6/6] ARM: dts: Add tps65090 FETs constraints

[Javier Martinez Canillas]

On 08/12/2014 07:25 PM, Mark Brown wrote:
> 
>>  		tps65090_fet1: fet1 {
>> +			regulator-min-microvolt = <5000000>;
>> +			regulator-max-microvolt = <17000000>;
>>  		};
> 
> No, this is completely broken and exactly the sort of thing that makes
> doing generic device .dtsi a bad idea.  We have absolutely no idea if
> these voltage ranges are suitable for use on a given board using the
> device, these are the design limits for the device but tell us nothing
> about the system they are deployed in.
> 

Thanks for the explanation. I did the refactoring because I saw that there are
.dtsi files for other PMICs already (twl4030, twl6030, tps65*) but now you also
explained that those are broken as well.

So, is adding these voltages ranges (the design limits) in the Peach Pit DTS
file directly an acceptable solution? Basically what my previous patch [0] did.

That matches what is in the board schematic so I assume that it's safe to use
these voltage ranges for that machine. If so I'll drop this series and repost
that patch fixing the typo error and commit message pointed by Doug that were
already addressed in $subject.

Best regards,
Javier

[0]: https://lkml.org/lkml/2014/8/11/204

Re: [PATCH 3/6] ARM: dts: Create cros-tps65090 fragment

[Javier Martinez Canillas]

Hello Doug,

Thanks for your feedback.

On 08/12/2014 07:26 PM, Doug Anderson wrote:
> Javier,
> 
> On Tue, Aug 12, 2014 at 9:44 AM, Javier Martinez Canillas
> <javier.martinez@collabora.co.uk> wrote:
>> The tps65090 PMU is a component used in many ChromeOS devices
>> so instead of having the same device tree definitions in many
>> files, create a .dtsi fragment that can be included in DTS.
>>
>> This fragment is based on the DT definitions for Peach boards.
>>
>> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
>> ---
>>  arch/arm/boot/dts/cros-tps65090.dtsi | 81 ++++++++++++++++++++++++++++++++++++
>>  1 file changed, 81 insertions(+)
>>  create mode 100644 arch/arm/boot/dts/cros-tps65090.dtsi
> 
> I'd probably skip this patch (just have duplication in pit vs. pi), or
> make it "peach" specific (like exynos-peach-tps65090.dtsi?).  This is
> really board-specific info and trying to guess what the various FETs
> are going to be for for all peach variants doesn't seem great.
>

Yes, I guess I went to far on the refactoring with this patch. Most likely I'll
drop the whole series anyway and just add the voltage constraints and parent
supplies to the board DTS, now that Mark explained to me that this refactoring
is actually doing more harm than good.

> -Doug
> 

Best regards,
Javier

Re: [PATCH 6/6] ARM: dts: Add tps65090 FETs constraints

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 978 bytes --]

On Tue, Aug 12, 2014 at 08:49:29PM +0200, Javier Martinez Canillas wrote:

> So, is adding these voltages ranges (the design limits) in the Peach Pit DTS
> file directly an acceptable solution? Basically what my previous patch [0] did.

> That matches what is in the board schematic so I assume that it's safe to use
> these voltage ranges for that machine. If so I'll drop this series and repost
> that patch fixing the typo error and commit message pointed by Doug that were
> already addressed in $subject.

Well, I think the question is if you understand where those numbers come
from and if they make sense.  I've not seen the schematic for the board
so I can't comment but it is quite unusual to see ranges listed for so
many supplies, for things like SD it's obviously normal but things like
video_mid are a bit more surprising.  Does anything actually vary those
voltages?

The change where you fix up the supply mappings still makes sense of
course.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 6/6] ARM: dts: Add tps65090 FETs constraints

[Javier Martinez Canillas]

Hello Mark,

On 08/12/2014 11:27 PM, Mark Brown wrote:
> On Tue, Aug 12, 2014 at 08:49:29PM +0200, Javier Martinez Canillas wrote:
> 
>> So, is adding these voltages ranges (the design limits) in the Peach Pit DTS
>> file directly an acceptable solution? Basically what my previous patch [0] did.
> 
>> That matches what is in the board schematic so I assume that it's safe to use
>> these voltage ranges for that machine. If so I'll drop this series and repost
>> that patch fixing the typo error and commit message pointed by Doug that were
>> already addressed in $subject.
> 
> Well, I think the question is if you understand where those numbers come
> from and if they make sense.  I've not seen the schematic for the board
> so I can't comment but it is quite unusual to see ranges listed for so
> many supplies, for things like SD it's obviously normal but things like
> video_mid are a bit more surprising.  Does anything actually vary those
> voltages?
> 

You are right, in fact even the voltage for the SD supply (tps65090 fet4) does
not change but still their constraints have to be defined, since it is used as a
vmmc-supply and the series "Adding UHS support for dw_mmc driver" [0] calls
mmc_regulator_get_ocrmask()/mmc_regulator_set_ocr() for mmc->supply.vmmc.

For fixed regulators (like fet4), mmc_regulator_set_ocr() just skips varying the
regulator voltage but still expects to be able to obtain its voltage.

Since the FET is just a switch and doesn't have an output voltage, the regulator
core gets its parent supply voltage but regulator_list_voltge() [1] checks that
the obtained parent voltage is between the child regulator constraints.

So in this particular case it makes sense to compare that the parent voltage is
between the (design limits) voltage ranges for the FET but I agree that it may
not make sense on other boards.

Since I needed to add the ranges for fet4 I (wrongly) thought that it would make
sense to add the ranges for all the other FETs in case other boards had a
similar need. But now I understand your concerns about this series so I'll drop
it and just post a patch that adds to the Peach Pit and Pi DTS, the constraints
for the fet4 used as vmmc-supply instead of pretending that it's a common setup.

> The change where you fix up the supply mappings still makes sense of
> course.
>

Good to know, again thanks a lot for your feedback and explanations.

Best regards,
Javier

[0]: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/265657.html
[1]:
https://git.kernel.org/cgit/linux/kernel/git/broonie/regulator.git/tree/drivers/regulator/core.c?h=for-next#n2209

Re: [PATCH 6/6] ARM: dts: Add tps65090 FETs constraints

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1594 bytes --]

On Wed, Aug 13, 2014 at 01:31:44PM +0200, Javier Martinez Canillas wrote:

Please fix your mailer to word wrap at less than 80 columns, it makes
your mails very hard to read when replying.

> On 08/12/2014 11:27 PM, Mark Brown wrote:

> > Well, I think the question is if you understand where those numbers come
> > from and if they make sense.  I've not seen the schematic for the board
> > so I can't comment but it is quite unusual to see ranges listed for so
> > many supplies, for things like SD it's obviously normal but things like
> > video_mid are a bit more surprising.  Does anything actually vary those
> > voltages?

> You are right, in fact even the voltage for the SD supply (tps65090 fet4) does
> not change but still their constraints have to be defined, since it is used as a
> vmmc-supply and the series "Adding UHS support for dw_mmc driver" [0] calls
> mmc_regulator_get_ocrmask()/mmc_regulator_set_ocr() for mmc->supply.vmmc.

No, that makes no sense.  If the voltage isn't allowed to change why
should the constraints be specifying a range of voltages for it to be
set to, especially a range with more than one value in it?

Please try to think about what you're doing in design terms rather than
just bashing on things until you get something that works in your use
case, it's important that things are understandable so that we avoid
fragility and special casing.

> For fixed regulators (like fet4), mmc_regulator_set_ocr() just skips varying the
> regulator voltage but still expects to be able to obtain its voltage.

Which can be done with regulator_get_voltage().

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 6/6] ARM: dts: Add tps65090 FETs constraints

[Javier Martinez Canillas]

On 08/13/2014 02:29 PM, Mark Brown wrote:
> 
> Please fix your mailer to word wrap at less than 80 columns, it makes
> your mails very hard to read when replying.
> 

Sorry I had my wrap length configured to 80 but just changed to 74 now.

>> On 08/12/2014 11:27 PM, Mark Brown wrote:
> 
> No, that makes no sense.  If the voltage isn't allowed to change why
> should the constraints be specifying a range of voltages for it to be
> set to, especially a range with more than one value in it?
> 
> Please try to think about what you're doing in design terms rather than
> just bashing on things until you get something that works in your use
> case, it's important that things are understandable so that we avoid
> fragility and special casing.
> 
>> For fixed regulators (like fet4), mmc_regulator_set_ocr() just skips >> varying the regulator voltage but still expects to be able to obtain
>> its voltage.
> 
> Which can be done with regulator_get_voltage().
> 

Indeed. I'll change mmc_regulator_get_ocrmask() in MMC core then to use
regulator_can_change_voltage() to detect if the regulator is a fixed one
and call regulator_get_voltage() instead of list_voltage() in that case.

Do you agree that this is the correct solution?

Best regards,
Javier

Re: [PATCH 6/6] ARM: dts: Add tps65090 FETs constraints

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 448 bytes --]

On Wed, Aug 13, 2014 at 03:34:12PM +0200, Javier Martinez Canillas wrote:

> Indeed. I'll change mmc_regulator_get_ocrmask() in MMC core then to use
> regulator_can_change_voltage() to detect if the regulator is a fixed one
> and call regulator_get_voltage() instead of list_voltage() in that case.

> Do you agree that this is the correct solution?

I would just fall back to get_voltage() if there are no voltages listed,
that seems more robust.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/6] ARM: dts: Create fragment for tps65090 PMU

[Stephen Warren]

On 08/12/2014 10:44 AM, Javier Martinez Canillas wrote:
> The tps65090 is a Power Management Unit (PMU) used in several
> boards so the same information is described on different DTS.
> It is better to create a .dtsi fragment that can be included.

To be honest, I'm not sure that this file is useful. The entire content 
of this file (with the exception of the compatible value and possibly 
the charger sub-node) needs to be repeated in any file that includes it, 
in order to add properties into all the nodes.

Re: [PATCH 6/6] ARM: dts: Add tps65090 FETs constraints

[Stephen Warren]

On 08/12/2014 10:44 AM, Javier Martinez Canillas wrote:
> The tps65090 PMU data manual [0] has a table that list the
> "Recommended operating conditions" for each regulator. Add
> the information about the FET constraints to its dtsi file.
>
> [0]: http://www.ti.com/lit/ds/symlink/tps65090.pdf

I'm worried that this file represents the limits of the PMIC itself, 
whereas the DT should be representing the limits of the circuits that 
the various PMIC regulators are attached to on the board.

For example:

> diff --git a/arch/arm/boot/dts/tps65090.dtsi b/arch/arm/boot/dts/tps65090.dtsi

>   		tps65090_fet3: fet3 {
> +			regulator-min-microvolt = <3000000>;
> +			regulator-max-microvolt = <5500000>;
>   		};

I guess that on some boards, this output rail might be attached to 
devices that must run at 3.3V exactly, and on other boards it might be 
attached to devices that must run at 5V exactly. The DT for those two 
boards should each have regulator-{min,max}-microvolt set to the same 
value, which describes the board requirements.

It feels dangerous/misleading to define the PMIC range by default. It 
might lead people to think that since the property already has a defined 
value, they don't need to think about what the correct value for their 
board is, and hence not change the value in their board file.

Re: [PATCH 6/6] ARM: dts: Add tps65090 FETs constraints

[Javier Martinez Canillas]

Hello Mark,

On 08/13/2014 05:51 PM, Mark Brown wrote:
> On Wed, Aug 13, 2014 at 03:34:12PM +0200, Javier Martinez Canillas wrote:
> 
>> Indeed. I'll change mmc_regulator_get_ocrmask() in MMC core then to use
>> regulator_can_change_voltage() to detect if the regulator is a fixed one
>> and call regulator_get_voltage() instead of list_voltage() in that case.
> 
>> Do you agree that this is the correct solution?
> 
> I would just fall back to get_voltage() if there are no voltages listed,
> that seems more robust.
> 

Great, thanks a lot for your suggestion and all the help.

Best regards,
Javier

Re: [PATCH 6/6] ARM: dts: Add tps65090 FETs constraints

[Javier Martinez Canillas]

Hello Stephen,

On 08/13/2014 06:16 PM, Stephen Warren wrote:
> 
> I'm worried that this file represents the limits of the PMIC itself, 
> whereas the DT should be representing the limits of the circuits that 
> the various PMIC regulators are attached to on the board.
> 
> For example:
> 
>> diff --git a/arch/arm/boot/dts/tps65090.dtsi b/arch/arm/boot/dts/tps65090.dtsi
> 
>>   		tps65090_fet3: fet3 {
>> +			regulator-min-microvolt = <3000000>;
>> +			regulator-max-microvolt = <5500000>;
>>   		};
> 
> I guess that on some boards, this output rail might be attached to 
> devices that must run at 3.3V exactly, and on other boards it might be 
> attached to devices that must run at 5V exactly. The DT for those two 
> boards should each have regulator-{min,max}-microvolt set to the same 
> value, which describes the board requirements.
> 
> It feels dangerous/misleading to define the PMIC range by default. It 
> might lead people to think that since the property already has a defined 
> value, they don't need to think about what the correct value for their 
> board is, and hence not change the value in their board file.
> 

Yes, Mark already explained to me why this approach was broken so I've
dropped the whole series. Thanks a lot for your feedback.

Best regards,
Javier