| Message ID | 1374442132-24040-2-git-send-email-dev@lynxeye.de |
|---|---|
| State | Superseded, archived |
| Headers | show |
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?
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
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?
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
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
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
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..
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);
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(-)