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;
Date:   Mon, 7 Nov 2022 16:30:55 -0600
From:   Bjorn Andersson <andersson@kernel.org>
To:     Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Cc:     viresh.kumar@linaro.org, krzysztof.kozlowski+dt@linaro.org,
        rafael@kernel.org, robh+dt@kernel.org, johan@kernel.org,
        devicetree@vger.kernel.org, linux-arm-msm@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org
Subject: Re: [PATCH v4 3/3] cpufreq: qcom-hw: Add CPU clock provider support
Message-ID: <20221107223055.k6bc7bp7djww5lxg@builder.lan>
References: <20221102090818.65321-1-manivannan.sadhasivam@linaro.org>
 <20221102090818.65321-4-manivannan.sadhasivam@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221102090818.65321-4-manivannan.sadhasivam@linaro.org>
Precedence: bulk

On Wed, Nov 02, 2022 at 02:38:18PM +0530, 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 frequency domain.
> 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 | 43 +++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
> 
> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> index 5e0598730a04..5b5f9a4d1466 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;
>  
> @@ -615,8 +617,20 @@ static struct cpufreq_driver cpufreq_qcom_hw_driver = {
>  	.ready		= qcom_cpufreq_ready,
>  };
>  
> +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;

Shouldn't this just be qcom_lmh_get_throttle_freq()? So that we get a
value in Hz.


I presume you got the division from qcom_lmh_dcvs_notify(), where
throttled_freq seems to supposed to be in kHz when passed to
topology_update_thermal_pressure(), as it contains a division by 1000
and a comment that it takes us to units of MHz.

But it's not clear what the frequency unit for freq_qos_update_request()
would be - but it's a 31 bit value, so better hope it's kHz there as
well(?)

Regards,
Bjorn

> +}
> +
> +static const struct clk_ops qcom_cpufreq_hw_clk_ops = {
> +	.recalc_rate = qcom_cpufreq_hw_recalc_rate,
> +};
> +
>  static int qcom_cpufreq_hw_driver_probe(struct platform_device *pdev)
>  {
> +	struct clk_hw_onecell_data *clk_data;
>  	struct device *dev = &pdev->dev;
>  	struct device *cpu_dev;
>  	struct clk *clk;
> @@ -659,8 +673,16 @@ static int qcom_cpufreq_hw_driver_probe(struct platform_device *pdev)
>  
>  	qcom_cpufreq.soc_data = of_device_get_match_data(dev);
>  
> +	clk_data = devm_kzalloc(dev, struct_size(clk_data, hws, num_domains), GFP_KERNEL);
> +	if (!clk_data)
> +		return -ENOMEM;
> +
> +	clk_data->num = num_domains;
> +
>  	for (i = 0; i < num_domains; i++) {
>  		struct qcom_cpufreq_data *data = &qcom_cpufreq.data[i];
> +		struct clk_init_data init = {};
> +		const char *clk_name;
>  		struct resource *res;
>  		void __iomem *base;
>  
> @@ -672,6 +694,27 @@ static int qcom_cpufreq_hw_driver_probe(struct platform_device *pdev)
>  
>  		data->base = base;
>  		data->res = res;
> +
> +		/* Register CPU clock for each frequency domain */
> +		clk_name = devm_kasprintf(dev, GFP_KERNEL, "qcom_cpufreq%d", i);
> +		init.name = clk_name;
> +		init.flags = CLK_GET_RATE_NOCACHE;
> +		init.ops = &qcom_cpufreq_hw_clk_ops;
> +		data->cpu_clk.init = &init;
> +
> +		ret = devm_clk_hw_register(dev, &data->cpu_clk);
> +		if (ret < 0) {
> +			dev_err(dev, "Failed to register Qcom CPUFreq clock\n");
> +			return ret;
> +		}
> +
> +		clk_data->hws[i] = &data->cpu_clk;
> +	}
> +
> +	ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, clk_data);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to add Qcom CPUFreq clock provider\n");
> +		return ret;
>  	}
>  
>  	ret = cpufreq_register_driver(&cpufreq_qcom_hw_driver);
> -- 
> 2.25.1
>