| Message ID | 1452865796-23527-4-git-send-email-wxt@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jan 15, 2016 at 09:49:50PM +0800, Caesar Wang wrote: > We need to claim the clock which is driving the codec so that when we > enable clock gating, we continue to clock the codec when needed. > I make this an optional clock since there might be some applications > where we don't need it but can still use the I2S block. > - As the perious discuss on https://patchwork.kernel.org/patch/5427131/, > I think Mark likes do it in codec driver, I have to say I agree this > patch in here since that's the i2s block output. I don't know if the > Mark has changed his mind. If the I2S block is providing a clock to the CODEC then that's what the software should do so that the CODEC can gate and ungate the clock as required. This patch has the I2S block using a clock, not providing one.
On Fri, Jan 15, 2016 at 9:46 AM, Mark Brown <broonie@kernel.org> wrote: > On Fri, Jan 15, 2016 at 09:49:50PM +0800, Caesar Wang wrote: > >> We need to claim the clock which is driving the codec so that when we >> enable clock gating, we continue to clock the codec when needed. >> I make this an optional clock since there might be some applications >> where we don't need it but can still use the I2S block. > >> - As the perious discuss on https://patchwork.kernel.org/patch/5427131/, >> I think Mark likes do it in codec driver, I have to say I agree this >> patch in here since that's the i2s block output. I don't know if the >> Mark has changed his mind. > > If the I2S block is providing a clock to the CODEC then that's what the > software should do so that the CODEC can gate and ungate the clock as > required. This patch has the I2S block using a clock, not providing > one. From my read of the clock diagram for RK3288 there is a single clock signal (labeled "clk_i2s0") that comes out of a fractional divider, and it is split such that one path gets sent to the I2S block and the second path is sent to a mux after which that signal is sent to an external pin that goes to the codec. There are separate clock gates for the two paths: one for the I2S block and one after that mux before the external pin. I'm not sure if it's being modeled that way in the Linux code or not, but at least physically I don't think this clock signal actually goes through the I2S block before being sent to the codec. Does that help clarify?
On Fri, Jan 15, 2016 at 01:48:04PM -0800, Sonny Rao wrote: > On Fri, Jan 15, 2016 at 9:46 AM, Mark Brown <broonie@kernel.org> wrote: > > If the I2S block is providing a clock to the CODEC then that's what the > > software should do so that the CODEC can gate and ungate the clock as > > required. This patch has the I2S block using a clock, not providing > > one. > From my read of the clock diagram for RK3288 there is a single clock > signal (labeled "clk_i2s0") that comes out of a fractional divider, > and it is split such that one path gets sent to the I2S block and the > second path is sent to a mux after which that signal is sent to an > external pin that goes to the codec. > There are separate clock gates for the two paths: one for the I2S > block and one after that mux before the external pin. > I'm not sure if it's being modeled that way in the Linux code or not, > but at least physically I don't think this clock signal actually goes > through the I2S block before being sent to the codec. That's not really the issue here, the issue is that it's not the I2S controller that is consuming the clock so it should not be the I2S controller driver that ensures that the clock is enabled. The driver that manages the clock should be the one that uses it, like I say this means you should add the code to enable the clock to the CODEC driver if the CODEC driver needs the clock enabled. > Does that help clarify? The problem here isn't a lack of clarity in the situation.
Hi Mark ? 23/01/2016 01:18, Mark Brown ??: > On Fri, Jan 15, 2016 at 01:48:04PM -0800, Sonny Rao wrote: >> On Fri, Jan 15, 2016 at 9:46 AM, Mark Brown <broonie@kernel.org> wrote: > >>> If the I2S block is providing a clock to the CODEC then that's what the >>> software should do so that the CODEC can gate and ungate the clock as >>> required. This patch has the I2S block using a clock, not providing >>> one. > >> From my read of the clock diagram for RK3288 there is a single clock >> signal (labeled "clk_i2s0") that comes out of a fractional divider, >> and it is split such that one path gets sent to the I2S block and the >> second path is sent to a mux after which that signal is sent to an >> external pin that goes to the codec. > >> There are separate clock gates for the two paths: one for the I2S >> block and one after that mux before the external pin. > >> I'm not sure if it's being modeled that way in the Linux code or not, >> but at least physically I don't think this clock signal actually goes >> through the I2S block before being sent to the codec. > > That's not really the issue here, the issue is that it's not the I2S > controller that is consuming the clock so it should not be the I2S > controller driver that ensures that the clock is enabled. The driver > that manages the clock should be the one that uses it, like I say this > means you should add the code to enable the clock to the CODEC driver if > the CODEC driver needs the clock enabled. > Agree, now we almost use the simple-card for the CODEC driver, so I think we should enable the mclk(i2s-outclk) in the simple-card driver, is it ? I found a subnode property from simple-card document: - mclk-fs : Multiplication factor between stream rate and codec mclk, applied only for the dai-link. But the property responsible to the factor, not care if the mclk source clock is enabled or not. So does the simple-card driver can add support to enable/disable mclk ? >> Does that help clarify? > > The problem here isn't a lack of clarity in the situation. >
Hi Jay, Am Montag, 25. Januar 2016, 09:15:31 schrieb Jianqun Xu: > ? 23/01/2016 01:18, Mark Brown ??: > > On Fri, Jan 15, 2016 at 01:48:04PM -0800, Sonny Rao wrote: > >> On Fri, Jan 15, 2016 at 9:46 AM, Mark Brown <broonie@kernel.org> wrote: > >>> If the I2S block is providing a clock to the CODEC then that's what the > >>> software should do so that the CODEC can gate and ungate the clock as > >>> required. This patch has the I2S block using a clock, not providing > >>> one. > >>> > >> From my read of the clock diagram for RK3288 there is a single clock > >> > >> signal (labeled "clk_i2s0") that comes out of a fractional divider, > >> and it is split such that one path gets sent to the I2S block and the > >> second path is sent to a mux after which that signal is sent to an > >> external pin that goes to the codec. > >> > >> There are separate clock gates for the two paths: one for the I2S > >> block and one after that mux before the external pin. > >> > >> I'm not sure if it's being modeled that way in the Linux code or not, > >> but at least physically I don't think this clock signal actually goes > >> through the I2S block before being sent to the codec. > > > > That's not really the issue here, the issue is that it's not the I2S > > controller that is consuming the clock so it should not be the I2S > > controller driver that ensures that the clock is enabled. The driver > > that manages the clock should be the one that uses it, like I say this > > means you should add the code to enable the clock to the CODEC driver if > > the CODEC driver needs the clock enabled. > > Agree, now we almost use the simple-card for the CODEC driver, so I > think we should enable the mclk(i2s-outclk) in the simple-card driver, > is it ? > > I found a subnode property from simple-card document: > - mclk-fs : Multiplication factor between > stream > rate and codec mclk, applied > only for > the dai-link. > But the property responsible to the factor, not care if the mclk source > clock is enabled or not. So does the simple-card driver can add support > to enable/disable mclk ? The mclk-input is part of the codec I'd think. So you'd want the clocks- property in the i2c entry describing the codec itself and implement the clk operations in the codec driver as well. See codec-drivers for da7213, da7218,max98090 and many more for reference. Heiko > >> Does that help clarify? > > > > The problem here isn't a lack of clarity in the situation.
On Tue, Jan 26, 2016 at 10:52:25AM +0100, Heiko Stübner wrote: > Am Montag, 25. Januar 2016, 09:15:31 schrieb Jianqun Xu: > > ? 23/01/2016 01:18, Mark Brown ??: > > > controller driver that ensures that the clock is enabled. The driver > > > that manages the clock should be the one that uses it, like I say this > > > means you should add the code to enable the clock to the CODEC driver if > > > the CODEC driver needs the clock enabled. > > But the property responsible to the factor, not care if the mclk source > > clock is enabled or not. So does the simple-card driver can add support > > to enable/disable mclk ? > The mclk-input is part of the codec I'd think. So you'd want the clocks- > property in the i2c entry describing the codec itself and implement the clk > operations in the codec driver as well. > See codec-drivers for da7213, da7218,max98090 and many more for reference. Yes, as I have said repeatedly including in the mail quoted above the CODEC is using the clock so should be managing it.
diff --git a/sound/soc/rockchip/rockchip_i2s.c b/sound/soc/rockchip/rockchip_i2s.c index 58ee645..5889bba 100644 --- a/sound/soc/rockchip/rockchip_i2s.c +++ b/sound/soc/rockchip/rockchip_i2s.c @@ -28,6 +28,7 @@ struct rk_i2s_dev { struct clk *hclk; struct clk *mclk; + struct clk *oclk; struct snd_dmaengine_dai_dma_data capture_dma_data; struct snd_dmaengine_dai_dma_data playback_dma_data; @@ -47,6 +48,9 @@ static int i2s_runtime_suspend(struct device *dev) { struct rk_i2s_dev *i2s = dev_get_drvdata(dev); + if (i2s->oclk) + clk_disable_unprepare(i2s->oclk); + clk_disable_unprepare(i2s->mclk); return 0; @@ -63,6 +67,14 @@ static int i2s_runtime_resume(struct device *dev) return ret; } + if (i2s->oclk) { + ret = clk_prepare_enable(i2s->oclk); + if (ret) { + dev_err(i2s->dev, "oclk enable failed %d\n", ret); + return ret; + } + } + return 0; } @@ -480,6 +492,15 @@ static int rockchip_i2s_probe(struct platform_device *pdev) return PTR_ERR(i2s->mclk); } + i2s->oclk = devm_clk_get(&pdev->dev, "i2s_clk_out"); + if (IS_ERR(i2s->oclk)) { + dev_dbg(&pdev->dev, "Didn't find output clock\n"); + i2s->oclk = NULL; + } + + if (i2s->oclk) + ret = clk_prepare_enable(i2s->oclk); + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); regs = devm_ioremap_resource(&pdev->dev, res); if (IS_ERR(regs)) @@ -552,6 +573,9 @@ static int rockchip_i2s_remove(struct platform_device *pdev) if (!pm_runtime_status_suspended(&pdev->dev)) i2s_runtime_suspend(&pdev->dev); + if (i2s->oclk) + clk_disable_unprepare(i2s->oclk); + clk_disable_unprepare(i2s->mclk); clk_disable_unprepare(i2s->hclk);