[5/5] clk: add a fixed factor clock

From: Sascha Hauer <s.hauer@pengutronix.de>

Having fixed factors/dividers in hardware is a common pattern, so
add a basic clock type doing this. It basically describes a fixed
factor clock using a nominator and a denominator.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Reviewed-by: Viresh Kumar <viresh.kumar@st.com>
[mturquette@linaro.org: constify parent_names in static init macro]
[mturquette@linaro.org: copy/paste bug from mux in static init macro]
[mturquette@linaro.org: fix error handling in clk_register_fixed_factor]
Signed-off-by: Mike Turquette <mturquette@linaro.org>
---
 drivers/clk/Makefile           |    2 +-
 drivers/clk/clk-fixed-factor.c |   95 ++++++++++++++++++++++++++++++++++++++++
 include/linux/clk-private.h    |   20 ++++++++
 include/linux/clk-provider.h   |   23 ++++++++++
 4 files changed, 139 insertions(+), 1 deletions(-)
 create mode 100644 drivers/clk/clk-fixed-factor.c

On 05/06/2012 10:08 PM, Mike Turquette wrote:
> From: Sascha Hauer<s.hauer@pengutronix.de>
>
> Having fixed factors/dividers in hardware is a common pattern, so
> add a basic clock type doing this. It basically describes a fixed
> factor clock using a nominator and a denominator.
>
> Signed-off-by: Sascha Hauer<s.hauer@pengutronix.de>
> Reviewed-by: Viresh Kumar<viresh.kumar@st.com>
> [mturquette@linaro.org: constify parent_names in static init macro]
> [mturquette@linaro.org: copy/paste bug from mux in static init macro]
> [mturquette@linaro.org: fix error handling in clk_register_fixed_factor]
> Signed-off-by: Mike Turquette<mturquette@linaro.org>
> ---
>   drivers/clk/Makefile           |    2 +-
>   drivers/clk/clk-fixed-factor.c |   95 ++++++++++++++++++++++++++++++++++++++++
>   include/linux/clk-private.h    |   20 ++++++++
>   include/linux/clk-provider.h   |   23 ++++++++++
>   4 files changed, 139 insertions(+), 1 deletions(-)
>   create mode 100644 drivers/clk/clk-fixed-factor.c
>
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index 1f736bc..24aa714 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -1,4 +1,4 @@
>
>   obj-$(CONFIG_CLKDEV_LOOKUP)	+= clkdev.o
>   obj-$(CONFIG_COMMON_CLK)	+= clk.o clk-fixed-rate.o clk-gate.o \
> -				   clk-mux.o clk-divider.o
> +				   clk-mux.o clk-divider.o clk-fixed-factor.o
> diff --git a/drivers/clk/clk-fixed-factor.c b/drivers/clk/clk-fixed-factor.c
> new file mode 100644
> index 0000000..1f2da01
> --- /dev/null
> +++ b/drivers/clk/clk-fixed-factor.c
> @@ -0,0 +1,95 @@
> +/*
> + * Copyright (C) 2011 Sascha Hauer, Pengutronix<s.hauer@pengutronix.de>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * Standard functionality for the common clock API.
> + */
> +#include<linux/module.h>
> +#include<linux/clk-provider.h>
> +#include<linux/slab.h>
> +#include<linux/err.h>
> +
> +/*
> + * DOC: basic fixed multiplier and divider clock that cannot gate
> + *
> + * Traits of this clock:
> + * prepare - clk_prepare only ensures that parents are prepared
> + * enable - clk_enable only ensures that parents are enabled
> + * rate - rate is fixed.  clk->rate = parent->rate / div * mult
> + * parent - fixed parent.  No clk_set_parent support
> + */
> +
> +#define to_clk_fixed_factor(_hw) container_of(_hw, struct clk_fixed_factor, hw)
> +
> +static unsigned long clk_factor_recalc_rate(struct clk_hw *hw,
> +		unsigned long parent_rate)
> +{
> +	struct clk_fixed_factor *fix = to_clk_fixed_factor(hw);
> +
> +	return (parent_rate / fix->div) * fix->mult;

Wouldn't multiplying and then dividing result in better accuracy? Is it 
worth doing that (since we will have to account for overflow)?

> +}
> +
> +static long clk_factor_round_rate(struct clk_hw *hw, unsigned long rate,
> +				unsigned long *prate)
> +{
> +	struct clk_fixed_factor *fix = to_clk_fixed_factor(hw);
> +
> +	if (__clk_get_flags(hw->clk)&  CLK_SET_RATE_PARENT) {
> +		unsigned long best_parent;
> +
> +		best_parent = (rate / fix->mult) * fix->div;
> +		*prate = __clk_round_rate(__clk_get_parent(hw->clk),
> +				best_parent);
> +	}
> +
> +	return (*prate / fix->div) * fix->mult;
> +}
> +
> +static int clk_factor_set_rate(struct clk_hw *hw, unsigned long rate,
> +				unsigned long parent_rate)
> +{
> +	return 0;
> +}
> +
> +struct clk_ops clk_fixed_factor_ops = {
> +	.round_rate = clk_factor_round_rate,
> +	.set_rate = clk_factor_set_rate,
> +	.recalc_rate = clk_factor_recalc_rate,
> +};
> +EXPORT_SYMBOL_GPL(clk_fixed_factor_ops);
> +
> +struct clk *clk_register_fixed_factor(struct device *dev, const char *name,
> +		const char *parent_name, unsigned long flags,
> +		unsigned int mult, unsigned int div)
> +{
> +	struct clk_fixed_factor *fix;
> +	struct clk_init_data init;
> +	struct clk *clk;
> +
> +	fix = kmalloc(sizeof(*fix), GFP_KERNEL);
> +	if (!fix) {
> +		pr_err("%s: could not allocate fixed factor clk\n", __func__);
> +		return ERR_PTR(-ENOMEM);
> +	}

Can we add a check for mult <= div? It doesn't look like this clock is 
meant to capture clock multipliers (if there is even anything like that 
other than PLLs).

> +
> +	/* struct clk_fixed_factor assignments */
> +	fix->mult = mult;
> +	fix->div = div;
> +	fix->hw.init =&init;
> +
> +	init.name = name;
> +	init.ops =&clk_fixed_factor_ops;
> +	init.flags = flags;
> +	init.parent_names =&parent_name;
> +	init.num_parents = 1;
> +
> +	clk = clk_register(dev,&fix->hw);
> +
> +	if (IS_ERR(clk))
> +		kfree(fix);
> +
> +	return clk;
> +}
> diff --git a/include/linux/clk-private.h b/include/linux/clk-private.h
> index b258532..eb3f84b 100644
> --- a/include/linux/clk-private.h
> +++ b/include/linux/clk-private.h
> @@ -143,6 +143,26 @@ struct clk {
>   	DEFINE_CLK(_name, clk_mux_ops, _flags, _parent_names,	\
>   			_parents);
>
> +#define DEFINE_CLK_FIXED_FACTOR(_name, _parent_name,		\
> +				_parent_ptr, _flags,		\
> +				_mult, _div)			\
> +	static struct clk _name;				\
> +	static const char *_name##_parent_names[] = {		\
> +		_parent_name,					\
> +	};							\
> +	static struct clk *_name##_parents[] = {		\
> +		_parent_ptr,					\
> +	};							\
> +	static struct clk_fixed_factor _name##_hw = {		\
> +		.hw = {						\
> +			.clk =&_name,				\
> +		},						\
> +		.mult = _mult,					\
> +		.div = _div,					\
> +	};							\
> +	DEFINE_CLK(_name, clk_fixed_factor_ops, _flags,		\
> +			_name##_parent_names, _name##_parents);
> +

I would prefer not defining a macro for this. But if we are going to do 
it, can we please at least stop doing nested macros here? It would be 
much easier for a new comer to read if we don't nest these clock macros.

Also, should we stop adding support for fully static allocation for new 
clock types? Since it's supposed to be going away soon. Since Mike 
didn't add this clock type, I'm guessing he doesn't need the clock type 
now and hence doesn't need static allocation support for it.

>   /**
>    * __clk_init - initialize the data structures in a struct clk
>    * @dev:	device initializing this clk, placeholder for now
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index 5db3412..c1c23b9 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -277,6 +277,29 @@ struct clk *clk_register_mux(struct device *dev, const char *name,
>   		u8 clk_mux_flags, spinlock_t *lock);
>
>   /**
> + * struct clk_fixed_factor - fixed multiplier and divider clock
> + *
> + * @hw:		handle between common and hardware-specific interfaces
> + * @mult:	multiplier
> + * @div:	divider
> + *
> + * Clock with a fixed multiplier and divider. The output frequency is the
> + * parent clock rate divided by div and multiplied by mult.
> + * Implements .recalc_rate, .set_rate and .round_rate
> + */
> +
> +struct clk_fixed_factor {
> +	struct clk_hw	hw;
> +	unsigned int	mult;
> +	unsigned int	div;
> +};
> +
> +extern struct clk_ops clk_fixed_factor_ops;
> +struct clk *clk_register_fixed_factor(struct device *dev, const char *name,
> +		const char *parent_name, unsigned long flags,
> +		unsigned int mult, unsigned int div);

Should we have one header file for each common clock type? We seem to be 
adding a lot of those (which is good), but it almost feels like 
clock-provider get out of hand soon.

Thanks,
Saravana

On Mon, May 7, 2012 at 12:54 PM, Saravana Kannan <skannan@codeaurora.org> wrote:
> On 05/06/2012 10:08 PM, Mike Turquette wrote:
>> From: Sascha Hauer<s.hauer@pengutronix.de>
>> +struct clk *clk_register_fixed_factor(struct device *dev, const char
>> *name,
>> +               const char *parent_name, unsigned long flags,
>> +               unsigned int mult, unsigned int div)
>> +{
>> +       struct clk_fixed_factor *fix;
>> +       struct clk_init_data init;
>> +       struct clk *clk;
>> +
>> +       fix = kmalloc(sizeof(*fix), GFP_KERNEL);
>> +       if (!fix) {
>> +               pr_err("%s: could not allocate fixed factor clk\n",
>> __func__);
>> +               return ERR_PTR(-ENOMEM);
>> +       }
>
>
> Can we add a check for mult <= div? It doesn't look like this clock is meant
> to capture clock multipliers (if there is even anything like that other than
> PLLs).
>

I don't think we should enforce a rule like that.  Some folks with
static PLLs that never change their rates (and maybe are set up by the
bootloader) might find this clock type to be perfect for them as a
multiplier.

<snip>
>> +#define DEFINE_CLK_FIXED_FACTOR(_name, _parent_name,           \
>> +                               _parent_ptr, _flags,            \
>> +                               _mult, _div)                    \
>> +       static struct clk _name;                                \
>> +       static const char *_name##_parent_names[] = {           \
>> +               _parent_name,                                   \
>> +       };                                                      \
>> +       static struct clk *_name##_parents[] = {                \
>> +               _parent_ptr,                                    \
>> +       };                                                      \
>> +       static struct clk_fixed_factor _name##_hw = {           \
>> +               .hw = {                                         \
>> +                       .clk =&_name,                           \
>> +               },                                              \
>> +               .mult = _mult,                                  \
>> +               .div = _div,                                    \
>> +       };                                                      \
>> +       DEFINE_CLK(_name, clk_fixed_factor_ops, _flags,         \
>> +                       _name##_parent_names, _name##_parents);
>> +
>
>
> I would prefer not defining a macro for this. But if we are going to do it,
> can we please at least stop doing nested macros here? It would be much
> easier for a new comer to read if we don't nest these clock macros.

This macro follows the others in every way.  Why should we make it
look less uniform?

> Also, should we stop adding support for fully static allocation for new
> clock types? Since it's supposed to be going away soon. Since Mike didn't
> add this clock type, I'm guessing he doesn't need the clock type now and
> hence doesn't need static allocation support for it.
>

I'm afraid I don't follow.  I do need this clock in fact (see
https://github.com/mturquette/linux/commits/clk-next-may06-omap), and
my platform's data is still static.

>
>>  /**
>>   * __clk_init - initialize the data structures in a struct clk
>>   * @dev:      device initializing this clk, placeholder for now
>> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
>> index 5db3412..c1c23b9 100644
>> --- a/include/linux/clk-provider.h
>> +++ b/include/linux/clk-provider.h
>> @@ -277,6 +277,29 @@ struct clk *clk_register_mux(struct device *dev,
>> const char *name,
>>                u8 clk_mux_flags, spinlock_t *lock);
>>
>>  /**
>> + * struct clk_fixed_factor - fixed multiplier and divider clock
>> + *
>> + * @hw:                handle between common and hardware-specific
>> interfaces
>> + * @mult:      multiplier
>> + * @div:       divider
>> + *
>> + * Clock with a fixed multiplier and divider. The output frequency is the
>> + * parent clock rate divided by div and multiplied by mult.
>> + * Implements .recalc_rate, .set_rate and .round_rate
>> + */
>> +
>> +struct clk_fixed_factor {
>> +       struct clk_hw   hw;
>> +       unsigned int    mult;
>> +       unsigned int    div;
>> +};
>> +
>> +extern struct clk_ops clk_fixed_factor_ops;
>> +struct clk *clk_register_fixed_factor(struct device *dev, const char
>> *name,
>> +               const char *parent_name, unsigned long flags,
>> +               unsigned int mult, unsigned int div);
>
>
> Should we have one header file for each common clock type? We seem to be
> adding a lot of those (which is good), but it almost feels like
> clock-provider get out of hand soon.
>

I think clk-provider.h is fine for now.  It's a good one-stop-shop for
people that are just now figuring out what basic clock types exist and
applying them to their platform.  The file itself is only 336 lines
which is hardly out of control...

Regards,
Mike

> Thanks,
> Saravana
>
> --
> Sent by an employee of the Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On 05/07/2012 01:14 PM, Turquette, Mike wrote:
> On Mon, May 7, 2012 at 12:54 PM, Saravana Kannan<skannan@codeaurora.org>  wrote:
>> On 05/06/2012 10:08 PM, Mike Turquette wrote:
>>> From: Sascha Hauer<s.hauer@pengutronix.de>
>>> +struct clk *clk_register_fixed_factor(struct device *dev, const char
>>> *name,
>>> +               const char *parent_name, unsigned long flags,
>>> +               unsigned int mult, unsigned int div)
>>> +{
>>> +       struct clk_fixed_factor *fix;
>>> +       struct clk_init_data init;
>>> +       struct clk *clk;
>>> +
>>> +       fix = kmalloc(sizeof(*fix), GFP_KERNEL);
>>> +       if (!fix) {
>>> +               pr_err("%s: could not allocate fixed factor clk\n",
>>> __func__);
>>> +               return ERR_PTR(-ENOMEM);
>>> +       }
>>
>>
>> Can we add a check for mult<= div? It doesn't look like this clock is meant
>> to capture clock multipliers (if there is even anything like that other than
>> PLLs).
>>
>
> I don't think we should enforce a rule like that.  Some folks with
> static PLLs that never change their rates (and maybe are set up by the
> bootloader) might find this clock type to be perfect for them as a
> multiplier.

I would think those clocks would have some type of register control. At 
least for enable/disable. This clock just seems to implement a simple 
integer divider/fractional multiplier. I think we should add this check.

> <snip>
>>> +#define DEFINE_CLK_FIXED_FACTOR(_name, _parent_name,           \
>>> +                               _parent_ptr, _flags,            \
>>> +                               _mult, _div)                    \
>>> +       static struct clk _name;                                \
>>> +       static const char *_name##_parent_names[] = {           \
>>> +               _parent_name,                                   \
>>> +       };                                                      \
>>> +       static struct clk *_name##_parents[] = {                \
>>> +               _parent_ptr,                                    \
>>> +       };                                                      \
>>> +       static struct clk_fixed_factor _name##_hw = {           \
>>> +               .hw = {                                         \
>>> +                       .clk =&_name,                           \
>>> +               },                                              \
>>> +               .mult = _mult,                                  \
>>> +               .div = _div,                                    \
>>> +       };                                                      \
>>> +       DEFINE_CLK(_name, clk_fixed_factor_ops, _flags,         \
>>> +                       _name##_parent_names, _name##_parents);
>>> +
>>
>>
>> I would prefer not defining a macro for this. But if we are going to do it,
>> can we please at least stop doing nested macros here? It would be much
>> easier for a new comer to read if we don't nest these clock macros.
>
> This macro follows the others in every way.  Why should we make it
> look less uniform?

May be the other macros should be refactored to not be nested too?

>> Also, should we stop adding support for fully static allocation for new
>> clock types? Since it's supposed to be going away soon. Since Mike didn't
>> add this clock type, I'm guessing he doesn't need the clock type now and
>> hence doesn't need static allocation support for it.
>>
>
> I'm afraid I don't follow.  I do need this clock in fact (see
> https://github.com/mturquette/linux/commits/clk-next-may06-omap), and
> my platform's data is still static.

Never mind. If you are using this clock type, then it's okay to support 
static init.

>> Should we have one header file for each common clock type? We seem to be
>> adding a lot of those (which is good), but it almost feels like
>> clock-provider get out of hand soon.
>>
>
> I think clk-provider.h is fine for now.  It's a good one-stop-shop for
> people that are just now figuring out what basic clock types exist and
> applying them to their platform.  The file itself is only 336 lines
> which is hardly out of control...

I still prefer them to be split out since one doesn't need to include 
(and be recompiled when it changes) stuff they don't care about. But 
it's not yet a significant point to argue about. So, let's wait and see 
how it goes.

-Saravana