| Message ID | 1387469182-14398-2-git-send-email-treding@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 12/19/2013 09:06 AM, Thierry Reding wrote: > This also adds the vmmc-supply property to the SDMMC node on Venice2. > Otherwise the core will turn the regulator off automatically because it > is unused. Laxman sent a version of this patch first, which I have already applied. His version is almost a pure subset of yours. I've pointed out the differences (other than simple naming) below so you two can work together to resolve them. (Laxman, as an aside, I'm not sure why you're upstreaming patches that don't exactly match the existing kernel support for this board...) > diff --git a/arch/arm/boot/dts/tegra124-venice2.dts b/arch/arm/boot/dts/tegra124-venice2.dts > + sd0 { > + regulator-name = "vdd_cpu"; > + regulator-min-microvolt = <700000>; > + regulator-max-microvolt = <1350000>; Laxman's patch has: regulator-max-microvolt = <1400000>; > + regulator-always-on; > + ams,ext-control = <2>; > + }; Laxman's patch adds: regulator-min-microamp = <3500000>; regulator-max-microamp = <3500000>; regulator-boot-on; Those properties (with various values) are added to quite a few nodes; I won't bother pointing them out elsewhere. > + sd1 { ... > + regulator-max-microamp = <3500000>; Laxman's patch has: regulator-max-microamp = <2500000>; Laxman's patch adds the following node: sd3 { regulator-name = "vddio-ddr-2phase"; regulator-min-microvolt = <1350000>; regulator-max-microvolt = <1350000>; regulator-always-on; regulator-boot-on; }; > + sd6 { ... > + regulator-min-microvolt = <800000>; Laxman's patch has: regulator-min-microvolt = <650000>; > + ldo6 { ... > + regulator-min-microvolt = <3300000>; Laxman's patch has: regulator-min-microvolt = <1800000>;
On 12/19/2013 09:06 AM, Thierry Reding wrote: > This also adds the vmmc-supply property to the SDMMC node on Venice2. > Otherwise the core will turn the regulator off automatically because it > is unused. Note: If I revert Laxman's "" and apply your patches 01/10 and 02/10 in its place, then I see the following during boot, and audio playback doesn't work any more: > [ 14.768649] max98090 0-0010: ASoC: Right Receiver Mixer DAPM update failed: -121 > [ 14.776715] max98090 0-0010: ASoC: Left Receiver Mixer DAPM update failed: -121 > [ 14.784601] max98090 0-0010: ASoC: Right Speaker Mixer DAPM update failed: -121 > [ 14.792602] max98090 0-0010: ASoC: Left Speaker Mixer DAPM update failed: -121 > [ 14.800423] max98090 0-0010: ASoC: Right Headphone Mixer DAPM update failed: -121 > [ 14.808655] max98090 0-0010: ASoC: Left Headphone Mixer DAPM update failed: -121 > [ 14.817628] max98090 0-0010: ASoC: Right ADC Mixer DAPM update failed: -121 > [ 14.825582] max98090 0-0010: ASoC: Left ADC Mixer DAPM update failed: -121
On Friday 20 December 2013 01:54 AM, Stephen Warren wrote: > On 12/19/2013 09:06 AM, Thierry Reding wrote: > (Laxman, as an aside, I'm not sure why you're upstreaming patches that > don't exactly match the existing kernel support for this board...) I did not get this based on what context it is. Can you please elaborate where I am missing the stuff? > >> diff --git a/arch/arm/boot/dts/tegra124-venice2.dts b/arch/arm/boot/dts/tegra124-venice2.dts >> + sd0 { >> + regulator-name = "vdd_cpu"; >> + regulator-min-microvolt = <700000>; >> + regulator-max-microvolt = <1350000>; > Laxman's patch has: > > regulator-max-microvolt = <1400000>; We have the Laguna platform on which Android and L4T is running fine. This is based on same PMIC used for Venice2. As we are running the more cpu stress on Laguna, I took this parameter from the Laguna Power tree where it is maximum 1.4V. Chrome have maximum as 1.35mV. > >> + regulator-always-on; >> + ams,ext-control = <2>; >> + }; > Laxman's patch adds: > > regulator-min-microamp = <3500000>; > regulator-max-microamp = <3500000>; > regulator-boot-on; > > Those properties (with various values) are added to quite a few nodes; I > won't bother pointing them out elsewhere. All rails does not support the current configuration. SD0, SD1 and SD6 supports this. We have configured these value on Laguna as per system eng recommendation for this rails. As the power tree s almost same, I configured same here for Venice2. SD0 and SD6 on 3500mA and SD1 is on 2500mA. > >> + sd1 { > ... >> + regulator-max-microamp = <3500000>; > Laxman's patch has: > > regulator-max-microamp = <2500000>; > > Laxman's patch adds the following node: > > sd3 { > regulator-name = "vddio-ddr-2phase"; > regulator-min-microvolt = <1350000>; > regulator-max-microvolt = <1350000>; > regulator-always-on; > regulator-boot-on; > }; As per schematic, SD2 and SD3 make 2 phase supply for the DDR. I added SD3 so that it should not get disabled at time of unused regulator disabled by regulator framework. > >> + sd6 { > ... >> + regulator-min-microvolt = <800000>; > Laxman's patch has: > > regulator-min-microvolt = <650000>; Again from downstream Laguna platform. > >> + ldo6 { > ... >> + regulator-min-microvolt = <3300000>; > Laxman's patch has: > > regulator-min-microvolt = <1800000>; VDDIO SDMMC and I think it is for SD3.0 (UHS) which switches voltage from 3.3 to 1.8 on UHS mode by sdmmc driver.
On Fri, Dec 20, 2013 at 12:23:28PM +0530, Laxman Dewangan wrote: > On Friday 20 December 2013 01:54 AM, Stephen Warren wrote: > >On 12/19/2013 09:06 AM, Thierry Reding wrote: > > >(Laxman, as an aside, I'm not sure why you're upstreaming patches that > >don't exactly match the existing kernel support for this board...) > > I did not get this based on what context it is. Can you please elaborate > where I am missing the stuff? > > > > > >>diff --git a/arch/arm/boot/dts/tegra124-venice2.dts b/arch/arm/boot/dts/tegra124-venice2.dts > >>+ sd0 { > >>+ regulator-name = "vdd_cpu"; > >>+ regulator-min-microvolt = <700000>; > >>+ regulator-max-microvolt = <1350000>; > >Laxman's patch has: > > > > regulator-max-microvolt = <1400000>; > > We have the Laguna platform on which Android and L4T is running fine. This > is based on same PMIC used for Venice2. As we are running the more cpu > stress on Laguna, I took this parameter from the Laguna Power tree where it > is maximum 1.4V. Chrome have maximum as 1.35mV. Whether this is used on Android, ChromeOS or L4T doesn't matter at all. It specifies hardware constraints and thus must be agnostic of the OS and workload. Also this file describes the power tree for Venice2, so using values from Laguna is wrong, no matter how similar they are. Thierry
On Friday 20 December 2013 03:55 PM, Thierry Reding wrote: > * PGP Signed by an unknown key > > On Fri, Dec 20, 2013 at 12:23:28PM +0530, Laxman Dewangan wrote: >> On Friday 20 December 2013 01:54 AM, Stephen Warren wrote: >>> On 12/19/2013 09:06 AM, Thierry Reding wrote: >>> (Laxman, as an aside, I'm not sure why you're upstreaming patches that >>> don't exactly match the existing kernel support for this board...) >> I did not get this based on what context it is. Can you please elaborate >> where I am missing the stuff? >> >> >>>> diff --git a/arch/arm/boot/dts/tegra124-venice2.dts b/arch/arm/boot/dts/tegra124-venice2.dts >>>> + sd0 { >>>> + regulator-name = "vdd_cpu"; >>>> + regulator-min-microvolt = <700000>; >>>> + regulator-max-microvolt = <1350000>; >>> Laxman's patch has: >>> >>> regulator-max-microvolt = <1400000>; >> We have the Laguna platform on which Android and L4T is running fine. This >> is based on same PMIC used for Venice2. As we are running the more cpu >> stress on Laguna, I took this parameter from the Laguna Power tree where it >> is maximum 1.4V. Chrome have maximum as 1.35mV. > Whether this is used on Android, ChromeOS or L4T doesn't matter at all. > It specifies hardware constraints and thus must be agnostic of the OS > and workload. > > Also this file describes the power tree for Venice2, so using values > from Laguna is wrong, no matter how similar they are. > Here, I used term "similar" means the which rail is feeding to Tegra's which vdd? So by this, if AMS SD0 is feeding to Tegra vdd-cpu then it should be same for Laguna. This is maximum possible for Tegra cpu from PMIC. Tegra can use desired voltage limited to this max based on their sku for going to maximum frequency. If we program less voltage as maximum then Tegra will not go to higher frequency assuming PMIC limitation which is not correct.
On Thu, Dec 19, 2013 at 02:47:09PM -0700, Stephen Warren wrote: > On 12/19/2013 09:06 AM, Thierry Reding wrote: > > This also adds the vmmc-supply property to the SDMMC node on Venice2. > > Otherwise the core will turn the regulator off automatically because it > > is unused. > > Note: If I revert Laxman's "" and apply your patches 01/10 and 02/10 in > its place, then I see the following during boot, and audio playback > doesn't work any more: > > > [ 14.768649] max98090 0-0010: ASoC: Right Receiver Mixer DAPM update failed: -121 > > [ 14.776715] max98090 0-0010: ASoC: Left Receiver Mixer DAPM update failed: -121 > > [ 14.784601] max98090 0-0010: ASoC: Right Speaker Mixer DAPM update failed: -121 > > [ 14.792602] max98090 0-0010: ASoC: Left Speaker Mixer DAPM update failed: -121 > > [ 14.800423] max98090 0-0010: ASoC: Right Headphone Mixer DAPM update failed: -121 > > [ 14.808655] max98090 0-0010: ASoC: Left Headphone Mixer DAPM update failed: -121 > > [ 14.817628] max98090 0-0010: ASoC: Right ADC Mixer DAPM update failed: -121 > > [ 14.825582] max98090 0-0010: ASoC: Left ADC Mixer DAPM update failed: -121 That's odd, I don't those errors in my local tree. When I try to play back audio I do get this one: [ 380.093679] max98090 0-0010: No audio clocks configured But other than that everything seems to work fine. I don't hear anything on the speakers or the headphones, but I'm likely missing the proper mixer settings. Can you provide an asound.conf that works for you? Thierry
On 12/20/2013 03:46 AM, Laxman Dewangan wrote: > On Friday 20 December 2013 03:55 PM, Thierry Reding wrote: >> * PGP Signed by an unknown key >> >> On Fri, Dec 20, 2013 at 12:23:28PM +0530, Laxman Dewangan wrote: >>> On Friday 20 December 2013 01:54 AM, Stephen Warren wrote: >>>> On 12/19/2013 09:06 AM, Thierry Reding wrote: >>>> (Laxman, as an aside, I'm not sure why you're upstreaming patches that >>>> don't exactly match the existing kernel support for this board...) >>> I did not get this based on what context it is. Can you please elaborate >>> where I am missing the stuff? >>> >>> >>>>> diff --git a/arch/arm/boot/dts/tegra124-venice2.dts >>>>> b/arch/arm/boot/dts/tegra124-venice2.dts >>>>> + sd0 { >>>>> + regulator-name = "vdd_cpu"; >>>>> + regulator-min-microvolt = <700000>; >>>>> + regulator-max-microvolt = <1350000>; >>>> Laxman's patch has: >>>> >>>> regulator-max-microvolt = <1400000>; >>> We have the Laguna platform on which Android and L4T is running fine. >>> This >>> is based on same PMIC used for Venice2. As we are running the more cpu >>> stress on Laguna, I took this parameter from the Laguna Power tree >>> where it >>> is maximum 1.4V. Chrome have maximum as 1.35mV. >> Whether this is used on Android, ChromeOS or L4T doesn't matter at all. >> It specifies hardware constraints and thus must be agnostic of the OS >> and workload. >> >> Also this file describes the power tree for Venice2, so using values >> from Laguna is wrong, no matter how similar they are. >> > > Here, I used term "similar" means the which rail is feeding to Tegra's > which vdd? > So by this, if AMS SD0 is feeding to Tegra vdd-cpu then it should be > same for Laguna. Look, this situation is very simple. This file describe Venice2. It doesn't matter whether Laguna "should be" similar to Venice2 or not, the file needs to describe Venice2 and not Laguna. Equally, we have a downstream kernel that fully supports Venice2. There is therefore absolutely ZERO reason why we should use the downstream Laguna board support to create the upstream Venice2 board support, rather than using the downstream Venice2 board support to create the upstream Venice2 board support. Please do let me know that you fully understand this issue. If not, future patches from you are going to need a heck of a lot more detailed review and manual checking, rather than my trusting you got it right.
On 12/20/2013 04:14 AM, Thierry Reding wrote: > On Thu, Dec 19, 2013 at 02:47:09PM -0700, Stephen Warren wrote: >> On 12/19/2013 09:06 AM, Thierry Reding wrote: >>> This also adds the vmmc-supply property to the SDMMC node on Venice2. >>> Otherwise the core will turn the regulator off automatically because it >>> is unused. >> >> Note: If I revert Laxman's "" and apply your patches 01/10 and 02/10 in >> its place, then I see the following during boot, and audio playback >> doesn't work any more: >> >>> [ 14.768649] max98090 0-0010: ASoC: Right Receiver Mixer DAPM update failed: -121 >>> [ 14.776715] max98090 0-0010: ASoC: Left Receiver Mixer DAPM update failed: -121 >>> [ 14.784601] max98090 0-0010: ASoC: Right Speaker Mixer DAPM update failed: -121 >>> [ 14.792602] max98090 0-0010: ASoC: Left Speaker Mixer DAPM update failed: -121 >>> [ 14.800423] max98090 0-0010: ASoC: Right Headphone Mixer DAPM update failed: -121 >>> [ 14.808655] max98090 0-0010: ASoC: Left Headphone Mixer DAPM update failed: -121 >>> [ 14.817628] max98090 0-0010: ASoC: Right ADC Mixer DAPM update failed: -121 >>> [ 14.825582] max98090 0-0010: ASoC: Left ADC Mixer DAPM update failed: -121 > > That's odd, I don't those errors in my local tree. When I try to play > back audio I do get this one: > > [ 380.093679] max98090 0-0010: No audio clocks configured I do see that too. In practice it's harmless since audio does work, and I /think/ it's really a false startup-condition error from the CODEC driver, but I need to investigate it sometime. > But other than that everything seems to work fine. I don't hear anything > on the speakers or the headphones, but I'm likely missing the proper > mixer settings. Can you provide an asound.conf that works for you? Yes, you will need to configure the mixer. Our internal kernel testing wiki page has asound.state attached. I'll send you a link to that internally.
On Friday 20 December 2013 10:27 PM, Stephen Warren wrote: > On 12/20/2013 03:46 AM, Laxman Dewangan wrote: >> On Friday 20 December 2013 03:55 PM, Thierry Reding wrote: >>> * PGP Signed by an unknown key >>> >>> On Fri, Dec 20, 2013 at 12:23:28PM +0530, Laxman Dewangan wrote: >>>> On Friday 20 December 2013 01:54 AM, Stephen Warren wrote: >>>>> On 12/19/2013 09:06 AM, Thierry Reding wrote: >>>>> (Laxman, as an aside, I'm not sure why you're upstreaming patches that >>>>> don't exactly match the existing kernel support for this board...) >>>> I did not get this based on what context it is. Can you please elaborate >>>> where I am missing the stuff? >>>> >>>> >>>>>> diff --git a/arch/arm/boot/dts/tegra124-venice2.dts >>>>>> b/arch/arm/boot/dts/tegra124-venice2.dts >>>>>> + sd0 { >>>>>> + regulator-name = "vdd_cpu"; >>>>>> + regulator-min-microvolt = <700000>; >>>>>> + regulator-max-microvolt = <1350000>; >>>>> Laxman's patch has: >>>>> >>>>> regulator-max-microvolt = <1400000>; >>>> We have the Laguna platform on which Android and L4T is running fine. >>>> This >>>> is based on same PMIC used for Venice2. As we are running the more cpu >>>> stress on Laguna, I took this parameter from the Laguna Power tree >>>> where it >>>> is maximum 1.4V. Chrome have maximum as 1.35mV. >>> Whether this is used on Android, ChromeOS or L4T doesn't matter at all. >>> It specifies hardware constraints and thus must be agnostic of the OS >>> and workload. >>> >>> Also this file describes the power tree for Venice2, so using values >>> from Laguna is wrong, no matter how similar they are. >>> >> Here, I used term "similar" means the which rail is feeding to Tegra's >> which vdd? >> So by this, if AMS SD0 is feeding to Tegra vdd-cpu then it should be >> same for Laguna. > Look, this situation is very simple. This file describe Venice2. It > doesn't matter whether Laguna "should be" similar to Venice2 or not, the > file needs to describe Venice2 and not Laguna. > > Equally, we have a downstream kernel that fully supports Venice2. There > is therefore absolutely ZERO reason why we should use the downstream > Laguna board support to create the upstream Venice2 board support, > rather than using the downstream Venice2 board support to create the > upstream Venice2 board support. > > Please do let me know that you fully understand this issue. If not, > future patches from you are going to need a heck of a lot more detailed > review and manual checking, rather than my trusting you got it right. I like to continue this discussion internally so that we can have proper expectations before sending patches.
diff --git a/arch/arm/boot/dts/tegra124-venice2.dts b/arch/arm/boot/dts/tegra124-venice2.dts index d6bb25c78c62..5dc0c64b7682 100644 --- a/arch/arm/boot/dts/tegra124-venice2.dts +++ b/arch/arm/boot/dts/tegra124-venice2.dts @@ -392,6 +392,176 @@ i2c@7000d000 { status = "okay"; clock-frequency = <100000>; + + pmic: pmic@40 { + compatible = "ams,as3722"; + reg = <0x40>; + interrupts = <GIC_SPI 86 IRQ_TYPE_LEVEL_LOW>; + + #interrupt-cells = <2>; + interrupt-controller; + + #gpio-cells = <2>; + gpio-controller; + + pinctrl-names = "default"; + pinctrl-0 = <&as3722_default>; + + as3722_default: pinmux { + gpio0 { + pins = "gpio0"; + function = "gpio"; + bias-pull-down; + }; + + gpio1_2_4_7 { + pins = "gpio1", "gpio2", "gpio4", + "gpio7"; + function = "gpio"; + bias-pull-up; + }; + + gpio3_6 { + pins = "gpio3", "gpio6"; + function = "gpio"; + bias-high-impedance; + }; + + gpio5 { + pins = "gpio5"; + function = "clk32k-out"; + bias-pull-up; + }; + }; + + regulators { + vsup-sd2-supply = <&sys_5v_reg>; + vsup-sd3-supply = <&sys_5v_reg>; + vsup-sd4-supply = <&sys_5v_reg>; + vsup-sd5-supply = <&sys_5v_reg>; + vin-ldo0-supply = <&as3722_sd2>; + vin-ldo1-6-supply = <&sys_5v_reg>; + vin-ldo2-5-7-supply = <&vddio_1v8>; + vin-ldo3-4-supply = <&sys_5v_reg>; + vin-ldo9-10-supply = <&sys_5v_reg>; + vin-ldo11-supply = <&sys_5v_reg>; + + sd0 { + regulator-name = "vdd_cpu"; + regulator-min-microvolt = <700000>; + regulator-max-microvolt = <1350000>; + regulator-always-on; + ams,ext-control = <2>; + }; + + sd1 { + regulator-name = "vdd_core"; + regulator-min-microvolt = <700000>; + regulator-max-microvolt = <1350000>; + regulator-max-microamp = <3500000>; + regulator-always-on; + ams,ext-control = <1>; + }; + + as3722_sd2: sd2 { + regulator-name = "vddio_ddr"; + regulator-min-microvolt = <1350000>; + regulator-max-microvolt = <1350000>; + regulator-always-on; + }; + + sd4 { + regulator-name = "avdd-hdmi-pex"; + regulator-min-microvolt = <1050000>; + regulator-max-microvolt = <1050000>; + }; + + vddio_1v8: sd5 { + regulator-name = "vdd-1v8"; + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <1800000>; + regulator-always-on; + }; + + sd6 { + regulator-name = "vdd_gpu"; + regulator-min-microvolt = <800000>; + regulator-max-microvolt = <1200000>; + regulator-boot-on; + regulator-always-on; + }; + + ldo0 { + regulator-name = "avdd-pll"; + regulator-min-microvolt = <1050000>; + regulator-max-microvolt = <1050000>; + regulator-always-on; + ams,ext-control = <1>; + }; + + ldo1 { + regulator-name = "vdd_cam"; + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <1800000>; + }; + + ldo2 { + regulator-name = "vddio_hsic"; + regulator-min-microvolt = <1200000>; + regulator-max-microvolt = <1200000>; + }; + + ldo3 { + regulator-name = "vdd_rtc"; + regulator-min-microvolt = <1000000>; + regulator-max-microvolt = <1000000>; + regulator-always-on; + ams,enable-tracking; + }; + + ldo4 { + regulator-name = "avdd_cam"; + regulator-min-microvolt = <2800000>; + regulator-max-microvolt = <2800000>; + }; + + ldo5 { + regulator-name = "vdig"; + regulator-min-microvolt = <1200000>; + regulator-max-microvolt = <1200000>; + }; + + ldo6 { + regulator-name = "vddio_sdmmc"; + regulator-min-microvolt = <3300000>; + regulator-max-microvolt = <3300000>; + }; + + ldo7 { + regulator-name = "vdd_cam1"; + regulator-min-microvolt = <1050000>; + regulator-max-microvolt = <1050000>; + }; + + ldo9 { + regulator-name = "avdd"; + regulator-min-microvolt = <2800000>; + regulator-max-microvolt = <2800000>; + }; + + ldo10 { + regulator-name = "avdd_af1_cam"; + regulator-min-microvolt = <2800000>; + regulator-max-microvolt = <2800000>; + }; + + ldo11 { + regulator-name = "vpp_fuse"; + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <1800000>; + }; + }; + }; }; pmc@7000e400 { @@ -436,6 +606,18 @@ }; }; + regulators { + compatible = "simple-bus"; + #address-cells = <1>; + #size-cells = <0>; + + sys_5v_reg: regulator@0 { + compatible = "regulator-fixed"; + reg = <0>; + regulator-name = "sys_5v"; + }; + }; + sound { compatible = "nvidia,tegra-audio-max98090-venice2", "nvidia,tegra-audio-max98090";
This also adds the vmmc-supply property to the SDMMC node on Venice2. Otherwise the core will turn the regulator off automatically because it is unused. Signed-off-by: Thierry Reding <treding@nvidia.com> --- Note: This was taken from a ChromeOS tree. --- arch/arm/boot/dts/tegra124-venice2.dts | 182 +++++++++++++++++++++++++++++++++ 1 file changed, 182 insertions(+)