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

Dmitry Baryshkov
2022-04-13T17:25:43Z

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