From: Weiyi Lu <weiyi.lu@mediatek.com>
To: Nicolas Boichat <drinkcat@chromium.org>
Cc: Rob Herring <robh@kernel.org>, <srv_heupstream@mediatek.com>,
<jamesjj.liao@mediatek.com>, <sboyd@codeaurora.org>,
lkml <linux-kernel@vger.kernel.org>, <fan.chen@mediatek.com>,
<linux-mediatek@lists.infradead.org>,
Matthias Brugger <matthias.bgg@gmail.com>,
<owen.chen@mediatek.com>, <linux-clk@vger.kernel.org>,
linux-arm Mailing List <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v1 01/11] clk: mediatek: add new clkmux register API
Date: Tue, 20 Nov 2018 14:34:57 +0800 [thread overview]
Message-ID: <1542695697.5132.24.camel@mtksdaap41> (raw)
In-Reply-To: <CANMq1KDEg-9WzfaRB-n4Ab0PxjqfZL_aeTAXD336N6VzX8oFhQ@mail.gmail.com>
On Tue, 2018-11-13 at 08:00 -0800, Nicolas Boichat wrote:
> On Mon, Nov 5, 2018 at 10:42 PM Weiyi Lu <weiyi.lu@mediatek.com> wrote:
> >
> > From: Owen Chen <owen.chen@mediatek.com>
> >
> > On both MT8183 & MT6765, there add "set/clr" register for
> > each clkmux setting, and one update register to trigger value change.
> > It is designed to prevent read-modify-write racing issue.
> > The sw design need to add a new API to handle this hw change with
> > a new mtk_clk_mux/mtk_mux struct in new file "clk-mux.c", "clk-mux.h".
> >
> > Signed-off-by: Owen Chen <owen.chen@mediatek.com>
> > Signed-off-by: Weiyi Lu <weiyi.lu@mediatek.com>
> > ---
> > drivers/clk/mediatek/Makefile | 2 +-
> > drivers/clk/mediatek/clk-mux.c | 252 +++++++++++++++++++++++++++++++++
> > drivers/clk/mediatek/clk-mux.h | 101 +++++++++++++
> > 3 files changed, 354 insertions(+), 1 deletion(-)
> > create mode 100644 drivers/clk/mediatek/clk-mux.c
> > create mode 100644 drivers/clk/mediatek/clk-mux.h
> >
> > diff --git a/drivers/clk/mediatek/Makefile b/drivers/clk/mediatek/Makefile
> > index 844b55d2770d..b97980dbb738 100644
> > --- a/drivers/clk/mediatek/Makefile
> > +++ b/drivers/clk/mediatek/Makefile
> > @@ -1,5 +1,5 @@
> > # SPDX-License-Identifier: GPL-2.0
> > -obj-$(CONFIG_COMMON_CLK_MEDIATEK) += clk-mtk.o clk-pll.o clk-gate.o clk-apmixed.o clk-cpumux.o reset.o
> > +obj-$(CONFIG_COMMON_CLK_MEDIATEK) += clk-mtk.o clk-pll.o clk-gate.o clk-apmixed.o clk-cpumux.o reset.o clk-mux.o
> > obj-$(CONFIG_COMMON_CLK_MT6797) += clk-mt6797.o
> > obj-$(CONFIG_COMMON_CLK_MT6797_IMGSYS) += clk-mt6797-img.o
> > obj-$(CONFIG_COMMON_CLK_MT6797_MMSYS) += clk-mt6797-mm.o
> > diff --git a/drivers/clk/mediatek/clk-mux.c b/drivers/clk/mediatek/clk-mux.c
> > new file mode 100644
> > index 000000000000..18bc9a146be0
> > --- /dev/null
> > +++ b/drivers/clk/mediatek/clk-mux.c
> > @@ -0,0 +1,252 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2018 MediaTek Inc.
> > + * Author: Owen Chen <owen.chen@mediatek.com>
> > + */
> > +
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/slab.h>
> > +#include <linux/mfd/syscon.h>
> > +
> > +#include "clk-mtk.h"
> > +#include "clk-mux.h"
> > +
> > +static inline struct mtk_clk_mux
> > + *to_mtk_clk_mux(struct clk_hw *hw)
> > +{
> > + return container_of(hw, struct mtk_clk_mux, hw);
> > +}
> > +
> > +static int mtk_clk_mux_enable(struct clk_hw *hw)
> > +{
> > + struct mtk_clk_mux *mux = to_mtk_clk_mux(hw);
> > + u32 mask = BIT(mux->gate_shift);
> > +
> > + regmap_update_bits(mux->regmap, mux->mux_ofs, mask, 0);
> > +
> > + return 0;
>
> Might as well return regmap_update_bits(...).
>
OK, will fix in next version
> > +}
> > +
> > +static void mtk_clk_mux_disable(struct clk_hw *hw)
> > +{
> > + struct mtk_clk_mux *mux = to_mtk_clk_mux(hw);
> > + u32 mask = BIT(mux->gate_shift);
> > +
> > + regmap_update_bits(mux->regmap, mux->mux_ofs, mask, mask);
> > +}
> > +
> > +static int mtk_clk_mux_enable_setclr(struct clk_hw *hw)
> > +{
> > + struct mtk_clk_mux *mux = to_mtk_clk_mux(hw);
> > + u32 val;
> > +
> > + val = BIT(mux->gate_shift);
> > + regmap_write(mux->regmap, mux->mux_clr_ofs, val);
> > +
> > + return 0;
>
> ditto: return regmap_write(...) .
>
will fix in next version
> > +}
> > +
> > +static void mtk_clk_mux_disable_setclr(struct clk_hw *hw)
> > +{
> > + struct mtk_clk_mux *mux = to_mtk_clk_mux(hw);
> > + u32 val;
> > +
> > + val = BIT(mux->gate_shift);
> > + regmap_write(mux->regmap, mux->mux_set_ofs, val);
> > +}
> > +
> > +static int mtk_clk_mux_is_enabled(struct clk_hw *hw)
> > +{
> > + struct mtk_clk_mux *mux = to_mtk_clk_mux(hw);
> > + u32 val = 0;
>
> No need to init to 0.
>
will fix in next version
> > +
> > + regmap_read(mux->regmap, mux->mux_ofs, &val);
> > +
> > + return ((val & BIT(mux->gate_shift)) == 0) ? 1 : 0;
>
> Just (val & BIT(mux->gate_shift)) != 0. Or more simply val &
> BIT(mux->gate_shift).
>
will fix in next version
> > +}
> > +
> > +static u8 mtk_clk_mux_get_parent(struct clk_hw *hw)
> > +{
> > + struct mtk_clk_mux *mux = to_mtk_clk_mux(hw);
> > + int num_parents = clk_hw_get_num_parents(hw);
> > + u32 mask = GENMASK(mux->mux_width - 1, 0);
> > + u32 val;
> > +
> > + regmap_read(mux->regmap, mux->mux_ofs, &val);
> > + val = (val >> mux->mux_shift) & mask;
> > +
> > + if (val >= num_parents || val >= U8_MAX)
>
> val > U8_MAX, technically.
>
> > + return -ERANGE;
>
> This looks wrong, -34 will be cast to an u8 and appear to be valid...
>
will fix in next version
> > +
> > + return val;
> > +}
> > +
> > +static int mtk_clk_mux_set_parent_lock(struct clk_hw *hw, u8 index)
> > +{
> > + struct mtk_clk_mux *mux = to_mtk_clk_mux(hw);
> > + u32 mask = GENMASK(mux->mux_width - 1, 0);
> > + u32 val;
> > + unsigned long flags = 0;
>
> No need to init to 0.
>
will fix in next version.
> > +
> > + if (mux->lock)
> > + spin_lock_irqsave(mux->lock, flags);
> > + else
> > + __acquire(mux->lock);
> > +
> > + regmap_read(mux->regmap, mux->mux_ofs, &val);
> > + val = (val & mask) >> mux->mux_shift;
> > +
> > + if (val != index) {
> > + val = index << mux->mux_shift;
> > + regmap_update_bits(mux->regmap, mux->mux_ofs, mask, val);
> > + }
>
> What's the point of doing read/compare/update? Are there side effects
> if you just write the value unconditionally?
>
Actually writing the value directly has no side effects. We just want to
reduce unnecessary operations if the target parent of mux is already
selected.
> > +
> > + if (mux->lock)
> > + spin_unlock_irqrestore(mux->lock, flags);
> > + else
> > + __release(mux->lock);
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_clk_mux_set_parent_setclr_lock(struct clk_hw *hw, u8 index)
> > +{
> > + struct mtk_clk_mux *mux = to_mtk_clk_mux(hw);
> > + u32 mask = GENMASK(mux->mux_width - 1, 0);
> > + u32 val, orig;
> > + unsigned long flags = 0;
> > +
> > + if (mux->lock)
> > + spin_lock_irqsave(mux->lock, flags);
> > + else
> > + __acquire(mux->lock);
> > +
> > + regmap_read(mux->regmap, mux->mux_ofs, &val);
> > + orig = val;
>
> Just read into &orig above.
>
will fix in next version
> > + val &= ~(mask << mux->mux_shift);
> > + val |= index << mux->mux_shift;
>
> Maybe this is more readable?
> val = orig & ~(mask << mux->mux_shift) | (index << mux->mux_shift)
>
will fix in next version
> > +
> > + if (val != orig) {
> > + val = (mask << mux->mux_shift);
> > + regmap_write(mux->regmap, mux->mux_clr_ofs, val);
>
> I'd just regmap_write(..., mask << mux->mux_shift), instead if overriding val.
>
will fix in next version
> > + val = (index << mux->mux_shift);
> > + regmap_write(mux->regmap, mux->mux_set_ofs, val);
> > +
> > + if (mux->upd_shift >= 0)
> > + regmap_write(mux->regmap, mux->upd_ofs,
> > + BIT(mux->upd_shift));
> > + }
> > +
> > + if (mux->lock)
> > + spin_unlock_irqrestore(mux->lock, flags);
> > + else
> > + __release(mux->lock);
> > +
> > + return 0;
> > +}
> > +
> > +const struct clk_ops mtk_mux_ops = {
> > + .get_parent = mtk_clk_mux_get_parent,
> > + .set_parent = mtk_clk_mux_set_parent_lock,
> > +};
> > +
> > +const struct clk_ops mtk_mux_clr_set_upd_ops = {
> > + .get_parent = mtk_clk_mux_get_parent,
> > + .set_parent = mtk_clk_mux_set_parent_setclr_lock,
> > +};
> > +
> > +const struct clk_ops mtk_mux_gate_ops = {
> > + .enable = mtk_clk_mux_enable,
> > + .disable = mtk_clk_mux_disable,
> > + .is_enabled = mtk_clk_mux_is_enabled,
> > + .get_parent = mtk_clk_mux_get_parent,
> > + .set_parent = mtk_clk_mux_set_parent_lock,
> > +};
> > +
> > +const struct clk_ops mtk_mux_gate_clr_set_upd_ops = {
> > + .enable = mtk_clk_mux_enable_setclr,
> > + .disable = mtk_clk_mux_disable_setclr,
> > + .is_enabled = mtk_clk_mux_is_enabled,
> > + .get_parent = mtk_clk_mux_get_parent,
> > + .set_parent = mtk_clk_mux_set_parent_setclr_lock,
> > +};
> > +
> > +struct clk *mtk_clk_register_mux(const struct mtk_mux *mux,
> > + struct regmap *regmap,
> > + spinlock_t *lock)
> > +{
> > + struct mtk_clk_mux *mtk_mux = NULL;
>
> Not necessary to init to NULL.
>
will fix in next version
> > + struct clk_init_data init;
> > + struct clk *clk;
> > +
> > + mtk_mux = kzalloc(sizeof(*mtk_mux), GFP_KERNEL);
> > + if (!mtk_mux)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + init.name = mux->name;
> > + init.flags = (mux->flags) | CLK_SET_RATE_PARENT;
>
> mux->flags | CLK_SET_RATE_PARENT
>
will fix in next version
> > + init.parent_names = mux->parent_names;
> > + init.num_parents = mux->num_parents;
> > + init.ops = mux->ops;
> > +
> > + mtk_mux->regmap = regmap;
> > + mtk_mux->name = mux->name;
> > + mtk_mux->mux_ofs = mux->mux_ofs;
> > + mtk_mux->mux_set_ofs = mux->set_ofs;
> > + mtk_mux->mux_clr_ofs = mux->clr_ofs;
> > + mtk_mux->upd_ofs = mux->upd_ofs;
> > + mtk_mux->mux_shift = mux->mux_shift;
> > + mtk_mux->mux_width = mux->mux_width;
> > + mtk_mux->gate_shift = mux->gate_shift;
> > + mtk_mux->upd_shift = mux->upd_shift;
> > +
> > + mtk_mux->lock = lock;
> > + mtk_mux->hw.init = &init;
> > +
> > + clk = clk_register(NULL, &mtk_mux->hw);
> > + if (IS_ERR(clk)) {
> > + kfree(mtk_mux);
> > + return clk;
> > + }
> > +
> > + return clk;
> > +}
> > +
> > +int mtk_clk_register_muxes(const struct mtk_mux *muxes,
> > + int num, struct device_node *node,
> > + spinlock_t *lock,
> > + struct clk_onecell_data *clk_data)
> > +{
> > + struct regmap *regmap;
> > + struct clk *clk;
> > + int i;
> > +
> > + if (!clk_data)
> > + return -ENOMEM;
>
> Is this check necessary? After all you know who the callers are and
> maybe you need to be careful not to call this with NULL clk_data?
>
You're right. it's unnecessary. will fix in next version
> > +
> > + regmap = syscon_node_to_regmap(node);
> > + if (IS_ERR(regmap)) {
> > + pr_err("Cannot find regmap for %pOF: %ld\n", node,
> > + PTR_ERR(regmap));
> > + return PTR_ERR(regmap);
> > + }
> > +
> > + for (i = 0; i < num; i++) {
> > + const struct mtk_mux *mux = &muxes[i];
> > +
> > + if (IS_ERR_OR_NULL(clk_data->clks[mux->id])) {
>
> A NULL check seems enough, as you'll never assign a PTR_ERR(..) value
> into clk_data->clks[mux->id].
>
> > + clk = mtk_clk_register_mux(mux, regmap, lock);
> > +
> > + if (IS_ERR(clk)) {
> > + pr_err("Failed to register clk %s: %ld\n",
> > + mux->name, PTR_ERR(clk));
> > + continue;
>
> Are we ok with returning 0 even though some of these calls failed?
>
We'd like to skip the failed one first and keep registering the
remainings.
> > + }
> > +
> > + clk_data->clks[mux->id] = clk;
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > diff --git a/drivers/clk/mediatek/clk-mux.h b/drivers/clk/mediatek/clk-mux.h
> > new file mode 100644
> > index 000000000000..ff0276bb771c
> > --- /dev/null
> > +++ b/drivers/clk/mediatek/clk-mux.h
> > @@ -0,0 +1,101 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (c) 2018 MediaTek Inc.
> > + * Author: Owen Chen <owen.chen@mediatek.com>
> > + */
> > +
> > +#ifndef __DRV_CLK_MUX_H
> > +#define __DRV_CLK_MUX_H
> > +
> > +#include <linux/clk-provider.h>
> > +
> > +struct mtk_clk_mux {
> > + struct clk_hw hw;
> > + struct regmap *regmap;
> > +
> > + const char *name;
> > +
> > + u32 mux_set_ofs;
> > + u32 mux_clr_ofs;
> > + u32 mux_ofs;
> > + u32 upd_ofs;
> > +
> > + u8 mux_shift;
> > + u8 mux_width;
> > + u8 gate_shift;
> > + u8 upd_shift;
> > +
> > + spinlock_t *lock;
> > +};
> > +
> > +struct mtk_mux {
> > + int id;
> > + const char *name;
> > + const char * const *parent_names;
> > + unsigned int flags;
> > +
> > + u32 mux_ofs;
> > + u32 set_ofs;
> > + u32 clr_ofs;
> > + u32 upd_ofs;
> > +
> > + u8 mux_shift;
> > + u8 mux_width;
> > + u8 gate_shift;
> > + u8 upd_shift;
> > +
> > + const struct clk_ops *ops;
> > +
> > + signed char num_parents;
> > +};
> > +
> > +extern const struct clk_ops mtk_mux_ops;
> > +extern const struct clk_ops mtk_mux_clr_set_upd_ops;
> > +extern const struct clk_ops mtk_mux_gate_ops;
> > +extern const struct clk_ops mtk_mux_gate_clr_set_upd_ops;
> > +
> > +#define GATE_CLR_SET_UPD_FLAGS(_id, _name, _parents, _mux_ofs, \
> > + _mux_set_ofs, _mux_clr_ofs, _shift, _width, \
> > + _gate, _upd_ofs, _upd, _flags, _ops) { \
> > + .id = _id, \
> > + .name = _name, \
> > + .mux_ofs = _mux_ofs, \
> > + .set_ofs = _mux_set_ofs, \
> > + .clr_ofs = _mux_clr_ofs, \
> > + .upd_ofs = _upd_ofs, \
> > + .mux_shift = _shift, \
> > + .mux_width = _width, \
> > + .gate_shift = _gate, \
> > + .upd_shift = _upd, \
> > + .parent_names = _parents, \
> > + .num_parents = ARRAY_SIZE(_parents), \
> > + .flags = _flags, \
> > + .ops = &_ops, \
> > + }
> > +
> > +#define MUX_GATE_CLR_SET_UPD_FLAGS(_id, _name, _parents, _mux_ofs, \
> > + _mux_set_ofs, _mux_clr_ofs, _shift, _width, \
> > + _gate, _upd_ofs, _upd, _flags) \
> > + GATE_CLR_SET_UPD_FLAGS(_id, _name, _parents, _mux_ofs, \
> > + _mux_set_ofs, _mux_clr_ofs, _shift, _width, \
> > + _gate, _upd_ofs, _upd, _flags, \
> > + mtk_mux_gate_clr_set_upd_ops)
> > +
> > +#define MUX_GATE_CLR_SET_UPD(_id, _name, _parents, _mux_ofs, \
> > + _mux_set_ofs, _mux_clr_ofs, _shift, _width, \
> > + _gate, _upd_ofs, _upd) \
> > + MUX_GATE_CLR_SET_UPD_FLAGS(_id, _name, _parents, \
> > + _mux_ofs, _mux_set_ofs, _mux_clr_ofs, _shift, \
> > + _width, _gate, _upd_ofs, _upd, \
> > + CLK_SET_RATE_PARENT)
> > +
> > +struct clk *mtk_clk_register_mux(const struct mtk_mux *mux,
> > + struct regmap *regmap,
> > + spinlock_t *lock);
> > +
> > +int mtk_clk_register_muxes(const struct mtk_mux *muxes,
> > + int num, struct device_node *node,
> > + spinlock_t *lock,
> > + struct clk_onecell_data *clk_data);
> > +
> > +#endif /* __DRV_CLK_MUX_H */
> > --
> > 2.18.0
> >
>
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek
WARNING: multiple messages have this Message-ID (diff)
From: Weiyi Lu <weiyi.lu@mediatek.com>
To: Nicolas Boichat <drinkcat@chromium.org>
Cc: Rob Herring <robh@kernel.org>,
srv_heupstream@mediatek.com, jamesjj.liao@mediatek.com,
sboyd@codeaurora.org, lkml <linux-kernel@vger.kernel.org>,
fan.chen@mediatek.com, linux-mediatek@lists.infradead.org,
Matthias Brugger <matthias.bgg@gmail.com>,
owen.chen@mediatek.com, linux-clk@vger.kernel.org,
linux-arm Mailing List <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v1 01/11] clk: mediatek: add new clkmux register API
Date: Tue, 20 Nov 2018 14:34:57 +0800 [thread overview]
Message-ID: <1542695697.5132.24.camel@mtksdaap41> (raw)
In-Reply-To: <CANMq1KDEg-9WzfaRB-n4Ab0PxjqfZL_aeTAXD336N6VzX8oFhQ@mail.gmail.com>
On Tue, 2018-11-13 at 08:00 -0800, Nicolas Boichat wrote:
> On Mon, Nov 5, 2018 at 10:42 PM Weiyi Lu <weiyi.lu@mediatek.com> wrote:
> >
> > From: Owen Chen <owen.chen@mediatek.com>
> >
> > On both MT8183 & MT6765, there add "set/clr" register for
> > each clkmux setting, and one update register to trigger value change.
> > It is designed to prevent read-modify-write racing issue.
> > The sw design need to add a new API to handle this hw change with
> > a new mtk_clk_mux/mtk_mux struct in new file "clk-mux.c", "clk-mux.h".
> >
> > Signed-off-by: Owen Chen <owen.chen@mediatek.com>
> > Signed-off-by: Weiyi Lu <weiyi.lu@mediatek.com>
> > ---
> > drivers/clk/mediatek/Makefile | 2 +-
> > drivers/clk/mediatek/clk-mux.c | 252 +++++++++++++++++++++++++++++++++
> > drivers/clk/mediatek/clk-mux.h | 101 +++++++++++++
> > 3 files changed, 354 insertions(+), 1 deletion(-)
> > create mode 100644 drivers/clk/mediatek/clk-mux.c
> > create mode 100644 drivers/clk/mediatek/clk-mux.h
> >
> > diff --git a/drivers/clk/mediatek/Makefile b/drivers/clk/mediatek/Makefile
> > index 844b55d2770d..b97980dbb738 100644
> > --- a/drivers/clk/mediatek/Makefile
> > +++ b/drivers/clk/mediatek/Makefile
> > @@ -1,5 +1,5 @@
> > # SPDX-License-Identifier: GPL-2.0
> > -obj-$(CONFIG_COMMON_CLK_MEDIATEK) += clk-mtk.o clk-pll.o clk-gate.o clk-apmixed.o clk-cpumux.o reset.o
> > +obj-$(CONFIG_COMMON_CLK_MEDIATEK) += clk-mtk.o clk-pll.o clk-gate.o clk-apmixed.o clk-cpumux.o reset.o clk-mux.o
> > obj-$(CONFIG_COMMON_CLK_MT6797) += clk-mt6797.o
> > obj-$(CONFIG_COMMON_CLK_MT6797_IMGSYS) += clk-mt6797-img.o
> > obj-$(CONFIG_COMMON_CLK_MT6797_MMSYS) += clk-mt6797-mm.o
> > diff --git a/drivers/clk/mediatek/clk-mux.c b/drivers/clk/mediatek/clk-mux.c
> > new file mode 100644
> > index 000000000000..18bc9a146be0
> > --- /dev/null
> > +++ b/drivers/clk/mediatek/clk-mux.c
> > @@ -0,0 +1,252 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2018 MediaTek Inc.
> > + * Author: Owen Chen <owen.chen@mediatek.com>
> > + */
> > +
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/slab.h>
> > +#include <linux/mfd/syscon.h>
> > +
> > +#include "clk-mtk.h"
> > +#include "clk-mux.h"
> > +
> > +static inline struct mtk_clk_mux
> > + *to_mtk_clk_mux(struct clk_hw *hw)
> > +{
> > + return container_of(hw, struct mtk_clk_mux, hw);
> > +}
> > +
> > +static int mtk_clk_mux_enable(struct clk_hw *hw)
> > +{
> > + struct mtk_clk_mux *mux = to_mtk_clk_mux(hw);
> > + u32 mask = BIT(mux->gate_shift);
> > +
> > + regmap_update_bits(mux->regmap, mux->mux_ofs, mask, 0);
> > +
> > + return 0;
>
> Might as well return regmap_update_bits(...).
>
OK, will fix in next version
> > +}
> > +
> > +static void mtk_clk_mux_disable(struct clk_hw *hw)
> > +{
> > + struct mtk_clk_mux *mux = to_mtk_clk_mux(hw);
> > + u32 mask = BIT(mux->gate_shift);
> > +
> > + regmap_update_bits(mux->regmap, mux->mux_ofs, mask, mask);
> > +}
> > +
> > +static int mtk_clk_mux_enable_setclr(struct clk_hw *hw)
> > +{
> > + struct mtk_clk_mux *mux = to_mtk_clk_mux(hw);
> > + u32 val;
> > +
> > + val = BIT(mux->gate_shift);
> > + regmap_write(mux->regmap, mux->mux_clr_ofs, val);
> > +
> > + return 0;
>
> ditto: return regmap_write(...) .
>
will fix in next version
> > +}
> > +
> > +static void mtk_clk_mux_disable_setclr(struct clk_hw *hw)
> > +{
> > + struct mtk_clk_mux *mux = to_mtk_clk_mux(hw);
> > + u32 val;
> > +
> > + val = BIT(mux->gate_shift);
> > + regmap_write(mux->regmap, mux->mux_set_ofs, val);
> > +}
> > +
> > +static int mtk_clk_mux_is_enabled(struct clk_hw *hw)
> > +{
> > + struct mtk_clk_mux *mux = to_mtk_clk_mux(hw);
> > + u32 val = 0;
>
> No need to init to 0.
>
will fix in next version
> > +
> > + regmap_read(mux->regmap, mux->mux_ofs, &val);
> > +
> > + return ((val & BIT(mux->gate_shift)) == 0) ? 1 : 0;
>
> Just (val & BIT(mux->gate_shift)) != 0. Or more simply val &
> BIT(mux->gate_shift).
>
will fix in next version
> > +}
> > +
> > +static u8 mtk_clk_mux_get_parent(struct clk_hw *hw)
> > +{
> > + struct mtk_clk_mux *mux = to_mtk_clk_mux(hw);
> > + int num_parents = clk_hw_get_num_parents(hw);
> > + u32 mask = GENMASK(mux->mux_width - 1, 0);
> > + u32 val;
> > +
> > + regmap_read(mux->regmap, mux->mux_ofs, &val);
> > + val = (val >> mux->mux_shift) & mask;
> > +
> > + if (val >= num_parents || val >= U8_MAX)
>
> val > U8_MAX, technically.
>
> > + return -ERANGE;
>
> This looks wrong, -34 will be cast to an u8 and appear to be valid...
>
will fix in next version
> > +
> > + return val;
> > +}
> > +
> > +static int mtk_clk_mux_set_parent_lock(struct clk_hw *hw, u8 index)
> > +{
> > + struct mtk_clk_mux *mux = to_mtk_clk_mux(hw);
> > + u32 mask = GENMASK(mux->mux_width - 1, 0);
> > + u32 val;
> > + unsigned long flags = 0;
>
> No need to init to 0.
>
will fix in next version.
> > +
> > + if (mux->lock)
> > + spin_lock_irqsave(mux->lock, flags);
> > + else
> > + __acquire(mux->lock);
> > +
> > + regmap_read(mux->regmap, mux->mux_ofs, &val);
> > + val = (val & mask) >> mux->mux_shift;
> > +
> > + if (val != index) {
> > + val = index << mux->mux_shift;
> > + regmap_update_bits(mux->regmap, mux->mux_ofs, mask, val);
> > + }
>
> What's the point of doing read/compare/update? Are there side effects
> if you just write the value unconditionally?
>
Actually writing the value directly has no side effects. We just want to
reduce unnecessary operations if the target parent of mux is already
selected.
> > +
> > + if (mux->lock)
> > + spin_unlock_irqrestore(mux->lock, flags);
> > + else
> > + __release(mux->lock);
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_clk_mux_set_parent_setclr_lock(struct clk_hw *hw, u8 index)
> > +{
> > + struct mtk_clk_mux *mux = to_mtk_clk_mux(hw);
> > + u32 mask = GENMASK(mux->mux_width - 1, 0);
> > + u32 val, orig;
> > + unsigned long flags = 0;
> > +
> > + if (mux->lock)
> > + spin_lock_irqsave(mux->lock, flags);
> > + else
> > + __acquire(mux->lock);
> > +
> > + regmap_read(mux->regmap, mux->mux_ofs, &val);
> > + orig = val;
>
> Just read into &orig above.
>
will fix in next version
> > + val &= ~(mask << mux->mux_shift);
> > + val |= index << mux->mux_shift;
>
> Maybe this is more readable?
> val = orig & ~(mask << mux->mux_shift) | (index << mux->mux_shift)
>
will fix in next version
> > +
> > + if (val != orig) {
> > + val = (mask << mux->mux_shift);
> > + regmap_write(mux->regmap, mux->mux_clr_ofs, val);
>
> I'd just regmap_write(..., mask << mux->mux_shift), instead if overriding val.
>
will fix in next version
> > + val = (index << mux->mux_shift);
> > + regmap_write(mux->regmap, mux->mux_set_ofs, val);
> > +
> > + if (mux->upd_shift >= 0)
> > + regmap_write(mux->regmap, mux->upd_ofs,
> > + BIT(mux->upd_shift));
> > + }
> > +
> > + if (mux->lock)
> > + spin_unlock_irqrestore(mux->lock, flags);
> > + else
> > + __release(mux->lock);
> > +
> > + return 0;
> > +}
> > +
> > +const struct clk_ops mtk_mux_ops = {
> > + .get_parent = mtk_clk_mux_get_parent,
> > + .set_parent = mtk_clk_mux_set_parent_lock,
> > +};
> > +
> > +const struct clk_ops mtk_mux_clr_set_upd_ops = {
> > + .get_parent = mtk_clk_mux_get_parent,
> > + .set_parent = mtk_clk_mux_set_parent_setclr_lock,
> > +};
> > +
> > +const struct clk_ops mtk_mux_gate_ops = {
> > + .enable = mtk_clk_mux_enable,
> > + .disable = mtk_clk_mux_disable,
> > + .is_enabled = mtk_clk_mux_is_enabled,
> > + .get_parent = mtk_clk_mux_get_parent,
> > + .set_parent = mtk_clk_mux_set_parent_lock,
> > +};
> > +
> > +const struct clk_ops mtk_mux_gate_clr_set_upd_ops = {
> > + .enable = mtk_clk_mux_enable_setclr,
> > + .disable = mtk_clk_mux_disable_setclr,
> > + .is_enabled = mtk_clk_mux_is_enabled,
> > + .get_parent = mtk_clk_mux_get_parent,
> > + .set_parent = mtk_clk_mux_set_parent_setclr_lock,
> > +};
> > +
> > +struct clk *mtk_clk_register_mux(const struct mtk_mux *mux,
> > + struct regmap *regmap,
> > + spinlock_t *lock)
> > +{
> > + struct mtk_clk_mux *mtk_mux = NULL;
>
> Not necessary to init to NULL.
>
will fix in next version
> > + struct clk_init_data init;
> > + struct clk *clk;
> > +
> > + mtk_mux = kzalloc(sizeof(*mtk_mux), GFP_KERNEL);
> > + if (!mtk_mux)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + init.name = mux->name;
> > + init.flags = (mux->flags) | CLK_SET_RATE_PARENT;
>
> mux->flags | CLK_SET_RATE_PARENT
>
will fix in next version
> > + init.parent_names = mux->parent_names;
> > + init.num_parents = mux->num_parents;
> > + init.ops = mux->ops;
> > +
> > + mtk_mux->regmap = regmap;
> > + mtk_mux->name = mux->name;
> > + mtk_mux->mux_ofs = mux->mux_ofs;
> > + mtk_mux->mux_set_ofs = mux->set_ofs;
> > + mtk_mux->mux_clr_ofs = mux->clr_ofs;
> > + mtk_mux->upd_ofs = mux->upd_ofs;
> > + mtk_mux->mux_shift = mux->mux_shift;
> > + mtk_mux->mux_width = mux->mux_width;
> > + mtk_mux->gate_shift = mux->gate_shift;
> > + mtk_mux->upd_shift = mux->upd_shift;
> > +
> > + mtk_mux->lock = lock;
> > + mtk_mux->hw.init = &init;
> > +
> > + clk = clk_register(NULL, &mtk_mux->hw);
> > + if (IS_ERR(clk)) {
> > + kfree(mtk_mux);
> > + return clk;
> > + }
> > +
> > + return clk;
> > +}
> > +
> > +int mtk_clk_register_muxes(const struct mtk_mux *muxes,
> > + int num, struct device_node *node,
> > + spinlock_t *lock,
> > + struct clk_onecell_data *clk_data)
> > +{
> > + struct regmap *regmap;
> > + struct clk *clk;
> > + int i;
> > +
> > + if (!clk_data)
> > + return -ENOMEM;
>
> Is this check necessary? After all you know who the callers are and
> maybe you need to be careful not to call this with NULL clk_data?
>
You're right. it's unnecessary. will fix in next version
> > +
> > + regmap = syscon_node_to_regmap(node);
> > + if (IS_ERR(regmap)) {
> > + pr_err("Cannot find regmap for %pOF: %ld\n", node,
> > + PTR_ERR(regmap));
> > + return PTR_ERR(regmap);
> > + }
> > +
> > + for (i = 0; i < num; i++) {
> > + const struct mtk_mux *mux = &muxes[i];
> > +
> > + if (IS_ERR_OR_NULL(clk_data->clks[mux->id])) {
>
> A NULL check seems enough, as you'll never assign a PTR_ERR(..) value
> into clk_data->clks[mux->id].
>
> > + clk = mtk_clk_register_mux(mux, regmap, lock);
> > +
> > + if (IS_ERR(clk)) {
> > + pr_err("Failed to register clk %s: %ld\n",
> > + mux->name, PTR_ERR(clk));
> > + continue;
>
> Are we ok with returning 0 even though some of these calls failed?
>
We'd like to skip the failed one first and keep registering the
remainings.
> > + }
> > +
> > + clk_data->clks[mux->id] = clk;
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > diff --git a/drivers/clk/mediatek/clk-mux.h b/drivers/clk/mediatek/clk-mux.h
> > new file mode 100644
> > index 000000000000..ff0276bb771c
> > --- /dev/null
> > +++ b/drivers/clk/mediatek/clk-mux.h
> > @@ -0,0 +1,101 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (c) 2018 MediaTek Inc.
> > + * Author: Owen Chen <owen.chen@mediatek.com>
> > + */
> > +
> > +#ifndef __DRV_CLK_MUX_H
> > +#define __DRV_CLK_MUX_H
> > +
> > +#include <linux/clk-provider.h>
> > +
> > +struct mtk_clk_mux {
> > + struct clk_hw hw;
> > + struct regmap *regmap;
> > +
> > + const char *name;
> > +
> > + u32 mux_set_ofs;
> > + u32 mux_clr_ofs;
> > + u32 mux_ofs;
> > + u32 upd_ofs;
> > +
> > + u8 mux_shift;
> > + u8 mux_width;
> > + u8 gate_shift;
> > + u8 upd_shift;
> > +
> > + spinlock_t *lock;
> > +};
> > +
> > +struct mtk_mux {
> > + int id;
> > + const char *name;
> > + const char * const *parent_names;
> > + unsigned int flags;
> > +
> > + u32 mux_ofs;
> > + u32 set_ofs;
> > + u32 clr_ofs;
> > + u32 upd_ofs;
> > +
> > + u8 mux_shift;
> > + u8 mux_width;
> > + u8 gate_shift;
> > + u8 upd_shift;
> > +
> > + const struct clk_ops *ops;
> > +
> > + signed char num_parents;
> > +};
> > +
> > +extern const struct clk_ops mtk_mux_ops;
> > +extern const struct clk_ops mtk_mux_clr_set_upd_ops;
> > +extern const struct clk_ops mtk_mux_gate_ops;
> > +extern const struct clk_ops mtk_mux_gate_clr_set_upd_ops;
> > +
> > +#define GATE_CLR_SET_UPD_FLAGS(_id, _name, _parents, _mux_ofs, \
> > + _mux_set_ofs, _mux_clr_ofs, _shift, _width, \
> > + _gate, _upd_ofs, _upd, _flags, _ops) { \
> > + .id = _id, \
> > + .name = _name, \
> > + .mux_ofs = _mux_ofs, \
> > + .set_ofs = _mux_set_ofs, \
> > + .clr_ofs = _mux_clr_ofs, \
> > + .upd_ofs = _upd_ofs, \
> > + .mux_shift = _shift, \
> > + .mux_width = _width, \
> > + .gate_shift = _gate, \
> > + .upd_shift = _upd, \
> > + .parent_names = _parents, \
> > + .num_parents = ARRAY_SIZE(_parents), \
> > + .flags = _flags, \
> > + .ops = &_ops, \
> > + }
> > +
> > +#define MUX_GATE_CLR_SET_UPD_FLAGS(_id, _name, _parents, _mux_ofs, \
> > + _mux_set_ofs, _mux_clr_ofs, _shift, _width, \
> > + _gate, _upd_ofs, _upd, _flags) \
> > + GATE_CLR_SET_UPD_FLAGS(_id, _name, _parents, _mux_ofs, \
> > + _mux_set_ofs, _mux_clr_ofs, _shift, _width, \
> > + _gate, _upd_ofs, _upd, _flags, \
> > + mtk_mux_gate_clr_set_upd_ops)
> > +
> > +#define MUX_GATE_CLR_SET_UPD(_id, _name, _parents, _mux_ofs, \
> > + _mux_set_ofs, _mux_clr_ofs, _shift, _width, \
> > + _gate, _upd_ofs, _upd) \
> > + MUX_GATE_CLR_SET_UPD_FLAGS(_id, _name, _parents, \
> > + _mux_ofs, _mux_set_ofs, _mux_clr_ofs, _shift, \
> > + _width, _gate, _upd_ofs, _upd, \
> > + CLK_SET_RATE_PARENT)
> > +
> > +struct clk *mtk_clk_register_mux(const struct mtk_mux *mux,
> > + struct regmap *regmap,
> > + spinlock_t *lock);
> > +
> > +int mtk_clk_register_muxes(const struct mtk_mux *muxes,
> > + int num, struct device_node *node,
> > + spinlock_t *lock,
> > + struct clk_onecell_data *clk_data);
> > +
> > +#endif /* __DRV_CLK_MUX_H */
> > --
> > 2.18.0
> >
>
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek
WARNING: multiple messages have this Message-ID (diff)
From: weiyi.lu@mediatek.com (Weiyi Lu)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v1 01/11] clk: mediatek: add new clkmux register API
Date: Tue, 20 Nov 2018 14:34:57 +0800 [thread overview]
Message-ID: <1542695697.5132.24.camel@mtksdaap41> (raw)
In-Reply-To: <CANMq1KDEg-9WzfaRB-n4Ab0PxjqfZL_aeTAXD336N6VzX8oFhQ@mail.gmail.com>
On Tue, 2018-11-13 at 08:00 -0800, Nicolas Boichat wrote:
> On Mon, Nov 5, 2018 at 10:42 PM Weiyi Lu <weiyi.lu@mediatek.com> wrote:
> >
> > From: Owen Chen <owen.chen@mediatek.com>
> >
> > On both MT8183 & MT6765, there add "set/clr" register for
> > each clkmux setting, and one update register to trigger value change.
> > It is designed to prevent read-modify-write racing issue.
> > The sw design need to add a new API to handle this hw change with
> > a new mtk_clk_mux/mtk_mux struct in new file "clk-mux.c", "clk-mux.h".
> >
> > Signed-off-by: Owen Chen <owen.chen@mediatek.com>
> > Signed-off-by: Weiyi Lu <weiyi.lu@mediatek.com>
> > ---
> > drivers/clk/mediatek/Makefile | 2 +-
> > drivers/clk/mediatek/clk-mux.c | 252 +++++++++++++++++++++++++++++++++
> > drivers/clk/mediatek/clk-mux.h | 101 +++++++++++++
> > 3 files changed, 354 insertions(+), 1 deletion(-)
> > create mode 100644 drivers/clk/mediatek/clk-mux.c
> > create mode 100644 drivers/clk/mediatek/clk-mux.h
> >
> > diff --git a/drivers/clk/mediatek/Makefile b/drivers/clk/mediatek/Makefile
> > index 844b55d2770d..b97980dbb738 100644
> > --- a/drivers/clk/mediatek/Makefile
> > +++ b/drivers/clk/mediatek/Makefile
> > @@ -1,5 +1,5 @@
> > # SPDX-License-Identifier: GPL-2.0
> > -obj-$(CONFIG_COMMON_CLK_MEDIATEK) += clk-mtk.o clk-pll.o clk-gate.o clk-apmixed.o clk-cpumux.o reset.o
> > +obj-$(CONFIG_COMMON_CLK_MEDIATEK) += clk-mtk.o clk-pll.o clk-gate.o clk-apmixed.o clk-cpumux.o reset.o clk-mux.o
> > obj-$(CONFIG_COMMON_CLK_MT6797) += clk-mt6797.o
> > obj-$(CONFIG_COMMON_CLK_MT6797_IMGSYS) += clk-mt6797-img.o
> > obj-$(CONFIG_COMMON_CLK_MT6797_MMSYS) += clk-mt6797-mm.o
> > diff --git a/drivers/clk/mediatek/clk-mux.c b/drivers/clk/mediatek/clk-mux.c
> > new file mode 100644
> > index 000000000000..18bc9a146be0
> > --- /dev/null
> > +++ b/drivers/clk/mediatek/clk-mux.c
> > @@ -0,0 +1,252 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2018 MediaTek Inc.
> > + * Author: Owen Chen <owen.chen@mediatek.com>
> > + */
> > +
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/slab.h>
> > +#include <linux/mfd/syscon.h>
> > +
> > +#include "clk-mtk.h"
> > +#include "clk-mux.h"
> > +
> > +static inline struct mtk_clk_mux
> > + *to_mtk_clk_mux(struct clk_hw *hw)
> > +{
> > + return container_of(hw, struct mtk_clk_mux, hw);
> > +}
> > +
> > +static int mtk_clk_mux_enable(struct clk_hw *hw)
> > +{
> > + struct mtk_clk_mux *mux = to_mtk_clk_mux(hw);
> > + u32 mask = BIT(mux->gate_shift);
> > +
> > + regmap_update_bits(mux->regmap, mux->mux_ofs, mask, 0);
> > +
> > + return 0;
>
> Might as well return regmap_update_bits(...).
>
OK, will fix in next version
> > +}
> > +
> > +static void mtk_clk_mux_disable(struct clk_hw *hw)
> > +{
> > + struct mtk_clk_mux *mux = to_mtk_clk_mux(hw);
> > + u32 mask = BIT(mux->gate_shift);
> > +
> > + regmap_update_bits(mux->regmap, mux->mux_ofs, mask, mask);
> > +}
> > +
> > +static int mtk_clk_mux_enable_setclr(struct clk_hw *hw)
> > +{
> > + struct mtk_clk_mux *mux = to_mtk_clk_mux(hw);
> > + u32 val;
> > +
> > + val = BIT(mux->gate_shift);
> > + regmap_write(mux->regmap, mux->mux_clr_ofs, val);
> > +
> > + return 0;
>
> ditto: return regmap_write(...) .
>
will fix in next version
> > +}
> > +
> > +static void mtk_clk_mux_disable_setclr(struct clk_hw *hw)
> > +{
> > + struct mtk_clk_mux *mux = to_mtk_clk_mux(hw);
> > + u32 val;
> > +
> > + val = BIT(mux->gate_shift);
> > + regmap_write(mux->regmap, mux->mux_set_ofs, val);
> > +}
> > +
> > +static int mtk_clk_mux_is_enabled(struct clk_hw *hw)
> > +{
> > + struct mtk_clk_mux *mux = to_mtk_clk_mux(hw);
> > + u32 val = 0;
>
> No need to init to 0.
>
will fix in next version
> > +
> > + regmap_read(mux->regmap, mux->mux_ofs, &val);
> > +
> > + return ((val & BIT(mux->gate_shift)) == 0) ? 1 : 0;
>
> Just (val & BIT(mux->gate_shift)) != 0. Or more simply val &
> BIT(mux->gate_shift).
>
will fix in next version
> > +}
> > +
> > +static u8 mtk_clk_mux_get_parent(struct clk_hw *hw)
> > +{
> > + struct mtk_clk_mux *mux = to_mtk_clk_mux(hw);
> > + int num_parents = clk_hw_get_num_parents(hw);
> > + u32 mask = GENMASK(mux->mux_width - 1, 0);
> > + u32 val;
> > +
> > + regmap_read(mux->regmap, mux->mux_ofs, &val);
> > + val = (val >> mux->mux_shift) & mask;
> > +
> > + if (val >= num_parents || val >= U8_MAX)
>
> val > U8_MAX, technically.
>
> > + return -ERANGE;
>
> This looks wrong, -34 will be cast to an u8 and appear to be valid...
>
will fix in next version
> > +
> > + return val;
> > +}
> > +
> > +static int mtk_clk_mux_set_parent_lock(struct clk_hw *hw, u8 index)
> > +{
> > + struct mtk_clk_mux *mux = to_mtk_clk_mux(hw);
> > + u32 mask = GENMASK(mux->mux_width - 1, 0);
> > + u32 val;
> > + unsigned long flags = 0;
>
> No need to init to 0.
>
will fix in next version.
> > +
> > + if (mux->lock)
> > + spin_lock_irqsave(mux->lock, flags);
> > + else
> > + __acquire(mux->lock);
> > +
> > + regmap_read(mux->regmap, mux->mux_ofs, &val);
> > + val = (val & mask) >> mux->mux_shift;
> > +
> > + if (val != index) {
> > + val = index << mux->mux_shift;
> > + regmap_update_bits(mux->regmap, mux->mux_ofs, mask, val);
> > + }
>
> What's the point of doing read/compare/update? Are there side effects
> if you just write the value unconditionally?
>
Actually writing the value directly has no side effects. We just want to
reduce unnecessary operations if the target parent of mux is already
selected.
> > +
> > + if (mux->lock)
> > + spin_unlock_irqrestore(mux->lock, flags);
> > + else
> > + __release(mux->lock);
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_clk_mux_set_parent_setclr_lock(struct clk_hw *hw, u8 index)
> > +{
> > + struct mtk_clk_mux *mux = to_mtk_clk_mux(hw);
> > + u32 mask = GENMASK(mux->mux_width - 1, 0);
> > + u32 val, orig;
> > + unsigned long flags = 0;
> > +
> > + if (mux->lock)
> > + spin_lock_irqsave(mux->lock, flags);
> > + else
> > + __acquire(mux->lock);
> > +
> > + regmap_read(mux->regmap, mux->mux_ofs, &val);
> > + orig = val;
>
> Just read into &orig above.
>
will fix in next version
> > + val &= ~(mask << mux->mux_shift);
> > + val |= index << mux->mux_shift;
>
> Maybe this is more readable?
> val = orig & ~(mask << mux->mux_shift) | (index << mux->mux_shift)
>
will fix in next version
> > +
> > + if (val != orig) {
> > + val = (mask << mux->mux_shift);
> > + regmap_write(mux->regmap, mux->mux_clr_ofs, val);
>
> I'd just regmap_write(..., mask << mux->mux_shift), instead if overriding val.
>
will fix in next version
> > + val = (index << mux->mux_shift);
> > + regmap_write(mux->regmap, mux->mux_set_ofs, val);
> > +
> > + if (mux->upd_shift >= 0)
> > + regmap_write(mux->regmap, mux->upd_ofs,
> > + BIT(mux->upd_shift));
> > + }
> > +
> > + if (mux->lock)
> > + spin_unlock_irqrestore(mux->lock, flags);
> > + else
> > + __release(mux->lock);
> > +
> > + return 0;
> > +}
> > +
> > +const struct clk_ops mtk_mux_ops = {
> > + .get_parent = mtk_clk_mux_get_parent,
> > + .set_parent = mtk_clk_mux_set_parent_lock,
> > +};
> > +
> > +const struct clk_ops mtk_mux_clr_set_upd_ops = {
> > + .get_parent = mtk_clk_mux_get_parent,
> > + .set_parent = mtk_clk_mux_set_parent_setclr_lock,
> > +};
> > +
> > +const struct clk_ops mtk_mux_gate_ops = {
> > + .enable = mtk_clk_mux_enable,
> > + .disable = mtk_clk_mux_disable,
> > + .is_enabled = mtk_clk_mux_is_enabled,
> > + .get_parent = mtk_clk_mux_get_parent,
> > + .set_parent = mtk_clk_mux_set_parent_lock,
> > +};
> > +
> > +const struct clk_ops mtk_mux_gate_clr_set_upd_ops = {
> > + .enable = mtk_clk_mux_enable_setclr,
> > + .disable = mtk_clk_mux_disable_setclr,
> > + .is_enabled = mtk_clk_mux_is_enabled,
> > + .get_parent = mtk_clk_mux_get_parent,
> > + .set_parent = mtk_clk_mux_set_parent_setclr_lock,
> > +};
> > +
> > +struct clk *mtk_clk_register_mux(const struct mtk_mux *mux,
> > + struct regmap *regmap,
> > + spinlock_t *lock)
> > +{
> > + struct mtk_clk_mux *mtk_mux = NULL;
>
> Not necessary to init to NULL.
>
will fix in next version
> > + struct clk_init_data init;
> > + struct clk *clk;
> > +
> > + mtk_mux = kzalloc(sizeof(*mtk_mux), GFP_KERNEL);
> > + if (!mtk_mux)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + init.name = mux->name;
> > + init.flags = (mux->flags) | CLK_SET_RATE_PARENT;
>
> mux->flags | CLK_SET_RATE_PARENT
>
will fix in next version
> > + init.parent_names = mux->parent_names;
> > + init.num_parents = mux->num_parents;
> > + init.ops = mux->ops;
> > +
> > + mtk_mux->regmap = regmap;
> > + mtk_mux->name = mux->name;
> > + mtk_mux->mux_ofs = mux->mux_ofs;
> > + mtk_mux->mux_set_ofs = mux->set_ofs;
> > + mtk_mux->mux_clr_ofs = mux->clr_ofs;
> > + mtk_mux->upd_ofs = mux->upd_ofs;
> > + mtk_mux->mux_shift = mux->mux_shift;
> > + mtk_mux->mux_width = mux->mux_width;
> > + mtk_mux->gate_shift = mux->gate_shift;
> > + mtk_mux->upd_shift = mux->upd_shift;
> > +
> > + mtk_mux->lock = lock;
> > + mtk_mux->hw.init = &init;
> > +
> > + clk = clk_register(NULL, &mtk_mux->hw);
> > + if (IS_ERR(clk)) {
> > + kfree(mtk_mux);
> > + return clk;
> > + }
> > +
> > + return clk;
> > +}
> > +
> > +int mtk_clk_register_muxes(const struct mtk_mux *muxes,
> > + int num, struct device_node *node,
> > + spinlock_t *lock,
> > + struct clk_onecell_data *clk_data)
> > +{
> > + struct regmap *regmap;
> > + struct clk *clk;
> > + int i;
> > +
> > + if (!clk_data)
> > + return -ENOMEM;
>
> Is this check necessary? After all you know who the callers are and
> maybe you need to be careful not to call this with NULL clk_data?
>
You're right. it's unnecessary. will fix in next version
> > +
> > + regmap = syscon_node_to_regmap(node);
> > + if (IS_ERR(regmap)) {
> > + pr_err("Cannot find regmap for %pOF: %ld\n", node,
> > + PTR_ERR(regmap));
> > + return PTR_ERR(regmap);
> > + }
> > +
> > + for (i = 0; i < num; i++) {
> > + const struct mtk_mux *mux = &muxes[i];
> > +
> > + if (IS_ERR_OR_NULL(clk_data->clks[mux->id])) {
>
> A NULL check seems enough, as you'll never assign a PTR_ERR(..) value
> into clk_data->clks[mux->id].
>
> > + clk = mtk_clk_register_mux(mux, regmap, lock);
> > +
> > + if (IS_ERR(clk)) {
> > + pr_err("Failed to register clk %s: %ld\n",
> > + mux->name, PTR_ERR(clk));
> > + continue;
>
> Are we ok with returning 0 even though some of these calls failed?
>
We'd like to skip the failed one first and keep registering the
remainings.
> > + }
> > +
> > + clk_data->clks[mux->id] = clk;
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > diff --git a/drivers/clk/mediatek/clk-mux.h b/drivers/clk/mediatek/clk-mux.h
> > new file mode 100644
> > index 000000000000..ff0276bb771c
> > --- /dev/null
> > +++ b/drivers/clk/mediatek/clk-mux.h
> > @@ -0,0 +1,101 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (c) 2018 MediaTek Inc.
> > + * Author: Owen Chen <owen.chen@mediatek.com>
> > + */
> > +
> > +#ifndef __DRV_CLK_MUX_H
> > +#define __DRV_CLK_MUX_H
> > +
> > +#include <linux/clk-provider.h>
> > +
> > +struct mtk_clk_mux {
> > + struct clk_hw hw;
> > + struct regmap *regmap;
> > +
> > + const char *name;
> > +
> > + u32 mux_set_ofs;
> > + u32 mux_clr_ofs;
> > + u32 mux_ofs;
> > + u32 upd_ofs;
> > +
> > + u8 mux_shift;
> > + u8 mux_width;
> > + u8 gate_shift;
> > + u8 upd_shift;
> > +
> > + spinlock_t *lock;
> > +};
> > +
> > +struct mtk_mux {
> > + int id;
> > + const char *name;
> > + const char * const *parent_names;
> > + unsigned int flags;
> > +
> > + u32 mux_ofs;
> > + u32 set_ofs;
> > + u32 clr_ofs;
> > + u32 upd_ofs;
> > +
> > + u8 mux_shift;
> > + u8 mux_width;
> > + u8 gate_shift;
> > + u8 upd_shift;
> > +
> > + const struct clk_ops *ops;
> > +
> > + signed char num_parents;
> > +};
> > +
> > +extern const struct clk_ops mtk_mux_ops;
> > +extern const struct clk_ops mtk_mux_clr_set_upd_ops;
> > +extern const struct clk_ops mtk_mux_gate_ops;
> > +extern const struct clk_ops mtk_mux_gate_clr_set_upd_ops;
> > +
> > +#define GATE_CLR_SET_UPD_FLAGS(_id, _name, _parents, _mux_ofs, \
> > + _mux_set_ofs, _mux_clr_ofs, _shift, _width, \
> > + _gate, _upd_ofs, _upd, _flags, _ops) { \
> > + .id = _id, \
> > + .name = _name, \
> > + .mux_ofs = _mux_ofs, \
> > + .set_ofs = _mux_set_ofs, \
> > + .clr_ofs = _mux_clr_ofs, \
> > + .upd_ofs = _upd_ofs, \
> > + .mux_shift = _shift, \
> > + .mux_width = _width, \
> > + .gate_shift = _gate, \
> > + .upd_shift = _upd, \
> > + .parent_names = _parents, \
> > + .num_parents = ARRAY_SIZE(_parents), \
> > + .flags = _flags, \
> > + .ops = &_ops, \
> > + }
> > +
> > +#define MUX_GATE_CLR_SET_UPD_FLAGS(_id, _name, _parents, _mux_ofs, \
> > + _mux_set_ofs, _mux_clr_ofs, _shift, _width, \
> > + _gate, _upd_ofs, _upd, _flags) \
> > + GATE_CLR_SET_UPD_FLAGS(_id, _name, _parents, _mux_ofs, \
> > + _mux_set_ofs, _mux_clr_ofs, _shift, _width, \
> > + _gate, _upd_ofs, _upd, _flags, \
> > + mtk_mux_gate_clr_set_upd_ops)
> > +
> > +#define MUX_GATE_CLR_SET_UPD(_id, _name, _parents, _mux_ofs, \
> > + _mux_set_ofs, _mux_clr_ofs, _shift, _width, \
> > + _gate, _upd_ofs, _upd) \
> > + MUX_GATE_CLR_SET_UPD_FLAGS(_id, _name, _parents, \
> > + _mux_ofs, _mux_set_ofs, _mux_clr_ofs, _shift, \
> > + _width, _gate, _upd_ofs, _upd, \
> > + CLK_SET_RATE_PARENT)
> > +
> > +struct clk *mtk_clk_register_mux(const struct mtk_mux *mux,
> > + struct regmap *regmap,
> > + spinlock_t *lock);
> > +
> > +int mtk_clk_register_muxes(const struct mtk_mux *muxes,
> > + int num, struct device_node *node,
> > + spinlock_t *lock,
> > + struct clk_onecell_data *clk_data);
> > +
> > +#endif /* __DRV_CLK_MUX_H */
> > --
> > 2.18.0
> >
>
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek
next prev parent reply other threads:[~2018-11-20 6:35 UTC|newest]
Thread overview: 71+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-11-06 6:41 [PATCH v1 00/11] Mediatek MT8183 clock and scpsys support Weiyi Lu
2018-11-06 6:41 ` Weiyi Lu
2018-11-06 6:41 ` Weiyi Lu
2018-11-06 6:41 ` Weiyi Lu
2018-11-06 6:41 ` Weiyi Lu
2018-11-06 6:41 ` Weiyi Lu
2018-11-06 6:41 ` [PATCH v1 01/11] clk: mediatek: add new clkmux register API Weiyi Lu
2018-11-06 6:41 ` Weiyi Lu
2018-11-06 6:41 ` Weiyi Lu
2018-11-13 16:00 ` Nicolas Boichat
2018-11-13 16:00 ` Nicolas Boichat
2018-11-20 6:34 ` Weiyi Lu [this message]
2018-11-20 6:34 ` Weiyi Lu
2018-11-20 6:34 ` Weiyi Lu
2018-11-06 6:41 ` [PATCH v1 02/11] clk: mediatek: add new member to mtk_pll_data Weiyi Lu
2018-11-06 6:41 ` Weiyi Lu
2018-11-06 6:41 ` Weiyi Lu
2018-11-13 16:18 ` Nicolas Boichat
2018-11-13 16:18 ` Nicolas Boichat
2018-11-20 3:51 ` Weiyi Lu
2018-11-20 3:51 ` Weiyi Lu
2018-11-20 3:51 ` Weiyi Lu
2018-11-21 8:03 ` Stephen Boyd
2018-11-21 8:03 ` Stephen Boyd
2018-11-06 6:41 ` [PATCH v1 03/11] clk: mediatek: Disable tuner_en before change PLL rate Weiyi Lu
2018-11-06 6:41 ` Weiyi Lu
2018-11-06 6:41 ` Weiyi Lu
2018-11-06 6:41 ` [PATCH v1 04/11] soc: mediatek: add new flow for mtcmos power Weiyi Lu
2018-11-06 6:41 ` Weiyi Lu
2018-11-06 6:41 ` Weiyi Lu
2018-11-13 19:31 ` Nicolas Boichat
2018-11-13 19:31 ` Nicolas Boichat
2018-11-20 2:37 ` Weiyi Lu
2018-11-20 2:37 ` Weiyi Lu
2018-11-20 2:37 ` Weiyi Lu
2018-11-21 8:07 ` Stephen Boyd
2018-11-21 8:07 ` Stephen Boyd
2018-11-21 9:21 ` Weiyi Lu
2018-11-21 9:21 ` Weiyi Lu
2018-11-21 9:21 ` Weiyi Lu
2018-11-06 6:42 ` [PATCH v1 05/11] dt-bindings: ARM: Mediatek: Document bindings for MT8183 Weiyi Lu
2018-11-06 6:42 ` Weiyi Lu
2018-11-06 6:42 ` Weiyi Lu
2018-11-06 6:42 ` [PATCH v1 06/11] clk: mediatek: Add dt-bindings for MT8183 clocks Weiyi Lu
2018-11-06 6:42 ` Weiyi Lu
2018-11-06 6:42 ` Weiyi Lu
2018-11-06 6:42 ` [PATCH v1 07/11] clk: mediatek: Add flags support for mtk_gate data Weiyi Lu
2018-11-06 6:42 ` Weiyi Lu
2018-11-06 6:42 ` Weiyi Lu
2018-11-06 6:42 ` [PATCH v1 08/11] clk: mediatek: Add MT8183 clock support Weiyi Lu
2018-11-06 6:42 ` Weiyi Lu
2018-11-06 6:42 ` Weiyi Lu
2018-11-14 6:25 ` Nicolas Boichat
2018-11-14 6:25 ` Nicolas Boichat
2018-11-19 4:14 ` Weiyi Lu
2018-11-19 4:14 ` Weiyi Lu
2018-11-19 4:14 ` Weiyi Lu
2018-11-06 6:42 ` [PATCH v1 09/11] dt-bindings: soc: fix typo of MT8173 power dt-bindings Weiyi Lu
2018-11-06 6:42 ` Weiyi Lu
2018-11-06 6:42 ` Weiyi Lu
2018-11-06 6:42 ` [PATCH v1 10/11] dt-bindings: soc: Add MT8183 " Weiyi Lu
2018-11-06 6:42 ` Weiyi Lu
2018-11-06 6:42 ` Weiyi Lu
2018-11-06 6:42 ` [PATCH v1 11/11] soc: mediatek: Add MT8183 scpsys support Weiyi Lu
2018-11-06 6:42 ` Weiyi Lu
2018-11-06 6:42 ` Weiyi Lu
2018-11-13 19:35 ` Nicolas Boichat
2018-11-13 19:35 ` Nicolas Boichat
2018-11-19 3:59 ` Weiyi Lu
2018-11-19 3:59 ` Weiyi Lu
2018-11-19 3:59 ` Weiyi Lu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1542695697.5132.24.camel@mtksdaap41 \
--to=weiyi.lu@mediatek.com \
--cc=drinkcat@chromium.org \
--cc=fan.chen@mediatek.com \
--cc=jamesjj.liao@mediatek.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-clk@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mediatek@lists.infradead.org \
--cc=matthias.bgg@gmail.com \
--cc=owen.chen@mediatek.com \
--cc=robh@kernel.org \
--cc=sboyd@codeaurora.org \
--cc=srv_heupstream@mediatek.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.