Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752864AbbD3VV2 (ORCPT ); Thu, 30 Apr 2015 17:21:28 -0400 Received: from mail-ie0-f176.google.com ([209.85.223.176]:34086 "EHLO mail-ie0-f176.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750717AbbD3VVW (ORCPT ); Thu, 30 Apr 2015 17:21:22 -0400 Date: Thu, 30 Apr 2015 16:20:59 -0500 From: Michael Welling To: Sebastian Hesselbarth Cc: Mike Turquette , Stephen Boyd , Jean-Francois Moine , Russell King , Jason Cooper , Andrew Lunn , Gregory Clement , devicetree@vger.kernel.org, linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 0/4] clk: si5351: Some fixes Message-ID: <20150430212059.GA14026@deathray> References: <1430415954-29517-1-git-send-email-sebastian.hesselbarth@gmail.com> <20150430193317.GC22000@deathray> <55429417.4070103@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <55429417.4070103@gmail.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8600 Lines: 169 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/