[v3,04/11] clk: davinci - add pll divider clock driver

Commit Message

Murali Karicheri Oct. 25, 2012, 4:11 p.m. UTC

pll dividers are present in the pll controller of DaVinci and Other
SoCs that re-uses the same hardware IP. This has a enable bit for
bypass the divider or enable the driver. This is a sub class of the
clk-divider clock checks the enable bit to calculare the rate and
invoke the recalculate() function of the clk-divider if enabled.

Signed-off-by: Murali Karicheri <[e-mail]>
---
 drivers/clk/davinci/clk-div.c |  124 +++++++++++++++++++++++++++++++++++++++++
 drivers/clk/davinci/clk-div.h |   42 ++++++++++++++
 2 files changed, 166 insertions(+)
 create mode 100644 drivers/clk/davinci/clk-div.c
 create mode 100644 drivers/clk/davinci/clk-div.h

Comments

Linus Walleij Oct. 28, 2012, 7:26 p.m. UTC | #1

On Thu, Oct 25, 2012 at 6:11 PM, Murali Karicheri <[e-mail]> wrote:

> pll dividers are present in the pll controller of DaVinci and Other
> SoCs that re-uses the same hardware IP. This has a enable bit for
> bypass the divider or enable the driver. This is a sub class of the
> clk-divider clock checks the enable bit to calculare the rate and
> invoke the recalculate() function of the clk-divider if enabled.
>
> Signed-off-by: Murali Karicheri <[e-mail]>

Looking good,
Acked-by: Linus Walleij <[e-mail]>

Yours,
Linus Walleij

Murali Karicheri Oct. 31, 2012, 1:22 p.m. UTC | #2

On 10/28/2012 03:26 PM, Linus Walleij wrote:
> On Thu, Oct 25, 2012 at 6:11 PM, Murali Karicheri <[e-mail]> wrote:
>
>> pll dividers are present in the pll controller of DaVinci and Other
>> SoCs that re-uses the same hardware IP. This has a enable bit for
>> bypass the divider or enable the driver. This is a sub class of the
>> clk-divider clock checks the enable bit to calculare the rate and
>> invoke the recalculate() function of the clk-divider if enabled.
>>
>> Signed-off-by: Murali Karicheri <[e-mail]>
> Looking good,
> Acked-by: Linus Walleij <[e-mail]>
>
> Yours,
> Linus Walleij
>
Linus,

Thanks. I will add your Acked-by in the next version.

Murali

Sekhar Nori Nov. 2, 2012, 11:33 a.m. UTC | #3

On 10/25/2012 9:41 PM, Murali Karicheri wrote:

> pll dividers are present in the pll controller of DaVinci and Other
> SoCs that re-uses the same hardware IP. This has a enable bit for
> bypass the divider or enable the driver. This is a sub class of the
> clk-divider clock checks the enable bit to calculare the rate and
> invoke the recalculate() function of the clk-divider if enabled.
> 
> Signed-off-by: Murali Karicheri <[e-mail]>
> ---
>  drivers/clk/davinci/clk-div.c |  124 +++++++++++++++++++++++++++++++++++++++++
>  drivers/clk/davinci/clk-div.h |   42 ++++++++++++++
>  2 files changed, 166 insertions(+)
>  create mode 100644 drivers/clk/davinci/clk-div.c
>  create mode 100644 drivers/clk/davinci/clk-div.h
> 
> diff --git a/drivers/clk/davinci/clk-div.c b/drivers/clk/davinci/clk-div.c
> new file mode 100644
> index 0000000..8147d99
> --- /dev/null
> +++ b/drivers/clk/davinci/clk-div.c
> @@ -0,0 +1,124 @@
> +/*
> + * Copyright 2012 Freescale Semiconductor, Inc.
> + * Copyright 2012 Texas instuments
> + *
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:

incomplete sentence.

> +/**
> + * clk_register_davinci_plldiv - register function for DaVinci PLL divider clk
> + *
> + * @dev: device ptr
> + * @name: name of the clock
> + * @parent_name: name of parent clock
> + * @plldiv_data: ptr to pll divider data
> + * @lock: ptr to spinlock passed to divider clock
> + */
> +struct clk *clk_register_davinci_plldiv(struct device *dev,

Why do you need a dev pointer here and which device does it point to? In
the only usage of this API in the series, you pass a NULL here. I should
have probably asked this question on one of the earlier patches itself.

> +			const char *name, const char *parent_name,
> +			struct clk_plldiv_data *plldiv_data,
> +			spinlock_t *lock)
> +{
> +	struct clk_div *div;
> +	struct clk *clk;
> +	struct clk_init_data init;
> +
> +	div = kzalloc(sizeof(*div), GFP_KERNEL);
> +	if (!div)
> +		return ERR_PTR(-ENOMEM);
> +
> +	init.name = name;
> +	init.ops = &clk_div_ops;
> +	init.flags = plldiv_data->flags;
> +	init.parent_names = (parent_name ? &parent_name : NULL);
> +	init.num_parents = (parent_name ? 1 : 0);
> +
> +	div->reg = plldiv_data->reg;
> +	div->en_id = plldiv_data->en_id;
> +
> +	div->divider.reg = plldiv_data->reg;
> +	div->divider.shift = plldiv_data->shift;
> +	div->divider.width = plldiv_data->width;
> +	div->divider.flags = plldiv_data->divider_flags;
> +	div->divider.lock = lock;
> +	div->divider.hw.init = &init;
> +	div->ops = &clk_divider_ops;
> +
> +	clk = clk_register(NULL, &div->divider.hw);

Shouldn't you be calling clk_register_divider() here which in turn will
do clk_register()?

Thanks,
Sekhar

Murali Karicheri Nov. 2, 2012, 1:53 p.m. UTC | #4

On 11/02/2012 07:33 AM, Sekhar Nori wrote:
> On 10/25/2012 9:41 PM, Murali Karicheri wrote:
>
>> pll dividers are present in the pll controller of DaVinci and Other
>> SoCs that re-uses the same hardware IP. This has a enable bit for
>> bypass the divider or enable the driver. This is a sub class of the
>> clk-divider clock checks the enable bit to calculare the rate and
>> invoke the recalculate() function of the clk-divider if enabled.
>>
>> Signed-off-by: Murali Karicheri <[e-mail]>
>> ---
>>   drivers/clk/davinci/clk-div.c |  124 +++++++++++++++++++++++++++++++++++++++++
>>   drivers/clk/davinci/clk-div.h |   42 ++++++++++++++
>>   2 files changed, 166 insertions(+)
>>   create mode 100644 drivers/clk/davinci/clk-div.c
>>   create mode 100644 drivers/clk/davinci/clk-div.h
>>
>> diff --git a/drivers/clk/davinci/clk-div.c b/drivers/clk/davinci/clk-div.c
>> new file mode 100644
>> index 0000000..8147d99
>> --- /dev/null
>> +++ b/drivers/clk/davinci/clk-div.c
>> @@ -0,0 +1,124 @@
>> +/*
>> + * Copyright 2012 Freescale Semiconductor, Inc.
>> + * Copyright 2012 Texas instuments
>> + *
>> + * The code contained herein is licensed under the GNU General Public
>> + * License. You may obtain a copy of the GNU General Public License
>> + * Version 2 or later at the following locations:
> incomplete sentence.
Will fix
>
>> +/**
>> + * clk_register_davinci_plldiv - register function for DaVinci PLL divider clk
>> + *
>> + * @dev: device ptr
>> + * @name: name of the clock
>> + * @parent_name: name of parent clock
>> + * @plldiv_data: ptr to pll divider data
>> + * @lock: ptr to spinlock passed to divider clock
>> + */
>> +struct clk *clk_register_davinci_plldiv(struct device *dev,
> Why do you need a dev pointer here and which device does it point to? In
> the only usage of this API in the series, you pass a NULL here. I should
> have probably asked this question on one of the earlier patches itself.
>
I did a grep in the drivers/clk directory. All of the platform drivers 
are having the device ptr and all of them are called with NULL. I am not 
sure what is the intent of this arg in the API.  As per documentation of 
the clk_register() API, the device ptr points to the device that is 
registering this clk. So if a specific device driver ever has to 
register a PLL div clk, this will be non NULL. In  the normal use case, 
clk is registered in a platform specific code and is always passed NULL.

The platform/SoC specific clock initialization code will be using 
davinci_plldiv_clk() that doesn't have a device ptr arg.
So this can be changed in future in sync with other drivers (assuming 
this will get removed if unused), and changes
doesn't impact the platform code that initialize the clock. So IMO, we 
should keep this arg so that it is in sync with other driver APIs.

+			const char *name, const char *parent_name,
+			struct clk_plldiv_data *plldiv_data,
+			spinlock_t *lock)
+{
+	struct clk_div *div;
+	struct clk *clk;
+	struct clk_init_data init;
+
+	div = kzalloc(sizeof(*div), GFP_KERNEL);
+	if (!div)
+		return ERR_PTR(-ENOMEM);
+
+	init.name = name;
+	init.ops = &clk_div_ops;
+	init.flags = plldiv_data->flags;
+	init.parent_names = (parent_name ? &parent_name : NULL);
+	init.num_parents = (parent_name ? 1 : 0);
+
+	div->reg = plldiv_data->reg;
+	div->en_id = plldiv_data->en_id;
+
+	div->divider.reg = plldiv_data->reg;
+	div->divider.shift = plldiv_data->shift;
+	div->divider.width = plldiv_data->width;
+	div->divider.flags = plldiv_data->divider_flags;
+	div->divider.lock = lock;
+	div->divider.hw.init = &init;
+	div->ops = &clk_divider_ops;
+
+	clk = clk_register(NULL, &div->divider.hw);

> Shouldn't you be calling clk_register_divider() here which in turn will
> do clk_register()?
As stated in the top of the file, this is a subclass driver of clk-div 
similar in line with mxs/clk-div.c. The
driver registers the clock instead of calling clk_register_divider() so 
that it's ops function has a chance to do whatever it wants to do and 
call the divider ops function after that.

Murali
> Thanks,
> Sekhar
>
>

Sekhar Nori Nov. 3, 2012, 12:03 p.m. UTC | #5

On 11/2/2012 7:23 PM, Murali Karicheri wrote:
> On 11/02/2012 07:33 AM, Sekhar Nori wrote:
>> On 10/25/2012 9:41 PM, Murali Karicheri wrote:
>>
>>> pll dividers are present in the pll controller of DaVinci and Other
>>> SoCs that re-uses the same hardware IP. This has a enable bit for
>>> bypass the divider or enable the driver. This is a sub class of the
>>> clk-divider clock checks the enable bit to calculare the rate and
>>> invoke the recalculate() function of the clk-divider if enabled.
>>>
>>> Signed-off-by: Murali Karicheri <[e-mail]>
>>> ---

>>> +/**
>>> + * clk_register_davinci_plldiv - register function for DaVinci PLL
>>> divider clk
>>> + *
>>> + * @dev: device ptr
>>> + * @name: name of the clock
>>> + * @parent_name: name of parent clock
>>> + * @plldiv_data: ptr to pll divider data
>>> + * @lock: ptr to spinlock passed to divider clock
>>> + */
>>> +struct clk *clk_register_davinci_plldiv(struct device *dev,
>> Why do you need a dev pointer here and which device does it point to? In
>> the only usage of this API in the series, you pass a NULL here. I should
>> have probably asked this question on one of the earlier patches itself.
>>
> I did a grep in the drivers/clk directory. All of the platform drivers
> are having the device ptr and all of them are called with NULL. I am not
> sure what is the intent of this arg in the API.  As per documentation of

I just took a look at the mxs example you referenced below and it does
not take a dev pointer.

struct clk *mxs_clk_div(const char *name, const char *parent_name,
                        void __iomem *reg, u8 shift, u8 width, u8 busy)
{

> the clk_register() API, the device ptr points to the device that is
> registering this clk. So if a specific device driver ever has to
> register a PLL div clk, this will be non NULL. In  the normal use case,
> clk is registered in a platform specific code and is always passed NULL.
> 
> The platform/SoC specific clock initialization code will be using
> davinci_plldiv_clk() that doesn't have a device ptr arg.
> So this can be changed in future in sync with other drivers (assuming
> this will get removed if unused), and changes
> doesn't impact the platform code that initialize the clock. So IMO, we
> should keep this arg so that it is in sync with other driver APIs.

I think you should get rid of this unused arg and introduce it when you
actually need it. That way we are clear about why we need it.

> 
> +            const char *name, const char *parent_name,
> +            struct clk_plldiv_data *plldiv_data,
> +            spinlock_t *lock)
> +{
> +    struct clk_div *div;
> +    struct clk *clk;
> +    struct clk_init_data init;
> +
> +    div = kzalloc(sizeof(*div), GFP_KERNEL);
> +    if (!div)
> +        return ERR_PTR(-ENOMEM);
> +
> +    init.name = name;
> +    init.ops = &clk_div_ops;
> +    init.flags = plldiv_data->flags;
> +    init.parent_names = (parent_name ? &parent_name : NULL);
> +    init.num_parents = (parent_name ? 1 : 0);
> +
> +    div->reg = plldiv_data->reg;
> +    div->en_id = plldiv_data->en_id;
> +
> +    div->divider.reg = plldiv_data->reg;
> +    div->divider.shift = plldiv_data->shift;
> +    div->divider.width = plldiv_data->width;
> +    div->divider.flags = plldiv_data->divider_flags;
> +    div->divider.lock = lock;
> +    div->divider.hw.init = &init;
> +    div->ops = &clk_divider_ops;
> +
> +    clk = clk_register(NULL, &div->divider.hw);
> 
>> Shouldn't you be calling clk_register_divider() here which in turn will
>> do clk_register()?
> As stated in the top of the file, this is a subclass driver of clk-div
> similar in line with mxs/clk-div.c. The
> driver registers the clock instead of calling clk_register_divider() so
> that it's ops function has a chance to do whatever it wants to do and
> call the divider ops function after that.

I see that now. I should have paid more attention.

Regards,
Sekhar

Murali Karicheri Nov. 5, 2012, 3:10 p.m. UTC | #6

On 11/03/2012 08:03 AM, Sekhar Nori wrote:
> On 11/2/2012 7:23 PM, Murali Karicheri wrote:
>> On 11/02/2012 07:33 AM, Sekhar Nori wrote:
>>> On 10/25/2012 9:41 PM, Murali Karicheri wrote:
>>>
>>>> pll dividers are present in the pll controller of DaVinci and Other
>>>> SoCs that re-uses the same hardware IP. This has a enable bit for
>>>> bypass the divider or enable the driver. This is a sub class of the
>>>> clk-divider clock checks the enable bit to calculare the rate and
>>>> invoke the recalculate() function of the clk-divider if enabled.
>>>>
>>>> Signed-off-by: Murali Karicheri <[e-mail]>
>>>> ---
>>>> +/**
>>>> + * clk_register_davinci_plldiv - register function for DaVinci PLL
>>>> divider clk
>>>> + *
>>>> + * @dev: device ptr
>>>> + * @name: name of the clock
>>>> + * @parent_name: name of parent clock
>>>> + * @plldiv_data: ptr to pll divider data
>>>> + * @lock: ptr to spinlock passed to divider clock
>>>> + */
>>>> +struct clk *clk_register_davinci_plldiv(struct device *dev,
>>> Why do you need a dev pointer here and which device does it point to? In
>>> the only usage of this API in the series, you pass a NULL here. I should
>>> have probably asked this question on one of the earlier patches itself.
>>>
>> I did a grep in the drivers/clk directory. All of the platform drivers
>> are having the device ptr and all of them are called with NULL. I am not
>> sure what is the intent of this arg in the API.  As per documentation of
> I just took a look at the mxs example you referenced below and it does
> not take a dev pointer.
>
> struct clk *mxs_clk_div(const char *name, const char *parent_name,
>                          void __iomem *reg, u8 shift, u8 width, u8 busy)
> {
>
>> the clk_register() API, the device ptr points to the device that is
>> registering this clk. So if a specific device driver ever has to
>> register a PLL div clk, this will be non NULL. In  the normal use case,
>> clk is registered in a platform specific code and is always passed NULL.
>>
>> The platform/SoC specific clock initialization code will be using
>> davinci_plldiv_clk() that doesn't have a device ptr arg.
>> So this can be changed in future in sync with other drivers (assuming
>> this will get removed if unused), and changes
>> doesn't impact the platform code that initialize the clock. So IMO, we
>> should keep this arg so that it is in sync with other driver APIs.
> I think you should get rid of this unused arg and introduce it when you
> actually need it. That way we are clear about why we need it.
>
You are right. I will remove it and for other drivers.
>> +            const char *name, const char *parent_name,
>> +            struct clk_plldiv_data *plldiv_data,
>> +            spinlock_t *lock)
>> +{
>> +    struct clk_div *div;
>> +    struct clk *clk;
>> +    struct clk_init_data init;
>> +
>> +    div = kzalloc(sizeof(*div), GFP_KERNEL);
>> +    if (!div)
>> +        return ERR_PTR(-ENOMEM);
>> +
>> +    init.name = name;
>> +    init.ops = &clk_div_ops;
>> +    init.flags = plldiv_data->flags;
>> +    init.parent_names = (parent_name ? &parent_name : NULL);
>> +    init.num_parents = (parent_name ? 1 : 0);
>> +
>> +    div->reg = plldiv_data->reg;
>> +    div->en_id = plldiv_data->en_id;
>> +
>> +    div->divider.reg = plldiv_data->reg;
>> +    div->divider.shift = plldiv_data->shift;
>> +    div->divider.width = plldiv_data->width;
>> +    div->divider.flags = plldiv_data->divider_flags;
>> +    div->divider.lock = lock;
>> +    div->divider.hw.init = &init;
>> +    div->ops = &clk_divider_ops;
>> +
>> +    clk = clk_register(NULL, &div->divider.hw);
>>
>>> Shouldn't you be calling clk_register_divider() here which in turn will
>>> do clk_register()?
>> As stated in the top of the file, this is a subclass driver of clk-div
>> similar in line with mxs/clk-div.c. The
>> driver registers the clock instead of calling clk_register_divider() so
>> that it's ops function has a chance to do whatever it wants to do and
>> call the divider ops function after that.
> I see that now. I should have paid more attention.
>
> Regards,
> Sekhar
>

Patch

diff --git a/drivers/clk/davinci/clk-div.c b/drivers/clk/davinci/clk-div.c
new file mode 100644
index 0000000..8147d99
--- /dev/null
+++ b/drivers/clk/davinci/clk-div.c
@@ -0,0 +1,124 @@ 
+/*
+ * Copyright 2012 Freescale Semiconductor, Inc.
+ * Copyright 2012 Texas instuments
+ *
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:
+ *
+ */
+
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/slab.h>
+
+#include "clk-div.h"
+
+/**
+ * struct clk_div - DaVinci integer pll divider clock
+ *
+ * @divider: the parent class
+ * @ops: pointer to clk_ops of parent class
+ * @reg: register address
+ * @en_id: enable bit id
+ *
+ * The DaVinci pll divider clock is a subclass of basic clk_divider with
+ * an additional enable bit
+ */
+struct clk_div {
+	struct clk_divider divider;
+	const struct clk_ops *ops;
+	void __iomem *reg;
+	u8 en_id;
+};
+
+static inline struct clk_div *to_clk_div(struct clk_hw *hw)
+{
+	struct clk_divider *divider = container_of(hw, struct clk_divider, hw);
+
+	return container_of(divider, struct clk_div, divider);
+}
+
+static unsigned long clk_div_recalc_rate(struct clk_hw *hw,
+					 unsigned long parent_rate)
+{
+	struct clk_div *div = to_clk_div(hw);
+	u32 val;
+
+	val = readl(div->reg);
+	if (val & BIT(div->en_id))
+		return div->ops->recalc_rate(&div->divider.hw, parent_rate);
+
+	/* pll divider bypassed, return parent rate */
+	return parent_rate;
+}
+
+static long clk_div_round_rate(struct clk_hw *hw, unsigned long rate,
+			       unsigned long *prate)
+{
+	struct clk_div *div = to_clk_div(hw);
+
+	return div->ops->round_rate(&div->divider.hw, rate, prate);
+}
+
+static int clk_div_set_rate(struct clk_hw *hw, unsigned long rate,
+			    unsigned long parent_rate)
+{
+	struct clk_div *div = to_clk_div(hw);
+
+	return div->ops->set_rate(&div->divider.hw, rate, parent_rate);
+}
+
+static struct clk_ops clk_div_ops = {
+	.recalc_rate = clk_div_recalc_rate,
+	.round_rate = clk_div_round_rate,
+	.set_rate = clk_div_set_rate,
+};
+
+/**
+ * clk_register_davinci_plldiv - register function for DaVinci PLL divider clk
+ *
+ * @dev: device ptr
+ * @name: name of the clock
+ * @parent_name: name of parent clock
+ * @plldiv_data: ptr to pll divider data
+ * @lock: ptr to spinlock passed to divider clock
+ */
+struct clk *clk_register_davinci_plldiv(struct device *dev,
+			const char *name, const char *parent_name,
+			struct clk_plldiv_data *plldiv_data,
+			spinlock_t *lock)
+{
+	struct clk_div *div;
+	struct clk *clk;
+	struct clk_init_data init;
+
+	div = kzalloc(sizeof(*div), GFP_KERNEL);
+	if (!div)
+		return ERR_PTR(-ENOMEM);
+
+	init.name = name;
+	init.ops = &clk_div_ops;
+	init.flags = plldiv_data->flags;
+	init.parent_names = (parent_name ? &parent_name : NULL);
+	init.num_parents = (parent_name ? 1 : 0);
+
+	div->reg = plldiv_data->reg;
+	div->en_id = plldiv_data->en_id;
+
+	div->divider.reg = plldiv_data->reg;
+	div->divider.shift = plldiv_data->shift;
+	div->divider.width = plldiv_data->width;
+	div->divider.flags = plldiv_data->divider_flags;
+	div->divider.lock = lock;
+	div->divider.hw.init = &init;
+	div->ops = &clk_divider_ops;
+
+	clk = clk_register(NULL, &div->divider.hw);
+	if (IS_ERR(clk))
+		kfree(div);
+
+	return clk;
+}
diff --git a/drivers/clk/davinci/clk-div.h b/drivers/clk/davinci/clk-div.h
new file mode 100644
index 0000000..0e3708c
--- /dev/null
+++ b/drivers/clk/davinci/clk-div.h
@@ -0,0 +1,42 @@ 
+/*
+ * Header file for DaVinci pll divider clk driver
+ *
+ * Copyright (C) 2006-2012 Texas Instruments.
+ *
+ * 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.
+ *
+ */
+
+#ifndef __CLK_DAVINCI_PLLDIV_H
+#define __CLK_DAVINCI_PLLDIV_H
+
+
+/**
+ * struct clk_plldiv_data - configuration for DaVinci PLL divider driver
+ *
+ * @flags - base clk driver flags
+ * @divider_flags - clk-divider flags
+ * @phys_div_reg - clk-divider register physical address
+ * @reg - clk-divider register io mapped address
+ * @shift - shift applied to the divider bits in the register
+ * @width - width of the divider bits in the register
+ * @en_id - bypass or enable bit mask id. mask = BIT(en_id)
+ */
+/* configuration data for PLL divider clock */
+struct clk_plldiv_data {
+	u32			flags;
+	u8			divider_flags;
+	u32                     phys_div_reg;
+	void __iomem		*reg;
+	u8			shift;
+	u8			width;
+	u8			en_id;
+};
+
+struct clk *clk_register_davinci_plldiv(struct device *dev,
+			const char *name, const char *parent_name,
+			struct clk_plldiv_data *plldiv_data,
+			spinlock_t *lock);
+#endif /* CLK_DAVINCI_PLLDIV_H */