RE: [PATCHv4 11/14] OMAP2+: dmtimer: convert to platform devices
> -Original Message- > From: Cousson, Benoit > Sent: Tuesday, November 23, 2010 11:21 PM > To: DebBarma, Tarun Kanti > Cc: linux-omap@vger.kernel.org; Gopinath, Thara; Basak, Partha > Subject: Re: [PATCHv4 11/14] OMAP2+: dmtimer: convert to platform devices > > On 11/20/2010 3:39 AM, Tarun Kanti DebBarma wrote: > > From: Thara Gopinath > > > > Add routines to converts dmtimers to platform devices. The device data > > is obtained from hwmod database of respective platform and is registered > > to device model after successful binding to driver. It also provides > > provision to access timers during early boot when pm_runtime framework > > is not completely up and running. > > > > Signed-off-by: Thara Gopinath > > Signed-off-by: Tarun Kanti DebBarma > > [p-bas...@ti.com: pm_runtime logic] > > Signed-off-by: Partha Basak > > --- > > arch/arm/mach-omap2/Makefile |2 +- > > arch/arm/mach-omap2/dmtimer.c | 296 > + > > 2 files changed, 297 insertions(+), 1 deletions(-) > > create mode 100644 arch/arm/mach-omap2/dmtimer.c > > > > diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile > > index ce7b1f0..148f4d7 100644 > > --- a/arch/arm/mach-omap2/Makefile > > +++ b/arch/arm/mach-omap2/Makefile > > @@ -4,7 +4,7 @@ > > > > # Common support > > obj-y := id.o io.o control.o mux.o devices.o serial.o gpmc.o timer- > gp.o pm.o \ > > -common.o > > +common.o dmtimer.o > > > > omap-2-3-common = irq.o sdrc.o prm2xxx_3xxx.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 100644 > > index 000..b5951b1 > > --- /dev/null > > +++ b/arch/arm/mach-omap2/dmtimer.c > > @@ -0,0 +1,296 @@ > > +/** > > + * OMAP2PLUS Dual-Mode Timers - platform device registration > > + * > > + * Contains first level initialization routines which extracts timers > > + * information from hwmod database and registers with linux device > model. > > + * It also has low level function to chnage the timer input clock > source. > > typo. Will change. > > > + * > > + * Copyright (C) 2010 Texas Instruments Incorporated - > http://www.ti.com/ > > + * Tarun Kanti DebBarma > > + * > > + * Copyright (C) 2010 Texas Instruments Incorporated > > + * Thara Gopinath > > + * > > + * 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. > > + * > > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any > > + * kind, whether express or implied; without even the implied warranty > > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + */ > > +#undef DEBUG > > I guess, that undef should not be there? Will remove. > > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#include > > Do you still need that? It have been already removed. Thanks. > > > +#include > > +#include > > Why do you need powerdomain in that file? This is also removed. After code optimization and cleanup this got missed. > > > +#include > > +#include > > +#include > > + > > +static int early_timer_count __initdata = 1; > > + > > +/** > > + * omap2_dm_timer_set_src - change the timer input clock source > > + * @pdev: timer platform device pointer > > + * @timer_clk: current clock source > > + * @source:array index of parent clock source > > + */ > > +static int omap2_dm_timer_set_src(struct platform_device *pdev, int > source) > > +{ > > + int ret; > > + struct dmtimer_platform_data *pdata = pdev->dev.platform_data; > > + struct clk *fclk = clk_get(&pdev->dev, "fck"); > > + struct clk *new_fclk; > > + char *fclk_name = "32k_ck"; /* default name */ > > + > > + switch(source) { > > + case OMAP_TIMER_SRC_SYS_CLK: > > + fclk_name = "sys_ck"; > > + break; > > + > > + case OMAP_TIMER_SRC_32_KHZ: > > + fclk_name = "32k_ck"; > > + break; > > + > > + case OMAP_TIMER_SRC_EXT_CLK: > > + if (pdata->
Re: [PATCHv4 11/14] OMAP2+: dmtimer: convert to platform devices
On 11/20/2010 3:39 AM, Tarun Kanti DebBarma wrote: From: Thara Gopinath Add routines to converts dmtimers to platform devices. The device data is obtained from hwmod database of respective platform and is registered to device model after successful binding to driver. It also provides provision to access timers during early boot when pm_runtime framework is not completely up and running. Signed-off-by: Thara Gopinath Signed-off-by: Tarun Kanti DebBarma [p-bas...@ti.com: pm_runtime logic] Signed-off-by: Partha Basak --- arch/arm/mach-omap2/Makefile |2 +- arch/arm/mach-omap2/dmtimer.c | 296 + 2 files changed, 297 insertions(+), 1 deletions(-) create mode 100644 arch/arm/mach-omap2/dmtimer.c diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile index ce7b1f0..148f4d7 100644 --- a/arch/arm/mach-omap2/Makefile +++ b/arch/arm/mach-omap2/Makefile @@ -4,7 +4,7 @@ # Common support obj-y := id.o io.o control.o mux.o devices.o serial.o gpmc.o timer-gp.o pm.o \ -common.o +common.o dmtimer.o omap-2-3-common = irq.o sdrc.o prm2xxx_3xxx.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 100644 index 000..b5951b1 --- /dev/null +++ b/arch/arm/mach-omap2/dmtimer.c @@ -0,0 +1,296 @@ +/** + * OMAP2PLUS Dual-Mode Timers - platform device registration + * + * Contains first level initialization routines which extracts timers + * information from hwmod database and registers with linux device model. + * It also has low level function to chnage the timer input clock source. typo. + * + * Copyright (C) 2010 Texas Instruments Incorporated - http://www.ti.com/ + * Tarun Kanti DebBarma + * + * Copyright (C) 2010 Texas Instruments Incorporated + * Thara Gopinath + * + * 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. + * + * This program is distributed "as is" WITHOUT ANY WARRANTY of any + * kind, whether express or implied; without even the implied warranty + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ +#undef DEBUG I guess, that undef should not be there? + +#include +#include +#include +#include +#include + +#include Do you still need that? +#include +#include Why do you need powerdomain in that file? +#include +#include +#include + +static int early_timer_count __initdata = 1; + +/** + * omap2_dm_timer_set_src - change the timer input clock source + * @pdev: timer platform device pointer + * @timer_clk: current clock source + * @source:array index of parent clock source + */ +static int omap2_dm_timer_set_src(struct platform_device *pdev, int source) +{ + int ret; + struct dmtimer_platform_data *pdata = pdev->dev.platform_data; + struct clk *fclk = clk_get(&pdev->dev, "fck"); + struct clk *new_fclk; + char *fclk_name = "32k_ck"; /* default name */ + + switch(source) { + case OMAP_TIMER_SRC_SYS_CLK: + fclk_name = "sys_ck"; + break; + + case OMAP_TIMER_SRC_32_KHZ: + fclk_name = "32k_ck"; + break; + + case OMAP_TIMER_SRC_EXT_CLK: + if (pdata->timer_ip_type == OMAP_TIMER_IP_VERSION_1) { + fclk_name = "alt_ck"; + break; + } + dev_dbg(&pdev->dev, "%s:%d: invalid clk src.\n", + __func__, __LINE__); + return -EINVAL; + } Do you really have to maintain the source enum? Cannot you just pass the char* to this API? It will avoid that code, and make that API much more flexible if we have to add extra clock source in the future. If the clock does not exist in a particular Soc, the clk_get will fail and that's all you have to know. + + + if (IS_ERR_OR_NULL(fclk)) { + dev_dbg(&pdev->dev, "%s:%d: clk_get() FAILED\n", + __func__, __LINE__); + return -EINVAL; + } + + new_fclk = clk_get(&pdev->dev, fclk_name); + if (IS_ERR_OR_NULL(new_fclk)) { + dev_dbg(&pdev->dev, "%s:%d: clk_get() %s FAILED\n", + __func__, __LINE__, fclk_name); + clk_put(fclk); + return -EINVAL; + } + + ret = clk_set_parent(fclk, new_fclk); + if (IS_ERR_VALUE(ret)) { + dev_dbg(&pdev->dev, "%s:clk_set_parent() to %s FAILED\n", + __func__, fclk_name); + ret = -EINVAL; + } + + clk_put(new_fclk); + clk_put(fclk); + + return ret; +} + +struct omap_device_pm_latency omap2_dmtimer_latency[] = { + { + .deactivate_func = omap_device_idle_hwm
Re: [PATCHv4 11/14] OMAP2+: dmtimer: convert to platform devices
On Mon, Nov 22, 2010 at 14:30, DebBarma, Tarun Kanti wrote: >> -Original Message- >> From: Varadarajan, Charulatha >> Sent: Monday, November 22, 2010 1:55 PM >> To: DebBarma, Tarun Kanti >> Cc: linux-omap@vger.kernel.org; Gopinath, Thara; Basak, Partha >> Subject: Re: [PATCHv4 11/14] OMAP2+: dmtimer: convert to platform devices >> >> <> >> >> > +/** >> > + * omap_timer_init - top level timer device initialization >> > + * >> > + * uses dedicated hwmod api to parse through hwmod database for >> > + * given class names and then build and register the timer device. >> > + */ >> > +static int __init omap2_dmtimer_device_init(void) >> > +{ >> > + int ret = omap_hwmod_for_each_by_class("timer", >> omap2_timer_init, NULL); >> > + >> > + if (unlikely(ret)) >> > + pr_debug("%s: device registration FAILED\n", __func__); >> > + >> > + return ret; >> > +} >> > +arch_initcall(omap2_dmtimer_device_init); >> >> While introducing this, calls to omap_dm_timer_init() should be >> removed. Else, the init >> will happen twice for dmtimer devices > This function is called only when the switch-over takes place. > That is why you still see the omap_dm_timer_init(). > So this is more to do with organizing the patch. Exactly. Please take care of this. > I could have introduced this init code as part of platform driver > Swith-over. > > -- > Tarun > >> >> > -- >> > 1.6.0.4 >> > > -- 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: [PATCHv4 11/14] OMAP2+: dmtimer: convert to platform devices
> -Original Message- > From: Varadarajan, Charulatha > Sent: Monday, November 22, 2010 1:55 PM > To: DebBarma, Tarun Kanti > Cc: linux-omap@vger.kernel.org; Gopinath, Thara; Basak, Partha > Subject: Re: [PATCHv4 11/14] OMAP2+: dmtimer: convert to platform devices > > <> > > > +/** > > + * omap_timer_init - top level timer device initialization > > + * > > + * uses dedicated hwmod api to parse through hwmod database for > > + * given class names and then build and register the timer device. > > + */ > > +static int __init omap2_dmtimer_device_init(void) > > +{ > > + int ret = omap_hwmod_for_each_by_class("timer", > omap2_timer_init, NULL); > > + > > + if (unlikely(ret)) > > + pr_debug("%s: device registration FAILED\n", __func__); > > + > > + return ret; > > +} > > +arch_initcall(omap2_dmtimer_device_init); > > While introducing this, calls to omap_dm_timer_init() should be > removed. Else, the init > will happen twice for dmtimer devices This function is called only when the switch-over takes place. That is why you still see the omap_dm_timer_init(). So this is more to do with organizing the patch. I could have introduced this init code as part of platform driver Swith-over. -- Tarun > > > -- > > 1.6.0.4 > > -- 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: [PATCHv4 11/14] OMAP2+: dmtimer: convert to platform devices
<> > +/** > + * omap_timer_init - top level timer device initialization > + * > + * uses dedicated hwmod api to parse through hwmod database for > + * given class names and then build and register the timer device. > + */ > +static int __init omap2_dmtimer_device_init(void) > +{ > + int ret = omap_hwmod_for_each_by_class("timer", omap2_timer_init, > NULL); > + > + if (unlikely(ret)) > + pr_debug("%s: device registration FAILED\n", __func__); > + > + return ret; > +} > +arch_initcall(omap2_dmtimer_device_init); While introducing this, calls to omap_dm_timer_init() should be removed. Else, the init will happen twice for dmtimer devices > -- > 1.6.0.4 > -- 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: [PATCHv4 11/14] OMAP2+: dmtimer: convert to platform devices
> -Original Message- > From: Varadarajan, Charulatha > Sent: Monday, November 22, 2010 12:04 PM > To: DebBarma, Tarun Kanti > Cc: linux-omap@vger.kernel.org; Gopinath, Thara; Basak, Partha > Subject: Re: [PATCHv4 11/14] OMAP2+: dmtimer: convert to platform devices > > On Sat, Nov 20, 2010 at 08:09, Tarun Kanti DebBarma > wrote: > > From: Thara Gopinath > > > > Add routines to converts dmtimers to platform devices. The device data > > is obtained from hwmod database of respective platform and is registered > > to device model after successful binding to driver. It also provides > > provision to access timers during early boot when pm_runtime framework > > is not completely up and running. > > > > Signed-off-by: Thara Gopinath > > Signed-off-by: Tarun Kanti DebBarma > > [p-bas...@ti.com: pm_runtime logic] > > Where is runtime logic in this patch? The patch description says > "pm_runtime > framework is not completely up and running" I have put in the wrong place. > > > Signed-off-by: Partha Basak > > --- > > arch/arm/mach-omap2/Makefile |2 +- > > arch/arm/mach-omap2/dmtimer.c | 296 > + > > 2 files changed, 297 insertions(+), 1 deletions(-) > > create mode 100644 arch/arm/mach-omap2/dmtimer.c > > > > diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile > > index ce7b1f0..148f4d7 100644 > > --- a/arch/arm/mach-omap2/Makefile > > +++ b/arch/arm/mach-omap2/Makefile > > @@ -4,7 +4,7 @@ > > > > # Common support > > obj-y := id.o io.o control.o mux.o devices.o serial.o gpmc.o timer-gp.o > pm.o \ > > -common.o > > +common.o dmtimer.o > > > > omap-2-3-common= irq.o sdrc.o > prm2xxx_3xxx.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 100644 > > index 000..b5951b1 > > --- /dev/null > > +++ b/arch/arm/mach-omap2/dmtimer.c > > @@ -0,0 +1,296 @@ > > +/** > > + * OMAP2PLUS Dual-Mode Timers - platform device registration > > + * > > + * Contains first level initialization routines which extracts timers > > + * information from hwmod database and registers with linux device > model. > > + * It also has low level function to chnage the timer input clock > source. > > + * > > + * Copyright (C) 2010 Texas Instruments Incorporated - > http://www.ti.com/ > > + * Tarun Kanti DebBarma > > + * > > + * Copyright (C) 2010 Texas Instruments Incorporated > > + * Thara Gopinath > > Don't repeat the same copyright information twice: > "Copyright (C) 2010 Texas Instruments Incorporated" > > > + * > > + * 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. > > + * > > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any > > + * kind, whether express or implied; without even the implied warranty > > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + */ > > +#undef DEBUG > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > Why is "linux/pm_runtime.h" required in this file? I don't find any use > here. > Also check if "plat/powerdomain.h" is required. Thanks, this is a clean up problem. Lot of implementations were optimized. But missed to cleanup the headers which include powerdomain.h, delay.h and Irqs.h > > > + > > +static int early_timer_count __initdata = 1; > > + > > +/** > > + * omap2_dm_timer_set_src - change the timer input clock source > > + * @pdev: timer platform device pointer > > + * @timer_clk: current clock source > > + * @source:array index of parent clock source > > + */ > > +static int omap2_dm_timer_set_src(struct platform_device *pdev, int > source) > > +{ > > + int ret; > > + struct dmtimer_platform_data *pdata = pdev->dev.platform_data; > > + struct clk *fclk = clk_get(&pdev->dev, "fck"); > > + struct clk *new_fclk; > > + char *fclk_name = "32k_ck"; /* def
Re: [PATCHv4 11/14] OMAP2+: dmtimer: convert to platform devices
On Sat, Nov 20, 2010 at 08:09, Tarun Kanti DebBarma wrote: > From: Thara Gopinath > > Add routines to converts dmtimers to platform devices. The device data > is obtained from hwmod database of respective platform and is registered > to device model after successful binding to driver. It also provides > provision to access timers during early boot when pm_runtime framework > is not completely up and running. > > Signed-off-by: Thara Gopinath > Signed-off-by: Tarun Kanti DebBarma > [p-bas...@ti.com: pm_runtime logic] Where is runtime logic in this patch? The patch description says "pm_runtime framework is not completely up and running" > Signed-off-by: Partha Basak > --- > arch/arm/mach-omap2/Makefile | 2 +- > arch/arm/mach-omap2/dmtimer.c | 296 > + > 2 files changed, 297 insertions(+), 1 deletions(-) > create mode 100644 arch/arm/mach-omap2/dmtimer.c > > diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile > index ce7b1f0..148f4d7 100644 > --- a/arch/arm/mach-omap2/Makefile > +++ b/arch/arm/mach-omap2/Makefile > @@ -4,7 +4,7 @@ > > # Common support > obj-y := id.o io.o control.o mux.o devices.o serial.o gpmc.o timer-gp.o pm.o > \ > - common.o > + common.o dmtimer.o > > omap-2-3-common = irq.o sdrc.o prm2xxx_3xxx.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 100644 > index 000..b5951b1 > --- /dev/null > +++ b/arch/arm/mach-omap2/dmtimer.c > @@ -0,0 +1,296 @@ > +/** > + * OMAP2PLUS Dual-Mode Timers - platform device registration > + * > + * Contains first level initialization routines which extracts timers > + * information from hwmod database and registers with linux device model. > + * It also has low level function to chnage the timer input clock source. > + * > + * Copyright (C) 2010 Texas Instruments Incorporated - http://www.ti.com/ > + * Tarun Kanti DebBarma > + * > + * Copyright (C) 2010 Texas Instruments Incorporated > + * Thara Gopinath Don't repeat the same copyright information twice: "Copyright (C) 2010 Texas Instruments Incorporated" > + * > + * 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. > + * > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any > + * kind, whether express or implied; without even the implied warranty > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > +#undef DEBUG > + > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > +#include > +#include > +#include Why is "linux/pm_runtime.h" required in this file? I don't find any use here. Also check if "plat/powerdomain.h" is required. > + > +static int early_timer_count __initdata = 1; > + > +/** > + * omap2_dm_timer_set_src - change the timer input clock source > + * @pdev: timer platform device pointer > + * @timer_clk: current clock source > + * @source: array index of parent clock source > + */ > +static int omap2_dm_timer_set_src(struct platform_device *pdev, int source) > +{ > + int ret; > + struct dmtimer_platform_data *pdata = pdev->dev.platform_data; > + struct clk *fclk = clk_get(&pdev->dev, "fck"); > + struct clk *new_fclk; > + char *fclk_name = "32k_ck"; /* default name */ > + > + switch(source) { > + case OMAP_TIMER_SRC_SYS_CLK: > + fclk_name = "sys_ck"; > + break; > + > + case OMAP_TIMER_SRC_32_KHZ: > + fclk_name = "32k_ck"; > + break; > + > + case OMAP_TIMER_SRC_EXT_CLK: > + if (pdata->timer_ip_type == OMAP_TIMER_IP_VERSION_1) { > + fclk_name = "alt_ck"; > + break; > + } > + dev_dbg(&pdev->dev, "%s:%d: invalid clk src.\n", > + __func__, __LINE__); > + return -EINVAL; > + } > + > + > + if (IS_ERR_OR_NULL(fclk)) { > + dev_dbg(&pdev->dev, "%s:%d: clk_get() FAILED\n", > + __func__, __LINE__); > + return -EINVAL; > + } > + > + new_fclk = clk_get(&pdev->dev, fclk_name); > + if (IS_ERR_OR_NULL(new_fclk)) { > + dev_dbg(&pdev->dev, "%s:%d: clk_get() %s FAILED\n", > + __func__, __LINE__, fclk_name); > + clk_put(fclk); > + return -EINVAL; > + } > + > + ret = clk_set_parent(fclk, new_fclk); > + if (IS_ERR_VALUE(ret)) { > + dev_dbg(&pdev->dev, "%s:clk_set_parent() to %s FAILED\n", > + __func__, fclk_name); > + ret = -EINVAL; > + } > + > + c