Re: [PATCH v6 5/8] PM / devfreq: tegra: remove operating-points

Re: [PATCH v6 5/8] PM / devfreq: tegra: remove operating-points

[MyungJoo Ham]

> As the DT bindings don't have an operating-points property any more,
> build the OPP table from the frequencies supported by the EMC clock.
> 
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> ---
>  drivers/devfreq/tegra-devfreq.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
> index 5a6164c..1de3f8b 100644
> --- a/drivers/devfreq/tegra-devfreq.c
> +++ b/drivers/devfreq/tegra-devfreq.c
> @@ -618,6 +618,7 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>  	struct tegra_devfreq_device *dev;
>  	struct resource *res;
>  	unsigned int i;
> +	unsigned long rate;
>  	int irq;
>  	int err;
>  
> @@ -649,12 +650,6 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>  		return PTR_ERR(tegra->emc_clock);
>  	}
>  
> -	err = of_init_opp_table(&pdev->dev);
> -	if (err) {
> -		dev_err(&pdev->dev, "Failed to init operating point table\n");
> -		return err;
> -	}
> -
>  	clk_set_rate(tegra->emc_clock, ULONG_MAX);
>  
>  	tegra->rate_change_nb.notifier_call = tegra_actmon_rate_notify_cb;
> @@ -691,6 +686,11 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>  		tegra_actmon_configure_device(tegra, dev);
>  	}
>  
> +	for (rate = 0; rate <= tegra->max_freq * KHZ; rate++) {
> +		rate = clk_round_rate(tegra->emc_clock, rate);
> +		dev_pm_opp_add(&pdev->dev, rate, 0);
> +	}
> +

Although I am not going to NACK for the single-time performance of a
single device driver for a device that I do not have or fully understand,
please note that you may be wasting several billion cycles unless
your product is running at MHZ/kHZ level.

What is going on with this loop? Do you really have such a virtually-continuous
frequency scaling in your product? (Wow.... but in such a case, I don't think
OPP is appropriate.)


Cheers,
MyungJoo


>  	irq = platform_get_irq(pdev, 0);
>  	if (irq <= 0) {
>  		dev_err(&pdev->dev, "Failed to get IRQ\n");
> -- 
> 2.1.0
> 

Re: [PATCH v6 5/8] PM / devfreq: tegra: remove operating-points

[Tomeu Vizoso]

On 18 March 2015 at 06:23, MyungJoo Ham <myungjoo.ham@samsung.com> wrote:
>> As the DT bindings don't have an operating-points property any more,
>> build the OPP table from the frequencies supported by the EMC clock.
>>
>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>> ---
>>  drivers/devfreq/tegra-devfreq.c | 12 ++++++------
>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
>> index 5a6164c..1de3f8b 100644
>> --- a/drivers/devfreq/tegra-devfreq.c
>> +++ b/drivers/devfreq/tegra-devfreq.c
>> @@ -618,6 +618,7 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>>       struct tegra_devfreq_device *dev;
>>       struct resource *res;
>>       unsigned int i;
>> +     unsigned long rate;
>>       int irq;
>>       int err;
>>
>> @@ -649,12 +650,6 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>>               return PTR_ERR(tegra->emc_clock);
>>       }
>>
>> -     err = of_init_opp_table(&pdev->dev);
>> -     if (err) {
>> -             dev_err(&pdev->dev, "Failed to init operating point table\n");
>> -             return err;
>> -     }
>> -
>>       clk_set_rate(tegra->emc_clock, ULONG_MAX);
>>
>>       tegra->rate_change_nb.notifier_call = tegra_actmon_rate_notify_cb;
>> @@ -691,6 +686,11 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>>               tegra_actmon_configure_device(tegra, dev);
>>       }
>>
>> +     for (rate = 0; rate <= tegra->max_freq * KHZ; rate++) {
>> +             rate = clk_round_rate(tegra->emc_clock, rate);
>> +             dev_pm_opp_add(&pdev->dev, rate, 0);
>> +     }
>> +
>
> Although I am not going to NACK for the single-time performance of a
> single device driver for a device that I do not have or fully understand,
> please note that you may be wasting several billion cycles unless
> your product is running at MHZ/kHZ level.
>
> What is going on with this loop? Do you really have such a virtually-continuous
> frequency scaling in your product? (Wow.... but in such a case, I don't think
> OPP is appropriate.)

Actually, that loop is expected to only execute as many times as
frequencies are supported by the clock. This is using knowledge of the
clock always rounding up when possible, which I think is fine in this
case.

Regards,

Tomeu

>
> Cheers,
> MyungJoo
>
>
>>       irq = platform_get_irq(pdev, 0);
>>       if (irq <= 0) {
>>               dev_err(&pdev->dev, "Failed to get IRQ\n");
>> --
>> 2.1.0
>>

[PATCH v6 0/8] Add support for Tegra Activity Monitor

[Tomeu Vizoso]

Hello,

something happened during the last cycle and an old version of the devfreq
driver was merged.

This thread contains patches that bring it up to date to the last submitted
version and also incorporates the feedback that that version received, plus
some other small fixes and improvements that came up during rebase and
testing.

These patches implement support for setting the rate of the EMC clock based on
stats collected from the ACTMON, a piece of hw in the Tegra124 that counts
memory accesses (among others).

It depends on the following in-flight patches:

* EMC driver: http://thread.gmane.org/gmane.linux.kernel/1907035
* CPUFreq driver: http://thread.gmane.org/gmane.linux.kernel/1897078

I have pushed a branch here for testing:

http://cgit.collabora.com/git/user/tomeu/linux.git/log/?h=actmon-v6

Regards,

Tomeu

Tomeu Vizoso (8):
  of: Add binding for NVIDIA Tegra ACTMON node
  PM / devfreq: tegra: Update to v5 of the submitted patches
  clk: tegra: Have EMC clock implement determine_rate()
  PM / devfreq: tegra: Use clock rate constraints
  PM / devfreq: tegra: remove operating-points
  PM / devfreq: tegra: Set drvdata before enabling the irq
  PM / devfreq: tegra: Enable interrupts after resuming the devfreq
    monitor
  ARM: tegra: Add Tegra124 ACTMON support

 .../devicetree/bindings/arm/tegra/actmon.txt       |  28 ++
 arch/arm/boot/dts/tegra124.dtsi                    |  11 +
 drivers/clk/tegra/clk-emc.c                        |  19 +-
 drivers/devfreq/tegra-devfreq.c                    | 480 +++++++++++----------
 4 files changed, 316 insertions(+), 222 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/tegra/actmon.txt

-- 
2.1.0

[PATCH v6 5/8] PM / devfreq: tegra: remove operating-points

[Tomeu Vizoso]

As the DT bindings don't have an operating-points property any more,
build the OPP table from the frequencies supported by the EMC clock.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
---
 drivers/devfreq/tegra-devfreq.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
index 5a6164c..1de3f8b 100644
--- a/drivers/devfreq/tegra-devfreq.c
+++ b/drivers/devfreq/tegra-devfreq.c
@@ -618,6 +618,7 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 	struct tegra_devfreq_device *dev;
 	struct resource *res;
 	unsigned int i;
+	unsigned long rate;
 	int irq;
 	int err;
 
@@ -649,12 +650,6 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 		return PTR_ERR(tegra->emc_clock);
 	}
 
-	err = of_init_opp_table(&pdev->dev);
-	if (err) {
-		dev_err(&pdev->dev, "Failed to init operating point table\n");
-		return err;
-	}
-
 	clk_set_rate(tegra->emc_clock, ULONG_MAX);
 
 	tegra->rate_change_nb.notifier_call = tegra_actmon_rate_notify_cb;
@@ -691,6 +686,11 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 		tegra_actmon_configure_device(tegra, dev);
 	}
 
+	for (rate = 0; rate <= tegra->max_freq * KHZ; rate++) {
+		rate = clk_round_rate(tegra->emc_clock, rate);
+		dev_pm_opp_add(&pdev->dev, rate, 0);
+	}
+
 	irq = platform_get_irq(pdev, 0);
 	if (irq <= 0) {
 		dev_err(&pdev->dev, "Failed to get IRQ\n");
-- 
2.1.0