From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Mack Subject: Re: [PATCH] clk: add si5351 i2c common clock driver Date: Mon, 18 Feb 2013 11:19:08 +0100 Message-ID: <5122001C.4010008@gmail.com> References: <1360414772-12232-1-git-send-email-sebastian.hesselbarth@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1360414772-12232-1-git-send-email-sebastian.hesselbarth@gmail.com> Sender: linux-doc-owner@vger.kernel.org To: Sebastian Hesselbarth Cc: Stephen Warren , linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, Rob Herring , Russell King - ARM Linux , Dom Cobley , Mike Turquette , Andrew Morton , Rabeeh Khoury , devicetree-discuss@lists.ozlabs.org, linux-arm-kernel@lists.infradead.org List-Id: devicetree@vger.kernel.org Hi Sebastian, On 09.02.2013 13:59, Sebastian Hesselbarth wrote: > This patch adds a common clock driver for Silicon Labs Si5351a/b/c > i2c programmable clock generators. Currently, the driver supports > DT kernels only and VXCO feature of si5351b is not implemented. DT > bindings selectively allow to overwrite stored Si5351 configuration > which is very helpful for clock generators with empty eeprom > configuration. Corresponding device tree binding documentation is > also added. Thank you very much for your work! I had to wait for OMAP platforms to move over to the common clock framework, but with that in place now, I could finally give it a test. Some comments below. > - The driver has been frequency tested for some common video/audio > clocks and manages it to tune in every frequency successfully. A > comparison with silabs windows tool shows a different heuristic > for vco frequencies. The tests have been comfirmed by visual > check on an 500MHz oscilloscope but no jitter measurements have > been carried out. I will provide comparison by email on request. I would be interested in more tests, yes. My first checks set up clkout1 to 32.768 KHz, and on my oscilloscope, I measured some 1.04 KHz only. Will do some debugging later. > +/* > + * Si5351 xtal clock input > + */ > +static int si5351_xtal_prepare(struct clk_hw *hw) > +{ > + struct si5351_driver_data *drvdata = > + container_of(hw, struct si5351_driver_data, xtal); > + si5351_set_bits(drvdata, SI5351_FANOUT_ENABLE, > + SI5351_XTAL_ENABLE, SI5351_XTAL_ENABLE); > + return 0; > +} > + > +static void si5351_xtal_unprepare(struct clk_hw *hw) > +{ > + struct si5351_driver_data *drvdata = > + container_of(hw, struct si5351_driver_data, xtal); > + si5351_set_bits(drvdata, SI5351_FANOUT_ENABLE, > + SI5351_XTAL_ENABLE, 0); > +} > + > +static int si5351_xtal_is_enabled(struct clk_hw *hw) > +{ > + struct si5351_driver_data *drvdata = > + container_of(hw, struct si5351_driver_data, xtal); > + unsigned char reg; > + reg = si5351_reg_read(drvdata, SI5351_FANOUT_ENABLE); > + return (reg & SI5351_XTAL_ENABLE) ? 1 : 0; > +} On an AM33xx platform, I got tons of BUGs like this one: [ 5.028525] BUG: scheduling while atomic: swapper/1/0x00000002 [ 5.034600] INFO: lockdep is turned off. [ 5.038682] Modules linked in: [ 5.041866] irq event stamp: 136090 [ 5.045496] hardirqs last enabled at (136089): [] _raw_spin_unlock_irqrestore+0x60/0x68 [ 5.054843] hardirqs last disabled at (136090): [] _raw_spin_lock_irqsave+0x1c/0x60 [ 5.063733] softirqs last enabled at (135026): [] __do_softirq+0x150/0x1b4 [ 5.071907] softirqs last disabled at (135019): [] irq_exit+0x98/0xa0 [ 5.079541] [] (unwind_backtrace+0x0/0xf8) from [] (__schedule_bug+0x58/0x78) [ 5.088793] [] (__schedule_bug+0x58/0x78) from [] (__schedule+0x3c8/0x45c) [ 5.097773] [] (__schedule+0x3c8/0x45c) from [] (schedule_timeout+0x120/0x1d4) [ 5.107114] [] (schedule_timeout+0x120/0x1d4) from [] (msleep+0x14/0x20) [ 5.115912] [] (msleep+0x14/0x20) from [] (omap_i2c_wait_for_bb+0x68/0xb0) [ 5.124890] [] (omap_i2c_wait_for_bb+0x68/0xb0) from [] (omap_i2c_xfer+0x3c/0xfc) [ 5.134503] [] (omap_i2c_xfer+0x3c/0xfc) from [] (__i2c_transfer+0x44/0x80) [ 5.143575] [] (__i2c_transfer+0x44/0x80) from [] (i2c_transfer+0x5c/0xbc) [ 5.152555] [] (i2c_transfer+0x5c/0xbc) from [] (regmap_i2c_read+0x4c/0x6c) [ 5.161625] [] (regmap_i2c_read+0x4c/0x6c) from [] (_regmap_raw_read+0x98/0xdc) [ 5.171056] [] (_regmap_raw_read+0x98/0xdc) from [] (_regmap_read+0x64/0x9c) [ 5.180215] [] (_regmap_read+0x64/0x9c) from [] (regmap_read+0x44/0x5c) [ 5.188923] [] (regmap_read+0x44/0x5c) from [] (si5351_clkout_is_enabled+0x74/0xb8) [ 5.198720] [] (si5351_clkout_is_enabled+0x74/0xb8) from [] (clk_disable_unused_subtree+0x68/0xac) [ 5.209873] [] (clk_disable_unused_subtree+0x68/0xac) from [] (clk_disable_unused_subtree+0x20/0xac) This is because clk_disable_unused_subtree() calls ->is_enabled() with a spinlock held, but si5351_xtal_is_enabled() will be sleeping, either by acquiring a mutex in the regmap core, or in the OMAP's i2c transfer routine. So bottom line is to make this callback atomic, and I did that locally for now by caching the 'prepare' state of xtal, clkin and the individual clocks. I can share a patch if you like. > +static unsigned long si5351_msynth_recalc_rate(struct clk_hw *hw, > + unsigned long parent_rate) > +{ > + struct si5351_hw_data *hwdata = > + container_of(hw, struct si5351_hw_data, hw); > + unsigned char reg = SI5351_CLK0_PARAMETERS + > + (SI5351_PARAMETERS_LENGTH * hwdata->num); > + unsigned long long rate; > + unsigned long m; > + > + if (!hwdata->params.valid) > + si5351_read_parameters(hwdata->drvdata, reg, &hwdata->params); > + > + if (hwdata->params.p3 == 0) > + return parent_rate; > + > + /* > + * multisync0-5: fOUT = (128 * P3 * fIN) / (P1*P3 + P2 + 512*P3) > + * multisync6-7: fOUT = fIN / P1 > + */ > + rate = parent_rate; > + if (hwdata->num > 5) > + m = hwdata->params.p1; > + else if ((si5351_reg_read(hwdata->drvdata, reg + 2) & > + SI5351_OUTPUT_CLK_DIVBY4) == SI5351_OUTPUT_CLK_DIVBY4) > + m = 4; > + else { > + rate *= 128 * hwdata->params.p3; > + m = hwdata->params.p1 * hwdata->params.p3; > + m += hwdata->params.p2; > + m += 512 * hwdata->params.p3; > + } > + do_div(rate, m); For p1 == p2 == p3, this will be a DIV0. I encountered this here by not specifying the clkout2 node. I think it's safe to just bail if m == 0? Again, thanks a lot for working on this! Daniel