Message-ID: <b88de305-cb1f-7251-ccb8-4ea3b62bc322@linaro.org>
Date:   Thu, 20 Oct 2022 08:39:50 +0300
From:   Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To:     Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
        andersson@...nel.org, viresh.kumar@...aro.org,
        krzysztof.kozlowski+dt@...aro.org, rafael@...nel.org,
        robh+dt@...nel.org
Cc:     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 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.

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