[v7,2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver

Commit Message

Taniya Das July 24, 2018, 10:42 a.m. UTC

The CPUfreq HW present in some QCOM chipsets offloads the steps necessary
for changing the frequency of CPUs. The driver implements the cpufreq
driver interface for this hardware engine.

Signed-off-by: Saravana Kannan <[e-mail]>
Signed-off-by: Taniya Das <[e-mail]>
---
 drivers/cpufreq/Kconfig.arm       |  11 ++
 drivers/cpufreq/Makefile          |   1 +
 drivers/cpufreq/qcom-cpufreq-hw.c | 348 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 360 insertions(+)
 create mode 100644 drivers/cpufreq/qcom-cpufreq-hw.c

--
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
of the Code Aurora Forum, hosted by the  Linux Foundation.

Comments

Evan Green Aug. 3, 2018, 7:40 p.m. UTC | #1

Hi Taniya,

On Tue, Jul 24, 2018 at 3:44 AM Taniya Das <[e-mail]> wrote:
>
> The CPUfreq HW present in some QCOM chipsets offloads the steps necessary
> for changing the frequency of CPUs. The driver implements the cpufreq
> driver interface for this hardware engine.
>
> Signed-off-by: Saravana Kannan <[e-mail]>
> Signed-off-by: Taniya Das <[e-mail]>
> ---
>  drivers/cpufreq/Kconfig.arm       |  11 ++
>  drivers/cpufreq/Makefile          |   1 +
>  drivers/cpufreq/qcom-cpufreq-hw.c | 348 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 360 insertions(+)
>  create mode 100644 drivers/cpufreq/qcom-cpufreq-hw.c
>
...
> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> new file mode 100644
> index 0000000..ea8f7d1
> --- /dev/null
> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
...
> +static int qcom_cpufreq_hw_read_lut(struct platform_device *pdev,
> +                                   struct cpufreq_qcom *c)
> +{
> +       struct device *dev = &pdev->dev;
> +       void __iomem *base;
> +       u32 data, src, lval, i, core_count, prev_cc, prev_freq, cur_freq;
> +
> +       c->table = devm_kcalloc(dev, LUT_MAX_ENTRIES + 1,
> +                               sizeof(*c->table), GFP_KERNEL);
> +       if (!c->table)
> +               return -ENOMEM;
> +
> +       base = c->reg_bases[REG_LUT_TABLE];
> +
> +       for (i = 0; i < LUT_MAX_ENTRIES; i++) {
> +               data = readl_relaxed(base + i * LUT_ROW_SIZE);
> +               src = (data & GENMASK(31, 30)) >> 30;
> +               lval = data & GENMASK(7, 0);
> +               core_count = CORE_COUNT_VAL(data);
> +
> +               if (src)
> +                       c->table[i].frequency = c->xo_rate * lval / 1000;
> +               else
> +                       c->table[i].frequency = INIT_RATE / 1000;

I don't know much about how this hardware works, but based on the
mask, src has 4 possible values. So does 0 mean INIT_RATE, and 1, 2,
and 3 all mean xo_rate?

Also, is INIT_RATE really constant? It sounds like gpll0 (or
gpll0_out_even?). You're already getting the xo clock, why not get
gpll0's real rate as well?

Saravana Kannan Aug. 3, 2018, 7:52 p.m. UTC | #2

On 2018-08-03 12:40, Evan Green wrote:
> Hi Taniya,
> 
> On Tue, Jul 24, 2018 at 3:44 AM Taniya Das <[e-mail]> wrote:
>> 
>> The CPUfreq HW present in some QCOM chipsets offloads the steps 
>> necessary
>> for changing the frequency of CPUs. The driver implements the cpufreq
>> driver interface for this hardware engine.
>> 
>> Signed-off-by: Saravana Kannan <[e-mail]>
>> Signed-off-by: Taniya Das <[e-mail]>
>> ---
>>  drivers/cpufreq/Kconfig.arm       |  11 ++
>>  drivers/cpufreq/Makefile          |   1 +
>>  drivers/cpufreq/qcom-cpufreq-hw.c | 348 
>> ++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 360 insertions(+)
>>  create mode 100644 drivers/cpufreq/qcom-cpufreq-hw.c
>> 
> ...
>> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c 
>> b/drivers/cpufreq/qcom-cpufreq-hw.c
>> new file mode 100644
>> index 0000000..ea8f7d1
>> --- /dev/null
>> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> ...
>> +static int qcom_cpufreq_hw_read_lut(struct platform_device *pdev,
>> +                                   struct cpufreq_qcom *c)
>> +{
>> +       struct device *dev = &pdev->dev;
>> +       void __iomem *base;
>> +       u32 data, src, lval, i, core_count, prev_cc, prev_freq, 
>> cur_freq;
>> +
>> +       c->table = devm_kcalloc(dev, LUT_MAX_ENTRIES + 1,
>> +                               sizeof(*c->table), GFP_KERNEL);
>> +       if (!c->table)
>> +               return -ENOMEM;
>> +
>> +       base = c->reg_bases[REG_LUT_TABLE];
>> +
>> +       for (i = 0; i < LUT_MAX_ENTRIES; i++) {
>> +               data = readl_relaxed(base + i * LUT_ROW_SIZE);
>> +               src = (data & GENMASK(31, 30)) >> 30;
>> +               lval = data & GENMASK(7, 0);
>> +               core_count = CORE_COUNT_VAL(data);
>> +
>> +               if (src)
>> +                       c->table[i].frequency = c->xo_rate * lval / 
>> 1000;
>> +               else
>> +                       c->table[i].frequency = INIT_RATE / 1000;
> 
> I don't know much about how this hardware works, but based on the
> mask, src has 4 possible values. So does 0 mean INIT_RATE, and 1, 2,
> and 3 all mean xo_rate?
> 
> Also, is INIT_RATE really constant? It sounds like gpll0 (or
> gpll0_out_even?). You're already getting the xo clock, why not get
> gpll0's real rate as well?

Actually I was about to comment and say NOT to get clocks just to get 
their rate. The XO_RATE is just a multiplication factor. This HW/FW can 
change in the future and make the multiplication factor to 1KHz. We also 
can't really control any of the clocks going to this block from Linux 
(it's all locked down). Adding clk_get significantly delays when this 
driver can be probed and increases boot up time. The INIT_RATE will 
always be 300 MHz independent of what source it's coming from (as in, if 
GPLL0 can't give 300, the HW design will be so that we'll find a 
different source).

So, I'd like to remove any clock bindings for this driver.

Thanks,
Saravana

Stephen Boyd Aug. 3, 2018, 10:24 p.m. UTC | #3

Quoting [e-mail] (2018-08-03 12:52:48)
> On 2018-08-03 12:40, Evan Green wrote:
> > Hi Taniya,
> > 
> > On Tue, Jul 24, 2018 at 3:44 AM Taniya Das <[e-mail]> wrote:
> >> 
> >> +               if (src)
> >> +                       c->table[i].frequency = c->xo_rate * lval / 
> >> 1000;
> >> +               else
> >> +                       c->table[i].frequency = INIT_RATE / 1000;
> > 
> > I don't know much about how this hardware works, but based on the
> > mask, src has 4 possible values. So does 0 mean INIT_RATE, and 1, 2,
> > and 3 all mean xo_rate?
> > 
> > Also, is INIT_RATE really constant? It sounds like gpll0 (or
> > gpll0_out_even?). You're already getting the xo clock, why not get
> > gpll0's real rate as well?
> 
> Actually I was about to comment and say NOT to get clocks just to get 
> their rate. The XO_RATE is just a multiplication factor. This HW/FW can 
> change in the future and make the multiplication factor to 1KHz.

So future changes to this hardware are going to make this register
return the final frequency that we should use? Sounds great! But that
isn't how it's working right now. We need to have XO in the binding here
so the driver can work with different XO frequencies in case that ever
happens. Same story for GPLL0. Hardcoding this stuff in the driver just
means we'll have to swizzle things in the driver when it changes.

> We also 
> can't really control any of the clocks going to this block from Linux 
> (it's all locked down).

This shouldn't matter. The clocks going to this hardware block are
described by the firmware to the OS by means of the DT node. If the
firmware or the hardware decides to change the input clks then the
binding can have different clk nodes used.

> Adding clk_get significantly delays when this 
> driver can be probed and increases boot up time.

Huh? Please fix your bootloader to increase the CPU frequency before
booting the kernel. I really doubt probe defer is going to happen if we
send GPLL0 here (XO is a DT clk so it's definitely registered before
this driver), and trying to work around probe defer because the CPU
won't go fast before that is papering over a larger problem with both
probe defer and bootloaders not supplying enough CPU speed for you.

> The INIT_RATE will 
> always be 300 MHz independent of what source it's coming from (as in, if 
> GPLL0 can't give 300, the HW design will be so that we'll find a 
> different source).
> 
> So, I'd like to remove any clock bindings for this driver.

No. Bindings describe how the hardware is connected. Please don't remove
clocks from the binding just because probe defer is a concern.

Saravana Kannan Aug. 6, 2018, 8:46 p.m. UTC | #4

On 2018-08-03 15:24, Stephen Boyd wrote:
> Quoting [e-mail] (2018-08-03 12:52:48)
>> On 2018-08-03 12:40, Evan Green wrote:
>> > Hi Taniya,
>> >
>> > On Tue, Jul 24, 2018 at 3:44 AM Taniya Das <[e-mail]> wrote:
>> >>
>> >> +               if (src)
>> >> +                       c->table[i].frequency = c->xo_rate * lval /
>> >> 1000;
>> >> +               else
>> >> +                       c->table[i].frequency = INIT_RATE / 1000;
>> >
>> > I don't know much about how this hardware works, but based on the
>> > mask, src has 4 possible values. So does 0 mean INIT_RATE, and 1, 2,
>> > and 3 all mean xo_rate?
>> >
>> > Also, is INIT_RATE really constant? It sounds like gpll0 (or
>> > gpll0_out_even?). You're already getting the xo clock, why not get
>> > gpll0's real rate as well?
>> 
>> Actually I was about to comment and say NOT to get clocks just to get
>> their rate. The XO_RATE is just a multiplication factor. This HW/FW 
>> can
>> change in the future and make the multiplication factor to 1KHz.
> 
> So future changes to this hardware are going to make this register
> return the final frequency that we should use? Sounds great! But that
> isn't how it's working right now. We need to have XO in the binding 
> here
> so the driver can work with different XO frequencies in case that ever
> happens. Same story for GPLL0. Hardcoding this stuff in the driver just
> means we'll have to swizzle things in the driver when it changes.

XO being a different frequency in a Qualcomm chip is way way less likely 
than anything else we are discussing here. In the 10+years I've been 
there this has never changed.

So, how about making this binding optional? If it's not present we can 
make assumptions for XO rate and init rate. That'll handle your 
hypothetical use case while also being optimized.

>> We also
>> can't really control any of the clocks going to this block from Linux
>> (it's all locked down).
> 
> This shouldn't matter. The clocks going to this hardware block are
> described by the firmware to the OS by means of the DT node. If the
> firmware or the hardware decides to change the input clks then the
> binding can have different clk nodes used.

There are tons of clocks that are input to blocks in an SoC that are 
never controlled by Linux. Or are expose in DT because they are set up 
ahead of time and can't change. So, why make an exception here?

>> Adding clk_get significantly delays when this
>> driver can be probed and increases boot up time.
> 
> Huh? Please fix your bootloader to increase the CPU frequency before
> booting the kernel. I really doubt probe defer is going to happen if we
> send GPLL0 here (XO is a DT clk so it's definitely registered before
> this driver), and trying to work around probe defer because the CPU
> won't go fast before that is papering over a larger problem with both
> probe defer and bootloaders not supplying enough CPU speed for you.

There are multiple reason the bootloader can't run the CPU faster. For 
one, it doesn't have thermal handling, etc. And adding all those 
features into the bootloader doesn't make sense when Linux already has 
support for it.

>> The INIT_RATE will
>> always be 300 MHz independent of what source it's coming from (as in, 
>> if
>> GPLL0 can't give 300, the HW design will be so that we'll find a
>> different source).
>> 
>> So, I'd like to remove any clock bindings for this driver.
> 
> No. Bindings describe how the hardware is connected. Please don't 
> remove
> clocks from the binding just because probe defer is a concern.

Binding describes hardware controllable by the OS. That's the reality. 
Let's not add mandatory clock bindings for clocks that the OS can't do 
anything about.

-Saravana

Stephen Boyd Aug. 8, 2018, 6:22 a.m. UTC | #5

Quoting [e-mail] (2018-08-06 13:46:05)
> On 2018-08-03 15:24, Stephen Boyd wrote:
> > Quoting [e-mail] (2018-08-03 12:52:48)
> >> On 2018-08-03 12:40, Evan Green wrote:
> >> > Hi Taniya,
> >> >
> >> > On Tue, Jul 24, 2018 at 3:44 AM Taniya Das <[e-mail]> wrote:
> >> >>
> >> >> +               if (src)
> >> >> +                       c->table[i].frequency = c->xo_rate * lval /
> >> >> 1000;
> >> >> +               else
> >> >> +                       c->table[i].frequency = INIT_RATE / 1000;
> >> >
> >> > I don't know much about how this hardware works, but based on the
> >> > mask, src has 4 possible values. So does 0 mean INIT_RATE, and 1, 2,
> >> > and 3 all mean xo_rate?
> >> >
> >> > Also, is INIT_RATE really constant? It sounds like gpll0 (or
> >> > gpll0_out_even?). You're already getting the xo clock, why not get
> >> > gpll0's real rate as well?
> >> 
> >> Actually I was about to comment and say NOT to get clocks just to get
> >> their rate. The XO_RATE is just a multiplication factor. This HW/FW 
> >> can
> >> change in the future and make the multiplication factor to 1KHz.
> > 
> > So future changes to this hardware are going to make this register
> > return the final frequency that we should use? Sounds great! But that
> > isn't how it's working right now. We need to have XO in the binding 
> > here
> > so the driver can work with different XO frequencies in case that ever
> > happens. Same story for GPLL0. Hardcoding this stuff in the driver just
> > means we'll have to swizzle things in the driver when it changes.
> 
> XO being a different frequency in a Qualcomm chip is way way less likely 
> than anything else we are discussing here. In the 10+years I've been 
> there this has never changed.
> 
> So, how about making this binding optional? If it's not present we can 
> make assumptions for XO rate and init rate. That'll handle your 
> hypothetical use case while also being optimized.

Optional properties are almost never correct for clks. Either the clk
goes there or it doesn't go there. The only time it's optional is if the
HW has the choice of generating the clk itself internally or to use an
external clk as input.

In this case, it's not optional, it's actually used so marking it
optional is highly confusing.

> 
> >> We also
> >> can't really control any of the clocks going to this block from Linux
> >> (it's all locked down).
> > 
> > This shouldn't matter. The clocks going to this hardware block are
> > described by the firmware to the OS by means of the DT node. If the
> > firmware or the hardware decides to change the input clks then the
> > binding can have different clk nodes used.
> 
> There are tons of clocks that are input to blocks in an SoC that are 
> never controlled by Linux. Or are expose in DT because they are set up 
> ahead of time and can't change. So, why make an exception here?

Because the driver doesn't have to hard-code some frequency that can
easily come from the DT, making it more flexible for frequency plan
changes. It doesn't matter about control of clks at all here, so this
comparison is just plain wrong.

> 
> >> The INIT_RATE will
> >> always be 300 MHz independent of what source it's coming from (as in, 
> >> if
> >> GPLL0 can't give 300, the HW design will be so that we'll find a
> >> different source).
> >> 
> >> So, I'd like to remove any clock bindings for this driver.
> > 
> > No. Bindings describe how the hardware is connected. Please don't 
> > remove
> > clocks from the binding just because probe defer is a concern.
> 
> Binding describes hardware controllable by the OS. That's the reality. 
> Let's not add mandatory clock bindings for clocks that the OS can't do 
> anything about.
> 

It seems that you believe clks should only be used to turn on/off and
control rates. That is not the whole truth. Sometimes clks are there
just to express the clk frequencies that are present in the design so
that drivers can figure out what to do.

Taniya Das Aug. 8, 2018, 10:15 a.m. UTC | #6

On 8/8/2018 11:52 AM, Stephen Boyd wrote:
> Quoting [e-mail] (2018-08-06 13:46:05)
>> On 2018-08-03 15:24, Stephen Boyd wrote:
>>> Quoting [e-mail] (2018-08-03 12:52:48)
>>>> On 2018-08-03 12:40, Evan Green wrote:
>>>>> Hi Taniya,
>>>>>
>>>>> On Tue, Jul 24, 2018 at 3:44 AM Taniya Das <[e-mail]> wrote:
>>>>>>
>>>>>> +               if (src)
>>>>>> +                       c->table[i].frequency = c->xo_rate * lval /
>>>>>> 1000;
>>>>>> +               else
>>>>>> +                       c->table[i].frequency = INIT_RATE / 1000;
>>>>>
>>>>> I don't know much about how this hardware works, but based on the
>>>>> mask, src has 4 possible values. So does 0 mean INIT_RATE, and 1, 2,
>>>>> and 3 all mean xo_rate?
>>>>>
>>>>> Also, is INIT_RATE really constant? It sounds like gpll0 (or
>>>>> gpll0_out_even?). You're already getting the xo clock, why not get
>>>>> gpll0's real rate as well?
>>>>
>>>> Actually I was about to comment and say NOT to get clocks just to get
>>>> their rate. The XO_RATE is just a multiplication factor. This HW/FW
>>>> can
>>>> change in the future and make the multiplication factor to 1KHz.
>>>
>>> So future changes to this hardware are going to make this register
>>> return the final frequency that we should use? Sounds great! But that
>>> isn't how it's working right now. We need to have XO in the binding
>>> here
>>> so the driver can work with different XO frequencies in case that ever
>>> happens. Same story for GPLL0. Hardcoding this stuff in the driver just
>>> means we'll have to swizzle things in the driver when it changes.
>>
>> XO being a different frequency in a Qualcomm chip is way way less likely
>> than anything else we are discussing here. In the 10+years I've been
>> there this has never changed.
>>
>> So, how about making this binding optional? If it's not present we can
>> make assumptions for XO rate and init rate. That'll handle your
>> hypothetical use case while also being optimized.
> 
> Optional properties are almost never correct for clks. Either the clk
> goes there or it doesn't go there. The only time it's optional is if the
> HW has the choice of generating the clk itself internally or to use an
> external clk as input.
> 
> In this case, it's not optional, it's actually used so marking it
> optional is highly confusing.
> 
>>
>>>> We also
>>>> can't really control any of the clocks going to this block from Linux
>>>> (it's all locked down).
>>>
>>> This shouldn't matter. The clocks going to this hardware block are
>>> described by the firmware to the OS by means of the DT node. If the
>>> firmware or the hardware decides to change the input clks then the
>>> binding can have different clk nodes used.
>>
>> There are tons of clocks that are input to blocks in an SoC that are
>> never controlled by Linux. Or are expose in DT because they are set up
>> ahead of time and can't change. So, why make an exception here?
> 
> Because the driver doesn't have to hard-code some frequency that can
> easily come from the DT, making it more flexible for frequency plan
> changes. It doesn't matter about control of clks at all here, so this
> comparison is just plain wrong.
> 
>>
>>>> The INIT_RATE will
>>>> always be 300 MHz independent of what source it's coming from (as in,
>>>> if
>>>> GPLL0 can't give 300, the HW design will be so that we'll find a
>>>> different source).
>>>>
>>>> So, I'd like to remove any clock bindings for this driver.
>>>
>>> No. Bindings describe how the hardware is connected. Please don't
>>> remove
>>> clocks from the binding just because probe defer is a concern.
>>
>> Binding describes hardware controllable by the OS. That's the reality.
>> Let's not add mandatory clock bindings for clocks that the OS can't do
>> anything about.
>>
> 
> It seems that you believe clks should only be used to turn on/off and
> control rates. That is not the whole truth. Sometimes clks are there
> just to express the clk frequencies that are present in the design so
> that drivers can figure out what to do.
> 

Stephen,

As this clock is not configurable by linux clock drivers and we really 
do not care the parent src(as mentioned by Saravana) to generate the 
300MHz, would it be good to define a fixed rate clock so as to express 
the HW connectivity & frequency?

Stephen Boyd Aug. 23, 2018, 6:38 p.m. UTC | #7

Quoting Taniya Das (2018-08-08 03:15:26)
> 
> 
> On 8/8/2018 11:52 AM, Stephen Boyd wrote:
> >>
> >> Binding describes hardware controllable by the OS. That's the reality.
> >> Let's not add mandatory clock bindings for clocks that the OS can't do
> >> anything about.
> >>
> > 
> > It seems that you believe clks should only be used to turn on/off and
> > control rates. That is not the whole truth. Sometimes clks are there
> > just to express the clk frequencies that are present in the design so
> > that drivers can figure out what to do.
> > 
> 
> Stephen,
> 
> As this clock is not configurable by linux clock drivers and we really 
> do not care the parent src(as mentioned by Saravana) to generate the 
> 300MHz, would it be good to define a fixed rate clock so as to express 
> the HW connectivity & frequency?
> 

As a hack that works great, but why do we need to workaround problems by
adding a fixed rate clk to DT for this PLL? The PLL is provided by GCC
node so it should be connected to the GCC node.

Matthias Kaehlcke Aug. 29, 2018, 6:01 p.m. UTC | #8

Hi Taniya,

On Tue, Jul 24, 2018 at 04:12:50PM +0530, Taniya Das wrote:
> The CPUfreq HW present in some QCOM chipsets offloads the steps necessary
> for changing the frequency of CPUs. The driver implements the cpufreq
> driver interface for this hardware engine.
> 
> Signed-off-by: Saravana Kannan <[e-mail]>
> Signed-off-by: Taniya Das <[e-mail]>
> ---
>  drivers/cpufreq/Kconfig.arm       |  11 ++
>  drivers/cpufreq/Makefile          |   1 +
>  drivers/cpufreq/qcom-cpufreq-hw.c | 348 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 360 insertions(+)
>  create mode 100644 drivers/cpufreq/qcom-cpufreq-hw.c
> 
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index 0cd8eb7..93a9d72 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -298,3 +298,14 @@ config ARM_PXA2xx_CPUFREQ
>  	  This add the CPUFreq driver support for Intel PXA2xx SOCs.
> 
>  	  If in doubt, say N.
> +
> +config ARM_QCOM_CPUFREQ_HW
> +	bool "QCOM CPUFreq HW driver"
> +	depends on ARCH_QCOM
> +	help
> +	 Support for the CPUFreq HW driver.
> +	 Some QCOM chipsets have a HW engine to offload the steps
> +	 necessary for changing the frequency of the CPUs. Firmware loaded
> +	 in this engine exposes a programming interface to the OS.
> +	 The driver implements the cpufreq interface for this HW engine.
> +	 Say Y if you want to support CPUFreq HW.
> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> index c1ffeab..ca48a1d 100644
> --- a/drivers/cpufreq/Makefile
> +++ b/drivers/cpufreq/Makefile
> @@ -85,6 +85,7 @@ obj-$(CONFIG_ARM_TEGRA124_CPUFREQ)	+= tegra124-cpufreq.o
>  obj-$(CONFIG_ARM_TEGRA186_CPUFREQ)	+= tegra186-cpufreq.o
>  obj-$(CONFIG_ARM_TI_CPUFREQ)		+= ti-cpufreq.o
>  obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ)	+= vexpress-spc-cpufreq.o
> +obj-$(CONFIG_ARM_QCOM_CPUFREQ_HW)	+= qcom-cpufreq-hw.o
> 
> 
>  ##################################################################################
> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> new file mode 100644
> index 0000000..ea8f7d1
> --- /dev/null
> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> @@ -0,0 +1,348 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/cpufreq.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_platform.h>
> +
> +#define INIT_RATE			300000000UL
> +#define LUT_MAX_ENTRIES			40U
> +#define CORE_COUNT_VAL(val)		(((val) & (GENMASK(18, 16))) >> 16)
> +#define LUT_ROW_SIZE			32
> +
> +enum {
> +	REG_ENABLE,
> +	REG_LUT_TABLE,
> +	REG_PERF_STATE,
> +
> +	REG_ARRAY_SIZE,
> +};
> +
> +struct cpufreq_qcom {
> +	struct cpufreq_frequency_table *table;
> +	struct device *dev;

'dev' is not used and can be removed.

> ...
> 
> +static int qcom_cpu_resources_init(struct platform_device *pdev,
> +				   struct device_node *np, unsigned int cpu,
> +				   unsigned long xo_rate)
> +{
> +	struct cpufreq_qcom *c;
> +	struct resource res;
> +	struct device *dev = &pdev->dev;
> +	const u16 *offsets;
> +	int ret, i, cpu_first, cpu_r;
> +	void __iomem *base;
> +
> +	if (qcom_freq_domain_map[cpu])
> +		return 0;
> +
> +	c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
> +	if (!c)
> +		return -ENOMEM;
> +
> +	offsets = of_device_get_match_data(&pdev->dev);
> +	if (!offsets)
> +		return -EINVAL;
> +
> +	if (of_address_to_resource(np, 0, &res))
> +		return -ENOMEM;
> +
> +	base = devm_ioremap_resource(dev, &res);
> +	if (!base)
> +		return -ENOMEM;
> +
> +	for (i = REG_ENABLE; i < REG_ARRAY_SIZE; i++)
> +		c->reg_bases[i] = base + offsets[i];
> +
> +	/* HW should be in enabled state to proceed */
> +	if (!(readl_relaxed(c->reg_bases[REG_ENABLE]) & 0x1)) {
> +		dev_err(dev, "%s cpufreq hardware not enabled\n", np->name);
> +		return -ENODEV;
> +	}
> +
> +	ret = qcom_get_related_cpus(np, &c->related_cpus);
> +	if (ret) {
> +		dev_err(dev, "%s failed to get related CPUs\n", np->name);
> +		return ret;
> +	}
> +
> +	c->max_cores = cpumask_weight(&c->related_cpus);
> +	if (!c->max_cores)
> +		return -ENOENT;
> +
> +	c->xo_rate = xo_rate;
> +
> +	ret = qcom_cpufreq_hw_read_lut(pdev, c);
> +	if (ret) {
> +		dev_err(dev, "%s failed to read LUT\n", np->name);
> +		return ret;
> +	}
> +
> +	qcom_freq_domain_map[cpu] = c;
> +
> +	/* Related CPUs */
> +	cpu_first = cpumask_first(&c->related_cpus);
> +
> +	for_each_cpu(cpu_r, &c->related_cpus) {
> +		if (cpu_r != cpu_first)
> +			qcom_freq_domain_map[cpu_r] =
> +				qcom_freq_domain_map[cpu_first];
> +	}

The above ten lines could be simplified to:

	for_each_cpu(cpu_r, &c->related_cpus)
		qcom_freq_domain_map[cpu_r] = c;

> ...
> 
> +static int __init qcom_cpufreq_hw_init(void)
> +{
> +	return platform_driver_register(&qcom_cpufreq_hw_driver);
> +}
> +subsys_initcall(qcom_cpufreq_hw_init);

Is subsys_initcall used for a particular reason? It will cause
problems when registering cooling devices, since the thermal device
class is initialized through an fs_initcall, which are executed
later.

Most cpufreq drivers use module_init, device_initcall or
late_initcall, can't this driver use one of those?

Cheers

Matthias

Amit Kucheria Sept. 9, 2018, 2:34 p.m. UTC | #9

Hi Taniya,

How much have you stressed this driver?

I tried it on top of an integration branch based on 4.19-rc2 that we
maintain[1] and was able to get the board to reboot fairly easily with
just a few "yes > /dev/null &" instances running in the background.

I even tried with interconnect framework disabled to be sure, and it
still reboots.

I'll continue trying to pinpoint the source of the problem, but it
would be nice to know what tree you're testing against.

Regards,
Amit
[1] https://git.linaro.org/people/amit.kucheria/kernel.git/log/?h=integration/qcomlt-automerge-result

On Tue, Jul 24, 2018 at 4:12 PM, Taniya Das <[e-mail]> wrote:
> The CPUfreq HW present in some QCOM chipsets offloads the steps necessary
> for changing the frequency of CPUs. The driver implements the cpufreq
> driver interface for this hardware engine.
>
> Signed-off-by: Saravana Kannan <[e-mail]>
> Signed-off-by: Taniya Das <[e-mail]>
> ---
>  drivers/cpufreq/Kconfig.arm       |  11 ++
>  drivers/cpufreq/Makefile          |   1 +
>  drivers/cpufreq/qcom-cpufreq-hw.c | 348 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 360 insertions(+)
>  create mode 100644 drivers/cpufreq/qcom-cpufreq-hw.c
>
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index 0cd8eb7..93a9d72 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -298,3 +298,14 @@ config ARM_PXA2xx_CPUFREQ
>           This add the CPUFreq driver support for Intel PXA2xx SOCs.
>
>           If in doubt, say N.
> +
> +config ARM_QCOM_CPUFREQ_HW
> +       bool "QCOM CPUFreq HW driver"
> +       depends on ARCH_QCOM
> +       help
> +        Support for the CPUFreq HW driver.
> +        Some QCOM chipsets have a HW engine to offload the steps
> +        necessary for changing the frequency of the CPUs. Firmware loaded
> +        in this engine exposes a programming interface to the OS.
> +        The driver implements the cpufreq interface for this HW engine.
> +        Say Y if you want to support CPUFreq HW.
> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> index c1ffeab..ca48a1d 100644
> --- a/drivers/cpufreq/Makefile
> +++ b/drivers/cpufreq/Makefile
> @@ -85,6 +85,7 @@ obj-$(CONFIG_ARM_TEGRA124_CPUFREQ)    += tegra124-cpufreq.o
>  obj-$(CONFIG_ARM_TEGRA186_CPUFREQ)     += tegra186-cpufreq.o
>  obj-$(CONFIG_ARM_TI_CPUFREQ)           += ti-cpufreq.o
>  obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ) += vexpress-spc-cpufreq.o
> +obj-$(CONFIG_ARM_QCOM_CPUFREQ_HW)      += qcom-cpufreq-hw.o
>
>
>  ##################################################################################
> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> new file mode 100644
> index 0000000..ea8f7d1
> --- /dev/null
> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> @@ -0,0 +1,348 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/cpufreq.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_platform.h>
> +
> +#define INIT_RATE                      300000000UL
> +#define LUT_MAX_ENTRIES                        40U
> +#define CORE_COUNT_VAL(val)            (((val) & (GENMASK(18, 16))) >> 16)
> +#define LUT_ROW_SIZE                   32
> +
> +enum {
> +       REG_ENABLE,
> +       REG_LUT_TABLE,
> +       REG_PERF_STATE,
> +
> +       REG_ARRAY_SIZE,
> +};
> +
> +struct cpufreq_qcom {
> +       struct cpufreq_frequency_table *table;
> +       struct device *dev;
> +       void __iomem *reg_bases[REG_ARRAY_SIZE];
> +       cpumask_t related_cpus;
> +       unsigned int max_cores;
> +       unsigned long xo_rate;
> +};
> +
> +static const u16 cpufreq_qcom_std_offsets[REG_ARRAY_SIZE] = {
> +       [REG_ENABLE]            = 0x0,
> +       [REG_LUT_TABLE]         = 0x110,
> +       [REG_PERF_STATE]        = 0x920,
> +};
> +
> +static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
> +
> +static int
> +qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
> +                            unsigned int index)
> +{
> +       struct cpufreq_qcom *c = policy->driver_data;
> +
> +       writel_relaxed(index, c->reg_bases[REG_PERF_STATE]);
> +
> +       return 0;
> +}
> +
> +static unsigned int qcom_cpufreq_hw_get(unsigned int cpu)
> +{
> +       struct cpufreq_qcom *c;
> +       struct cpufreq_policy *policy;
> +       unsigned int index;
> +
> +       policy = cpufreq_cpu_get_raw(cpu);
> +       if (!policy)
> +               return 0;
> +
> +       c = policy->driver_data;
> +
> +       index = readl_relaxed(c->reg_bases[REG_PERF_STATE]);
> +       index = min(index, LUT_MAX_ENTRIES - 1);
> +
> +       return policy->freq_table[index].frequency;
> +}
> +
> +static unsigned int
> +qcom_cpufreq_hw_fast_switch(struct cpufreq_policy *policy,
> +                           unsigned int target_freq)
> +{
> +       struct cpufreq_qcom *c = policy->driver_data;
> +       int index;
> +
> +       index = policy->cached_resolved_idx;
> +       if (index < 0)
> +               return 0;
> +
> +       writel_relaxed(index, c->reg_bases[REG_PERF_STATE]);
> +
> +       return policy->freq_table[index].frequency;
> +}
> +
> +static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
> +{
> +       struct cpufreq_qcom *c;
> +
> +       c = qcom_freq_domain_map[policy->cpu];
> +       if (!c) {
> +               pr_err("No scaling support for CPU%d\n", policy->cpu);
> +               return -ENODEV;
> +       }
> +
> +       cpumask_copy(policy->cpus, &c->related_cpus);
> +
> +       policy->fast_switch_possible = true;
> +       policy->freq_table = c->table;
> +       policy->driver_data = c;
> +
> +       return 0;
> +}
> +
> +static struct freq_attr *qcom_cpufreq_hw_attr[] = {
> +       &cpufreq_freq_attr_scaling_available_freqs,
> +       &cpufreq_freq_attr_scaling_boost_freqs,
> +       NULL
> +};
> +
> +static struct cpufreq_driver cpufreq_qcom_hw_driver = {
> +       .flags          = CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK |
> +                         CPUFREQ_HAVE_GOVERNOR_PER_POLICY,
> +       .verify         = cpufreq_generic_frequency_table_verify,
> +       .target_index   = qcom_cpufreq_hw_target_index,
> +       .get            = qcom_cpufreq_hw_get,
> +       .init           = qcom_cpufreq_hw_cpu_init,
> +       .fast_switch    = qcom_cpufreq_hw_fast_switch,
> +       .name           = "qcom-cpufreq-hw",
> +       .attr           = qcom_cpufreq_hw_attr,
> +       .boost_enabled  = true,
> +};
> +
> +static int qcom_cpufreq_hw_read_lut(struct platform_device *pdev,
> +                                   struct cpufreq_qcom *c)
> +{
> +       struct device *dev = &pdev->dev;
> +       void __iomem *base;
> +       u32 data, src, lval, i, core_count, prev_cc, prev_freq, cur_freq;
> +
> +       c->table = devm_kcalloc(dev, LUT_MAX_ENTRIES + 1,
> +                               sizeof(*c->table), GFP_KERNEL);
> +       if (!c->table)
> +               return -ENOMEM;
> +
> +       base = c->reg_bases[REG_LUT_TABLE];
> +
> +       for (i = 0; i < LUT_MAX_ENTRIES; i++) {
> +               data = readl_relaxed(base + i * LUT_ROW_SIZE);
> +               src = (data & GENMASK(31, 30)) >> 30;
> +               lval = data & GENMASK(7, 0);
> +               core_count = CORE_COUNT_VAL(data);
> +
> +               if (src)
> +                       c->table[i].frequency = c->xo_rate * lval / 1000;
> +               else
> +                       c->table[i].frequency = INIT_RATE / 1000;
> +
> +               cur_freq = c->table[i].frequency;
> +
> +               dev_dbg(dev, "index=%d freq=%d, core_count %d\n",
> +                       i, c->table[i].frequency, core_count);
> +
> +               if (core_count != c->max_cores)
> +                       cur_freq = CPUFREQ_ENTRY_INVALID;
> +
> +               /*
> +                * Two of the same frequencies with the same core counts means
> +                * end of table.
> +                */
> +               if (i > 0 && c->table[i - 1].frequency ==
> +                  c->table[i].frequency && prev_cc == core_count) {
> +                       struct cpufreq_frequency_table *prev = &c->table[i - 1];
> +
> +                       if (prev_freq == CPUFREQ_ENTRY_INVALID)
> +                               prev->flags = CPUFREQ_BOOST_FREQ;
> +                       break;
> +               }
> +               prev_cc = core_count;
> +               prev_freq = cur_freq;
> +       }
> +
> +       c->table[i].frequency = CPUFREQ_TABLE_END;
> +
> +       return 0;
> +}
> +
> +static int qcom_get_related_cpus(struct device_node *np, struct cpumask *m)
> +{
> +       struct device_node *cpu_np, *freq_np;
> +       int cpu;
> +
> +       for_each_possible_cpu(cpu) {
> +               cpu_np = of_cpu_device_node_get(cpu);
> +               if (!cpu_np)
> +                       continue;
> +               freq_np = of_parse_phandle(cpu_np, "qcom,freq-domain", 0);
> +               of_node_put(cpu_np);
> +               if (!freq_np)
> +                       continue;
> +
> +               if (freq_np == np)
> +                       cpumask_set_cpu(cpu, m);
> +       }
> +
> +       return 0;
> +}
> +
> +static int qcom_cpu_resources_init(struct platform_device *pdev,
> +                                  struct device_node *np, unsigned int cpu,
> +                                  unsigned long xo_rate)
> +{
> +       struct cpufreq_qcom *c;
> +       struct resource res;
> +       struct device *dev = &pdev->dev;
> +       const u16 *offsets;
> +       int ret, i, cpu_first, cpu_r;
> +       void __iomem *base;
> +
> +       if (qcom_freq_domain_map[cpu])
> +               return 0;
> +
> +       c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
> +       if (!c)
> +               return -ENOMEM;
> +
> +       offsets = of_device_get_match_data(&pdev->dev);
> +       if (!offsets)
> +               return -EINVAL;
> +
> +       if (of_address_to_resource(np, 0, &res))
> +               return -ENOMEM;
> +
> +       base = devm_ioremap_resource(dev, &res);
> +       if (!base)
> +               return -ENOMEM;
> +
> +       for (i = REG_ENABLE; i < REG_ARRAY_SIZE; i++)
> +               c->reg_bases[i] = base + offsets[i];
> +
> +       /* HW should be in enabled state to proceed */
> +       if (!(readl_relaxed(c->reg_bases[REG_ENABLE]) & 0x1)) {
> +               dev_err(dev, "%s cpufreq hardware not enabled\n", np->name);
> +               return -ENODEV;
> +       }
> +
> +       ret = qcom_get_related_cpus(np, &c->related_cpus);
> +       if (ret) {
> +               dev_err(dev, "%s failed to get related CPUs\n", np->name);
> +               return ret;
> +       }
> +
> +       c->max_cores = cpumask_weight(&c->related_cpus);
> +       if (!c->max_cores)
> +               return -ENOENT;
> +
> +       c->xo_rate = xo_rate;
> +
> +       ret = qcom_cpufreq_hw_read_lut(pdev, c);
> +       if (ret) {
> +               dev_err(dev, "%s failed to read LUT\n", np->name);
> +               return ret;
> +       }
> +
> +       qcom_freq_domain_map[cpu] = c;
> +
> +       /* Related CPUs */
> +       cpu_first = cpumask_first(&c->related_cpus);
> +
> +       for_each_cpu(cpu_r, &c->related_cpus) {
> +               if (cpu_r != cpu_first)
> +                       qcom_freq_domain_map[cpu_r] =
> +                               qcom_freq_domain_map[cpu_first];
> +       }
> +
> +       return 0;
> +}
> +
> +static int qcom_resources_init(struct platform_device *pdev)
> +{
> +       struct device_node *np, *cpu_np;
> +       struct clk *clk;
> +       unsigned int cpu;
> +       int ret;
> +
> +       clk = devm_clk_get(&pdev->dev, "xo");
> +       if (IS_ERR(clk))
> +               return PTR_ERR(clk);
> +
> +       for_each_possible_cpu(cpu) {
> +               cpu_np = of_cpu_device_node_get(cpu);
> +               if (!cpu_np) {
> +                       dev_dbg(&pdev->dev, "Failed to get cpu %d device\n",
> +                               cpu);
> +                       continue;
> +               }
> +
> +               np = of_parse_phandle(cpu_np, "qcom,freq-domain", 0);
> +               of_node_put(cpu_np);
> +               if (!np) {
> +                       dev_err(&pdev->dev, "Failed to get freq-domain device\n");
> +                       return -EINVAL;
> +               }
> +
> +               ret = qcom_cpu_resources_init(pdev, np, cpu, clk_get_rate(clk));
> +               if (ret)
> +                       return ret;
> +       }
> +
> +       devm_clk_put(&pdev->dev, clk);
> +
> +       return 0;
> +}
> +
> +static int qcom_cpufreq_hw_driver_probe(struct platform_device *pdev)
> +{
> +       int rc;
> +
> +       /* Get the bases of cpufreq for domains */
> +       rc = qcom_resources_init(pdev);
> +       if (rc) {
> +               dev_err(&pdev->dev, "CPUFreq resource init failed\n");
> +               return rc;
> +       }
> +
> +       rc = cpufreq_register_driver(&cpufreq_qcom_hw_driver);
> +       if (rc) {
> +               dev_err(&pdev->dev, "CPUFreq HW driver failed to register\n");
> +               return rc;
> +       }
> +
> +       dev_dbg(&pdev->dev, "QCOM CPUFreq HW driver initialized\n");
> +
> +       return 0;
> +}
> +
> +static const struct of_device_id qcom_cpufreq_hw_match[] = {
> +       { .compatible = "qcom,cpufreq-hw", .data = &cpufreq_qcom_std_offsets },
> +       {}
> +};
> +
> +static struct platform_driver qcom_cpufreq_hw_driver = {
> +       .probe = qcom_cpufreq_hw_driver_probe,
> +       .driver = {
> +               .name = "qcom-cpufreq-hw",
> +               .of_match_table = qcom_cpufreq_hw_match,
> +       },
> +};
> +
> +static int __init qcom_cpufreq_hw_init(void)
> +{
> +       return platform_driver_register(&qcom_cpufreq_hw_driver);
> +}
> +subsys_initcall(qcom_cpufreq_hw_init);
> +
> +MODULE_DESCRIPTION("QCOM firmware-based CPU Frequency driver");
> --
> Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
> of the Code Aurora Forum, hosted by the  Linux Foundation.
>

Matthias Kaehlcke Sept. 10, 2018, 7:30 p.m. UTC | #10

On Tue, Jul 24, 2018 at 04:12:50PM +0530, Taniya Das wrote:
> The CPUfreq HW present in some QCOM chipsets offloads the steps necessary
> for changing the frequency of CPUs. The driver implements the cpufreq
> driver interface for this hardware engine.
> 
> Signed-off-by: Saravana Kannan <[e-mail]>
> Signed-off-by: Taniya Das <[e-mail]>
> ---
>  drivers/cpufreq/Kconfig.arm       |  11 ++
>  drivers/cpufreq/Makefile          |   1 +
>  drivers/cpufreq/qcom-cpufreq-hw.c | 348 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 360 insertions(+)
>  create mode 100644 drivers/cpufreq/qcom-cpufreq-hw.c
> 
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index 0cd8eb7..93a9d72 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -298,3 +298,14 @@ config ARM_PXA2xx_CPUFREQ
>  	  This add the CPUFreq driver support for Intel PXA2xx SOCs.
> 
>  	  If in doubt, say N.
> +
> +config ARM_QCOM_CPUFREQ_HW
> +	bool "QCOM CPUFreq HW driver"
> +	depends on ARCH_QCOM
> +	help
> +	 Support for the CPUFreq HW driver.
> +	 Some QCOM chipsets have a HW engine to offload the steps
> +	 necessary for changing the frequency of the CPUs. Firmware loaded
> +	 in this engine exposes a programming interface to the OS.
> +	 The driver implements the cpufreq interface for this HW engine.
> +	 Say Y if you want to support CPUFreq HW.
> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> index c1ffeab..ca48a1d 100644
> --- a/drivers/cpufreq/Makefile
> +++ b/drivers/cpufreq/Makefile
> @@ -85,6 +85,7 @@ obj-$(CONFIG_ARM_TEGRA124_CPUFREQ)	+= tegra124-cpufreq.o
>  obj-$(CONFIG_ARM_TEGRA186_CPUFREQ)	+= tegra186-cpufreq.o
>  obj-$(CONFIG_ARM_TI_CPUFREQ)		+= ti-cpufreq.o
>  obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ)	+= vexpress-spc-cpufreq.o
> +obj-$(CONFIG_ARM_QCOM_CPUFREQ_HW)	+= qcom-cpufreq-hw.o
> 
> 
>  ##################################################################################
> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> new file mode 100644
> index 0000000..ea8f7d1
> --- /dev/null
> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> @@ -0,0 +1,348 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/cpufreq.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_platform.h>
> +
> +#define INIT_RATE			300000000UL
> +#define LUT_MAX_ENTRIES			40U
> +#define CORE_COUNT_VAL(val)		(((val) & (GENMASK(18, 16))) >> 16)
> +#define LUT_ROW_SIZE			32
> +
> +enum {
> +	REG_ENABLE,
> +	REG_LUT_TABLE,
> +	REG_PERF_STATE,
> +
> +	REG_ARRAY_SIZE,
> +};
> +
> +struct cpufreq_qcom {
> +	struct cpufreq_frequency_table *table;
> +	struct device *dev;
> +	void __iomem *reg_bases[REG_ARRAY_SIZE];
> +	cpumask_t related_cpus;
> +	unsigned int max_cores;
> +	unsigned long xo_rate;
> +};
> +
> +static const u16 cpufreq_qcom_std_offsets[REG_ARRAY_SIZE] = {
> +	[REG_ENABLE]		= 0x0,
> +	[REG_LUT_TABLE]		= 0x110,
> +	[REG_PERF_STATE]	= 0x920,
> +};
> +
> +static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
> +
> +static int
> +qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
> +			     unsigned int index)
> +{
> +	struct cpufreq_qcom *c = policy->driver_data;
> +
> +	writel_relaxed(index, c->reg_bases[REG_PERF_STATE]);
> +
> +	return 0;
> +}
> +
> +static unsigned int qcom_cpufreq_hw_get(unsigned int cpu)
> +{
> +	struct cpufreq_qcom *c;
> +	struct cpufreq_policy *policy;
> +	unsigned int index;
> +
> +	policy = cpufreq_cpu_get_raw(cpu);
> +	if (!policy)
> +		return 0;
> +
> +	c = policy->driver_data;
> +
> +	index = readl_relaxed(c->reg_bases[REG_PERF_STATE]);
> +	index = min(index, LUT_MAX_ENTRIES - 1);
> +
> +	return policy->freq_table[index].frequency;
> +}
> +
> +static unsigned int
> +qcom_cpufreq_hw_fast_switch(struct cpufreq_policy *policy,
> +			    unsigned int target_freq)
> +{
> +	struct cpufreq_qcom *c = policy->driver_data;
> +	int index;
> +
> +	index = policy->cached_resolved_idx;
> +	if (index < 0)
> +		return 0;
> +
> +	writel_relaxed(index, c->reg_bases[REG_PERF_STATE]);
> +
> +	return policy->freq_table[index].frequency;
> +}
> +
> +static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
> +{
> +	struct cpufreq_qcom *c;
> +
> +	c = qcom_freq_domain_map[policy->cpu];
> +	if (!c) {
> +		pr_err("No scaling support for CPU%d\n", policy->cpu);
> +		return -ENODEV;
> +	}
> +
> +	cpumask_copy(policy->cpus, &c->related_cpus);
> +
> +	policy->fast_switch_possible = true;
> +	policy->freq_table = c->table;
> +	policy->driver_data = c;
> +
> +	return 0;
> +}
> +
> +static struct freq_attr *qcom_cpufreq_hw_attr[] = {
> +	&cpufreq_freq_attr_scaling_available_freqs,
> +	&cpufreq_freq_attr_scaling_boost_freqs,
> +	NULL
> +};
> +
> +static struct cpufreq_driver cpufreq_qcom_hw_driver = {
> +	.flags		= CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK |
> +			  CPUFREQ_HAVE_GOVERNOR_PER_POLICY,
> +	.verify		= cpufreq_generic_frequency_table_verify,
> +	.target_index	= qcom_cpufreq_hw_target_index,
> +	.get		= qcom_cpufreq_hw_get,
> +	.init		= qcom_cpufreq_hw_cpu_init,
> +	.fast_switch    = qcom_cpufreq_hw_fast_switch,
> +	.name		= "qcom-cpufreq-hw",
> +	.attr		= qcom_cpufreq_hw_attr,
> +	.boost_enabled	= true,
> +};
> +
> +static int qcom_cpufreq_hw_read_lut(struct platform_device *pdev,
> +				    struct cpufreq_qcom *c)
> +{
> +	struct device *dev = &pdev->dev;
> +	void __iomem *base;
> +	u32 data, src, lval, i, core_count, prev_cc, prev_freq, cur_freq;
> +
> +	c->table = devm_kcalloc(dev, LUT_MAX_ENTRIES + 1,
> +				sizeof(*c->table), GFP_KERNEL);
> +	if (!c->table)
> +		return -ENOMEM;
> +
> +	base = c->reg_bases[REG_LUT_TABLE];
> +
> +	for (i = 0; i < LUT_MAX_ENTRIES; i++) {
> +		data = readl_relaxed(base + i * LUT_ROW_SIZE);
> +		src = (data & GENMASK(31, 30)) >> 30;
> +		lval = data & GENMASK(7, 0);
> +		core_count = CORE_COUNT_VAL(data);
> +
> +		if (src)
> +			c->table[i].frequency = c->xo_rate * lval / 1000;
> +		else
> +			c->table[i].frequency = INIT_RATE / 1000;
> +
> +		cur_freq = c->table[i].frequency;
> +
> +		dev_dbg(dev, "index=%d freq=%d, core_count %d\n",
> +			i, c->table[i].frequency, core_count);
> +
> +		if (core_count != c->max_cores)
> +			cur_freq = CPUFREQ_ENTRY_INVALID;
> +

I noticed that the 'power_allocator' thermal governor currently can't
be used with this driver since there is no OPP table with frequency and
voltage information. Does the LUT contain information about the
voltage or is there another mechanism to retrieve it?

Taniya Das Sept. 23, 2018, 9:35 a.m. UTC | #11

Hello Stephen,


On 8/24/2018 12:08 AM, Stephen Boyd wrote:
> Quoting Taniya Das (2018-08-08 03:15:26)
>>
>>
>> On 8/8/2018 11:52 AM, Stephen Boyd wrote:
>>>>
>>>> Binding describes hardware controllable by the OS. That's the reality.
>>>> Let's not add mandatory clock bindings for clocks that the OS can't do
>>>> anything about.
>>>>
>>>
>>> It seems that you believe clks should only be used to turn on/off and
>>> control rates. That is not the whole truth. Sometimes clks are there
>>> just to express the clk frequencies that are present in the design so
>>> that drivers can figure out what to do.
>>>
>>
>> Stephen,
>>
>> As this clock is not configurable by linux clock drivers and we really
>> do not care the parent src(as mentioned by Saravana) to generate the
>> 300MHz, would it be good to define a fixed rate clock so as to express
>> the HW connectivity & frequency?
>>
> 
> As a hack that works great, but why do we need to workaround problems by
> adding a fixed rate clk to DT for this PLL? The PLL is provided by GCC
> node so it should be connected to the GCC node.
> 

Please help with review the next patch series which would take the PLL 
phandle from DT.

Taniya Das Sept. 23, 2018, 9:40 a.m. UTC | #12

Hello Matthias,

Thanks for your review comments.

On 8/29/2018 11:31 PM, Matthias Kaehlcke wrote:
> Hi Taniya,
> 
> On Tue, Jul 24, 2018 at 04:12:50PM +0530, Taniya Das wrote:
>> The CPUfreq HW present in some QCOM chipsets offloads the steps necessary
>> for changing the frequency of CPUs. The driver implements the cpufreq
>> driver interface for this hardware engine.
>>
>> Signed-off-by: Saravana Kannan <[e-mail]>
>> Signed-off-by: Taniya Das <[e-mail]>
>> ---
>>   drivers/cpufreq/Kconfig.arm       |  11 ++
>>   drivers/cpufreq/Makefile          |   1 +
>>   drivers/cpufreq/qcom-cpufreq-hw.c | 348 ++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 360 insertions(+)
>>   create mode 100644 drivers/cpufreq/qcom-cpufreq-hw.c
>>
>> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
>> index 0cd8eb7..93a9d72 100644
>> --- a/drivers/cpufreq/Kconfig.arm
>> +++ b/drivers/cpufreq/Kconfig.arm
>> @@ -298,3 +298,14 @@ config ARM_PXA2xx_CPUFREQ
>>   	  This add the CPUFreq driver support for Intel PXA2xx SOCs.
>>
>>   	  If in doubt, say N.
>> +
>> +config ARM_QCOM_CPUFREQ_HW
>> +	bool "QCOM CPUFreq HW driver"
>> +	depends on ARCH_QCOM
>> +	help
>> +	 Support for the CPUFreq HW driver.
>> +	 Some QCOM chipsets have a HW engine to offload the steps
>> +	 necessary for changing the frequency of the CPUs. Firmware loaded
>> +	 in this engine exposes a programming interface to the OS.
>> +	 The driver implements the cpufreq interface for this HW engine.
>> +	 Say Y if you want to support CPUFreq HW.
>> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
>> index c1ffeab..ca48a1d 100644
>> --- a/drivers/cpufreq/Makefile
>> +++ b/drivers/cpufreq/Makefile
>> @@ -85,6 +85,7 @@ obj-$(CONFIG_ARM_TEGRA124_CPUFREQ)	+= tegra124-cpufreq.o
>>   obj-$(CONFIG_ARM_TEGRA186_CPUFREQ)	+= tegra186-cpufreq.o
>>   obj-$(CONFIG_ARM_TI_CPUFREQ)		+= ti-cpufreq.o
>>   obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ)	+= vexpress-spc-cpufreq.o
>> +obj-$(CONFIG_ARM_QCOM_CPUFREQ_HW)	+= qcom-cpufreq-hw.o
>>
>>
>>   ##################################################################################
>> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
>> new file mode 100644
>> index 0000000..ea8f7d1
>> --- /dev/null
>> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
>> @@ -0,0 +1,348 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +#include <linux/cpufreq.h>
>> +#include <linux/init.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_platform.h>
>> +
>> +#define INIT_RATE			300000000UL
>> +#define LUT_MAX_ENTRIES			40U
>> +#define CORE_COUNT_VAL(val)		(((val) & (GENMASK(18, 16))) >> 16)
>> +#define LUT_ROW_SIZE			32
>> +
>> +enum {
>> +	REG_ENABLE,
>> +	REG_LUT_TABLE,
>> +	REG_PERF_STATE,
>> +
>> +	REG_ARRAY_SIZE,
>> +};
>> +
>> +struct cpufreq_qcom {
>> +	struct cpufreq_frequency_table *table;
>> +	struct device *dev;
> 
> 'dev' is not used and can be removed.
> 

Thanks, would remove in the next patch.

>> ...
>>
>> +static int qcom_cpu_resources_init(struct platform_device *pdev,
>> +				   struct device_node *np, unsigned int cpu,
>> +				   unsigned long xo_rate)
>> +{
>> +	struct cpufreq_qcom *c;
>> +	struct resource res;
>> +	struct device *dev = &pdev->dev;
>> +	const u16 *offsets;
>> +	int ret, i, cpu_first, cpu_r;
>> +	void __iomem *base;
>> +
>> +	if (qcom_freq_domain_map[cpu])
>> +		return 0;
>> +
>> +	c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
>> +	if (!c)
>> +		return -ENOMEM;
>> +
>> +	offsets = of_device_get_match_data(&pdev->dev);
>> +	if (!offsets)
>> +		return -EINVAL;
>> +
>> +	if (of_address_to_resource(np, 0, &res))
>> +		return -ENOMEM;
>> +
>> +	base = devm_ioremap_resource(dev, &res);
>> +	if (!base)
>> +		return -ENOMEM;
>> +
>> +	for (i = REG_ENABLE; i < REG_ARRAY_SIZE; i++)
>> +		c->reg_bases[i] = base + offsets[i];
>> +
>> +	/* HW should be in enabled state to proceed */
>> +	if (!(readl_relaxed(c->reg_bases[REG_ENABLE]) & 0x1)) {
>> +		dev_err(dev, "%s cpufreq hardware not enabled\n", np->name);
>> +		return -ENODEV;
>> +	}
>> +
>> +	ret = qcom_get_related_cpus(np, &c->related_cpus);
>> +	if (ret) {
>> +		dev_err(dev, "%s failed to get related CPUs\n", np->name);
>> +		return ret;
>> +	}
>> +
>> +	c->max_cores = cpumask_weight(&c->related_cpus);
>> +	if (!c->max_cores)
>> +		return -ENOENT;
>> +
>> +	c->xo_rate = xo_rate;
>> +
>> +	ret = qcom_cpufreq_hw_read_lut(pdev, c);
>> +	if (ret) {
>> +		dev_err(dev, "%s failed to read LUT\n", np->name);
>> +		return ret;
>> +	}
>> +
>> +	qcom_freq_domain_map[cpu] = c;
>> +
>> +	/* Related CPUs */
>> +	cpu_first = cpumask_first(&c->related_cpus);
>> +
>> +	for_each_cpu(cpu_r, &c->related_cpus) {
>> +		if (cpu_r != cpu_first)
>> +			qcom_freq_domain_map[cpu_r] =
>> +				qcom_freq_domain_map[cpu_first];
>> +	}
> 
> The above ten lines could be simplified to:
> 
> 	for_each_cpu(cpu_r, &c->related_cpus)
> 		qcom_freq_domain_map[cpu_r] = c;
> 

Would clean it up in the next patch.

>> ...
>>
>> +static int __init qcom_cpufreq_hw_init(void)
>> +{
>> +	return platform_driver_register(&qcom_cpufreq_hw_driver);
>> +}
>> +subsys_initcall(qcom_cpufreq_hw_init);
> 
> Is subsys_initcall used for a particular reason? It will cause
> problems when registering cooling devices, since the thermal device
> class is initialized through an fs_initcall, which are executed
> later.
> 
> Most cpufreq drivers use module_init, device_initcall or
> late_initcall, can't this driver use one of those?
> 

We want the CPU to be scaling to the highest frequency at the earliest.

> Cheers
> 
> Matthias
>

Taniya Das Sept. 23, 2018, 9:43 a.m. UTC | #13

Hi Amit,

On 9/9/2018 8:04 PM, Amit Kucheria wrote:
> Hi Taniya,
> 
> How much have you stressed this driver?
> 
> I tried it on top of an integration branch based on 4.19-rc2 that we
> maintain[1] and was able to get the board to reboot fairly easily with
> just a few "yes > /dev/null &" instances running in the background.
> 
> I even tried with interconnect framework disabled to be sure, and it
> still reboots.
> 

The driver is being tested with 4.14 kernel version and I have not yet 
got any reports of reboots. Though I would also try to check with teams 
internally.

> I'll continue trying to pinpoint the source of the problem, but it
> would be nice to know what tree you're testing against.
> 
> Regards,
> Amit
> [1] https://git.linaro.org/people/amit.kucheria/kernel.git/log/?h=integration/qcomlt-automerge-result
> 
> On Tue, Jul 24, 2018 at 4:12 PM, Taniya Das <[e-mail]> wrote:
>> The CPUfreq HW present in some QCOM chipsets offloads the steps necessary
>> for changing the frequency of CPUs. The driver implements the cpufreq
>> driver interface for this hardware engine.
>>
>> Signed-off-by: Saravana Kannan <[e-mail]>
>> Signed-off-by: Taniya Das <[e-mail]>
>> ---
>>   drivers/cpufreq/Kconfig.arm       |  11 ++
>>   drivers/cpufreq/Makefile          |   1 +
>>   drivers/cpufreq/qcom-cpufreq-hw.c | 348 ++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 360 insertions(+)
>>   create mode 100644 drivers/cpufreq/qcom-cpufreq-hw.c
>>
>> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
>> index 0cd8eb7..93a9d72 100644
>> --- a/drivers/cpufreq/Kconfig.arm
>> +++ b/drivers/cpufreq/Kconfig.arm
>> @@ -298,3 +298,14 @@ config ARM_PXA2xx_CPUFREQ
>>            This add the CPUFreq driver support for Intel PXA2xx SOCs.
>>
>>            If in doubt, say N.
>> +
>> +config ARM_QCOM_CPUFREQ_HW
>> +       bool "QCOM CPUFreq HW driver"
>> +       depends on ARCH_QCOM
>> +       help
>> +        Support for the CPUFreq HW driver.
>> +        Some QCOM chipsets have a HW engine to offload the steps
>> +        necessary for changing the frequency of the CPUs. Firmware loaded
>> +        in this engine exposes a programming interface to the OS.
>> +        The driver implements the cpufreq interface for this HW engine.
>> +        Say Y if you want to support CPUFreq HW.
>> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
>> index c1ffeab..ca48a1d 100644
>> --- a/drivers/cpufreq/Makefile
>> +++ b/drivers/cpufreq/Makefile
>> @@ -85,6 +85,7 @@ obj-$(CONFIG_ARM_TEGRA124_CPUFREQ)    += tegra124-cpufreq.o
>>   obj-$(CONFIG_ARM_TEGRA186_CPUFREQ)     += tegra186-cpufreq.o
>>   obj-$(CONFIG_ARM_TI_CPUFREQ)           += ti-cpufreq.o
>>   obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ) += vexpress-spc-cpufreq.o
>> +obj-$(CONFIG_ARM_QCOM_CPUFREQ_HW)      += qcom-cpufreq-hw.o
>>
>>
>>   ##################################################################################
>> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
>> new file mode 100644
>> index 0000000..ea8f7d1
>> --- /dev/null
>> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
>> @@ -0,0 +1,348 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +#include <linux/cpufreq.h>
>> +#include <linux/init.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_platform.h>
>> +
>> +#define INIT_RATE                      300000000UL
>> +#define LUT_MAX_ENTRIES                        40U
>> +#define CORE_COUNT_VAL(val)            (((val) & (GENMASK(18, 16))) >> 16)
>> +#define LUT_ROW_SIZE                   32
>> +
>> +enum {
>> +       REG_ENABLE,
>> +       REG_LUT_TABLE,
>> +       REG_PERF_STATE,
>> +
>> +       REG_ARRAY_SIZE,
>> +};
>> +
>> +struct cpufreq_qcom {
>> +       struct cpufreq_frequency_table *table;
>> +       struct device *dev;
>> +       void __iomem *reg_bases[REG_ARRAY_SIZE];
>> +       cpumask_t related_cpus;
>> +       unsigned int max_cores;
>> +       unsigned long xo_rate;
>> +};
>> +
>> +static const u16 cpufreq_qcom_std_offsets[REG_ARRAY_SIZE] = {
>> +       [REG_ENABLE]            = 0x0,
>> +       [REG_LUT_TABLE]         = 0x110,
>> +       [REG_PERF_STATE]        = 0x920,
>> +};
>> +
>> +static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
>> +
>> +static int
>> +qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
>> +                            unsigned int index)
>> +{
>> +       struct cpufreq_qcom *c = policy->driver_data;
>> +
>> +       writel_relaxed(index, c->reg_bases[REG_PERF_STATE]);
>> +
>> +       return 0;
>> +}
>> +
>> +static unsigned int qcom_cpufreq_hw_get(unsigned int cpu)
>> +{
>> +       struct cpufreq_qcom *c;
>> +       struct cpufreq_policy *policy;
>> +       unsigned int index;
>> +
>> +       policy = cpufreq_cpu_get_raw(cpu);
>> +       if (!policy)
>> +               return 0;
>> +
>> +       c = policy->driver_data;
>> +
>> +       index = readl_relaxed(c->reg_bases[REG_PERF_STATE]);
>> +       index = min(index, LUT_MAX_ENTRIES - 1);
>> +
>> +       return policy->freq_table[index].frequency;
>> +}
>> +
>> +static unsigned int
>> +qcom_cpufreq_hw_fast_switch(struct cpufreq_policy *policy,
>> +                           unsigned int target_freq)
>> +{
>> +       struct cpufreq_qcom *c = policy->driver_data;
>> +       int index;
>> +
>> +       index = policy->cached_resolved_idx;
>> +       if (index < 0)
>> +               return 0;
>> +
>> +       writel_relaxed(index, c->reg_bases[REG_PERF_STATE]);
>> +
>> +       return policy->freq_table[index].frequency;
>> +}
>> +
>> +static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
>> +{
>> +       struct cpufreq_qcom *c;
>> +
>> +       c = qcom_freq_domain_map[policy->cpu];
>> +       if (!c) {
>> +               pr_err("No scaling support for CPU%d\n", policy->cpu);
>> +               return -ENODEV;
>> +       }
>> +
>> +       cpumask_copy(policy->cpus, &c->related_cpus);
>> +
>> +       policy->fast_switch_possible = true;
>> +       policy->freq_table = c->table;
>> +       policy->driver_data = c;
>> +
>> +       return 0;
>> +}
>> +
>> +static struct freq_attr *qcom_cpufreq_hw_attr[] = {
>> +       &cpufreq_freq_attr_scaling_available_freqs,
>> +       &cpufreq_freq_attr_scaling_boost_freqs,
>> +       NULL
>> +};
>> +
>> +static struct cpufreq_driver cpufreq_qcom_hw_driver = {
>> +       .flags          = CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK |
>> +                         CPUFREQ_HAVE_GOVERNOR_PER_POLICY,
>> +       .verify         = cpufreq_generic_frequency_table_verify,
>> +       .target_index   = qcom_cpufreq_hw_target_index,
>> +       .get            = qcom_cpufreq_hw_get,
>> +       .init           = qcom_cpufreq_hw_cpu_init,
>> +       .fast_switch    = qcom_cpufreq_hw_fast_switch,
>> +       .name           = "qcom-cpufreq-hw",
>> +       .attr           = qcom_cpufreq_hw_attr,
>> +       .boost_enabled  = true,
>> +};
>> +
>> +static int qcom_cpufreq_hw_read_lut(struct platform_device *pdev,
>> +                                   struct cpufreq_qcom *c)
>> +{
>> +       struct device *dev = &pdev->dev;
>> +       void __iomem *base;
>> +       u32 data, src, lval, i, core_count, prev_cc, prev_freq, cur_freq;
>> +
>> +       c->table = devm_kcalloc(dev, LUT_MAX_ENTRIES + 1,
>> +                               sizeof(*c->table), GFP_KERNEL);
>> +       if (!c->table)
>> +               return -ENOMEM;
>> +
>> +       base = c->reg_bases[REG_LUT_TABLE];
>> +
>> +       for (i = 0; i < LUT_MAX_ENTRIES; i++) {
>> +               data = readl_relaxed(base + i * LUT_ROW_SIZE);
>> +               src = (data & GENMASK(31, 30)) >> 30;
>> +               lval = data & GENMASK(7, 0);
>> +               core_count = CORE_COUNT_VAL(data);
>> +
>> +               if (src)
>> +                       c->table[i].frequency = c->xo_rate * lval / 1000;
>> +               else
>> +                       c->table[i].frequency = INIT_RATE / 1000;
>> +
>> +               cur_freq = c->table[i].frequency;
>> +
>> +               dev_dbg(dev, "index=%d freq=%d, core_count %d\n",
>> +                       i, c->table[i].frequency, core_count);
>> +
>> +               if (core_count != c->max_cores)
>> +                       cur_freq = CPUFREQ_ENTRY_INVALID;
>> +
>> +               /*
>> +                * Two of the same frequencies with the same core counts means
>> +                * end of table.
>> +                */
>> +               if (i > 0 && c->table[i - 1].frequency ==
>> +                  c->table[i].frequency && prev_cc == core_count) {
>> +                       struct cpufreq_frequency_table *prev = &c->table[i - 1];
>> +
>> +                       if (prev_freq == CPUFREQ_ENTRY_INVALID)
>> +                               prev->flags = CPUFREQ_BOOST_FREQ;
>> +                       break;
>> +               }
>> +               prev_cc = core_count;
>> +               prev_freq = cur_freq;
>> +       }
>> +
>> +       c->table[i].frequency = CPUFREQ_TABLE_END;
>> +
>> +       return 0;
>> +}
>> +
>> +static int qcom_get_related_cpus(struct device_node *np, struct cpumask *m)
>> +{
>> +       struct device_node *cpu_np, *freq_np;
>> +       int cpu;
>> +
>> +       for_each_possible_cpu(cpu) {
>> +               cpu_np = of_cpu_device_node_get(cpu);
>> +               if (!cpu_np)
>> +                       continue;
>> +               freq_np = of_parse_phandle(cpu_np, "qcom,freq-domain", 0);
>> +               of_node_put(cpu_np);
>> +               if (!freq_np)
>> +                       continue;
>> +
>> +               if (freq_np == np)
>> +                       cpumask_set_cpu(cpu, m);
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static int qcom_cpu_resources_init(struct platform_device *pdev,
>> +                                  struct device_node *np, unsigned int cpu,
>> +                                  unsigned long xo_rate)
>> +{
>> +       struct cpufreq_qcom *c;
>> +       struct resource res;
>> +       struct device *dev = &pdev->dev;
>> +       const u16 *offsets;
>> +       int ret, i, cpu_first, cpu_r;
>> +       void __iomem *base;
>> +
>> +       if (qcom_freq_domain_map[cpu])
>> +               return 0;
>> +
>> +       c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
>> +       if (!c)
>> +               return -ENOMEM;
>> +
>> +       offsets = of_device_get_match_data(&pdev->dev);
>> +       if (!offsets)
>> +               return -EINVAL;
>> +
>> +       if (of_address_to_resource(np, 0, &res))
>> +               return -ENOMEM;
>> +
>> +       base = devm_ioremap_resource(dev, &res);
>> +       if (!base)
>> +               return -ENOMEM;
>> +
>> +       for (i = REG_ENABLE; i < REG_ARRAY_SIZE; i++)
>> +               c->reg_bases[i] = base + offsets[i];
>> +
>> +       /* HW should be in enabled state to proceed */
>> +       if (!(readl_relaxed(c->reg_bases[REG_ENABLE]) & 0x1)) {
>> +               dev_err(dev, "%s cpufreq hardware not enabled\n", np->name);
>> +               return -ENODEV;
>> +       }
>> +
>> +       ret = qcom_get_related_cpus(np, &c->related_cpus);
>> +       if (ret) {
>> +               dev_err(dev, "%s failed to get related CPUs\n", np->name);
>> +               return ret;
>> +       }
>> +
>> +       c->max_cores = cpumask_weight(&c->related_cpus);
>> +       if (!c->max_cores)
>> +               return -ENOENT;
>> +
>> +       c->xo_rate = xo_rate;
>> +
>> +       ret = qcom_cpufreq_hw_read_lut(pdev, c);
>> +       if (ret) {
>> +               dev_err(dev, "%s failed to read LUT\n", np->name);
>> +               return ret;
>> +       }
>> +
>> +       qcom_freq_domain_map[cpu] = c;
>> +
>> +       /* Related CPUs */
>> +       cpu_first = cpumask_first(&c->related_cpus);
>> +
>> +       for_each_cpu(cpu_r, &c->related_cpus) {
>> +               if (cpu_r != cpu_first)
>> +                       qcom_freq_domain_map[cpu_r] =
>> +                               qcom_freq_domain_map[cpu_first];
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static int qcom_resources_init(struct platform_device *pdev)
>> +{
>> +       struct device_node *np, *cpu_np;
>> +       struct clk *clk;
>> +       unsigned int cpu;
>> +       int ret;
>> +
>> +       clk = devm_clk_get(&pdev->dev, "xo");
>> +       if (IS_ERR(clk))
>> +               return PTR_ERR(clk);
>> +
>> +       for_each_possible_cpu(cpu) {
>> +               cpu_np = of_cpu_device_node_get(cpu);
>> +               if (!cpu_np) {
>> +                       dev_dbg(&pdev->dev, "Failed to get cpu %d device\n",
>> +                               cpu);
>> +                       continue;
>> +               }
>> +
>> +               np = of_parse_phandle(cpu_np, "qcom,freq-domain", 0);
>> +               of_node_put(cpu_np);
>> +               if (!np) {
>> +                       dev_err(&pdev->dev, "Failed to get freq-domain device\n");
>> +                       return -EINVAL;
>> +               }
>> +
>> +               ret = qcom_cpu_resources_init(pdev, np, cpu, clk_get_rate(clk));
>> +               if (ret)
>> +                       return ret;
>> +       }
>> +
>> +       devm_clk_put(&pdev->dev, clk);
>> +
>> +       return 0;
>> +}
>> +
>> +static int qcom_cpufreq_hw_driver_probe(struct platform_device *pdev)
>> +{
>> +       int rc;
>> +
>> +       /* Get the bases of cpufreq for domains */
>> +       rc = qcom_resources_init(pdev);
>> +       if (rc) {
>> +               dev_err(&pdev->dev, "CPUFreq resource init failed\n");
>> +               return rc;
>> +       }
>> +
>> +       rc = cpufreq_register_driver(&cpufreq_qcom_hw_driver);
>> +       if (rc) {
>> +               dev_err(&pdev->dev, "CPUFreq HW driver failed to register\n");
>> +               return rc;
>> +       }
>> +
>> +       dev_dbg(&pdev->dev, "QCOM CPUFreq HW driver initialized\n");
>> +
>> +       return 0;
>> +}
>> +
>> +static const struct of_device_id qcom_cpufreq_hw_match[] = {
>> +       { .compatible = "qcom,cpufreq-hw", .data = &cpufreq_qcom_std_offsets },
>> +       {}
>> +};
>> +
>> +static struct platform_driver qcom_cpufreq_hw_driver = {
>> +       .probe = qcom_cpufreq_hw_driver_probe,
>> +       .driver = {
>> +               .name = "qcom-cpufreq-hw",
>> +               .of_match_table = qcom_cpufreq_hw_match,
>> +       },
>> +};
>> +
>> +static int __init qcom_cpufreq_hw_init(void)
>> +{
>> +       return platform_driver_register(&qcom_cpufreq_hw_driver);
>> +}
>> +subsys_initcall(qcom_cpufreq_hw_init);
>> +
>> +MODULE_DESCRIPTION("QCOM firmware-based CPU Frequency driver");
>> --
>> Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
>> of the Code Aurora Forum, hosted by the  Linux Foundation.
>>

Taniya Das Sept. 23, 2018, 9:48 a.m. UTC | #14

On 9/11/2018 1:00 AM, Matthias Kaehlcke wrote:
> On Tue, Jul 24, 2018 at 04:12:50PM +0530, Taniya Das wrote:
>> The CPUfreq HW present in some QCOM chipsets offloads the steps necessary
>> for changing the frequency of CPUs. The driver implements the cpufreq
>> driver interface for this hardware engine.
>>
>> Signed-off-by: Saravana Kannan <[e-mail]>
>> Signed-off-by: Taniya Das <[e-mail]>
>> ---
>>   drivers/cpufreq/Kconfig.arm       |  11 ++
>>   drivers/cpufreq/Makefile          |   1 +
>>   drivers/cpufreq/qcom-cpufreq-hw.c | 348 ++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 360 insertions(+)
>>   create mode 100644 drivers/cpufreq/qcom-cpufreq-hw.c
>>
>> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
>> index 0cd8eb7..93a9d72 100644
>> --- a/drivers/cpufreq/Kconfig.arm
>> +++ b/drivers/cpufreq/Kconfig.arm
>> @@ -298,3 +298,14 @@ config ARM_PXA2xx_CPUFREQ
>>   	  This add the CPUFreq driver support for Intel PXA2xx SOCs.
>>
>>   	  If in doubt, say N.
>> +
>> +config ARM_QCOM_CPUFREQ_HW
>> +	bool "QCOM CPUFreq HW driver"
>> +	depends on ARCH_QCOM
>> +	help
>> +	 Support for the CPUFreq HW driver.
>> +	 Some QCOM chipsets have a HW engine to offload the steps
>> +	 necessary for changing the frequency of the CPUs. Firmware loaded
>> +	 in this engine exposes a programming interface to the OS.
>> +	 The driver implements the cpufreq interface for this HW engine.
>> +	 Say Y if you want to support CPUFreq HW.
>> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
>> index c1ffeab..ca48a1d 100644
>> --- a/drivers/cpufreq/Makefile
>> +++ b/drivers/cpufreq/Makefile
>> @@ -85,6 +85,7 @@ obj-$(CONFIG_ARM_TEGRA124_CPUFREQ)	+= tegra124-cpufreq.o
>>   obj-$(CONFIG_ARM_TEGRA186_CPUFREQ)	+= tegra186-cpufreq.o
>>   obj-$(CONFIG_ARM_TI_CPUFREQ)		+= ti-cpufreq.o
>>   obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ)	+= vexpress-spc-cpufreq.o
>> +obj-$(CONFIG_ARM_QCOM_CPUFREQ_HW)	+= qcom-cpufreq-hw.o
>>
>>
>>   ##################################################################################
>> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
>> new file mode 100644
>> index 0000000..ea8f7d1
>> --- /dev/null
>> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
>> @@ -0,0 +1,348 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +#include <linux/cpufreq.h>
>> +#include <linux/init.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_platform.h>
>> +
>> +#define INIT_RATE			300000000UL
>> +#define LUT_MAX_ENTRIES			40U
>> +#define CORE_COUNT_VAL(val)		(((val) & (GENMASK(18, 16))) >> 16)
>> +#define LUT_ROW_SIZE			32
>> +
>> +enum {
>> +	REG_ENABLE,
>> +	REG_LUT_TABLE,
>> +	REG_PERF_STATE,
>> +
>> +	REG_ARRAY_SIZE,
>> +};
>> +
>> +struct cpufreq_qcom {
>> +	struct cpufreq_frequency_table *table;
>> +	struct device *dev;
>> +	void __iomem *reg_bases[REG_ARRAY_SIZE];
>> +	cpumask_t related_cpus;
>> +	unsigned int max_cores;
>> +	unsigned long xo_rate;
>> +};
>> +
>> +static const u16 cpufreq_qcom_std_offsets[REG_ARRAY_SIZE] = {
>> +	[REG_ENABLE]		= 0x0,
>> +	[REG_LUT_TABLE]		= 0x110,
>> +	[REG_PERF_STATE]	= 0x920,
>> +};
>> +
>> +static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
>> +
>> +static int
>> +qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
>> +			     unsigned int index)
>> +{
>> +	struct cpufreq_qcom *c = policy->driver_data;
>> +
>> +	writel_relaxed(index, c->reg_bases[REG_PERF_STATE]);
>> +
>> +	return 0;
>> +}
>> +
>> +static unsigned int qcom_cpufreq_hw_get(unsigned int cpu)
>> +{
>> +	struct cpufreq_qcom *c;
>> +	struct cpufreq_policy *policy;
>> +	unsigned int index;
>> +
>> +	policy = cpufreq_cpu_get_raw(cpu);
>> +	if (!policy)
>> +		return 0;
>> +
>> +	c = policy->driver_data;
>> +
>> +	index = readl_relaxed(c->reg_bases[REG_PERF_STATE]);
>> +	index = min(index, LUT_MAX_ENTRIES - 1);
>> +
>> +	return policy->freq_table[index].frequency;
>> +}
>> +
>> +static unsigned int
>> +qcom_cpufreq_hw_fast_switch(struct cpufreq_policy *policy,
>> +			    unsigned int target_freq)
>> +{
>> +	struct cpufreq_qcom *c = policy->driver_data;
>> +	int index;
>> +
>> +	index = policy->cached_resolved_idx;
>> +	if (index < 0)
>> +		return 0;
>> +
>> +	writel_relaxed(index, c->reg_bases[REG_PERF_STATE]);
>> +
>> +	return policy->freq_table[index].frequency;
>> +}
>> +
>> +static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
>> +{
>> +	struct cpufreq_qcom *c;
>> +
>> +	c = qcom_freq_domain_map[policy->cpu];
>> +	if (!c) {
>> +		pr_err("No scaling support for CPU%d\n", policy->cpu);
>> +		return -ENODEV;
>> +	}
>> +
>> +	cpumask_copy(policy->cpus, &c->related_cpus);
>> +
>> +	policy->fast_switch_possible = true;
>> +	policy->freq_table = c->table;
>> +	policy->driver_data = c;
>> +
>> +	return 0;
>> +}
>> +
>> +static struct freq_attr *qcom_cpufreq_hw_attr[] = {
>> +	&cpufreq_freq_attr_scaling_available_freqs,
>> +	&cpufreq_freq_attr_scaling_boost_freqs,
>> +	NULL
>> +};
>> +
>> +static struct cpufreq_driver cpufreq_qcom_hw_driver = {
>> +	.flags		= CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK |
>> +			  CPUFREQ_HAVE_GOVERNOR_PER_POLICY,
>> +	.verify		= cpufreq_generic_frequency_table_verify,
>> +	.target_index	= qcom_cpufreq_hw_target_index,
>> +	.get		= qcom_cpufreq_hw_get,
>> +	.init		= qcom_cpufreq_hw_cpu_init,
>> +	.fast_switch    = qcom_cpufreq_hw_fast_switch,
>> +	.name		= "qcom-cpufreq-hw",
>> +	.attr		= qcom_cpufreq_hw_attr,
>> +	.boost_enabled	= true,
>> +};
>> +
>> +static int qcom_cpufreq_hw_read_lut(struct platform_device *pdev,
>> +				    struct cpufreq_qcom *c)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	void __iomem *base;
>> +	u32 data, src, lval, i, core_count, prev_cc, prev_freq, cur_freq;
>> +
>> +	c->table = devm_kcalloc(dev, LUT_MAX_ENTRIES + 1,
>> +				sizeof(*c->table), GFP_KERNEL);
>> +	if (!c->table)
>> +		return -ENOMEM;
>> +
>> +	base = c->reg_bases[REG_LUT_TABLE];
>> +
>> +	for (i = 0; i < LUT_MAX_ENTRIES; i++) {
>> +		data = readl_relaxed(base + i * LUT_ROW_SIZE);
>> +		src = (data & GENMASK(31, 30)) >> 30;
>> +		lval = data & GENMASK(7, 0);
>> +		core_count = CORE_COUNT_VAL(data);
>> +
>> +		if (src)
>> +			c->table[i].frequency = c->xo_rate * lval / 1000;
>> +		else
>> +			c->table[i].frequency = INIT_RATE / 1000;
>> +
>> +		cur_freq = c->table[i].frequency;
>> +
>> +		dev_dbg(dev, "index=%d freq=%d, core_count %d\n",
>> +			i, c->table[i].frequency, core_count);
>> +
>> +		if (core_count != c->max_cores)
>> +			cur_freq = CPUFREQ_ENTRY_INVALID;
>> +
> 
> I noticed that the 'power_allocator' thermal governor currently can't
> be used with this driver since there is no OPP table with frequency and
> voltage information. Does the LUT contain information about the
> voltage or is there another mechanism to retrieve it?
> 

No, currently there is no way of reading the voltage information.

Matthias Kaehlcke Sept. 24, 2018, 4:44 p.m. UTC | #15

On Sun, Sep 23, 2018 at 03:18:20PM +0530, Taniya Das wrote:
> 
> 
> On 9/11/2018 1:00 AM, Matthias Kaehlcke wrote:
> > On Tue, Jul 24, 2018 at 04:12:50PM +0530, Taniya Das wrote:
> > > The CPUfreq HW present in some QCOM chipsets offloads the steps necessary
> > > for changing the frequency of CPUs. The driver implements the cpufreq
> > > driver interface for this hardware engine.
> > > 
> > > Signed-off-by: Saravana Kannan <[e-mail]>
> > > Signed-off-by: Taniya Das <[e-mail]>
> > > ---
> > >   drivers/cpufreq/Kconfig.arm       |  11 ++
> > >   drivers/cpufreq/Makefile          |   1 +
> > >   drivers/cpufreq/qcom-cpufreq-hw.c | 348 ++++++++++++++++++++++++++++++++++++++
> > >   3 files changed, 360 insertions(+)
> > >   create mode 100644 drivers/cpufreq/qcom-cpufreq-hw.c
> > > 
> > > diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> > > index 0cd8eb7..93a9d72 100644
> > > --- a/drivers/cpufreq/Kconfig.arm
> > > +++ b/drivers/cpufreq/Kconfig.arm
> > > @@ -298,3 +298,14 @@ config ARM_PXA2xx_CPUFREQ
> > >   	  This add the CPUFreq driver support for Intel PXA2xx SOCs.
> > > 
> > >   	  If in doubt, say N.
> > > +
> > > +config ARM_QCOM_CPUFREQ_HW
> > > +	bool "QCOM CPUFreq HW driver"
> > > +	depends on ARCH_QCOM
> > > +	help
> > > +	 Support for the CPUFreq HW driver.
> > > +	 Some QCOM chipsets have a HW engine to offload the steps
> > > +	 necessary for changing the frequency of the CPUs. Firmware loaded
> > > +	 in this engine exposes a programming interface to the OS.
> > > +	 The driver implements the cpufreq interface for this HW engine.
> > > +	 Say Y if you want to support CPUFreq HW.
> > > diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> > > index c1ffeab..ca48a1d 100644
> > > --- a/drivers/cpufreq/Makefile
> > > +++ b/drivers/cpufreq/Makefile
> > > @@ -85,6 +85,7 @@ obj-$(CONFIG_ARM_TEGRA124_CPUFREQ)	+= tegra124-cpufreq.o
> > >   obj-$(CONFIG_ARM_TEGRA186_CPUFREQ)	+= tegra186-cpufreq.o
> > >   obj-$(CONFIG_ARM_TI_CPUFREQ)		+= ti-cpufreq.o
> > >   obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ)	+= vexpress-spc-cpufreq.o
> > > +obj-$(CONFIG_ARM_QCOM_CPUFREQ_HW)	+= qcom-cpufreq-hw.o
> > > 
> > > 
> > >   ##################################################################################
> > > diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> > > new file mode 100644
> > > index 0000000..ea8f7d1
> > > --- /dev/null
> > > +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> > > @@ -0,0 +1,348 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> > > + */
> > > +
> > > +#include <linux/cpufreq.h>
> > > +#include <linux/init.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of_address.h>
> > > +#include <linux/of_platform.h>
> > > +
> > > +#define INIT_RATE			300000000UL
> > > +#define LUT_MAX_ENTRIES			40U
> > > +#define CORE_COUNT_VAL(val)		(((val) & (GENMASK(18, 16))) >> 16)
> > > +#define LUT_ROW_SIZE			32
> > > +
> > > +enum {
> > > +	REG_ENABLE,
> > > +	REG_LUT_TABLE,
> > > +	REG_PERF_STATE,
> > > +
> > > +	REG_ARRAY_SIZE,
> > > +};
> > > +
> > > +struct cpufreq_qcom {
> > > +	struct cpufreq_frequency_table *table;
> > > +	struct device *dev;
> > > +	void __iomem *reg_bases[REG_ARRAY_SIZE];
> > > +	cpumask_t related_cpus;
> > > +	unsigned int max_cores;
> > > +	unsigned long xo_rate;
> > > +};
> > > +
> > > +static const u16 cpufreq_qcom_std_offsets[REG_ARRAY_SIZE] = {
> > > +	[REG_ENABLE]		= 0x0,
> > > +	[REG_LUT_TABLE]		= 0x110,
> > > +	[REG_PERF_STATE]	= 0x920,
> > > +};
> > > +
> > > +static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
> > > +
> > > +static int
> > > +qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
> > > +			     unsigned int index)
> > > +{
> > > +	struct cpufreq_qcom *c = policy->driver_data;
> > > +
> > > +	writel_relaxed(index, c->reg_bases[REG_PERF_STATE]);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static unsigned int qcom_cpufreq_hw_get(unsigned int cpu)
> > > +{
> > > +	struct cpufreq_qcom *c;
> > > +	struct cpufreq_policy *policy;
> > > +	unsigned int index;
> > > +
> > > +	policy = cpufreq_cpu_get_raw(cpu);
> > > +	if (!policy)
> > > +		return 0;
> > > +
> > > +	c = policy->driver_data;
> > > +
> > > +	index = readl_relaxed(c->reg_bases[REG_PERF_STATE]);
> > > +	index = min(index, LUT_MAX_ENTRIES - 1);
> > > +
> > > +	return policy->freq_table[index].frequency;
> > > +}
> > > +
> > > +static unsigned int
> > > +qcom_cpufreq_hw_fast_switch(struct cpufreq_policy *policy,
> > > +			    unsigned int target_freq)
> > > +{
> > > +	struct cpufreq_qcom *c = policy->driver_data;
> > > +	int index;
> > > +
> > > +	index = policy->cached_resolved_idx;
> > > +	if (index < 0)
> > > +		return 0;
> > > +
> > > +	writel_relaxed(index, c->reg_bases[REG_PERF_STATE]);
> > > +
> > > +	return policy->freq_table[index].frequency;
> > > +}
> > > +
> > > +static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
> > > +{
> > > +	struct cpufreq_qcom *c;
> > > +
> > > +	c = qcom_freq_domain_map[policy->cpu];
> > > +	if (!c) {
> > > +		pr_err("No scaling support for CPU%d\n", policy->cpu);
> > > +		return -ENODEV;
> > > +	}
> > > +
> > > +	cpumask_copy(policy->cpus, &c->related_cpus);
> > > +
> > > +	policy->fast_switch_possible = true;
> > > +	policy->freq_table = c->table;
> > > +	policy->driver_data = c;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static struct freq_attr *qcom_cpufreq_hw_attr[] = {
> > > +	&cpufreq_freq_attr_scaling_available_freqs,
> > > +	&cpufreq_freq_attr_scaling_boost_freqs,
> > > +	NULL
> > > +};
> > > +
> > > +static struct cpufreq_driver cpufreq_qcom_hw_driver = {
> > > +	.flags		= CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK |
> > > +			  CPUFREQ_HAVE_GOVERNOR_PER_POLICY,
> > > +	.verify		= cpufreq_generic_frequency_table_verify,
> > > +	.target_index	= qcom_cpufreq_hw_target_index,
> > > +	.get		= qcom_cpufreq_hw_get,
> > > +	.init		= qcom_cpufreq_hw_cpu_init,
> > > +	.fast_switch    = qcom_cpufreq_hw_fast_switch,
> > > +	.name		= "qcom-cpufreq-hw",
> > > +	.attr		= qcom_cpufreq_hw_attr,
> > > +	.boost_enabled	= true,
> > > +};
> > > +
> > > +static int qcom_cpufreq_hw_read_lut(struct platform_device *pdev,
> > > +				    struct cpufreq_qcom *c)
> > > +{
> > > +	struct device *dev = &pdev->dev;
> > > +	void __iomem *base;
> > > +	u32 data, src, lval, i, core_count, prev_cc, prev_freq, cur_freq;
> > > +
> > > +	c->table = devm_kcalloc(dev, LUT_MAX_ENTRIES + 1,
> > > +				sizeof(*c->table), GFP_KERNEL);
> > > +	if (!c->table)
> > > +		return -ENOMEM;
> > > +
> > > +	base = c->reg_bases[REG_LUT_TABLE];
> > > +
> > > +	for (i = 0; i < LUT_MAX_ENTRIES; i++) {
> > > +		data = readl_relaxed(base + i * LUT_ROW_SIZE);
> > > +		src = (data & GENMASK(31, 30)) >> 30;
> > > +		lval = data & GENMASK(7, 0);
> > > +		core_count = CORE_COUNT_VAL(data);
> > > +
> > > +		if (src)
> > > +			c->table[i].frequency = c->xo_rate * lval / 1000;
> > > +		else
> > > +			c->table[i].frequency = INIT_RATE / 1000;
> > > +
> > > +		cur_freq = c->table[i].frequency;
> > > +
> > > +		dev_dbg(dev, "index=%d freq=%d, core_count %d\n",
> > > +			i, c->table[i].frequency, core_count);
> > > +
> > > +		if (core_count != c->max_cores)
> > > +			cur_freq = CPUFREQ_ENTRY_INVALID;
> > > +
> > 
> > I noticed that the 'power_allocator' thermal governor currently can't
> > be used with this driver since there is no OPP table with frequency and
> > voltage information. Does the LUT contain information about the
> > voltage or is there another mechanism to retrieve it?
> > 
> 
> No, currently there is no way of reading the voltage information.

That leaves the 'power_allocator' out of question :(

Which thermal governor is/should typically be used on these systems?
Step wise and user space should work out of the box, however the
response of step wise could be slow (step by step) and user space
requires a thermal daemon (and could suffer from latencies). Fair
share could be an option if the thermal cooling devices are registered
with a 'weight', which could come from the device tree.

Matthias Kaehlcke Sept. 24, 2018, 5:02 p.m. UTC | #16

On Sun, Sep 23, 2018 at 03:10:55PM +0530, Taniya Das wrote:
> Hello Matthias,
> 
> Thanks for your review comments.
> 
> On 8/29/2018 11:31 PM, Matthias Kaehlcke wrote:
> > Hi Taniya,
> > 
> > On Tue, Jul 24, 2018 at 04:12:50PM +0530, Taniya Das wrote:
> > > The CPUfreq HW present in some QCOM chipsets offloads the steps necessary
> > > for changing the frequency of CPUs. The driver implements the cpufreq
> > > driver interface for this hardware engine.
> > > 
> > > Signed-off-by: Saravana Kannan <[e-mail]>
> > > Signed-off-by: Taniya Das <[e-mail]>
> > > ---
> > >   drivers/cpufreq/Kconfig.arm       |  11 ++
> > >   drivers/cpufreq/Makefile          |   1 +
> > >   drivers/cpufreq/qcom-cpufreq-hw.c | 348 ++++++++++++++++++++++++++++++++++++++
> > >   3 files changed, 360 insertions(+)
> > >   create mode 100644 drivers/cpufreq/qcom-cpufreq-hw.c
> > > 
> > > diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> > > index 0cd8eb7..93a9d72 100644
> > > --- a/drivers/cpufreq/Kconfig.arm
> > > +++ b/drivers/cpufreq/Kconfig.arm
> > > @@ -298,3 +298,14 @@ config ARM_PXA2xx_CPUFREQ
> > >   	  This add the CPUFreq driver support for Intel PXA2xx SOCs.
> > > 
> > >   	  If in doubt, say N.
> > > +
> > > +config ARM_QCOM_CPUFREQ_HW
> > > +	bool "QCOM CPUFreq HW driver"
> > > +	depends on ARCH_QCOM
> > > +	help
> > > +	 Support for the CPUFreq HW driver.
> > > +	 Some QCOM chipsets have a HW engine to offload the steps
> > > +	 necessary for changing the frequency of the CPUs. Firmware loaded
> > > +	 in this engine exposes a programming interface to the OS.
> > > +	 The driver implements the cpufreq interface for this HW engine.
> > > +	 Say Y if you want to support CPUFreq HW.
> > > diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> > > index c1ffeab..ca48a1d 100644
> > > --- a/drivers/cpufreq/Makefile
> > > +++ b/drivers/cpufreq/Makefile
> > > @@ -85,6 +85,7 @@ obj-$(CONFIG_ARM_TEGRA124_CPUFREQ)	+= tegra124-cpufreq.o
> > >   obj-$(CONFIG_ARM_TEGRA186_CPUFREQ)	+= tegra186-cpufreq.o
> > >   obj-$(CONFIG_ARM_TI_CPUFREQ)		+= ti-cpufreq.o
> > >   obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ)	+= vexpress-spc-cpufreq.o
> > > +obj-$(CONFIG_ARM_QCOM_CPUFREQ_HW)	+= qcom-cpufreq-hw.o
> > > 
> > > 
> > >   ##################################################################################
> > > diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> > > new file mode 100644
> > > index 0000000..ea8f7d1
> > > --- /dev/null
> > > +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> > > @@ -0,0 +1,348 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> > > + */
> > > +
> > > +#include <linux/cpufreq.h>
> > > +#include <linux/init.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of_address.h>
> > > +#include <linux/of_platform.h>
> > > +
> > > +#define INIT_RATE			300000000UL
> > > +#define LUT_MAX_ENTRIES			40U
> > > +#define CORE_COUNT_VAL(val)		(((val) & (GENMASK(18, 16))) >> 16)
> > > +#define LUT_ROW_SIZE			32
> > > +
> > > +enum {
> > > +	REG_ENABLE,
> > > +	REG_LUT_TABLE,
> > > +	REG_PERF_STATE,
> > > +
> > > +	REG_ARRAY_SIZE,
> > > +};
> > > +
> > > +struct cpufreq_qcom {
> > > +	struct cpufreq_frequency_table *table;
> > > +	struct device *dev;
> > 
> > 'dev' is not used and can be removed.
> > 
> 
> Thanks, would remove in the next patch.
> 
> > > ...
> > > 
> > > +static int qcom_cpu_resources_init(struct platform_device *pdev,
> > > +				   struct device_node *np, unsigned int cpu,
> > > +				   unsigned long xo_rate)
> > > +{
> > > +	struct cpufreq_qcom *c;
> > > +	struct resource res;
> > > +	struct device *dev = &pdev->dev;
> > > +	const u16 *offsets;
> > > +	int ret, i, cpu_first, cpu_r;
> > > +	void __iomem *base;
> > > +
> > > +	if (qcom_freq_domain_map[cpu])
> > > +		return 0;
> > > +
> > > +	c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
> > > +	if (!c)
> > > +		return -ENOMEM;
> > > +
> > > +	offsets = of_device_get_match_data(&pdev->dev);
> > > +	if (!offsets)
> > > +		return -EINVAL;
> > > +
> > > +	if (of_address_to_resource(np, 0, &res))
> > > +		return -ENOMEM;
> > > +
> > > +	base = devm_ioremap_resource(dev, &res);
> > > +	if (!base)
> > > +		return -ENOMEM;
> > > +
> > > +	for (i = REG_ENABLE; i < REG_ARRAY_SIZE; i++)
> > > +		c->reg_bases[i] = base + offsets[i];
> > > +
> > > +	/* HW should be in enabled state to proceed */
> > > +	if (!(readl_relaxed(c->reg_bases[REG_ENABLE]) & 0x1)) {
> > > +		dev_err(dev, "%s cpufreq hardware not enabled\n", np->name);
> > > +		return -ENODEV;
> > > +	}
> > > +
> > > +	ret = qcom_get_related_cpus(np, &c->related_cpus);
> > > +	if (ret) {
> > > +		dev_err(dev, "%s failed to get related CPUs\n", np->name);
> > > +		return ret;
> > > +	}
> > > +
> > > +	c->max_cores = cpumask_weight(&c->related_cpus);
> > > +	if (!c->max_cores)
> > > +		return -ENOENT;
> > > +
> > > +	c->xo_rate = xo_rate;
> > > +
> > > +	ret = qcom_cpufreq_hw_read_lut(pdev, c);
> > > +	if (ret) {
> > > +		dev_err(dev, "%s failed to read LUT\n", np->name);
> > > +		return ret;
> > > +	}
> > > +
> > > +	qcom_freq_domain_map[cpu] = c;
> > > +
> > > +	/* Related CPUs */
> > > +	cpu_first = cpumask_first(&c->related_cpus);
> > > +
> > > +	for_each_cpu(cpu_r, &c->related_cpus) {
> > > +		if (cpu_r != cpu_first)
> > > +			qcom_freq_domain_map[cpu_r] =
> > > +				qcom_freq_domain_map[cpu_first];
> > > +	}
> > 
> > The above ten lines could be simplified to:
> > 
> > 	for_each_cpu(cpu_r, &c->related_cpus)
> > 		qcom_freq_domain_map[cpu_r] = c;
> > 
> 
> Would clean it up in the next patch.
> 
> > > ...
> > > 
> > > +static int __init qcom_cpufreq_hw_init(void)
> > > +{
> > > +	return platform_driver_register(&qcom_cpufreq_hw_driver);
> > > +}
> > > +subsys_initcall(qcom_cpufreq_hw_init);
> > 
> > Is subsys_initcall used for a particular reason? It will cause
> > problems when registering cooling devices, since the thermal device
> > class is initialized through an fs_initcall, which are executed
> > later.
> > 
> > Most cpufreq drivers use module_init, device_initcall or
> > late_initcall, can't this driver use one of those?
> > 
> 
> We want the CPU to be scaling to the highest frequency at the
> earliest.

I guess you also want thermal management for the CPU. With the
subsys_initcall registration of cooling devices fails, as mentioned in
my earlier comment. Do you plan to defer the registration of cooling
devices?

Cheers

Matthias

Patch

diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
index 0cd8eb7..93a9d72 100644
--- a/drivers/cpufreq/Kconfig.arm
+++ b/drivers/cpufreq/Kconfig.arm
@@ -298,3 +298,14 @@  config ARM_PXA2xx_CPUFREQ
 	  This add the CPUFreq driver support for Intel PXA2xx SOCs.

 	  If in doubt, say N.
+
+config ARM_QCOM_CPUFREQ_HW
+	bool "QCOM CPUFreq HW driver"
+	depends on ARCH_QCOM
+	help
+	 Support for the CPUFreq HW driver.
+	 Some QCOM chipsets have a HW engine to offload the steps
+	 necessary for changing the frequency of the CPUs. Firmware loaded
+	 in this engine exposes a programming interface to the OS.
+	 The driver implements the cpufreq interface for this HW engine.
+	 Say Y if you want to support CPUFreq HW.
diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
index c1ffeab..ca48a1d 100644
--- a/drivers/cpufreq/Makefile
+++ b/drivers/cpufreq/Makefile
@@ -85,6 +85,7 @@  obj-$(CONFIG_ARM_TEGRA124_CPUFREQ)	+= tegra124-cpufreq.o
 obj-$(CONFIG_ARM_TEGRA186_CPUFREQ)	+= tegra186-cpufreq.o
 obj-$(CONFIG_ARM_TI_CPUFREQ)		+= ti-cpufreq.o
 obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ)	+= vexpress-spc-cpufreq.o
+obj-$(CONFIG_ARM_QCOM_CPUFREQ_HW)	+= qcom-cpufreq-hw.o


 ##################################################################################
diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
new file mode 100644
index 0000000..ea8f7d1
--- /dev/null
+++ b/drivers/cpufreq/qcom-cpufreq-hw.c
@@ -0,0 +1,348 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ */
+
+#include <linux/cpufreq.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
+
+#define INIT_RATE			300000000UL
+#define LUT_MAX_ENTRIES			40U
+#define CORE_COUNT_VAL(val)		(((val) & (GENMASK(18, 16))) >> 16)
+#define LUT_ROW_SIZE			32
+
+enum {
+	REG_ENABLE,
+	REG_LUT_TABLE,
+	REG_PERF_STATE,
+
+	REG_ARRAY_SIZE,
+};
+
+struct cpufreq_qcom {
+	struct cpufreq_frequency_table *table;
+	struct device *dev;
+	void __iomem *reg_bases[REG_ARRAY_SIZE];
+	cpumask_t related_cpus;
+	unsigned int max_cores;
+	unsigned long xo_rate;
+};
+
+static const u16 cpufreq_qcom_std_offsets[REG_ARRAY_SIZE] = {
+	[REG_ENABLE]		= 0x0,
+	[REG_LUT_TABLE]		= 0x110,
+	[REG_PERF_STATE]	= 0x920,
+};
+
+static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
+
+static int
+qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
+			     unsigned int index)
+{
+	struct cpufreq_qcom *c = policy->driver_data;
+
+	writel_relaxed(index, c->reg_bases[REG_PERF_STATE]);
+
+	return 0;
+}
+
+static unsigned int qcom_cpufreq_hw_get(unsigned int cpu)
+{
+	struct cpufreq_qcom *c;
+	struct cpufreq_policy *policy;
+	unsigned int index;
+
+	policy = cpufreq_cpu_get_raw(cpu);
+	if (!policy)
+		return 0;
+
+	c = policy->driver_data;
+
+	index = readl_relaxed(c->reg_bases[REG_PERF_STATE]);
+	index = min(index, LUT_MAX_ENTRIES - 1);
+
+	return policy->freq_table[index].frequency;
+}
+
+static unsigned int
+qcom_cpufreq_hw_fast_switch(struct cpufreq_policy *policy,
+			    unsigned int target_freq)
+{
+	struct cpufreq_qcom *c = policy->driver_data;
+	int index;
+
+	index = policy->cached_resolved_idx;
+	if (index < 0)
+		return 0;
+
+	writel_relaxed(index, c->reg_bases[REG_PERF_STATE]);
+
+	return policy->freq_table[index].frequency;
+}
+
+static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
+{
+	struct cpufreq_qcom *c;
+
+	c = qcom_freq_domain_map[policy->cpu];
+	if (!c) {
+		pr_err("No scaling support for CPU%d\n", policy->cpu);
+		return -ENODEV;
+	}
+
+	cpumask_copy(policy->cpus, &c->related_cpus);
+
+	policy->fast_switch_possible = true;
+	policy->freq_table = c->table;
+	policy->driver_data = c;
+
+	return 0;
+}
+
+static struct freq_attr *qcom_cpufreq_hw_attr[] = {
+	&cpufreq_freq_attr_scaling_available_freqs,
+	&cpufreq_freq_attr_scaling_boost_freqs,
+	NULL
+};
+
+static struct cpufreq_driver cpufreq_qcom_hw_driver = {
+	.flags		= CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK |
+			  CPUFREQ_HAVE_GOVERNOR_PER_POLICY,
+	.verify		= cpufreq_generic_frequency_table_verify,
+	.target_index	= qcom_cpufreq_hw_target_index,
+	.get		= qcom_cpufreq_hw_get,
+	.init		= qcom_cpufreq_hw_cpu_init,
+	.fast_switch    = qcom_cpufreq_hw_fast_switch,
+	.name		= "qcom-cpufreq-hw",
+	.attr		= qcom_cpufreq_hw_attr,
+	.boost_enabled	= true,
+};
+
+static int qcom_cpufreq_hw_read_lut(struct platform_device *pdev,
+				    struct cpufreq_qcom *c)
+{
+	struct device *dev = &pdev->dev;
+	void __iomem *base;
+	u32 data, src, lval, i, core_count, prev_cc, prev_freq, cur_freq;
+
+	c->table = devm_kcalloc(dev, LUT_MAX_ENTRIES + 1,
+				sizeof(*c->table), GFP_KERNEL);
+	if (!c->table)
+		return -ENOMEM;
+
+	base = c->reg_bases[REG_LUT_TABLE];
+
+	for (i = 0; i < LUT_MAX_ENTRIES; i++) {
+		data = readl_relaxed(base + i * LUT_ROW_SIZE);
+		src = (data & GENMASK(31, 30)) >> 30;
+		lval = data & GENMASK(7, 0);
+		core_count = CORE_COUNT_VAL(data);
+
+		if (src)
+			c->table[i].frequency = c->xo_rate * lval / 1000;
+		else
+			c->table[i].frequency = INIT_RATE / 1000;
+
+		cur_freq = c->table[i].frequency;
+
+		dev_dbg(dev, "index=%d freq=%d, core_count %d\n",
+			i, c->table[i].frequency, core_count);
+
+		if (core_count != c->max_cores)
+			cur_freq = CPUFREQ_ENTRY_INVALID;
+
+		/*
+		 * Two of the same frequencies with the same core counts means
+		 * end of table.
+		 */
+		if (i > 0 && c->table[i - 1].frequency ==
+		   c->table[i].frequency && prev_cc == core_count) {
+			struct cpufreq_frequency_table *prev = &c->table[i - 1];
+
+			if (prev_freq == CPUFREQ_ENTRY_INVALID)
+				prev->flags = CPUFREQ_BOOST_FREQ;
+			break;
+		}
+		prev_cc = core_count;
+		prev_freq = cur_freq;
+	}
+
+	c->table[i].frequency = CPUFREQ_TABLE_END;
+
+	return 0;
+}
+
+static int qcom_get_related_cpus(struct device_node *np, struct cpumask *m)
+{
+	struct device_node *cpu_np, *freq_np;
+	int cpu;
+
+	for_each_possible_cpu(cpu) {
+		cpu_np = of_cpu_device_node_get(cpu);
+		if (!cpu_np)
+			continue;
+		freq_np = of_parse_phandle(cpu_np, "qcom,freq-domain", 0);
+		of_node_put(cpu_np);
+		if (!freq_np)
+			continue;
+
+		if (freq_np == np)
+			cpumask_set_cpu(cpu, m);
+	}
+
+	return 0;
+}
+
+static int qcom_cpu_resources_init(struct platform_device *pdev,
+				   struct device_node *np, unsigned int cpu,
+				   unsigned long xo_rate)
+{
+	struct cpufreq_qcom *c;
+	struct resource res;
+	struct device *dev = &pdev->dev;
+	const u16 *offsets;
+	int ret, i, cpu_first, cpu_r;
+	void __iomem *base;
+
+	if (qcom_freq_domain_map[cpu])
+		return 0;
+
+	c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
+	if (!c)
+		return -ENOMEM;
+
+	offsets = of_device_get_match_data(&pdev->dev);
+	if (!offsets)
+		return -EINVAL;
+
+	if (of_address_to_resource(np, 0, &res))
+		return -ENOMEM;
+
+	base = devm_ioremap_resource(dev, &res);
+	if (!base)
+		return -ENOMEM;
+
+	for (i = REG_ENABLE; i < REG_ARRAY_SIZE; i++)
+		c->reg_bases[i] = base + offsets[i];
+
+	/* HW should be in enabled state to proceed */
+	if (!(readl_relaxed(c->reg_bases[REG_ENABLE]) & 0x1)) {
+		dev_err(dev, "%s cpufreq hardware not enabled\n", np->name);
+		return -ENODEV;
+	}
+
+	ret = qcom_get_related_cpus(np, &c->related_cpus);
+	if (ret) {
+		dev_err(dev, "%s failed to get related CPUs\n", np->name);
+		return ret;
+	}
+
+	c->max_cores = cpumask_weight(&c->related_cpus);
+	if (!c->max_cores)
+		return -ENOENT;
+
+	c->xo_rate = xo_rate;
+
+	ret = qcom_cpufreq_hw_read_lut(pdev, c);
+	if (ret) {
+		dev_err(dev, "%s failed to read LUT\n", np->name);
+		return ret;
+	}
+
+	qcom_freq_domain_map[cpu] = c;
+
+	/* Related CPUs */
+	cpu_first = cpumask_first(&c->related_cpus);
+
+	for_each_cpu(cpu_r, &c->related_cpus) {
+		if (cpu_r != cpu_first)
+			qcom_freq_domain_map[cpu_r] =
+				qcom_freq_domain_map[cpu_first];
+	}
+
+	return 0;
+}
+
+static int qcom_resources_init(struct platform_device *pdev)
+{
+	struct device_node *np, *cpu_np;
+	struct clk *clk;
+	unsigned int cpu;
+	int ret;
+
+	clk = devm_clk_get(&pdev->dev, "xo");
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+
+	for_each_possible_cpu(cpu) {
+		cpu_np = of_cpu_device_node_get(cpu);
+		if (!cpu_np) {
+			dev_dbg(&pdev->dev, "Failed to get cpu %d device\n",
+				cpu);
+			continue;
+		}
+
+		np = of_parse_phandle(cpu_np, "qcom,freq-domain", 0);
+		of_node_put(cpu_np);
+		if (!np) {
+			dev_err(&pdev->dev, "Failed to get freq-domain device\n");
+			return -EINVAL;
+		}
+
+		ret = qcom_cpu_resources_init(pdev, np, cpu, clk_get_rate(clk));
+		if (ret)
+			return ret;
+	}
+
+	devm_clk_put(&pdev->dev, clk);
+
+	return 0;
+}
+
+static int qcom_cpufreq_hw_driver_probe(struct platform_device *pdev)
+{
+	int rc;
+
+	/* Get the bases of cpufreq for domains */
+	rc = qcom_resources_init(pdev);
+	if (rc) {
+		dev_err(&pdev->dev, "CPUFreq resource init failed\n");
+		return rc;
+	}
+
+	rc = cpufreq_register_driver(&cpufreq_qcom_hw_driver);
+	if (rc) {
+		dev_err(&pdev->dev, "CPUFreq HW driver failed to register\n");
+		return rc;
+	}
+
+	dev_dbg(&pdev->dev, "QCOM CPUFreq HW driver initialized\n");
+
+	return 0;
+}
+
+static const struct of_device_id qcom_cpufreq_hw_match[] = {
+	{ .compatible = "qcom,cpufreq-hw", .data = &cpufreq_qcom_std_offsets },
+	{}
+};
+
+static struct platform_driver qcom_cpufreq_hw_driver = {
+	.probe = qcom_cpufreq_hw_driver_probe,
+	.driver = {
+		.name = "qcom-cpufreq-hw",
+		.of_match_table = qcom_cpufreq_hw_match,
+	},
+};
+
+static int __init qcom_cpufreq_hw_init(void)
+{
+	return platform_driver_register(&qcom_cpufreq_hw_driver);
+}
+subsys_initcall(qcom_cpufreq_hw_init);
+
+MODULE_DESCRIPTION("QCOM firmware-based CPU Frequency driver");