[02/13] clk: mediatek: Add initial common clock support for Mediatek SoCs.

Commit Message

Sascha Hauer Feb. 9, 2015, 10:47 a.m. UTC

From: James Liao <[e-mail]>

This patch adds common clock support for Mediatek SoCs, including plls,
muxes and clock gates.

Signed-off-by: James Liao <[e-mail]>
Signed-off-by: Henry Chen <[e-mail]>
Signed-off-by: Sascha Hauer <[e-mail]>
---
 drivers/clk/Makefile            |   1 +
 drivers/clk/mediatek/Makefile   |   1 +
 drivers/clk/mediatek/clk-gate.c | 137 +++++++++++++++++++++++++++++++++++
 drivers/clk/mediatek/clk-gate.h |  49 +++++++++++++
 drivers/clk/mediatek/clk-mtk.c  | 155 ++++++++++++++++++++++++++++++++++++++++
 drivers/clk/mediatek/clk-mtk.h  | 133 ++++++++++++++++++++++++++++++++++
 drivers/clk/mediatek/clk-pll.c  |  63 ++++++++++++++++
 drivers/clk/mediatek/clk-pll.h  |  52 ++++++++++++++
 8 files changed, 591 insertions(+)
 create mode 100644 drivers/clk/mediatek/Makefile
 create mode 100644 drivers/clk/mediatek/clk-gate.c
 create mode 100644 drivers/clk/mediatek/clk-gate.h
 create mode 100644 drivers/clk/mediatek/clk-mtk.c
 create mode 100644 drivers/clk/mediatek/clk-mtk.h
 create mode 100644 drivers/clk/mediatek/clk-pll.c
 create mode 100644 drivers/clk/mediatek/clk-pll.h

Comments

Tomasz Figa Feb. 13, 2015, 7:41 a.m. UTC | #1

Hi,

Let me add some suggestions inline.

On Mon, Feb 9, 2015 at 7:47 PM, Sascha Hauer <[e-mail]> wrote:
> From: James Liao <[e-mail]>
>
> This patch adds common clock support for Mediatek SoCs, including plls,
> muxes and clock gates.

[snip]

> +static int mtk_cg_enable(struct clk_hw *hw)
> +{
> +       mtk_cg_clr_bit(hw);
> +
> +       return 0;
> +}
> +
> +static void mtk_cg_disable(struct clk_hw *hw)
> +{
> +       mtk_cg_set_bit(hw);
> +}
> +
> +static int mtk_cg_enable_inv(struct clk_hw *hw)
> +{
> +       mtk_cg_set_bit(hw);
> +
> +       return 0;
> +}
> +
> +static void mtk_cg_disable_inv(struct clk_hw *hw)
> +{
> +       mtk_cg_clr_bit(hw);
> +}

Instead of duplicating the ops, couldn't you add a flag or something
to mtk_clk_gate struct and then act appropriately in the ops? Also,
see below.

> +
> +const struct clk_ops mtk_clk_gate_ops_setclr = {
> +       .is_enabled     = mtk_cg_bit_is_cleared,
> +       .enable         = mtk_cg_enable,
> +       .disable        = mtk_cg_disable,
> +};
> +
> +const struct clk_ops mtk_clk_gate_ops_setclr_inv = {
> +       .is_enabled     = mtk_cg_bit_is_set,
> +       .enable         = mtk_cg_enable_inv,
> +       .disable        = mtk_cg_disable_inv,
> +};
> +
> +struct clk *mtk_clk_register_gate(
> +               const char *name,
> +               const char *parent_name,
> +               struct regmap *regmap,
> +               int set_ofs,
> +               int clr_ofs,
> +               int sta_ofs,
> +               u8 bit,
> +               const struct clk_ops *ops)

Instead of passing the ops here you would have some flags or even just
a single bool inverted. Then the ops struct could be made static.

Also it would be nice to have a kerneldoc-style comment documenting
arguments of this function. Same thing applies to other structs added
by this and related patches and non-static functions.

also CodingStyle: I believe it is not kernel coding style to push
every argument to new line, even if few of them can fit one line.
Similar thing applies to other functions added by this and related
patches using this convention.

> +{
> +       struct mtk_clk_gate *cg;
> +       struct clk *clk;
> +       struct clk_init_data init;
> +
> +       cg = kzalloc(sizeof(*cg), GFP_KERNEL);
> +       if (!cg)
> +               return ERR_PTR(-ENOMEM);
> +
> +       init.name = name;
> +       init.flags = CLK_SET_RATE_PARENT;
> +       init.parent_names = parent_name ? &parent_name : NULL;
> +       init.num_parents = parent_name ? 1 : 0;
> +       init.ops = ops;
> +
> +       cg->regmap = regmap;
> +       cg->set_ofs = set_ofs;
> +       cg->clr_ofs = clr_ofs;
> +       cg->sta_ofs = sta_ofs;
> +       cg->bit = bit;
> +
> +       cg->hw.init = &init;
> +
> +       clk = clk_register(NULL, &cg->hw);
> +       if (IS_ERR(clk))
> +               kfree(cg);
> +
> +       return clk;
> +}
> diff --git a/drivers/clk/mediatek/clk-gate.h b/drivers/clk/mediatek/clk-gate.h
> new file mode 100644
> index 0000000..a44dcbf
> --- /dev/null
> +++ b/drivers/clk/mediatek/clk-gate.h
> @@ -0,0 +1,49 @@
> +/*
> + * Copyright (c) 2014 MediaTek Inc.
> + * Author: James Liao <[e-mail]>

Might not be necessary, but maybe the other people (all or some of
them) from signed-off-by should be added to this and other copyright
statements?

> + *
> + * 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 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.
> + */
> +
> +#ifndef __DRV_CLK_GATE_H
> +#define __DRV_CLK_GATE_H
> +
> +/*
> + * This is a private header file. DO NOT include it except clk-*.c.
> + */

I believe the above comment is unnecessary, because the file is
already located in drivers/clk/mediatek.

> +#include <linux/regmap.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +
> +struct mtk_clk_gate {

It would be nice to have a kerneldoc-style comment describing fields
of this struct.

> +       struct clk_hw   hw;
> +       struct regmap   *regmap;
> +       int             set_ofs;
> +       int             clr_ofs;
> +       int             sta_ofs;
> +       u8              bit;
> +};
> +
> +#define to_clk_gate(_hw) container_of(_hw, struct mtk_clk_gate, hw)

I believe static inline is preferred to macros for such helpers, due
to increased type safety.

> +
> +extern const struct clk_ops mtk_clk_gate_ops_setclr;
> +extern const struct clk_ops mtk_clk_gate_ops_setclr_inv;
> +
> +struct clk *mtk_clk_register_gate(
> +               const char *name,
> +               const char *parent_name,
> +               struct regmap *regmap,
> +               int set_ofs,
> +               int clr_ofs,
> +               int sta_ofs,
> +               u8 bit,
> +               const struct clk_ops *ops);
> +
> +#endif /* __DRV_CLK_GATE_H */
> diff --git a/drivers/clk/mediatek/clk-mtk.c b/drivers/clk/mediatek/clk-mtk.c
> new file mode 100644
> index 0000000..479857c
> --- /dev/null
> +++ b/drivers/clk/mediatek/clk-mtk.c
> @@ -0,0 +1,155 @@
> +/*
> + * Copyright (c) 2014 MediaTek Inc.
> + * Author: James Liao <[e-mail]>
> + *
> + * 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 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.
> + */
> +
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +
> +#include <linux/io.h>
> +#include <linux/slab.h>
> +#include <linux/delay.h>
> +#include <linux/clkdev.h>
> +
> +#include "clk-mtk.h"
> +#include "clk-gate.h"
> +
> +void mtk_init_factors(struct mtk_fixed_factor *clks, int num,
> +               struct clk_onecell_data *clk_data)
> +{
> +       int i;
> +       struct clk *clk;
> +
> +       for (i = 0; i < num; i++) {
> +               struct mtk_fixed_factor *ff = &clks[i];
> +
> +               clk = clk_register_fixed_factor(NULL, ff->name, ff->parent_name,
> +                               CLK_SET_RATE_PARENT, ff->mult, ff->div);
> +
> +               if (IS_ERR(clk)) {
> +                       pr_err("Failed to register clk %s: %ld\n",
> +                                       ff->name, PTR_ERR(clk));
> +                       continue;
> +               }
> +
> +               if (clk_data)
> +                       clk_data->clks[ff->id] = clk;
> +       }
> +}
> +
> +void mtk_init_clk_gates(struct regmap *regmap,
> +               struct mtk_gate *clks, int num,
> +               struct clk_onecell_data *clk_data)
> +{
> +       int i;
> +       struct clk *clk;
> +
> +       for (i = 0; i < num; i++) {
> +               struct mtk_gate *gate = &clks[i];
> +
> +               clk = mtk_clk_register_gate(gate->name, gate->parent_name,
> +                               regmap,
> +                               gate->regs->set_ofs,
> +                               gate->regs->clr_ofs,
> +                               gate->regs->sta_ofs,
> +                               gate->shift, gate->ops);
> +
> +               if (IS_ERR(clk)) {
> +                       pr_err("Failed to register clk %s: %ld\n",
> +                                       gate->name, PTR_ERR(clk));
> +                       continue;
> +               }
> +
> +               if (clk_data)
> +                       clk_data->clks[gate->id] = clk;
> +       }
> +}
> +
> +struct clk_onecell_data *mtk_alloc_clk_data(unsigned int clk_num)
> +{
> +       int i;
> +       struct clk_onecell_data *clk_data;
> +
> +       clk_data = kzalloc(sizeof(clk_data), GFP_KERNEL);

Shouldn't it be sizeof(*clk_data)?

> +       if (!clk_data)
> +               return NULL;
> +
> +       clk_data->clks = kcalloc(clk_num, sizeof(struct clk *), GFP_KERNEL);

sizeof(*clk_data->clks)

> +       if (!clk_data->clks) {
> +               kfree(clk_data);
> +               return NULL;
> +       }
> +
> +       clk_data->clk_num = clk_num;
> +
> +       for (i = 0; i < clk_num; ++i)
> +               clk_data->clks[i] = ERR_PTR(-ENOENT);
> +
> +       return clk_data;
> +}
> +
> +struct clk *mtk_clk_register_mux(
> +               const char *name,
> +               const char **parent_names,
> +               u8 num_parents,
> +               void __iomem *base_addr,
> +               u8 shift,
> +               u8 width,
> +               u8 gate_bit,
> +               spinlock_t *lock)
> +{
> +       struct clk *clk;
> +       struct clk_mux *mux;
> +       struct clk_gate *gate = NULL;
> +       struct clk_hw *gate_hw = NULL;
> +       const struct clk_ops *gate_ops = NULL;
> +       u32 mask = BIT(width) - 1;
> +
> +       mux = kzalloc(sizeof(struct clk_mux), GFP_KERNEL);

sizeof(*mux)

> +       if (!mux)
> +               return ERR_PTR(-ENOMEM);
> +
> +       mux->reg = base_addr;
> +       mux->mask = mask;
> +       mux->shift = shift;
> +       mux->flags = 0;

Flags field was already zeroed by kzalloc().

> +       mux->lock = lock;
> +
> +       if (gate_bit <= MAX_MUX_GATE_BIT) {
> +               gate = kzalloc(sizeof(struct clk_gate), GFP_KERNEL);

sizeof(*gate)

> +               if (!gate) {
> +                       kfree(mux);

Please use goto style error path below the main code instead of
duplicating roll-back actions in every error check.

> +                       return ERR_PTR(-ENOMEM);
> +               }
> +
> +               gate->reg = base_addr;
> +               gate->bit_idx = gate_bit;
> +               gate->flags = CLK_GATE_SET_TO_DISABLE;
> +               gate->lock = lock;
> +
> +               gate_hw = &gate->hw;
> +               gate_ops = &clk_gate_ops;
> +       }
> +
> +       clk = clk_register_composite(NULL, name, parent_names, num_parents,
> +               &mux->hw, &clk_mux_ops,
> +               NULL, NULL,
> +               gate_hw, gate_ops,
> +               CLK_SET_RATE_PARENT);
> +
> +       if (IS_ERR(clk)) {
> +               kfree(gate);
> +               kfree(mux);
> +       }
> +
> +       return clk;
> +}
> diff --git a/drivers/clk/mediatek/clk-mtk.h b/drivers/clk/mediatek/clk-mtk.h
> new file mode 100644
> index 0000000..35cf9a3
> --- /dev/null
> +++ b/drivers/clk/mediatek/clk-mtk.h
> @@ -0,0 +1,133 @@
> +/*
> + * Copyright (c) 2014 MediaTek Inc.
> + * Author: James Liao <[e-mail]>
> + *
> + * 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 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.
> + */
> +
> +#ifndef __DRV_CLK_MTK_H
> +#define __DRV_CLK_MTK_H
> +
> +/*
> + * This is a private header file. DO NOT include it except clk-*.c.
> + */
> +
> +#include <linux/regmap.h>
> +#include <linux/bitops.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +
> +#define MAX_MUX_GATE_BIT       31
> +#define INVALID_MUX_GATE_BIT   (MAX_MUX_GATE_BIT + 1)

It might be a good idea to describe what this value is used for.

> +
> +struct mtk_fixed_factor {
> +       int id;
> +       const char *name;
> +       const char *parent_name;
> +       int mult;
> +       int div;
> +};
> +
> +#define FACTOR(_id, _name, _parent, _mult, _div) {     \
> +               .id = _id,                              \
> +               .name = _name,                          \
> +               .parent_name = _parent,                 \
> +               .mult = _mult,                          \
> +               .div = _div,                            \
> +       }
> +
> +extern void mtk_init_factors(struct mtk_fixed_factor *clks, int num,
> +               struct clk_onecell_data *clk_data);
> +
> +struct mtk_mux {
> +       int id;
> +       const char *name;
> +       uint32_t reg;
> +       int shift;
> +       int width;
> +       int gate;
> +       const char **parent_names;
> +       int num_parents;
> +};
> +
> +#define MUX(_id, _name, _parents, _reg, _shift, _width, _gate) {       \
> +               .id = _id,                                              \
> +               .name = _name,                                          \
> +               .reg = _reg,                                            \
> +               .shift = _shift,                                        \
> +               .width = _width,                                        \
> +               .gate = _gate,                                          \
> +               .parent_names = (const char **)_parents,                \

Hmm, it doesn't sound like a good idea to hide casts like this inside
a macro. Anyway, is this cast even necessary? Your _parents argument
to this macro should be always of correct type.

> +               .num_parents = ARRAY_SIZE(_parents),                    \
> +       }
> +
> +struct mtk_pll {
> +       int id;
> +       const char *name;
> +       const char *parent_name;
> +       uint32_t reg;
> +       uint32_t pwr_reg;
> +       uint32_t en_mask;
> +       unsigned int flags;
> +       const struct clk_ops *ops;
> +};
> +
> +#define PLL(_id, _name, _parent, _reg, _pwr_reg, _en_mask, _flags, _ops) { \
> +               .id = _id,                                              \
> +               .name = _name,                                          \
> +               .parent_name = _parent,                                 \
> +               .reg = _reg,                                            \
> +               .pwr_reg = _pwr_reg,                                    \
> +               .en_mask = _en_mask,                                    \
> +               .flags = _flags,                                        \
> +               .ops = _ops,                                            \
> +       }
> +
> +struct mtk_gate_regs {
> +       u32 sta_ofs;
> +       u32 clr_ofs;
> +       u32 set_ofs;
> +};
> +
> +struct mtk_gate {
> +       int id;
> +       const char *name;
> +       const char *parent_name;
> +       struct mtk_gate_regs *regs;
> +       int shift;
> +       const struct clk_ops *ops;
> +};
> +
> +#define GATE(_id, _name, _parent, _regs, _shift, _ops) {       \
> +               .id = _id,                                      \
> +               .name = _name,                                  \
> +               .parent_name = _parent,                         \
> +               .regs = &_regs,                                 \

Also hiding operators like & inside macros like this doesn't help with
code readability and it doesn't cost that much to just add & when
calling this macro.

> +               .shift = _shift,                                \
> +               .ops = _ops,                                    \
> +       }
> +
> +void mtk_init_clk_gates(struct regmap *regmap,
> +               struct mtk_gate *clks, int num,
> +               struct clk_onecell_data *clk_data);
> +
> +struct clk_onecell_data *mtk_alloc_clk_data(unsigned int clk_num);
> +
> +struct clk *mtk_clk_register_mux(
> +               const char *name,
> +               const char **parent_names,
> +               u8 num_parents,
> +               void __iomem *base_addr,
> +               u8 shift,
> +               u8 width,
> +               u8 gate_bit,
> +               spinlock_t *lock);
> +
> +#endif /* __DRV_CLK_MTK_H */
> diff --git a/drivers/clk/mediatek/clk-pll.c b/drivers/clk/mediatek/clk-pll.c
> new file mode 100644
> index 0000000..59dee83
> --- /dev/null
> +++ b/drivers/clk/mediatek/clk-pll.c
> @@ -0,0 +1,63 @@
> +/*
> + * Copyright (c) 2014 MediaTek Inc.
> + * Author: James Liao <[e-mail]>
> + *
> + * 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 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.
> + */
> +
> +#include <linux/io.h>
> +#include <linux/slab.h>
> +#include <linux/clkdev.h>
> +
> +#include "clk-mtk.h"
> +#include "clk-pll.h"
> +
> +struct clk *mtk_clk_register_pll(
> +               const char *name,
> +               const char *parent_name,
> +               u32 *base_addr,
> +               u32 *pwr_addr,
> +               u32 en_mask,
> +               u32 flags,
> +               const struct clk_ops *ops,
> +               spinlock_t *lock)
> +{
> +       struct mtk_clk_pll *pll;
> +       struct clk_init_data init;
> +       struct clk *clk;
> +
> +       pr_debug("%s(): name: %s\n", __func__, name);
> +
> +       if (!lock)
> +               return ERR_PTR(-EINVAL);

Hmm, this check seems to be slightly inconsistent. Why you need to
check lock, but not name, parent_name and other arguments of this
function? Also bailing out without any error message isn't really the
best practice.

Anyway, if this function is defined to require the lock argument to be
non-NULL then this is not a natural error condition but rather a
mistake of the author of code calling this function with lock == NULL.
IMHO either BUG_ON(!lock) or just remove this check, but I'd like to
know Mike's thoughts on this before you make this change.

> +
> +       pll = kzalloc(sizeof(*pll), GFP_KERNEL);
> +       if (!pll)
> +               return ERR_PTR(-ENOMEM);
> +
> +       pll->base_addr = base_addr;
> +       pll->pwr_addr = pwr_addr;
> +       pll->en_mask = en_mask;
> +       pll->flags = flags;
> +       pll->lock = lock;
> +       pll->hw.init = &init;
> +
> +       init.name = name;
> +       init.ops = ops;
> +       init.parent_names = &parent_name;
> +       init.num_parents = 1;
> +
> +       clk = clk_register(NULL, &pll->hw);
> +
> +       if (IS_ERR(clk))
> +               kfree(pll);
> +
> +       return clk;
> +}
> diff --git a/drivers/clk/mediatek/clk-pll.h b/drivers/clk/mediatek/clk-pll.h
> new file mode 100644
> index 0000000..341d2fe
> --- /dev/null
> +++ b/drivers/clk/mediatek/clk-pll.h
> @@ -0,0 +1,52 @@
> +/*
> + * Copyright (c) 2014 MediaTek Inc.
> + * Author: James Liao <[e-mail]>
> + *
> + * 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 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.
> + */
> +
> +#ifndef __DRV_CLK_PLL_H
> +#define __DRV_CLK_PLL_H
> +
> +/*
> + * This is a private header file. DO NOT include it except clk-*.c.
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +
> +struct mtk_clk_pll {
> +       struct clk_hw   hw;
> +       void __iomem    *base_addr;
> +       void __iomem    *pwr_addr;
> +       u32             en_mask;
> +       u32             flags;
> +       spinlock_t      *lock;
> +};
> +
> +#define to_mtk_clk_pll(_hw) container_of(_hw, struct mtk_clk_pll, hw)

Static inline please.

Best regards,
Tomasz

Sascha Hauer Feb. 13, 2015, 12:06 p.m. UTC | #2

Hi Tomasz,

> > +static void mtk_cg_disable(struct clk_hw *hw)
> > +{
> > +       mtk_cg_set_bit(hw);
> > +}
> > +
> > +static int mtk_cg_enable_inv(struct clk_hw *hw)
> > +{
> > +       mtk_cg_set_bit(hw);
> > +
> > +       return 0;
> > +}
> > +
> > +static void mtk_cg_disable_inv(struct clk_hw *hw)
> > +{
> > +       mtk_cg_clr_bit(hw);
> > +}
> 
> Instead of duplicating the ops, couldn't you add a flag or something
> to mtk_clk_gate struct and then act appropriately in the ops? Also,
> see below.

I prefer duplicating the ops. It makes the functions simpler without
ifs.

> > diff --git a/drivers/clk/mediatek/clk-gate.h b/drivers/clk/mediatek/clk-gate.h
> > new file mode 100644
> > index 0000000..a44dcbf
> > --- /dev/null
> > +++ b/drivers/clk/mediatek/clk-gate.h
> > @@ -0,0 +1,49 @@
> > +/*
> > + * Copyright (c) 2014 MediaTek Inc.
> > + * Author: James Liao <[e-mail]>
> 
> Might not be necessary, but maybe the other people (all or some of
> them) from signed-off-by should be added to this and other copyright
> statements?

I rather do not want to update these copyrights frequently. Otherwise we
would see a lot of patches with an extra hunk changing the copyrights.
I'm glad we left that behind and look at the git history instead.
The above is the original author. I don't want to remove him, but I also
do not want to add every other person who touched that file.

The other stuff will be fixed in the next round. Thanks for reviewing.

Sascha

Tomasz Figa Feb. 13, 2015, 1:22 p.m. UTC | #3

Hi Sascha,

On Fri, Feb 13, 2015 at 9:06 PM, Sascha Hauer <[e-mail]> wrote:
>
> Hi Tomasz,
>
>> > +static void mtk_cg_disable(struct clk_hw *hw)
>> > +{
>> > +       mtk_cg_set_bit(hw);
>> > +}
>> > +
>> > +static int mtk_cg_enable_inv(struct clk_hw *hw)
>> > +{
>> > +       mtk_cg_set_bit(hw);
>> > +
>> > +       return 0;
>> > +}
>> > +
>> > +static void mtk_cg_disable_inv(struct clk_hw *hw)
>> > +{
>> > +       mtk_cg_clr_bit(hw);
>> > +}
>>
>> Instead of duplicating the ops, couldn't you add a flag or something
>> to mtk_clk_gate struct and then act appropriately in the ops? Also,
>> see below.
>
> I prefer duplicating the ops. It makes the functions simpler without
> ifs.

I meant something else. Compared to ifs I'd prefer duplicated ops too.

is_enabled()
{
    status = regmap_read() ^ (inverted << shift);
    return status & BIT(shift);
}

However I missed the fact that writing uses set and clear registers,
which effectively means that this approach can't really be used for
writing, so I'm okay with keeping this as is.

>
>> > diff --git a/drivers/clk/mediatek/clk-gate.h b/drivers/clk/mediatek/clk-gate.h
>> > new file mode 100644
>> > index 0000000..a44dcbf
>> > --- /dev/null
>> > +++ b/drivers/clk/mediatek/clk-gate.h
>> > @@ -0,0 +1,49 @@
>> > +/*
>> > + * Copyright (c) 2014 MediaTek Inc.
>> > + * Author: James Liao <[e-mail]>
>>
>> Might not be necessary, but maybe the other people (all or some of
>> them) from signed-off-by should be added to this and other copyright
>> statements?
>
> I rather do not want to update these copyrights frequently. Otherwise we
> would see a lot of patches with an extra hunk changing the copyrights.
> I'm glad we left that behind and look at the git history instead.
> The above is the original author. I don't want to remove him, but I also
> do not want to add every other person who touched that file.

Alright. I just wanted to make sure that this is desired state.

>
> The other stuff will be fixed in the next round. Thanks for reviewing.

You're welcome. Looking forward for next revision.

Best regards,
Tomasz

Patch

diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index d5fba5b..ce6c250 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -47,6 +47,7 @@  obj-$(CONFIG_ARCH_HI3xxx)		+= hisilicon/
 obj-$(CONFIG_ARCH_HIP04)		+= hisilicon/
 obj-$(CONFIG_ARCH_HIX5HD2)		+= hisilicon/
 obj-$(CONFIG_COMMON_CLK_KEYSTONE)	+= keystone/
+obj-$(CONFIG_ARCH_MEDIATEK)		+= mediatek/
 ifeq ($(CONFIG_COMMON_CLK), y)
 obj-$(CONFIG_ARCH_MMP)			+= mmp/
 endif
diff --git a/drivers/clk/mediatek/Makefile b/drivers/clk/mediatek/Makefile
new file mode 100644
index 0000000..c384e97
--- /dev/null
+++ b/drivers/clk/mediatek/Makefile
@@ -0,0 +1 @@ 
+obj-y += clk-mtk.o clk-pll.o clk-gate.o
diff --git a/drivers/clk/mediatek/clk-gate.c b/drivers/clk/mediatek/clk-gate.c
new file mode 100644
index 0000000..9d77ee3
--- /dev/null
+++ b/drivers/clk/mediatek/clk-gate.c
@@ -0,0 +1,137 @@ 
+/*
+ * Copyright (c) 2014 MediaTek Inc.
+ * Author: James Liao <[e-mail]>
+ *
+ * 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 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.
+ */
+
+#include <linux/of.h>
+#include <linux/of_address.h>
+
+#include <linux/io.h>
+#include <linux/slab.h>
+#include <linux/delay.h>
+#include <linux/clkdev.h>
+
+#include "clk-mtk.h"
+#include "clk-gate.h"
+
+static int mtk_cg_bit_is_cleared(struct clk_hw *hw)
+{
+	struct mtk_clk_gate *cg = to_clk_gate(hw);
+	u32 val;
+
+	regmap_read(cg->regmap, cg->sta_ofs, &val);
+
+	val &= BIT(cg->bit);
+
+	return val == 0;
+}
+
+static int mtk_cg_bit_is_set(struct clk_hw *hw)
+{
+	struct mtk_clk_gate *cg = to_clk_gate(hw);
+	u32 val;
+
+	regmap_read(cg->regmap, cg->sta_ofs, &val);
+
+	val &= BIT(cg->bit);
+
+	return val != 0;
+}
+
+static void mtk_cg_set_bit(struct clk_hw *hw)
+{
+	struct mtk_clk_gate *cg = to_clk_gate(hw);
+
+	regmap_write(cg->regmap, cg->set_ofs, BIT(cg->bit));
+}
+
+static void mtk_cg_clr_bit(struct clk_hw *hw)
+{
+	struct mtk_clk_gate *cg = to_clk_gate(hw);
+
+	regmap_write(cg->regmap, cg->clr_ofs, BIT(cg->bit));
+}
+
+static int mtk_cg_enable(struct clk_hw *hw)
+{
+	mtk_cg_clr_bit(hw);
+
+	return 0;
+}
+
+static void mtk_cg_disable(struct clk_hw *hw)
+{
+	mtk_cg_set_bit(hw);
+}
+
+static int mtk_cg_enable_inv(struct clk_hw *hw)
+{
+	mtk_cg_set_bit(hw);
+
+	return 0;
+}
+
+static void mtk_cg_disable_inv(struct clk_hw *hw)
+{
+	mtk_cg_clr_bit(hw);
+}
+
+const struct clk_ops mtk_clk_gate_ops_setclr = {
+	.is_enabled	= mtk_cg_bit_is_cleared,
+	.enable		= mtk_cg_enable,
+	.disable	= mtk_cg_disable,
+};
+
+const struct clk_ops mtk_clk_gate_ops_setclr_inv = {
+	.is_enabled	= mtk_cg_bit_is_set,
+	.enable		= mtk_cg_enable_inv,
+	.disable	= mtk_cg_disable_inv,
+};
+
+struct clk *mtk_clk_register_gate(
+		const char *name,
+		const char *parent_name,
+		struct regmap *regmap,
+		int set_ofs,
+		int clr_ofs,
+		int sta_ofs,
+		u8 bit,
+		const struct clk_ops *ops)
+{
+	struct mtk_clk_gate *cg;
+	struct clk *clk;
+	struct clk_init_data init;
+
+	cg = kzalloc(sizeof(*cg), GFP_KERNEL);
+	if (!cg)
+		return ERR_PTR(-ENOMEM);
+
+	init.name = name;
+	init.flags = CLK_SET_RATE_PARENT;
+	init.parent_names = parent_name ? &parent_name : NULL;
+	init.num_parents = parent_name ? 1 : 0;
+	init.ops = ops;
+
+	cg->regmap = regmap;
+	cg->set_ofs = set_ofs;
+	cg->clr_ofs = clr_ofs;
+	cg->sta_ofs = sta_ofs;
+	cg->bit = bit;
+
+	cg->hw.init = &init;
+
+	clk = clk_register(NULL, &cg->hw);
+	if (IS_ERR(clk))
+		kfree(cg);
+
+	return clk;
+}
diff --git a/drivers/clk/mediatek/clk-gate.h b/drivers/clk/mediatek/clk-gate.h
new file mode 100644
index 0000000..a44dcbf
--- /dev/null
+++ b/drivers/clk/mediatek/clk-gate.h
@@ -0,0 +1,49 @@ 
+/*
+ * Copyright (c) 2014 MediaTek Inc.
+ * Author: James Liao <[e-mail]>
+ *
+ * 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 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.
+ */
+
+#ifndef __DRV_CLK_GATE_H
+#define __DRV_CLK_GATE_H
+
+/*
+ * This is a private header file. DO NOT include it except clk-*.c.
+ */
+#include <linux/regmap.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+
+struct mtk_clk_gate {
+	struct clk_hw	hw;
+	struct regmap	*regmap;
+	int		set_ofs;
+	int		clr_ofs;
+	int		sta_ofs;
+	u8		bit;
+};
+
+#define to_clk_gate(_hw) container_of(_hw, struct mtk_clk_gate, hw)
+
+extern const struct clk_ops mtk_clk_gate_ops_setclr;
+extern const struct clk_ops mtk_clk_gate_ops_setclr_inv;
+
+struct clk *mtk_clk_register_gate(
+		const char *name,
+		const char *parent_name,
+		struct regmap *regmap,
+		int set_ofs,
+		int clr_ofs,
+		int sta_ofs,
+		u8 bit,
+		const struct clk_ops *ops);
+
+#endif /* __DRV_CLK_GATE_H */
diff --git a/drivers/clk/mediatek/clk-mtk.c b/drivers/clk/mediatek/clk-mtk.c
new file mode 100644
index 0000000..479857c
--- /dev/null
+++ b/drivers/clk/mediatek/clk-mtk.c
@@ -0,0 +1,155 @@ 
+/*
+ * Copyright (c) 2014 MediaTek Inc.
+ * Author: James Liao <[e-mail]>
+ *
+ * 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 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.
+ */
+
+#include <linux/of.h>
+#include <linux/of_address.h>
+
+#include <linux/io.h>
+#include <linux/slab.h>
+#include <linux/delay.h>
+#include <linux/clkdev.h>
+
+#include "clk-mtk.h"
+#include "clk-gate.h"
+
+void mtk_init_factors(struct mtk_fixed_factor *clks, int num,
+		struct clk_onecell_data *clk_data)
+{
+	int i;
+	struct clk *clk;
+
+	for (i = 0; i < num; i++) {
+		struct mtk_fixed_factor *ff = &clks[i];
+
+		clk = clk_register_fixed_factor(NULL, ff->name, ff->parent_name,
+				CLK_SET_RATE_PARENT, ff->mult, ff->div);
+
+		if (IS_ERR(clk)) {
+			pr_err("Failed to register clk %s: %ld\n",
+					ff->name, PTR_ERR(clk));
+			continue;
+		}
+
+		if (clk_data)
+			clk_data->clks[ff->id] = clk;
+	}
+}
+
+void mtk_init_clk_gates(struct regmap *regmap,
+		struct mtk_gate *clks, int num,
+		struct clk_onecell_data *clk_data)
+{
+	int i;
+	struct clk *clk;
+
+	for (i = 0; i < num; i++) {
+		struct mtk_gate *gate = &clks[i];
+
+		clk = mtk_clk_register_gate(gate->name, gate->parent_name,
+				regmap,
+				gate->regs->set_ofs,
+				gate->regs->clr_ofs,
+				gate->regs->sta_ofs,
+				gate->shift, gate->ops);
+
+		if (IS_ERR(clk)) {
+			pr_err("Failed to register clk %s: %ld\n",
+					gate->name, PTR_ERR(clk));
+			continue;
+		}
+
+		if (clk_data)
+			clk_data->clks[gate->id] = clk;
+	}
+}
+
+struct clk_onecell_data *mtk_alloc_clk_data(unsigned int clk_num)
+{
+	int i;
+	struct clk_onecell_data *clk_data;
+
+	clk_data = kzalloc(sizeof(clk_data), GFP_KERNEL);
+	if (!clk_data)
+		return NULL;
+
+	clk_data->clks = kcalloc(clk_num, sizeof(struct clk *), GFP_KERNEL);
+	if (!clk_data->clks) {
+		kfree(clk_data);
+		return NULL;
+	}
+
+	clk_data->clk_num = clk_num;
+
+	for (i = 0; i < clk_num; ++i)
+		clk_data->clks[i] = ERR_PTR(-ENOENT);
+
+	return clk_data;
+}
+
+struct clk *mtk_clk_register_mux(
+		const char *name,
+		const char **parent_names,
+		u8 num_parents,
+		void __iomem *base_addr,
+		u8 shift,
+		u8 width,
+		u8 gate_bit,
+		spinlock_t *lock)
+{
+	struct clk *clk;
+	struct clk_mux *mux;
+	struct clk_gate *gate = NULL;
+	struct clk_hw *gate_hw = NULL;
+	const struct clk_ops *gate_ops = NULL;
+	u32 mask = BIT(width) - 1;
+
+	mux = kzalloc(sizeof(struct clk_mux), GFP_KERNEL);
+	if (!mux)
+		return ERR_PTR(-ENOMEM);
+
+	mux->reg = base_addr;
+	mux->mask = mask;
+	mux->shift = shift;
+	mux->flags = 0;
+	mux->lock = lock;
+
+	if (gate_bit <= MAX_MUX_GATE_BIT) {
+		gate = kzalloc(sizeof(struct clk_gate), GFP_KERNEL);
+		if (!gate) {
+			kfree(mux);
+			return ERR_PTR(-ENOMEM);
+		}
+
+		gate->reg = base_addr;
+		gate->bit_idx = gate_bit;
+		gate->flags = CLK_GATE_SET_TO_DISABLE;
+		gate->lock = lock;
+
+		gate_hw = &gate->hw;
+		gate_ops = &clk_gate_ops;
+	}
+
+	clk = clk_register_composite(NULL, name, parent_names, num_parents,
+		&mux->hw, &clk_mux_ops,
+		NULL, NULL,
+		gate_hw, gate_ops,
+		CLK_SET_RATE_PARENT);
+
+	if (IS_ERR(clk)) {
+		kfree(gate);
+		kfree(mux);
+	}
+
+	return clk;
+}
diff --git a/drivers/clk/mediatek/clk-mtk.h b/drivers/clk/mediatek/clk-mtk.h
new file mode 100644
index 0000000..35cf9a3
--- /dev/null
+++ b/drivers/clk/mediatek/clk-mtk.h
@@ -0,0 +1,133 @@ 
+/*
+ * Copyright (c) 2014 MediaTek Inc.
+ * Author: James Liao <[e-mail]>
+ *
+ * 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 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.
+ */
+
+#ifndef __DRV_CLK_MTK_H
+#define __DRV_CLK_MTK_H
+
+/*
+ * This is a private header file. DO NOT include it except clk-*.c.
+ */
+
+#include <linux/regmap.h>
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+
+#define MAX_MUX_GATE_BIT	31
+#define INVALID_MUX_GATE_BIT	(MAX_MUX_GATE_BIT + 1)
+
+struct mtk_fixed_factor {
+	int id;
+	const char *name;
+	const char *parent_name;
+	int mult;
+	int div;
+};
+
+#define FACTOR(_id, _name, _parent, _mult, _div) {	\
+		.id = _id,				\
+		.name = _name,				\
+		.parent_name = _parent,			\
+		.mult = _mult,				\
+		.div = _div,				\
+	}
+
+extern void mtk_init_factors(struct mtk_fixed_factor *clks, int num,
+		struct clk_onecell_data *clk_data);
+
+struct mtk_mux {
+	int id;
+	const char *name;
+	uint32_t reg;
+	int shift;
+	int width;
+	int gate;
+	const char **parent_names;
+	int num_parents;
+};
+
+#define MUX(_id, _name, _parents, _reg, _shift, _width, _gate) {	\
+		.id = _id,						\
+		.name = _name,						\
+		.reg = _reg,						\
+		.shift = _shift,					\
+		.width = _width,					\
+		.gate = _gate,						\
+		.parent_names = (const char **)_parents,		\
+		.num_parents = ARRAY_SIZE(_parents),			\
+	}
+
+struct mtk_pll {
+	int id;
+	const char *name;
+	const char *parent_name;
+	uint32_t reg;
+	uint32_t pwr_reg;
+	uint32_t en_mask;
+	unsigned int flags;
+	const struct clk_ops *ops;
+};
+
+#define PLL(_id, _name, _parent, _reg, _pwr_reg, _en_mask, _flags, _ops) { \
+		.id = _id,						\
+		.name = _name,						\
+		.parent_name = _parent,					\
+		.reg = _reg,						\
+		.pwr_reg = _pwr_reg,					\
+		.en_mask = _en_mask,					\
+		.flags = _flags,					\
+		.ops = _ops,						\
+	}
+
+struct mtk_gate_regs {
+	u32 sta_ofs;
+	u32 clr_ofs;
+	u32 set_ofs;
+};
+
+struct mtk_gate {
+	int id;
+	const char *name;
+	const char *parent_name;
+	struct mtk_gate_regs *regs;
+	int shift;
+	const struct clk_ops *ops;
+};
+
+#define GATE(_id, _name, _parent, _regs, _shift, _ops) {	\
+		.id = _id,					\
+		.name = _name,					\
+		.parent_name = _parent,				\
+		.regs = &_regs,					\
+		.shift = _shift,				\
+		.ops = _ops,					\
+	}
+
+void mtk_init_clk_gates(struct regmap *regmap,
+		struct mtk_gate *clks, int num,
+		struct clk_onecell_data *clk_data);
+
+struct clk_onecell_data *mtk_alloc_clk_data(unsigned int clk_num);
+
+struct clk *mtk_clk_register_mux(
+		const char *name,
+		const char **parent_names,
+		u8 num_parents,
+		void __iomem *base_addr,
+		u8 shift,
+		u8 width,
+		u8 gate_bit,
+		spinlock_t *lock);
+
+#endif /* __DRV_CLK_MTK_H */
diff --git a/drivers/clk/mediatek/clk-pll.c b/drivers/clk/mediatek/clk-pll.c
new file mode 100644
index 0000000..59dee83
--- /dev/null
+++ b/drivers/clk/mediatek/clk-pll.c
@@ -0,0 +1,63 @@ 
+/*
+ * Copyright (c) 2014 MediaTek Inc.
+ * Author: James Liao <[e-mail]>
+ *
+ * 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 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.
+ */
+
+#include <linux/io.h>
+#include <linux/slab.h>
+#include <linux/clkdev.h>
+
+#include "clk-mtk.h"
+#include "clk-pll.h"
+
+struct clk *mtk_clk_register_pll(
+		const char *name,
+		const char *parent_name,
+		u32 *base_addr,
+		u32 *pwr_addr,
+		u32 en_mask,
+		u32 flags,
+		const struct clk_ops *ops,
+		spinlock_t *lock)
+{
+	struct mtk_clk_pll *pll;
+	struct clk_init_data init;
+	struct clk *clk;
+
+	pr_debug("%s(): name: %s\n", __func__, name);
+
+	if (!lock)
+		return ERR_PTR(-EINVAL);
+
+	pll = kzalloc(sizeof(*pll), GFP_KERNEL);
+	if (!pll)
+		return ERR_PTR(-ENOMEM);
+
+	pll->base_addr = base_addr;
+	pll->pwr_addr = pwr_addr;
+	pll->en_mask = en_mask;
+	pll->flags = flags;
+	pll->lock = lock;
+	pll->hw.init = &init;
+
+	init.name = name;
+	init.ops = ops;
+	init.parent_names = &parent_name;
+	init.num_parents = 1;
+
+	clk = clk_register(NULL, &pll->hw);
+
+	if (IS_ERR(clk))
+		kfree(pll);
+
+	return clk;
+}
diff --git a/drivers/clk/mediatek/clk-pll.h b/drivers/clk/mediatek/clk-pll.h
new file mode 100644
index 0000000..341d2fe
--- /dev/null
+++ b/drivers/clk/mediatek/clk-pll.h
@@ -0,0 +1,52 @@ 
+/*
+ * Copyright (c) 2014 MediaTek Inc.
+ * Author: James Liao <[e-mail]>
+ *
+ * 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 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.
+ */
+
+#ifndef __DRV_CLK_PLL_H
+#define __DRV_CLK_PLL_H
+
+/*
+ * This is a private header file. DO NOT include it except clk-*.c.
+ */
+
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+
+struct mtk_clk_pll {
+	struct clk_hw	hw;
+	void __iomem	*base_addr;
+	void __iomem	*pwr_addr;
+	u32		en_mask;
+	u32		flags;
+	spinlock_t	*lock;
+};
+
+#define to_mtk_clk_pll(_hw) container_of(_hw, struct mtk_clk_pll, hw)
+
+#define HAVE_RST_BAR	BIT(0)
+#define HAVE_PLL_HP	BIT(1)
+#define HAVE_FIX_FRQ	BIT(2)
+#define PLL_AO		BIT(3)
+
+struct clk *mtk_clk_register_pll(
+		const char *name,
+		const char *parent_name,
+		u32 *base_addr,
+		u32 *pwr_addr,
+		u32 en_mask,
+		u32 flags,
+		const struct clk_ops *ops,
+		spinlock_t *lock);
+
+#endif /* __DRV_CLK_PLL_H */