Message-ID: <20221021093140.GC93287@thinkpad>
Date:   Fri, 21 Oct 2022 15:01:40 +0530
From:   Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
To:     Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
Cc:     andersson@...nel.org, viresh.kumar@...aro.org,
        krzysztof.kozlowski+dt@...aro.org, rafael@...nel.org,
        robh+dt@...nel.org, johan@...nel.org, devicetree@...r.kernel.org,
        linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-pm@...r.kernel.org
Subject: Re: [PATCH 3/4] cpufreq: qcom-hw: Add CPU clock provider support

On Thu, Oct 20, 2022 at 08:39:50AM +0300, Dmitry Baryshkov wrote:
> On 19/10/2022 16:59, Manivannan Sadhasivam wrote:
> > Qcom CPUFreq hardware (EPSS/OSM) controls clock and voltage to the CPU
> > cores. But this relationship is not represented with the clk framework
> > so far.
> > 
> > So, let's make the qcom-cpufreq-hw driver a clock provider. This makes the
> > clock producer/consumer relationship cleaner and is also useful for CPU
> > related frameworks like OPP to know the frequency at which the CPUs are
> > running.
> > 
> > The clock frequency provided by the driver is for each CPU policy. We
> > cannot get the frequency of each CPU core because, not all platforms
> > support per-core DCVS feature.
> > 
> > Also the frequency supplied by the driver is the actual frequency that
> > comes out of the EPSS/OSM block after the DCVS operation. This frequency is
> > not same as what the CPUFreq framework has set but it is the one that gets
> > supplied to the CPUs after throttling by LMh.
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
> > ---
> >   drivers/cpufreq/qcom-cpufreq-hw.c | 67 +++++++++++++++++++++++++++++--
> >   1 file changed, 63 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> > index a5b3b8d0e164..4dd710f9fb69 100644
> > --- a/drivers/cpufreq/qcom-cpufreq-hw.c
> > +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> > @@ -4,6 +4,7 @@
> >    */
> >   #include <linux/bitfield.h>
> > +#include <linux/clk-provider.h>
> >   #include <linux/cpufreq.h>
> >   #include <linux/init.h>
> >   #include <linux/interconnect.h>
> > @@ -54,6 +55,7 @@ struct qcom_cpufreq_data {
> >   	bool cancel_throttle;
> >   	struct delayed_work throttle_work;
> >   	struct cpufreq_policy *policy;
> > +	struct clk_hw cpu_clk;
> >   	bool per_core_dcvs;
> >   };
> > @@ -482,6 +484,54 @@ static void qcom_cpufreq_hw_lmh_exit(struct qcom_cpufreq_data *data)
> >   	free_irq(data->throttle_irq, data);
> >   }
> > +static unsigned long qcom_cpufreq_hw_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
> > +{
> > +	struct qcom_cpufreq_data *data = container_of(hw, struct qcom_cpufreq_data, cpu_clk);
> > +
> > +	return qcom_lmh_get_throttle_freq(data) / HZ_PER_KHZ;
> > +}
> > +
> > +static const struct clk_ops qcom_cpufreq_hw_clk_ops = {
> > +	.recalc_rate = qcom_cpufreq_hw_recalc_rate,
> > +};
> > +
> > +static int qcom_cpufreq_hw_clk_add(struct qcom_cpufreq_data *data, u32 index)
> > +{
> > +	struct platform_device *pdev = cpufreq_get_driver_data();
> > +	struct device *dev = &pdev->dev;
> > +	char *clk_name = devm_kasprintf(dev, GFP_KERNEL, "qcom_cpufreq%d", index);
> > +	static struct clk_init_data init = {};
> > +	int ret;
> > +
> > +	init.name = clk_name;
> > +	init.flags = CLK_GET_RATE_NOCACHE;
> > +	init.ops = &qcom_cpufreq_hw_clk_ops;
> > +	data->cpu_clk.init = &init;
> > +
> > +	ret = clk_hw_register(dev, &data->cpu_clk);
> > +	if (ret < 0) {
> > +		dev_err(dev, "Failed to register Qcom CPUFreq clock\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = of_clk_add_hw_provider(dev->of_node, of_clk_hw_simple_get, &data->cpu_clk);
> 
> This doesn't look corresponding to the DT bindings you are adding.
> of_clk_hw_simple_get() would return a single clock per dt node, whichever
> arguments were passed, while you are adding clocks correspoding to CPU
> clusters.
> 
> From what I see according to the bindings, you should register a single
> provider using the of_clk_hw_onecell_get() function.
> 

Well, that won't work either :( The detail that I missed in first place is
that the clock providers are added for the same DT node for each policy. So
there is a single clock under the clock provider for a policy but they all
belong to the same DT node.

This works when a clk provider gets added and then followed by "clk_get()"
(that's what happening during the ->init() callback). But each time a new
provider gets added, it is replacing the old for the same DT node.

The problem here is, we do not know how many policys are going to be there
during the probe time. I'll think about a proper solution and update.

Thanks,
Mani

> > +	if (ret < 0) {
> > +		dev_err(dev, "Failed to add Qcom CPUFreq clock provider\n");
> > +		clk_hw_unregister(&data->cpu_clk);
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static void qcom_cpufreq_hw_clk_remove(struct qcom_cpufreq_data *data)
> > +{
> > +	struct platform_device *pdev = cpufreq_get_driver_data();
> > +	struct device *dev = &pdev->dev;
> > +
> > +	of_clk_del_provider(dev->of_node);
> > +	clk_hw_unregister(&data->cpu_clk);
> > +}
> > +
> >   static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
> >   {
> >   	struct platform_device *pdev = cpufreq_get_driver_data();
> > @@ -556,19 +606,24 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
> >   	policy->driver_data = data;
> >   	policy->dvfs_possible_from_any_cpu = true;
> > +	ret = qcom_cpufreq_hw_clk_add(data, index);
> > +	if (ret) {
> > +		dev_err(dev, "Domain-%d failed to add CPU clock\n", index);
> > +		goto error;
> > +	}
> > +
> >   	ret = qcom_cpufreq_hw_read_lut(cpu_dev, policy);
> >   	if (ret) {
> >   		dev_err(dev, "Domain-%d failed to read LUT\n", index);
> > -		goto error;
> > +		goto clk_remove;
> >   	}
> >   	ret = dev_pm_opp_get_opp_count(cpu_dev);
> >   	if (ret <= 0) {
> >   		dev_err(cpu_dev, "Failed to add OPPs\n");
> >   		ret = -ENODEV;
> > -		goto error;
> > +		goto clk_remove;
> >   	}
> > -
> >   	if (policy_has_boost_freq(policy)) {
> >   		ret = cpufreq_enable_boost_support();
> >   		if (ret)
> > @@ -577,9 +632,12 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
> >   	ret = qcom_cpufreq_hw_lmh_init(policy, index);
> >   	if (ret)
> > -		goto error;
> > +		goto clk_remove;
> >   	return 0;
> > +
> > +clk_remove:
> > +	qcom_cpufreq_hw_clk_remove(data);
> >   error:
> >   	kfree(data);
> >   unmap_base:
> > @@ -599,6 +657,7 @@ static int qcom_cpufreq_hw_cpu_exit(struct cpufreq_policy *policy)
> >   	dev_pm_opp_remove_all_dynamic(cpu_dev);
> >   	dev_pm_opp_of_cpumask_remove_table(policy->related_cpus);
> >   	qcom_cpufreq_hw_lmh_exit(data);
> > +	qcom_cpufreq_hw_clk_remove(data);
> >   	kfree(policy->freq_table);
> >   	kfree(data);
> >   	iounmap(base);
> 
> -- 
> With best wishes
> Dmitry
> 

-- 
மணிவண்ணன் சதாசிவம்

Powered by blists - more mailing lists