[PATCH v3 01/11] clk: samsung: add common clock framework helper functions for Samsung platforms
Thomas Abraham
thomas.abraham at linaro.org
Thu Nov 15 03:33:12 EST 2012
Hi Tomasz,
Thanks for reviewing these patches!
On 15 November 2012 04:42, Tomasz Figa <tomasz.figa at gmail.com> wrote:
> Hi Thomas,
>
> Looks mostly good, but I have some minor comments inline.
>
> On Thursday 15 of November 2012 03:37:23 Thomas Abraham wrote:
>> All Samsung platforms include different types of clock including
>> fixed-rate, mux, divider and gate clock types. There are typically
>> hundreds of such clocks on each of the Samsung platforms. To enable
>> Samsung platforms to register these clocks using the common clock
>> framework, a bunch of utility functions are introduced here which
>> simplify the clock registration process. The clocks are usually
>> statically instantiated and registered with common clock framework.
>>
>> Cc: Mike Turquette <mturquette at linaro.org>
>> Cc: Kukjin Kim <kgene.kim at samsung.com>
>> Signed-off-by: Thomas Abraham <thomas.abraham at linaro.org>
>> ---
>> drivers/clk/Makefile | 1 +
>> drivers/clk/samsung/Makefile | 5 +
>> drivers/clk/samsung/clk.c | 176 ++++++++++++++++++++++++++++++++++
>> drivers/clk/samsung/clk.h | 218
>> ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 400
>> insertions(+), 0 deletions(-)
>> create mode 100644 drivers/clk/samsung/Makefile
>> create mode 100644 drivers/clk/samsung/clk.c
>> create mode 100644 drivers/clk/samsung/clk.h
>>
>> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
>> index 2701235..808f8e1 100644
>> --- a/drivers/clk/Makefile
>> +++ b/drivers/clk/Makefile
>> @@ -19,6 +19,7 @@ endif
>> obj-$(CONFIG_MACH_LOONGSON1) += clk-ls1x.o
>> obj-$(CONFIG_ARCH_U8500) += ux500/
>> obj-$(CONFIG_ARCH_VT8500) += clk-vt8500.o
>> +obj-$(CONFIG_PLAT_SAMSUNG) += samsung/
>>
>> # Chip specific
>> obj-$(CONFIG_COMMON_CLK_WM831X) += clk-wm831x.o
>> diff --git a/drivers/clk/samsung/Makefile b/drivers/clk/samsung/Makefile
>> new file mode 100644
>> index 0000000..3f926b0
>> --- /dev/null
>> +++ b/drivers/clk/samsung/Makefile
>> @@ -0,0 +1,5 @@
>> +#
>> +# Samsung Clock specific Makefile
>> +#
>> +
>> +obj-$(CONFIG_PLAT_SAMSUNG) += clk.o
>> diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c
>> new file mode 100644
>> index 0000000..ebc6fb6
>> --- /dev/null
>> +++ b/drivers/clk/samsung/clk.c
>> @@ -0,0 +1,176 @@
>> +/*
>> + * Copyright (c) 2012 Samsung Electronics Co., Ltd.
>> + * Copyright (c) 2012 Linaro Ltd.
>> + * Author: Thomas Abraham <thomas.ab at samsung.com>
>> + *
>> + * 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.
>> + *
>> + * This file includes utility functions to register clocks to common
>> + * clock framework for Samsung platforms.
>> +*/
>> +
>> +#include "clk.h"
>> +
>> +static DEFINE_SPINLOCK(lock);
>> +static struct clk **clk_table;
>> +static struct clk_onecell_data clk_data;
>> +void __iomem *reg_base;
>
> Shouldn't it be static?
Yes, I missed that. Will fix.
>
>> +
>> +/* setup the essentials required to support clock lookup using ccf */
>> +void __init samsung_clk_init(struct device_node *np, void __iomem
>> *base, + unsigned long nr_clks)
>> +{
>> + reg_base = base;
>> + if (!np)
>> + return;
>> +
>> + clk_table = kzalloc(sizeof(struct clk *) * nr_clks, GFP_KERNEL);
>> + if (!clk_table)
>> + panic("could not allocate clock lookup table\n");
>> +
>> + clk_data.clks = clk_table;
>> + clk_data.clk_num = nr_clks;
>> + of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data);
>> +}
>> +
>> +/* add a clock instance to the clock lookup table used for dt based
>> lookup */ +void samsung_clk_add_lookup(struct clk *clk, unsigned int
>> id) +{
>> + if (clk_table && id)
>
> I'm not sure if we really need this kind of checks, but if we do, then
> shouldn't we also check id against clk_data.clk_num to prevent out of
> bound index?
The entry into the lookup table is required only for device tree based
platforms. And clk_table is a dynamically allocated table if booting
with device tree support. Since the call to samsung_clk_add_lookup is
made for non-dt platforms as well, the check for clk_table ensures
that the entry to lookup table is done only for device tree enabled
platforms. The check for 'id' ensures that the lookup entry index 0 is
not used. There is no clock which has id as 0.
>
>> + clk_table[id] = clk;
>> +}
>> +
>> +/* register a list of fixed clocks */
>> +void __init samsung_clk_register_fixed_rate(
>> + struct samsung_fixed_rate_clock *list, unsigned int nr_clk)
>> +{
>> + struct clk *clk;
>> + unsigned int idx, ret;
>> +
>> + for (idx = 0; idx < nr_clk; idx++, list++) {
>> + clk = clk_register_fixed_rate(NULL, list->name,
>> + list->parent_name, list->flags, list->fixed_rate);
>> + if (IS_ERR(clk)) {
>> + pr_err("%s: failed to register clock %s\n", __func__,
>> + list->name);
>> + continue;
>> + }
>> +
>> + samsung_clk_add_lookup(clk, list->id);
>> +
>> + /*
>> + * Unconditionally add a clock lookup for the fixed rate
> clocks.
>> + * There are not many of these on any of Samsung platforms.
>> + */
>> + ret = clk_register_clkdev(clk, list->name, NULL);
>> + if (ret)
>> + pr_err("%s: failed to register clock lookup for %s",
>> + __func__, list->name);
>> + }
>> +}
>> +
>> +/* register a list of mux clocks */
>> +void __init samsung_clk_register_mux(struct samsung_mux_clock *list,
>> + unsigned int nr_clk)
>> +{
>> + struct clk *clk;
>> + unsigned int idx, ret;
>> +
>> + for (idx = 0; idx < nr_clk; idx++, list++) {
>> + clk = clk_register_mux(NULL, list->name, list->parent_names,
>> + list->num_parents, list->flags, reg_base + list->offset,
>> + list->shift, list->width, list->mux_flags, &lock);
>> + if (IS_ERR(clk)) {
>> + pr_err("%s: failed to register clock %s\n", __func__,
>> + list->name);
>> + continue;
>> + }
>> +
>> + samsung_clk_add_lookup(clk, list->id);
>> +
>> + /* register a clock lookup only if a clock alias is specified
> */
>> + if (list->alias) {
>> + ret = clk_register_clkdev(clk, list->alias,
>> + list->dev_name);
>> + if (ret)
>> + pr_err("%s: failed to register lookup %s\n",
>> + __func__, list->alias);
>> + }
>> + }
>> +}
>> +
>> +/* register a list of div clocks */
>> +void __init samsung_clk_register_div(struct samsung_div_clock *list,
>> + unsigned int nr_clk)
>> +{
>> + struct clk *clk;
>> + unsigned int idx, ret;
>> +
>> + for (idx = 0; idx < nr_clk; idx++, list++) {
>> + clk = clk_register_divider(NULL, list->name, list-
>>parent_name,
>> + list->flags, reg_base + list->offset, list->shift,
>> + list->width, list->div_flags, &lock);
>> + if (IS_ERR(clk)) {
>> + pr_err("clock: failed to register clock %s\n",
>> + list->name);
>> + continue;
>> + }
>> +
>> + samsung_clk_add_lookup(clk, list->id);
>> +
>> + /* register a clock lookup only if a clock alias is specified
> */
>> + if (list->alias) {
>> + ret = clk_register_clkdev(clk, list->alias,
>> + list->dev_name);
>> + if (ret)
>> + pr_err("%s: failed to register lookup %s\n",
>> + __func__, list->alias);
>> + }
>> + }
>> +}
>> +
>> +/* register a list of gate clocks */
>> +void __init samsung_clk_register_gate(struct samsung_gate_clock *list,
>> + unsigned int nr_clk)
>> +{
>> + struct clk *clk;
>> + unsigned int idx, ret;
>> +
>> + for (idx = 0; idx < nr_clk; idx++, list++) {
>> + clk = clk_register_gate(NULL, list->name, list->parent_name,
>> + list->flags, reg_base + list->offset,
>> + list->bit_idx, list->gate_flags, &lock);
>> + if (IS_ERR(clk)) {
>> + pr_err("clock: failed to register clock %s\n",
>> + list->name);
>> + continue;
>> + }
>> +
>> + /* register a clock lookup only if a clock alias is specified
> */
>> + if (list->alias) {
>> + ret = clk_register_clkdev(clk, list->alias,
>> + list->dev_name);
>> + if (ret)
>> + pr_err("%s: failed to register lookup %s\n",
>> + __func__, list->alias);
>> + }
>> +
>> + samsung_clk_add_lookup(clk, list->id);
>> + }
>> +}
>> +
>> +/* utility function to get the rate of a specified clock */
>> +unsigned long _get_rate(const char *clk_name)
>> +{
>> + struct clk *clk;
>> + unsigned long rate;
>> +
>> + clk = clk_get(NULL, clk_name);
>> + if (IS_ERR(clk))
>> + return 0;
>> + rate = clk_get_rate(clk);
>> + clk_put(clk);
>> + return rate;
>> +}
>> diff --git a/drivers/clk/samsung/clk.h b/drivers/clk/samsung/clk.h
>> new file mode 100644
>> index 0000000..ab43498
>> --- /dev/null
>> +++ b/drivers/clk/samsung/clk.h
>> @@ -0,0 +1,218 @@
>> +/*
>> + * Copyright (c) 2012 Samsung Electronics Co., Ltd.
>> + * Copyright (c) 2012 Linaro Ltd.
>> + * Author: Thomas Abraham <thomas.ab at samsung.com>
>> + *
>> + * 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.
>> + *
>> + * Common Clock Framework support for all Samsung platforms
>> +*/
>> +
>> +#ifndef __SAMSUNG_CLK_H
>> +#define __SAMSUNG_CLK_H
>> +
>> +#include <linux/clk.h>
>> +#include <linux/clkdev.h>
>> +#include <linux/io.h>
>> +#include <linux/clk-provider.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +
>> +#include <mach/map.h>
>> +
>> +/**
>> + * struct samsung_fixed_rate_clock: information about fixed-rate clock
>> + * @id: platform specific id of the clock.
>> + * @name: name of this fixed-rate clock.
>> + * @parent_name: optional parent clock name.
>> + * @flags: optional fixed-rate clock flags.
>> + * @fixed-rate: fixed clock rate of this clock.
>> + */
>> +struct samsung_fixed_rate_clock {
>> + unsigned int id;
>> + char *name;
>
> Shouldn't it be const char *name?
Yes, I will fix this.
>
>> + const char *parent_name;
>> + unsigned long flags;
>> + unsigned long fixed_rate;
>> +};
>> +
>> +#define FRATE(_id, cname, pname, f, frate) \
>> + { \
>> + .id = _id, \
>> + .name = cname, \
>> + .parent_name = pname, \
>> + .flags = f, \
>> + .fixed_rate = frate, \
>> + }
>> +
>> +/**
>> + * struct samsung_mux_clock: information about mux clock
>> + * @id: platform specific id of the clock.
>> + * @dev_name: name of the device to which this clock belongs.
>> + * @name: name of this mux clock.
>> + * @parent_names: array of pointer to parent clock names.
>> + * @num_parents: number of parents listed in @parent_names.
>> + * @flags: optional flags for basic clock.
>> + * @offset: offset of the register for configuring the mux.
>> + * @shift: starting bit location of the mux control bit-field in @reg.
>> + * @width: width of the mux control bit-field in @reg.
>> + * @mux_flags: flags for mux-type clock.
>> + * @alias: optional clock alias name to be assigned to this clock.
>> + */
>> +struct samsung_mux_clock {
>> + const unsigned int id;
>
> Is const unsigned int really correct?
Sorry, I did not get the problem here.
>
>> + const char *dev_name;
>> + const char *name;
>> + const char **parent_names;
>> + u8 num_parents;
>> + unsigned long flags;
>> + unsigned long offset;
>> + u8 shift;
>> + u8 width;
>> + u8 mux_flags;
>> + const char *alias;
>> +};
>> +
>> +#define __MUX(_id, dname, cname, pnames, o, s, w, f, mf, a) \
>> + { \
>> + .id = _id, \
>> + .dev_name = dname, \
>> + .name = cname, \
>> + .parent_names = pnames, \
>> + .num_parents = ARRAY_SIZE(pnames), \
>> + .flags = f, \
>> + .offset = o, \
>> + .shift = s, \
>> + .width = w, \
>> + .mux_flags = mf, \
>> + .alias = a, \
>> + }
>> +
>> +#define MUX(_id, cname, pnames, o, s, w) \
>> + __MUX(_id, NULL, cname, pnames, o, s, w, 0, 0, NULL)
>> +
>> +#define MUX_A(_id, cname, pnames, o, s, w, a) \
>> + __MUX(_id, NULL, cname, pnames, o, s, w, 0, 0, a)
>> +
>> +#define MUX_F(_id, cname, pnames, o, s, w, f, mf) \
>> + __MUX(_id, NULL, cname, pnames, o, s, w, f, mf, NULL)
>> +
>> +/**
>> + * @id: platform specific id of the clock.
>> + * struct samsung_div_clock: information about div clock
>> + * @dev_name: name of the device to which this clock belongs.
>> + * @name: name of this div clock.
>> + * @parent_name: name of the parent clock.
>> + * @flags: optional flags for basic clock.
>> + * @offset: offset of the register for configuring the div.
>> + * @shift: starting bit location of the div control bit-field in @reg.
>> + * @div_flags: flags for div-type clock.
>> + * @alias: optional clock alias name to be assigned to this clock.
>> + */
>> +struct samsung_div_clock {
>> + const unsigned int id;
>
> ditto
>
>> + const char *dev_name;
>> + const char *name;
>> + const char *parent_name;
>> + unsigned long flags;
>> + unsigned long offset;
>> + u8 shift;
>> + u8 width;
>> + u8 div_flags;
>> + const char *alias;
>> +};
>> +
>> +#define __DIV(_id, dname, cname, pname, o, s, w, f, df, a) \
>> + { \
>> + .id = _id, \
>> + .dev_name = dname, \
>> + .name = cname, \
>> + .parent_name = pname, \
>> + .flags = f, \
>> + .offset = o, \
>> + .shift = s, \
>> + .width = w, \
>> + .div_flags = df, \
>> + .alias = a, \
>> + }
>> +
>> +#define DIV(_id, cname, pname, o, s, w) \
>> + __DIV(_id, NULL, cname, pname, o, s, w, 0, 0, NULL)
>> +
>> +#define DIV_A(_id, cname, pname, o, s, w, a) \
>> + __DIV(_id, NULL, cname, pname, o, s, w, 0, 0, a)
>> +
>> +#define DIV_F(_id, cname, pname, o, s, w, f, df) \
>> + __DIV(_id, NULL, cname, pname, o, s, w, f, df, NULL)
>> +
>> +/**
>> + * struct samsung_gate_clock: information about gate clock
>> + * @id: platform specific id of the clock.
>> + * @dev_name: name of the device to which this clock belongs.
>> + * @name: name of this gate clock.
>> + * @parent_name: name of the parent clock.
>> + * @flags: optional flags for basic clock.
>> + * @offset: offset of the register for configuring the gate.
>> + * @bit_idx: bit index of the gate control bit-field in @reg.
>> + * @gate_flags: flags for gate-type clock.
>> + * @alias: optional clock alias name to be assigned to this clock.
>> + */
>> +struct samsung_gate_clock {
>> + const unsigned int id;
>
> ditto
>
>> + const char *dev_name;
>> + const char *name;
>> + const char *parent_name;
>> + unsigned long flags;
>> + unsigned long offset;
>> + u8 bit_idx;
>> + u8 gate_flags;
>> + const char *alias;
>> +};
>> +
>> +#define __GATE(_id, dname, cname, pname, o, b, f, gf, a) \
>> + { \
>> + .id = _id, \
>> + .dev_name = dname, \
>> + .name = cname, \
>> + .parent_name = pname, \
>> + .flags = f, \
>> + .offset = o, \
>> + .bit_idx = b, \
>> + .gate_flags = gf, \
>> + .alias = a, \
>> + }
>> +
>> +#define GATE(_id, cname, pname, o, b, f, gf) \
>> + __GATE(_id, NULL, cname, pname, o, b, f, gf, NULL)
>> +
>> +#define GATE_A(_id, cname, pname, o, b, f, gf, a) \
>> + __GATE(_id, NULL, cname, pname, o, b, f, gf, a)
>> +
>> +#define GATE_D(_id, dname, cname, pname, o, b, f, gf) \
>> + __GATE(_id, dname, cname, pname, o, b, f, gf, NULL)
>> +
>> +#define GATE_DA(_id, dname, cname, pname, o, b, f, gf, a) \
>> + __GATE(_id, dname, cname, pname, o, b, f, gf, a)
>> +
>> +#define PNAME(x) static const char *x[] __initdata
>> +
>> +extern void __iomem *reg_base;
>
> Where is it used? The name suggests that it should rather be static.
Right, this should not have been there. I will remove this.
Thanks,
Thomas.
More information about the linux-arm-kernel
mailing list