Skip to contentSkip to conversation

OMAP2xxx clock/CPUFreq: fix compilation errors; clean up

PATCH Superseded Paul Walmsley 2 participants 12 messages Jan 5, 2010 – Jan 19, 2010

Subsystems (4)

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Paul Walmsley

author maintainer
#0 lore raw

cover OMAP2xxx clock/CPUFreq: fix compilation errors; clean up

Hello,

This series fixes several compilation errors for OMAP2xxx chips when CONFIG_CPU_FREQ is set. These were reported and partially fixed by Felipe Balbi <[email protected]>.

Considerable work is still needed to fully support dynamic frequency changes on OMAP2xxx-series chips. Readers interested in such a project are encouraged to review the Maemo Diablo RX-34 and RX-44 kernel source at:

http://repository.maemo.org/pool/diablo/free/k/kernel-source-diablo/

Boot-tested on N800 with n8x0_defconfig with CONFIG_CPU_FREQ; compile-tested for 2430SDP.

- Paul

---

Felipe Balbi (1):
OMAP2: clock: clk2xxx.c doesn't compile if CPUFREQ is enabled

Paul Walmsley (3):
OMAP2xxx OPP: clean up comments in OPP data
OMAP2 clock: dynamically allocate CPUFreq frequency table OMAP clock/CPUFreq: avoid leaking the CPUFreq frequency table

 arch/arm/mach-omap2/clock2xxx.c         |   60 ++++++++++++++++++++++++++-----
 arch/arm/mach-omap2/opp2420_data.c      |   38 ++++++++++----------
 arch/arm/mach-omap2/opp2430_data.c      |   30 ++++++++--------
 arch/arm/plat-omap/clock.c              |   10 +++++
 arch/arm/plat-omap/cpu-omap.c           |    1 +
 arch/arm/plat-omap/include/plat/clock.h |    2 +
 6 files changed, 99 insertions(+), 42 deletions(-)
author maintainer
#1 lore raw

PATCH 1/4 OMAP2: clock: clk2xxx.c doesn't compile if CPUFREQ is enabled

From: Felipe Balbi <[email protected]>

if we enable CPUFREQ we can't build omap2 for two reasons, one of them is fixed by the patch below.

It's failing because the __must_be_array() check in ARRAY_SIZE() is failing and printing the following message:

arch/arm/mach-omap2/clock2xxx.c:453: error: negative width in bit-field '<anonymous>'

[[email protected]: commit message updated; changed rate variable name]
---
 arch/arm/mach-omap2/clock2xxx.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
arch/arm/mach-omap2/clock2xxx.c +4 -1
index d0e3fb7..2f3c3ba 100644
@@ -455,12 +455,15 @@ static struct cpufreq_frequency_table freq_table[ARRAY_SIZE(rate_table)];
 void omap2_clk_init_cpufreq_table(struct cpufreq_frequency_table **table)
 {
 	struct prcm_config *prcm;
+	long rate;
 	int i = 0;
 
+	sys_ck_rate = clk_get_rate(sclk);
+
 	for (prcm = rate_table; prcm->mpu_speed; prcm++) {
 		if (!(prcm->flags & cpu_mask))
 			continue;
-		if (prcm->xtal_speed != sys_ck.rate)
+		if (prcm->xtal_speed != sys_ck_rate)
 			continue;
 
 		/* don't put bypass rates in table */
#2 lore raw

Re: [PATCH 1/4] OMAP2: clock: clk2xxx.c doesn't compile if CPUFREQ is enabled

Hi,

Quoted 5 lines · ext Paul Walmsley

From: Felipe Balbi <[email protected]>

if we enable CPUFREQ we can't build omap2 for two reasons, one of them is fixed by the patch below.

Quoted 7 lines

It's failing because the __must_be_array() check in ARRAY_SIZE() is failing and printing the following message:

arch/arm/mach-omap2/clock2xxx.c:453: error: negative width in bit-field '<anonymous>'

[[email protected]: commit message updated; changed rate variable name]

and you forgot to change the variable definition...

Quoted 10 lines

---
arch/arm/mach-omap2/clock2xxx.c | 5 ++++-

1 files changed, 4 insertions(+), 1 deletions(-)
arch/arm/mach-omap2/clock2xxx.c +0 -0
index d0e3fb7..2f3c3ba 100644

arch/arm/mach-omap2/clock2xxx.c static struct cpufreq_frequency_table freq_table[ARRAY_SIZE(rate_table)]; · line 461

Quoted 7 lines
arch/arm/mach-omap2/clock2xxx.c +2 -0
@@ -455,12 +455,15 @@ static struct cpufreq_frequency_table freq_table[ARRAY_SIZE(rate_table)];
 void omap2_clk_init_cpufreq_table(struct cpufreq_frequency_table **table)
 {
 	struct prcm_config *prcm;
+	long rate;
 	int i = 0;
 
+	sys_ck_rate = clk_get_rate(sclk);

The variable defined is called rate but you use sys_ck_rate. I used rate, because that's what other functions are using and I wanted to keep the code consistent. Anyways, as is now this patch won't compile again :-p

Signature · 1 line
balbi
author maintainer
#3 lore raw

Re: [PATCH 1/4] OMAP2: clock: clk2xxx.c doesn't compile if CPUFREQ is enabled

Quoted 1 line · Felipe Balbi

and you forgot to change the variable definition...

Thanks, looks like I sent an older version of the series. I will resend.

Quoted 3 lines

The variable defined is called rate but you use sys_ck_rate. I used rate, because that's what other functions are using and I wanted to keep the code consistent.

Did you examine the other functions omap2_round_to_table_rate() and omap2_select_table_rate()?

What advantage is there to keeping this variable named "rate", which is less specific given that this function also deals with DPLL rates, MPU rates, and crystal rates?

- Paul

author maintainer
#4 lore raw

PATCH 2/4 OMAP2xxx OPP: clean up comments in OPP data

In reply to Paul Walmsley

Revise some of the comments in the OMAP2xxx OPP data for clarity.

Cc Richard Woodruff <[email protected]>
---
 arch/arm/mach-omap2/opp2420_data.c |   38 +++++++++++++++++++-----------------
 arch/arm/mach-omap2/opp2430_data.c |   30 ++++++++++++++--------------
 2 files changed, 35 insertions(+), 33 deletions(-)
arch/arm/mach-omap2/opp2420_data.c +20 -18
index 126a939..e6dda69 100644
@@ -9,45 +9,47 @@
  * The OMAP2 processor can be run at several discrete 'PRCM configurations'.
  * These configurations are characterized by voltage and speed for clocks.
  * The device is only validated for certain combinations. One way to express
- * these combinations is via the 'ratio's' which the clocks operate with
+ * these combinations is via the 'ratios' which the clocks operate with
  * respect to each other. These ratio sets are for a given voltage/DPLL
- * setting. All configurations can be described by a DPLL setting and a ratio
- * There are 3 ratio sets for the 2430 and X ratio sets for 2420.
- *
- * 2430 differs from 2420 in that there are no more phase synchronizers used.
- * They both have a slightly different clock domain setup. 2420(iva1,dsp) vs
- * 2430 (iva2.1, NOdsp, mdm)
+ * setting. All configurations can be described by a DPLL setting and a ratio.
  *
  * XXX Missing voltage data.
+ * XXX Missing 19.2MHz sys_clk rate sets (needed for N800/N810)
  *
  * THe format described in this file is deprecated.  Once a reasonable
  * OPP API exists, the data in this file should be converted to use it.
  *
  * This is technically part of the OMAP2xxx clock code.
+ *
+ * Considerable work is still needed to fully support dynamic frequency
+ * changes on OMAP2xxx-series chips.  Readers interested in such a
+ * project are encouraged to review the Maemo Diablo RX-34 and RX-44
+ * kernel source at:
+ *     http://repository.maemo.org/pool/diablo/free/k/kernel-source-diablo/
  */
 
 #include "opp2xxx.h"
 #include "sdrc.h"
 #include "clock.h"
 
-/*-------------------------------------------------------------------------
- * Key dividers which make up a PRCM set. Ratio's for a PRCM are mandated.
+/*
+ * Key dividers which make up a PRCM set. Ratios for a PRCM are mandated.
  * xtal_speed, dpll_speed, mpu_speed, CM_CLKSEL_MPU,
  * CM_CLKSEL_DSP, CM_CLKSEL_GFX, CM_CLKSEL1_CORE, CM_CLKSEL1_PLL,
  * CM_CLKSEL2_PLL, CM_CLKSEL_MDM
  *
- * Filling in table based on H4 boards and 2430-SDPs variants available.
- * There are quite a few more rates combinations which could be defined.
+ * Filling in table based on H4 boards available.  There are quite a
+ * few more rate combinations which could be defined.
  *
- * When multiple values are defined the start up will try and choose the
- * fastest one. If a 'fast' value is defined, then automatically, the /2
- * one should be included as it can be used.	Generally having more that
- * one fast set does not make sense, as static timings need to be changed
- * to change the set.	 The exception is the bypass setting which is
- * availble for low power bypass.
+ * When multiple values are defined the start up will try and choose
+ * the fastest one. If a 'fast' value is defined, then automatically,
+ * the /2 one should be included as it can be used.  Generally having
+ * more than one fast set does not make sense, as static timings need
+ * to be changed to change the set.  The exception is the bypass
+ * setting which is available for low power bypass.
  *
  * Note: This table needs to be sorted, fastest to slowest.
- *-------------------------------------------------------------------------*/
+ **/
 const struct prcm_config omap2420_rate_table[] = {
 	/* PRCM I - FAST */
 	{S12M, S660M, S330M, RI_CM_CLKSEL_MPU_VAL,		/* 330MHz ARM */
arch/arm/mach-omap2/opp2430_data.c +15 -15
index edb8167..1b9596a 100644
@@ -1,5 +1,5 @@
 /*
- * opp2420_data.c - old-style "OPP" table for OMAP2420
+ * opp2430_data.c - old-style "OPP" table for OMAP2430
  *
  * Copyright (C) 2005-2009 Texas Instruments, Inc.
  * Copyright (C) 2004-2009 Nokia Corporation
@@ -9,16 +9,16 @@
  * The OMAP2 processor can be run at several discrete 'PRCM configurations'.
  * These configurations are characterized by voltage and speed for clocks.
  * The device is only validated for certain combinations. One way to express
- * these combinations is via the 'ratio's' which the clocks operate with
+ * these combinations is via the 'ratios' which the clocks operate with
  * respect to each other. These ratio sets are for a given voltage/DPLL
- * setting. All configurations can be described by a DPLL setting and a ratio
- * There are 3 ratio sets for the 2430 and X ratio sets for 2420.
+ * setting. All configurations can be described by a DPLL setting and a ratio.
  *
  * 2430 differs from 2420 in that there are no more phase synchronizers used.
  * They both have a slightly different clock domain setup. 2420(iva1,dsp) vs
  * 2430 (iva2.1, NOdsp, mdm)
  *
  * XXX Missing voltage data.
+ * XXX Missing 19.2MHz sys_clk rate sets.
  *
  * THe format described in this file is deprecated.  Once a reasonable
  * OPP API exists, the data in this file should be converted to use it.
@@ -30,24 +30,24 @@
 #include "sdrc.h"
 #include "clock.h"
 
-/*-------------------------------------------------------------------------
- * Key dividers which make up a PRCM set. Ratio's for a PRCM are mandated.
+/*
+ * Key dividers which make up a PRCM set. Ratios for a PRCM are mandated.
  * xtal_speed, dpll_speed, mpu_speed, CM_CLKSEL_MPU,
  * CM_CLKSEL_DSP, CM_CLKSEL_GFX, CM_CLKSEL1_CORE, CM_CLKSEL1_PLL,
  * CM_CLKSEL2_PLL, CM_CLKSEL_MDM
  *
- * Filling in table based on H4 boards and 2430-SDPs variants available.
- * There are quite a few more rates combinations which could be defined.
+ * Filling in table based on 2430-SDPs variants available.  There are
+ * quite a few more rate combinations which could be defined.
  *
- * When multiple values are defined the start up will try and choose the
- * fastest one. If a 'fast' value is defined, then automatically, the /2
- * one should be included as it can be used.	Generally having more that
- * one fast set does not make sense, as static timings need to be changed
- * to change the set.	 The exception is the bypass setting which is
- * availble for low power bypass.
+ * When multiple values are defined the start up will try and choose
+ * the fastest one. If a 'fast' value is defined, then automatically,
+ * the /2 one should be included as it can be used.  Generally having
+ * more than one fast set does not make sense, as static timings need
+ * to be changed to change the set.  The exception is the bypass
+ * setting which is available for low power bypass.
  *
  * Note: This table needs to be sorted, fastest to slowest.
- *-------------------------------------------------------------------------*/
+ */
 const struct prcm_config omap2430_rate_table[] = {
 	/* PRCM #4 - ratio2 (ES2.1) - FAST */
 	{S13M, S798M, S399M, R2_CM_CLKSEL_MPU_VAL,		/* 399MHz ARM */
author maintainer
#5 lore raw

PATCH 3/4 OMAP2 clock: dynamically allocate CPUFreq frequency table

In reply to Paul Walmsley

Dynamically allocate the CPUFreq frequency table on OMAP2xxx chips. This fixes some compilation problems, since the kernel may not know what chip it is running on until boot-time. This also reduces the size of the CPUFreq frequency table.

Problem reported by Felipe Balbi <[email protected]>.

---
 arch/arm/mach-omap2/clock2xxx.c |   50 ++++++++++++++++++++++++++++++++-------
 1 files changed, 41 insertions(+), 9 deletions(-)
arch/arm/mach-omap2/clock2xxx.c +41 -9
index 2f3c3ba..5c21afe 100644
@@ -449,14 +449,16 @@ int omap2_select_table_rate(struct clk *clk, unsigned long rate)
 #ifdef CONFIG_CPU_FREQ
 /*
  * Walk PRCM rate table and fillout cpufreq freq_table
+ * XXX This should be replaced by an OPP layer in the near future
  */
-static struct cpufreq_frequency_table freq_table[ARRAY_SIZE(rate_table)];
+static struct cpufreq_frequency_table *freq_table;
 
 void omap2_clk_init_cpufreq_table(struct cpufreq_frequency_table **table)
 {
-	struct prcm_config *prcm;
-	long rate;
+	const struct prcm_config *prcm;
+	long sys_ck_rate;
 	int i = 0;
+	int tbl_sz = 0;
 
 	sys_ck_rate = clk_get_rate(sclk);
 
@@ -470,21 +472,51 @@ void omap2_clk_init_cpufreq_table(struct cpufreq_frequency_table **table)
 		if (prcm->dpll_speed == prcm->xtal_speed)
 			continue;
 
+		tbl_sz++;
+	}
+
+	/*
+	 * XXX Ensure that we're doing what CPUFreq expects for this error
+	 * case and the following one
+	 */
+	if (tbl_sz == 0) {
+		pr_warning("%s: no matching entries in rate_table\n",
+			   __func__);
+		goto ocict_exit;
+	}
+
+	/* Include the CPUFREQ_TABLE_END terminator entry */
+	tbl_sz++;
+
+	freq_table = kzalloc(sizeof(struct cpufreq_frequency_table) * tbl_sz,
+			     GFP_KERNEL);
+	if (!freq_table) {
+		pr_err("%s: could not kzalloc frequency table\n", __func__);
+		goto ocict_exit;
+	}
+
+	for (prcm = rate_table; prcm->mpu_speed; prcm++) {
+		if (!(prcm->flags & cpu_mask))
+			continue;
+		if (prcm->xtal_speed != sys_ck_rate)
+			continue;
+
+		/* don't put bypass rates in table */
+		if (prcm->dpll_speed == prcm->xtal_speed)
+			continue;
+
 		freq_table[i].index = i;
 		freq_table[i].frequency = prcm->mpu_speed / 1000;
 		i++;
 	}
 
-	if (i == 0) {
-		printk(KERN_WARNING "%s: failed to initialize frequency "
-		       "table\n", __func__);
-		return;
-	}
-
 	freq_table[i].index = i;
 	freq_table[i].frequency = CPUFREQ_TABLE_END;
 
 	*table = &freq_table[0];
+
+ocict_exit:
+	return;
 }
 #endif
 
#6 lore raw

Re: [PATCH 3/4] OMAP2 clock: dynamically allocate CPUFreq frequency table

Hi,

Quoted 5 lines · ext Paul Walmsley
arch/arm/mach-omap2/clock2xxx.c +1 -0
@@ -470,21 +472,51 @@ void omap2_clk_init_cpufreq_table(struct cpufreq_frequency_table **table)
 		if (prcm->dpll_speed == prcm->xtal_speed)
 			continue;
 
+		tbl_sz++;

arch/arm/mach-omap2/clock2xxx.c void omap2_clk_init_cpufreq_table(struct cpufreq_frequency_table **table) · line 485

Quoted 10 lines
arch/arm/mach-omap2/clock2xxx.c +10 -0
+	}
+
+	/*
+	 * XXX Ensure that we're doing what CPUFreq expects for this error
+	 * case and the following one
+	 */
+	if (tbl_sz == 0) {
+		pr_warning("%s: no matching entries in rate_table\n",
+			   __func__);
+		goto ocict_exit;

you only use this ocict_exit here, so how about only return instead of a goto to a return ?

Signature · 1 line
balbi
author maintainer
#7 lore raw

Re: [PATCH 3/4] OMAP2 clock: dynamically allocate CPUFreq frequency table

Quoted 6 lines · Felipe Balbi
Quoted 5 lines · ext Paul Walmsley
arch/arm/mach-omap2/clock2xxx.c +0 -0
@@ -470,21 +472,51 @@ void omap2_clk_init_cpufreq_table(struct
 cpufreq_frequency_table **table)
 		if (prcm->dpll_speed == prcm->xtal_speed)
 			continue;
 
Quoted 14 lines

arch/arm/mach-omap2/clock2xxx.c void omap2_clk_init_cpufreq_table(struct cpufreq_frequency_table **table) · line 485

Quoted 11 lines
arch/arm/mach-omap2/clock2xxx.c +11 -0
+		tbl_sz++;
+	}
+
+	/*
+	 * XXX Ensure that we're doing what CPUFreq expects for this error
+	 * case and the following one
+	 */
+	if (tbl_sz == 0) {
+		pr_warning("%s: no matching entries in rate_table\n",
+			   __func__);
+		goto ocict_exit;

you only use this ocict_exit here, so how about only return instead of a goto to a return ?

It's used twice. But your latter point is a good one. I will change that.

- Paul

author maintainer
#8 lore raw

PATCH 4/4 OMAP clock/CPUFreq: avoid leaking the CPUFreq frequency table

In reply to Paul Walmsley

OMAP2xxx uses a dynamically-allocated cpufreq_frequency_table array, so this patch ensures that it is freed if CPUFreq terminates.

---
 arch/arm/mach-omap2/clock2xxx.c         |    7 +++++++
 arch/arm/plat-omap/clock.c              |   10 ++++++++++
 arch/arm/plat-omap/cpu-omap.c           |    1 +
 arch/arm/plat-omap/include/plat/clock.h |    2 ++
 4 files changed, 20 insertions(+), 0 deletions(-)
arch/arm/mach-omap2/clock2xxx.c +7 -0
index 5c21afe..ec57927 100644
@@ -518,6 +518,12 @@ void omap2_clk_init_cpufreq_table(struct cpufreq_frequency_table **table)
 ocict_exit:
 	return;
 }
+
+void omap2_clk_exit_cpufreq_table(struct cpufreq_frequency_table **table)
+{
+	kfree(freq_table);
+}
+
 #endif
 
 struct clk_functions omap2_clk_functions = {
@@ -529,6 +535,7 @@ struct clk_functions omap2_clk_functions = {
 	.clk_disable_unused	= omap2_clk_disable_unused,
 #ifdef	CONFIG_CPU_FREQ
 	.clk_init_cpufreq_table	= omap2_clk_init_cpufreq_table,
+	.clk_exit_cpufreq_table	= omap2_clk_exit_cpufreq_table,
 #endif
 };
 
arch/arm/plat-omap/clock.c +10 -0
index a64d84c..d9f8c84 100644
@@ -329,6 +329,16 @@ void clk_init_cpufreq_table(struct cpufreq_frequency_table **table)
 		arch_clock->clk_init_cpufreq_table(table);
 	spin_unlock_irqrestore(&clockfw_lock, flags);
 }
+
+void clk_exit_cpufreq_table(struct cpufreq_frequency_table **table)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&clockfw_lock, flags);
+	if (arch_clock->clk_exit_cpufreq_table)
+		arch_clock->clk_exit_cpufreq_table(table);
+	spin_unlock_irqrestore(&clockfw_lock, flags);
+}
 #endif
 
 /*-------------------------------------------------------------------------*/
arch/arm/plat-omap/cpu-omap.c +1 -0
index f8ddbdd..6d3d333 100644
@@ -134,6 +134,7 @@ static int __init omap_cpu_init(struct cpufreq_policy *policy)
 
 static int omap_cpu_exit(struct cpufreq_policy *policy)
 {
+	clk_exit_cpufreq_table(&freq_table);
 	clk_put(mpu_clk);
 	return 0;
 }
arch/arm/plat-omap/include/plat/clock.h +2 -0
index 309b6d1..94fe2a0 100644
@@ -119,6 +119,7 @@ struct clk_functions {
 	void		(*clk_disable_unused)(struct clk *clk);
 #ifdef CONFIG_CPU_FREQ
 	void		(*clk_init_cpufreq_table)(struct cpufreq_frequency_table **);
+	void		(*clk_exit_cpufreq_table)(struct cpufreq_frequency_table **);
 #endif
 };
 
@@ -135,6 +136,7 @@ extern unsigned long followparent_recalc(struct clk *clk);
 extern void clk_enable_init_clocks(void);
 #ifdef CONFIG_CPU_FREQ
 extern void clk_init_cpufreq_table(struct cpufreq_frequency_table **table);
+extern void clk_exit_cpufreq_table(struct cpufreq_frequency_table **table);
 #endif
 
 extern const struct clkops clkops_null;
#9 lore raw

Re: [PATCH 4/4] OMAP clock/CPUFreq: avoid leaking the CPUFreq frequency table

Quoted 2 lines · ext Paul Walmsley

OMAP2xxx uses a dynamically-allocated cpufreq_frequency_table array, so this patch ensures that it is freed if CPUFreq terminates.

wasn't this introduced by the previous patch ?

Signature · 1 line
balbi
author maintainer
#10 lore raw

Re: [PATCH 4/4] OMAP clock/CPUFreq: avoid leaking the CPUFreq frequency table

Quoted 5 lines · Felipe Balbi
Quoted 2 lines · ext Paul Walmsley

OMAP2xxx uses a dynamically-allocated cpufreq_frequency_table array, so this patch ensures that it is freed if CPUFreq terminates.

wasn't this introduced by the previous patch ?

The dynamically-allocated array? Yes. Your point being?

- Paul

author maintainer
#11 lore raw

Re: [PATCH 4/4] OMAP clock/CPUFreq: avoid leaking the CPUFreq frequency table

Hi Felipe,

Quoted 8 lines · Paul Walmsley
Quoted 5 lines · Felipe Balbi
Quoted 2 lines · ext Paul Walmsley

OMAP2xxx uses a dynamically-allocated cpufreq_frequency_table array, so this patch ensures that it is freed if CPUFreq terminates.

wasn't this introduced by the previous patch ?

The dynamically-allocated array? Yes. Your point being?

By the way, I am sorry for this - it was too brusque for how I believe collaborative development should be. Next time I will be more clear about the question...

regards,

- Paul