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:   Wed, 9 Nov 2022 13:25:27 +0530
From:   Manivannan Sadhasivam <mani@kernel.org>
To:     Matthias Kaehlcke <mka@chromium.org>
Cc:     Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>,
        andersson@kernel.org, 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 v5 3/3] cpufreq: qcom-hw: Add CPU clock provider support
Message-ID: <20221109075527.GD4651@thinkpad>
References: <20221108154037.111794-1-manivannan.sadhasivam@linaro.org>
 <20221108154037.111794-4-manivannan.sadhasivam@linaro.org>
 <Y2qfmFVK665b1Nbw@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <Y2qfmFVK665b1Nbw@google.com>
Precedence: bulk

On Tue, Nov 08, 2022 at 06:27:36PM +0000, Matthias Kaehlcke wrote:
> Hi,
> 
> On Tue, Nov 08, 2022 at 09:10:37PM +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..86bb11de347f 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);
> > +}
> > +
> > +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;
> 
> nit: 'clk_name' isn't really needed, the result of devm_kasprintf() could be
> assigned directly to 'init.name'. 'init' could be renamed to 'clk_init' if
> the purpose of using 'clk_name' is to make clear that this is the name of a
> clock.
> 

Ack.

Thanks,
Mani

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

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