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@xxxxxxxxxx>
> Cc: Kukjin Kim <kgene.kim@xxxxxxxxxxx>
> Signed-off-by: Thomas Abraham <thomas.abraham@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxxx>
> + *
> + * 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?
> +
> +/* 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?
> + 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@xxxxxxxxxxx>
> + *
> + * 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?
> + 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?
> + 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.
--
Best regards,
Tomasz Figa
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
