[PATCH 1/2] clk: rockchip: add I2S internal clock IDs for rk3288

[PATCH 1/2] clk: rockchip: add I2S internal clock IDs for rk3288

[John Keeping]

To minimize jitter on the I2S clocks, it is important that the
denominator in the fractional divider is much greater than the
numerator.  Add identifiers for these internal clocks so that the
specific clock topology and rates can be specified in the device tree.

Signed-off-by: John Keeping <john@metanate.com>
---
 include/dt-bindings/clock/rk3288-cru.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/dt-bindings/clock/rk3288-cru.h b/include/dt-bindings/clock/rk3288-cru.h
index 9a586e2d9c91..9526653383d9 100644
--- a/include/dt-bindings/clock/rk3288-cru.h
+++ b/include/dt-bindings/clock/rk3288-cru.h
@@ -88,6 +88,9 @@
 #define SCLK_PVTM_GPU		124
 #define SCLK_CRYPTO		125
 #define SCLK_MIPIDSI_24M	126
+#define SCLK_I2S_PRE		127
+#define SCLK_I2S_SRC		128
+#define SCLK_I2S_FRAC		129
 
 #define SCLK_MAC		151
 #define SCLK_MACREF_OUT		152
-- 
2.9.3.728.g30b24b4.dirty

[PATCH 2/2] clk: rockchip: use identifiers for rk3288 I2S clocks

[John Keeping]

Export these so that the rates can be set via the device tree.

Signed-off-by: John Keeping <john@metanate.com>
---
 drivers/clk/rockchip/clk-rk3288.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/rockchip/clk-rk3288.c b/drivers/clk/rockchip/clk-rk3288.c
index 39af05a589b3..8817a5b3bd0f 100644
--- a/drivers/clk/rockchip/clk-rk3288.c
+++ b/drivers/clk/rockchip/clk-rk3288.c
@@ -229,7 +229,7 @@ static struct clk_div_table div_hclk_cpu_t[] = {
 #define IFLAGS ROCKCHIP_INVERTER_HIWORD_MASK
 
 static struct rockchip_clk_branch rk3288_i2s_fracmux __initdata =
-	MUX(0, "i2s_pre", mux_i2s_pre_p, CLK_SET_RATE_PARENT,
+	MUX(SCLK_I2S_PRE, "i2s_pre", mux_i2s_pre_p, CLK_SET_RATE_PARENT,
 			RK3288_CLKSEL_CON(4), 8, 2, MFLAGS);
 
 static struct rockchip_clk_branch rk3288_spdif_fracmux __initdata =
@@ -338,10 +338,10 @@ static struct rockchip_clk_branch rk3288_clk_branches[] __initdata = {
 
 	FACTOR(0, "xin12m", "xin24m", 0, 1, 2),
 
-	COMPOSITE(0, "i2s_src", mux_pll_src_cpll_gpll_p, 0,
+	COMPOSITE(SCLK_I2S_SRC, "i2s_src", mux_pll_src_cpll_gpll_p, 0,
 			RK3288_CLKSEL_CON(4), 15, 1, MFLAGS, 0, 7, DFLAGS,
 			RK3288_CLKGATE_CON(4), 1, GFLAGS),
-	COMPOSITE_FRACMUX(0, "i2s_frac", "i2s_src", CLK_SET_RATE_PARENT,
+	COMPOSITE_FRACMUX(SCLK_I2S_FRAC, "i2s_frac", "i2s_src", CLK_SET_RATE_PARENT,
 			RK3288_CLKSEL_CON(8), 0,
 			RK3288_CLKGATE_CON(4), 2, GFLAGS,
 			&rk3288_i2s_fracmux),
-- 
2.9.3.728.g30b24b4.dirty

Re: [PATCH 1/2] clk: rockchip: add I2S internal clock IDs for rk3288

[Heiko Stuebner]

Hi John,

Am Mittwoch, 7. September 2016, 17:53:29 CEST schrieb John Keeping:
> To minimize jitter on the I2S clocks, it is important that the
> denominator in the fractional divider is much greater than the
> numerator.  Add identifiers for these internal clocks so that the
> specific clock topology and rates can be specified in the device tree.

The TRM states that the denominator must be bigger than 20. Is this the one 
you found or did you find further constraints?

Did you try teaching the fractional divider to handle these constraints before 
going this way?

Exporting the internal clocks really would be sort of plan d or e, after 
handling this in the clock framework failed. Especially as i2s rates are 
probably dependant on the media being handled (frequencies and such), so 
setting fractional rates statically in the dts won't help you much in the 
general case, as any new playback could trigger a clk_set_rate call anyway?


Heiko

> Signed-off-by: John Keeping <john-HooS5bfzL4hWk0Htik3J/w@public.gmane.org>
> ---
>  include/dt-bindings/clock/rk3288-cru.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/include/dt-bindings/clock/rk3288-cru.h
> b/include/dt-bindings/clock/rk3288-cru.h index 9a586e2d9c91..9526653383d9
> 100644
> --- a/include/dt-bindings/clock/rk3288-cru.h
> +++ b/include/dt-bindings/clock/rk3288-cru.h
> @@ -88,6 +88,9 @@
>  #define SCLK_PVTM_GPU		124
>  #define SCLK_CRYPTO		125
>  #define SCLK_MIPIDSI_24M	126
> +#define SCLK_I2S_PRE		127
> +#define SCLK_I2S_SRC		128
> +#define SCLK_I2S_FRAC		129
> 
>  #define SCLK_MAC		151
>  #define SCLK_MACREF_OUT		152


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] clk: rockchip: add I2S internal clock IDs for rk3288

[John Keeping]

On Wed, 07 Sep 2016 19:58:31 +0200, Heiko Stuebner wrote:

> Am Mittwoch, 7. September 2016, 17:53:29 CEST schrieb John Keeping:
> > To minimize jitter on the I2S clocks, it is important that the
> > denominator in the fractional divider is much greater than the
> > numerator.  Add identifiers for these internal clocks so that the
> > specific clock topology and rates can be specified in the device tree.  
> 
> The TRM states that the denominator must be bigger than 20. Is this the one 
> you found or did you find further constraints?

Is it not that the denominator must be bigger than 20 times the
numerator?  That's what we found, although it seems that the greater the
divisor the better, so our aim is dividing down from 594MHz from GPLL to
the target rate in i2s_frac.

> Did you try teaching the fractional divider to handle these constraints before 
> going this way?

No, I hadn't looked at doing that.  I'm not sure how that would work,
we'd need to add support for fractional divider changing the parent rate
wouldn't we?

> Exporting the internal clocks really would be sort of plan d or e, after 
> handling this in the clock framework failed. Especially as i2s rates are 
> probably dependant on the media being handled (frequencies and such), so 
> setting fractional rates statically in the dts won't help you much in the 
> general case, as any new playback could trigger a clk_set_rate call anyway?

We're not setting the fractional rate specifically, instead we're
setting the i2s_pre rate and parent explicitly and banning the i2s_src
mux setting to i2s_pre, which means that clk_set_rate on sclk_i2s0 will
always change i2s_frac but leave i2s_pre alone.

> > Signed-off-by: John Keeping <john-HooS5bfzL4hWk0Htik3J/w@public.gmane.org>
> > ---
> >  include/dt-bindings/clock/rk3288-cru.h | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/include/dt-bindings/clock/rk3288-cru.h
> > b/include/dt-bindings/clock/rk3288-cru.h index 9a586e2d9c91..9526653383d9
> > 100644
> > --- a/include/dt-bindings/clock/rk3288-cru.h
> > +++ b/include/dt-bindings/clock/rk3288-cru.h
> > @@ -88,6 +88,9 @@
> >  #define SCLK_PVTM_GPU		124
> >  #define SCLK_CRYPTO		125
> >  #define SCLK_MIPIDSI_24M	126
> > +#define SCLK_I2S_PRE		127
> > +#define SCLK_I2S_SRC		128
> > +#define SCLK_I2S_FRAC		129
> > 
> >  #define SCLK_MAC		151
> >  #define SCLK_MACREF_OUT		152  
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] clk: rockchip: add I2S internal clock IDs for rk3288

[Heiko Stuebner]

Hi John,

sorry this took so long,

Am Mittwoch, 7. September 2016, 19:18:23 CEST schrieb John Keeping:
> On Wed, 07 Sep 2016 19:58:31 +0200, Heiko Stuebner wrote:
> > Am Mittwoch, 7. September 2016, 17:53:29 CEST schrieb John Keeping:
> > > To minimize jitter on the I2S clocks, it is important that the
> > > denominator in the fractional divider is much greater than the
> > > numerator.  Add identifiers for these internal clocks so that the
> > > specific clock topology and rates can be specified in the device tree.
> > 
> > The TRM states that the denominator must be bigger than 20. Is this the
> > one
> > you found or did you find further constraints?
> 
> Is it not that the denominator must be bigger than 20 times the
> numerator?  That's what we found, although it seems that the greater the
> divisor the better, so our aim is dividing down from 594MHz from GPLL to
> the target rate in i2s_frac.

That is good to know ... the TRM probably lost the real information during 
translation or so and only states that hard value. Looking at the rk3368 TRM 
just now, the value really is 20 times greater than the numerator.


> > Did you try teaching the fractional divider to handle these constraints
> > before going this way?
> 
> No, I hadn't looked at doing that.  I'm not sure how that would work,
> we'd need to add support for fractional divider changing the parent rate
> wouldn't we?

Doug had a somewhat similar problem, mentioning the lack of a "bestdiv" 
equivalent that the standard divider provides. I'm not sure if there isn't a 
way to provide something like this and/or make the fractional clock honor 
specific requirments concerning numerator/denominator - any mathematicians 
around? :-)

One could also replace the generic fraction divider use with a rockchip-
specific implementation that somehow handles that.


> > Exporting the internal clocks really would be sort of plan d or e, after
> > handling this in the clock framework failed. Especially as i2s rates are
> > probably dependant on the media being handled (frequencies and such), so
> > setting fractional rates statically in the dts won't help you much in the
> > general case, as any new playback could trigger a clk_set_rate call
> > anyway?
> 
> We're not setting the fractional rate specifically, instead we're
> setting the i2s_pre rate and parent explicitly and banning the i2s_src
> mux setting to i2s_pre, which means that clk_set_rate on sclk_i2s0 will
> always change i2s_frac but leave i2s_pre alone.

But then you are again in the situation where some requested rate can ignore 
those fractional-divider constraints? In general I really don't like encoding 
such board-specific behaviour into the generic clock tree, like needing to 
disable parent relationships.

It works for your board now but does not necessarily for all other boards, 
which might require other "hacks" and also doesn't solve the general problem 
for all other fractional dividers we have.


Heiko

Re: [PATCH 1/2] clk: rockchip: add I2S internal clock IDs for rk3288

[John Keeping]

On Sun, 25 Sep 2016 23:50:29 +0200, Heiko Stuebner wrote:

> Am Mittwoch, 7. September 2016, 19:18:23 CEST schrieb John Keeping:
> > On Wed, 07 Sep 2016 19:58:31 +0200, Heiko Stuebner wrote:  
> > > Am Mittwoch, 7. September 2016, 17:53:29 CEST schrieb John Keeping:  
> > > > To minimize jitter on the I2S clocks, it is important that the
> > > > denominator in the fractional divider is much greater than the
> > > > numerator.  Add identifiers for these internal clocks so that the
> > > > specific clock topology and rates can be specified in the device tree.  
> > > 
> > > The TRM states that the denominator must be bigger than 20. Is this the
> > > one
> > > you found or did you find further constraints?  
> > 
> > Is it not that the denominator must be bigger than 20 times the
> > numerator?  That's what we found, although it seems that the greater the
> > divisor the better, so our aim is dividing down from 594MHz from GPLL to
> > the target rate in i2s_frac.  
> 
> That is good to know ... the TRM probably lost the real information during 
> translation or so and only states that hard value. Looking at the rk3368 TRM 
> just now, the value really is 20 times greater than the numerator.
> 
> 
> > > Did you try teaching the fractional divider to handle these constraints
> > > before going this way?  
> > 
> > No, I hadn't looked at doing that.  I'm not sure how that would work,
> > we'd need to add support for fractional divider changing the parent rate
> > wouldn't we?  
> 
> Doug had a somewhat similar problem, mentioning the lack of a "bestdiv" 
> equivalent that the standard divider provides. I'm not sure if there isn't a 
> way to provide something like this and/or make the fractional clock honor 
> specific requirments concerning numerator/denominator - any mathematicians 
> around? :-)

Yes, this is difficult, although I expect a simple approach of setting
the input as fast as possible would work reasonably well in practice.
But even when the input is a mux with a fixed set of possible rates it
is difficult to extract its maximum frequency within the clock
framework.

> One could also replace the generic fraction divider use with a rockchip-
> specific implementation that somehow handles that.

I hadn't considered that, I assumed there would be a strong preference
for enhancing the generic code rather than writing a special case
implementation.

> > > Exporting the internal clocks really would be sort of plan d or e, after
> > > handling this in the clock framework failed. Especially as i2s rates are
> > > probably dependant on the media being handled (frequencies and such), so
> > > setting fractional rates statically in the dts won't help you much in the
> > > general case, as any new playback could trigger a clk_set_rate call
> > > anyway?  
> > 
> > We're not setting the fractional rate specifically, instead we're
> > setting the i2s_pre rate and parent explicitly and banning the i2s_src
> > mux setting to i2s_pre, which means that clk_set_rate on sclk_i2s0 will
> > always change i2s_frac but leave i2s_pre alone.  
> 
> But then you are again in the situation where some requested rate can ignore 
> those fractional-divider constraints? In general I really don't like encoding 
> such board-specific behaviour into the generic clock tree, like needing to 
> disable parent relationships.

Yes, I wouldn't suggest changing the clock mux in the upstream kernel.
I was hoping specifying an assigned parent in DT would be enough to keep
the parent set correctly, but it doesn't seem to be.

> It works for your board now but does not necessarily for all other boards, 
> which might require other "hacks" and also doesn't solve the general problem 
> for all other fractional dividers we have.

The sensible heuristic is to minimize the length of the clock path when
there are two equally good routes, which would handle the case where the
I2S clock can be generated without the need for a fractional divider.
But I don't think the clock framework has any way to take into account
that a mux may end up bypassing another clock.

It seems that even without the fractional divider we can't get low
enough jitter to meet our requirements, so we've decided to use an
external clock for I2S.  As a result of that, we're no longer using this
part of the clock tree at all, so I'm happy to drop these patches.


John