This exposes the clock that comes out of the i2s block which generally
goes to the audio codec.
Signed-off-by: Sonny Rao <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
On Sun, Nov 23, 2014 at 4:07 PM, Heiko Stübner <[email protected]> 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 <[email protected]>
>> ---
>> 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
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 <[email protected]>
> ---
> 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
Heiko,
On Wed, Nov 26, 2014 at 10:09 AM, Heiko Stübner <[email protected]> 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 <[email protected]>
>> ---
>> 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
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
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 <[email protected]>
applied to my clk branch after removing the CLK_SET_RATE_PARENT
Heiko
On Wed, Nov 26, 2014 at 3:32 PM, Heiko Stübner <[email protected]> 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 <[email protected]>
>
> 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 <[email protected]>
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
Am Mittwoch, 26. November 2014, 19:47:10 schrieb Sonny Rao:
> On Wed, Nov 26, 2014 at 3:32 PM, Heiko St?bner <[email protected]> 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 <[email protected]>
> >
> > 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 <[email protected]>
> 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