Skip to contentSkip to conversation

PowerOP, OMAP1 PM Core 2/3

RFC In review Eugeny S. Mints 2 participants 3 messages Aug 8, 2006 – Aug 17, 2006

Subsystems (4)
Arm (32-bit) Arm sub-architectures OMAP1 support OMAP power management support
lore
Share & tools
lore

Eugeny S. Mints

author
#0 lore raw

RFC 1/1 PowerOP, OMAP1 PM Core 2/3

PM Core for OMAP1 platforms.

PM Core provides arch dependent routine to create/get/set operating points, hooks up with arhc independent PowerOP layer and utilizes clock/voltage framework to access hardware. PM Core is expected to handle arch specific actions required to take place in case of simultaneous coupled clocks/voltage change (for example what has to happen first) as well.

Current implementation relies on "smart" virtual mpu clock introduced by OMAP1 clock framework (#define PM_CORE_USE_MPU_CLOCK) but implementation for more common case is presented for reference as well in #else branch for PM_CORE_USE_MPU_CLOCK macro.

Since voltage framework is not merged into mainline yet routine to change voltage is temporary implemented in the PM Core for reference.

Please note that PM Core implementation is far incomplete and servers mainly reference and proof of concept purpose for the time being (yes, I know parsing implementation is ugly ;).

arch/arm/Kconfig +15 -0
index bad6269..7175bfb 100644
@@ -719,6 +719,21 @@ config CPU_FREQ_INTEGRATOR
 
 endmenu
 
+if (ARCH_OMAP)
+
+menu "PM Core"
+
+config OMAP1_PM_CORE
+	tristate "OMAP1 PM Core"
+	depends POWEROP
+	help
+	This enables OMAP1 PM Core to control platform clocks/power via
+	PowerOP interface
+
+endmenu
+
+endif
+
 endif
 
 menu "Floating point emulation"
arch/arm/mach-omap1/Makefile +3 -0
index 7165f74..de61ab1 100644
@@ -38,3 +38,6 @@ led-$(CONFIG_MACH_OMAP_PERSEUS2)	+= leds
 led-$(CONFIG_MACH_OMAP_OSK)		+= leds-osk.o
 obj-$(CONFIG_LEDS)			+= $(led-y)
 
+#PM Core
+obj-$(CONFIG_OMAP1_PM_CORE)		+=pm_core.o
+
arch/arm/mach-omap1/pm_core.c new file +483 -0

483 lines not on this page · raw message

include/asm-arm/arch-omap/pm_core.h new file +46 -0

46 lines not on this page · raw message

#1 lore raw

Hi!

Not only it does string parsing in generc code, it pushes it to architectures, too. Ouch.

Quoted 1 line

+static char *pwr_param_names_list = "cpu_vltg dpll cpu tc per dsp dspmmu lcd ";

...

Quoted 34 lines
arch/arm/mach-omap1/pm_core.c new file +34 -0
+#define PWR_PARAM_SET	1
+#define PWR_PARAM_GET	2
+/* FIXME: very temporary implementation, just to prove the concept !! */
+static int 
+process_pwr_param(struct pm_core_point *opt, int op, char *param_name,
+		  int va_arg)
+{
+	if (strcmp(param_name, "cpu_vltg") == 0) {
+		if (op == PWR_PARAM_SET)
+			opt->cpu_vltg = va_arg;
+		else if (opt != NULL)
+			*(int *)va_arg = opt->cpu_vltg;
+		else if (unlikely((*(int *)va_arg = get_vtg("v1")) <= 0))
+			return -EINVAL;
+
+		return 0;
+	}
+
+	if (strcmp(param_name, "dpll") == 0) {
+		if (op == PWR_PARAM_SET)
+			opt->dpll = va_arg;
+		else if (opt != NULL)
+			*(int *)va_arg = opt->dpll;
+		else if ((*(int *)va_arg = get_clk_rate("ck_dpll1")) <= 0)
+			return -EINVAL;
+
+		return 0;
+	}
+
+	if (strcmp(param_name, "cpu") == 0) {
+		if (op == PWR_PARAM_SET)
+			opt->cpu = va_arg;
+		else if (opt != NULL)
+			*(int *)va_arg = opt->cpu;

arch/arm/mach-omap1/pm_core.c line 265

Quoted 10 lines
arch/arm/mach-omap1/pm_core.c new file +10 -0
+		else if ((*(int *)va_arg = get_clk_rate("arm_ck")) <= 0)
+			return -EINVAL;
+
+		return 0;
+	}
+
+	/* FIXME: more parameters to process */
+
+	return -EINVAL;
+}

It certainly tells me I do not like the concept :-(.

Signature · 1 line
Thanks for all the (sleeping) penguins.
author
#2 lore raw
Quoted 5 lines · Pavel Machek

Hi!

Not only it does string parsing in generc code, it pushes it to architectures, too. Ouch.

are you talking about particular implementation of the parsing or about interfaces? Please note that the patches were mainly introduced now for _interfaces_ discussion and may contain suboptimal or just reference implementations.
If you are talking about having parsing implementation itself in a separate arch independent library then that's is intention.

I already addressed the question why parsing is needed at this level in separate reply to you.

Quoted 46 lines
Quoted 4 lines

+static char *pwr_param_names_list = "cpu_vltg dpll cpu tc per dsp dspmmu lcd ";

...

Quoted 39 lines
arch/arm/mach-omap1/pm_core.c new file +39 -0
+#define PWR_PARAM_SET	1
+#define PWR_PARAM_GET	2
+/* FIXME: very temporary implementation, just to prove the concept !! */
+static int 
+process_pwr_param(struct pm_core_point *opt, int op, char *param_name,
+		  int va_arg)
+{
+	if (strcmp(param_name, "cpu_vltg") == 0) {
+		if (op == PWR_PARAM_SET)
+			opt->cpu_vltg = va_arg;
+		else if (opt != NULL)
+			*(int *)va_arg = opt->cpu_vltg;
+		else if (unlikely((*(int *)va_arg = get_vtg("v1")) <= 0))
+			return -EINVAL;
+
+		return 0;
+	}
+
+	if (strcmp(param_name, "dpll") == 0) {
+		if (op == PWR_PARAM_SET)
+			opt->dpll = va_arg;
+		else if (opt != NULL)
+			*(int *)va_arg = opt->dpll;
+		else if ((*(int *)va_arg = get_clk_rate("ck_dpll1")) <= 0)
+			return -EINVAL;
+
+		return 0;
+	}
+
+	if (strcmp(param_name, "cpu") == 0) {
+		if (op == PWR_PARAM_SET)
+			opt->cpu = va_arg;
+		else if (opt != NULL)
+			*(int *)va_arg = opt->cpu;
+		else if ((*(int *)va_arg = get_clk_rate("arm_ck")) <= 0)
+			return -EINVAL;
+
+		return 0;
+	}
Quoted 8 lines

arch/arm/mach-omap1/pm_core.c line 265

Quoted 6 lines
arch/arm/mach-omap1/pm_core.c new file +5 -0
+
+	/* FIXME: more parameters to process */
+
+	return -EINVAL;
+}

It certainly tells me I do not like the concept :-(.

Concept is interfaces. And the concept addresses important design requirements (again as I've mentioned already in previous reply) while leading to some "parsing" at this level. So what is your point here?

Thanks,
Eugeny