> -----Original Message-----
> From: Hilman, Kevin
> Sent: Friday, March 04, 2011 6:32 AM
> To: DebBarma, Tarun Kanti
> Cc: [email protected]; Gopinath, Thara
> Subject: Re: [PATCH v11 4/8] OMAP2+: dmtimer: convert to platform devices
>
> Tarun Kanti DebBarma <[email protected]> writes:
>
> > 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: Tarun Kanti DebBarma <[email protected]>
> > Signed-off-by: Thara Gopinath <[email protected]>
> > Acked-by: Cousson, Benoit <[email protected]>
> > ---
> > arch/arm/mach-omap2/Makefile | 2 +-
> > arch/arm/mach-omap2/dmtimer.c | 199
> +++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 200 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 1c3635d..7e5014b 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 gpio.o dma.o wd_timer.o
> > + common.o gpio.o dma.o wd_timer.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 100644
> > index 0000000..00cebe9
> > --- /dev/null
> > +++ b/arch/arm/mach-omap2/dmtimer.c
> > @@ -0,0 +1,199 @@
> > +/**
> > + * OMAP2+ 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 change the timer input clock
> source.
> > + *
> > + * Copyright (C) 2010 Texas Instruments Incorporated -
> http://www.ti.com/
> > + * Tarun Kanti DebBarma <[email protected]>
> > + * Thara Gopinath <[email protected]>
> > + *
> > + * 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.
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/err.h>
> > +#include <linux/slab.h>
> > +
> > +#include <plat/dmtimer.h>
> > +#include <plat/omap_device.h>
> > +#include <plat/cpu.h>
> > +
> > +/*
> > + * OMAP4 IP revision has different register offsets
> > + * for interrupt registers and functional registers.
> > + */
> > +#define VERSION2_TIMER_WAKEUP_EN_REG_OFFSET 0x14
> > +#define VERSION2_TIMER_STAT_REG_OFFSET 0x10
>
> These should be in the driver along with all the other register offsets.
>
> This device code is already setting the IP rev type, the driver can
> handle the rest.
Ok, I will make the changes.
>
> > +static int early_timer_count __initdata = 1;
>
> dumb question: why does this start at 1?
>
This is related to one of your early comments where this variable is used:
early_platform_driver_probe("earlytimer", early_timer_count, 0);
It was passed as early_timer_count + 1, as static variable starts at 0.
So it is just to avoid this addition of 1.
> > +struct dm_timer_data {
> > + struct omap_device *od;
> > + struct dmtimer_platform_data *pdata;
> > + struct list_head node;
> > +};
> > +
> > +static __initdata LIST_HEAD(dm_timer_data_list);
> > +
> > +/**
> > + * 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_err(&pdev->dev, "%s: %d: invalid clk src.\n",
> > + __func__, __LINE__);
> > + return -EINVAL;
> > + }
> > +
> > + if (IS_ERR_OR_NULL(fclk)) {
> > + dev_err(&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_err(&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_err(&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_hwmods,
> > + .activate_func = omap_device_enable_hwmods,
> > + .flags = OMAP_DEVICE_LATENCY_AUTO_ADJUST,
> > + },
> > +};
> > +
> > +/**
> > + * omap_timer_init - build and register timer device with an
> > + * associated timer hwmod
> > + * @oh: timer hwmod pointer to be used to build timer device
> > + * @user: parameter that can be passed from calling hwmod API
> > + *
> > + * Called by omap_hwmod_for_each_by_class to register each of the timer
> > + * devices present in the system. The number of timer devices is known
> > + * by parsing through the hwmod database for a given class name. At the
> > + * end of function call memory is allocated for timer device and it is
> > + * registered to the framework ready to be proved by the driver.
> > + */
> > +static int __init omap_timer_init(struct omap_hwmod *oh, void *unused)
> > +{
> > + int id;
> > + int ret = 0;
> > + char *name = "omap_timer";
> > + struct dmtimer_platform_data *pdata;
> > + struct omap_device *od;
> > + struct dm_timer_data *timer_data = NULL;
> > +
> > + pr_debug("%s: %s\n", __func__, oh->name);
> > +
> > + pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
>
> Memory leak.
>
> This needs to be free'd after omap_device_build(). The platform_device
> core makes a copy of the pdata (via kmemdup.)
Yes, it was there and I missed it while implementing the double registration
Issue :( Thanks!
>
> > + if (!pdata) {
> > + pr_err("%s: No memory for [%s]\n", __func__, oh->name);
> > + return -ENOMEM;
> > + }
> > +
> > + pdata->is_early_init = 1;
> > + pdata->is_omap16xx = 0;
> > + pdata->dm_timer_reset = NULL;
> > + pdata->set_timer_src = omap2_dm_timer_set_src;
> > +
> > + pdata->timer_ip_type = oh->class->rev;
> > + if (pdata->timer_ip_type == OMAP_TIMER_IP_VERSION_2) {
> > + pdata->func_offset = VERSION2_TIMER_WAKEUP_EN_REG_OFFSET;
> > + pdata->intr_offset = VERSION2_TIMER_STAT_REG_OFFSET;
> > + } else {
> > + pdata->func_offset = 0;
> > + pdata->intr_offset = 0;
> > + }
> > +
> > + /*
> > + * Extract the IDs from name field in hwmod database
> > + * and use the same for constructing ids' for the
> > + * timer devices. In a way, we are avoiding usage of
> > + * static variable witin the function to do the same.
> > + * CAUTION: We have to be careful and make sure the
> > + * name in hwmod database does not change in which case
> > + * we might either make corresponding change here or
> > + * switch back static variable mechanism.
> > + */
> > + sscanf(oh->name, "timer%2d", &id);
> > +
> > + /* do registration of timer12 on GP device only */
> > + if (id == 12 && omap_type() != OMAP2_DEVICE_TYPE_GP)
> > + return ret;
>
> hmm, grumble
>
> Is this true on OMAP4? I thought GPT12 was not accessible on OMAP4 even
> on GP devices.
On OMAP4 hwmod database there is no entry for GPT12.
So, this condition would NOT be applicable to OMAP4 at all as I understand.
Of course, as I see it the comment is not very clear and I can change that.
>
> As I've said a few times before, what we need here is some sort of
> "capabilities" flag for each timer in the hwmod. In addition to this
> "secure-mode only" feature, there are other capabilities such as 1ms
> timers, PWM capable, etc.
I can consider this. However I feel this would take sometime to align on the
Final choice/option of implementation.
So, is the above implementation acceptable for now?
>
> > + od = omap_device_build(name, id, oh, pdata, sizeof(*pdata),
> > + omap2_dmtimer_latency,
> > + ARRAY_SIZE(omap2_dmtimer_latency),
> > + pdata->is_early_init);
> > +
> > + if (IS_ERR(od)) {
> > + pr_err("%s: Can't build omap_device for %s: %s.\n",
> > + __func__, name, oh->name);
> > + ret = -EINVAL;
> > + } else if (pdata->is_early_init)
> > + early_timer_count++;
> > +
> > + timer_data = kzalloc(sizeof(*timer_data), GFP_KERNEL);
> > + if (!timer_data) {
> > + pr_debug("%s: no memory for dm_timer_data\n",
> > + __func__);
> > + return -ENOMEM;
> > + }
> > +
> > + /* store od and pdata to be used later during normal initialization
> */
> > + timer_data->od = od;
> > + timer_data->pdata = pdata;
> > + list_add_tail(&timer_data->node, &dm_timer_data_list);
> > +
> > + return ret;
> > +}
>
> Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
