cpufreq: tegra20: Constify rate value of the intermediate clk

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 <[email protected]>

---
 drivers/cpufreq/tegra20-cpufreq.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

drivers/cpufreq/tegra20-cpufreq.c +3 -1

index 3ad6bded6efc..4bf5ba7da40b 100644
@@ -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;
 };
 

Quoted 38 lines · 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 <[email protected]>

---
 drivers/cpufreq/tegra20-cpufreq.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

drivers/cpufreq/tegra20-cpufreq.c +3 -1

index 3ad6bded6efc..4bf5ba7da40b 100644
@@ -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;
 };
 

drivers/cpufreq/tegra20-cpufreq.c static unsigned int tegra_get_intermediate(struct cpufreq_policy *policy, · line 64

Quoted 6 lines

drivers/cpufreq/tegra20-cpufreq.c +2 -1

@@ -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.

Quoted 50 lines · Viresh Kumar

Quoted 38 lines · 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 <[email protected]>

---
 drivers/cpufreq/tegra20-cpufreq.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

drivers/cpufreq/tegra20-cpufreq.c +3 -1

index 3ad6bded6efc..4bf5ba7da40b 100644
@@ -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;
 };
 

drivers/cpufreq/tegra20-cpufreq.c static unsigned int tegra_get_intermediate(struct cpufreq_policy *policy, · line 64

Quoted 6 lines

drivers/cpufreq/tegra20-cpufreq.c +2 -1

@@ -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.

Quoted 2 lines

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.

Quoted 63 lines · Dmitry Osipenko

Quoted 50 lines · Viresh Kumar

Quoted 38 lines · 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 <[email protected]>

---
 drivers/cpufreq/tegra20-cpufreq.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

drivers/cpufreq/tegra20-cpufreq.c +3 -1

index 3ad6bded6efc..4bf5ba7da40b 100644
@@ -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;
 };
 

drivers/cpufreq/tegra20-cpufreq.c static unsigned int tegra_get_intermediate(struct cpufreq_policy *policy, · line 64

Quoted 6 lines

drivers/cpufreq/tegra20-cpufreq.c +2 -1

@@ -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.

Quoted 2 lines

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.

Quoted 4 lines

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?

Quoted 69 lines · Rafael J. Wysocki

Quoted 67 lines · Dmitry Osipenko

Quoted 50 lines · Viresh Kumar

Quoted 38 lines · 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 <[email protected]>

---
 drivers/cpufreq/tegra20-cpufreq.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

drivers/cpufreq/tegra20-cpufreq.c +3 -1

index 3ad6bded6efc..4bf5ba7da40b 100644
@@ -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;
 };
 

drivers/cpufreq/tegra20-cpufreq.c static unsigned int tegra_get_intermediate(struct cpufreq_policy *policy, · line 64

Quoted 6 lines

drivers/cpufreq/tegra20-cpufreq.c +2 -1

@@ -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.

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 5 lines

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.

Quoted 73 lines · Dmitry Osipenko

Quoted 71 lines · Rafael J. Wysocki

Quoted 63 lines · Dmitry Osipenko

Quoted 50 lines · Viresh Kumar

Quoted 38 lines · 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 <[email protected]>

---
 drivers/cpufreq/tegra20-cpufreq.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

drivers/cpufreq/tegra20-cpufreq.c +3 -1

index 3ad6bded6efc..4bf5ba7da40b 100644
@@ -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;
 };
 

drivers/cpufreq/tegra20-cpufreq.c static unsigned int tegra_get_intermediate(struct cpufreq_policy *policy, · line 64

Quoted 6 lines

drivers/cpufreq/tegra20-cpufreq.c +2 -1

@@ -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.

Quoted 2 lines

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.

Quoted 4 lines

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.

Quoted 4 lines

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.

Quoted 79 lines · Rafael J. Wysocki

Quoted 77 lines · Dmitry Osipenko

Quoted 64 lines · Rafael J. Wysocki

Quoted 63 lines · Dmitry Osipenko

Quoted 50 lines · Viresh Kumar

Quoted 38 lines · 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 <[email protected]>

---
 drivers/cpufreq/tegra20-cpufreq.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

drivers/cpufreq/tegra20-cpufreq.c +3 -1

index 3ad6bded6efc..4bf5ba7da40b 100644
@@ -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;
 };
 

drivers/cpufreq/tegra20-cpufreq.c static unsigned int tegra_get_intermediate(struct cpufreq_policy *policy, · line 64

Quoted 6 lines

drivers/cpufreq/tegra20-cpufreq.c +2 -1

@@ -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.

Quoted 2 lines

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.

Quoted 7 lines

Quoted 4 lines

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.

Quoted 2 lines

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.

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 <[email protected]>

---
 drivers/cpufreq/tegra20-cpufreq.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

drivers/cpufreq/tegra20-cpufreq.c +21 -4

index 3ad6bded6efc..4bf5ba7da40b 100644
@@ -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);

Signature · 1 line

2.17.0

Quoted 5 lines · 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.

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

Quoted 7 lines · Peter De Schrijver

Quoted 5 lines · 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.

Quoted 2 lines

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.

Quoted 8 lines · Dmitry Osipenko

Quoted 7 lines · Peter De Schrijver

Quoted 5 lines · 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.

Quoted 8 lines

Quoted 2 lines

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.

Quoted 3 lines

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.

Quoted 3 lines

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.

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

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/cpufreq/tegra20-cpufreq.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

drivers/cpufreq/tegra20-cpufreq.c +4 -6

index 05f57dcd5215..3ad6bded6efc 100644
@@ -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;