Re: [PATCH v3 01/11] clk: samsung: add common clock framework helper functions for Samsung platforms

[Tomasz Figa <tomasz.figa@gmail.com>]

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@linaro.org>
> Cc: Kukjin Kim <kgene.kim@samsung.com>
> Signed-off-by: Thomas Abraham <thomas.abraham@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@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?

> +
> +/* 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@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?

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