Re: [PATCH 0/4] clk: si5351: Some fixes

From: Michael Welling
Date: Thu Apr 30 2015 - 17:21:31 EST

On Thu, Apr 30, 2015 at 10:44:07PM +0200, Sebastian Hesselbarth wrote:
> On 30.04.2015 21:33, Michael Welling wrote:
> >On Thu, Apr 30, 2015 at 07:45:50PM +0200, Sebastian Hesselbarth wrote:
> >>For Si5351 clock driver, Michael Welling and Jean-Francois Moine reported
> >>issues with recent v4.x kernels due to broken/missing/wrong parent clock
> >>claming. This patch set now deals with the issues reported.
>
> I should have been more precise above,
> e.g. s/deals with/deals with some/
>
> [...]
> >
> >Okay, the results are in and they are mixed. Firstly the clocks register
> >unlike before. This is a positive step that was certianly expected.
>
> Yes, claiming parent clocks is the main fix.
>
> The pll reset patch should improve stability but datasheet isn't very
> clear about when to reset pll nor how long it may take at max. Even the
> Loss-of-Lock check isn't documented but seems to make sense.
>
> >Second the reported and measured clock frequencies do not match the
> >device tree entries.
>
> But generated frequencies do always match reported frequencies.
>
> I striped down your reports the the very essential lines.
>
> >Measured frequencies:
> >clk0 12.5Mhz
> >clk1 5.357Mhz
> >clk2 0 Hz
> >
> >Reported frequencies:
> >root@som3517-som200:~# head -n 15 /sys/kernel/debug/clk/clk_summary
> > clock enable_cnt prepare_cnt rate accuracy phase
> >----------------------------------------------------------------------------------------
> > clk2 0 0 12288000 0 0
> > clk0 0 0 12499999 0 0
> > clk1 0 0 5357142 0 0
>
> What I noticed about your clk2 that you always measure as 0 Hz is
> that none of your clocks is prepared/enabled.
>
> Currently, the si5351 driver only ensures the output is enabled
> when si5351_clkout_prepare() is called.
>
> As long as you do not have a clk consumer that properly prepare/enables
> the clock output, it may remain disabled.
>
> We should probably have additional DT properties and corresponding
> pdata to force clkoutN always on.

Does the silabs,disable-state of 3 (SI5351_DISABLE_NEVER) take care
of this?

Otherwise is there a simple registration that will do this?

>
> >Device tree entry:
> > si5351: clock-generator {
> > /* connect xtal input to 27MHz reference */
> > clocks = <&ref27>;
> > clock-names = "xtal";
> >
> > clkout0: clkout0 {
> > reg = <0>;
> > clock-frequency = <18432000>;
> > };
> >
> > clkout1: clkout1 {
> > reg = <1>;
> > clock-frequency = <8000000>;
> > };
> >
> > clkout2: clkout2 {
> > reg = <2>;
> > clock-frequency = <12288000>;
> > };
> > };
> >
> >Lastly if #define DEBUG is added the behavior is different.
>
> Ok, I didn't dig into this. I think I'll rebuild your DT setup above
> and see if I can reproduce it. It will be different with respect to
> XTAL frequency, which is 25MHz on CuBox.
>
> >Debugging output:
> >[ 2.970753] si5351 0-0060: si5351_clkout_round_rate - clk0: rdiv = 1, parent_rate = 18432000, rate = 18432000
> >[ 2.981207] si5351 0-0060: si5351_msynth_round_rate - ms0: a = 48, b = 0, c = 1, divby4 = 0, parent_rate = 884736000, rate = 18432000
> >[ 2.993777] si5351 0-0060: si5351_pll_round_rate - pllb: a = 32, b = 96, c = 125, parent_rate = 27000000, rate = 884736000
> >[ 3.005362] si5351 0-0060: si5351_msynth_recalc_rate - ms0: p1 = 5632, p2 = 0, p3 = 1, m = 6144, parent_rate = 884736000, rate = 18432000
> >[ 3.026281] si5351 0-0060: si5351_pll_set_rate - pllb: p1 = 3682, p2 = 38, p3 = 125, parent_rate = 27000000, rate = 884736000
> >[ 3.038151] si5351 0-0060: si5351_pll_recalc_rate - pllb: p1 = 3682, p2 = 38, p3 = 125, parent_rate = 27000000, rate = 884736000
> >[ 3.053933] si5351 0-0060: si5351_msynth_set_rate - ms2: p1 = 0, p2 = 0, p3 = 0, divby4 = 1, parent_rate = 884736000, rate = 884736000
>
> Above ms2 .set_rate() doesn't look good. It is called because ms2 is
> child of pllb but the .params have not been setup. Usually this is
> done in si5351_msynth_recalc_rate() but it has not been called yet.
>
> I will probably initialize .params with current register contents
> on probe().
>
> >[ 3.068067] si5351 0-0060: si5351_msynth_set_rate - ms0: p1 = 5632, p2 = 0, p3 = 1, divby4 = 0, parent_rate = 884736000, rate = 18432000
> >[ 3.080913] si5351 0-0060: si5351_msynth_recalc_rate - ms0: p1 = 5632, p2 = 0, p3 = 1, m = 6144, parent_rate = 884736000, rate = 18432000
> >[ 3.093843] si5351 0-0060: si5351_clkout_set_rate - clk0: rdiv = 1, parent_rate = 18432000, rate = 18432000
>
> >[ 3.104184] si5351 0-0060: si5351_clkout_round_rate - clk1: rdiv = 1, parent_rate = 8000000, rate = 8000000
> >[ 3.114408] si5351 0-0060: si5351_msynth_round_rate - ms1: a = 112, b = 0, c = 1, divby4 = 0, parent_rate = 896000000, rate = 8000000
> >[ 3.126973] si5351 0-0060: si5351_pll_round_rate - plla: a = 33, b = 37037, c = 200000, parent_rate = 27000000, rate = 895999995
> >[ 3.139085] si5351 0-0060: si5351_msynth_recalc_rate - ms1: p1 = 13824, p2 = 0, p3 = 1, m = 14336, parent_rate = 895999995, rate = 7999999
> >[ 3.155510] si5351 0-0060: si5351_pll_set_rate - plla: p1 = 3735, p2 = 140736, p3 = 200000, parent_rate = 27000000, rate = 895999995
> >[ 3.167993] si5351 0-0060: si5351_pll_recalc_rate - plla: p1 = 3735, p2 = 140736, p3 = 200000, parent_rate = 27000000, rate = 895999995
> >[ 3.182186] si5351 0-0060: si5351_msynth_set_rate - ms1: p1 = 13824, p2 = 0, p3 = 1, divby4 = 0, parent_rate = 895999995, rate = 8000000
> >[ 3.195028] si5351 0-0060: si5351_msynth_recalc_rate - ms1: p1 = 13824, p2 = 0, p3 = 1, m = 14336, parent_rate = 895999995, rate = 7999999
> >[ 3.208046] si5351 0-0060: si5351_clkout_set_rate - clk1: rdiv = 1, parent_rate = 7999999, rate = 8000000
>
> >[ 3.218150] si5351 0-0060: si5351_clkout_round_rate - clk2: rdiv = 1, parent_rate = 12288000, rate = 12288000
> >[ 3.228544] si5351 0-0060: si5351_msynth_round_rate - ms2: a = 72, b = 0, c = 1, divby4 = 0, parent_rate = 884736000, rate = 12288000
> >[ 3.242565] si5351 0-0060: si5351_msynth_set_rate - ms2: p1 = 8704, p2 = 0, p3 = 1, divby4 = 0, parent_rate = 884736000, rate = 12288000
> >[ 3.255416] si5351 0-0060: si5351_msynth_recalc_rate - ms2: p1 = 8704, p2 = 0, p3 = 1, m = 9216, parent_rate = 884736000, rate = 12288000
> >[ 3.268345] si5351 0-0060: si5351_clkout_set_rate - clk2: rdiv = 1, parent_rate = 12288000, rate = 12288000
>
> >Measured frequencies:
> >clk0 18.432Mhz
> >clk1 8Mhz
> >clk2 0Hz
> >
> >Reported frequencies:
> >root@som3517-som200:~# head -n 15 /sys/kernel/debug/clk/clk_summary
> > clock enable_cnt prepare_cnt rate accuracy phase
> >----------------------------------------------------------------------------------------
> > clk2 0 0 12288000 0 0
> > clk0 0 0 18432000 0 0
> > clk1 0 0 7999999 0 0
>
> Here again, reported frequencies and measured frequencies look quite
> good. Why the requested frequencies differ when enabling DEBUG, I'll
> have to have a closer look.
>
> >It should be noted that if I program the device's register map in the
> >bootloader the device keeps the correct frequency outputs.
>
> "keeps"? You mean "generates", don't you?
>

Yes the clocks are generated and do not get effected by the driver.

> >So the patch series appears to fix the registration issue but there is still
> >more work to be done.
>
> Yep. And your testing on a different setup definitely helps.
>

I have been battling this chip for a while now as it is on a few
different products I have dealt with.

> >Still not sure how to explain the difference when DEBUG is defined.
> >I will dig into the datasheet and see what I can find.
>
> Me neither.
>
> Sebastian
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/