[PATCH] ARM: tegra: Overhaul Venice2 regulators

[PATCH] ARM: tegra: Overhaul Venice2 regulators

[Thierry Reding]

From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Some of the regulators and the relationships to other regulators are
wrong. This commit attempts to rectify this by making them more similar
to what the schematics contain. This starts by adding a +VDD_MUX supply
that represents the 12V input and derives the main +3.3V_SYS and +5V_SYS
supplies from that. The majority of the other regulators derive from one
of those three.

While at it, rename the regulators to match the names in the schematics
to make them easier to match up.

Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/boot/dts/tegra124-venice2.dts | 137 ++++++++++++++++++---------------
 1 file changed, 73 insertions(+), 64 deletions(-)

diff --git a/arch/arm/boot/dts/tegra124-venice2.dts b/arch/arm/boot/dts/tegra124-venice2.dts
index c950c76185d2..ed2cbf5b5999 100644
--- a/arch/arm/boot/dts/tegra124-venice2.dts
+++ b/arch/arm/boot/dts/tegra124-venice2.dts
@@ -644,19 +644,19 @@
 			};
 
 			regulators {
-				vsup-sd2-supply = <&vdd_ac_bat_reg>;
-				vsup-sd3-supply = <&vdd_ac_bat_reg>;
-				vsup-sd4-supply = <&vdd_ac_bat_reg>;
-				vsup-sd5-supply = <&vdd_ac_bat_reg>;
+				vsup-sd2-supply = <&vdd_5v0_sys>;
+				vsup-sd3-supply = <&vdd_5v0_sys>;
+				vsup-sd4-supply = <&vdd_5v0_sys>;
+				vsup-sd5-supply = <&vdd_5v0_sys>;
 				vin-ldo0-supply = <&as3722_sd2>;
-				vin-ldo1-6-supply = <&vdd_ac_bat_reg>;
+				vin-ldo1-6-supply = <&vdd_3v3_run>;
 				vin-ldo2-5-7-supply = <&as3722_sd5>;
-				vin-ldo3-4-supply = <&vdd_ac_bat_reg>;
-				vin-ldo9-10-supply = <&vdd_ac_bat_reg>;
-				vin-ldo11-supply = <&vdd_ac_bat_reg>;
+				vin-ldo3-4-supply = <&vdd_3v3_sys>;
+				vin-ldo9-10-supply = <&vdd_5v0_sys>;
+				vin-ldo11-supply = <&vdd_3v3_run>;
 
 				sd0 {
-					regulator-name = "vdd-cpu";
+					regulator-name = "+VDD_CPU";
 					regulator-min-microvolt = <700000>;
 					regulator-max-microvolt = <1400000>;
 					regulator-min-microamp = <3500000>;
@@ -667,7 +667,7 @@
 				};
 
 				sd1 {
-					regulator-name = "vdd-core";
+					regulator-name = "+VDD_CORE";
 					regulator-min-microvolt = <700000>;
 					regulator-max-microvolt = <1350000>;
 					regulator-min-microamp = <2500000>;
@@ -678,7 +678,7 @@
 				};
 
 				as3722_sd2: sd2 {
-					regulator-name = "vddio-ddr";
+					regulator-name = "+1.35V_LP0";
 					regulator-min-microvolt = <1350000>;
 					regulator-max-microvolt = <1350000>;
 					regulator-always-on;
@@ -686,7 +686,7 @@
 				};
 
 				sd3 {
-					regulator-name = "vddio-ddr-2phase";
+					regulator-name = "+1.35V_LP0";
 					regulator-min-microvolt = <1350000>;
 					regulator-max-microvolt = <1350000>;
 					regulator-always-on;
@@ -694,7 +694,7 @@
 				};
 
 				sd4 {
-					regulator-name = "avdd-pex-sata";
+					regulator-name = "+1.05V_RUN";
 					regulator-min-microvolt = <1050000>;
 					regulator-max-microvolt = <1050000>;
 					regulator-boot-on;
@@ -702,7 +702,7 @@
 				};
 
 				as3722_sd5: sd5 {
-					regulator-name = "vddio-sys";
+					regulator-name = "+1.8V_VDDIO";
 					regulator-min-microvolt = <1800000>;
 					regulator-max-microvolt = <1800000>;
 					regulator-boot-on;
@@ -710,7 +710,7 @@
 				};
 
 				sd6 {
-					regulator-name = "vdd-gpu";
+					regulator-name = "+VDD_GPU";
 					regulator-min-microvolt = <650000>;
 					regulator-max-microvolt = <1200000>;
 					regulator-min-microamp = <3500000>;
@@ -720,7 +720,7 @@
 				};
 
 				ldo0 {
-					regulator-name = "avdd_pll";
+					regulator-name = "+1.05V_RUN_AVDD";
 					regulator-min-microvolt = <1050000>;
 					regulator-max-microvolt = <1050000>;
 					regulator-boot-on;
@@ -729,13 +729,13 @@
 				};
 
 				ldo1 {
-					regulator-name = "run-cam-1.8";
+					regulator-name = "+1.8V_RUN_CAM";
 					regulator-min-microvolt = <1800000>;
 					regulator-max-microvolt = <1800000>;
 				};
 
 				ldo2 {
-					regulator-name = "gen-avdd,vddio-hsic";
+					regulator-name = "+1.2V_GEN_AVDD";
 					regulator-min-microvolt = <1200000>;
 					regulator-max-microvolt = <1200000>;
 					regulator-boot-on;
@@ -743,7 +743,7 @@
 				};
 
 				ldo3 {
-					regulator-name = "vdd-rtc";
+					regulator-name = "+1.00V_LP0_VDD_RTC";
 					regulator-min-microvolt = <1000000>;
 					regulator-max-microvolt = <1000000>;
 					regulator-boot-on;
@@ -752,21 +752,19 @@
 				};
 
 				ldo4 {
-					regulator-name = "vdd-cam";
+					regulator-name = "+3.3V_RUN_CAM";
 					regulator-min-microvolt = <2800000>;
 					regulator-max-microvolt = <2800000>;
-					regulator-boot-on;
-					regulator-always-on;
 				};
 
 				ldo5 {
-					regulator-name = "vdd-cam-front";
+					regulator-name = "+1.2V_RUN_CAM_FRONT";
 					regulator-min-microvolt = <1200000>;
 					regulator-max-microvolt = <1200000>;
 				};
 
 				ldo6 {
-					regulator-name = "vddio-sdmmc3";
+					regulator-name = "+VDDIO_SDMMC3";
 					regulator-min-microvolt = <1800000>;
 					regulator-max-microvolt = <3300000>;
 					regulator-boot-on;
@@ -774,25 +772,25 @@
 				};
 
 				ldo7 {
-					regulator-name = "vdd-cam-rear";
+					regulator-name = "+1.05V_RUN_CAM_REAR";
 					regulator-min-microvolt = <1050000>;
 					regulator-max-microvolt = <1050000>;
 				};
 
 				ldo9 {
-					regulator-name = "vdd-touch";
+					regulator-name = "+2.8V_RUN_TOUCH";
 					regulator-min-microvolt = <2800000>;
 					regulator-max-microvolt = <2800000>;
 				};
 
 				ldo10 {
-					regulator-name = "vdd-cam-af";
+					regulator-name = "+2.8V_RUN_CAM_AF";
 					regulator-min-microvolt = <2800000>;
 					regulator-max-microvolt = <2800000>;
 				};
 
 				ldo11 {
-					regulator-name = "vpp-fuse";
+					regulator-name = "+1.8V_RUN_VPP_FUSE";
 					regulator-min-microvolt = <1800000>;
 					regulator-max-microvolt = <1800000>;
 				};
@@ -975,99 +973,110 @@
 		#address-cells = <1>;
 		#size-cells = <0>;
 
-		vdd_ac_bat_reg: regulator@0 {
+		vdd_mux: regulator@0 {
 			compatible = "regulator-fixed";
 			reg = <0>;
-			regulator-name = "vdd_ac_bat";
-			regulator-min-microvolt = <5000000>;
-			regulator-max-microvolt = <5000000>;
+			regulator-name = "+VDD_MUX";
+			regulator-min-microvolt = <12000000>;
+			regulator-max-microvolt = <12000000>;
 			regulator-always-on;
+			regulator-boot-on;
 		};
 
-		vdd_3v3_reg: regulator@1 {
+		vdd_5v0_sys: regulator@1 {
 			compatible = "regulator-fixed";
 			reg = <1>;
-			regulator-name = "vdd_3v3";
-			regulator-min-microvolt = <3300000>;
-			regulator-max-microvolt = <3300000>;
+			regulator-name = "+5V_SYS";
+			regulator-min-microvolt = <5000000>;
+			regulator-max-microvolt = <5000000>;
 			regulator-always-on;
 			regulator-boot-on;
-			enable-active-high;
-			gpio = <&as3722 1 GPIO_ACTIVE_HIGH>;
+			vin-supply = <&vdd_mux>;
 		};
 
-		vdd_3v3_modem_reg: regulator@2 {
+		vdd_3v3_sys: regulator@2 {
 			compatible = "regulator-fixed";
 			reg = <2>;
-			regulator-name = "vdd-modem-3v3";
+			regulator-name = "+3.3V_SYS";
 			regulator-min-microvolt = <3300000>;
 			regulator-max-microvolt = <3300000>;
-			enable-active-high;
-			gpio = <&as3722 2 GPIO_ACTIVE_HIGH>;
+			regulator-always-on;
+			regulator-boot-on;
+			vin-supply = <&vdd_mux>;
 		};
 
-		vdd_hdmi_5v0_reg: regulator@3 {
+		vdd_3v3_run: regulator@3 {
 			compatible = "regulator-fixed";
 			reg = <3>;
-			regulator-name = "vdd-hdmi-5v0";
-			regulator-min-microvolt = <5000000>;
-			regulator-max-microvolt = <5000000>;
-			enable-active-high;
-			gpio = <&gpio TEGRA_GPIO(K, 6) GPIO_ACTIVE_HIGH>;
+			regulator-name = "+3.3V_RUN";
+			regulator-min-microvolt = <3300000>;
+			regulator-max-microvolt = <3300000>;
+			vin-supply = <&vdd_3v3_sys>;
 		};
 
-		vdd_bl_reg: regulator@4 {
+		vdd_3v3_hdmi: regulator@4 {
 			compatible = "regulator-fixed";
 			reg = <4>;
-			regulator-name = "vdd-bl";
+			regulator-name = "+3.3V_AVDD_HDMI";
 			regulator-min-microvolt = <3300000>;
 			regulator-max-microvolt = <3300000>;
-			gpio = <&gpio TEGRA_GPIO(P, 2) GPIO_ACTIVE_LOW>;
+			vin-supply = <&vdd_3v3_run>;
 		};
 
-		vdd_ts_sw_5v0: regulator@5 {
+		vdd_led: regulator@5 {
 			compatible = "regulator-fixed";
 			reg = <5>;
-			regulator-name = "vdd_ts_sw";
+			regulator-name = "+VDD_LED";
+			gpio = <&gpio TEGRA_GPIO(P, 2) GPIO_ACTIVE_HIGH>;
+			enable-active-high;
+			vin-supply = <&vdd_mux>;
+		};
+
+		vdd_5v0_ts: regulator@6 {
+			compatible = "regulator-fixed";
+			reg = <6>;
+			regulator-name = "+5V_VDD_TS_SW";
 			regulator-min-microvolt = <5000000>;
 			regulator-max-microvolt = <5000000>;
 			enable-active-high;
 			regulator-boot-on;
 			gpio = <&gpio TEGRA_GPIO(K, 1) GPIO_ACTIVE_LOW>;
+			vin-supply = <&vdd_5v0_sys>;
 		};
 
-		usb1_vbus_reg: regulator@6 {
+		vdd_usb1_vbus: regulator@7 {
 			compatible = "regulator-fixed";
-			reg = <6>;
-			regulator-name = "usb1_vbus";
+			reg = <7>;
+			regulator-name = "+5V_USB_HS";
 			regulator-min-microvolt = <5000000>;
 			regulator-max-microvolt = <5000000>;
-			regulator-boot-on;
 			enable-active-high;
 			gpio = <&gpio TEGRA_GPIO(N, 4) GPIO_ACTIVE_HIGH>;
 			gpio-open-drain;
+			vin-supply = <&vdd_5v0_sys>;
 		};
 
-		usb3_vbus_reg: regulator@7 {
+		vdd_usb3_vbus: regulator@8 {
 			compatible = "regulator-fixed";
-			reg = <7>;
-			regulator-name = "usb3_vbus";
+			reg = <8>;
+			regulator-name = "+5V_USB_SS";
 			regulator-min-microvolt = <5000000>;
 			regulator-max-microvolt = <5000000>;
-			regulator-boot-on;
 			enable-active-high;
 			gpio = <&gpio TEGRA_GPIO(N, 5) GPIO_ACTIVE_HIGH>;
 			gpio-open-drain;
+			vin-supply = <&vdd_5v0_sys>;
 		};
 
-		panel_3v3_reg: regulator@8 {
+		vdd_3v3_panel: regulator@9 {
 			compatible = "regulator-fixed";
-			reg = <8>;
-			regulator-name = "panel_3v3";
+			reg = <9>;
+			regulator-name = "+3.3V_PANEL";
 			regulator-min-microvolt = <3300000>;
 			regulator-max-microvolt = <3300000>;
 			enable-active-high;
 			gpio = <&as3722 4 GPIO_ACTIVE_HIGH>;
+			vin-supply = <&vdd_3v3_run>;
 		};
 	};
 
-- 
1.8.4.2

Re: [PATCH] ARM: tegra: Overhaul Venice2 regulators

[Stephen Warren]

On 02/25/2014 09:30 AM, Thierry Reding wrote:
> Some of the regulators and the relationships to other regulators are
> wrong. This commit attempts to rectify this by making them more similar
> to what the schematics contain. This starts by adding a +VDD_MUX supply
> that represents the 12V input and derives the main +3.3V_SYS and +5V_SYS
> supplies from that. The majority of the other regulators derive from one
> of those three.
> 
> While at it, rename the regulators to match the names in the schematics
> to make them easier to match up.

> diff --git a/arch/arm/boot/dts/tegra124-venice2.dts b/arch/arm/boot/dts/tegra124-venice2.dts

>  			regulators {
> -				vsup-sd2-supply = <&vdd_ac_bat_reg>;
> -				vsup-sd3-supply = <&vdd_ac_bat_reg>;
> -				vsup-sd4-supply = <&vdd_ac_bat_reg>;
> -				vsup-sd5-supply = <&vdd_ac_bat_reg>;
> +				vsup-sd2-supply = <&vdd_5v0_sys>;
> +				vsup-sd3-supply = <&vdd_5v0_sys>;
> +				vsup-sd4-supply = <&vdd_5v0_sys>;
> +				vsup-sd5-supply = <&vdd_5v0_sys>;
>  				vin-ldo0-supply = <&as3722_sd2>;
> -				vin-ldo1-6-supply = <&vdd_ac_bat_reg>;
> +				vin-ldo1-6-supply = <&vdd_3v3_run>;
>  				vin-ldo2-5-7-supply = <&as3722_sd5>;
> -				vin-ldo3-4-supply = <&vdd_ac_bat_reg>;
> -				vin-ldo9-10-supply = <&vdd_ac_bat_reg>;
> -				vin-ldo11-supply = <&vdd_ac_bat_reg>;
> +				vin-ldo3-4-supply = <&vdd_3v3_sys>;
> +				vin-ldo9-10-supply = <&vdd_5v0_sys>;
> +				vin-ldo11-supply = <&vdd_3v3_run>;

Looking at the schematic, the binding should require a vin-ldo3-lv and
vin-lod3_sw property too, although that's not really anything to do with
this change.

>  
>  				sd0 {
> -					regulator-name = "vdd-cpu";
> +					regulator-name = "+VDD_CPU";

I don't see a +VDD_CPU signal anywhere in the schematic
(602-7R371-0000-A00.Schematics.Rev.1.23). I do see +VDD_CPU_AP, but
that's driven by a pair of AS3728 ICs, not the AS3722. Perhaps the
AS3728 are considered logically part of the AS3722 since it seems like
control of the AS3728s is by wires from the AS3722?

>  
>  				as3722_sd2: sd2 {
> -					regulator-name = "vddio-ddr";
> +					regulator-name = "+1.35V_LP0";

can we rename this label to e.g. vdd_1v35_lp0, so that it's more obvious
that the bin-ldo0-supply property above references the correct node.

>  				sd3 {
> -					regulator-name = "vddio-ddr-2phase";
> +					regulator-name = "+1.35V_LP0";

Both sd2 and sd3 nodes have the same value for regulator-name. I'm not
sure that's correct, even if both the outputs are indeed wired together
on the board?


>  				as3722_sd5: sd5 {
> -					regulator-name = "vddio-sys";
> +					regulator-name = "+1.8V_VDDIO";

Rename the label vddio_1v8?

>  					regulator-min-microvolt = <1800000>;
>  					regulator-max-microvolt = <1800000>;
>  					regulator-boot-on;
> @@ -710,7 +710,7 @@
>  				};
>  
>  				sd6 {
> -					regulator-name = "vdd-gpu";
> +					regulator-name = "+VDD_GPU";

I think that's +VDD_GPU_AP on the schematics.

BTW, all the AS3728 chips (SD0, SD1, SD6) get +5V_SYS as input, as well
as one of +VDD_MUX_GPU, +VDD_MUX_CORE, +VDD_MUX_CPU. This doesn't seem
to be represented anywhere in the DT.

For the /regulators/regulators* nodes, I'll just quote the result of
applying this patch, rather than the diff itself, since there were so
many changes the diff is confusing.

> 		vdd_3v3_run: regulator@3 {
> 			compatible = "regulator-fixed";
> 			reg = <3>;
> 			regulator-name = "+3.3V_RUN";
> 			regulator-min-microvolt = <3300000>;
> 			regulator-max-microvolt = <3300000>;
> 			vin-supply = <&vdd_3v3_sys>;
> 		};

Isn't this controlled by signal PMU_REGEN3 (AS3722 GPIO1) ; that seems
to be routed into the "ON" pin on U20A7 SLG5NV1430V. Is this something
you're planning on fixing later, by creating a standalone PMU_REGEN3
regulator, and inserting into the hierarchy?

Incidentally, that IC also takes in +5V_SYS, but perhaps it isn't worth
worrying about that, since +5V_SYS is always-on?

On the same schematic page as U20A7, I also see supplies +3.3V_SD_CARD,
+1.8V_VDDIO_LP0_OFF, and +3.3V_LP0 which I think are missing from the DT.

> 		vdd_3v3_hdmi: regulator@4 {
> 			compatible = "regulator-fixed";
> 			reg = <4>;
> 			regulator-name = "+3.3V_AVDD_HDMI";

I don't see a signal of that name in the schematics.

> 		vdd_5v0_ts: regulator@6 {
> 			compatible = "regulator-fixed";
> 			reg = <6>;
> 			regulator-name = "+5V_VDD_TS_SW";
> 			regulator-min-microvolt = <5000000>;
> 			regulator-max-microvolt = <5000000>;
> 			enable-active-high;
> 			regulator-boot-on;
> 			gpio = <&gpio TEGRA_GPIO(K, 1) GPIO_ACTIVE_LOW>;

I think that should be ACTIVE_HIGH; The signal name is TS_SHDN_L, so
it's active-low as a shutdown signal, but active-high as an enable
signal, which is presumably how the regulator subsystem wants to treat
it (and which matches the enable-active-high property).

Re: [PATCH] ARM: tegra: Overhaul Venice2 regulators

[Thierry Reding]

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

On Tue, Feb 25, 2014 at 03:10:26PM -0700, Stephen Warren wrote:
> On 02/25/2014 09:30 AM, Thierry Reding wrote:
> > Some of the regulators and the relationships to other regulators are
> > wrong. This commit attempts to rectify this by making them more similar
> > to what the schematics contain. This starts by adding a +VDD_MUX supply
> > that represents the 12V input and derives the main +3.3V_SYS and +5V_SYS
> > supplies from that. The majority of the other regulators derive from one
> > of those three.
> > 
> > While at it, rename the regulators to match the names in the schematics
> > to make them easier to match up.
> 
> > diff --git a/arch/arm/boot/dts/tegra124-venice2.dts b/arch/arm/boot/dts/tegra124-venice2.dts
> 
> >  			regulators {
> > -				vsup-sd2-supply = <&vdd_ac_bat_reg>;
> > -				vsup-sd3-supply = <&vdd_ac_bat_reg>;
> > -				vsup-sd4-supply = <&vdd_ac_bat_reg>;
> > -				vsup-sd5-supply = <&vdd_ac_bat_reg>;
> > +				vsup-sd2-supply = <&vdd_5v0_sys>;
> > +				vsup-sd3-supply = <&vdd_5v0_sys>;
> > +				vsup-sd4-supply = <&vdd_5v0_sys>;
> > +				vsup-sd5-supply = <&vdd_5v0_sys>;
> >  				vin-ldo0-supply = <&as3722_sd2>;
> > -				vin-ldo1-6-supply = <&vdd_ac_bat_reg>;
> > +				vin-ldo1-6-supply = <&vdd_3v3_run>;
> >  				vin-ldo2-5-7-supply = <&as3722_sd5>;
> > -				vin-ldo3-4-supply = <&vdd_ac_bat_reg>;
> > -				vin-ldo9-10-supply = <&vdd_ac_bat_reg>;
> > -				vin-ldo11-supply = <&vdd_ac_bat_reg>;
> > +				vin-ldo3-4-supply = <&vdd_3v3_sys>;
> > +				vin-ldo9-10-supply = <&vdd_5v0_sys>;
> > +				vin-ldo11-supply = <&vdd_3v3_run>;
> 
> Looking at the schematic, the binding should require a vin-ldo3-lv and
> vin-lod3_sw property too, although that's not really anything to do with
> this change.

Indeed. And also vin-gpio and vin-gpio-lv. I'm not sure to which degree
we really want to describe the power tree. Some of these properties are
of no relevance to the operating system. Well, maybe for some special
cases they might be.

> >  				sd0 {
> > -					regulator-name = "vdd-cpu";
> > +					regulator-name = "+VDD_CPU";
> 
> I don't see a +VDD_CPU signal anywhere in the schematic
> (602-7R371-0000-A00.Schematics.Rev.1.23). I do see +VDD_CPU_AP, but
> that's driven by a pair of AS3728 ICs, not the AS3722. Perhaps the
> AS3728 are considered logically part of the AS3722 since it seems like
> control of the AS3728s is by wires from the AS3722?

Yes, I'll rename this to +VDD_CPU_AP. I must have confused this with the
name of the Tegra124 input (VDD_CPU).

And yes, I think in this case it makes sense to consider the AS3728 as
part of the PMIC, since it isn't in any way controllable by software it
seems.

> >  
> >  				as3722_sd2: sd2 {
> > -					regulator-name = "vddio-ddr";
> > +					regulator-name = "+1.35V_LP0";
> 
> can we rename this label to e.g. vdd_1v35_lp0, so that it's more obvious
> that the bin-ldo0-supply property above references the correct node.

Done.

> >  				sd3 {
> > -					regulator-name = "vddio-ddr-2phase";
> > +					regulator-name = "+1.35V_LP0";
> 
> Both sd2 and sd3 nodes have the same value for regulator-name. I'm not
> sure that's correct, even if both the outputs are indeed wired together
> on the board?

As far as I can tell the name is only used for descriptive purposes, so
this should work. It could be annoying if either of them fails for some
reason and an error message wouldn't necessarily point to the exact
regulator.

Then again, if one of these were to fail, I'm not sure one would even
get to see any output because they power the DRAMs.

The reason why I chose to give them the same name is that they aren't
distinguishable in the schematics, so any other name would be arbitrary
again. Perhaps naming them something like +1.35V_LP0(sd2) and
+1.35V_LP0(sd3) would work?

> >  				as3722_sd5: sd5 {
> > -					regulator-name = "vddio-sys";
> > +					regulator-name = "+1.8V_VDDIO";
> 
> Rename the label vddio_1v8?

Done.

> >  					regulator-min-microvolt = <1800000>;
> >  					regulator-max-microvolt = <1800000>;
> >  					regulator-boot-on;
> > @@ -710,7 +710,7 @@
> >  				};
> >  
> >  				sd6 {
> > -					regulator-name = "vdd-gpu";
> > +					regulator-name = "+VDD_GPU";
> 
> I think that's +VDD_GPU_AP on the schematics.

Okay, I renamed it.

> BTW, all the AS3728 chips (SD0, SD1, SD6) get +5V_SYS as input, as well
> as one of +VDD_MUX_GPU, +VDD_MUX_CORE, +VDD_MUX_CPU. This doesn't seem
> to be represented anywhere in the DT.

Like I said above, I think that's below the level of detail that we may
want to represent in DT.

If we decide that we need it anyway, I think we can do it in a separate
patch. It will also require changes to the regulator core to implement
multiple supply support.

Perhaps another idea would be to not support multiple regulators in one
vin-supply property, but maybe add other properties. Some of the chips
that take more than one supply use the supplies for different purposes.
As an example, the various SLG5NV1430V chips (such as U20A6) take one
supply that they derive the output from and another supply that they use
to power themselves. So one alternative to multiple phandles in
vin-supply would be to add vdd-supply. But again, I'm not sure if we
should concern ourselves with that level of detail.

> > 		vdd_3v3_run: regulator@3 {
> > 			compatible = "regulator-fixed";
> > 			reg = <3>;
> > 			regulator-name = "+3.3V_RUN";
> > 			regulator-min-microvolt = <3300000>;
> > 			regulator-max-microvolt = <3300000>;
> > 			vin-supply = <&vdd_3v3_sys>;
> > 		};
> 
> Isn't this controlled by signal PMU_REGEN3 (AS3722 GPIO1) ; that seems
> to be routed into the "ON" pin on U20A7 SLG5NV1430V. Is this something
> you're planning on fixing later, by creating a standalone PMU_REGEN3
> regulator, and inserting into the hierarchy?

That's something that could be done. One of the issues is that there
isn't a proper regulator for PMU_REGEN3, but at the same time it is used
to control also the +1.8V_VDDIO_LP0_OFF regulator. That means we'd have
to used a dummy regulator to wrap the PMU_REGEN3 GPIO just so it can be
used as the input for multiple other regulators.

Of course in that case it isn't simply an enable pin anymore. Therefore
it will need to be listed in a vin-supply property, rather than the gpio
property for the regulator. But since the regulator is also fed by other
supplies this too will require multiple supply support in the regulator
core.

One other issue is that PMU_REGEN3 is also used to discharge the
+1.05V_RUN, +3.3V_RUN and +1.8V_VDDIO_LP0_OFF rails. I can't claim to
completely understand that circuitry, but it seems that when PMU_REGEN3
is pulled low, then all of the above rails will be discharged. For both
+3.3V_RUN and +1.8V_VDDIO_LP0_OFF shouldn't be too bad because they are
dependent on PMU_REGEN3 anyway. But for +1.05V_RUN that's not the case.
That's used for PCIe and SATA (both not used on Venice2 I think) as well
as HDMI (AVDD_HDMI_PLL).

Then again, HDMI also depends on +3.3V_RUN for AVDD_HDMI, so it won't
work anyway when PMU_REGEN3 goes low. Perhaps there are other reasons
why +1.05V_RUN can't run separately from the other two either, so maybe
my concerns aren't valid.

I do seem to remember an issue related to one of the PMU_REGEN signals
that caused eDP to not work, but perhaps that was PMU_REGEN1. That's
used to discharge the +1.8V_VDDIO, +1.35V_LP0_VDDIO_DDR_AP and +3.3V_SYS
rails. Since +3.3V_SYS is used as input to +3.3V_RUN, which in turn is
used to generate +3.3V_PANEL, I suspect that could be the explanation.

So for starters I could wire up PMU_REGEN3 to this regulator (+3.3V_RUN)
but it means we may have to change it again if we ever want to support
the +1.8V_VDDIO_LP0_OFF rail as well.

> Incidentally, that IC also takes in +5V_SYS, but perhaps it isn't worth
> worrying about that, since +5V_SYS is always-on?

Yes, I'd rather ignore that for now.

> On the same schematic page as U20A7, I also see supplies +3.3V_SD_CARD,
> +1.8V_VDDIO_LP0_OFF, and +3.3V_LP0 which I think are missing from the DT.

+3.3V_SD_CARD isn't represented as a regulator. The EN_VDD_SD pin (GPIO
R.00) is directly hooked up via the sdhci@700b0400 node's power-gpios
property.

+1.8V_VDDIO_LP0_OFF isn't used yet and might be tricky to support along
with +3.3V_RUN since they are controlled by the same GPIO (PMU_REGEN3,
see above).

+3.3V_LP0 is interesting. That's used among other things for PCIe, SATA,
HDMI CEC, but is also connected to the AVDD_USB input on Tegra124. It
seems like that's the input that will supply the VBUS outputs, so I'm
beginning to wonder if perhaps that's what causes the USB issue we've
been seeing. I'll see if adding a regulator for this will improve
things.

> > 		vdd_3v3_hdmi: regulator@4 {
> > 			compatible = "regulator-fixed";
> > 			reg = <4>;
> > 			regulator-name = "+3.3V_AVDD_HDMI";
> 
> I don't see a signal of that name in the schematics.

The signal name is +3.3V_AVDD_HDMI_AP_GATED. Looking at the schematics
again the EN_VDD_HDMI pin (GPIO K.06) also isn't actually enabling this
regulator. +1.05V_RUN is.

Looking further the EN_VDD_HDMI pin is actually used to enable the U21B4
regulator that produces the +5V_HDMI signal. But there's some slight
inconsistency here.

The Tegra HDMI block has two input voltages, VDD and PLL. These are both
represented by the vdd-supply and pll-supply properties. However, at
least on Dalmore and on Venice2 now, the vdd-supply is one controllable
via a GPIO and therefore goes to the +5V pin on the HDMI connector
rather than the 3.3V AVDD_HDMI input of Tegra. For all other boards it
seems like we don't handle that pin at all, presumably because it is
always on anyway (on Beaver, the +5V pin of the HDMI connector is
supplied by +SYS_3V3 and +VDD_1V8, both of which are always on).

To rectify the situation I think we'll need another supply in the HDMI
DT bindings, say hdmi-supply, that can be used to describe the signal
that should go to the connector's +5V pin. Or perhaps add a subnode
connector to the hdmi node to represent this more accurately:

	hdmi@54280000 {
		vdd-supply = <&vdd_3v3_hdmi>;
		pll-supply = <&vdd_hdmi_pll>;

		...

		connector {
			supply = <&vdd_5v0_hdmi>;
		};
	};

Does that sound reasonable?

> > 		vdd_5v0_ts: regulator@6 {
> > 			compatible = "regulator-fixed";
> > 			reg = <6>;
> > 			regulator-name = "+5V_VDD_TS_SW";
> > 			regulator-min-microvolt = <5000000>;
> > 			regulator-max-microvolt = <5000000>;
> > 			enable-active-high;
> > 			regulator-boot-on;
> > 			gpio = <&gpio TEGRA_GPIO(K, 1) GPIO_ACTIVE_LOW>;
> 
> I think that should be ACTIVE_HIGH; The signal name is TS_SHDN_L, so
> it's active-low as a shutdown signal, but active-high as an enable
> signal, which is presumably how the regulator subsystem wants to treat
> it (and which matches the enable-active-high property).

Yes, you're right. It probably only works because the GPIO flags are
being ignored and enable-active-high is what defines the polarity.

Thierry

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

Re: [PATCH] ARM: tegra: Overhaul Venice2 regulators

[Stephen Warren]

On 02/26/2014 04:02 AM, Thierry Reding wrote:
> On Tue, Feb 25, 2014 at 03:10:26PM -0700, Stephen Warren wrote:
>> On 02/25/2014 09:30 AM, Thierry Reding wrote:
>>> Some of the regulators and the relationships to other regulators are
>>> wrong. This commit attempts to rectify this by making them more similar
>>> to what the schematics contain. This starts by adding a +VDD_MUX supply
>>> that represents the 12V input and derives the main +3.3V_SYS and +5V_SYS
>>> supplies from that. The majority of the other regulators derive from one
>>> of those three.
>>>
>>> While at it, rename the regulators to match the names in the schematics
>>> to make them easier to match up.
>>
>>> diff --git a/arch/arm/boot/dts/tegra124-venice2.dts b/arch/arm/boot/dts/tegra124-venice2.dts
>>
>>>  			regulators {
>>> -				vsup-sd2-supply = <&vdd_ac_bat_reg>;
>>> -				vsup-sd3-supply = <&vdd_ac_bat_reg>;
>>> -				vsup-sd4-supply = <&vdd_ac_bat_reg>;
>>> -				vsup-sd5-supply = <&vdd_ac_bat_reg>;
>>> +				vsup-sd2-supply = <&vdd_5v0_sys>;
>>> +				vsup-sd3-supply = <&vdd_5v0_sys>;
>>> +				vsup-sd4-supply = <&vdd_5v0_sys>;
>>> +				vsup-sd5-supply = <&vdd_5v0_sys>;
>>>  				vin-ldo0-supply = <&as3722_sd2>;
>>> -				vin-ldo1-6-supply = <&vdd_ac_bat_reg>;
>>> +				vin-ldo1-6-supply = <&vdd_3v3_run>;
>>>  				vin-ldo2-5-7-supply = <&as3722_sd5>;
>>> -				vin-ldo3-4-supply = <&vdd_ac_bat_reg>;
>>> -				vin-ldo9-10-supply = <&vdd_ac_bat_reg>;
>>> -				vin-ldo11-supply = <&vdd_ac_bat_reg>;
>>> +				vin-ldo3-4-supply = <&vdd_3v3_sys>;
>>> +				vin-ldo9-10-supply = <&vdd_5v0_sys>;
>>> +				vin-ldo11-supply = <&vdd_3v3_run>;
>>
>> Looking at the schematic, the binding should require a vin-ldo3-lv and
>> vin-lod3_sw property too, although that's not really anything to do with
>> this change.
> 
> Indeed. And also vin-gpio and vin-gpio-lv. I'm not sure to which degree
> we really want to describe the power tree. Some of these properties are
> of no relevance to the operating system. Well, maybe for some special
> cases they might be.

I guess any supply that can reasonably expected will never have SW
control in any design ever (which may be reasonable for inputs to a PMIC
that are derived directly from a battery or AC input), can reasonably be
left out. I suppose if we ever do need SW control, we can always make
them optional supplies later.

>>>  				sd3 {
>>> -					regulator-name = "vddio-ddr-2phase";
>>> +					regulator-name = "+1.35V_LP0";
>>
>> Both sd2 and sd3 nodes have the same value for regulator-name. I'm not
>> sure that's correct, even if both the outputs are indeed wired together
>> on the board?
> 
> As far as I can tell the name is only used for descriptive purposes, so
> this should work. It could be annoying if either of them fails for some
> reason and an error message wouldn't necessarily point to the exact
> regulator.
> 
> Then again, if one of these were to fail, I'm not sure one would even
> get to see any output because they power the DRAMs.
> 
> The reason why I chose to give them the same name is that they aren't
> distinguishable in the schematics, so any other name would be arbitrary
> again. Perhaps naming them something like +1.35V_LP0(sd2) and
> +1.35V_LP0(sd3) would work?

Yes, that might be better. I believe the regulator-name is used for
debugfs filename regulator/${regulator-name}, so one of these isn't
showing up at present.


>> BTW, all the AS3728 chips (SD0, SD1, SD6) get +5V_SYS as input, as well
>> as one of +VDD_MUX_GPU, +VDD_MUX_CORE, +VDD_MUX_CPU. This doesn't seem
>> to be represented anywhere in the DT.
> 
> Like I said above, I think that's below the level of detail that we may
> want to represent in DT.
> 
> If we decide that we need it anyway, I think we can do it in a separate
> patch. It will also require changes to the regulator core to implement
> multiple supply support.
> 
> Perhaps another idea would be to not support multiple regulators in one
> vin-supply property, but maybe add other properties.

Yes, if we do address this issue (and it's probably fine not to), we
should definitely use different properties; each property is meant to
represent a specific supply to the chip; it's not like each chip is
supposed to choose some different name for the supply property, then
throw all of its sources into that one property.

>>> 		vdd_3v3_run: regulator@3 {
>>> 			compatible = "regulator-fixed";
>>> 			reg = <3>;
>>> 			regulator-name = "+3.3V_RUN";
>>> 			regulator-min-microvolt = <3300000>;
>>> 			regulator-max-microvolt = <3300000>;
>>> 			vin-supply = <&vdd_3v3_sys>;
>>> 		};
>>
>> Isn't this controlled by signal PMU_REGEN3 (AS3722 GPIO1) ; that seems
>> to be routed into the "ON" pin on U20A7 SLG5NV1430V. Is this something
>> you're planning on fixing later, by creating a standalone PMU_REGEN3
>> regulator, and inserting into the hierarchy?
> 
> That's something that could be done. One of the issues is that there
> isn't a proper regulator for PMU_REGEN3, but at the same time it is used
> to control also the +1.8V_VDDIO_LP0_OFF regulator. That means we'd have
> to used a dummy regulator to wrap the PMU_REGEN3 GPIO just so it can be
> used as the input for multiple other regulators.
> 
> Of course in that case it isn't simply an enable pin anymore. Therefore
> it will need to be listed in a vin-supply property, rather than the gpio
> property for the regulator. But since the regulator is also fed by other
> supplies this too will require multiple supply support in the regulator
> core.
> 
> One other issue is that PMU_REGEN3 is also used to discharge the
> +1.05V_RUN, +3.3V_RUN and +1.8V_VDDIO_LP0_OFF rails. I can't claim to
> completely understand that circuitry, but it seems that when PMU_REGEN3
> is pulled low, then all of the above rails will be discharged. For both
> +3.3V_RUN and +1.8V_VDDIO_LP0_OFF shouldn't be too bad because they are
> dependent on PMU_REGEN3 anyway. But for +1.05V_RUN that's not the case.
> That's used for PCIe and SATA (both not used on Venice2 I think) as well
> as HDMI (AVDD_HDMI_PLL).

Hmm. So if we turn off (pull low) PMU_REGEN3 while +1.05V_RUN is
enabled, we'll end up shorting +1.05V_RUN to ground? Is that possible in
the latest version of your patch? If not, then I'll just trust your
latest patch:-) If it is possible, then I think we need to rejig the
patch somehow so we absolutely can't do that.

...
> The Tegra HDMI block has two input voltages, VDD and PLL. These are both
> represented by the vdd-supply and pll-supply properties. However, at
> least on Dalmore and on Venice2 now, the vdd-supply is one controllable
> via a GPIO and therefore goes to the +5V pin on the HDMI connector
> rather than the 3.3V AVDD_HDMI input of Tegra. For all other boards it
> seems like we don't handle that pin at all, presumably because it is
> always on anyway (on Beaver, the +5V pin of the HDMI connector is
> supplied by +SYS_3V3 and +VDD_1V8, both of which are always on).
> 
> To rectify the situation I think we'll need another supply in the HDMI
> DT bindings, say hdmi-supply, that can be used to describe the signal
> that should go to the connector's +5V pin. Or perhaps add a subnode
> connector to the hdmi node to represent this more accurately:
> 
> 	hdmi@54280000 {
> 		vdd-supply = <&vdd_3v3_hdmi>;
> 		pll-supply = <&vdd_hdmi_pll>;
> 
> 		...
> 
> 		connector {
> 			supply = <&vdd_5v0_hdmi>;
> 		};
> 	};
> 
> Does that sound reasonable?

Yes. I think a hdmi-supply property would be fine; I don't think we need
another node? Either way is fine though.

Re: [PATCH] ARM: tegra: Overhaul Venice2 regulators

[Thierry Reding]

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

On Thu, Feb 27, 2014 at 03:19:47PM -0700, Stephen Warren wrote:
> On 02/26/2014 04:02 AM, Thierry Reding wrote:
> > On Tue, Feb 25, 2014 at 03:10:26PM -0700, Stephen Warren wrote:
> >> On 02/25/2014 09:30 AM, Thierry Reding wrote:
[...]
> >>> 		vdd_3v3_run: regulator@3 {
> >>> 			compatible = "regulator-fixed";
> >>> 			reg = <3>;
> >>> 			regulator-name = "+3.3V_RUN";
> >>> 			regulator-min-microvolt = <3300000>;
> >>> 			regulator-max-microvolt = <3300000>;
> >>> 			vin-supply = <&vdd_3v3_sys>;
> >>> 		};
> >>
> >> Isn't this controlled by signal PMU_REGEN3 (AS3722 GPIO1) ; that seems
> >> to be routed into the "ON" pin on U20A7 SLG5NV1430V. Is this something
> >> you're planning on fixing later, by creating a standalone PMU_REGEN3
> >> regulator, and inserting into the hierarchy?
> > 
> > That's something that could be done. One of the issues is that there
> > isn't a proper regulator for PMU_REGEN3, but at the same time it is used
> > to control also the +1.8V_VDDIO_LP0_OFF regulator. That means we'd have
> > to used a dummy regulator to wrap the PMU_REGEN3 GPIO just so it can be
> > used as the input for multiple other regulators.
> > 
> > Of course in that case it isn't simply an enable pin anymore. Therefore
> > it will need to be listed in a vin-supply property, rather than the gpio
> > property for the regulator. But since the regulator is also fed by other
> > supplies this too will require multiple supply support in the regulator
> > core.
> > 
> > One other issue is that PMU_REGEN3 is also used to discharge the
> > +1.05V_RUN, +3.3V_RUN and +1.8V_VDDIO_LP0_OFF rails. I can't claim to
> > completely understand that circuitry, but it seems that when PMU_REGEN3
> > is pulled low, then all of the above rails will be discharged. For both
> > +3.3V_RUN and +1.8V_VDDIO_LP0_OFF shouldn't be too bad because they are
> > dependent on PMU_REGEN3 anyway. But for +1.05V_RUN that's not the case.
> > That's used for PCIe and SATA (both not used on Venice2 I think) as well
> > as HDMI (AVDD_HDMI_PLL).
> 
> Hmm. So if we turn off (pull low) PMU_REGEN3 while +1.05V_RUN is
> enabled, we'll end up shorting +1.05V_RUN to ground? Is that possible in
> the latest version of your patch? If not, then I'll just trust your
> latest patch:-) If it is possible, then I think we need to rejig the
> patch somehow so we absolutely can't do that.

Currently +1.05V_RUN is always on, so indeed if none of the regulators
supplied by +3.3V_RUN is enabled it will cause +3.3V_RUN to become
disabled as well and therefore short +1.05V_RUN to ground. However, we
currently don't use +1.05V_RUN. It's used by PCIe and SATA which aren't
even connected on Venice2 as far as I can tell, and HDMI. Since we don't
support HDMI yet it shouldn't be a problem.

One problem is that +1.05V_RUN is currently marked always-on, so I'll
resend another version of the patch with that removed, which should
cause it to always remain disabled and therefore not cause any problems
for now.

But even once we do add HDMI support there should be no issue, since
HDMI also needs the +3.3V_AVDD_HDMI_AP_GATED, which is supplied by
+3.3V_RUN. Currently the driver also enables the AVDD_HDMI regulator
before the AVDD_HDMI_PLL regulator, therefore the situation where
+1.05V_RUN could be shorted to ground should never be able to occur.

One related note below...

> > The Tegra HDMI block has two input voltages, VDD and PLL. These are both
> > represented by the vdd-supply and pll-supply properties. However, at
> > least on Dalmore and on Venice2 now, the vdd-supply is one controllable
> > via a GPIO and therefore goes to the +5V pin on the HDMI connector
> > rather than the 3.3V AVDD_HDMI input of Tegra. For all other boards it
> > seems like we don't handle that pin at all, presumably because it is
> > always on anyway (on Beaver, the +5V pin of the HDMI connector is
> > supplied by +SYS_3V3 and +VDD_1V8, both of which are always on).
> > 
> > To rectify the situation I think we'll need another supply in the HDMI
> > DT bindings, say hdmi-supply, that can be used to describe the signal
> > that should go to the connector's +5V pin. Or perhaps add a subnode
> > connector to the hdmi node to represent this more accurately:
> > 
> > 	hdmi@54280000 {
> > 		vdd-supply = <&vdd_3v3_hdmi>;
> > 		pll-supply = <&vdd_hdmi_pll>;
> > 
> > 		...
> > 
> > 		connector {
> > 			supply = <&vdd_5v0_hdmi>;
> > 		};
> > 	};
> > 
> > Does that sound reasonable?
> 
> Yes. I think a hdmi-supply property would be fine; I don't think we need
> another node? Either way is fine though.

As far as I remember, the reason for commit 18ebc0f404d5 "drm/tegra:
hdmi: Enable VDD earlier for hotplug/DDC" was that the +5V to the HDMI
connector needs to be enabled early in order to allow hotplug detection
since the HDMI sink uses those 5V to return the HPD signal.

Now like I mentioned above, there seems to have been a mixup and on
Dalmore the vdd-supply is used to control both the AVDD_HDMI input (via
the Palmas' ldoln supply) and the +5V that goes to the HDMI connector
(via GPIO K.01). With the above proposal those would be split up into
two supplies, vdd_3v3_hdmi that is really only the ldoln supply and the
new vdd_5v0_hdmi that's controlled by GPIO K.01 (and supplied by
VDD_5V0_SYS it would seem).

With that change in place, the new hdmi-supply property can be used in
the driver to obtain a reference to that regulator and enable it early
and for the whole time during which HDMI is loaded. That means that
commit 18ebc0f404d5 can be reverted, because we can potentially save
some power by only enabling the AVDD_HDMI supply when HDMI is actually
enabled (as opposed to just loaded, as in the driver needs to be able
to receive hotplug events).

But looking at that commit, before that change was made, the VDD supply
was always enabled before the PLL supply, which means that for Venice2's
PMU_REGEN3 issue this should ensure that +1.05V_RUN supply can never be
shorted to ground.

As a side-note, the regulators on Dalmore seem to suffer from the same
issues as Venice2. The regulator names don't map to signal names in the
schematics and some of the uses seem to be plain wrong. So perhaps I
should make a similar pass over the Dalmore DTS file and fix things up.
Perhaps it isn't worth the churn, though, but either way we should make
sure this is done more properly for future boards.

Thierry

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