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

Tomasz Figa t.figa at samsung.com
Thu Nov 15 05:25:55 EST 2012


On Thursday 15 of November 2012 14:03:12 Thomas Abraham wrote:
> 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.

Basically this const does not give us anything, while it could make 
problems in future.

Let's say that one would want to register a clock in runtime, dynamically 
allocating a samsung_*_clock structure. It wouldn't be possible, because 
he wouldn't be able to set the id field.

Instead of making particular fields in the structures constant, I would 
prefer making the arrays of these structures constant, as they are 
statically initialized anyway.

Best regards,
-- 
Tomasz Figa
Samsung Poland R&D Center
SW Solution Development, Linux Platform




More information about the linux-arm-kernel mailing list