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:   Tue, 8 Nov 2022 20:56:25 +0530
From:   Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
To:     Bjorn Andersson <andersson@kernel.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: <20221108152625.GB88842@thinkpad>
References: <20221102090818.65321-1-manivannan.sadhasivam@linaro.org>
 <20221102090818.65321-4-manivannan.sadhasivam@linaro.org>
 <20221107223055.k6bc7bp7djww5lxg@builder.lan>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20221107223055.k6bc7bp7djww5lxg@builder.lan>
Precedence: bulk

On Mon, Nov 07, 2022 at 04:30:55PM -0600, Bjorn Andersson wrote:
> 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.
> 

Right.

> 
> 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(?)
> 

No, I think it is better to use Hz. Let me fix it in next revision.

Thanks,
Mani

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

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