Patchwork [1/4] ASOC: tegra: move AC97 clock defines to the controller node

login
register
mail settings
Submitter Lucas Stach
Date July 21, 2013, 9:28 p.m.
Message ID <1374442132-24040-2-git-send-email-dev@lynxeye.de>
Download mbox | patch
Permalink /patch/260545/
State Superseded, archived
Headers show

Comments

Lucas Stach - July 21, 2013, 9:28 p.m.
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.

Signed-off-by: Lucas Stach <dev@lynxeye.de>
---
CC: <alsa-devel@alsa-project.org>
CC: <broonie@kernel.org>
---
 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(-)
Mark Brown - July 21, 2013, 11:36 p.m.
On Sun, Jul 21, 2013 at 11:28:49PM +0200, Lucas Stach wrote:
> 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.

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?
Lucas Stach - July 22, 2013, 7:08 a.m.
Am Montag, den 22.07.2013, 00:36 +0100 schrieb Mark Brown:
> On Sun, Jul 21, 2013 at 11:28:49PM +0200, Lucas Stach wrote:
> > 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.
> 
> 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?

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

--
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
Mark Brown - July 22, 2013, 9:46 a.m.
On Mon, Jul 22, 2013 at 09:08:47AM +0200, Lucas Stach wrote:
> Am Montag, den 22.07.2013, 00:36 +0100 schrieb Mark Brown:

> > 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?

> 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.

Why is the way the other devices are doing this sensible?
Lucas Stach - July 22, 2013, 4:26 p.m.
Am Montag, den 22.07.2013, 10:46 +0100 schrieb Mark Brown:
> On Mon, Jul 22, 2013 at 09:08:47AM +0200, Lucas Stach wrote:
> > Am Montag, den 22.07.2013, 00:36 +0100 schrieb Mark Brown:
> 
> > > 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?
> 
> > 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.
> 
> Why is the way the other devices are doing this sensible?

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

--
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
Stephen Warren - July 23, 2013, 4:47 p.m.
On 07/21/2013 02:28 PM, Lucas Stach wrote:
> 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.

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.
--
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
Lucas Stach - July 23, 2013, 8:37 p.m.
Am Dienstag, den 23.07.2013, 09:47 -0700 schrieb Stephen Warren:
> On 07/21/2013 02:28 PM, Lucas Stach wrote:
> > 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.
> 
> 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.

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


--
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
Mark Brown - July 24, 2013, 9:44 a.m.
On Mon, Jul 22, 2013 at 06:26:20PM +0200, Lucas Stach wrote:
> Am Montag, den 22.07.2013, 10:46 +0100 schrieb Mark Brown:
> > On Mon, Jul 22, 2013 at 09:08:47AM +0200, Lucas Stach wrote:

> > > 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

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

> 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.

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

Patch

diff --git a/arch/arm/boot/dts/tegra20-colibri-512.dtsi b/arch/arm/boot/dts/tegra20-colibri-512.dtsi
index 2fcb3f2..fbb52e0 100644
--- a/arch/arm/boot/dts/tegra20-colibri-512.dtsi
+++ b/arch/arm/boot/dts/tegra20-colibri-512.dtsi
@@ -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 {
diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
index 9653fd8..ad13f57 100644
--- a/arch/arm/boot/dts/tegra20.dtsi
+++ b/arch/arm/boot/dts/tegra20.dtsi
@@ -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";
 	};
 
diff --git a/sound/soc/tegra/tegra20_ac97.c b/sound/soc/tegra/tegra20_ac97.c
index 6c48662..6bbffd1 100644
--- a/sound/soc/tegra/tegra20_ac97.c
+++ b/sound/soc/tegra/tegra20_ac97.c
@@ -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);