[2/5] clk: hi3xxx: add clock support

Add clock support with device tree on Hisilicon SoC.

Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
Cc: Mike Turquette <mturquette@linaro.org>
---
 .../devicetree/bindings/clock/hisilicon.txt        |   73 +++
 drivers/clk/Makefile                               |    1 +
 drivers/clk/clk-hi3xxx.c                           |  641 ++++++++++++++++++++
 3 files changed, 715 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/hisilicon.txt
 create mode 100644 drivers/clk/clk-hi3xxx.c

On 14 May 2013 03:47, Mike Turquette <mturquette@linaro.org> wrote:
> Quoting Haojian Zhuang (2013-04-19 03:49:24)
>> diff --git a/drivers/clk/clk-hi3xxx.c b/drivers/clk/clk-hi3xxx.c
>> new file mode 100644
>> index 0000000..e93e583
>> --- /dev/null
>> +++ b/drivers/clk/clk-hi3xxx.c
>> @@ -0,0 +1,641 @@
>> +/*
>> + * Hisilicon clock driver
>> + *
>> + * Copyright (c) 2012-2013 Hisilicon Limited.
>> + * Copyright (c) 2012-2013 Linaro Limited.
>> + *
>> + * Author: Haojian Zhuang <haojian.zhuang@linaro.org>
>> + *        Xin Li <li.xin@linaro.org>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License along
>> + * with this program; if not, write to the Free Software Foundation, Inc.,
>> + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
>> + *
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/clk-provider.h>
>> +#include <linux/clk-private.h>
>
> clk-private.h is deprecated and can no longer be included by any new
> clock drivers.  It will hopefully be removed in 3.11.  Please remove it
> in your next version.
>
OK. I'll remove it.

>> +#include <linux/clkdev.h>
>> +#include <linux/delay.h>
>> +#include <linux/io.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_device.h>
>> +#include <linux/slab.h>
>> +#include <linux/clk.h>
>> +
>> +#define HI3620_DISABLE_OFF             0x4
>> +#define HI3620_STATUS_OFF              0x8
>> +
>> +#define WIDTH_TO_MASK(width)   ((1 << (width)) - 1)
>> +
>> +/*
>> + * The reverse of DIV_ROUND_UP: The maximum number which
>> + * divided by m is r
>> + */
>> +#define MULT_ROUND_UP(r, m) ((r) * (m) + (m) - 1)
>> +
>> +struct hi3620_periclk {
>> +       struct clk_hw   hw;
>> +       void __iomem    *enable;        /* enable register */
>> +       void __iomem    *reset;         /* reset register */
>> +       u32             ebits;          /* bits in enable/disable register */
>> +       u32             rbits;          /* bits in reset/unreset register */
>> +       spinlock_t      *lock;
>> +};
>> +
>> +struct hi3620_muxclk {
>> +       struct clk_hw   hw;
>> +       void __iomem    *reg;           /* mux register */
>> +       u8              shift;
>> +       u8              width;
>> +       u32             mbits;          /* mask bits in mux register */
>
> You need shift, width AND a mask?  Normally either shift+width OR a mask
> would suffice.

No, it's a little different. In their mux registers, there're two
parts. One is lower
16 bits that are used to set mux, and the other is higher 16 bits that are
used to set mux mask.

At here, I use mbits for higher 16 bits and shift & width for lower 16 bits.

>
>> +       spinlock_t      *lock;
>> +};
>> +
>> +struct hi3620_divclk {
>> +       struct clk_hw   hw;
>> +       void __iomem    *reg;           /* divider register */
>> +       u8              shift;
>> +       u8              width;
>> +       u32             mbits;          /* mask bits in divider register */
>
> Ditto.

I can't replace. There's the same reason in above.

>
>> +       const struct clk_div_table      *table;
>> +       spinlock_t      *lock;
>> +};
>> +
>> +struct hs_clk {
>> +       void __iomem    *pmctrl;
>> +       void __iomem    *sctrl;
>> +       spinlock_t      lock;
>> +};
>> +
>> +static struct hs_clk hs_clk;
>> +
>> +static void __init hs_init_clocks(void)
>> +{
>> +       struct device_node *node = NULL;
>> +
>> +       if (!hs_clk.pmctrl) {
>> +               /* map pmctrl registers */
>> +               node = of_find_compatible_node(NULL, NULL, "hisilicon,pmctrl");
>> +               hs_clk.pmctrl = of_iomap(node, 0);
>> +               WARN_ON(!hs_clk.pmctrl);
>> +       }
>> +
>> +       if (!hs_clk.sctrl) {
>> +               node = of_find_compatible_node(NULL, NULL, "hisilicon,sctrl");
>> +               hs_clk.sctrl = of_iomap(node, 0);
>> +       }
>> +}
>> +
>> +static int hi3620_clkgate_prepare(struct clk_hw *hw)
>> +{
>> +       struct hi3620_periclk *pclk;
>> +       unsigned long flags = 0;
>> +
>> +       pclk = container_of(hw, struct hi3620_periclk, hw);
>> +
>> +       if (pclk->lock)
>> +               spin_lock_irqsave(pclk->lock, flags);
>> +       writel_relaxed(pclk->ebits, pclk->enable + HI3620_DISABLE_OFF);
>> +       writel_relaxed(pclk->rbits, pclk->reset + HI3620_DISABLE_OFF);
>> +       readl_relaxed(pclk->reset + HI3620_STATUS_OFF);
>> +       if (pclk->lock)
>> +               spin_unlock_irqrestore(pclk->lock, flags);
>> +       return 0;
>> +}
>> +
>> +static int hi3620_clkgate_enable(struct clk_hw *hw)
>> +{
>> +       struct hi3620_periclk *pclk;
>> +       unsigned long flags = 0;
>> +
>> +       pclk = container_of(hw, struct hi3620_periclk, hw);
>> +       if (pclk->lock)
>> +               spin_lock_irqsave(pclk->lock, flags);
>> +       writel_relaxed(pclk->ebits, pclk->enable);
>> +       readl_relaxed(pclk->enable + HI3620_STATUS_OFF);
>> +       if (pclk->lock)
>> +               spin_unlock_irqrestore(pclk->lock, flags);
>> +       return 0;
>> +}
>> +
>> +static void hi3620_clkgate_disable(struct clk_hw *hw)
>> +{
>> +       struct hi3620_periclk *pclk;
>> +       unsigned long flags = 0;
>> +
>> +       pclk = container_of(hw, struct hi3620_periclk, hw);
>> +       if (pclk->lock)
>> +               spin_lock_irqsave(pclk->lock, flags);
>> +       writel_relaxed(pclk->ebits, pclk->enable + HI3620_DISABLE_OFF);
>> +       readl_relaxed(pclk->enable + HI3620_STATUS_OFF);
>> +       if (pclk->lock)
>> +               spin_unlock_irqrestore(pclk->lock, flags);
>> +}
>> +
>> +static struct clk_ops hi3620_clkgate_ops = {
>> +       .prepare        = hi3620_clkgate_prepare,
>> +       .enable         = hi3620_clkgate_enable,
>> +       .disable        = hi3620_clkgate_disable,
>
> .unprepare?

I didn't implement unprepare at here. Since I find some device controller
as uart not working if I disable clocks in unprepare(). Now I leave it as
unimplemented.

>
>> +};
>> +
>> +static void __init hi3620_clkgate_setup(struct device_node *np)
>> +{
>> +       struct hi3620_periclk *pclk;
>> +       struct clk_init_data *init;
>> +       struct clk *clk;
>> +       const char *clk_name, *name, **parent_names;
>> +       u32 rdata[2], gdata[2];
>> +
>> +       if (!hs_clk.sctrl)
>> +               return;
>> +
>> +       if (of_property_read_string(np, "clock-output-names", &clk_name))
>> +               return;
>> +       if (of_property_read_u32_array(np, "hisilicon,hi3620-clkreset",
>> +                                      &rdata[0], 2))
>> +               return;
>> +       if (of_property_read_u32_array(np, "hisilicon,hi3620-clkgate",
>> +                                      &gdata[0], 2))
>> +               return;
>> +
>> +       /* gate only has the fixed parent */
>> +       parent_names = kzalloc(sizeof(char *), GFP_KERNEL);
>> +       if (!parent_names)
>> +               return;
>> +       parent_names[0] = of_clk_get_parent_name(np, 0);
>> +
>> +       pclk = kzalloc(sizeof(*pclk), GFP_KERNEL);
>> +       if (!pclk)
>> +               goto err_pclk;
>> +
>> +       init = kzalloc(sizeof(*init), GFP_KERNEL);
>> +       if (!init)
>> +               goto err_init;
>> +       init->name = kstrdup(clk_name, GFP_KERNEL);
>> +       init->ops = &hi3620_clkgate_ops;
>> +       init->flags = CLK_SET_RATE_PARENT;
>> +       init->parent_names = parent_names;
>> +       init->num_parents = 1;
>> +
>> +       pclk->reset = hs_clk.sctrl + rdata[0];
>> +       pclk->rbits = rdata[1];
>> +       pclk->enable = hs_clk.sctrl + gdata[0];
>> +       pclk->ebits = gdata[1];
>> +       pclk->lock = &hs_clk.lock;
>> +       pclk->hw.init = init;
>> +
>> +       clk = clk_register(NULL, &pclk->hw);
>> +       if (IS_ERR(clk))
>> +               goto err_clk;
>> +       if (!of_property_read_string(np, "clock-names", &name))
>> +               clk_register_clkdev(clk, name, NULL);
>> +       of_clk_add_provider(np, of_clk_src_simple_get, clk);
>> +       return;
>> +err_clk:
>> +       kfree(init);
>> +err_init:
>> +       kfree(pclk);
>> +err_pclk:
>> +       kfree(parent_names);
>> +}
>> +
>> +static u8 hi3620_clk_get_parent(struct clk_hw *hw)
>> +{
>> +       struct hi3620_muxclk *mclk;
>> +       u32 data;
>> +       unsigned long flags = 0;
>> +
>> +       mclk = container_of(hw, struct hi3620_muxclk, hw);
>> +
>> +       if (mclk->lock)
>> +               spin_lock_irqsave(mclk->lock, flags);
>> +
>> +       data = readl_relaxed(mclk->reg) >> mclk->shift;
>> +       data &= WIDTH_TO_MASK(mclk->width);
>> +
>> +       if (mclk->lock)
>> +               spin_unlock_irqrestore(mclk->lock, flags);
>> +
>> +       if (data >= __clk_get_num_parents(hw->clk))
>> +               return -EINVAL;
>> +
>> +       return (u8)data;
>> +}
>> +
>> +static int hi3620_clk_set_parent(struct clk_hw *hw, u8 index)
>> +{
>> +       struct hi3620_muxclk *mclk;
>> +       u32 data;
>> +       unsigned long flags = 0;
>> +
>> +       mclk = container_of(hw, struct hi3620_muxclk, hw);
>> +
>> +       if (mclk->lock)
>> +               spin_lock_irqsave(mclk->lock, flags);
>> +
>> +       data = readl_relaxed(mclk->reg);
>> +       data &= ~(WIDTH_TO_MASK(mclk->width) << mclk->shift);
>> +       data |= index << mclk->shift;
>> +       writel_relaxed(data, mclk->reg);
>> +       /* set mask enable bits */
>> +       data |= mclk->mbits;
>> +       writel_relaxed(data, mclk->reg);
>
> Is mclk->mbits equivalent to (mclk->width << mclk->shift)?  If so you
> can drop shift+width and use only the mask + helpers like ffs().  If
> that is the case then this clock can probably use the generic clk-mux
> type.

No, I can't replace. Since they have higher 16 bits for mask bits.

>
>> +
>> +       if (mclk->lock)
>> +               spin_unlock_irqrestore(mclk->lock, flags);
>> +
>> +       return 0;
>> +}
>> +
>> +static struct clk_ops hi3620_clkmux_ops = {
>> +       .get_parent     = hi3620_clk_get_parent,
>> +       .set_parent     = hi3620_clk_set_parent,
>> +};
>> +
>> +static void __init hi3620_clkmux_setup(struct device_node *np)
>> +{
>> +       struct hi3620_muxclk *mclk;
>> +       struct clk_init_data *init;
>> +       struct clk *clk;
>> +       const char *clk_name, **parent_names;
>> +       u32 rdata[2];
>> +       u8 num_parents;
>> +       int i;
>> +
>> +       hs_init_clocks();
>> +       if (!hs_clk.sctrl)
>> +               return;
>> +
>> +       if (of_property_read_string(np, "clock-output-names", &clk_name))
>> +               return;
>> +       if (of_property_read_u32_array(np, "hisilicon,hi3620-clkmux",
>> +                                      &rdata[0], 2))
>> +               return;
>> +       /* get the count of items in mux */
>> +       for (i = 0; ; i++) {
>> +               /* parent's #clock-cells property is always 0 */
>> +               if (!of_parse_phandle(np, "clocks", i))
>> +                       break;
>> +       }
>> +       parent_names = kzalloc(sizeof(char *) * i, GFP_KERNEL);
>> +       if (!parent_names)
>> +               return;
>> +
>> +       for (num_parents = i, i = 0; i < num_parents; i++)
>> +               parent_names[i] = of_clk_get_parent_name(np, i);
>> +
>> +       mclk = kzalloc(sizeof(*mclk), GFP_KERNEL);
>> +       if (!mclk)
>> +               goto err_mclk;
>> +       init = kzalloc(sizeof(*init), GFP_KERNEL);
>> +       if (!init)
>> +               goto err_init;
>> +       init->name = kstrdup(clk_name, GFP_KERNEL);
>> +       init->ops = &hi3620_clkmux_ops;
>> +       init->flags = CLK_SET_RATE_PARENT;
>> +       init->parent_names = parent_names;
>> +       init->num_parents = num_parents;
>> +
>> +       mclk->reg = hs_clk.sctrl + rdata[0];
>> +       /* enable_mask bits are in higher 16bits */
>> +       mclk->mbits = rdata[1] << 16;
>> +       mclk->shift = ffs(rdata[1]) - 1;
>> +       mclk->width = fls(rdata[1]) - ffs(rdata[1]) + 1;
>> +       mclk->lock = &hs_clk.lock;
>> +       mclk->hw.init = init;
>> +
>> +       clk = clk_register(NULL, &mclk->hw);
>> +       if (IS_ERR(clk))
>> +               goto err_clk;
>> +       of_clk_add_provider(np, of_clk_src_simple_get, clk);
>> +
>> +       return;
>> +err_clk:
>> +       kfree(init);
>> +err_init:
>> +       kfree(mclk);
>> +err_mclk:
>> +       kfree(parent_names);
>> +}
>> +
>> +static void __init hs_clkgate_setup(struct device_node *np)
>> +{
>> +       struct clk *clk;
>> +       const char *clk_name, **parent_names, *name;
>> +       unsigned long flags = 0;
>> +       u32 data[2];
>> +
>> +       hs_init_clocks();
>> +       if (!hs_clk.sctrl)
>> +               return;
>> +       if (of_property_read_string(np, "clock-output-names", &clk_name))
>> +               return;
>> +       if (of_property_read_u32_array(np, "hisilicon,clkgate",
>> +                                      &data[0], 2))
>> +               return;
>> +       if (of_property_read_bool(np, "hisilicon,clkgate-inverted"))
>> +               flags = CLK_GATE_SET_TO_DISABLE;
>> +       /* gate only has the fixed parent */
>> +       parent_names = kzalloc(sizeof(char *), GFP_KERNEL);
>> +       if (!parent_names)
>> +               return;
>> +       parent_names[0] = of_clk_get_parent_name(np, 0);
>> +
>> +       clk = clk_register_gate(NULL, clk_name, parent_names[0], 0,
>> +                               hs_clk.sctrl + data[0], (u8)data[1], flags,
>> +                               &hs_clk.lock);
>> +       if (IS_ERR(clk))
>> +               goto err;
>> +       if (!of_property_read_string(np, "clock-names", &name))
>> +               clk_register_clkdev(clk, name, NULL);
>> +       of_clk_add_provider(np, of_clk_src_simple_get, clk);
>> +       return;
>> +err:
>> +       kfree(parent_names);
>> +}
>
> I'm working on clock bindings for the generic clock types, so hopefully
> this can be replaced by those instead of using your own compatible
> string to wrap around clk_register_gate.

It's great. Could you cc me when you send patches out?

>
>> +
>> +void __init hs_fixed_factor_setup(struct device_node *np)
>> +{
>> +       struct clk *clk;
>> +       const char *clk_name, **parent_names;
>> +       u32 data[2];
>> +
>> +       if (of_property_read_string(np, "clock-output-names", &clk_name))
>> +               return;
>> +       if (of_property_read_u32_array(np, "hisilicon,fixed-factor",
>> +                                      data, 2))
>> +               return ;
>> +       /* gate only has the fixed parent */
>> +       parent_names = kzalloc(sizeof(char *), GFP_KERNEL);
>> +       if (!parent_names)
>> +               return;
>> +       parent_names[0] = of_clk_get_parent_name(np, 0);
>> +
>> +       clk = clk_register_fixed_factor(NULL, clk_name, parent_names[0], 0,
>> +                                       data[0], data[1]);
>> +       if (IS_ERR(clk))
>> +               goto err;
>> +       of_clk_add_provider(np, of_clk_src_simple_get, clk);
>> +       return;
>> +err:
>> +       kfree(parent_names);
>> +}
>
> Ditto.
>
>> +
>> +static unsigned int hi3620_get_table_maxdiv(const struct clk_div_table *table)
>> +{
>> +       unsigned int maxdiv = 0;
>> +       const struct clk_div_table *clkt;
>> +
>> +       for (clkt = table; clkt->div; clkt++)
>> +               if (clkt->div > maxdiv)
>> +                       maxdiv = clkt->div;
>> +       return maxdiv;
>> +}
>> +
>> +static unsigned int hi3620_get_table_div(const struct clk_div_table *table,
>> +                                                       unsigned int val)
>> +{
>> +       const struct clk_div_table *clkt;
>> +
>> +       for (clkt = table; clkt->div; clkt++)
>> +               if (clkt->val == val)
>> +                       return clkt->div;
>> +       return 0;
>> +}
>> +
>> +static unsigned int hi3620_get_table_val(const struct clk_div_table *table,
>> +                                        unsigned int div)
>> +{
>> +       const struct clk_div_table *clkt;
>> +
>> +       for (clkt = table; clkt->div; clkt++)
>> +               if (clkt->div == div)
>> +                       return clkt->val;
>> +       return 0;
>> +}
>> +
>> +static unsigned long hi3620_clkdiv_recalc_rate(struct clk_hw *hw,
>> +                                              unsigned long parent_rate)
>> +{
>> +       struct hi3620_divclk *dclk = container_of(hw, struct hi3620_divclk, hw);
>> +       unsigned int div, val;
>> +
>> +       val = readl_relaxed(dclk->reg) >> dclk->shift;
>> +       val &= WIDTH_TO_MASK(dclk->width);
>> +
>> +       div = hi3620_get_table_div(dclk->table, val);
>> +       if (!div) {
>> +               pr_warning("%s: Invalid divisor for clock %s\n", __func__,
>> +                          __clk_get_name(hw->clk));
>> +               return parent_rate;
>> +       }
>> +
>> +       return parent_rate / div;
>> +}
>> +
>> +static bool hi3620_is_valid_table_div(const struct clk_div_table *table,
>> +                                     unsigned int div)
>> +{
>> +       const struct clk_div_table *clkt;
>> +
>> +       for (clkt = table; clkt->div; clkt++)
>> +               if (clkt->div == div)
>> +                       return true;
>> +       return false;
>> +}
>> +
>> +static int hi3620_clkdiv_bestdiv(struct clk_hw *hw, unsigned long rate,
>> +                                unsigned long *best_parent_rate)
>> +{
>> +       struct hi3620_divclk *dclk = container_of(hw, struct hi3620_divclk, hw);
>> +       struct clk *clk_parent = __clk_get_parent(hw->clk);
>> +       int i, bestdiv = 0;
>> +       unsigned long parent_rate, best = 0, now, maxdiv;
>> +
>> +       maxdiv = hi3620_get_table_maxdiv(dclk->table);
>> +
>> +       if (!(__clk_get_flags(hw->clk) & CLK_SET_RATE_PARENT)) {
>> +               parent_rate = *best_parent_rate;
>> +               bestdiv = DIV_ROUND_UP(parent_rate, rate);
>> +               bestdiv = bestdiv == 0 ? 1 : bestdiv;
>> +               bestdiv = bestdiv > maxdiv ? maxdiv : bestdiv;
>> +               return bestdiv;
>> +       }
>> +
>> +       /*
>> +        * The maximum divider we can use without overflowing
>> +        * unsigned long in rate * i below
>> +        */
>> +       maxdiv = min(ULONG_MAX / rate, maxdiv);
>> +
>> +       for (i = 1; i <= maxdiv; i++) {
>> +               if (!hi3620_is_valid_table_div(dclk->table, i))
>> +                       continue;
>> +               parent_rate = __clk_round_rate(clk_parent,
>> +                                              MULT_ROUND_UP(rate, i));
>> +               now = parent_rate / i;
>> +               if (now <= rate && now > best) {
>> +                       bestdiv = i;
>> +                       best = now;
>> +                       *best_parent_rate = parent_rate;
>> +               }
>> +       }
>> +
>> +       if (!bestdiv) {
>> +               bestdiv = hi3620_get_table_maxdiv(dclk->table);
>> +               *best_parent_rate = __clk_round_rate(clk_parent, 1);
>> +       }
>> +
>> +       return bestdiv;
>> +}
>> +
>> +static long hi3620_clkdiv_round_rate(struct clk_hw *hw, unsigned long rate,
>> +                                    unsigned long *prate)
>> +{
>> +       int div;
>> +
>> +       if (!rate)
>> +               rate = 1;
>> +       div = hi3620_clkdiv_bestdiv(hw, rate, prate);
>> +
>> +       return *prate / div;
>> +}
>> +
>> +static int hi3620_clkdiv_set_rate(struct clk_hw *hw, unsigned long rate,
>> +                                 unsigned long parent_rate)
>> +{
>> +       struct hi3620_divclk *dclk = container_of(hw, struct hi3620_divclk, hw);
>> +       unsigned int div, value;
>> +       unsigned long flags = 0;
>> +       u32 data;
>> +
>> +       div = parent_rate / rate;
>> +       value = hi3620_get_table_val(dclk->table, div);
>> +
>> +       if (value > WIDTH_TO_MASK(dclk->width))
>> +               value = WIDTH_TO_MASK(dclk->width);
>> +
>> +       if (dclk->lock)
>> +               spin_lock_irqsave(dclk->lock, flags);
>> +
>> +       data = readl_relaxed(dclk->reg);
>> +       data &= ~(WIDTH_TO_MASK(dclk->width) << dclk->shift);
>> +       data |= value << dclk->shift;
>> +       data |= dclk->mbits;
>> +       writel_relaxed(data, dclk->reg);
>> +
>> +       if (dclk->lock)
>> +               spin_unlock_irqrestore(dclk->lock, flags);
>> +
>> +       return 0;
>> +}
>> +
>> +static struct clk_ops hi3620_clkdiv_ops = {
>> +       .recalc_rate = hi3620_clkdiv_recalc_rate,
>> +       .round_rate = hi3620_clkdiv_round_rate,
>> +       .set_rate = hi3620_clkdiv_set_rate,
>> +};
>> +
>> +void __init hi3620_clkdiv_setup(struct device_node *np)
>> +{
>> +       struct clk *clk;
>> +       const char *clk_name, **parent_names;
>> +       struct clk_init_data *init;
>> +       struct clk_div_table *table;
>> +       struct hi3620_divclk *dclk;
>> +       unsigned int table_num;
>> +       int i;
>> +       u32 data[2];
>> +       const char *propname = "hisilicon,clkdiv-table";
>> +       const char *cellname = "#hisilicon,clkdiv-table-cells";
>> +       struct of_phandle_args div_table;
>> +
>> +       hs_init_clocks();
>> +       if (!hs_clk.sctrl)
>> +               return;
>> +
>> +       if (of_property_read_string(np, "clock-output-names", &clk_name))
>> +               return;
>> +       if (of_property_read_u32_array(np, "hisilicon,clkdiv",
>> +                                      &data[0], 2))
>> +               return;
>> +
>> +       /*process the div_table*/
>> +       for (i = 0; ; i++) {
>> +               if (of_parse_phandle_with_args(np, propname, cellname,
>> +                                              i, &div_table))
>> +                       break;
>> +       }
>> +
>> +       /*table ends with <0, 0>, so plus one to table_num*/
>> +       table_num = i + 1;
>> +
>> +       table = kzalloc(sizeof(struct clk_div_table) * table_num, GFP_KERNEL);
>> +       if (!table)
>> +               return ;
>> +
>> +       for (i = 0; ; i++) {
>> +               if (of_parse_phandle_with_args(np, propname, cellname,
>> +                                              i, &div_table))
>> +                       break;
>> +
>> +               table[i].val = div_table.args[0];
>> +               table[i].div = div_table.args[1];
>> +       }
>> +
>> +       /* gate only has the fixed parent */
>> +       parent_names = kzalloc(sizeof(char *), GFP_KERNEL);
>> +       if (!parent_names)
>> +               goto err_par;
>> +       parent_names[0] = of_clk_get_parent_name(np, 0);
>> +
>> +       dclk = kzalloc(sizeof(*dclk), GFP_KERNEL);
>> +       if (!dclk)
>> +               goto err_dclk;
>> +       init = kzalloc(sizeof(*init), GFP_KERNEL);
>> +       if (!init)
>> +               goto err_init;
>> +       init->name = kstrdup(clk_name, GFP_KERNEL);
>> +       init->ops = &hi3620_clkdiv_ops;
>> +       init->parent_names = parent_names;
>> +       init->num_parents = 1;
>> +
>> +       dclk->reg = hs_clk.sctrl + data[0];
>> +       dclk->shift = ffs(data[1]) - 1;
>> +       dclk->width = fls(data[1]) - ffs(data[1]) + 1;
>> +       dclk->mbits = data[1] << 16;
>> +       dclk->lock = &hs_clk.lock;
>> +       dclk->hw.init = init;
>> +       dclk->table = table;
>> +       clk = clk_register(NULL, &dclk->hw);
>> +       if (IS_ERR(clk))
>> +               goto err_clk;
>> +       of_clk_add_provider(np, of_clk_src_simple_get, clk);
>> +       return;
>> +err_clk:
>> +       kfree(init);
>> +err_init:
>> +       kfree(dclk);
>> +err_dclk:
>> +       kfree(parent_names);
>> +err_par:
>> +       kfree(table);
>> +}
>
> This looks like it was copy/pasted from the generic clk-divider type.
> Is there some reason you cannot use that instead of duplicating all of
> that code here?

Yes, it's a copy. The registers are not wrapped in their silicon. In most
vendor's SoC, the registers are read/write.

But they links the read/write to different registers. So one clock divider
register is split into four registers, and they're set, unset, status & final
status.

And the other reason is that they use higher 16 bits as mask bits. So
they're not compatible with other vendor's SoC. Of course, I can also
use generic clk-divider/clk-mux type, but I need to change a lot in
common files, such as abstracting read/write. Is it OK for you?

Regards
Haojian

>
> Thanks,
> Mike
>
>> +CLK_OF_DECLARE(hi3620_mux, "hisilicon,hi3620-clk-mux", hi3620_clkmux_setup)
>> +CLK_OF_DECLARE(hi3620_gate, "hisilicon,hi3620-clk-gate", hi3620_clkgate_setup)
>> +CLK_OF_DECLARE(hi3620_div, "hisilicon,hi3620-clk-div", hi3620_clkdiv_setup)
>> +CLK_OF_DECLARE(hs_gate, "hisilicon,clk-gate", hs_clkgate_setup)
>> +CLK_OF_DECLARE(hs_fixed, "hisilicon,clk-fixed-factor", hs_fixed_factor_setup)
>> --
>> 1.7.10.4

Quoting Haojian Zhuang (2013-05-13 18:30:38)
> On 14 May 2013 03:47, Mike Turquette <mturquette@linaro.org> wrote:
> > Quoting Haojian Zhuang (2013-04-19 03:49:24)
> >> +struct hi3620_muxclk {
> >> +       struct clk_hw   hw;
> >> +       void __iomem    *reg;           /* mux register */
> >> +       u8              shift;
> >> +       u8              width;
> >> +       u32             mbits;          /* mask bits in mux register */
> >
> > You need shift, width AND a mask?  Normally either shift+width OR a mask
> > would suffice.
> 
> No, it's a little different. In their mux registers, there're two
> parts. One is lower
> 16 bits that are used to set mux, and the other is higher 16 bits that are
> used to set mux mask.
> 
> At here, I use mbits for higher 16 bits and shift & width for lower 16 bits.
> 

Ok, good to know.

<snip>

> >> +static struct clk_ops hi3620_clkgate_ops = {
> >> +       .prepare        = hi3620_clkgate_prepare,
> >> +       .enable         = hi3620_clkgate_enable,
> >> +       .disable        = hi3620_clkgate_disable,
> >
> > .unprepare?
> 
> I didn't implement unprepare at here. Since I find some device controller
> as uart not working if I disable clocks in unprepare(). Now I leave it as
> unimplemented.
> 

Sounds like the UART driver needs to be fixed.  Do you have plans to
look into that further?  Having only the prepare and not the unprepare
callback feels like a hack to me.

<snip>

> >> +static void __init hs_clkgate_setup(struct device_node *np)
> >> +{
> >> +       struct clk *clk;
> >> +       const char *clk_name, **parent_names, *name;
> >> +       unsigned long flags = 0;
> >> +       u32 data[2];
> >> +
> >> +       hs_init_clocks();
> >> +       if (!hs_clk.sctrl)
> >> +               return;
> >> +       if (of_property_read_string(np, "clock-output-names", &clk_name))
> >> +               return;
> >> +       if (of_property_read_u32_array(np, "hisilicon,clkgate",
> >> +                                      &data[0], 2))
> >> +               return;
> >> +       if (of_property_read_bool(np, "hisilicon,clkgate-inverted"))
> >> +               flags = CLK_GATE_SET_TO_DISABLE;
> >> +       /* gate only has the fixed parent */
> >> +       parent_names = kzalloc(sizeof(char *), GFP_KERNEL);
> >> +       if (!parent_names)
> >> +               return;
> >> +       parent_names[0] = of_clk_get_parent_name(np, 0);
> >> +
> >> +       clk = clk_register_gate(NULL, clk_name, parent_names[0], 0,
> >> +                               hs_clk.sctrl + data[0], (u8)data[1], flags,
> >> +                               &hs_clk.lock);
> >> +       if (IS_ERR(clk))
> >> +               goto err;
> >> +       if (!of_property_read_string(np, "clock-names", &name))
> >> +               clk_register_clkdev(clk, name, NULL);
> >> +       of_clk_add_provider(np, of_clk_src_simple_get, clk);
> >> +       return;
> >> +err:
> >> +       kfree(parent_names);
> >> +}
> >
> > I'm working on clock bindings for the generic clock types, so hopefully
> > this can be replaced by those instead of using your own compatible
> > string to wrap around clk_register_gate.
> 
> It's great. Could you cc me when you send patches out?
> 

Yes I will Cc you.  I'll publish a gate-clock binding either this week
or next.  Do you want to wait for that and refactor your next version
based on it?

<snip>

> > This looks like it was copy/pasted from the generic clk-divider type.
> > Is there some reason you cannot use that instead of duplicating all of
> > that code here?
> 
> Yes, it's a copy. The registers are not wrapped in their silicon. In most
> vendor's SoC, the registers are read/write.
> 
> But they links the read/write to different registers. So one clock divider
> register is split into four registers, and they're set, unset, status & final
> status.
> 
> And the other reason is that they use higher 16 bits as mask bits. So
> they're not compatible with other vendor's SoC. Of course, I can also
> use generic clk-divider/clk-mux type, but I need to change a lot in
> common files, such as abstracting read/write. Is it OK for you?
> 

Ok, thanks for explaining that.  The purpose of the basic clock types
(like clk_divider) is to provide common code for platforms that use a
common programming model for their clock controls.  If you think that
you'll have other platforms with similar 4-register dividers then it
may be worth your time to try to abstract it.  However since no other
platforms have this sort of requirement it does not qualify as being
"common".  The choice is yours if you want to give it a shot.  If the
changes are very invasive and ugly for the common divider type (such as
modifying the existing registration function for all users) then I won't
take it.

An alternative might be to create a "superset" clock using the basic
divider.  An example of this is the i.MX divider which uses a "busy
bit".  See here: arch/arm/mach-imx/clk-busy.c

I think you're programming model may be to different to use that, I'm
not sure, but the idea is to reuse the code and just wrap the basic
clock type with your specific clock requirements.

Regards,
Mike

> Regards
> Haojian
> 
> >
> > Thanks,
> > Mike
> >
> >> +CLK_OF_DECLARE(hi3620_mux, "hisilicon,hi3620-clk-mux", hi3620_clkmux_setup)
> >> +CLK_OF_DECLARE(hi3620_gate, "hisilicon,hi3620-clk-gate", hi3620_clkgate_setup)
> >> +CLK_OF_DECLARE(hi3620_div, "hisilicon,hi3620-clk-div", hi3620_clkdiv_setup)
> >> +CLK_OF_DECLARE(hs_gate, "hisilicon,clk-gate", hs_clkgate_setup)
> >> +CLK_OF_DECLARE(hs_fixed, "hisilicon,clk-fixed-factor", hs_fixed_factor_setup)
> >> --
> >> 1.7.10.4

On 15 May 2013 00:22, Mike Turquette <mturquette@linaro.org> wrote:

> Quoting Haojian Zhuang (2013-05-13 18:30:38)
> > On 14 May 2013 03:47, Mike Turquette <mturquette@linaro.org> wrote:
> > >> +static struct clk_ops hi3620_clkgate_ops = {
> > >> +       .prepare        = hi3620_clkgate_prepare,
> > >> +       .enable         = hi3620_clkgate_enable,
> > >> +       .disable        = hi3620_clkgate_disable,
> > >
> > > .unprepare?
> >
> > I didn't implement unprepare at here. Since I find some device controller
> > as uart not working if I disable clocks in unprepare(). Now I leave it as
> > unimplemented.
> >
>
> Sounds like the UART driver needs to be fixed.  Do you have plans to
> look into that further?  Having only the prepare and not the unprepare
> callback feels like a hack to me.
>
>
> I can't get a conclusion that there's an issue in UART driver. I don't have
a good debug tool to investigate this issue. So I want to keep it as a
workaround. Of course, this issue should be resolved in the future.


> > >> +static void __init hs_clkgate_setup(struct device_node *np)
> > >> +{
> > >> +       struct clk *clk;
> > >> +       const char *clk_name, **parent_names, *name;
> > >> +       unsigned long flags = 0;
> > >> +       u32 data[2];
> > >> +
> > >> +       hs_init_clocks();
> > >> +       if (!hs_clk.sctrl)
> > >> +               return;
> > >> +       if (of_property_read_string(np, "clock-output-names",
> &clk_name))
> > >> +               return;
> > >> +       if (of_property_read_u32_array(np, "hisilicon,clkgate",
> > >> +                                      &data[0], 2))
> > >> +               return;
> > >> +       if (of_property_read_bool(np, "hisilicon,clkgate-inverted"))
> > >> +               flags = CLK_GATE_SET_TO_DISABLE;
> > >> +       /* gate only has the fixed parent */
> > >> +       parent_names = kzalloc(sizeof(char *), GFP_KERNEL);
> > >> +       if (!parent_names)
> > >> +               return;
> > >> +       parent_names[0] = of_clk_get_parent_name(np, 0);
> > >> +
> > >> +       clk = clk_register_gate(NULL, clk_name, parent_names[0], 0,
> > >> +                               hs_clk.sctrl + data[0], (u8)data[1],
> flags,
> > >> +                               &hs_clk.lock);
> > >> +       if (IS_ERR(clk))
> > >> +               goto err;
> > >> +       if (!of_property_read_string(np, "clock-names", &name))
> > >> +               clk_register_clkdev(clk, name, NULL);
> > >> +       of_clk_add_provider(np, of_clk_src_simple_get, clk);
> > >> +       return;
> > >> +err:
> > >> +       kfree(parent_names);
> > >> +}
> > >
> > > I'm working on clock bindings for the generic clock types, so hopefully
> > > this can be replaced by those instead of using your own compatible
> > > string to wrap around clk_register_gate.
> >
> > It's great. Could you cc me when you send patches out?
> >
>
> Yes I will Cc you.  I'll publish a gate-clock binding either this week
> or next.  Do you want to wait for that and refactor your next version
> based on it?
>
>
> Yes, I want to refactor my next version based on your patches.


> > > This looks like it was copy/pasted from the generic clk-divider type.
> > > Is there some reason you cannot use that instead of duplicating all of
> > > that code here?
> >
> > Yes, it's a copy. The registers are not wrapped in their silicon. In most
> > vendor's SoC, the registers are read/write.
> >
> > But they links the read/write to different registers. So one clock
> divider
> > register is split into four registers, and they're set, unset, status &
> final
> > status.
> >
> > And the other reason is that they use higher 16 bits as mask bits. So
> > they're not compatible with other vendor's SoC. Of course, I can also
> > use generic clk-divider/clk-mux type, but I need to change a lot in
> > common files, such as abstracting read/write. Is it OK for you?
> >
>
> Ok, thanks for explaining that.  The purpose of the basic clock types
> (like clk_divider) is to provide common code for platforms that use a
> common programming model for their clock controls.  If you think that
> you'll have other platforms with similar 4-register dividers then it
> may be worth your time to try to abstract it.  However since no other
> platforms have this sort of requirement it does not qualify as being
> "common".  The choice is yours if you want to give it a shot.  If the
> changes are very invasive and ugly for the common divider type (such as
> modifying the existing registration function for all users) then I won't
> take it.
>
> An alternative might be to create a "superset" clock using the basic
> divider.  An example of this is the i.MX divider which uses a "busy
> bit".  See here: arch/arm/mach-imx/clk-busy.c
>
> I think you're programming model may be to different to use that, I'm
> not sure, but the idea is to reuse the code and just wrap the basic
> clock type with your specific clock requirements.
>
> Regards,
> Mike
>
>
I'll add some flags to reuse the common code. I'll try to keep it simple
and clear.

Regards
Haojian