Colibri T20 fixes for 3.11

Some fixes for the Colibri T20 board and to AC97 audio. With this applied 3.11 should be the first kernel with all parts of AC97 audio support in place and working.

Lucas Stach (4):
ASOC: tegra: move AC97 clock defines to the controller node
ASOC: tegra: fix matching of AC97 components
ARM: tegra: enable ULPI phy on Colibri T20
ARM: tegra: correct Colibri T20 regulator settings

 arch/arm/boot/dts/tegra20-colibri-512.dtsi | 39 +++++++++++++++---------------
 arch/arm/boot/dts/tegra20.dtsi             |  6 ++++-
 sound/soc/tegra/tegra20_ac97.c             |  2 +-
 sound/soc/tegra/tegra_wm9712.c             |  1 -
 4 files changed, 25 insertions(+), 23 deletions(-)

1.8.3.1

Different from other Tegra sound controllers drivers, the AC97 controller driver uses the tegra asoc utils directly to request the needed clocks, as they are needed at AC97 init time. Move the DT clock defines to the right place.

---
CC: <alsa-devel-K7yf7f+aM1XWsZ/[email protected]>
CC: <[email protected]>

---
 arch/arm/boot/dts/tegra20-colibri-512.dtsi | 5 -----
 arch/arm/boot/dts/tegra20.dtsi             | 6 +++++-
 sound/soc/tegra/tegra20_ac97.c             | 2 +-
 3 files changed, 6 insertions(+), 7 deletions(-)

index 2fcb3f2..fbb52e0 100644
@@ -491,11 +491,6 @@
 			"Mic", "MIC1";
 
 		nvidia,ac97-controller = <&ac97>;
-
-		clocks = <&tegra_car TEGRA20_CLK_PLL_A>,
-			 <&tegra_car TEGRA20_CLK_PLL_A_OUT0>,
-			 <&tegra_car TEGRA20_CLK_CDEV1>;
-		clock-names = "pll_a", "pll_a_out0", "mclk";
 	};
 
 	regulators {

index 9653fd8..ad13f57 100644
@@ -223,7 +223,11 @@
 		reg = <0x70002000 0x200>;
 		interrupts = <GIC_SPI 81 IRQ_TYPE_LEVEL_HIGH>;
 		nvidia,dma-request-selector = <&apbdma 12>;
-		clocks = <&tegra_car TEGRA20_CLK_AC97>;
+		clocks = <&tegra_car TEGRA20_CLK_PLL_A>,
+			 <&tegra_car TEGRA20_CLK_PLL_A_OUT0>,
+			 <&tegra_car TEGRA20_CLK_CDEV1>,
+			 <&tegra_car TEGRA20_CLK_AC97>;
+		clock-names = "pll_a", "pll_a_out0", "mclk", "ac97";
 		status = "disabled";
 	};
 

index 6c48662..6bbffd1 100644
@@ -326,7 +326,7 @@ static int tegra20_ac97_platform_probe(struct platform_device *pdev)
 	}
 	dev_set_drvdata(&pdev->dev, ac97);
 
-	ac97->clk_ac97 = devm_clk_get(&pdev->dev, NULL);
+	ac97->clk_ac97 = devm_clk_get(&pdev->dev, "ac97");
 	if (IS_ERR(ac97->clk_ac97)) {
 		dev_err(&pdev->dev, "Can't retrieve ac97 clock\n");
 		ret = PTR_ERR(ac97->clk_ac97);

1.8.3.1

I'm sorry but I just don't understand what this change is supposed to do
- what is the current place, what is wrong with it and what is the correct place?

Am Montag, den 22.07.2013, 00:36 +0100 schrieb Mark Brown:

The clocks used by the Tegra ASoC utils were defined in the machine driver DT node for all boards, as this is were they get requested by the I2C and SPDIF Tegra audio drivers. Differently from those two the AC97 driver has to request those clocks in the controller drivers, as they are needed at this point for proper initialisation.
So the patch moves the clocks from the machine driver node to the AC97 controller DT node, so they can be requested in the right driver.

Regards,
Lucas

Why is the way the other devices are doing this sensible?

Am Montag, den 22.07.2013, 10:46 +0100 schrieb Mark Brown:

Because they only need those clocks when actually playing audio, so requesting them late in the process when loading the machine driver is perfectly fine for them. The AC97 controller however already needs those clocks when initializing the controller and codec, so it has to request them earlier.

Regards,
Lucas

But why does it make sense to do that? The clocks are directly connected to the IP blocks in hardware after all..

I'm not convinced this is the correct approach.

The machine driver needs to manage these clocks, so that it can co-ordinate between different audio paths. For example, consider a system that supports two different I2S paths. In HW, if both are active at once, these need to both run at a derivative of 48KHz or both run at a derivative of 44.1KHz. The machine driver is the central place that enforces that, and should eventually automatically place constraints on one stream when another is configured for a specific sample rate. By the same argument, AC'97 can't be a special case here, in case there's some system with both AC'97 and I2S hooked up.

Am Dienstag, den 23.07.2013, 09:47 -0700 schrieb Stephen Warren:

I find it highly unlikely to find any Tegra 20 board with such a configuration. But as your argument is technically correct, I'll try to fix things up the right way and see how big the impact is.

Regards,
Lucas

Matching works completely based on the cpu of_node.

---
This got broken some time ago. No cc stable as it's not really a regression, as I can't remember a single point in time when everything AC97 related would have been in place and working.

---
 sound/soc/tegra/tegra_wm9712.c | 1 -
 1 file changed, 1 deletion(-)

index 5e11963..45b5789 100644
@@ -55,7 +55,6 @@ static int tegra_wm9712_init(struct snd_soc_pcm_runtime *rtd)
 static struct snd_soc_dai_link tegra_wm9712_dai = {
 	.name = "AC97 HiFi",
 	.stream_name = "AC97 HiFi",
-	.cpu_dai_name = "tegra20-ac97",
 	.codec_dai_name = "wm9712-hifi",
 	.codec_name = "wm9712-codec",
 	.init = tegra_wm9712_init,

1.8.3.1

This was missed when splitting out the phy from the controller node in commit 9dffe3be3f32 (ARM: tegra: modify ULPI reset GPIO properties).

---
 arch/arm/boot/dts/tegra20-colibri-512.dtsi | 1 +
 1 file changed, 1 insertion(+)

index fbb52e0..92551ba 100644
@@ -457,6 +457,7 @@
 	};
 
 	usb-phy@c5004000 {
+		status = "okay";
 		nvidia,phy-reset-gpio = <&gpio TEGRA_GPIO(V, 1)
 			GPIO_ACTIVE_LOW>;
 	};

1.8.3.1

This one patch looks fine. I'll bounce it to [email protected] to be applied as a fix for 3.11.

Core and CPU voltage settings were a bit on the safe side. The actually used chips on the Colibri allow for lower voltages and work just fine this way.

SM2 is not a the parent of LDO regs, but actually the DDR regulator. The Colibri uses a different version of the TPS with other voltage mapping tables for SM2, currently we cheat by setting a fake 3,2V which results in 1,8V physical.

---
The issue with the used version of the PMIC having a different voltage mapping for SM2 should be resolved properly. As this needs some bigger adjustments at the regulator driver this quick fix is just aimed at stopping slight overvolting of the ram with 3.11 kernels. A proper fix should land in time for 3.12.

---
 arch/arm/boot/dts/tegra20-colibri-512.dtsi | 33 ++++++++++++++++--------------
 1 file changed, 18 insertions(+), 15 deletions(-)

index 92551ba..9b56dcb 100644
@@ -226,14 +226,14 @@
 			gpio-controller;
 
 			sys-supply = <&vdd_5v0_reg>;
-			vin-sm0-supply = <&sys_reg>;
-			vin-sm1-supply = <&sys_reg>;
-			vin-sm2-supply = <&sys_reg>;
-			vinldo01-supply = <&sm2_reg>;
-			vinldo23-supply = <&sm2_reg>;
-			vinldo4-supply = <&sm2_reg>;
-			vinldo678-supply = <&sm2_reg>;
-			vinldo9-supply = <&sm2_reg>;
+			vin-sm0-supply = <&vdd_5v0_reg>;
+			vin-sm1-supply = <&vdd_5v0_reg>;
+			vin-sm2-supply = <&vdd_5v0_reg>;
+			vinldo01-supply = <&vdd_5v0_reg>;
+			vinldo23-supply = <&vdd_5v0_reg>;
+			vinldo4-supply = <&vdd_5v0_reg>;
+			vinldo678-supply = <&vdd_5v0_reg>;
+			vinldo9-supply = <&vdd_5v0_reg>;
 
 			regulators {
 				#address-cells = <1>;

@@ -250,8 +250,8 @@
 					reg = <1>;
 					regulator-compatible = "sm0";
 					regulator-name = "vdd_sm0,vdd_core";
-					regulator-min-microvolt = <1275000>;
-					regulator-max-microvolt = <1275000>;
+					regulator-min-microvolt = <1225000>;
+					regulator-max-microvolt = <1225000>;
 					regulator-always-on;
 				};
 

@@ -259,17 +259,20 @@
 					reg = <2>;
 					regulator-compatible = "sm1";
 					regulator-name = "vdd_sm1,vdd_cpu";
-					regulator-min-microvolt = <1100000>;
-					regulator-max-microvolt = <1100000>;
+					regulator-min-microvolt = <1025000>;
+					regulator-max-microvolt = <1025000>;
 					regulator-always-on;
 				};
 
 				sm2_reg: regulator@3 {
 					reg = <3>;
 					regulator-compatible = "sm2";
-					regulator-name = "vdd_sm2,vin_ldo*";
-					regulator-min-microvolt = <3700000>;
-					regulator-max-microvolt = <3700000>;
+					regulator-name = "vdd_sm2,vdd_ddr";
+					/* The voltage is lying, but results
+					in the desired 1,8V on the TPS version
+					used on the Colibri */
+					regulator-min-microvolt = <3200000>;
+					regulator-max-microvolt = <3200000>;
 					regulator-always-on;
 				};
 

1.8.3.1

That sounds like a non-critical issue. Shouldn't that part be separated out into a patch for 3.12?

That's quite unfortunate. Since DT is supposed to be an ABI, the existing DT should continue to work for arbitrary kernels, and the modified DT should also work for arbitrary kernels. Clearly that isn't possible if we start putting incorrect voltage values into the DT. Isn't there some way to make an isolated fix to the PMIC driver itself so that it actually programs the HW correctly? Even if that patch is larger than this patch, it still seems more likely to be acceptable for 3.11.

But is this a regression? If not, how far back in CC: stable should this change go?

Am Dienstag, den 23.07.2013, 11:07 -0700 schrieb Stephen Warren:

Hm, yes.

I'm a bit one the fence here. One the one hand it's a severe issue as the DDR ram gets overvolted by a fair bit and should be fixed by putting the correct voltage in the DT and getting this into stable. On the other hand this will prevent older kernels from working correctly as with 1,8V set on this rail the regulator driver will just plainly refuse to probe.

Thinking again about this maybe it really the best thing to get the DT right first with CC stable and then changing the regulator driver and try and see if stable also accepts this patch. I don't really want to leave people with a supposedly working system, but constantly overvolting their RAM.

This is not a regression. It was introduced with the original Colibri T20 commit and was caused by Toradex not providing any schematics for the Module plus me not physically checking this voltage rail before pushing things out.

Regards,
Lucas

...

...

FYI, the reason I ask is that there's more push-back on adding patches late in the rc cycle (or after release, to -stable) for things that are not regressions. A safety issue like over-voltage might well still qualify to be accepted, but fixes for regressions are even more likely to be accepted.

Hi,

I ran the mainline Kernel the first time on my Colibri T20. I compiled 3.11-rc2 without any patch, but the kernel refused to boot, the boot process freezed just while probing SM2:

[    0.420075] vdd_sm2,vdd_ddr: 3700 mV

I then applied this patch, but without luck, the freeze happend even earlier:

[    0.413314] vdd_sm1,vdd_cpu: supplied by vdd_5v0

However, using 3.8 works for me:

[    0.419848] vdd_sm2,vdd_ddr: 3800 mV
[    0.423851] vdd_sm2,vdd_ddr: supplied by vdd_5v0

My Module Version is 1.2a with TPS658643 PMIC on it.

Am 2013-07-21 23:28, schrieb Lucas Stach:

This voltages seem to work for me too...

I could successfully boot with 1.8V, but with messages complaining that probe failed...

--
Stefan

This wouldn't happen to be the TPS658640? I remember a similar problem with a special version of Tamonten that had this version of the PMU which supposedly is pin and register-compatible. Well, it is register compatible alright, and the pins are the same as well, but the voltage tables are slightly different.

I did attempt to work the changes into the tps6586x driver at some point but there were a few things that made it much more difficult than I had imagined.

Thierry