Re: [PATCH] clk: add si5351 i2c common clock driver

From: Daniel Mack
Date: Mon Feb 18 2013 - 05:19:19 EST

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): [<c04ae9bc>]
_raw_spin_unlock_irqrestore+0x60/0x68
[ 5.054843] hardirqs last disabled at (136090): [<c04ae1f4>]
_raw_spin_lock_irqsave+0x1c/0x60
[ 5.063733] softirqs last enabled at (135026): [<c0039d84>]
__do_softirq+0x150/0x1b4
[ 5.071907] softirqs last disabled at (135019): [<c003a18c>]
irq_exit+0x98/0xa0
[ 5.079541] [<c0013610>] (unwind_backtrace+0x0/0xf8) from
[<c0058ec8>] (__schedule_bug+0x58/0x78)
[ 5.088793] [<c0058ec8>] (__schedule_bug+0x58/0x78) from [<c04ad50c>]
(__schedule+0x3c8/0x45c)
[ 5.097773] [<c04ad50c>] (__schedule+0x3c8/0x45c) from [<c04ab3f0>]
(schedule_timeout+0x120/0x1d4)
[ 5.107114] [<c04ab3f0>] (schedule_timeout+0x120/0x1d4) from
[<c003f780>] (msleep+0x14/0x20)
[ 5.115912] [<c003f780>] (msleep+0x14/0x20) from [<c033dd08>]
(omap_i2c_wait_for_bb+0x68/0xb0)
[ 5.124890] [<c033dd08>] (omap_i2c_wait_for_bb+0x68/0xb0) from
[<c033e528>] (omap_i2c_xfer+0x3c/0xfc)
[ 5.134503] [<c033e528>] (omap_i2c_xfer+0x3c/0xfc) from [<c033a550>]
(__i2c_transfer+0x44/0x80)
[ 5.143575] [<c033a550>] (__i2c_transfer+0x44/0x80) from [<c033bab8>]
(i2c_transfer+0x5c/0xbc)
[ 5.152555] [<c033bab8>] (i2c_transfer+0x5c/0xbc) from [<c02b4458>]
(regmap_i2c_read+0x4c/0x6c)
[ 5.161625] [<c02b4458>] (regmap_i2c_read+0x4c/0x6c) from
[<c02b1840>] (_regmap_raw_read+0x98/0xdc)
[ 5.171056] [<c02b1840>] (_regmap_raw_read+0x98/0xdc) from
[<c02b18e8>] (_regmap_read+0x64/0x9c)
[ 5.180215] [<c02b18e8>] (_regmap_read+0x64/0x9c) from [<c02b1964>]
(regmap_read+0x44/0x5c)
[ 5.188923] [<c02b1964>] (regmap_read+0x44/0x5c) from [<c036d45c>]
(si5351_clkout_is_enabled+0x74/0xb8)
[ 5.198720] [<c036d45c>] (si5351_clkout_is_enabled+0x74/0xb8) from
[<c036a9f4>] (clk_disable_unused_subtree+0x68/0xac)
[ 5.209873] [<c036a9f4>] (clk_disable_unused_subtree+0x68/0xac) from
[<c036a9ac>] (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

--
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/