[PATCH] clk: rockchip: rk3288 export i2s0_clkout for use in DT

[PATCH] clk: rockchip: rk3288 export i2s0_clkout for use in DT

[Sonny Rao]

This exposes the clock that comes out of the i2s block which generally
goes to the audio codec.

Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
---
 drivers/clk/rockchip/clk-rk3288.c      | 3 ++-
 include/dt-bindings/clock/rk3288-cru.h | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/rockchip/clk-rk3288.c b/drivers/clk/rockchip/clk-rk3288.c
index 2327829..8777737 100644
--- a/drivers/clk/rockchip/clk-rk3288.c
+++ b/drivers/clk/rockchip/clk-rk3288.c
@@ -305,7 +305,8 @@ static struct rockchip_clk_branch rk3288_clk_branches[] __initdata = {
 			RK3288_CLKGATE_CON(4), 2, GFLAGS),
 	MUX(0, "i2s_pre", mux_i2s_pre_p, CLK_SET_RATE_PARENT,
 			RK3288_CLKSEL_CON(4), 8, 2, MFLAGS),
-	COMPOSITE_NODIV(0, "i2s0_clkout", mux_i2s_clkout_p, CLK_SET_RATE_PARENT,
+	COMPOSITE_NODIV(SCLK_I2S0_OUT, "i2s0_clkout", mux_i2s_clkout_p,
+			CLK_SET_RATE_PARENT,
 			RK3288_CLKSEL_CON(4), 12, 1, MFLAGS,
 			RK3288_CLKGATE_CON(4), 0, GFLAGS),
 	GATE(SCLK_I2S0, "sclk_i2s0", "i2s_pre", CLK_SET_RATE_PARENT,
diff --git a/include/dt-bindings/clock/rk3288-cru.h b/include/dt-bindings/clock/rk3288-cru.h
index 100a08c..4acc730 100644
--- a/include/dt-bindings/clock/rk3288-cru.h
+++ b/include/dt-bindings/clock/rk3288-cru.h
@@ -71,6 +71,7 @@
 #define SCLK_HDMI_CEC		110
 #define SCLK_HEVC_CABAC		111
 #define SCLK_HEVC_CORE		112
+#define SCLK_I2S0_OUT           113
 
 #define DCLK_VOP0		190
 #define DCLK_VOP1		191
-- 
2.1.0.rc2.206.gedb03e5

Re: [PATCH] clk: rockchip: rk3288 export i2s0_clkout for use in DT

[Heiko Stübner]

Hi Sonny,

Am Dienstag, 18. November 2014, 23:15:19 schrieb Sonny Rao:
> This exposes the clock that comes out of the i2s block which generally
> goes to the audio codec.
> 
> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
> ---
>  drivers/clk/rockchip/clk-rk3288.c      | 3 ++-
>  include/dt-bindings/clock/rk3288-cru.h | 1 +
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 

[...]

> diff --git a/include/dt-bindings/clock/rk3288-cru.h
> b/include/dt-bindings/clock/rk3288-cru.h index 100a08c..4acc730 100644
> --- a/include/dt-bindings/clock/rk3288-cru.h
> +++ b/include/dt-bindings/clock/rk3288-cru.h
> @@ -71,6 +71,7 @@
>  #define SCLK_HDMI_CEC		110
>  #define SCLK_HEVC_CABAC		111
>  #define SCLK_HEVC_CORE		112
> +#define SCLK_I2S0_OUT           113
> 
>  #define DCLK_VOP0		190
>  #define DCLK_VOP1		191

just to get branches right, do you plan on sending a patch using this new 
clock-id in a devicetree file in time for 3.19 (i.e. during the next week).

If you plan on doing this, we'll need a 2-patch series like Alexandru did for 
the mmc phases [because we would need a shared branch between clk and dts 
branches]. If not the patch can stay as it is.


Thanks
Heiko

Re: [PATCH] clk: rockchip: rk3288 export i2s0_clkout for use in DT

[Sonny Rao]

On Sun, Nov 23, 2014 at 4:07 PM, Heiko Stübner <heiko@sntech.de> wrote:
> Hi Sonny,
>
> Am Dienstag, 18. November 2014, 23:15:19 schrieb Sonny Rao:
>> This exposes the clock that comes out of the i2s block which generally
>> goes to the audio codec.
>>
>> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
>> ---
>>  drivers/clk/rockchip/clk-rk3288.c      | 3 ++-
>>  include/dt-bindings/clock/rk3288-cru.h | 1 +
>>  2 files changed, 3 insertions(+), 1 deletion(-)
>>
>
> [...]
>
>> diff --git a/include/dt-bindings/clock/rk3288-cru.h
>> b/include/dt-bindings/clock/rk3288-cru.h index 100a08c..4acc730 100644
>> --- a/include/dt-bindings/clock/rk3288-cru.h
>> +++ b/include/dt-bindings/clock/rk3288-cru.h
>> @@ -71,6 +71,7 @@
>>  #define SCLK_HDMI_CEC                110
>>  #define SCLK_HEVC_CABAC              111
>>  #define SCLK_HEVC_CORE               112
>> +#define SCLK_I2S0_OUT           113
>>
>>  #define DCLK_VOP0            190
>>  #define DCLK_VOP1            191
>
> just to get branches right, do you plan on sending a patch using this new
> clock-id in a devicetree file in time for 3.19 (i.e. during the next week).
>
> If you plan on doing this, we'll need a 2-patch series like Alexandru did for
> the mmc phases [because we would need a shared branch between clk and dts
> branches]. If not the patch can stay as it is.
>

Hi, I'm not planning on sending anything with this new clock in the
immediate future, so I think you can take it as is.  Eventually, we
will submit something that uses it for the audio codec on Pinky using
simple-card but I don't have that ready yet.  Thanks!

> Thanks
> Heiko

Re: [PATCH] clk: rockchip: rk3288 export i2s0_clkout for use in DT

[Heiko Stübner]

Hi Sonny,

Am Dienstag, 18. November 2014, 23:15:19 schrieb Sonny Rao:
> This exposes the clock that comes out of the i2s block which generally
> goes to the audio codec.
> 
> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
> ---
>  drivers/clk/rockchip/clk-rk3288.c      | 3 ++-
>  include/dt-bindings/clock/rk3288-cru.h | 1 +
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/rockchip/clk-rk3288.c
> b/drivers/clk/rockchip/clk-rk3288.c index 2327829..8777737 100644
> --- a/drivers/clk/rockchip/clk-rk3288.c
> +++ b/drivers/clk/rockchip/clk-rk3288.c
> @@ -305,7 +305,8 @@ static struct rockchip_clk_branch rk3288_clk_branches[]
> __initdata = { RK3288_CLKGATE_CON(4), 2, GFLAGS),
>  	MUX(0, "i2s_pre", mux_i2s_pre_p, CLK_SET_RATE_PARENT,
>  			RK3288_CLKSEL_CON(4), 8, 2, MFLAGS),
> -	COMPOSITE_NODIV(0, "i2s0_clkout", mux_i2s_clkout_p, CLK_SET_RATE_PARENT,
> +	COMPOSITE_NODIV(SCLK_I2S0_OUT, "i2s0_clkout", mux_i2s_clkout_p,
> +			CLK_SET_RATE_PARENT,

this patch fails to apply, as the current i2s_clkout definition does not have 
the CLK_SET_RATE_PARENT yet. I'm not sure if I missed a patch somewhere but my 
search in my inbox for _not yet handled_ patches has come up empty so far.

But having CLK_SET_RATE_PARENT there will probably cause problems anyway.
i2s0_clkout is sourced by either i2s_pre or xin12m. i2s_pre also is the source 
for the core SCLK_I2S0 going to the i2s controller. So having both SCLK_I2S0 
and (the new) SCLK_I2S0_OUT fiddling with the i2s_pre rate calls for trouble.

So I think the i2s0_clkout should limit itself to selecting the best frequency 
from its sources without changing i2s_pre.


And a personal style nitpick: the macros are laid out in a way to facilitate 
ease of reading by for example always having the same number of lines per 
COMPOSITE definition and having each element in roughly the same place. So a 
CLK_SET_RATE_PARENT param should stay on the first line :-) .


Heiko

Re: [PATCH] clk: rockchip: rk3288 export i2s0_clkout for use in DT

[Doug Anderson]

Heiko,

On Wed, Nov 26, 2014 at 10:09 AM, Heiko Stübner <heiko@sntech.de> wrote:
> Hi Sonny,
>
> Am Dienstag, 18. November 2014, 23:15:19 schrieb Sonny Rao:
>> This exposes the clock that comes out of the i2s block which generally
>> goes to the audio codec.
>>
>> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
>> ---
>>  drivers/clk/rockchip/clk-rk3288.c      | 3 ++-
>>  include/dt-bindings/clock/rk3288-cru.h | 1 +
>>  2 files changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/rockchip/clk-rk3288.c
>> b/drivers/clk/rockchip/clk-rk3288.c index 2327829..8777737 100644
>> --- a/drivers/clk/rockchip/clk-rk3288.c
>> +++ b/drivers/clk/rockchip/clk-rk3288.c
>> @@ -305,7 +305,8 @@ static struct rockchip_clk_branch rk3288_clk_branches[]
>> __initdata = { RK3288_CLKGATE_CON(4), 2, GFLAGS),
>>       MUX(0, "i2s_pre", mux_i2s_pre_p, CLK_SET_RATE_PARENT,
>>                       RK3288_CLKSEL_CON(4), 8, 2, MFLAGS),
>> -     COMPOSITE_NODIV(0, "i2s0_clkout", mux_i2s_clkout_p, CLK_SET_RATE_PARENT,
>> +     COMPOSITE_NODIV(SCLK_I2S0_OUT, "i2s0_clkout", mux_i2s_clkout_p,
>> +                     CLK_SET_RATE_PARENT,
>
> this patch fails to apply, as the current i2s_clkout definition does not have
> the CLK_SET_RATE_PARENT yet. I'm not sure if I missed a patch somewhere but my
> search in my inbox for _not yet handled_ patches has come up empty so far.

I don't think it has been sent.


> But having CLK_SET_RATE_PARENT there will probably cause problems anyway.
> i2s0_clkout is sourced by either i2s_pre or xin12m. i2s_pre also is the source
> for the core SCLK_I2S0 going to the i2s controller. So having both SCLK_I2S0
> and (the new) SCLK_I2S0_OUT fiddling with the i2s_pre rate calls for trouble.
>
> So I think the i2s0_clkout should limit itself to selecting the best frequency
> from its sources without changing i2s_pre.

Probably this is my fault.  At some point I had the thought process
that this might be a nice way to make it so that we didn't need to add
special handling to the i2s driver.  AKA, we wouldn't need
<https://patchwork.kernel.org/patch/5334971/>.

See my comments on
<https://chromium-review.googlesource.com/#/c/230347/3>.  For those
that don't want to follow links, that would be:

> An alternative that might require no code changes to the i2s driver (untested!):
>
> clock-names = "i2s_hclk", "i2s_clk";
> clocks = <&cru HCLK_I2S0>, <&cru SCLK_I2S0_CLKOUT>;
> assigned-clocks = <&cru SCLK_I2S0>, <&cru SCLK_I2S0_OUT>;
> assigned-clock-parents = <&cru SCLK_I2S0>;
>
> Then you'd add CLK_SET_RATE_PARENT to SCLK_I2S0_CLKOUT.
> You might need to disable the auto-remuxing on SCLK_I2S0_CLKOUT too.
>
> Basically the idea is to pass the child in as the main clock and the parent will
> get set implicitly.
>
> I have no idea if that's better or worse than what you have here...

I'm no longer convinced that's a good idea, so we should just axe the
CLK_SET_RATE_PARENT.


If timing is working out well and Sonny isn't available to spin this
right now (it's Thanksgiving holidays here in the US), I doubt he
would mind if you fixed up the patch and submitted it.


-Doug

Re: [PATCH] clk: rockchip: rk3288 export i2s0_clkout for use in DT

[Heiko Stübner]

Hi Doug,

Am Mittwoch, 26. November 2014, 15:05:53 schrieb Doug Anderson:
> I'm no longer convinced that's a good idea, so we should just axe the
> CLK_SET_RATE_PARENT.
> 
> If timing is working out well and Sonny isn't available to spin this
> right now (it's Thanksgiving holidays here in the US), I doubt he
> would mind if you fixed up the patch and submitted it.

yep I can do that ... I just wanted some sort of confirmation to verify that 
I'm not missing anything obvious before I start modifying patches :-)


Heiko

Re: [PATCH] clk: rockchip: rk3288 export i2s0_clkout for use in DT

[Heiko Stübner]

Am Dienstag, 18. November 2014, 23:15:19 schrieb Sonny Rao:
> This exposes the clock that comes out of the i2s block which generally
> goes to the audio codec.
> 
> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>

applied to my clk branch after removing the CLK_SET_RATE_PARENT

Heiko

Re: [PATCH] clk: rockchip: rk3288 export i2s0_clkout for use in DT

[Sonny Rao]

On Wed, Nov 26, 2014 at 3:32 PM, Heiko Stübner <heiko@sntech.de> wrote:
> Am Dienstag, 18. November 2014, 23:15:19 schrieb Sonny Rao:
>> This exposes the clock that comes out of the i2s block which generally
>> goes to the audio codec.
>>
>> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
>
> applied to my clk branch after removing the CLK_SET_RATE_PARENT

Hi, sorry for the delay, and thanks for fixing it.  I think when I
applied the patch to next-20141118 that had a CLK_SET_RATE_PARENT in
it from this patch:

commit fc69ed70c16a31d6a77ec47a30a9fe941f763f1e
Author: Jianqun <jay.xu@rock-chips.com>
Date:   Tue Sep 30 11:12:04 2014 +0800

    clk: rockchip: rk3288: i2s_frac adds flag to set parent's rate


I agree that is is not necessary and maybe not desirable.  Thanks again!


> Heiko

Re: [PATCH] clk: rockchip: rk3288 export i2s0_clkout for use in DT

[Heiko Stübner]

Am Mittwoch, 26. November 2014, 19:47:10 schrieb Sonny Rao:
> On Wed, Nov 26, 2014 at 3:32 PM, Heiko Stübner <heiko@sntech.de> wrote:
> > Am Dienstag, 18. November 2014, 23:15:19 schrieb Sonny Rao:
> >> This exposes the clock that comes out of the i2s block which generally
> >> goes to the audio codec.
> >> 
> >> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
> > 
> > applied to my clk branch after removing the CLK_SET_RATE_PARENT
> 
> Hi, sorry for the delay, and thanks for fixing it.  I think when I
> applied the patch to next-20141118 that had a CLK_SET_RATE_PARENT in
> it from this patch:
> 
> commit fc69ed70c16a31d6a77ec47a30a9fe941f763f1e
> Author: Jianqun <jay.xu@rock-chips.com>
> Date:   Tue Sep 30 11:12:04 2014 +0800
> 
>     clk: rockchip: rk3288: i2s_frac adds flag to set parent's rate
> 
> 
> I agree that is is not necessary and maybe not desirable.  Thanks again!

glad to be of help :-)

Just for reference, the commit removing this CLK_SET_RATE_PARENT is [0] that 
incidentally got pulled in by Mike on the same 20141118 and was therefore part 
of linux-next 20141119



[0] https://git.kernel.org/cgit/linux/kernel/git/mmind/linux-rockchip.git/commit/?id=8f06f5d392b3fbd58a5d7e00b047b6ee08c6d9b0