| Message ID | 1416381319-31117-1-git-send-email-sonnyrao@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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 <sonnyrao@chromium.org> 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 <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
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
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
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(-)