RE: [PATCHv2 6/13] dmtimer: hwmod: OMAP2PLUS: device registration
Kevin,
-Original Message-
From: Kevin Hilman [mailto:khil...@deeprootsystems.com]
Sent: Wednesday, September 01, 2010 9:00 PM
To: DebBarma, Tarun Kanti
Cc: linux-omap@vger.kernel.org; Gopinath, Thara; Basak, Partha; Nayak,
Rajendra; Paul Walmsley; Tony Lindgren
Subject: Re: [PATCHv2 6/13] dmtimer: hwmod: OMAP2PLUS: device registration
DebBarma, Tarun Kanti tarun.ka...@ti.com writes:
[...]
+static void omap2_dm_timer_enable(struct platform_device *pdev)
+{
+ if (!pdev) {
+ dev_err(pdev-dev, %s: invalid pdev\n, __func__);
+ return;
+ }
+ pm_runtime_get_sync(pdev-dev);
+}
+
+static void omap2_dm_timer_disable(struct platform_device *pdev)
+{
+ if (!pdev) {
+ dev_err(pdev-dev, %s: invalid pdev\n, __func__);
+ return;
+ }
+ pm_runtime_put_sync(pdev-dev);
+}
There is no need for these functions. Driver should be calling runtime
PM API directly.
In that case, driver will have to make call by directly accessing
acquired timer's internal variables as shown below.
pm_runtime_put_sync(timer-pdev-dev);
However, I believe we would like other drivers' perform all operations
on acquired timers through exported APIs and keep the
pm_runtime_put_sync() call internal to exported APIs as is the case
current implementation.
Let me know if I am missing anything here!!
These new functions are not part of the exported API. You have made
them static, and part of the device layer.
These are created as wrappers to pm_runtime_* API which are then called
by the driver layer. What I suggest is not creating these functions
(and the pdata- function pointers to wrap them) but instead calling the
runtime PM api directly instead of using the pdata- function pointers.
OK.
Kevin
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCHv2 6/13] dmtimer: hwmod: OMAP2PLUS: device registration
Kevin,
-Original Message-
From: Kevin Hilman [mailto:khil...@deeprootsystems.com]
Sent: Tuesday, August 24, 2010 5:41 AM
To: DebBarma, Tarun Kanti
Cc: linux-omap@vger.kernel.org; Gopinath, Thara; Basak, Partha; Nayak,
Rajendra; Paul Walmsley; Tony Lindgren
Subject: Re: [PATCHv2 6/13] dmtimer: hwmod: OMAP2PLUS: device registration
Tarun Kanti DebBarma tarun.ka...@ti.com writes:
+/**
+* omap2_dm_timer_early_init - top level early timer initialization
+* called in the last part of omap2_init_common_hw
+*
+* uses dedicated hwmod api to parse through hwmod database for
+* given class name and then build and register the timer device.
+* at the end driver is registered and early probe initiated.
+**/
+void __init omap2_dm_timer_early_init(void)
+{
+ omap_hwmod_for_each_by_class(timer_1ms,
+ omap_dm_timer_early_init, NULL);
+ omap2_dm_timer_setup();
+ early_platform_driver_register_all(earlytimer);
+ early_platform_driver_probe(earlytimer, early_timer_count + 1, 0);
+}
It's not clear (or documented) why on the 1ms timers should be the only
earlydevices.
For example, GPT12 is used as the system timer on Beagle due to a board
bug in early revs of the board. That will no longer function with this
approach.
OK, I will change the design so that any timers could be used as early timers.
Kevin
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCHv2 6/13] dmtimer: hwmod: OMAP2PLUS: device registration
Kevin,
-Original Message-
From: Kevin Hilman [mailto:khil...@deeprootsystems.com]
Sent: Tuesday, August 24, 2010 5:37 AM
To: DebBarma, Tarun Kanti
Cc: linux-omap@vger.kernel.org; Gopinath, Thara; Basak, Partha; Nayak,
Rajendra; Paul Walmsley; Tony Lindgren
Subject: Re: [PATCHv2 6/13] dmtimer: hwmod: OMAP2PLUS: device registration
Tarun Kanti DebBarma tarun.ka...@ti.com writes:
From: Thara Gopinath th...@ti.com
This patch converts dmtimers into platform devices using omap hwmod,
omap device and runtime pm frameworks. It also allows GPTIMER1 and
GPTIMER2 and GPTIMER10 to be registered as early devices. This allows
any one of the these three to be registered as system timer very early
during system boot sequence. Later during normal plaform_device_register
these are converted to normal platform device.
What about GPT12 which is used as system timer on Beagle due to a board
bug on early revs of board?
Will modify design to incorporate any of the timers as an early timer.
comments incorporated:
(1) registration of devices through automatic learning from hwmod
database
(2) enabling functionality both with and without pm_runtime
(3) extract device id from hwmod structure's name field.
(4) free platform data at the end of successful platform device
registration
I still don't like the way early devices are implemented. My comments
from the first time I reviewed this series appear to have been ignored.
It is not at all clear why the separate pdata functions are needed for
early and normal devices. Many more questions/comments on this below...
Will merge the two and embed them within pdata.
Signed-off-by: Partha Basak p-bas...@ti.com
Signed-off-by: Thara Gopinath th...@ti.com
Signed-off-by: Rajendra Nayak rna...@ti.com
Signed-off-by: Tarun Kanti DebBarma tarun.ka...@ti.com
Cc: Paul Walmsley p...@pwsan.com
Cc: Kevin Hilman khil...@deeprootsystems.com
Cc: Tony Lindgren t...@atomide.com
---
arch/arm/mach-omap2/Makefile |2 +-
arch/arm/mach-omap2/dmtimer.c | 403
arch/arm/mach-omap2/dmtimer.h | 19 ++
arch/arm/mach-omap2/io.c |2 +
arch/arm/mach-omap2/timer-gp.c |1 -
5 files changed, 425 insertions(+), 2 deletions(-)
create mode 100755 arch/arm/mach-omap2/dmtimer.c
create mode 100755 arch/arm/mach-omap2/dmtimer.h
mode change 100644 = 100755 arch/arm/mach-omap2/io.c
diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile
index 9b44773..c54f6e8 100644
--- a/arch/arm/mach-omap2/Makefile
+++ b/arch/arm/mach-omap2/Makefile
@@ -3,7 +3,7 @@
#
# Common support
-obj-y := id.o io.o control.o mux.o devices.o serial.o gpmc.o timer-gp.o
pm.o
+obj-y := id.o io.o control.o mux.o devices.o serial.o gpmc.o timer-gp.o
pm.o dmtimer.o
omap-2-3-common= irq.o sdrc.o
hwmod-common = omap_hwmod.o \
diff --git a/arch/arm/mach-omap2/dmtimer.c b/arch/arm/mach-
omap2/dmtimer.c
new file mode 100755
index 000..2c40a9b
--- /dev/null
+++ b/arch/arm/mach-omap2/dmtimer.c
@@ -0,0 +1,403 @@
+/**
+ * linux/arch/arm/mach-omap2/dmtimer.c
+ *
+ * Copyright (C) 2010 Texas Instruments, Inc.
+ * Thara Gopinath th...@ti.com
+ * Tarun Kanti DebBarma tarun.ka...@ti.com
+ * - Highlander ip support on omap4
+ * - hwmod support
+ *
+ * OMAP2 Dual-Mode Timers
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#include linux/clk.h
+#include linux/delay.h
+#include linux/io.h
+#include linux/err.h
+#include linux/slab.h
+
+#include mach/irqs.h
+#include plat/dmtimer.h
+#include plat/omap_hwmod.h
+#include plat/omap_device.h
+#include linux/pm_runtime.h
+
+static int early_timer_count __initdata;
+static int is_early_init __initdata = 1;
+
+static char *omap2_dm_source_names[] __initdata = {
+ sys_ck,
+ func_32k_ck,
+ alt_ck,
+ NULL
+};
+static struct clk *omap2_dm_source_clocks[3];
+static char *omap3_dm_source_names[] __initdata = {
+ sys_ck,
+ omap_32k_fck,
+ NULL
+};
+static struct clk *omap3_dm_source_clocks[2];
+
+static char *omap4_dm_source_names[] __initdata = {
+ sys_clkin_ck,
+ sys_32k_ck,
+ syc_clk_div_ck,
+ NULL
+};
+static struct clk *omap4_dm_source_clocks[3];
+
+static struct clk **omap_dm_source_clocks;
The clock names should all be part of pdata too. There is no reason
to keep these SoC specifics in the driver.
OK, I will change.
+static void omap2_dm_timer_enable(struct platform_device *pdev)
+{
+ if (!pdev) {
+ dev_err(pdev-dev, %s: invalid pdev\n, __func__);
+ return;
+ }
+ pm_runtime_get_sync(pdev-dev);
+}
+
+static void
Re: [PATCHv2 6/13] dmtimer: hwmod: OMAP2PLUS: device registration
DebBarma, Tarun Kanti tarun.ka...@ti.com writes:
[...]
+static void omap2_dm_timer_enable(struct platform_device *pdev)
+{
+ if (!pdev) {
+ dev_err(pdev-dev, %s: invalid pdev\n, __func__);
+ return;
+ }
+ pm_runtime_get_sync(pdev-dev);
+}
+
+static void omap2_dm_timer_disable(struct platform_device *pdev)
+{
+ if (!pdev) {
+ dev_err(pdev-dev, %s: invalid pdev\n, __func__);
+ return;
+ }
+ pm_runtime_put_sync(pdev-dev);
+}
There is no need for these functions. Driver should be calling runtime
PM API directly.
In that case, driver will have to make call by directly accessing
acquired timer's internal variables as shown below.
pm_runtime_put_sync(timer-pdev-dev);
However, I believe we would like other drivers' perform all operations
on acquired timers through exported APIs and keep the
pm_runtime_put_sync() call internal to exported APIs as is the case
current implementation.
Let me know if I am missing anything here!!
These new functions are not part of the exported API. You have made
them static, and part of the device layer.
These are created as wrappers to pm_runtime_* API which are then called
by the driver layer. What I suggest is not creating these functions
(and the pdata- function pointers to wrap them) but instead calling the
runtime PM api directly instead of using the pdata- function pointers.
Kevin
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 6/13] dmtimer: hwmod: OMAP2PLUS: device registration
Tarun Kanti DebBarma tarun.ka...@ti.com writes:
From: Thara Gopinath th...@ti.com
This patch converts dmtimers into platform devices using omap hwmod,
omap device and runtime pm frameworks. It also allows GPTIMER1 and
GPTIMER2 and GPTIMER10 to be registered as early devices. This allows
any one of the these three to be registered as system timer very early
during system boot sequence. Later during normal plaform_device_register
these are converted to normal platform device.
What about GPT12 which is used as system timer on Beagle due to a board
bug on early revs of board?
comments incorporated:
(1) registration of devices through automatic learning from hwmod database
(2) enabling functionality both with and without pm_runtime
(3) extract device id from hwmod structure's name field.
(4) free platform data at the end of successful platform device registration
I still don't like the way early devices are implemented. My comments
from the first time I reviewed this series appear to have been ignored.
It is not at all clear why the separate pdata functions are needed for
early and normal devices. Many more questions/comments on this below...
Signed-off-by: Partha Basak p-bas...@ti.com
Signed-off-by: Thara Gopinath th...@ti.com
Signed-off-by: Rajendra Nayak rna...@ti.com
Signed-off-by: Tarun Kanti DebBarma tarun.ka...@ti.com
Cc: Paul Walmsley p...@pwsan.com
Cc: Kevin Hilman khil...@deeprootsystems.com
Cc: Tony Lindgren t...@atomide.com
---
arch/arm/mach-omap2/Makefile |2 +-
arch/arm/mach-omap2/dmtimer.c | 403
arch/arm/mach-omap2/dmtimer.h | 19 ++
arch/arm/mach-omap2/io.c |2 +
arch/arm/mach-omap2/timer-gp.c |1 -
5 files changed, 425 insertions(+), 2 deletions(-)
create mode 100755 arch/arm/mach-omap2/dmtimer.c
create mode 100755 arch/arm/mach-omap2/dmtimer.h
mode change 100644 = 100755 arch/arm/mach-omap2/io.c
diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile
index 9b44773..c54f6e8 100644
--- a/arch/arm/mach-omap2/Makefile
+++ b/arch/arm/mach-omap2/Makefile
@@ -3,7 +3,7 @@
#
# Common support
-obj-y := id.o io.o control.o mux.o devices.o serial.o gpmc.o timer-gp.o pm.o
+obj-y := id.o io.o control.o mux.o devices.o serial.o gpmc.o timer-gp.o pm.o
dmtimer.o
omap-2-3-common = irq.o sdrc.o
hwmod-common = omap_hwmod.o \
diff --git a/arch/arm/mach-omap2/dmtimer.c b/arch/arm/mach-omap2/dmtimer.c
new file mode 100755
index 000..2c40a9b
--- /dev/null
+++ b/arch/arm/mach-omap2/dmtimer.c
@@ -0,0 +1,403 @@
+/**
+ * linux/arch/arm/mach-omap2/dmtimer.c
+ *
+ * Copyright (C) 2010 Texas Instruments, Inc.
+ * Thara Gopinath th...@ti.com
+ * Tarun Kanti DebBarma tarun.ka...@ti.com
+ * - Highlander ip support on omap4
+ * - hwmod support
+ *
+ * OMAP2 Dual-Mode Timers
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#include linux/clk.h
+#include linux/delay.h
+#include linux/io.h
+#include linux/err.h
+#include linux/slab.h
+
+#include mach/irqs.h
+#include plat/dmtimer.h
+#include plat/omap_hwmod.h
+#include plat/omap_device.h
+#include linux/pm_runtime.h
+
+static int early_timer_count __initdata;
+static int is_early_init __initdata = 1;
+
+static char *omap2_dm_source_names[] __initdata = {
+ sys_ck,
+ func_32k_ck,
+ alt_ck,
+ NULL
+};
+static struct clk *omap2_dm_source_clocks[3];
+static char *omap3_dm_source_names[] __initdata = {
+ sys_ck,
+ omap_32k_fck,
+ NULL
+};
+static struct clk *omap3_dm_source_clocks[2];
+
+static char *omap4_dm_source_names[] __initdata = {
+ sys_clkin_ck,
+ sys_32k_ck,
+ syc_clk_div_ck,
+ NULL
+};
+static struct clk *omap4_dm_source_clocks[3];
+
+static struct clk **omap_dm_source_clocks;
The clock names should all be part of pdata too. There is no reason
to keep these SoC specifics in the driver.
+static void omap2_dm_timer_enable(struct platform_device *pdev)
+{
+ if (!pdev) {
+ dev_err(pdev-dev, %s: invalid pdev\n, __func__);
+ return;
+ }
+ pm_runtime_get_sync(pdev-dev);
+}
+
+static void omap2_dm_timer_disable(struct platform_device *pdev)
+{
+ if (!pdev) {
+ dev_err(pdev-dev, %s: invalid pdev\n, __func__);
+ return;
+ }
+ pm_runtime_put_sync(pdev-dev);
+}
There is no need for these functions. Driver should be calling runtime
PM API directly.
+static void omap2_dm_early_timer_enable(struct platform_device *pdev)
+{
+#ifdef CONFIG_PM_RUNTIME
+ /* when pm_runtime is enabled, it is still inactive at this point
wrong multi-line comment style
+ * that is why this call is needed as it is not enabled by default
+ */
so
Re: [PATCHv2 6/13] dmtimer: hwmod: OMAP2PLUS: device registration
Tarun Kanti DebBarma tarun.ka...@ti.com writes:
+/**
+* omap2_dm_timer_early_init - top level early timer initialization
+* called in the last part of omap2_init_common_hw
+*
+* uses dedicated hwmod api to parse through hwmod database for
+* given class name and then build and register the timer device.
+* at the end driver is registered and early probe initiated.
+**/
+void __init omap2_dm_timer_early_init(void)
+{
+ omap_hwmod_for_each_by_class(timer_1ms,
+ omap_dm_timer_early_init, NULL);
+ omap2_dm_timer_setup();
+ early_platform_driver_register_all(earlytimer);
+ early_platform_driver_probe(earlytimer, early_timer_count + 1, 0);
+}
It's not clear (or documented) why on the 1ms timers should be the only
earlydevices.
For example, GPT12 is used as the system timer on Beagle due to a board
bug in early revs of the board. That will no longer function with this
approach.
Kevin
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
