[PATCH 0/2] clk: mediatek: Fix mux clock re-parenting

[PATCH 0/2] clk: mediatek: Fix mux clock re-parenting

[Laurent Pinchart]

Hello,

This small patch series fixes re-parenting of the mux clocks on MediaTek
platforms. Patch 1/2 is a drive-by cleanup that removes unneeded code
(as I didn't want to update that code in the next patch), while patch
2/2 fixes the problem. Please see invidual patches for details.

The code has been tested with the CAMTG2 and CAMTG3 clocks on MT8183.
Quite interestingly some parents can be selected without this fix (I can
for instance select a parent that has a lower frequency), but not all of
them. I'm not sure if all mux clocks are affected, but as far as I can
tell the fix shouldn't introduce regressions.

Laurent Pinchart (2):
  clk: mediatek: mux: Drop unused clock ops
  clk: mediatek: mux: Update parent at enable time

 drivers/clk/mediatek/clk-mux.c | 93 ++++++++++++----------------------
 drivers/clk/mediatek/clk-mux.h | 14 ++---
 2 files changed, 34 insertions(+), 73 deletions(-)

-- 
Regards,

Laurent Pinchart

[PATCH 1/2] clk: mediatek: mux: Drop unused clock ops

[Laurent Pinchart]

Three out of the four defined clock ops are unused. Drop them.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/clk/mediatek/clk-mux.c | 61 ++--------------------------------
 drivers/clk/mediatek/clk-mux.h | 13 ++------
 2 files changed, 4 insertions(+), 70 deletions(-)

diff --git a/drivers/clk/mediatek/clk-mux.c b/drivers/clk/mediatek/clk-mux.c
index dcc1352bf13c..9370bebca7f8 100644
--- a/drivers/clk/mediatek/clk-mux.c
+++ b/drivers/clk/mediatek/clk-mux.c
@@ -17,23 +17,6 @@ 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->data->gate_shift);
-
-	return regmap_update_bits(mux->regmap, mux->data->mux_ofs,
-			mask, ~mask);
-}
-
-static void mtk_clk_mux_disable(struct clk_hw *hw)
-{
-	struct mtk_clk_mux *mux = to_mtk_clk_mux(hw);
-	u32 mask = BIT(mux->data->gate_shift);
-
-	regmap_update_bits(mux->regmap, mux->data->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);
@@ -72,28 +55,6 @@ static u8 mtk_clk_mux_get_parent(struct clk_hw *hw)
 	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->data->mux_width - 1, 0);
-	unsigned long flags = 0;
-
-	if (mux->lock)
-		spin_lock_irqsave(mux->lock, flags);
-	else
-		__acquire(mux->lock);
-
-	regmap_update_bits(mux->regmap, mux->data->mux_ofs, mask,
-		index << mux->data->mux_shift);
-
-	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);
@@ -129,25 +90,7 @@ static int mtk_clk_mux_set_parent_setclr_lock(struct clk_hw *hw, u8 index)
 	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 = {
+static const struct clk_ops mtk_mux_ops = {
 	.enable = mtk_clk_mux_enable_setclr,
 	.disable = mtk_clk_mux_disable_setclr,
 	.is_enabled = mtk_clk_mux_is_enabled,
@@ -171,7 +114,7 @@ static struct clk *mtk_clk_register_mux(const struct mtk_mux *mux,
 	init.flags = mux->flags | CLK_SET_RATE_PARENT;
 	init.parent_names = mux->parent_names;
 	init.num_parents = mux->num_parents;
-	init.ops = mux->ops;
+	init.ops = &mtk_mux_ops;
 
 	clk_mux->regmap = regmap;
 	clk_mux->data = mux;
diff --git a/drivers/clk/mediatek/clk-mux.h b/drivers/clk/mediatek/clk-mux.h
index 8e2f927dd2ff..15c62366ba9a 100644
--- a/drivers/clk/mediatek/clk-mux.h
+++ b/drivers/clk/mediatek/clk-mux.h
@@ -32,19 +32,12 @@ struct mtk_mux {
 	u8 gate_shift;
 	s8 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) {		\
+			_gate, _upd_ofs, _upd, _flags) {		\
 		.id = _id,						\
 		.name = _name,						\
 		.mux_ofs = _mux_ofs,					\
@@ -58,7 +51,6 @@ extern const struct clk_ops mtk_mux_gate_clr_set_upd_ops;
 		.parent_names = _parents,				\
 		.num_parents = ARRAY_SIZE(_parents),			\
 		.flags = _flags,					\
-		.ops = &_ops,						\
 	}
 
 #define MUX_GATE_CLR_SET_UPD_FLAGS(_id, _name, _parents, _mux_ofs,	\
@@ -66,8 +58,7 @@ extern const struct clk_ops mtk_mux_gate_clr_set_upd_ops;
 			_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)
+			_gate, _upd_ofs, _upd, _flags)			\
 
 #define MUX_GATE_CLR_SET_UPD(_id, _name, _parents, _mux_ofs,		\
 			_mux_set_ofs, _mux_clr_ofs, _shift, _width,	\
-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/2] clk: mediatek: mux: Drop unused clock ops

[Stephen Boyd]

Quoting Laurent Pinchart (2021-01-25 09:08:18)
> Three out of the four defined clock ops are unused. Drop them.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---

Applied to clk-next

[PATCH 2/2] clk: mediatek: mux: Update parent at enable time

[Laurent Pinchart]

The mux clocks don't always correctly take the new parent into account
when the parent is updated while the clock is disabled. Set the update
bit when enabling the clock to force an update of the mux.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/clk/mediatek/clk-mux.c | 32 +++++++++++++++++++++++++++++---
 drivers/clk/mediatek/clk-mux.h |  1 +
 2 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/mediatek/clk-mux.c b/drivers/clk/mediatek/clk-mux.c
index 9370bebca7f8..b0c61709bacc 100644
--- a/drivers/clk/mediatek/clk-mux.c
+++ b/drivers/clk/mediatek/clk-mux.c
@@ -20,9 +20,33 @@ static inline struct mtk_clk_mux *to_mtk_clk_mux(struct clk_hw *hw)
 static int mtk_clk_mux_enable_setclr(struct clk_hw *hw)
 {
 	struct mtk_clk_mux *mux = to_mtk_clk_mux(hw);
+	unsigned long flags = 0;
 
-	return regmap_write(mux->regmap, mux->data->clr_ofs,
-			BIT(mux->data->gate_shift));
+	if (mux->lock)
+		spin_lock_irqsave(mux->lock, flags);
+	else
+		__acquire(mux->lock);
+
+	regmap_write(mux->regmap, mux->data->clr_ofs,
+		     BIT(mux->data->gate_shift));
+
+	/*
+	 * If the parent has been changed when the clock was disabled, it will
+	 * not be effective yet. Set the update bit to ensure the mux gets
+	 * updated.
+	 */
+	if (mux->reparent && mux->data->upd_shift >= 0) {
+		regmap_write(mux->regmap, mux->data->upd_ofs,
+			     BIT(mux->data->upd_shift));
+		mux->reparent = false;
+	}
+
+	if (mux->lock)
+		spin_unlock_irqrestore(mux->lock, flags);
+	else
+		__release(mux->lock);
+
+	return 0;
 }
 
 static void mtk_clk_mux_disable_setclr(struct clk_hw *hw)
@@ -77,9 +101,11 @@ static int mtk_clk_mux_set_parent_setclr_lock(struct clk_hw *hw, u8 index)
 		regmap_write(mux->regmap, mux->data->set_ofs,
 				index << mux->data->mux_shift);
 
-		if (mux->data->upd_shift >= 0)
+		if (mux->data->upd_shift >= 0) {
 			regmap_write(mux->regmap, mux->data->upd_ofs,
 					BIT(mux->data->upd_shift));
+			mux->reparent = true;
+		}
 	}
 
 	if (mux->lock)
diff --git a/drivers/clk/mediatek/clk-mux.h b/drivers/clk/mediatek/clk-mux.h
index 15c62366ba9a..f1946161ade1 100644
--- a/drivers/clk/mediatek/clk-mux.h
+++ b/drivers/clk/mediatek/clk-mux.h
@@ -14,6 +14,7 @@ struct mtk_clk_mux {
 	struct regmap *regmap;
 	const struct mtk_mux *data;
 	spinlock_t *lock;
+	bool reparent;
 };
 
 struct mtk_mux {
-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/2] clk: mediatek: mux: Update parent at enable time

[Weiyi Lu]

On Mon, 2021-01-25 at 19:08 +0200, Laurent Pinchart wrote:
> The mux clocks don't always correctly take the new parent into account
> when the parent is updated while the clock is disabled. Set the update
> bit when enabling the clock to force an update of the mux.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Hi Laurent,

Thank you for the patch. Looks good to me.
Reviewed-by: Weiyi Lu <weiyi.lu@mediatek.com>

> ---
>  drivers/clk/mediatek/clk-mux.c | 32 +++++++++++++++++++++++++++++---
>  drivers/clk/mediatek/clk-mux.h |  1 +
>  2 files changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/clk/mediatek/clk-mux.c b/drivers/clk/mediatek/clk-mux.c
> index 9370bebca7f8..b0c61709bacc 100644
> --- a/drivers/clk/mediatek/clk-mux.c
> +++ b/drivers/clk/mediatek/clk-mux.c
> @@ -20,9 +20,33 @@ static inline struct mtk_clk_mux *to_mtk_clk_mux(struct clk_hw *hw)
>  static int mtk_clk_mux_enable_setclr(struct clk_hw *hw)
>  {
>  	struct mtk_clk_mux *mux = to_mtk_clk_mux(hw);
> +	unsigned long flags = 0;
>  
> -	return regmap_write(mux->regmap, mux->data->clr_ofs,
> -			BIT(mux->data->gate_shift));
> +	if (mux->lock)
> +		spin_lock_irqsave(mux->lock, flags);
> +	else
> +		__acquire(mux->lock);
> +
> +	regmap_write(mux->regmap, mux->data->clr_ofs,
> +		     BIT(mux->data->gate_shift));
> +
> +	/*
> +	 * If the parent has been changed when the clock was disabled, it will
> +	 * not be effective yet. Set the update bit to ensure the mux gets
> +	 * updated.
> +	 */
> +	if (mux->reparent && mux->data->upd_shift >= 0) {
> +		regmap_write(mux->regmap, mux->data->upd_ofs,
> +			     BIT(mux->data->upd_shift));
> +		mux->reparent = false;
> +	}
> +
> +	if (mux->lock)
> +		spin_unlock_irqrestore(mux->lock, flags);
> +	else
> +		__release(mux->lock);
> +
> +	return 0;
>  }
>  
>  static void mtk_clk_mux_disable_setclr(struct clk_hw *hw)
> @@ -77,9 +101,11 @@ static int mtk_clk_mux_set_parent_setclr_lock(struct clk_hw *hw, u8 index)
>  		regmap_write(mux->regmap, mux->data->set_ofs,
>  				index << mux->data->mux_shift);
>  
> -		if (mux->data->upd_shift >= 0)
> +		if (mux->data->upd_shift >= 0) {
>  			regmap_write(mux->regmap, mux->data->upd_ofs,
>  					BIT(mux->data->upd_shift));
> +			mux->reparent = true;
> +		}
>  	}
>  
>  	if (mux->lock)
> diff --git a/drivers/clk/mediatek/clk-mux.h b/drivers/clk/mediatek/clk-mux.h
> index 15c62366ba9a..f1946161ade1 100644
> --- a/drivers/clk/mediatek/clk-mux.h
> +++ b/drivers/clk/mediatek/clk-mux.h
> @@ -14,6 +14,7 @@ struct mtk_clk_mux {
>  	struct regmap *regmap;
>  	const struct mtk_mux *data;
>  	spinlock_t *lock;
> +	bool reparent;
>  };
>  
>  struct mtk_mux {

Re: [PATCH 2/2] clk: mediatek: mux: Update parent at enable time

[Stephen Boyd]

Quoting Laurent Pinchart (2021-01-25 09:08:19)
> The mux clocks don't always correctly take the new parent into account
> when the parent is updated while the clock is disabled. Set the update
> bit when enabling the clock to force an update of the mux.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---

Applied to clk-next