| Message ID | 1393345802-6121-1-git-send-email-thierry.reding@gmail.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
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). -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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>; }; };