Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752851AbbD3TdY (ORCPT ); Thu, 30 Apr 2015 15:33:24 -0400 Received: from mail-ig0-f169.google.com ([209.85.213.169]:33485 "EHLO mail-ig0-f169.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752464AbbD3TdV (ORCPT ); Thu, 30 Apr 2015 15:33:21 -0400 Date: Thu, 30 Apr 2015 14:33:17 -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: <20150430193317.GC22000@deathray> References: <1430415954-29517-1-git-send-email-sebastian.hesselbarth@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1430415954-29517-1-git-send-email-sebastian.hesselbarth@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: 10657 Lines: 191 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. > > Patch 1 amends the binding documentation mention clock-names property > for the "xtal" and "clkin" parent clock inputs of Si5351 variants. > > Patch 2 adds the clock-names property for the SolidRun CuBox using Si5351 > with a fixed oscillator connected to "xtal" input. > > Patch 3 reworks the way we claim parent clocks by using devm_clk_get() > for both DT and platform_data based registration. Also, properly check > for errors returned by devm_clk_get() and prepare/enable the parent clocks. > > Patch 4 introduces a function to reset PLLs on rate change. This should > improve generated clock output stability. I currently have no scope at hand > to actually test that properly, so there may be more issues remaining. > > @Michael, Jean-Francois: Please test and report if there are still > issues remaining. > Okay, the results are in and they are mixed. Firstly the clocks register unlike before. This is a positive step that was certianly expected. Second the reported and measured clock frequencies do not match the device tree entries. 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 ---------------------------------------------------------------------------------------- ref27 1 1 27000000 0 0 xtal 0 0 27000000 0 0 pllb 0 0 599999994 0 0 ms2 0 0 12288000 0 0 clk2 0 0 12288000 0 0 ms0 0 0 12499999 0 0 clk0 0 0 12499999 0 0 plla 0 0 599999994 0 0 ms1 0 0 5357142 0 0 clk1 0 0 5357142 0 0 tclkin_ck 0 0 12000000 0 0 virt_26000000_ck 1 1 26000000 0 0 sys_clkin_ck 11 23 26000000 0 0 Device tree entry: si5351: clock-generator { #address-cells = <1>; #size-cells = <0>; #clock-cells = <1>; compatible = "silabs,si5351a-msop"; reg = <0x60>; status = "okay"; /* connect xtal input to 27MHz reference */ clocks = <&ref27>; clock-names = "xtal"; /* connect xtal input as source of pll0 and pll1 */ silabs,pll-source = <0 0>, <1 0>; clkout0: clkout0 { reg = <0>; silabs,drive-strength = <8>; silabs,multisynth-source = <1>; silabs,clock-source = <0>; silabs,pll-master; clock-frequency = <18432000>; }; clkout1: clkout1 { reg = <1>; silabs,drive-strength = <8>; silabs,multisynth-source = <0>; silabs,clock-source = <0>; silabs,pll-master; clock-frequency = <8000000>; }; clkout2: clkout2 { reg = <2>; silabs,drive-strength = <8>; silabs,multisynth-source = <1>; silabs,clock-source = <0>; clock-frequency = <12288000>; }; }; Lastly if #define DEBUG is added the behavior is different. 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 [ 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 ---------------------------------------------------------------------------------------- ref27 1 1 27000000 0 0 xtal 0 0 27000000 0 0 pllb 0 0 884736000 0 0 ms2 0 0 12288000 0 0 clk2 0 0 12288000 0 0 ms0 0 0 18432000 0 0 clk0 0 0 18432000 0 0 plla 0 0 895999995 0 0 ms1 0 0 7999999 0 0 clk1 0 0 7999999 0 0 tclkin_ck 0 0 12000000 0 0 virt_26000000_ck 1 1 26000000 0 0 sys_clkin_ck 11 23 26000000 0 0 It should be noted that if I program the device's register map in the bootloader the device keeps the correct frequency outputs. So the patch series appears to fix the registration issue but there is still more work to be done. Still not sure how to explain the difference when DEBUG is defined. I will dig into the datasheet and see what I can find. > Sebastian > > Sebastian Hesselbarth (4): > clk: si5351: Mention clock-names in the binding documentation > ARM: dove: Add clock-names to CuBox Si5351 clk generator > clk: si5351: Do not pass struct clk in platform_data > clk: si5351: Reset PLL after rate change > > .../devicetree/bindings/clock/silabs,si5351.txt | 4 +- > arch/arm/boot/dts/dove-cubox.dts | 1 + > drivers/clk/clk-si5351.c | 87 +++++++++++++++++----- > include/linux/platform_data/si5351.h | 4 - > 4 files changed, 73 insertions(+), 23 deletions(-) > > --- > Cc: Mike Turquette > Cc: Stephen Boyd > Cc: Jean-Francois Moine > Cc: Michael Welling > Cc: Russell King > Cc: Jason Cooper > Cc: Andrew Lunn > Cc: Gregory Clement > Cc: devicetree@vger.kernel.org > Cc: linux-clk@vger.kernel.org > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-kernel@vger.kernel.org > -- > 2.1.0 > -- 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/