Re: [PATCH v2 3/4] clk: mediatek: mux: add clk notifier functions

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

Il 23/05/22 10:59, Chen-Yu Tsai ha scritto:
> With device frequency scaling, the mux clock that (indirectly) feeds the
> device selects between a dedicated PLL, and some other stable clocks.
> 
> When a clk rate change is requested, the (normally) upstream PLL is
> reconfigured. It's possible for the clock output of the PLL to become
> unstable during this process.
> 
> To avoid causing the device to glitch, the mux should temporarily be
> switched over to another "stable" clock during the PLL rate change.
> This is done with clk notifiers.
> 
> This patch adds common functions for notifiers to temporarily and
> transparently reparent mux clocks.
> 
> This was loosely based on commit 8adfb08605a9 ("clk: sunxi-ng: mux: Add
> clk notifier functions").
> 
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> ---
>   drivers/clk/mediatek/clk-mux.c | 42 ++++++++++++++++++++++++++++++++++
>   drivers/clk/mediatek/clk-mux.h | 15 ++++++++++++
>   2 files changed, 57 insertions(+)
> 
> diff --git a/drivers/clk/mediatek/clk-mux.c b/drivers/clk/mediatek/clk-mux.c
> index cd5f9fd8cb98..f84a5a753c09 100644
> --- a/drivers/clk/mediatek/clk-mux.c
> +++ b/drivers/clk/mediatek/clk-mux.c
> @@ -4,6 +4,7 @@
>    * Author: Owen Chen <owen.chen@mediatek.com>
>    */
>   
> +#include <linux/clk.h>
>   #include <linux/clk-provider.h>
>   #include <linux/compiler_types.h>
>   #include <linux/container_of.h>
> @@ -259,4 +260,45 @@ void mtk_clk_unregister_muxes(const struct mtk_mux *muxes, int num,
>   }
>   EXPORT_SYMBOL_GPL(mtk_clk_unregister_muxes);
>   
> +/*
> + * This clock notifier is called when the frequency of the of the parent
> + * PLL clock is to be changed. The idea is to switch the parent to a
> + * stable clock, such as the main oscillator, while the PLL frequency
> + * stabilizes.
> + */
> +static int mtk_clk_mux_notifier_cb(struct notifier_block *nb,
> +				   unsigned long event, void *_data)
> +{
> +	struct clk_notifier_data *data = _data;
> +	struct mtk_mux_nb *mux_nb = to_mtk_mux_nb(nb);
> +	const struct mtk_mux *mux = mux_nb->mux;
> +	struct clk_hw *hw;
> +	int ret = 0;
> +
> +	hw = __clk_get_hw(data->clk);
> +
> +	switch (event) {
> +	case PRE_RATE_CHANGE:
> +		mux_nb->original_index = mux->ops->get_parent(hw);
> +		ret = mux->ops->set_parent(hw, mux_nb->bypass_index);
> +		break;
> +
> +	case POST_RATE_CHANGE:
> +	case ABORT_RATE_CHANGE:

I agree with this change, entirely - but there's an issue here.
If we enter ABORT_RATE_CHANGE, this means that "something has failed": now,
what if the failure point was the PLL being unable to lock?

In that case, we would switch the parent back to a PLL that's not outputting
any clock, crashing the GPU, or a bogus rate, potentially undervolting the GPU.

I think that the best idea here would be to do something like..

	switch (event) {
	case PRE_RATE_CHANGE:
		mux_nb->old_parent_idx = mux->ops->get_parent(hw);
		ret = mux->ops->set_parent(hw, mux_nb->safe_parent_idx);
		break;
	case POST_RATE_CHANGE:
		ret = mux->ops->set_parent(hw, mux_nb->old_parent_idx);
		break;
	case ABORT_RATE_CHANGE:
		ret = -EINVAL; /* or -ECANCELED, whatever... */
		break;
	}

> +		ret = mux->ops->set_parent(hw, mux_nb->original_index);
> +		break;
> +	}
> +
> +	return notifier_from_errno(ret);
> +}
> +
> +int devm_mtk_clk_mux_notifier_register(struct device *dev, struct clk *clk,
> +				       struct mtk_mux_nb *mux_nb)
> +{
> +	mux_nb->nb.notifier_call = mtk_clk_mux_notifier_cb;
> +
> +	return devm_clk_notifier_register(dev, clk, &mux_nb->nb);
> +}
> +EXPORT_SYMBOL_GPL(devm_mtk_clk_mux_notifier_register);
> +
>   MODULE_LICENSE("GPL");
> diff --git a/drivers/clk/mediatek/clk-mux.h b/drivers/clk/mediatek/clk-mux.h
> index 6539c58f5d7d..506e91125a3d 100644
> --- a/drivers/clk/mediatek/clk-mux.h
> +++ b/drivers/clk/mediatek/clk-mux.h
> @@ -7,12 +7,14 @@
>   #ifndef __DRV_CLK_MTK_MUX_H
>   #define __DRV_CLK_MTK_MUX_H
>   
> +#include <linux/notifier.h>
>   #include <linux/spinlock.h>
>   #include <linux/types.h>
>   
>   struct clk;
>   struct clk_hw_onecell_data;
>   struct clk_ops;
> +struct device;
>   struct device_node;
>   
>   struct mtk_mux {
> @@ -89,4 +91,17 @@ int mtk_clk_register_muxes(const struct mtk_mux *muxes,
>   void mtk_clk_unregister_muxes(const struct mtk_mux *muxes, int num,
>   			      struct clk_hw_onecell_data *clk_data);
>   
> +struct mtk_mux_nb {
> +	struct notifier_block	nb;
> +	const struct mtk_mux	*mux;
> +
> +	u8	bypass_index;	/* Which parent to temporarily use */
> +	u8	original_index;	/* Set by notifier callback */

I think that the following names are more explanatory:

	u8	safe_parent_idx;
	u8	old_parent_idx;

...because I see this as a mechanism to switch the mux to a "safe" clock output
and then back to the PLL (like it's done on some qcom clocks as well).

You're free to ignore this comment, as this is, of course, just a personal opinion.

Cheers,
Angelo