Re: [PATCH v6 10/18] clk: qcom: krait-cc: drop hardcoded safe_sel

From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
To: Ansuel Smith <ansuelsmth@gmail.com>,
	Rob Herring <robh+dt@kernel.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Andy Gross <agross@kernel.org>,
	Michael Turquette <mturquette@baylibre.com>,
	Stephen Boyd <sboyd@kernel.org>,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-arm-msm@vger.kernel.org, linux-clk@vger.kernel.org
Subject: Re: [PATCH v6 10/18] clk: qcom: krait-cc: drop hardcoded safe_sel
Date: Wed, 13 Apr 2022 20:25:31 +0300	[thread overview]
Message-ID: <af104d82-34bc-2ab6-75bf-e54aafb3ae83@linaro.org> (raw)
In-Reply-To: <20220321231548.14276-11-ansuelsmth@gmail.com>

On 22/03/2022 02:15, Ansuel Smith wrote:
> Drop hardcoded safe_sel definition and use helper to correctly calculate
> it. We assume qsb clk is always present as it should be declared in DTS
> per Documentation and in the absence of that, it's declared as a fixed
> clk.

Why? Can safe_sel (sec_mux index) change?

> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---
>   drivers/clk/qcom/krait-cc.c | 40 +++++++++++++++++++++++++------------
>   1 file changed, 27 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/clk/qcom/krait-cc.c b/drivers/clk/qcom/krait-cc.c
> index e9508e3104ea..5f98ee1c3681 100644
> --- a/drivers/clk/qcom/krait-cc.c
> +++ b/drivers/clk/qcom/krait-cc.c
> @@ -26,6 +26,17 @@ static unsigned int pri_mux_map[] = {
>   	0,
>   };
>   
> +static u8 krait_get_mux_sel(struct krait_mux_clk *mux, struct clk *safe_clk)
> +{
> +	struct clk_hw *safe_hw = __clk_get_hw(safe_clk);
> +
> +	/*
> +	 * We can ignore errors from clk_hw_get_index_of_parent()
> +	 * as we create these parents in this driver.
> +	 */
> +	return clk_hw_get_index_of_parent(&mux->hw, safe_hw);
> +}
> +
>   /*
>    * Notifier function for switching the muxes to safe parent
>    * while the hfpll is getting reprogrammed.
> @@ -116,8 +127,8 @@ krait_add_div(struct device *dev, int id, const char *s, unsigned int offset)
>   }
>   
>   static struct clk *
> -krait_add_sec_mux(struct device *dev, int id, const char *s,
> -		  unsigned int offset, bool unique_aux)
> +krait_add_sec_mux(struct device *dev, struct clk *qsb, int id,
> +		  const char *s, unsigned int offset, bool unique_aux)
>   {
>   	int ret;
>   	struct krait_mux_clk *mux;
> @@ -144,7 +155,6 @@ krait_add_sec_mux(struct device *dev, int id, const char *s,
>   	mux->shift = 2;
>   	mux->parent_map = sec_mux_map;
>   	mux->hw.init = &init;
> -	mux->safe_sel = 0;
>   
>   	init.name = kasprintf(GFP_KERNEL, "krait%s_sec_mux", s);
>   	if (!init.name)
> @@ -166,6 +176,7 @@ krait_add_sec_mux(struct device *dev, int id, const char *s,
>   	if (IS_ERR(clk))
>   		goto err_clk;
>   
> +	mux->safe_sel = krait_get_mux_sel(mux, qsb);
>   	ret = krait_notifier_register(dev, clk, mux);
>   	if (ret)
>   		clk = ERR_PTR(ret);
> @@ -204,7 +215,6 @@ krait_add_pri_mux(struct device *dev, struct clk *hfpll_div, struct clk *sec_mux
>   	mux->lpl = id >= 0;
>   	mux->parent_map = pri_mux_map;
>   	mux->hw.init = &init;
> -	mux->safe_sel = 2;
>   
>   	init.name = kasprintf(GFP_KERNEL, "krait%s_pri_mux", s);
>   	if (!init.name)
> @@ -226,6 +236,7 @@ krait_add_pri_mux(struct device *dev, struct clk *hfpll_div, struct clk *sec_mux
>   	if (IS_ERR(clk))
>   		goto err_clk;
>   
> +	mux->safe_sel = krait_get_mux_sel(mux, sec_mux);
>   	ret = krait_notifier_register(dev, clk, mux);
>   	if (ret)
>   		clk = ERR_PTR(ret);
> @@ -238,7 +249,9 @@ krait_add_pri_mux(struct device *dev, struct clk *hfpll_div, struct clk *sec_mux
>   }
>   
>   /* id < 0 for L2, otherwise id == physical CPU number */
> -static struct clk *krait_add_clks(struct device *dev, int id, bool unique_aux)
> +static struct clk *
> +krait_add_clks(struct device *dev, struct clk *qsb, int id,
> +	       bool unique_aux)
>   {
>   	unsigned int offset;
>   	void *p = NULL;
> @@ -261,7 +274,7 @@ static struct clk *krait_add_clks(struct device *dev, int id, bool unique_aux)
>   		goto err;
>   	}
>   
> -	sec_mux = krait_add_sec_mux(dev, id, s, offset, unique_aux);
> +	sec_mux = krait_add_sec_mux(dev, qsb, id, s, offset, unique_aux);
>   	if (IS_ERR(sec_mux)) {
>   		clk = sec_mux;
>   		goto err;
> @@ -301,18 +314,19 @@ static int krait_cc_probe(struct platform_device *pdev)
>   	int cpu;
>   	struct clk *clk;
>   	struct clk **clks;
> -	struct clk *l2_pri_mux_clk;
> +	struct clk *l2_pri_mux_clk, *qsb;
>   
>   	id = of_match_device(krait_cc_match_table, dev);
>   	if (!id)
>   		return -ENODEV;
>   
>   	/* Rate is 1 because 0 causes problems for __clk_mux_determine_rate */
> -	if (IS_ERR(clk_get(dev, "qsb")))
> -		clk = clk_register_fixed_rate(dev, "qsb", NULL, 0, 1);
> +	qsb = clk_get(dev, "qsb");
> +	if (IS_ERR(qsb))
> +		qsb = clk_register_fixed_rate(dev, "qsb", NULL, 0, 1);
>   
> -	if (IS_ERR(clk))
> -		return PTR_ERR(clk);
> +	if (IS_ERR(qsb))
> +		return PTR_ERR(qsb);
>   
>   	if (!id->data) {
>   		clk = clk_register_fixed_factor(dev, "acpu_aux",
> @@ -327,13 +341,13 @@ static int krait_cc_probe(struct platform_device *pdev)
>   		return -ENOMEM;
>   
>   	for_each_possible_cpu(cpu) {
> -		clk = krait_add_clks(dev, cpu, id->data);
> +		clk = krait_add_clks(dev, qsb, cpu, id->data);
>   		if (IS_ERR(clk))
>   			return PTR_ERR(clk);
>   		clks[cpu] = clk;
>   	}
>   
> -	l2_pri_mux_clk = krait_add_clks(dev, -1, id->data);
> +	l2_pri_mux_clk = krait_add_clks(dev, qsb, -1, id->data);
>   	if (IS_ERR(l2_pri_mux_clk))
>   		return PTR_ERR(l2_pri_mux_clk);
>   	clks[4] = l2_pri_mux_clk;

-- 
With best wishes
Dmitry