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

[Dmitry Osipenko <digetx@gmail.com>]

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.