All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC] Common clock framework for external clock generators
@ 2012-05-10  1:11 Sebastian Hesselbarh
  2012-05-13 12:29 ` Mark Brown
  2012-10-11  8:34 ` Daniel Mack
  0 siblings, 2 replies; 17+ messages in thread
From: Sebastian Hesselbarh @ 2012-05-10  1:11 UTC (permalink / raw)
  To: linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1935 bytes --]

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







[-- Attachment #2: clk-si5351.lkml.c --]
[-- Type: text/x-csrc, Size: 10705 bytes --]

/*
 * 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;
}


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC] Common clock framework for external clock generators
  2012-05-10  1:11 [RFC] Common clock framework for external clock generators Sebastian Hesselbarh
@ 2012-05-13 12:29 ` Mark Brown
  2012-05-13 16:30   ` Sebastian Hesselbarh
  2012-10-11  8:34 ` Daniel Mack
  1 sibling, 1 reply; 17+ messages in thread
From: Mark Brown @ 2012-05-13 12:29 UTC (permalink / raw)
  To: Sebastian Hesselbarh; +Cc: linux-kernel, mturquette

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.

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

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.

> static inline u8 si5351_reg_read(struct si5351_driver_data *data, u8 addr)
> {
> 	return (u8)i2c_smbus_read_byte_data(data->client, addr);
> }

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

> 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;
> }

...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).

> static const struct clk_ops si5351_xtal_ops = {
> 	.enable = si5351_xtal_enable,
> 	.disable = si5351_xtal_disable,

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

> static u8 si5351_clkout_get_parent(struct clk_hw *hw)
> {
> 	struct si5351_driver_data *sidata = si5351_clkout_get_data(hw);
> 	return 0;
> }

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

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

devm_kzalloc().

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC] Common clock framework for external clock generators
  2012-05-13 12:29 ` Mark Brown
@ 2012-05-13 16:30   ` Sebastian Hesselbarh
  2012-05-13 16:43     ` Mark Brown
  0 siblings, 1 reply; 17+ messages in thread
From: Sebastian Hesselbarh @ 2012-05-13 16:30 UTC (permalink / raw)
  To: Mark Brown; +Cc: linux-kernel, mturquette

On 05/13/2012 02:29 PM, Mark Brown wrote:
> Always CC maintainers on mails, things get lost on high volume lists.

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'd suggest using regmap for the cache support (which helps with
> read/modify/write cycles) and because...

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

>> static const struct clk_ops si5351_xtal_ops = {
>> 	.enable = si5351_xtal_enable,
>> 	.disable = si5351_xtal_disable,
>
> These should be prepare() and unprepare() - enable() and disable() are
> called from atomic context so you can't do I2C I/O.

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

>> static u8 si5351_clkout_get_parent(struct clk_hw *hw)
>> {
>> 	struct si5351_driver_data *sidata = si5351_clkout_get_data(hw);
>> 	return 0;
>> }
>
> If we need empty functions like this we should fix the framework.

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC] Common clock framework for external clock generators
  2012-05-13 16:30   ` Sebastian Hesselbarh
@ 2012-05-13 16:43     ` Mark Brown
  2012-05-13 17:11       ` Sebastian Hesselbarh
  0 siblings, 1 reply; 17+ messages in thread
From: Mark Brown @ 2012-05-13 16:43 UTC (permalink / raw)
  To: Sebastian Hesselbarh; +Cc: linux-kernel, mturquette

[-- Attachment #1: Type: text/plain, Size: 788 bytes --]

On Sun, May 13, 2012 at 06:30:55PM +0200, Sebastian Hesselbarh wrote:

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

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC] Common clock framework for external clock generators
  2012-05-13 16:43     ` Mark Brown
@ 2012-05-13 17:11       ` Sebastian Hesselbarh
  2012-05-13 17:16         ` Mark Brown
  2012-05-14 18:08         ` Turquette, Mike
  0 siblings, 2 replies; 17+ messages in thread
From: Sebastian Hesselbarh @ 2012-05-13 17:11 UTC (permalink / raw)
  To: Mark Brown; +Cc: linux-kernel, mturquette

On 05/13/2012 06:43 PM, Mark Brown wrote:
> 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.

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC] Common clock framework for external clock generators
  2012-05-13 17:11       ` Sebastian Hesselbarh
@ 2012-05-13 17:16         ` Mark Brown
  2012-05-14 18:08         ` Turquette, Mike
  1 sibling, 0 replies; 17+ messages in thread
From: Mark Brown @ 2012-05-13 17:16 UTC (permalink / raw)
  To: Sebastian Hesselbarh; +Cc: linux-kernel, mturquette

[-- Attachment #1: Type: text/plain, Size: 1252 bytes --]

On Sun, May 13, 2012 at 07:11:09PM +0200, Sebastian Hesselbarh wrote:

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

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC] Common clock framework for external clock generators
  2012-05-13 17:11       ` Sebastian Hesselbarh
  2012-05-13 17:16         ` Mark Brown
@ 2012-05-14 18:08         ` Turquette, Mike
  2012-05-14 18:12           ` Mark Brown
  1 sibling, 1 reply; 17+ messages in thread
From: Turquette, Mike @ 2012-05-14 18:08 UTC (permalink / raw)
  To: Sebastian Hesselbarh; +Cc: Mark Brown, linux-kernel

On Sun, May 13, 2012 at 10:11 AM, Sebastian Hesselbarh
<sebastian.hesselbarth@googlemail.com> wrote:
> On 05/13/2012 06:43 PM, Mark Brown wrote:
>>
>> 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.
>
>
> Well, of course I don't plan to unload the driver ever but basically it
> is possible..
>

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC] Common clock framework for external clock generators
  2012-05-14 18:08         ` Turquette, Mike
@ 2012-05-14 18:12           ` Mark Brown
  0 siblings, 0 replies; 17+ messages in thread
From: Mark Brown @ 2012-05-14 18:12 UTC (permalink / raw)
  To: Turquette, Mike; +Cc: Sebastian Hesselbarh, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 500 bytes --]

On Mon, May 14, 2012 at 11:08:58AM -0700, Turquette, Mike wrote:

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

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC] Common clock framework for external clock generators
  2012-05-10  1:11 [RFC] Common clock framework for external clock generators Sebastian Hesselbarh
  2012-05-13 12:29 ` Mark Brown
@ 2012-10-11  8:34 ` Daniel Mack
  2012-10-11 16:00   ` Sebastian Hesselbarth
  1 sibling, 1 reply; 17+ messages in thread
From: Daniel Mack @ 2012-10-11  8:34 UTC (permalink / raw)
  To: Sebastian Hesselbarh; +Cc: linux-kernel, Mark Brown, mturquette

Hi Sebastian,

On 10.05.2012 03:11, Sebastian Hesselbarh wrote:
> 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.

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


Many thanks,
Daniel




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


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC] Common clock framework for external clock generators
  2012-10-11  8:34 ` Daniel Mack
@ 2012-10-11 16:00   ` Sebastian Hesselbarth
  2012-10-12 18:17     ` Daniel Mack
  0 siblings, 1 reply; 17+ messages in thread
From: Sebastian Hesselbarth @ 2012-10-11 16:00 UTC (permalink / raw)
  To: Daniel Mack; +Cc: Sebastian Hesselbarh, linux-kernel, Mark Brown, mturquette

On 10/11/2012 10:34 AM, Daniel Mack wrote:
>> 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.
>
> May I kindly ask what happened to this driver? Are you planning to do
> another spin for submission?

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC] Common clock framework for external clock generators
  2012-10-11 16:00   ` Sebastian Hesselbarth
@ 2012-10-12 18:17     ` Daniel Mack
  2012-10-14 10:59         ` Sebastian Hesselbarth
  0 siblings, 1 reply; 17+ messages in thread
From: Daniel Mack @ 2012-10-12 18:17 UTC (permalink / raw)
  To: Sebastian Hesselbarth
  Cc: Sebastian Hesselbarh, linux-kernel, Mark Brown, mturquette

On 11.10.2012 18:00, Sebastian Hesselbarth wrote:
> On 10/11/2012 10:34 AM, Daniel Mack wrote:
>>> 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.
>>
>> May I kindly ask what happened to this driver? Are you planning to do
>> another spin for submission?
> 
> 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?

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


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC] Common clock framework for external clock generators
  2012-10-12 18:17     ` Daniel Mack
@ 2012-10-14 10:59         ` Sebastian Hesselbarth
  0 siblings, 0 replies; 17+ messages in thread
From: Sebastian Hesselbarth @ 2012-10-14 10:59 UTC (permalink / raw)
  To: Daniel Mack
  Cc: linux-kernel, Mark Brown, Mike Turquette, linux ARM,
	Grant Likely, Rob Herring, devicetree-discuss

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

On 10/12/2012 08:17 PM, Daniel Mack wrote:
> On 11.10.2012 18:00, Sebastian Hesselbarth wrote:
 >> [...]
>> Does any of you work rely on a working si5351 driver?
>
> 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 :)

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@60 {
	compatible = "silabs,si5351a";
	reg = <0x60>;

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

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

	si_clkout0: clkout@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@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 */
};

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

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [RFC] Common clock framework for external clock generators
@ 2012-10-14 10:59         ` Sebastian Hesselbarth
  0 siblings, 0 replies; 17+ messages in thread
From: Sebastian Hesselbarth @ 2012-10-14 10:59 UTC (permalink / raw)
  To: linux-arm-kernel

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

On 10/12/2012 08:17 PM, Daniel Mack wrote:
> On 11.10.2012 18:00, Sebastian Hesselbarth wrote:
 >> [...]
>> Does any of you work rely on a working si5351 driver?
>
> 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 :)

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 */
};

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

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC] Common clock framework for external clock generators
  2012-10-14 10:59         ` Sebastian Hesselbarth
@ 2012-10-14 11:13           ` Daniel Mack
  -1 siblings, 0 replies; 17+ messages in thread
From: Daniel Mack @ 2012-10-14 11:13 UTC (permalink / raw)
  To: Sebastian Hesselbarth
  Cc: linux-kernel, Mark Brown, Mike Turquette, linux ARM,
	Grant Likely, Rob Herring, devicetree-discuss

Hi,

On 14.10.2012 12:59, Sebastian Hesselbarth wrote:
> Adding LAKML and devtree as there might be people willing to comment about
> DT representation of i2c-attached clock generators, too.
> 
> On 10/12/2012 08:17 PM, Daniel Mack wrote:
>> On 11.10.2012 18:00, Sebastian Hesselbarth wrote:
>  >> [...]
>>> Does any of you work rely on a working si5351 driver?
>>
>> 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 :)
> 
> 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@60 {
> 	compatible = "silabs,si5351a";
> 	reg = <0x60>;
> 
> 	si_osc: osc {
> 		compatible = "fixed-clock";
> 		clock-frequency = <270000000>;
> 	};
> 
> 	si_plla: pll@0 {
> 		compatible = "silabs,si5351-pll";
> 		/* hook-up plla to osc input */
> 		clocks = <&si_osc>;
> 	};
> 
> 	si_clkout0: clkout@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:

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.

>> Do you still have access to hardware you wrote the driver for? Let me
>> know if you need any help around here.
> 
> 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.

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

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

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


Thanks,
Daniel



^ permalink raw reply	[flat|nested] 17+ messages in thread

* [RFC] Common clock framework for external clock generators
@ 2012-10-14 11:13           ` Daniel Mack
  0 siblings, 0 replies; 17+ messages in thread
From: Daniel Mack @ 2012-10-14 11:13 UTC (permalink / raw)
  To: linux-arm-kernel

Hi,

On 14.10.2012 12:59, Sebastian Hesselbarth wrote:
> Adding LAKML and devtree as there might be people willing to comment about
> DT representation of i2c-attached clock generators, too.
> 
> On 10/12/2012 08:17 PM, Daniel Mack wrote:
>> On 11.10.2012 18:00, Sebastian Hesselbarth wrote:
>  >> [...]
>>> Does any of you work rely on a working si5351 driver?
>>
>> 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 :)
> 
> 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:

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.

>> Do you still have access to hardware you wrote the driver for? Let me
>> know if you need any help around here.
> 
> 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.

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

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

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


Thanks,
Daniel

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC] Common clock framework for external clock generators
  2012-10-14 11:13           ` Daniel Mack
@ 2012-10-14 16:16             ` Sebastian Hesselbarth
  -1 siblings, 0 replies; 17+ messages in thread
From: Sebastian Hesselbarth @ 2012-10-14 16:16 UTC (permalink / raw)
  To: Daniel Mack
  Cc: linux-kernel, Mark Brown, Mike Turquette, linux ARM,
	Grant Likely, Rob Herring, devicetree-discuss

On 10/14/2012 01:13 PM, Daniel Mack wrote:
> 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.

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


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [RFC] Common clock framework for external clock generators
@ 2012-10-14 16:16             ` Sebastian Hesselbarth
  0 siblings, 0 replies; 17+ messages in thread
From: Sebastian Hesselbarth @ 2012-10-14 16:16 UTC (permalink / raw)
  To: linux-arm-kernel

On 10/14/2012 01:13 PM, Daniel Mack wrote:
> 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.

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2012-10-14 16:16 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-05-10  1:11 [RFC] Common clock framework for external clock generators Sebastian Hesselbarh
2012-05-13 12:29 ` Mark Brown
2012-05-13 16:30   ` Sebastian Hesselbarh
2012-05-13 16:43     ` Mark Brown
2012-05-13 17:11       ` Sebastian Hesselbarh
2012-05-13 17:16         ` Mark Brown
2012-05-14 18:08         ` Turquette, Mike
2012-05-14 18:12           ` Mark Brown
2012-10-11  8:34 ` Daniel Mack
2012-10-11 16:00   ` Sebastian Hesselbarth
2012-10-12 18:17     ` Daniel Mack
2012-10-14 10:59       ` Sebastian Hesselbarth
2012-10-14 10:59         ` Sebastian Hesselbarth
2012-10-14 11:13         ` Daniel Mack
2012-10-14 11:13           ` Daniel Mack
2012-10-14 16:16           ` Sebastian Hesselbarth
2012-10-14 16:16             ` Sebastian Hesselbarth

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.