diff mbox

[0/6] Add CPU Frequency scaling support on Armada 37xx

Message ID 874lp4ulbs.fsf@free-electrons.com (mailing list archive)
State New, archived
Headers show

Commit Message

Gregory CLEMENT Dec. 6, 2017, 11:50 a.m. UTC 
Hi Andre,
 
 On dim., déc. 03 2017, Andre Heider <a.heider@gmail.com> wrote:

> Hi Gregory,
>
> I applied this and the dvfs patch set ([PATCH 0/3] Add DVFS support on
> CPU clock for Armada 37xx) on top of 4.14.3 and gave it a try on
> espressobin.

Thanks for testing.

>
> Upon modprobe I get:
> [   66.231652] freq_table: Duplicate freq-table entries: 0
> [   66.236967] cpu cpu0: cpufreq_init: invalid frequency table: -22
> [   66.243534] freq_table: Duplicate freq-table entries: 0
> [   66.248575] cpu cpu1: cpufreq_init: invalid frequency table: -22
>
> Is this supposed to work already? Do I miss something?

I did a last rebase before sending the series to remove the avs part not
working yet. And during this rebase I introduced an bug.

I will send a v2 soon if  you are intersected by testing it right now,
here it is the fix:

iff --git a/drivers/cpufreq/armada-37xx-cpufreq.c b/drivers/cpufreq/armada-37xx-cpufreq.c
index 40c9a744cc6e..96c2600009b5 100644


Gregory

>
> Oh, and on that scenario it's not possible to rmmod the driver again, EBUSY.
>
> Thanks,
> Andre
>
> On 01/12/17 12:25, Gregory CLEMENT
> <gregory.clement@free-electrons.com> wrote:
>> Hi,
>>
>> This series adds the CPU Frequency support on Armada 37xx using
>> DVFS. It is based on the initial work of Evan Wang and Victor Gu.
>>
>> DVFS control is done by a set of registers from the North Bridge Power
>> Management block. The binding for this block is documented in patch 1.
>>
>> While adding a new cpufreq driver I found that the Kconfig and
>> Makefile were no more in order, so it is fixed by patch 2 and 3.
>>
>> The 4th patch is just about updating the MAINTAINERS file with the new
>> driver.
>>
>> The next patch is the real purpose of the series. The main goal of
>> this driver is to setup the CPU load level in the hardware to
>> associate them to CPU frequencies and register a standard cpufreq
>> driver. Note that the hardware also capable of doing AVS (Adaptive
>> Voltage Scaling), by associating a voltage on each level beside the
>> CPU frequency. However, this support is not yet ready, so it is not
>> part of this series.
>>
>> Finally, the last patch is for arm-soc the arm-soc subsystem through
>> mvebu and update the device tree to support the CPU frequency scaling.
>>
>> An update on the CPU clock driver is needed in order to take into
>> account the DVFS setting. It's the purpose of an other series already
>> sent, but is no dependencies between the series (for building or at
>> runtime).
>>
>> Thanks,
>>
>> Gregory
>>
>> Gregory CLEMENT (6):
>>    dt-bindings: marvell: Add documentation for the North Bridge PM on
>>      Armada 37xx
>>    cpufreq: ARM: sort the Kconfig menu
>>    cpufreq: sort the drivers in ARM part
>>    MAINTAINERS: add new entries for Armada 37xx cpufreq driver
>>    cpufreq: Add DVFS support for Armada 37xx
>>    arm64: dts: marvell: armada-37xx: add nodes allowing cpufreq support
>>
>>   .../bindings/arm/marvell/armada-37xx.txt           |  19 ++
>>   MAINTAINERS                                        |   1 +
>>   arch/arm64/boot/dts/marvell/armada-372x.dtsi       |   1 +
>>   arch/arm64/boot/dts/marvell/armada-37xx.dtsi       |   7 +
>>   drivers/cpufreq/Kconfig.arm                        |  89 ++++----
>>   drivers/cpufreq/Makefile                           |   9 +-
>>   drivers/cpufreq/armada-37xx-cpufreq.c              | 241 +++++++++++++++++++++
>>   7 files changed, 322 insertions(+), 45 deletions(-)
>>   create mode 100644 drivers/cpufreq/armada-37xx-cpufreq.c
>>
>

Comments

Andre Heider Dec. 6, 2017, 2:08 p.m. UTC | #1 
On 06/12/17 12:50, Gregory CLEMENT wrote:
> I did a last rebase before sending the series to remove the avs part not
> working yet. And during this rebase I introduced an bug.
> 
> I will send a v2 soon if  you are intersected by testing it right now,
> here it is the fix:
> 
> iff --git a/drivers/cpufreq/armada-37xx-cpufreq.c b/drivers/cpufreq/armada-37xx-cpufreq.c
> index 40c9a744cc6e..96c2600009b5 100644
> --- a/drivers/cpufreq/armada-37xx-cpufreq.c
> +++ b/drivers/cpufreq/armada-37xx-cpufreq.c
> @@ -219,7 +219,8 @@ static int __init armada37xx_cpufreq_driver_init(void)
>           */
>          for (load_level = ARMADA_37XX_DVFS_LOAD_0; load_level < LOAD_LEVEL_NR;
>               load_level++) {
> -               unsigned long freq = dvfs->divider[load_level];
> +               unsigned long freq = cur_frequency /
> +                       dvfs->divider[load_level];
>   
>                  ret = dev_pm_opp_add(cpu_dev, freq, 0);
>                  if (ret)

Much better, that seems to work so far, thanks!

My espressobin now gets these frequencies: 200 MHz, 250 MHz, 500 MHz and 
1000 MHz.

With the schedutil governor and `watch -n 0.2 cpufreq-info -f -m` it 
jumps over all of those depending on the load, nice:

Tested-by: Andre Heider <a.heider@gmail.com>

The measured power usage doesn't drop though, I guess that requires the 
AVS part you mentioned? Looking forward to it ;)

On a related note: Do you know if power usage can be lowered by 
disabling eth phys? Is that possible on mainline?

Regards,
Andre
Gregory CLEMENT Dec. 18, 2017, 5:03 p.m. UTC | #2 
Hi Andre,
 
 On mer., déc. 06 2017, Andre Heider <a.heider@gmail.com> wrote:

> On 06/12/17 12:50, Gregory CLEMENT wrote:
>> I did a last rebase before sending the series to remove the avs part not
>> working yet. And during this rebase I introduced an bug.
>>
>> I will send a v2 soon if  you are intersected by testing it right now,
>> here it is the fix:
>>
>> iff --git a/drivers/cpufreq/armada-37xx-cpufreq.c b/drivers/cpufreq/armada-37xx-cpufreq.c
>> index 40c9a744cc6e..96c2600009b5 100644
>> --- a/drivers/cpufreq/armada-37xx-cpufreq.c
>> +++ b/drivers/cpufreq/armada-37xx-cpufreq.c
>> @@ -219,7 +219,8 @@ static int __init armada37xx_cpufreq_driver_init(void)
>>           */
>>          for (load_level = ARMADA_37XX_DVFS_LOAD_0; load_level < LOAD_LEVEL_NR;
>>               load_level++) {
>> -               unsigned long freq = dvfs->divider[load_level];
>> +               unsigned long freq = cur_frequency /
>> +                       dvfs->divider[load_level];
>>                    ret = dev_pm_opp_add(cpu_dev, freq, 0);
>>                  if (ret)
>
> Much better, that seems to work so far, thanks!
>
> My espressobin now gets these frequencies: 200 MHz, 250 MHz, 500 MHz
> and 1000 MHz.
>
> With the schedutil governor and `watch -n 0.2 cpufreq-info -f -m` it
> jumps over all of those depending on the load, nice:
>
> Tested-by: Andre Heider <a.heider@gmail.com>
>
> The measured power usage doesn't drop though, I guess that requires
> the AVS part you mentioned? Looking forward to it ;)

Well I did some measurement and i saw some drop, according to my notes:

@ 250MHz: 222mA@12V => 2.66W
@ 1000MHz: 238mA@12CV => 2.87W

Not something huge, but only the CPUs which are concerned so it's not so
bad.

>
> On a related note: Do you know if power usage can be lowered by
> disabling eth phys? Is that possible on mainline?

For the test I have done just by removing an Ethernet cable we save a
lot of power, so it helps. I'm sure it is doable on mainline, but maybe
 currently, there is something still missing.

Gregory

>
> Regards,
> Andre
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
Andre Heider Dec. 21, 2017, 7:45 a.m. UTC | #3 
On 18/12/17 18:03, Gregory CLEMENT wrote:
> Hi Andre,
>   
>   On mer., déc. 06 2017, Andre Heider <a.heider@gmail.com> wrote:
> 
>> On 06/12/17 12:50, Gregory CLEMENT wrote:
>>> I did a last rebase before sending the series to remove the avs part not
>>> working yet. And during this rebase I introduced an bug.
>>>
>>> I will send a v2 soon if  you are intersected by testing it right now,
>>> here it is the fix:
>>>
>>> iff --git a/drivers/cpufreq/armada-37xx-cpufreq.c b/drivers/cpufreq/armada-37xx-cpufreq.c
>>> index 40c9a744cc6e..96c2600009b5 100644
>>> --- a/drivers/cpufreq/armada-37xx-cpufreq.c
>>> +++ b/drivers/cpufreq/armada-37xx-cpufreq.c
>>> @@ -219,7 +219,8 @@ static int __init armada37xx_cpufreq_driver_init(void)
>>>            */
>>>           for (load_level = ARMADA_37XX_DVFS_LOAD_0; load_level < LOAD_LEVEL_NR;
>>>                load_level++) {
>>> -               unsigned long freq = dvfs->divider[load_level];
>>> +               unsigned long freq = cur_frequency /
>>> +                       dvfs->divider[load_level];
>>>                     ret = dev_pm_opp_add(cpu_dev, freq, 0);
>>>                   if (ret)
>>
>> Much better, that seems to work so far, thanks!
>>
>> My espressobin now gets these frequencies: 200 MHz, 250 MHz, 500 MHz
>> and 1000 MHz.
>>
>> With the schedutil governor and `watch -n 0.2 cpufreq-info -f -m` it
>> jumps over all of those depending on the load, nice:
>>
>> Tested-by: Andre Heider <a.heider@gmail.com>
>>
>> The measured power usage doesn't drop though, I guess that requires
>> the AVS part you mentioned? Looking forward to it ;)
> 
> Well I did some measurement and i saw some drop, according to my notes:
> 
> @ 250MHz: 222mA@12V => 2.66W
> @ 1000MHz: 238mA@12CV => 2.87W
> 
> Not something huge, but only the CPUs which are concerned so it's not so
> bad.

Oh, okay, my cheap equipment isn't capable to measure a difference of 
just 0.2w... Guess I was hoping for more ;)

>> On a related note: Do you know if power usage can be lowered by
>> disabling eth phys? Is that possible on mainline?
> 
> For the test I have done just by removing an Ethernet cable we save a
> lot of power, so it helps. I'm sure it is doable on mainline, but maybe
>   currently, there is something still missing.

Promising, was that with a downstream kernel?

Thanks,
Andre
diff mbox

Patch

--- a/drivers/cpufreq/armada-37xx-cpufreq.c
+++ b/drivers/cpufreq/armada-37xx-cpufreq.c
@@ -219,7 +219,8 @@  static int __init armada37xx_cpufreq_driver_init(void)
         */
        for (load_level = ARMADA_37XX_DVFS_LOAD_0; load_level < LOAD_LEVEL_NR;
             load_level++) {
-               unsigned long freq = dvfs->divider[load_level];
+               unsigned long freq = cur_frequency /
+                       dvfs->divider[load_level];
 
                ret = dev_pm_opp_add(cpu_dev, freq, 0);
                if (ret)