Common clock framework for external clock generators

Hi,

first of all I apologize for the quite long attachment but I think it is useful for the following discussion.

I recently read about the newly introduced common clock framework (ccf) and wondered if this could be also used for external, e.g. i2c attached, clock generators.

Based on my current understanding of the framework I wrote such a driver and now I want to present it here for clarification of some remarks I have regarding the framework itself.

Please do not see this driver as mature but as some kind of proof-of-concept. I have the driver somewhat running but stumbled upon some issues.

First I want to give a brief overview of the intended use case of this driver:

It is a driver for a clock generator that is externally attached to a Marvell Dove (arm/mach-dove) SoC. It will provide driver configurable clocks that are connected to dedicated clock inputs of the SoC, e.g. external audio clock for i2s controller.

The basic intention I had in mind when writing this driver was to add it during platform init and pass a list of clock aliases and clock hierarchy description to allow the receiving driver, e.g. i2s, to set the rate of the supplied clock without poking the clock generator directly.

Please comment on the following aspects:
- there is no clk_unregister which is okay for platform clocks but should be there since clock generators can be detached
- the clock generator has two plls and up to 8 clocks; inside the clk_ops it is quite hard to find the correct struct clk_hw when using container_of()
- most of clk_ops are spin-locked but i2c drivers tend to sleep during read or write; this causes a "BUG: scheduling while atomic"

I know that ccf is quite new but it is well suited for generic, i.e. platform independent, external clock generator drivers. Maybe I got the overall concept just wrong but maybe this RFC helps to straighten things up for future drivers.

Regards,
Sebastian

/*
* clk-si5351.c: Silicon Laboratories Si5351A/B/C I2C Clock Generator
* (stripped for LKML discussion)
*/

struct si5351_driver_data {
	struct clk_hw		hw_xtal;
	struct clk_hw		hw_clkin;
	struct clk_hw		hw_vxco;
	struct clk_hw		hw_plla;
	struct clk_hw		hw_pllb;
	struct clk_hw		hw_clkout0;
	struct clk_hw		hw_clkout1;
	struct clk_hw		hw_clkout2;
	struct clk_hw		hw_clkout3;
	struct clk_hw		hw_clkout4;
	struct clk_hw		hw_clkout5;
	struct clk_hw		hw_clkout6;
	struct clk_hw		hw_clkout7;
	struct i2c_client	*client;
	unsigned long		fxtal;
	unsigned long		fclkin;
	u8                      variant;
	u8			clkmask;
};

static char* si5351_clkout_names[] = {
	"clkout0", "clkout1", "clkout2", "clkout3",
	"clkout4", "clkout5", "clkout6", "clkout7"};

/*
* Si5351 helper
*/

static struct clk_hw *si5351_get_clkout_hw_from_num(struct si5351_driver_data* sidata, int num)

{
	switch(num) {
	case 0: return &sidata->hw_clkout0;
	case 1: return &sidata->hw_clkout1;
	case 2: return &sidata->hw_clkout2;
	case 3: return &sidata->hw_clkout3;
	case 4: return &sidata->hw_clkout4;
	case 5: return &sidata->hw_clkout5;
	case 6: return &sidata->hw_clkout6;
	case 7: return &sidata->hw_clkout7;
	}
	return NULL;
}

/*
* Si5351 i2c register read/write
*/

static inline u8 si5351_reg_read(struct si5351_driver_data *data, u8 addr)

{
	return (u8)i2c_smbus_read_byte_data(data->client, addr);
}

static inline int si5351_regs_read(struct si5351_driver_data *data, u8 addr, u8 length, void *buf)

{
	return i2c_smbus_read_i2c_block_data(data->client, addr, length, buf);
}

static inline int si5351_reg_write(struct si5351_driver_data *data, u8 addr, u8 val)

{
	return i2c_smbus_write_byte_data(data->client, addr, val);
}

static inline int si5351_regs_write(struct si5351_driver_data *data, u8 addr, u8 length, const void *buf)

{
	return i2c_smbus_write_i2c_block_data(data->client, addr, length, buf);
}

/*
* Si5351 xtal clock input
*/

static int si5351_xtal_enable(struct clk_hw *hw)

{
	struct si5351_driver_data *sidata = 
		container_of(hw, struct si5351_driver_data, hw_xtal);
	u8 reg;

	reg = si5351_reg_read(sidata, SI5351_FANOUT_ENABLE);
	reg |= SI5351_XTAL_ENABLE;
	si5351_reg_write(sidata, SI5351_FANOUT_ENABLE, reg);
	return 0;
}

static void si5351_xtal_disable(struct clk_hw *hw)

{
	struct si5351_driver_data *sidata = 
		container_of(hw, struct si5351_driver_data, hw_xtal);
	u8 reg;

	reg = si5351_reg_read(sidata, SI5351_FANOUT_ENABLE);
	reg &= ~SI5351_XTAL_ENABLE;
	si5351_reg_write(sidata, SI5351_FANOUT_ENABLE, reg);
}

static unsigned long si5351_xtal_recalc_rate(struct clk_hw *hw,
unsigned long parent_rate)

{
	struct si5351_driver_data *sidata = 
		container_of(hw, struct si5351_driver_data, hw_xtal);
	return sidata->fxtal;
}

static const struct clk_ops si5351_xtal_ops = {
	.enable = si5351_xtal_enable,
	.disable = si5351_xtal_disable,
	.recalc_rate = si5351_xtal_recalc_rate,
};

/*
* Si5351 pll a/b
*/
static struct si5351_driver_data *si5351_pll_get_data(struct clk_hw *hw)

{
	if (strncmp(hw->clk->name, "plla", 4) == 0)
		return container_of(hw, struct si5351_driver_data, hw_plla);
	return container_of(hw, struct si5351_driver_data, hw_pllb);
}

static u8 si5351_pll_get_parent(struct clk_hw *hw)

{
	struct si5351_driver_data *sidata = 
		container_of(hw, struct si5351_driver_data, hw_plla);
	u8 mask = (hw == &sidata->hw_plla) ? 
		SI5351_PLLA_SOURCE : SI5351_PLLB_SOURCE;
	u8 reg;

	if (sidata->variant != SI5351_VARIANT_C)
		return 0;
	
	reg = si5351_reg_read(sidata, SI5351_PLL_INPUT_SOURCE);

	return (reg & mask) ? 1 : 0;
}

static int si5351_pll_set_parent(struct clk_hw *hw, u8 index)

{
	struct si5351_driver_data *sidata = 
		container_of(hw, struct si5351_driver_data, hw_plla);
	u8 mask = (hw == &sidata->hw_plla) ? 
		SI5351_PLLA_SOURCE : SI5351_PLLB_SOURCE;
	u8 reg;

	if (sidata->variant != SI5351_VARIANT_C)
		return -EPERM;

	if (index > 1)
		return -EINVAL;

	reg = si5351_reg_read(sidata, SI5351_PLL_INPUT_SOURCE);
	if (index)
		reg |= mask;
	else
		reg &= ~mask;
	si5351_reg_write(sidata, SI5351_PLL_INPUT_SOURCE, reg);

	return 0;
}

static unsigned long si5351_pll_recalc_rate(struct clk_hw *hw,
unsigned long parent_rate)

{
	struct si5351_driver_data *sidata = si5351_pll_get_data(hw);
	struct si5351_parameters params;
	unsigned long m;
	unsigned long long rate;
	u32 p1, p2, p3;
	u8 addr = (hw == &sidata->hw_plla) ? SI5351_PLLA_PARAMETERS : SI5351_PLLB_PARAMETERS;

	si5351_regs_read(sidata, addr, SI5351_PARAMETERS_LENGTH, &params);

	p1 = ((params.p1_high    & 0x03) << 16) | (params.p1_mid << 8) | params.p1_low;
	p2 = ((params.p2_p3_high & 0x0f) << 16) | (params.p2_mid << 8) | params.p2_low;
	p3 = ((params.p2_p3_high & 0xf0) << 12) | (params.p3_mid << 8) | params.p3_low;

	if (p3 == 0)
		return 0;

	rate  = (p1*p3 + p2 + 512*p3);
	rate *= parent_rate;
	do_div(rate,p3);
	do_div(rate,128);

	return (unsigned long)rate;
}

static long si5351_pll_round_rate(struct clk_hw *hw, unsigned long rate,
unsigned long *parent_rate)

{
	struct si5351_driver_data *sidata = si5351_pll_get_data(hw);

	return rate;
}

static const struct clk_ops si5351_pll_ops = {
	.set_parent = si5351_pll_set_parent,
	.get_parent = si5351_pll_get_parent,
	.recalc_rate = si5351_pll_recalc_rate,
	.round_rate = si5351_pll_round_rate,
};

/*
* Si5351 clkout
*/
static struct si5351_driver_data *si5351_clkout_get_data(struct clk_hw *hw)

{
	char index = hw->clk->name[6];
	switch(index) {
	case '0': return container_of(hw, struct si5351_driver_data, hw_clkout0);
	case '1': return container_of(hw, struct si5351_driver_data, hw_clkout1);
	case '2': return container_of(hw, struct si5351_driver_data, hw_clkout2);
	case '3': return container_of(hw, struct si5351_driver_data, hw_clkout3);
	case '4': return container_of(hw, struct si5351_driver_data, hw_clkout4);
	case '5': return container_of(hw, struct si5351_driver_data, hw_clkout5);
	case '6': return container_of(hw, struct si5351_driver_data, hw_clkout6);
	case '7': return container_of(hw, struct si5351_driver_data, hw_clkout7);
	}
	return NULL;
}

static int si5351_clkout_enable(struct clk_hw *hw)

{
	struct si5351_driver_data *sidata = si5351_clkout_get_data(hw);
	return 0;
}

static void si5351_clkout_disable(struct clk_hw *hw)

{
	struct si5351_driver_data *sidata = si5351_clkout_get_data(hw);
}

static int si5351_clkout_set_parent(struct clk_hw *hw, u8 index)

{
	struct si5351_driver_data *sidata = si5351_clkout_get_data(hw);
	return 0;
}

static u8 si5351_clkout_get_parent(struct clk_hw *hw)

{
	struct si5351_driver_data *sidata = si5351_clkout_get_data(hw);
	return 0;
}

static unsigned long si5351_clkout_recalc_rate(struct clk_hw *hw,
unsigned long parent_rate)

{
	struct si5351_driver_data *sidata = si5351_clkout_get_data(hw);
	return 0;
}

static long si5351_clkout_round_rate(struct clk_hw *hw, unsigned long rate,
unsigned long *parent_rate)

{
	struct si5351_driver_data *sidata = si5351_clkout_get_data(hw);
	return 0;
}

static const struct clk_ops si5351_clkout_ops = {
	.enable = si5351_clkout_enable,
	.disable = si5351_clkout_disable,
	.set_parent = si5351_clkout_set_parent,
	.get_parent = si5351_clkout_get_parent,
	.recalc_rate = si5351_clkout_recalc_rate,
	.round_rate = si5351_clkout_round_rate,
};

/*
* Si5351 i2c probe
*/

static int si5351_probe(struct i2c_client *client,
const struct i2c_device_id *id)

{
	struct si5351_driver_data *sidata;
	struct si5351_clocks_data *drvdata = 
		(struct si5351_clocks_data *)client->dev.platform_data;

	sidata = kzalloc(sizeof(struct si5351_driver_data), GFP_KERNEL);
	if (sidata == NULL) {
		dev_err(&client->dev, "unable to allocate driver data\n");
		return -ENOMEM;
	}

	i2c_set_clientdata(client, sidata);
	sidata->client = client;
	sidata->fxtal = drvdata->fxtal;

	if (!clk_register(&client->dev, "xtal", &si5351_xtal_ops, 
			  &sidata->hw_xtal, NULL, 0, CLK_IS_ROOT)) {
		dev_err(&client->dev, "unable to register xtal\n");
		ret = -EINVAL;
		goto si5351_probe_error_register;
	}

	if (!clk_register(&client->dev, "plla", &si5351_pll_ops, 
			  &sidata->hw_plla, si5351_common_pll_parents, 
			  num_parents, 0)) {
		dev_err(&client->dev, "unable to register pll a\n");
		ret = -EINVAL;
		goto si5351_probe_error_register;
	}

	if (!clk_register(&client->dev, "pllb", &si5351_pll_ops, 
			  &sidata->hw_pllb, pll_parents, num_parents, 0)) {
		dev_err(&client->dev, "unable to register pll b\n");
		ret = -EINVAL;
		goto si5351_probe_error_register;
	}

	/* Disable interrupts */
	si5351_reg_write(sidata, SI5351_INTERRUPT_MASK, 0xf0);
	/* Set disabled output drivers to drive low */
	si5351_reg_write(sidata, SI5351_CLK3_0_DISABLE_STATE, 0x00);
	si5351_reg_write(sidata, SI5351_CLK7_4_DISABLE_STATE, 0x00);
	/* Disable outputs */
	si5351_reg_write(sidata, SI5351_OUTPUT_ENABLE_CTRL, 0xff);
	/* Power down output drivers */
	for(i=SI5351_CLK0_CTRL; i<=SI5351_CLK7_CTRL; i++)
		si5351_reg_write(sidata, i, SI5351_CLK_POWERDOWN);

	sidata->clkmask = 0x00;
	for(i=0; i<num_clocks; i++) {
		struct clk *clk;
		struct clk_lookup *cl;
		clk = clk_register(&client->dev, si5351_clkout_names[i], 
				   &si5351_clkout_ops, 
				   si5351_get_clkout_hw_from_num(sidata, i), 
				   si5351_common_clkout_parents, num_parents, 0);
		if (IS_ERR(clk)) {
			dev_err(&client->dev, "unable to register %s\n", 
				si5351_clkout_names[i]);
			ret = -EINVAL;
			goto si5351_probe_error_register;
		}
		cl = clkdev_alloc(clk, si5351_clkout_names[i], dev_name(&client->dev));
		if (cl)
			clkdev_add(cl);
	}

	drvdata->clkdev = &client->dev;
	for(i=0; i<drvdata->num_clocks; i++) {
		const struct si5351_clock_descr *descr = &drvdata->clocks[i];
		struct clk_hw *hw;
		u8 clkbit = (1 << descr->clkout);
		
		if (sidata->clkmask & clkbit) {
			dev_err(&client->dev, "slip already allocated clkout%d\n", 
				descr->clkout); 
			continue;
		}
		
		sidata->clkmask |= clkbit;

		hw = si5351_get_clkout_hw_from_num(sidata, descr->clkout);
		if (descr->pllmaster)
			hw->clk->flags |= CLK_SET_RATE_PARENT;

		switch(descr->clksrc) {
		case SI5351_CLKINSEL_PLLA:
			clk_set_parent(hw->clk, sidata->hw_plla.clk);
			break;
		case SI5351_CLKINSEL_PLLB:
			clk_set_parent(hw->clk, sidata->hw_pllb.clk);
			break;
		case SI5351_CLKINSEL_XTAL:
			clk_set_parent(hw->clk, sidata->hw_xtal.clk);
			break;
		}
		clk_add_alias(descr->alias, descr->alias_dev, 
			      hw->clk->name, &client->dev);
	}

	return 0;

si5351_probe_error_register:

	i2c_set_clientdata(client, NULL);
	kfree(sidata);
	return ret;
}

static int si5351_remove(struct i2c_client *client)

{
	struct si5351_driver_data *sidata = i2c_get_clientdata(client);
	i2c_set_clientdata(client, NULL);
	kfree(sidata);
	return 0;
}

On Thu, May 10, 2012 at 03:11:55AM +0200, Sebastian Hesselbarh wrote:

Always CC maintainers on mails, things get lost on high volume lists.

Yes, it should be able to be used for this. I've been posting a driver for the wm831x PMICs for quite some considerable time.

I'd suggest using regmap for the cache support (which helps with read/modify/write cycles) and because...

...we should be able to factor things like this out of the drivers using regmap (we've been getting some great results doing that for regulator over this release).

These should be prepare() and unprepare() - enable() and disable() are called from atomic context so you can't do I2C I/O.

If we need empty functions like this we should fix the framework.

devm_kzalloc().

Yeah, right after posting I thought of this but didn't want to bother you again. I read a lot of kernel code since then and some things are much more clearer to me now. Besides the driver is almost done now and works as expected. Will have to clean up things and will post it as a patch then.

I will have a look at regmap before I post a new driver patch for sure.

You are right. The kernel code states this correctly and I should have read it more carefully.

Of course, this function is not empty at all but wasn't done at the time I posted the RFC.

One question that remains from my original RFC is the missing clk_unregister() as i2c can be removed there also should be an function to unregister previously registered clocks?

Sebastian

One of the patches I've been sending adds a dummy clk_unregister() for the sake of making the drivers look nicer - practically speaking it's not likely to be terribly important as these things don't get unloaded terribly often. It looks like that patch didn't get applied either.

Mike's stuff is due to hit -next on Monday so I was planning to refresh my driver and resend it then, probably with the unregister() though I'm considering just removing the unregistration code to try to get make it a bit more likely to get applied.

Well, of course I don't plan to unload the driver ever but basically it is possible..

One more thing I thought about: The platform I currently use needs to pass the external clocks to the platform devices that can use them later. IMHO the correct way of creating clocks would be:

- register i2c clock driver and let it register its clocks with names like e.g. si5351, clkout0. The clock driver itself cannot and should not know who uses it later on.
- let drivers look for e.g. kirkwood-i2s.1, extclk because the i2s driver cannot know where the external clock comes from.
- have a board-specific function that configures clock hierarchy and create suitable clk_aliases e.g.
si5351,clkout0 = kirkwood-i2s.1,extclk.

Currently I added a callback function pointer to the platform data passed to the i2c clock driver that is called at the end of clock driver probe. I doubt it will be accepted that way but can't think of any other way..

Sebastian

This is what clkdev is for - it provides such a layer of indirection. The only trick right now is that it needs the struct clk to add the lookup but some platform data for your driver would probably do the trick in the short term, I don't know what the plan is long term with that stuff.

Mark,

Seems like the clk_unregister patch fell through the cracks. My mistake. I'm going to be sending another pull request to arm-soc with Orion/Kirkwood support. I'll add your patch in at that time.

Sebastian,

Please CC me on the next revision of your patch. Mark's review hit all of the high points for this version.

Regards,
Mike

Ah, great. I dropped usage from the wm831x driver I just posted, I'd rather get that merged if I can, but I can easily add it back - I'll either roll it in to the next version or send a followup if this version is basically OK.

Hi Sebastian,

May I kindly ask what happened to this driver? Are you planning to do another spin for submission?

Many thanks,
Daniel

Daniel,

the driver is still on my list but moved more and more downwards as I
got caught by mach-dove DT integration. There is still some work to do,
namely regmap and DT. It is used by some off-mainline tree for SolidRun CuBox quite successfully since then. Does any of you work rely on a working si5351 driver?

Sebastian

Yes, it does actually. I can hack around it for now, but at some point, a proper driver is needed. And yours looks quite feature complete, so it would be easiest to finish this one :)

Do you still have access to hardware you wrote the driver for? Let me know if you need any help around here.

Thanks,
Daniel

Adding LAKML and devtree as there might be people willing to comment about DT representation of i2c-attached clock generators, too.

Well, yes it is some kind of feature complete except regmap and DT. Adding regmap to the driver should be quite easy but with DT I am still thinking of the best way to represent the internal connections between OSC, PLLs, and CLKOUTs. In the last version of the driver I had a callback that was board specific to setup these connections but with DT there will be no board specific code anymore.

Maybe one of the common clk maintainers can give a hint how this could be done in a clean way. The question is how to represent a i2c-attached clock generator config in DT where you want to setup clock parents of CLKOUTs and PLLs.

A possible solution would be something like this:

si5351a at 60 {
	compatible = "silabs,si5351a";
	reg = <0x60>;

	si_osc: osc {
		compatible = "fixed-clock";
		clock-frequency = <270000000>;
	};

	si_plla: pll at 0 {
		compatible = "silabs,si5351-pll";
		/* hook-up plla to osc input */
		clocks = <&si_osc>;
	};

	si_clkout0: clkout at 0 {
		compatible = "silabs,si5351-clkout";
		/* hook-up clkout0 to plla */
		clocks = <&si_plla>;
		/* request target frequency */
		clock-frequency = <148500000>;
	};	
};

Although this perfectly describes the clock hierarchy I still don't like the sub-node style. Another flat solution would be something like:

si_osc: osc {
	compatible = "fixed-clock";
	clock-frequency = <270000000>;
};

si5351a at 60 {
	compatible = "silabs,si5351a";
	reg = <0x60>;
	clocks = <&si_osc>;

	si5351-pll-config = <0 0   /* pll A to osc */
			     1 0>; /* pll B to osc */

	si5351-clock-config = <0 0 148500000   /* clkout 0 to pll A, 148.5MHz */
			       1 0         0   /* clkout 1 to pll A, disabled */
			       2 1  24000000>; /* clkout 2 to pll B,  24.0Mhz */
};

Yes, hardware is still available although I only have access to the Si5351a with 3 clkouts. The code should be compatible for Si5351a with 8 clkouts and code skeleton for 5351b (OSC and VXCO input) and 5351c (OSC and CLKIN) is there but untested.

I've transferred the current driver version to my repository to work on. As soon as I have regmap done, I will push it online and give you a note.

Sebastian

Hi,

I think the sub-node style above it nicer because it allows referencing the individual clocks outputs with a phandle. We use this chip to generate base-frequencies for audio clocks, and so we have to switch between two freqs for the multiples of 22.5KHz and 24KHz at runtime.

The 3 clkout model is the only one I have access to as well.

That's great. Let me know if you want me to test anything.

Thanks,
Daniel

Both examples allow you to have a phandle for all individual clock-outputs. The examples weren't complete but with the sub-node style you'll reference with e.g. <&clkout0> while the flat one will use <&si5351 0>. I still prefer the flat-style as it will not allow to have a phandle of plls.

Sebastian