Re: [PATCH 3/4] cpufreq: qcom-hw: Add CPU clock provider support

Dmitry Baryshkov
2022-10-20T05:39:59Z

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@linaro.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