Re: [PATCH v1 01/11] clk: mediatek: add new clkmux register API
On Tue, 2018-11-13 at 08:00 -0800, Nicolas Boichat wrote:
> On Mon, Nov 5, 2018 at 10:42 PM Weiyi Lu wrote:
> >
> > From: Owen Chen
> >
> > 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
> > Signed-off-by: Weiyi Lu
> > ---
> > 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 ..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
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#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);
> >
Re: [PATCH v1 01/11] clk: mediatek: add new clkmux register API
On Mon, Nov 5, 2018 at 10:42 PM Weiyi Lu wrote:
>
> From: Owen Chen
>
> 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
> Signed-off-by: Weiyi Lu
> ---
> 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 ..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
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +
> +#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(...).
> +}
> +
> +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(...) .
> +}
> +
> +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.
> +
> + 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).
> +}
> +
> +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...
> +
> + 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.
> +
> + 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?
> +
> + if (mux->lock)
> + spin_unlock_irqrestore(mux->lock, flag
[PATCH v1 01/11] clk: mediatek: add new clkmux register API
From: Owen Chen
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
Signed-off-by: Weiyi Lu
---
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 ..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
+ */
+
+#include
+#include
+#include
+#include
+
+#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;
+}
+
+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;
+}
+
+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;
+
+ regmap_read(mux->regmap, mux->mux_ofs, &val);
+
+ return ((val & BIT(mux->gate_shift)) == 0) ? 1 : 0;
+}
+
+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)
+ return -ERANGE;
+
+ 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;
+
+ 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);
+ }
+
+ 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;
+ val &= ~(mask << mux->mux_shift);
+ val |= index << mux->mux_shift;
+
+ if (val != orig) {
+ val = (mask << mux->mux_shift);
+ regmap_write(mux->regmap, mux->mux_clr_ofs, val);
+ val
