[PATCH v1 1/2] cpufreq: tegra20: Constify rate value of the intermediate clk

[PATCH v1 1/2] cpufreq: tegra20: Constify rate value of the intermediate clk

[Dmitry Osipenko]

PLL_P is known to be always running at 216MHz, hence there is no need to
query its rate.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/cpufreq/tegra20-cpufreq.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c
index 05f57dcd5215..3ad6bded6efc 100644
--- a/drivers/cpufreq/tegra20-cpufreq.c
+++ b/drivers/cpufreq/tegra20-cpufreq.c
@@ -24,6 +24,8 @@
 #include <linux/platform_device.h>
 #include <linux/types.h>
 
+#define PLL_P_FREQ	216000
+
 static struct cpufreq_frequency_table freq_table[] = {
 	{ .frequency = 216000 },
 	{ .frequency = 312000 },
@@ -48,18 +50,15 @@ struct tegra20_cpufreq {
 static unsigned int tegra_get_intermediate(struct cpufreq_policy *policy,
 					   unsigned int index)
 {
-	struct tegra20_cpufreq *cpufreq = cpufreq_get_driver_data();
-	unsigned int ifreq = clk_get_rate(cpufreq->pll_p_clk) / 1000;
-
 	/*
 	 * Don't switch to intermediate freq if:
 	 * - we are already at it, i.e. policy->cur == ifreq
 	 * - index corresponds to ifreq
 	 */
-	if (freq_table[index].frequency == ifreq || policy->cur == ifreq)
+	if (index == 0 || policy->cur == PLL_P_FREQ)
 		return 0;
 
-	return ifreq;
+	return PLL_P_FREQ;
 }
 
 static int tegra_target_intermediate(struct cpufreq_policy *policy,
@@ -93,14 +92,13 @@ static int tegra_target(struct cpufreq_policy *policy, unsigned int index)
 {
 	struct tegra20_cpufreq *cpufreq = cpufreq_get_driver_data();
 	unsigned long rate = freq_table[index].frequency;
-	unsigned int ifreq = clk_get_rate(cpufreq->pll_p_clk) / 1000;
 	int ret;
 
 	/*
 	 * target freq == pll_p, don't need to take extra reference to pll_x_clk
 	 * as it isn't used anymore.
 	 */
-	if (rate == ifreq)
+	if (index == 0)
 		return clk_set_parent(cpufreq->cpu_clk, cpufreq->pll_p_clk);
 
 	ret = clk_set_rate(cpufreq->pll_x_clk, rate * 1000);
-- 
2.17.0

[PATCH v1 2/2] cpufreq: tegra20: Use PLL_C as intermediate clock source

[Dmitry Osipenko]

PLL_C is running at 600MHz which is significantly higher than the 216MHz
of the PLL_P and it is known that PLL_C is always-ON because AHB BUS is
running on that PLL. Let's use PLL_C as intermediate clock source, making
CPU snappier a tad during of the frequency transition.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/cpufreq/tegra20-cpufreq.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c
index 3ad6bded6efc..4bf5ba7da40b 100644
--- a/drivers/cpufreq/tegra20-cpufreq.c
+++ b/drivers/cpufreq/tegra20-cpufreq.c
@@ -25,12 +25,13 @@
 #include <linux/types.h>
 
 #define PLL_P_FREQ	216000
+#define PLL_C_FREQ	600000
 
 static struct cpufreq_frequency_table freq_table[] = {
 	{ .frequency = 216000 },
 	{ .frequency = 312000 },
 	{ .frequency = 456000 },
-	{ .frequency = 608000 },
+	{ .frequency = 600000 },
 	{ .frequency = 760000 },
 	{ .frequency = 816000 },
 	{ .frequency = 912000 },
@@ -44,6 +45,7 @@ struct tegra20_cpufreq {
 	struct clk *cpu_clk;
 	struct clk *pll_x_clk;
 	struct clk *pll_p_clk;
+	struct clk *pll_c_clk;
 	bool pll_x_prepared;
 };
 
@@ -58,7 +60,10 @@ static unsigned int tegra_get_intermediate(struct cpufreq_policy *policy,
 	if (index == 0 || policy->cur == PLL_P_FREQ)
 		return 0;
 
-	return PLL_P_FREQ;
+	if (index == 3 || policy->cur == PLL_C_FREQ)
+		return 0;
+
+	return PLL_C_FREQ;
 }
 
 static int tegra_target_intermediate(struct cpufreq_policy *policy,
@@ -79,7 +84,7 @@ static int tegra_target_intermediate(struct cpufreq_policy *policy,
 	 */
 	clk_prepare_enable(cpufreq->pll_x_clk);
 
-	ret = clk_set_parent(cpufreq->cpu_clk, cpufreq->pll_p_clk);
+	ret = clk_set_parent(cpufreq->cpu_clk, cpufreq->pll_c_clk);
 	if (ret)
 		clk_disable_unprepare(cpufreq->pll_x_clk);
 	else
@@ -101,6 +106,9 @@ static int tegra_target(struct cpufreq_policy *policy, unsigned int index)
 	if (index == 0)
 		return clk_set_parent(cpufreq->cpu_clk, cpufreq->pll_p_clk);
 
+	if (index == 3)
+		return clk_set_parent(cpufreq->cpu_clk, cpufreq->pll_c_clk);
+
 	ret = clk_set_rate(cpufreq->pll_x_clk, rate * 1000);
 	/* Restore to earlier frequency on error, i.e. pll_x */
 	if (ret)
@@ -174,6 +182,12 @@ static int tegra20_cpufreq_probe(struct platform_device *pdev)
 		goto put_pll_x;
 	}
 
+	cpufreq->pll_c_clk = clk_get_sys(NULL, "pll_c");
+	if (IS_ERR(cpufreq->pll_c_clk)) {
+		err = PTR_ERR(cpufreq->pll_c_clk);
+		goto put_pll_p;
+	}
+
 	cpufreq->dev = &pdev->dev;
 	cpufreq->driver.get = cpufreq_generic_get;
 	cpufreq->driver.attr = cpufreq_generic_attr;
@@ -190,12 +204,14 @@ static int tegra20_cpufreq_probe(struct platform_device *pdev)
 
 	err = cpufreq_register_driver(&cpufreq->driver);
 	if (err)
-		goto put_pll_p;
+		goto put_pll_c;
 
 	platform_set_drvdata(pdev, cpufreq);
 
 	return 0;
 
+put_pll_c:
+	clk_put(cpufreq->pll_c_clk);
 put_pll_p:
 	clk_put(cpufreq->pll_p_clk);
 put_pll_x:
@@ -212,6 +228,7 @@ static int tegra20_cpufreq_remove(struct platform_device *pdev)
 
 	cpufreq_unregister_driver(&cpufreq->driver);
 
+	clk_put(cpufreq->pll_c_clk);
 	clk_put(cpufreq->pll_p_clk);
 	clk_put(cpufreq->pll_x_clk);
 	clk_put(cpufreq->cpu_clk);
-- 
2.17.0

Re: [PATCH v1 1/2] cpufreq: tegra20: Constify rate value of the intermediate clk

[Viresh Kumar]

On 23-05-18, 19:00, Dmitry Osipenko wrote:
> PLL_P is known to be always running at 216MHz, hence there is no need to
> query its rate.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/cpufreq/tegra20-cpufreq.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c
> index 05f57dcd5215..3ad6bded6efc 100644
> --- a/drivers/cpufreq/tegra20-cpufreq.c
> +++ b/drivers/cpufreq/tegra20-cpufreq.c
> @@ -24,6 +24,8 @@
>  #include <linux/platform_device.h>
>  #include <linux/types.h>
>  
> +#define PLL_P_FREQ	216000
> +
>  static struct cpufreq_frequency_table freq_table[] = {
>  	{ .frequency = 216000 },
>  	{ .frequency = 312000 },
> @@ -48,18 +50,15 @@ struct tegra20_cpufreq {
>  static unsigned int tegra_get_intermediate(struct cpufreq_policy *policy,
>  					   unsigned int index)
>  {
> -	struct tegra20_cpufreq *cpufreq = cpufreq_get_driver_data();
> -	unsigned int ifreq = clk_get_rate(cpufreq->pll_p_clk) / 1000;
> -
>  	/*
>  	 * Don't switch to intermediate freq if:
>  	 * - we are already at it, i.e. policy->cur == ifreq
>  	 * - index corresponds to ifreq
>  	 */
> -	if (freq_table[index].frequency == ifreq || policy->cur == ifreq)
> +	if (index == 0 || policy->cur == PLL_P_FREQ)
>  		return 0;
>  
> -	return ifreq;
> +	return PLL_P_FREQ;
>  }
>  
>  static int tegra_target_intermediate(struct cpufreq_policy *policy,
> @@ -93,14 +92,13 @@ static int tegra_target(struct cpufreq_policy *policy, unsigned int index)
>  {
>  	struct tegra20_cpufreq *cpufreq = cpufreq_get_driver_data();
>  	unsigned long rate = freq_table[index].frequency;
> -	unsigned int ifreq = clk_get_rate(cpufreq->pll_p_clk) / 1000;
>  	int ret;
>  
>  	/*
>  	 * target freq == pll_p, don't need to take extra reference to pll_x_clk
>  	 * as it isn't used anymore.
>  	 */
> -	if (rate == ifreq)
> +	if (index == 0)
>  		return clk_set_parent(cpufreq->cpu_clk, cpufreq->pll_p_clk);
>  
>  	ret = clk_set_rate(cpufreq->pll_x_clk, rate * 1000);

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

Re: [PATCH v1 2/2] cpufreq: tegra20: Use PLL_C as intermediate clock source

[Viresh Kumar]

On 23-05-18, 19:00, Dmitry Osipenko wrote:
> PLL_C is running at 600MHz which is significantly higher than the 216MHz
> of the PLL_P and it is known that PLL_C is always-ON because AHB BUS is
> running on that PLL. Let's use PLL_C as intermediate clock source, making
> CPU snappier a tad during of the frequency transition.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/cpufreq/tegra20-cpufreq.c | 25 +++++++++++++++++++++----
>  1 file changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c
> index 3ad6bded6efc..4bf5ba7da40b 100644
> --- a/drivers/cpufreq/tegra20-cpufreq.c
> +++ b/drivers/cpufreq/tegra20-cpufreq.c
> @@ -25,12 +25,13 @@
>  #include <linux/types.h>
>  
>  #define PLL_P_FREQ	216000
> +#define PLL_C_FREQ	600000
>  
>  static struct cpufreq_frequency_table freq_table[] = {
>  	{ .frequency = 216000 },
>  	{ .frequency = 312000 },
>  	{ .frequency = 456000 },
> -	{ .frequency = 608000 },
> +	{ .frequency = 600000 },
>  	{ .frequency = 760000 },
>  	{ .frequency = 816000 },
>  	{ .frequency = 912000 },
> @@ -44,6 +45,7 @@ struct tegra20_cpufreq {
>  	struct clk *cpu_clk;
>  	struct clk *pll_x_clk;
>  	struct clk *pll_p_clk;
> +	struct clk *pll_c_clk;
>  	bool pll_x_prepared;
>  };
>  
> @@ -58,7 +60,10 @@ static unsigned int tegra_get_intermediate(struct cpufreq_policy *policy,
>  	if (index == 0 || policy->cur == PLL_P_FREQ)
>  		return 0;
>  
> -	return PLL_P_FREQ;
> +	if (index == 3 || policy->cur == PLL_C_FREQ)
> +		return 0;

So we can choose between two different intermediate frequencies ? And
I didn't like the way magic number 3 is used here. Its prone to errors
and we better use a macro or something else here.

Like instead of doing index == 3, what about freq_table[index].freq ==
PLL_C_FREQ ? Same for the previous patch as well.

-- 
viresh

Re: [PATCH v1 2/2] cpufreq: tegra20: Use PLL_C as intermediate clock source

[Dmitry Osipenko]

On 24.05.2018 07:30, Viresh Kumar wrote:
> On 23-05-18, 19:00, Dmitry Osipenko wrote:
>> PLL_C is running at 600MHz which is significantly higher than the 216MHz
>> of the PLL_P and it is known that PLL_C is always-ON because AHB BUS is
>> running on that PLL. Let's use PLL_C as intermediate clock source, making
>> CPU snappier a tad during of the frequency transition.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/cpufreq/tegra20-cpufreq.c | 25 +++++++++++++++++++++----
>>  1 file changed, 21 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c
>> index 3ad6bded6efc..4bf5ba7da40b 100644
>> --- a/drivers/cpufreq/tegra20-cpufreq.c
>> +++ b/drivers/cpufreq/tegra20-cpufreq.c
>> @@ -25,12 +25,13 @@
>>  #include <linux/types.h>
>>  
>>  #define PLL_P_FREQ	216000
>> +#define PLL_C_FREQ	600000
>>  
>>  static struct cpufreq_frequency_table freq_table[] = {
>>  	{ .frequency = 216000 },
>>  	{ .frequency = 312000 },
>>  	{ .frequency = 456000 },
>> -	{ .frequency = 608000 },
>> +	{ .frequency = 600000 },
>>  	{ .frequency = 760000 },
>>  	{ .frequency = 816000 },
>>  	{ .frequency = 912000 },
>> @@ -44,6 +45,7 @@ struct tegra20_cpufreq {
>>  	struct clk *cpu_clk;
>>  	struct clk *pll_x_clk;
>>  	struct clk *pll_p_clk;
>> +	struct clk *pll_c_clk;
>>  	bool pll_x_prepared;
>>  };
>>  
>> @@ -58,7 +60,10 @@ static unsigned int tegra_get_intermediate(struct cpufreq_policy *policy,
>>  	if (index == 0 || policy->cur == PLL_P_FREQ)
>>  		return 0;
>>  
>> -	return PLL_P_FREQ;
>> +	if (index == 3 || policy->cur == PLL_C_FREQ)
>> +		return 0;
> 
> So we can choose between two different intermediate frequencies ? And
> I didn't like the way magic number 3 is used here. Its prone to errors
> and we better use a macro or something else here.
> 
> Like instead of doing index == 3, what about freq_table[index].freq ==
> PLL_C_FREQ ? Same for the previous patch as well.

The frequency is determined by the parent clock of CCLK (CPU clock), we can
choose between different parents for the CCLK. PLL_C as PLL_P and PLL_X are
among the available parents for the CCLK to choose from and there some others.

I don't mind to use freq_table[index].freq, though I'd like to keep compiled
assembly minimal where possible. Hence the freq_table should be made constant to
tell compiler that it doesn't need to emit data fetches for the table values and
could embed the constants into the code where appropriate.

Could we constify the "struct cpufreq_frequency_table" within the cpufreq core?
Seems nothing prevents this (I already tried to constify - there are no
obstacles), unless some cpufreq driver would try to modify
policy->freq_table->... within the cpufreq callback implementation.

Re: [PATCH v1 2/2] cpufreq: tegra20: Use PLL_C as intermediate clock source

[Rafael J. Wysocki]

On Thu, May 24, 2018 at 7:37 AM, Dmitry Osipenko <digetx@gmail.com> wrote:
> On 24.05.2018 07:30, Viresh Kumar wrote:
>> On 23-05-18, 19:00, Dmitry Osipenko wrote:
>>> PLL_C is running at 600MHz which is significantly higher than the 216MHz
>>> of the PLL_P and it is known that PLL_C is always-ON because AHB BUS is
>>> running on that PLL. Let's use PLL_C as intermediate clock source, making
>>> CPU snappier a tad during of the frequency transition.
>>>
>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>> ---
>>>  drivers/cpufreq/tegra20-cpufreq.c | 25 +++++++++++++++++++++----
>>>  1 file changed, 21 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c
>>> index 3ad6bded6efc..4bf5ba7da40b 100644
>>> --- a/drivers/cpufreq/tegra20-cpufreq.c
>>> +++ b/drivers/cpufreq/tegra20-cpufreq.c
>>> @@ -25,12 +25,13 @@
>>>  #include <linux/types.h>
>>>
>>>  #define PLL_P_FREQ  216000
>>> +#define PLL_C_FREQ  600000
>>>
>>>  static struct cpufreq_frequency_table freq_table[] = {
>>>      { .frequency = 216000 },
>>>      { .frequency = 312000 },
>>>      { .frequency = 456000 },
>>> -    { .frequency = 608000 },
>>> +    { .frequency = 600000 },
>>>      { .frequency = 760000 },
>>>      { .frequency = 816000 },
>>>      { .frequency = 912000 },
>>> @@ -44,6 +45,7 @@ struct tegra20_cpufreq {
>>>      struct clk *cpu_clk;
>>>      struct clk *pll_x_clk;
>>>      struct clk *pll_p_clk;
>>> +    struct clk *pll_c_clk;
>>>      bool pll_x_prepared;
>>>  };
>>>
>>> @@ -58,7 +60,10 @@ static unsigned int tegra_get_intermediate(struct cpufreq_policy *policy,
>>>      if (index == 0 || policy->cur == PLL_P_FREQ)
>>>              return 0;
>>>
>>> -    return PLL_P_FREQ;
>>> +    if (index == 3 || policy->cur == PLL_C_FREQ)
>>> +            return 0;
>>
>> So we can choose between two different intermediate frequencies ? And
>> I didn't like the way magic number 3 is used here. Its prone to errors
>> and we better use a macro or something else here.
>>
>> Like instead of doing index == 3, what about freq_table[index].freq ==
>> PLL_C_FREQ ? Same for the previous patch as well.
>
> The frequency is determined by the parent clock of CCLK (CPU clock), we can
> choose between different parents for the CCLK. PLL_C as PLL_P and PLL_X are
> among the available parents for the CCLK to choose from and there some others.
>
> I don't mind to use freq_table[index].freq, though I'd like to keep compiled
> assembly minimal where possible. Hence the freq_table should be made constant to
> tell compiler that it doesn't need to emit data fetches for the table values and
> could embed the constants into the code where appropriate.
>
> Could we constify the "struct cpufreq_frequency_table" within the cpufreq core?
> Seems nothing prevents this (I already tried to constify - there are no
> obstacles), unless some cpufreq driver would try to modify
> policy->freq_table->... within the cpufreq callback implementation.

Some drivers generate frequency tables out of external data
unavailable at compile time, like ACPI tables.

But if you know it for the fact that the core doesn't modify the
frequency table, you could pass a constant table from the driver to
it, can't you?

Re: [PATCH v1 2/2] cpufreq: tegra20: Use PLL_C as intermediate clock source

[Peter De Schrijver]

On Wed, May 23, 2018 at 07:00:20PM +0300, Dmitry Osipenko wrote:
> PLL_C is running at 600MHz which is significantly higher than the 216MHz
> of the PLL_P and it is known that PLL_C is always-ON because AHB BUS is
> running on that PLL. Let's use PLL_C as intermediate clock source, making
> CPU snappier a tad during of the frequency transition.
> 

pll_c isn't necessarily 600Mhz when used as a source for the second display
head.

Peter.

> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/cpufreq/tegra20-cpufreq.c | 25 +++++++++++++++++++++----
>  1 file changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c
> index 3ad6bded6efc..4bf5ba7da40b 100644
> --- a/drivers/cpufreq/tegra20-cpufreq.c
> +++ b/drivers/cpufreq/tegra20-cpufreq.c
> @@ -25,12 +25,13 @@
>  #include <linux/types.h>
>  
>  #define PLL_P_FREQ	216000
> +#define PLL_C_FREQ	600000
>  
>  static struct cpufreq_frequency_table freq_table[] = {
>  	{ .frequency = 216000 },
>  	{ .frequency = 312000 },
>  	{ .frequency = 456000 },
> -	{ .frequency = 608000 },
> +	{ .frequency = 600000 },
>  	{ .frequency = 760000 },
>  	{ .frequency = 816000 },
>  	{ .frequency = 912000 },
> @@ -44,6 +45,7 @@ struct tegra20_cpufreq {
>  	struct clk *cpu_clk;
>  	struct clk *pll_x_clk;
>  	struct clk *pll_p_clk;
> +	struct clk *pll_c_clk;
>  	bool pll_x_prepared;
>  };
>  
> @@ -58,7 +60,10 @@ static unsigned int tegra_get_intermediate(struct cpufreq_policy *policy,
>  	if (index == 0 || policy->cur == PLL_P_FREQ)
>  		return 0;
>  
> -	return PLL_P_FREQ;
> +	if (index == 3 || policy->cur == PLL_C_FREQ)
> +		return 0;
> +
> +	return PLL_C_FREQ;
>  }
>  
>  static int tegra_target_intermediate(struct cpufreq_policy *policy,
> @@ -79,7 +84,7 @@ static int tegra_target_intermediate(struct cpufreq_policy *policy,
>  	 */
>  	clk_prepare_enable(cpufreq->pll_x_clk);
>  
> -	ret = clk_set_parent(cpufreq->cpu_clk, cpufreq->pll_p_clk);
> +	ret = clk_set_parent(cpufreq->cpu_clk, cpufreq->pll_c_clk);
>  	if (ret)
>  		clk_disable_unprepare(cpufreq->pll_x_clk);
>  	else
> @@ -101,6 +106,9 @@ static int tegra_target(struct cpufreq_policy *policy, unsigned int index)
>  	if (index == 0)
>  		return clk_set_parent(cpufreq->cpu_clk, cpufreq->pll_p_clk);
>  
> +	if (index == 3)
> +		return clk_set_parent(cpufreq->cpu_clk, cpufreq->pll_c_clk);
> +
>  	ret = clk_set_rate(cpufreq->pll_x_clk, rate * 1000);
>  	/* Restore to earlier frequency on error, i.e. pll_x */
>  	if (ret)
> @@ -174,6 +182,12 @@ static int tegra20_cpufreq_probe(struct platform_device *pdev)
>  		goto put_pll_x;
>  	}
>  
> +	cpufreq->pll_c_clk = clk_get_sys(NULL, "pll_c");
> +	if (IS_ERR(cpufreq->pll_c_clk)) {
> +		err = PTR_ERR(cpufreq->pll_c_clk);
> +		goto put_pll_p;
> +	}
> +
>  	cpufreq->dev = &pdev->dev;
>  	cpufreq->driver.get = cpufreq_generic_get;
>  	cpufreq->driver.attr = cpufreq_generic_attr;
> @@ -190,12 +204,14 @@ static int tegra20_cpufreq_probe(struct platform_device *pdev)
>  
>  	err = cpufreq_register_driver(&cpufreq->driver);
>  	if (err)
> -		goto put_pll_p;
> +		goto put_pll_c;
>  
>  	platform_set_drvdata(pdev, cpufreq);
>  
>  	return 0;
>  
> +put_pll_c:
> +	clk_put(cpufreq->pll_c_clk);
>  put_pll_p:
>  	clk_put(cpufreq->pll_p_clk);
>  put_pll_x:
> @@ -212,6 +228,7 @@ static int tegra20_cpufreq_remove(struct platform_device *pdev)
>  
>  	cpufreq_unregister_driver(&cpufreq->driver);
>  
> +	clk_put(cpufreq->pll_c_clk);
>  	clk_put(cpufreq->pll_p_clk);
>  	clk_put(cpufreq->pll_x_clk);
>  	clk_put(cpufreq->cpu_clk);
> -- 
> 2.17.0
> 

Re: [PATCH v1 2/2] cpufreq: tegra20: Use PLL_C as intermediate clock source

[Peter De Schrijver]

On Wed, May 23, 2018 at 07:00:20PM +0300, Dmitry Osipenko wrote:
> PLL_C is running at 600MHz which is significantly higher than the 216MHz
> of the PLL_P and it is known that PLL_C is always-ON because AHB BUS is
> running on that PLL. Let's use PLL_C as intermediate clock source, making
> CPU snappier a tad during of the frequency transition.
> 

pll_c isn't necessarily 600Mhz when used as a source for the second display
head.

Peter.

> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/cpufreq/tegra20-cpufreq.c | 25 +++++++++++++++++++++----
>  1 file changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c
> index 3ad6bded6efc..4bf5ba7da40b 100644
> --- a/drivers/cpufreq/tegra20-cpufreq.c
> +++ b/drivers/cpufreq/tegra20-cpufreq.c
> @@ -25,12 +25,13 @@
>  #include <linux/types.h>
>  
>  #define PLL_P_FREQ	216000
> +#define PLL_C_FREQ	600000
>  
>  static struct cpufreq_frequency_table freq_table[] = {
>  	{ .frequency = 216000 },
>  	{ .frequency = 312000 },
>  	{ .frequency = 456000 },
> -	{ .frequency = 608000 },
> +	{ .frequency = 600000 },
>  	{ .frequency = 760000 },
>  	{ .frequency = 816000 },
>  	{ .frequency = 912000 },
> @@ -44,6 +45,7 @@ struct tegra20_cpufreq {
>  	struct clk *cpu_clk;
>  	struct clk *pll_x_clk;
>  	struct clk *pll_p_clk;
> +	struct clk *pll_c_clk;
>  	bool pll_x_prepared;
>  };
>  
> @@ -58,7 +60,10 @@ static unsigned int tegra_get_intermediate(struct cpufreq_policy *policy,
>  	if (index == 0 || policy->cur == PLL_P_FREQ)
>  		return 0;
>  
> -	return PLL_P_FREQ;
> +	if (index == 3 || policy->cur == PLL_C_FREQ)
> +		return 0;
> +
> +	return PLL_C_FREQ;
>  }
>  
>  static int tegra_target_intermediate(struct cpufreq_policy *policy,
> @@ -79,7 +84,7 @@ static int tegra_target_intermediate(struct cpufreq_policy *policy,
>  	 */
>  	clk_prepare_enable(cpufreq->pll_x_clk);
>  
> -	ret = clk_set_parent(cpufreq->cpu_clk, cpufreq->pll_p_clk);
> +	ret = clk_set_parent(cpufreq->cpu_clk, cpufreq->pll_c_clk);
>  	if (ret)
>  		clk_disable_unprepare(cpufreq->pll_x_clk);
>  	else
> @@ -101,6 +106,9 @@ static int tegra_target(struct cpufreq_policy *policy, unsigned int index)
>  	if (index == 0)
>  		return clk_set_parent(cpufreq->cpu_clk, cpufreq->pll_p_clk);
>  
> +	if (index == 3)
> +		return clk_set_parent(cpufreq->cpu_clk, cpufreq->pll_c_clk);
> +
>  	ret = clk_set_rate(cpufreq->pll_x_clk, rate * 1000);
>  	/* Restore to earlier frequency on error, i.e. pll_x */
>  	if (ret)
> @@ -174,6 +182,12 @@ static int tegra20_cpufreq_probe(struct platform_device *pdev)
>  		goto put_pll_x;
>  	}
>  
> +	cpufreq->pll_c_clk = clk_get_sys(NULL, "pll_c");
> +	if (IS_ERR(cpufreq->pll_c_clk)) {
> +		err = PTR_ERR(cpufreq->pll_c_clk);
> +		goto put_pll_p;
> +	}
> +
>  	cpufreq->dev = &pdev->dev;
>  	cpufreq->driver.get = cpufreq_generic_get;
>  	cpufreq->driver.attr = cpufreq_generic_attr;
> @@ -190,12 +204,14 @@ static int tegra20_cpufreq_probe(struct platform_device *pdev)
>  
>  	err = cpufreq_register_driver(&cpufreq->driver);
>  	if (err)
> -		goto put_pll_p;
> +		goto put_pll_c;
>  
>  	platform_set_drvdata(pdev, cpufreq);
>  
>  	return 0;
>  
> +put_pll_c:
> +	clk_put(cpufreq->pll_c_clk);
>  put_pll_p:
>  	clk_put(cpufreq->pll_p_clk);
>  put_pll_x:
> @@ -212,6 +228,7 @@ static int tegra20_cpufreq_remove(struct platform_device *pdev)
>  
>  	cpufreq_unregister_driver(&cpufreq->driver);
>  
> +	clk_put(cpufreq->pll_c_clk);
>  	clk_put(cpufreq->pll_p_clk);
>  	clk_put(cpufreq->pll_x_clk);
>  	clk_put(cpufreq->cpu_clk);
> -- 
> 2.17.0
> 

Re: [PATCH v1 2/2] cpufreq: tegra20: Use PLL_C as intermediate clock source

[Dmitry Osipenko]

On 24.05.2018 11:01, Rafael J. Wysocki wrote:
> On Thu, May 24, 2018 at 7:37 AM, Dmitry Osipenko <digetx@gmail.com> wrote:
>> On 24.05.2018 07:30, Viresh Kumar wrote:
>>> On 23-05-18, 19:00, Dmitry Osipenko wrote:
>>>> PLL_C is running at 600MHz which is significantly higher than the 216MHz
>>>> of the PLL_P and it is known that PLL_C is always-ON because AHB BUS is
>>>> running on that PLL. Let's use PLL_C as intermediate clock source, making
>>>> CPU snappier a tad during of the frequency transition.
>>>>
>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>> ---
>>>>  drivers/cpufreq/tegra20-cpufreq.c | 25 +++++++++++++++++++++----
>>>>  1 file changed, 21 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c
>>>> index 3ad6bded6efc..4bf5ba7da40b 100644
>>>> --- a/drivers/cpufreq/tegra20-cpufreq.c
>>>> +++ b/drivers/cpufreq/tegra20-cpufreq.c
>>>> @@ -25,12 +25,13 @@
>>>>  #include <linux/types.h>
>>>>
>>>>  #define PLL_P_FREQ  216000
>>>> +#define PLL_C_FREQ  600000
>>>>
>>>>  static struct cpufreq_frequency_table freq_table[] = {
>>>>      { .frequency = 216000 },
>>>>      { .frequency = 312000 },
>>>>      { .frequency = 456000 },
>>>> -    { .frequency = 608000 },
>>>> +    { .frequency = 600000 },
>>>>      { .frequency = 760000 },
>>>>      { .frequency = 816000 },
>>>>      { .frequency = 912000 },
>>>> @@ -44,6 +45,7 @@ struct tegra20_cpufreq {
>>>>      struct clk *cpu_clk;
>>>>      struct clk *pll_x_clk;
>>>>      struct clk *pll_p_clk;
>>>> +    struct clk *pll_c_clk;
>>>>      bool pll_x_prepared;
>>>>  };
>>>>
>>>> @@ -58,7 +60,10 @@ static unsigned int tegra_get_intermediate(struct cpufreq_policy *policy,
>>>>      if (index == 0 || policy->cur == PLL_P_FREQ)
>>>>              return 0;
>>>>
>>>> -    return PLL_P_FREQ;
>>>> +    if (index == 3 || policy->cur == PLL_C_FREQ)
>>>> +            return 0;
>>>
>>> So we can choose between two different intermediate frequencies ? And
>>> I didn't like the way magic number 3 is used here. Its prone to errors
>>> and we better use a macro or something else here.
>>>
>>> Like instead of doing index == 3, what about freq_table[index].freq ==
>>> PLL_C_FREQ ? Same for the previous patch as well.
>>
>> The frequency is determined by the parent clock of CCLK (CPU clock), we can
>> choose between different parents for the CCLK. PLL_C as PLL_P and PLL_X are
>> among the available parents for the CCLK to choose from and there some others.
>>
>> I don't mind to use freq_table[index].freq, though I'd like to keep compiled
>> assembly minimal where possible. Hence the freq_table should be made constant to
>> tell compiler that it doesn't need to emit data fetches for the table values and
>> could embed the constants into the code where appropriate.
>>
>> Could we constify the "struct cpufreq_frequency_table" within the cpufreq core?
>> Seems nothing prevents this (I already tried to constify - there are no
>> obstacles), unless some cpufreq driver would try to modify
>> policy->freq_table->... within the cpufreq callback implementation.
> 
> Some drivers generate frequency tables out of external data
> unavailable at compile time, like ACPI tables.

Instead of making the table constant itself (with its values), seems we can just
make the policy->freq_table pointer constant. I'll try to make a patch for that,
adjusting the pointers in cpufreq core and the drivers. This works for the
acpi-cpufreq at least.

> But if you know it for the fact that the core doesn't modify the
> frequency table, you could pass a constant table from the driver to
> it, can't you?
> 

Yes, but that will require to explicitly silencing the compiler warning about
const -> non-const pointer conversion (if you're meaning this pointer
conversion), which generally should be avoided.

Re: [PATCH v1 2/2] cpufreq: tegra20: Use PLL_C as intermediate clock source

[Dmitry Osipenko]

On 24.05.2018 13:04, Peter De Schrijver wrote:
> On Wed, May 23, 2018 at 07:00:20PM +0300, Dmitry Osipenko wrote:
>> PLL_C is running at 600MHz which is significantly higher than the 216MHz
>> of the PLL_P and it is known that PLL_C is always-ON because AHB BUS is
>> running on that PLL. Let's use PLL_C as intermediate clock source, making
>> CPU snappier a tad during of the frequency transition.
>>
> 
> pll_c isn't necessarily 600Mhz when used as a source for the second display
> head.

Hmm, indeed.

Even if PLL_C rate will be adjusted, it will be higher than the PLL_P.. won't
it? That's likely to be good enough.

Do you know if any of the available CCLK parents has a glitch-less rate
switching? I.e. CPU won't hang on the rate switch.

There is other possible 600MHz source, the PLL_M. Can we use it? This one also
may become dynamic if we'll consider implementing the memory scaling, but the
memory frequency probably will fit the transition role pretty well.

Re: [PATCH v1 2/2] cpufreq: tegra20: Use PLL_C as intermediate clock source

[Peter De Schrijver]

On Thu, May 24, 2018 at 03:49:22PM +0300, Dmitry Osipenko wrote:
> On 24.05.2018 13:04, Peter De Schrijver wrote:
> > On Wed, May 23, 2018 at 07:00:20PM +0300, Dmitry Osipenko wrote:
> >> PLL_C is running at 600MHz which is significantly higher than the 216MHz
> >> of the PLL_P and it is known that PLL_C is always-ON because AHB BUS is
> >> running on that PLL. Let's use PLL_C as intermediate clock source, making
> >> CPU snappier a tad during of the frequency transition.
> >>
> > 
> > pll_c isn't necessarily 600Mhz when used as a source for the second display
> > head.
> 
> Hmm, indeed.
> 
> Even if PLL_C rate will be adjusted, it will be higher than the PLL_P.. won't
> it? That's likely to be good enough.
> 

Yes. I think it can be always higher than pll_p, but that assumes the display
driver will always program the highest possible rate for pll_c for a given mode
and then program the display divider to divide it down to the required rate.

> Do you know if any of the available CCLK parents has a glitch-less rate
> switching? I.e. CPU won't hang on the rate switch.
> 

Tegra20 doesn't have dynamic ramp PLLs no. So you always have to switch to a
backup clock source before changing the rate of pll_x.

> There is other possible 600MHz source, the PLL_M. Can we use it? This one also
> may become dynamic if we'll consider implementing the memory scaling, but the
> memory frequency probably will fit the transition role pretty well.

I think this should work, but as you mention it may very well be lower than
pll_p if Tegra20 EMC scaling is re-introduced. I think that's why historically
this was never done. 

Peter.

Re: [PATCH v1 2/2] cpufreq: tegra20: Use PLL_C as intermediate clock source

[Peter De Schrijver]

On Thu, May 24, 2018 at 03:49:22PM +0300, Dmitry Osipenko wrote:
> On 24.05.2018 13:04, Peter De Schrijver wrote:
> > On Wed, May 23, 2018 at 07:00:20PM +0300, Dmitry Osipenko wrote:
> >> PLL_C is running at 600MHz which is significantly higher than the 216MHz
> >> of the PLL_P and it is known that PLL_C is always-ON because AHB BUS is
> >> running on that PLL. Let's use PLL_C as intermediate clock source, making
> >> CPU snappier a tad during of the frequency transition.
> >>
> > 
> > pll_c isn't necessarily 600Mhz when used as a source for the second display
> > head.
> 
> Hmm, indeed.
> 
> Even if PLL_C rate will be adjusted, it will be higher than the PLL_P.. won't
> it? That's likely to be good enough.
> 

Yes. I think it can be always higher than pll_p, but that assumes the display
driver will always program the highest possible rate for pll_c for a given mode
and then program the display divider to divide it down to the required rate.

> Do you know if any of the available CCLK parents has a glitch-less rate
> switching? I.e. CPU won't hang on the rate switch.
> 

Tegra20 doesn't have dynamic ramp PLLs no. So you always have to switch to a
backup clock source before changing the rate of pll_x.

> There is other possible 600MHz source, the PLL_M. Can we use it? This one also
> may become dynamic if we'll consider implementing the memory scaling, but the
> memory frequency probably will fit the transition role pretty well.

I think this should work, but as you mention it may very well be lower than
pll_p if Tegra20 EMC scaling is re-introduced. I think that's why historically
this was never done. 

Peter.

Re: [PATCH v1 2/2] cpufreq: tegra20: Use PLL_C as intermediate clock source

[Rafael J. Wysocki]

On Thu, May 24, 2018 at 2:28 PM, Dmitry Osipenko <digetx@gmail.com> wrote:
> On 24.05.2018 11:01, Rafael J. Wysocki wrote:
>> On Thu, May 24, 2018 at 7:37 AM, Dmitry Osipenko <digetx@gmail.com> wrote:
>>> On 24.05.2018 07:30, Viresh Kumar wrote:
>>>> On 23-05-18, 19:00, Dmitry Osipenko wrote:
>>>>> PLL_C is running at 600MHz which is significantly higher than the 216MHz
>>>>> of the PLL_P and it is known that PLL_C is always-ON because AHB BUS is
>>>>> running on that PLL. Let's use PLL_C as intermediate clock source, making
>>>>> CPU snappier a tad during of the frequency transition.
>>>>>
>>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>>> ---
>>>>>  drivers/cpufreq/tegra20-cpufreq.c | 25 +++++++++++++++++++++----
>>>>>  1 file changed, 21 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c
>>>>> index 3ad6bded6efc..4bf5ba7da40b 100644
>>>>> --- a/drivers/cpufreq/tegra20-cpufreq.c
>>>>> +++ b/drivers/cpufreq/tegra20-cpufreq.c
>>>>> @@ -25,12 +25,13 @@
>>>>>  #include <linux/types.h>
>>>>>
>>>>>  #define PLL_P_FREQ  216000
>>>>> +#define PLL_C_FREQ  600000
>>>>>
>>>>>  static struct cpufreq_frequency_table freq_table[] = {
>>>>>      { .frequency = 216000 },
>>>>>      { .frequency = 312000 },
>>>>>      { .frequency = 456000 },
>>>>> -    { .frequency = 608000 },
>>>>> +    { .frequency = 600000 },
>>>>>      { .frequency = 760000 },
>>>>>      { .frequency = 816000 },
>>>>>      { .frequency = 912000 },
>>>>> @@ -44,6 +45,7 @@ struct tegra20_cpufreq {
>>>>>      struct clk *cpu_clk;
>>>>>      struct clk *pll_x_clk;
>>>>>      struct clk *pll_p_clk;
>>>>> +    struct clk *pll_c_clk;
>>>>>      bool pll_x_prepared;
>>>>>  };
>>>>>
>>>>> @@ -58,7 +60,10 @@ static unsigned int tegra_get_intermediate(struct cpufreq_policy *policy,
>>>>>      if (index == 0 || policy->cur == PLL_P_FREQ)
>>>>>              return 0;
>>>>>
>>>>> -    return PLL_P_FREQ;
>>>>> +    if (index == 3 || policy->cur == PLL_C_FREQ)
>>>>> +            return 0;
>>>>
>>>> So we can choose between two different intermediate frequencies ? And
>>>> I didn't like the way magic number 3 is used here. Its prone to errors
>>>> and we better use a macro or something else here.
>>>>
>>>> Like instead of doing index == 3, what about freq_table[index].freq ==
>>>> PLL_C_FREQ ? Same for the previous patch as well.
>>>
>>> The frequency is determined by the parent clock of CCLK (CPU clock), we can
>>> choose between different parents for the CCLK. PLL_C as PLL_P and PLL_X are
>>> among the available parents for the CCLK to choose from and there some others.
>>>
>>> I don't mind to use freq_table[index].freq, though I'd like to keep compiled
>>> assembly minimal where possible. Hence the freq_table should be made constant to
>>> tell compiler that it doesn't need to emit data fetches for the table values and
>>> could embed the constants into the code where appropriate.
>>>
>>> Could we constify the "struct cpufreq_frequency_table" within the cpufreq core?
>>> Seems nothing prevents this (I already tried to constify - there are no
>>> obstacles), unless some cpufreq driver would try to modify
>>> policy->freq_table->... within the cpufreq callback implementation.
>>
>> Some drivers generate frequency tables out of external data
>> unavailable at compile time, like ACPI tables.
>
> Instead of making the table constant itself (with its values), seems we can just
> make the policy->freq_table pointer constant. I'll try to make a patch for that,
> adjusting the pointers in cpufreq core and the drivers. This works for the
> acpi-cpufreq at least.

Honestly, messing up with the whole subsystem in order to avoid an
explicit pointer case doesn't sound right to me.

>
>> But if you know it for the fact that the core doesn't modify the
>> frequency table, you could pass a constant table from the driver to
>> it, can't you?
>>
>
> Yes, but that will require to explicitly silencing the compiler warning about
> const -> non-const pointer conversion (if you're meaning this pointer
> conversion), which generally should be avoided.

Why?

Re: [PATCH v1 2/2] cpufreq: tegra20: Use PLL_C as intermediate clock source

[Dmitry Osipenko]

On 25.05.2018 09:32, Peter De Schrijver wrote:
> On Thu, May 24, 2018 at 03:49:22PM +0300, Dmitry Osipenko wrote:
>> On 24.05.2018 13:04, Peter De Schrijver wrote:
>>> On Wed, May 23, 2018 at 07:00:20PM +0300, Dmitry Osipenko wrote:
>>>> PLL_C is running at 600MHz which is significantly higher than the 216MHz
>>>> of the PLL_P and it is known that PLL_C is always-ON because AHB BUS is
>>>> running on that PLL. Let's use PLL_C as intermediate clock source, making
>>>> CPU snappier a tad during of the frequency transition.
>>>>
>>>
>>> pll_c isn't necessarily 600Mhz when used as a source for the second display
>>> head.
>>
>> Hmm, indeed.
>>
>> Even if PLL_C rate will be adjusted, it will be higher than the PLL_P.. won't
>> it? That's likely to be good enough.
>>
> 
> Yes. I think it can be always higher than pll_p, but that assumes the display
> driver will always program the highest possible rate for pll_c for a given mode
> and then program the display divider to divide it down to the required rate.
> 
>> Do you know if any of the available CCLK parents has a glitch-less rate
>> switching? I.e. CPU won't hang on the rate switch.
>>
> 
> Tegra20 doesn't have dynamic ramp PLLs no. So you always have to switch to a
> backup clock source before changing the rate of pll_x.
> 
>> There is other possible 600MHz source, the PLL_M. Can we use it? This one also
>> may become dynamic if we'll consider implementing the memory scaling, but the
>> memory frequency probably will fit the transition role pretty well.
> 
> I think this should work, but as you mention it may very well be lower than
> pll_p if Tegra20 EMC scaling is re-introduced. I think that's why historically
> this was never done. 
> 
> Peter.
> 

Okay, thank you very much for the input. Let's put on hold these patches then.

Peter Geis just made a cpufreq driver for the Tegra30 and turned out it's just a
dozen lines of code that we'll have to add to the tegra20-cpufreq driver to
support Tegra30. The tegra20-cpufreq driver code suits very well for the Tegra30
integration, use of PLL_C for the intermediate scaling will make that
integration a bit messy. I'll return to considering of the different
transition-PLL variant after adding the Tegra30 support.

Re: [PATCH v1 2/2] cpufreq: tegra20: Use PLL_C as intermediate clock source

[Rafael J. Wysocki]

On Fri, May 25, 2018 at 10:14 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Thu, May 24, 2018 at 2:28 PM, Dmitry Osipenko <digetx@gmail.com> wrote:
>> On 24.05.2018 11:01, Rafael J. Wysocki wrote:
>>> On Thu, May 24, 2018 at 7:37 AM, Dmitry Osipenko <digetx@gmail.com> wrote:
>>>> On 24.05.2018 07:30, Viresh Kumar wrote:
>>>>> On 23-05-18, 19:00, Dmitry Osipenko wrote:
>>>>>> PLL_C is running at 600MHz which is significantly higher than the 216MHz
>>>>>> of the PLL_P and it is known that PLL_C is always-ON because AHB BUS is
>>>>>> running on that PLL. Let's use PLL_C as intermediate clock source, making
>>>>>> CPU snappier a tad during of the frequency transition.
>>>>>>
>>>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>>>> ---
>>>>>>  drivers/cpufreq/tegra20-cpufreq.c | 25 +++++++++++++++++++++----
>>>>>>  1 file changed, 21 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c
>>>>>> index 3ad6bded6efc..4bf5ba7da40b 100644
>>>>>> --- a/drivers/cpufreq/tegra20-cpufreq.c
>>>>>> +++ b/drivers/cpufreq/tegra20-cpufreq.c
>>>>>> @@ -25,12 +25,13 @@
>>>>>>  #include <linux/types.h>
>>>>>>
>>>>>>  #define PLL_P_FREQ  216000
>>>>>> +#define PLL_C_FREQ  600000
>>>>>>
>>>>>>  static struct cpufreq_frequency_table freq_table[] = {
>>>>>>      { .frequency = 216000 },
>>>>>>      { .frequency = 312000 },
>>>>>>      { .frequency = 456000 },
>>>>>> -    { .frequency = 608000 },
>>>>>> +    { .frequency = 600000 },
>>>>>>      { .frequency = 760000 },
>>>>>>      { .frequency = 816000 },
>>>>>>      { .frequency = 912000 },
>>>>>> @@ -44,6 +45,7 @@ struct tegra20_cpufreq {
>>>>>>      struct clk *cpu_clk;
>>>>>>      struct clk *pll_x_clk;
>>>>>>      struct clk *pll_p_clk;
>>>>>> +    struct clk *pll_c_clk;
>>>>>>      bool pll_x_prepared;
>>>>>>  };
>>>>>>
>>>>>> @@ -58,7 +60,10 @@ static unsigned int tegra_get_intermediate(struct cpufreq_policy *policy,
>>>>>>      if (index == 0 || policy->cur == PLL_P_FREQ)
>>>>>>              return 0;
>>>>>>
>>>>>> -    return PLL_P_FREQ;
>>>>>> +    if (index == 3 || policy->cur == PLL_C_FREQ)
>>>>>> +            return 0;
>>>>>
>>>>> So we can choose between two different intermediate frequencies ? And
>>>>> I didn't like the way magic number 3 is used here. Its prone to errors
>>>>> and we better use a macro or something else here.
>>>>>
>>>>> Like instead of doing index == 3, what about freq_table[index].freq ==
>>>>> PLL_C_FREQ ? Same for the previous patch as well.
>>>>
>>>> The frequency is determined by the parent clock of CCLK (CPU clock), we can
>>>> choose between different parents for the CCLK. PLL_C as PLL_P and PLL_X are
>>>> among the available parents for the CCLK to choose from and there some others.
>>>>
>>>> I don't mind to use freq_table[index].freq, though I'd like to keep compiled
>>>> assembly minimal where possible. Hence the freq_table should be made constant to
>>>> tell compiler that it doesn't need to emit data fetches for the table values and
>>>> could embed the constants into the code where appropriate.
>>>>
>>>> Could we constify the "struct cpufreq_frequency_table" within the cpufreq core?
>>>> Seems nothing prevents this (I already tried to constify - there are no
>>>> obstacles), unless some cpufreq driver would try to modify
>>>> policy->freq_table->... within the cpufreq callback implementation.
>>>
>>> Some drivers generate frequency tables out of external data
>>> unavailable at compile time, like ACPI tables.
>>
>> Instead of making the table constant itself (with its values), seems we can just
>> make the policy->freq_table pointer constant. I'll try to make a patch for that,
>> adjusting the pointers in cpufreq core and the drivers. This works for the
>> acpi-cpufreq at least.
>
> Honestly, messing up with the whole subsystem in order to avoid an
> explicit pointer case doesn't sound right to me.

Actually, on a second thought I agree that it is better to do it as
you suggested: make the policy->freq_table pointer constant
everywhere.

Sorry for the noise.

Re: [PATCH v1 2/2] cpufreq: tegra20: Use PLL_C as intermediate clock source

[Dmitry Osipenko]

On 25.05.2018 11:36, Rafael J. Wysocki wrote:
> On Fri, May 25, 2018 at 10:14 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> On Thu, May 24, 2018 at 2:28 PM, Dmitry Osipenko <digetx@gmail.com> wrote:
>>> On 24.05.2018 11:01, Rafael J. Wysocki wrote:
>>>> On Thu, May 24, 2018 at 7:37 AM, Dmitry Osipenko <digetx@gmail.com> wrote:
>>>>> On 24.05.2018 07:30, Viresh Kumar wrote:
>>>>>> On 23-05-18, 19:00, Dmitry Osipenko wrote:
>>>>>>> PLL_C is running at 600MHz which is significantly higher than the 216MHz
>>>>>>> of the PLL_P and it is known that PLL_C is always-ON because AHB BUS is
>>>>>>> running on that PLL. Let's use PLL_C as intermediate clock source, making
>>>>>>> CPU snappier a tad during of the frequency transition.
>>>>>>>
>>>>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>>>>> ---
>>>>>>>  drivers/cpufreq/tegra20-cpufreq.c | 25 +++++++++++++++++++++----
>>>>>>>  1 file changed, 21 insertions(+), 4 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c
>>>>>>> index 3ad6bded6efc..4bf5ba7da40b 100644
>>>>>>> --- a/drivers/cpufreq/tegra20-cpufreq.c
>>>>>>> +++ b/drivers/cpufreq/tegra20-cpufreq.c
>>>>>>> @@ -25,12 +25,13 @@
>>>>>>>  #include <linux/types.h>
>>>>>>>
>>>>>>>  #define PLL_P_FREQ  216000
>>>>>>> +#define PLL_C_FREQ  600000
>>>>>>>
>>>>>>>  static struct cpufreq_frequency_table freq_table[] = {
>>>>>>>      { .frequency = 216000 },
>>>>>>>      { .frequency = 312000 },
>>>>>>>      { .frequency = 456000 },
>>>>>>> -    { .frequency = 608000 },
>>>>>>> +    { .frequency = 600000 },
>>>>>>>      { .frequency = 760000 },
>>>>>>>      { .frequency = 816000 },
>>>>>>>      { .frequency = 912000 },
>>>>>>> @@ -44,6 +45,7 @@ struct tegra20_cpufreq {
>>>>>>>      struct clk *cpu_clk;
>>>>>>>      struct clk *pll_x_clk;
>>>>>>>      struct clk *pll_p_clk;
>>>>>>> +    struct clk *pll_c_clk;
>>>>>>>      bool pll_x_prepared;
>>>>>>>  };
>>>>>>>
>>>>>>> @@ -58,7 +60,10 @@ static unsigned int tegra_get_intermediate(struct cpufreq_policy *policy,
>>>>>>>      if (index == 0 || policy->cur == PLL_P_FREQ)
>>>>>>>              return 0;
>>>>>>>
>>>>>>> -    return PLL_P_FREQ;
>>>>>>> +    if (index == 3 || policy->cur == PLL_C_FREQ)
>>>>>>> +            return 0;
>>>>>>
>>>>>> So we can choose between two different intermediate frequencies ? And
>>>>>> I didn't like the way magic number 3 is used here. Its prone to errors
>>>>>> and we better use a macro or something else here.
>>>>>>
>>>>>> Like instead of doing index == 3, what about freq_table[index].freq ==
>>>>>> PLL_C_FREQ ? Same for the previous patch as well.
>>>>>
>>>>> The frequency is determined by the parent clock of CCLK (CPU clock), we can
>>>>> choose between different parents for the CCLK. PLL_C as PLL_P and PLL_X are
>>>>> among the available parents for the CCLK to choose from and there some others.
>>>>>
>>>>> I don't mind to use freq_table[index].freq, though I'd like to keep compiled
>>>>> assembly minimal where possible. Hence the freq_table should be made constant to
>>>>> tell compiler that it doesn't need to emit data fetches for the table values and
>>>>> could embed the constants into the code where appropriate.
>>>>>
>>>>> Could we constify the "struct cpufreq_frequency_table" within the cpufreq core?
>>>>> Seems nothing prevents this (I already tried to constify - there are no
>>>>> obstacles), unless some cpufreq driver would try to modify
>>>>> policy->freq_table->... within the cpufreq callback implementation.
>>>>
>>>> Some drivers generate frequency tables out of external data
>>>> unavailable at compile time, like ACPI tables.
>>>
>>> Instead of making the table constant itself (with its values), seems we can just
>>> make the policy->freq_table pointer constant. I'll try to make a patch for that,
>>> adjusting the pointers in cpufreq core and the drivers. This works for the
>>> acpi-cpufreq at least.
>>
>> Honestly, messing up with the whole subsystem in order to avoid an
>> explicit pointer case doesn't sound right to me.
> 
> Actually, on a second thought I agree that it is better to do it as
> you suggested: make the policy->freq_table pointer constant
> everywhere.
> 
> Sorry for the noise.

No worries.

As I wrote in the reply to the other patch, the Tegra30 support is now on the
way. These changes will collide a tad with the support integration, so I'll
return to re-considering the changes made in this patchset after Tegra30 support
will land. Thank you very much for your reviews and suggestions, I'll take them
into account in the next iteration.