* [PATCH 0/2] rockchip: i2s: patches to add optional clock output from @ 2014-12-02 14:49 Jianqun Xu 2014-12-02 14:49 ` [PATCH 1/2] ASoC: rockchip-i2s: dt: add an optional property "i2s_clk_out" Jianqun Xu ` (2 more replies) 0 siblings, 3 replies; 17+ messages in thread From: Jianqun Xu @ 2014-12-02 14:49 UTC (permalink / raw) To: lgirdwood, broonie, perex, tiwai, jay.xu, sonnyrao, heiko Cc: linux-arm-kernel, alsa-devel, linux-kernel, devicetree, linux-rockchip The clock output from rockchip SoCs' CRU to chip outside is from the same source as hclk which is for i2s controller. We make it optional because some audio codec outside not need the master clock from SoC but from other clock source. Jianqun Xu (2): ASoC: rockchip-i2s: dt: add an optional property "i2s_clk_out" ASoC: rockchip: i2s: add support for grabbing output clock to codec Documentation/devicetree/bindings/sound/rockchip-i2s.txt | 8 ++++++-- sound/soc/rockchip/rockchip_i2s.c | 11 +++++++++++ 2 files changed, 17 insertions(+), 2 deletions(-) -- 1.9.1 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/2] ASoC: rockchip-i2s: dt: add an optional property "i2s_clk_out" 2014-12-02 14:49 [PATCH 0/2] rockchip: i2s: patches to add optional clock output from Jianqun Xu @ 2014-12-02 14:49 ` Jianqun Xu 2014-12-02 17:57 ` Doug Anderson 2014-12-02 14:52 ` [PATCH 2/2] ASoC: rockchip: i2s: add support for grabbing output clock to codec Jianqun Xu 2014-12-03 7:18 ` [PATCH v2 0/2] ASoC: rockchip: i2s: driver could output "i2s-clk-out" optionally Jianqun Xu 2 siblings, 1 reply; 17+ messages in thread From: Jianqun Xu @ 2014-12-02 14:49 UTC (permalink / raw) To: lgirdwood, broonie, perex, tiwai, jay.xu, sonnyrao, heiko Cc: linux-arm-kernel, alsa-devel, linux-kernel, devicetree, linux-rockchip Add an property "i2s_clk_out", which enables to output clock to outside of rockchip SoCs. Let's make it optional since not each board needs it. Signed-off-by: Jianqun Xu <jay.xu@rock-chips.com> --- Documentation/devicetree/bindings/sound/rockchip-i2s.txt | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/Documentation/devicetree/bindings/sound/rockchip-i2s.txt b/Documentation/devicetree/bindings/sound/rockchip-i2s.txt index 9b82c20..c8f5ac0 100644 --- a/Documentation/devicetree/bindings/sound/rockchip-i2s.txt +++ b/Documentation/devicetree/bindings/sound/rockchip-i2s.txt @@ -22,6 +22,10 @@ Required properties: - "i2s_hclk": clock for I2S BUS - "i2s_clk" : clock for I2S controller +Optional property: +- "i2s_clk_out" : clock output to outside of chip, usally as master clock for audio + codec chip + Example for rk3288 I2S controller: i2s@ff890000 { @@ -32,6 +36,6 @@ i2s@ff890000 { #size-cells = <0>; dmas = <&pdma1 0>, <&pdma1 1>; dma-names = "tx", "rx"; - clock-names = "i2s_hclk", "i2s_clk"; - clocks = <&cru HCLK_I2S0>, <&cru SCLK_I2S0>; + clock-names = "i2s_hclk", "i2s_clk", "i2s_clk_out"; + clocks = <&cru HCLK_I2S0>, <&cru SCLK_I2S0>, <&cru SCLK_I2S0_CLKOUT>; }; -- 1.9.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] ASoC: rockchip-i2s: dt: add an optional property "i2s_clk_out" 2014-12-02 14:49 ` [PATCH 1/2] ASoC: rockchip-i2s: dt: add an optional property "i2s_clk_out" Jianqun Xu @ 2014-12-02 17:57 ` Doug Anderson 0 siblings, 0 replies; 17+ messages in thread From: Doug Anderson @ 2014-12-02 17:57 UTC (permalink / raw) To: Jianqun Xu Cc: Liam Girdwood, broonie, perex, tiwai, Sonny Rao, Heiko Stübner, linux-arm-kernel, alsa-devel, linux-kernel, devicetree, open list:ARM/Rockchip SoC... Jianqun, On Tue, Dec 2, 2014 at 6:49 AM, Jianqun Xu <jay.xu@rock-chips.com> wrote: > Add an property "i2s_clk_out", which enables to output clock to outside > of rockchip SoCs. Let's make it optional since not each board needs it. > > Signed-off-by: Jianqun Xu <jay.xu@rock-chips.com> > --- > Documentation/devicetree/bindings/sound/rockchip-i2s.txt | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) You forgot to mention that this is a v3 in the subject line and forgot to mention what changed between v2 and v3 (nothing changed, but you should say that). Despite that, this patch looks reasonable to me. Reviewed-by: Doug Anderson <dianders@chromium.org> ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 2/2] ASoC: rockchip: i2s: add support for grabbing output clock to codec 2014-12-02 14:49 [PATCH 0/2] rockchip: i2s: patches to add optional clock output from Jianqun Xu 2014-12-02 14:49 ` [PATCH 1/2] ASoC: rockchip-i2s: dt: add an optional property "i2s_clk_out" Jianqun Xu @ 2014-12-02 14:52 ` Jianqun Xu 2014-12-02 17:54 ` Doug Anderson 2014-12-03 7:18 ` [PATCH v2 0/2] ASoC: rockchip: i2s: driver could output "i2s-clk-out" optionally Jianqun Xu 2 siblings, 1 reply; 17+ messages in thread From: Jianqun Xu @ 2014-12-02 14:52 UTC (permalink / raw) To: lgirdwood, broonie, perex, tiwai, jay.xu, sonnyrao, heiko Cc: linux-arm-kernel, alsa-devel, linux-kernel, devicetree, linux-rockchip From: Sonny Rao <sonnyrao@chromium.org> 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. Signed-off-by: Sonny Rao <sonnyrao@chromium.org> --- sound/soc/rockchip/rockchip_i2s.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/sound/soc/rockchip/rockchip_i2s.c b/sound/soc/rockchip/rockchip_i2s.c index c74ba37..2820ade 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; @@ -439,6 +440,14 @@ 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)) @@ -505,6 +514,8 @@ 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); snd_dmaengine_pcm_unregister(&pdev->dev); -- 1.9.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] ASoC: rockchip: i2s: add support for grabbing output clock to codec 2014-12-02 14:52 ` [PATCH 2/2] ASoC: rockchip: i2s: add support for grabbing output clock to codec Jianqun Xu @ 2014-12-02 17:54 ` Doug Anderson 2014-12-03 1:03 ` Jianqun 0 siblings, 1 reply; 17+ messages in thread From: Doug Anderson @ 2014-12-02 17:54 UTC (permalink / raw) To: Jianqun Xu Cc: Liam Girdwood, broonie, perex, tiwai, Sonny Rao, Heiko Stübner, linux-arm-kernel, alsa-devel, linux-kernel, devicetree, open list:ARM/Rockchip SoC... Jianqun, This ought to be a "v3" patch and ideally ought to describe differences from v2 (after the cut). Please have Kever or Chris review your next patch before sending it out since I think they are familiar with the process. On Tue, Dec 2, 2014 at 6:52 AM, Jianqun Xu <jay.xu@rock-chips.com> wrote: > From: Sonny Rao <sonnyrao@chromium.org> > > 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. > > Signed-off-by: Sonny Rao <sonnyrao@chromium.org> You still forgot your own signed-off-by. Please try again. See <https://patchwork.kernel.org/patch/5334991/> > + 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; > + } You still forgot the blank line here requested by Heiko. Please try again. See <https://patchwork.kernel.org/patch/5334991/> > + if (i2s->oclk) > + ret = clk_prepare_enable(i2s->oclk); > + ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] ASoC: rockchip: i2s: add support for grabbing output clock to codec 2014-12-02 17:54 ` Doug Anderson @ 2014-12-03 1:03 ` Jianqun 2014-12-03 5:18 ` Doug Anderson 0 siblings, 1 reply; 17+ messages in thread From: Jianqun @ 2014-12-03 1:03 UTC (permalink / raw) To: Doug Anderson, Jianqun Xu Cc: xjq, Liam Girdwood, broonie, perex, tiwai, Sonny Rao, Heiko Stübner, linux-arm-kernel, alsa-devel, linux-kernel, devicetree, open list:ARM/Rockchip SoC... Hi Doug: 在 12/03/2014 01:54 AM, Doug Anderson 写道: > Jianqun, > > This ought to be a "v3" patch and ideally ought to describe > differences from v2 (after the cut). Please have Kever or Chris > review your next patch before sending it out since I think they are > familiar with the process. > > > On Tue, Dec 2, 2014 at 6:52 AM, Jianqun Xu <jay.xu@rock-chips.com> wrote: >> From: Sonny Rao <sonnyrao@chromium.org> >> >> 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. >> >> Signed-off-by: Sonny Rao <sonnyrao@chromium.org> > > You still forgot your own signed-off-by. Please try again. See > <https://patchwork.kernel.org/patch/5334991/> ok, I will add my signed-off-by > > >> + 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; >> + } > > You still forgot the blank line here requested by Heiko. Please try > again. See <https://patchwork.kernel.org/patch/5334991/> Although I thought there needn't a blank ~, I wll add it next patch > > >> + if (i2s->oclk) >> + ret = clk_prepare_enable(i2s->oclk); >> + > > > -- Jianqun Xu **************************************************************************** *IMPORTANT NOTICE:*This email is from Fuzhou Rockchip Electronics Co., Ltd .The contents of this email and any attachments may contain information that is privileged, confidential and/or exempt from disclosure under applicable law and relevant NDA. If you are not the intended recipient, you are hereby notified that any disclosure, copying, distribution, or use of the information is STRICTLY PROHIBITED. Please immediately contact the sender as soon as possible and destroy the material in its entirety in any format. Thank you. **************************************************************************** ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] ASoC: rockchip: i2s: add support for grabbing output clock to codec 2014-12-03 1:03 ` Jianqun @ 2014-12-03 5:18 ` Doug Anderson 2014-12-03 12:55 ` Mark Brown 0 siblings, 1 reply; 17+ messages in thread From: Doug Anderson @ 2014-12-03 5:18 UTC (permalink / raw) To: Jianqun Cc: Jianqun Xu, Liam Girdwood, broonie, perex, tiwai, Sonny Rao, Heiko Stübner, linux-arm-kernel, alsa-devel, linux-kernel, devicetree, open list:ARM/Rockchip SoC... On Tue, Dec 2, 2014 at 5:03 PM, Jianqun <xjq@rock-chips.com> wrote: > Hi Doug: > > 在 12/03/2014 01:54 AM, Doug Anderson 写道: >> Jianqun, >> >> This ought to be a "v3" patch and ideally ought to describe >> differences from v2 (after the cut). Please have Kever or Chris >> review your next patch before sending it out since I think they are >> familiar with the process. You still seem to be missing versions in your subject line and missing info on what changed version to version. The latest version you just sent (v4?) is still missing it. Unless Mark says so then I don't think you need to spin this series, but please try to add that in the future. >> You still forgot the blank line here requested by Heiko. Please try >> again. See <https://patchwork.kernel.org/patch/5334991/> > Although I thought there needn't a blank ~, I wll add it next patch That would have been OK, but in that case you should have responded to Heiko's patch and explained that you weren't taking his feedback (and why you weren't). -Doug ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] ASoC: rockchip: i2s: add support for grabbing output clock to codec 2014-12-03 5:18 ` Doug Anderson @ 2014-12-03 12:55 ` Mark Brown 0 siblings, 0 replies; 17+ messages in thread From: Mark Brown @ 2014-12-03 12:55 UTC (permalink / raw) To: Doug Anderson Cc: Jianqun, Jianqun Xu, Liam Girdwood, perex, tiwai, Sonny Rao, Heiko Stübner, linux-arm-kernel, alsa-devel, linux-kernel, devicetree, open list:ARM/Rockchip SoC... [-- Attachment #1: Type: text/plain, Size: 752 bytes --] On Tue, Dec 02, 2014 at 09:18:35PM -0800, Doug Anderson wrote: > You still seem to be missing versions in your subject line and missing > info on what changed version to version. The latest version you just > sent (v4?) is still missing it. > Unless Mark says so then I don't think you need to spin this series, > but please try to add that in the future. Don't resubmit. I actually don't care much about any of that stuff and mostly ignore it unless the change set is saying something other than "addressed review comments". One thing you should avoid doing is posting new versions in reply to old versions as has been done here, that just makes for a confusing and hard to follow mail archive with new serieses buried in the middle of threads. [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 473 bytes --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 0/2] ASoC: rockchip: i2s: driver could output "i2s-clk-out" optionally 2014-12-02 14:49 [PATCH 0/2] rockchip: i2s: patches to add optional clock output from Jianqun Xu 2014-12-02 14:49 ` [PATCH 1/2] ASoC: rockchip-i2s: dt: add an optional property "i2s_clk_out" Jianqun Xu 2014-12-02 14:52 ` [PATCH 2/2] ASoC: rockchip: i2s: add support for grabbing output clock to codec Jianqun Xu @ 2014-12-03 7:18 ` Jianqun Xu 2014-12-03 7:18 ` [PATCH v2 1/2] ASoC: dt-bindings: rockchip-i2s: add an optional property "i2s_clk_out" Jianqun Xu 2014-12-03 7:18 ` [PATCH v2 2/2] ASoC: rockchip: i2s: add support for grabbing output clock to codec Jianqun Xu 2 siblings, 2 replies; 17+ messages in thread From: Jianqun Xu @ 2014-12-03 7:18 UTC (permalink / raw) To: lgirdwood, broonie, perex, tiwai, sonnyrao, heiko Cc: linux-arm-kernel, alsa-devel, linux-kernel, devicetree, linux-rockchip, Jianqun Xu Clock generate unit in rockchip SoCs could generate clock named "i2s-clk-out" separate from clock named "i2s-hclk", which is for i2s controller. Since there may not need the "i2s-clk-out" outside, we make it optional in driver, that could be decided in dts by add "i2s-clk-out" or not. Tested on devices based on rk3288 with max98090 codec. Jianqun Xu (1): ASoC: dt-bindings: rockchip-i2s: add an optional property "i2s_clk_out" Sonny Rao (1): ASoC: rockchip: i2s: add support for grabbing output clock to codec Documentation/devicetree/bindings/sound/rockchip-i2s.txt | 8 ++++++-- sound/soc/rockchip/rockchip_i2s.c | 13 +++++++++++++ 2 files changed, 19 insertions(+), 2 deletions(-) -- changes since v1: - rework with patches commit message, reference to old commit message of the same related files. - generate patch again by apply sonny's patch - fix blank line in driver, suggested by Heiko and Doug, so add my owner singed-off-by 1.9.1 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 1/2] ASoC: dt-bindings: rockchip-i2s: add an optional property "i2s_clk_out" 2014-12-03 7:18 ` [PATCH v2 0/2] ASoC: rockchip: i2s: driver could output "i2s-clk-out" optionally Jianqun Xu @ 2014-12-03 7:18 ` Jianqun Xu 2014-12-03 7:18 ` [PATCH v2 2/2] ASoC: rockchip: i2s: add support for grabbing output clock to codec Jianqun Xu 1 sibling, 0 replies; 17+ messages in thread From: Jianqun Xu @ 2014-12-03 7:18 UTC (permalink / raw) To: lgirdwood, broonie, perex, tiwai, sonnyrao, heiko Cc: linux-arm-kernel, alsa-devel, linux-kernel, devicetree, linux-rockchip, Jianqun Xu Add an property "i2s_clk_out", which enables to output clock to outside of rockchip SoCs. Let's make it optional since not each board needs it. Signed-off-by: Jianqun Xu <jay.xu@rock-chips.com> Reviewed-by: Doug Anderson <dianders@chromium.org> --- Documentation/devicetree/bindings/sound/rockchip-i2s.txt | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/Documentation/devicetree/bindings/sound/rockchip-i2s.txt b/Documentation/devicetree/bindings/sound/rockchip-i2s.txt index 9b82c20..c8f5ac0 100644 --- a/Documentation/devicetree/bindings/sound/rockchip-i2s.txt +++ b/Documentation/devicetree/bindings/sound/rockchip-i2s.txt @@ -22,6 +22,10 @@ Required properties: - "i2s_hclk": clock for I2S BUS - "i2s_clk" : clock for I2S controller +Optional property: +- "i2s_clk_out" : clock output to outside of chip, usally as master clock for audio + codec chip + Example for rk3288 I2S controller: i2s@ff890000 { @@ -32,6 +36,6 @@ i2s@ff890000 { #size-cells = <0>; dmas = <&pdma1 0>, <&pdma1 1>; dma-names = "tx", "rx"; - clock-names = "i2s_hclk", "i2s_clk"; - clocks = <&cru HCLK_I2S0>, <&cru SCLK_I2S0>; + clock-names = "i2s_hclk", "i2s_clk", "i2s_clk_out"; + clocks = <&cru HCLK_I2S0>, <&cru SCLK_I2S0>, <&cru SCLK_I2S0_CLKOUT>; }; -- 1.9.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 2/2] ASoC: rockchip: i2s: add support for grabbing output clock to codec 2014-12-03 7:18 ` [PATCH v2 0/2] ASoC: rockchip: i2s: driver could output "i2s-clk-out" optionally Jianqun Xu 2014-12-03 7:18 ` [PATCH v2 1/2] ASoC: dt-bindings: rockchip-i2s: add an optional property "i2s_clk_out" Jianqun Xu @ 2014-12-03 7:18 ` Jianqun Xu 2014-12-03 19:20 ` Mark Brown 1 sibling, 1 reply; 17+ messages in thread From: Jianqun Xu @ 2014-12-03 7:18 UTC (permalink / raw) To: lgirdwood, broonie, perex, tiwai, sonnyrao, heiko Cc: linux-arm-kernel, alsa-devel, linux-kernel, devicetree, linux-rockchip, Jianqun Xu From: Sonny Rao <sonnyrao@chromium.org> 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. Signed-off-by: Sonny Rao <sonnyrao@chromium.org> Signed-off-by: Jianqun Xu <jay.xu@rock-chips.com> --- sound/soc/rockchip/rockchip_i2s.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/sound/soc/rockchip/rockchip_i2s.c b/sound/soc/rockchip/rockchip_i2s.c index c74ba37..fd144ac 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; @@ -439,6 +440,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)) @@ -505,6 +515,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); snd_dmaengine_pcm_unregister(&pdev->dev); -- 1.9.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/2] ASoC: rockchip: i2s: add support for grabbing output clock to codec 2014-12-03 7:18 ` [PATCH v2 2/2] ASoC: rockchip: i2s: add support for grabbing output clock to codec Jianqun Xu @ 2014-12-03 19:20 ` Mark Brown 2014-12-03 19:38 ` Sonny Rao 0 siblings, 1 reply; 17+ messages in thread From: Mark Brown @ 2014-12-03 19:20 UTC (permalink / raw) To: Jianqun Xu Cc: lgirdwood, perex, tiwai, sonnyrao, heiko, linux-arm-kernel, alsa-devel, linux-kernel, devicetree, linux-rockchip [-- Attachment #1: Type: text/plain, Size: 752 bytes --] On Wed, Dec 03, 2014 at 03:18:38PM +0800, Jianqun Xu wrote: > From: Sonny Rao <sonnyrao@chromium.org> > > 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. I would expect that the clock for the CODEC should be managed by the CODEC if at all possible - that seems more logical than having the CPU I2S controller request and manage it if it's a separate clock. Why add this to the CPU side driver? We've not always done this for older systems due to the lack of a usable clock API but that's starting to be addressed. [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 473 bytes --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/2] ASoC: rockchip: i2s: add support for grabbing output clock to codec 2014-12-03 19:20 ` Mark Brown @ 2014-12-03 19:38 ` Sonny Rao 2014-12-03 20:03 ` Mark Brown 0 siblings, 1 reply; 17+ messages in thread From: Sonny Rao @ 2014-12-03 19:38 UTC (permalink / raw) To: Mark Brown Cc: Jianqun Xu, Liam Girdwood, perex, tiwai, Heiko Stübner, linux-arm-kernel, alsa-devel, linux-kernel, devicetree, linux-rockchip On Wed, Dec 3, 2014 at 11:20 AM, Mark Brown <broonie@kernel.org> wrote: > On Wed, Dec 03, 2014 at 03:18:38PM +0800, Jianqun Xu wrote: >> From: Sonny Rao <sonnyrao@chromium.org> >> >> 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. > > I would expect that the clock for the CODEC should be managed by the > CODEC if at all possible - that seems more logical than having the CPU > I2S controller request and manage it if it's a separate clock. Why add > this to the CPU side driver? It's a good question. Right now the way I'm running this stuff we're mostly setting all the i2s the clock rates from the codec driver, but I think this isn't the correct way to go, and the i2s driver needs to set it's rate based on the hw params, but that isn't happening (yet). This output clock has a mux and can either be a fixed 12Mhz output or can be derived from the same fractional divider which drives the i2s block. I thought it was simpler to keep them all the same, but need to put ownership in the i2s in anticipation of the i2s driver setting it's own clock rate. If you think this is an implementation detail and this output clock should just be owned by the codec driver, even though I'm guessing it will just have to be the same as i2s, then I think we can drop this and make sure simple card (or whatever other codec driver) claims this clock. > > We've not always done this for older systems due to the lack of a usable > clock API but that's starting to be addressed. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/2] ASoC: rockchip: i2s: add support for grabbing output clock to codec 2014-12-03 19:38 ` Sonny Rao @ 2014-12-03 20:03 ` Mark Brown 2014-12-03 23:03 ` Sonny Rao 0 siblings, 1 reply; 17+ messages in thread From: Mark Brown @ 2014-12-03 20:03 UTC (permalink / raw) To: Sonny Rao Cc: Jianqun Xu, Liam Girdwood, perex, tiwai, Heiko Stübner, linux-arm-kernel, alsa-devel, linux-kernel, devicetree, linux-rockchip [-- Attachment #1: Type: text/plain, Size: 1325 bytes --] On Wed, Dec 03, 2014 at 11:38:13AM -0800, Sonny Rao wrote: > On Wed, Dec 3, 2014 at 11:20 AM, Mark Brown <broonie@kernel.org> wrote: > > I would expect that the clock for the CODEC should be managed by the > > CODEC if at all possible - that seems more logical than having the CPU > > I2S controller request and manage it if it's a separate clock. Why add > > this to the CPU side driver? > This output clock has a mux and can either be a fixed 12Mhz output or > can be derived from the same fractional divider which drives the i2s > block. I thought it was simpler to keep them all the same, but need > to put ownership in the i2s in anticipation of the i2s driver setting > it's own clock rate. > If you think this is an implementation detail and this output clock > should just be owned by the codec driver, even though I'm guessing it > will just have to be the same as i2s, then I think we can drop this > and make sure simple card (or whatever other codec driver) claims this > clock. simple-card obviously isn't a CODEC driver... For generality I think the clock does need to be exposed to the CODEC driver, otherwise this will work differently to how other systems are working and we can't substitute in a different clock on the CODEC side so easily if it doesn't happen to use the output from the I2S block. [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 473 bytes --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/2] ASoC: rockchip: i2s: add support for grabbing output clock to codec 2014-12-03 20:03 ` Mark Brown @ 2014-12-03 23:03 ` Sonny Rao 2014-12-03 23:22 ` [alsa-devel] " Dylan Reid 0 siblings, 1 reply; 17+ messages in thread From: Sonny Rao @ 2014-12-03 23:03 UTC (permalink / raw) To: Mark Brown Cc: Jianqun Xu, Liam Girdwood, perex, tiwai, Heiko Stübner, linux-arm-kernel, alsa-devel, linux-kernel, devicetree, linux-rockchip On Wed, Dec 3, 2014 at 12:03 PM, Mark Brown <broonie@kernel.org> wrote: > On Wed, Dec 03, 2014 at 11:38:13AM -0800, Sonny Rao wrote: >> On Wed, Dec 3, 2014 at 11:20 AM, Mark Brown <broonie@kernel.org> wrote: > >> > I would expect that the clock for the CODEC should be managed by the >> > CODEC if at all possible - that seems more logical than having the CPU >> > I2S controller request and manage it if it's a separate clock. Why add >> > this to the CPU side driver? > >> This output clock has a mux and can either be a fixed 12Mhz output or >> can be derived from the same fractional divider which drives the i2s >> block. I thought it was simpler to keep them all the same, but need >> to put ownership in the i2s in anticipation of the i2s driver setting >> it's own clock rate. > >> If you think this is an implementation detail and this output clock >> should just be owned by the codec driver, even though I'm guessing it >> will just have to be the same as i2s, then I think we can drop this >> and make sure simple card (or whatever other codec driver) claims this >> clock. > > simple-card obviously isn't a CODEC driver... Yeah, sorry. > For generality I think > the clock does need to be exposed to the CODEC driver, otherwise this > will work differently to how other systems are working and we can't > substitute in a different clock on the CODEC side so easily if it > doesn't happen to use the output from the I2S block. Ok, then I think what we will do is abandon this patch and I will send something that adds this functionality to the particular codec that I'm interested in -- max98090. I'm a little tied up at the moment so I'm not going to send that for a little while, but will come eventually. Thanks for the advice! ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [alsa-devel] [PATCH v2 2/2] ASoC: rockchip: i2s: add support for grabbing output clock to codec 2014-12-03 23:03 ` Sonny Rao @ 2014-12-03 23:22 ` Dylan Reid 2014-12-03 23:52 ` Sonny Rao 0 siblings, 1 reply; 17+ messages in thread From: Dylan Reid @ 2014-12-03 23:22 UTC (permalink / raw) To: Sonny Rao Cc: Mark Brown, devicetree, alsa-devel, Heiko Stübner, Takashi Iwai, linux-kernel, Liam Girdwood, linux-rockchip, Jianqun Xu, linux-arm-kernel On Wed, Dec 3, 2014 at 3:03 PM, Sonny Rao <sonnyrao@chromium.org> wrote: > On Wed, Dec 3, 2014 at 12:03 PM, Mark Brown <broonie@kernel.org> wrote: >> On Wed, Dec 03, 2014 at 11:38:13AM -0800, Sonny Rao wrote: >>> On Wed, Dec 3, 2014 at 11:20 AM, Mark Brown <broonie@kernel.org> wrote: >> >>> > I would expect that the clock for the CODEC should be managed by the >>> > CODEC if at all possible - that seems more logical than having the CPU >>> > I2S controller request and manage it if it's a separate clock. Why add >>> > this to the CPU side driver? >> >>> This output clock has a mux and can either be a fixed 12Mhz output or >>> can be derived from the same fractional divider which drives the i2s >>> block. I thought it was simpler to keep them all the same, but need >>> to put ownership in the i2s in anticipation of the i2s driver setting >>> it's own clock rate. >> >>> If you think this is an implementation detail and this output clock >>> should just be owned by the codec driver, even though I'm guessing it >>> will just have to be the same as i2s, then I think we can drop this >>> and make sure simple card (or whatever other codec driver) claims this >>> clock. >> >> simple-card obviously isn't a CODEC driver... > > Yeah, sorry. > >> For generality I think >> the clock does need to be exposed to the CODEC driver, otherwise this >> will work differently to how other systems are working and we can't >> substitute in a different clock on the CODEC side so easily if it >> doesn't happen to use the output from the I2S block. > > Ok, then I think what we will do is abandon this patch and I will send > something that adds this functionality to the particular codec that > I'm interested in -- max98090. Sorry I didn't read this earlier. I don't think that this belongs in the max98090. The original patch description is a bit confusing. The clock being grabbed here is actually i2s mclk. My understanding is that, on this SoC, the mclk is driven from a different IP block than the rest of the i2s signals. The i2s driver needs to be told about the clock and enable/disable it at the appropriate times. I'm assuming it's optional because there are boards using this SoC with i2s slave mode that don't drive mclk at all. Please correct me if I'm wrong on any of the above. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [alsa-devel] [PATCH v2 2/2] ASoC: rockchip: i2s: add support for grabbing output clock to codec 2014-12-03 23:22 ` [alsa-devel] " Dylan Reid @ 2014-12-03 23:52 ` Sonny Rao 0 siblings, 0 replies; 17+ messages in thread From: Sonny Rao @ 2014-12-03 23:52 UTC (permalink / raw) To: Dylan Reid Cc: Mark Brown, devicetree, alsa-devel, Heiko Stübner, Takashi Iwai, linux-kernel, Liam Girdwood, linux-rockchip, Jianqun Xu, linux-arm-kernel On Wed, Dec 3, 2014 at 3:22 PM, Dylan Reid <dgreid@chromium.org> wrote: > On Wed, Dec 3, 2014 at 3:03 PM, Sonny Rao <sonnyrao@chromium.org> wrote: >> On Wed, Dec 3, 2014 at 12:03 PM, Mark Brown <broonie@kernel.org> wrote: >>> On Wed, Dec 03, 2014 at 11:38:13AM -0800, Sonny Rao wrote: >>>> On Wed, Dec 3, 2014 at 11:20 AM, Mark Brown <broonie@kernel.org> wrote: >>> >>>> > I would expect that the clock for the CODEC should be managed by the >>>> > CODEC if at all possible - that seems more logical than having the CPU >>>> > I2S controller request and manage it if it's a separate clock. Why add >>>> > this to the CPU side driver? >>> >>>> This output clock has a mux and can either be a fixed 12Mhz output or >>>> can be derived from the same fractional divider which drives the i2s >>>> block. I thought it was simpler to keep them all the same, but need >>>> to put ownership in the i2s in anticipation of the i2s driver setting >>>> it's own clock rate. >>> >>>> If you think this is an implementation detail and this output clock >>>> should just be owned by the codec driver, even though I'm guessing it >>>> will just have to be the same as i2s, then I think we can drop this >>>> and make sure simple card (or whatever other codec driver) claims this >>>> clock. >>> >>> simple-card obviously isn't a CODEC driver... >> >> Yeah, sorry. >> >>> For generality I think >>> the clock does need to be exposed to the CODEC driver, otherwise this >>> will work differently to how other systems are working and we can't >>> substitute in a different clock on the CODEC side so easily if it >>> doesn't happen to use the output from the I2S block. >> >> Ok, then I think what we will do is abandon this patch and I will send >> something that adds this functionality to the particular codec that >> I'm interested in -- max98090. > > Sorry I didn't read this earlier. I don't think that this belongs in > the max98090. The original patch description is a bit confusing. The > clock being grabbed here is actually i2s mclk. My understanding is > that, on this SoC, the mclk is driven from a different IP block than > the rest of the i2s signals. The i2s driver needs to be told about > the clock and enable/disable it at the appropriate times. I'm > assuming it's optional because there are boards using this SoC with > i2s slave mode that don't drive mclk at all. > > Please correct me if I'm wrong on any of the above. I don't think you're wrong, and I'm an audio/i2s neophyte so I think you're probably right and hopefully Mark can confirm that this is how we want it. One important thing to point out, which might be causing confusion, is that this driver is claiming a clock which it internally calls "mclk" but the way it's specified for rk3288 in the DT, that one is just the one which drives the internal logic and has a gate. This clock I'm adding is the actual mclk which is being driven to the i2s slave device, and it has it's own gate and also has a mux, and we need to claim both to be able to enable proper clock gating. ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2014-12-03 23:53 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-12-02 14:49 [PATCH 0/2] rockchip: i2s: patches to add optional clock output from Jianqun Xu 2014-12-02 14:49 ` [PATCH 1/2] ASoC: rockchip-i2s: dt: add an optional property "i2s_clk_out" Jianqun Xu 2014-12-02 17:57 ` Doug Anderson 2014-12-02 14:52 ` [PATCH 2/2] ASoC: rockchip: i2s: add support for grabbing output clock to codec Jianqun Xu 2014-12-02 17:54 ` Doug Anderson 2014-12-03 1:03 ` Jianqun 2014-12-03 5:18 ` Doug Anderson 2014-12-03 12:55 ` Mark Brown 2014-12-03 7:18 ` [PATCH v2 0/2] ASoC: rockchip: i2s: driver could output "i2s-clk-out" optionally Jianqun Xu 2014-12-03 7:18 ` [PATCH v2 1/2] ASoC: dt-bindings: rockchip-i2s: add an optional property "i2s_clk_out" Jianqun Xu 2014-12-03 7:18 ` [PATCH v2 2/2] ASoC: rockchip: i2s: add support for grabbing output clock to codec Jianqun Xu 2014-12-03 19:20 ` Mark Brown 2014-12-03 19:38 ` Sonny Rao 2014-12-03 20:03 ` Mark Brown 2014-12-03 23:03 ` Sonny Rao 2014-12-03 23:22 ` [alsa-devel] " Dylan Reid 2014-12-03 23:52 ` Sonny Rao
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).