Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <cf95b1b4-194b-09c0-5bd4-80c962c18818@linaro.org>
Date:   Wed, 13 Apr 2022 21:24:25 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.8.0
Subject: Re: [PATCH v6 10/18] clk: qcom: krait-cc: drop hardcoded safe_sel
Content-Language: en-GB
To:     Ansuel Smith <ansuelsmth@gmail.com>
Cc:     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
References: <20220321231548.14276-1-ansuelsmth@gmail.com>
 <20220321231548.14276-11-ansuelsmth@gmail.com>
 <af104d82-34bc-2ab6-75bf-e54aafb3ae83@linaro.org>
 <625709ca.1c69fb81.b4268.12bc@mx.google.com>
From:   Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
In-Reply-To: <625709ca.1c69fb81.b4268.12bc@mx.google.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 13/04/2022 20:35, Ansuel Smith wrote:
> On Wed, Apr 13, 2022 at 08:25:31PM +0300, Dmitry Baryshkov wrote:
>> 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?
>>
> 
> No it can't but I think it would be better to have stuff that is based
> on real defined data instead of wrong struct that works just because a
> hardcoded value is used.
> 
> Example for the reason of using this hardcoded value, nobody notice that
> the mux list for the secondary mux was wrong. Now it didn't cause any
> problem as we use the secondary mux just to source out of qsb and we
> used the hardcoded value so the error was bypassed but as soon as the
> value was actually parsed from the defined table, bam secondary mux was
> sourcing out of pll8.
> 
> I honestly think that dropping the hardcoded value would make more clear
> what the safe sel is and what is referring to.

Can you define indices in the parents and use defined indice instead?

I see your point, but in my opinion adding an API to determine a 
compile-time constant value is a bit of overkill.

> 
>>>
>>> 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
> 


-- 
With best wishes
Dmitry