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@xxxxxxxxxx>
> > ---
> > 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
> >
--
மணிவண்ணன் சதாசிவம்
