[PATCH] clk: add si5351 i2c common clock driver

[Daniel Mack]

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