Message ID | 20121016013213.21844.20016.stgit@dusk.lan (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Hi, On Mon, Oct 15, 2012 at 07:32:33PM -0600, Paul Walmsley wrote: > The OMAP watchdog timer driver directly calls a function exported by > code in arch/arm/mach-omap2. This is not good; it tightly couples > this driver to the mach-omap2 integration code. Instead, add a > temporary platform_data function pointer to abstract this function > call. A subsequent patch will convert the watchdog driver to use this > function pointer. Why a function is needed? Reset cause won't change until the next reset, so it should be enough to read it once during the init and store it into the platform data. A.
Terve, On Thu, 25 Oct 2012, Aaro Koskinen wrote: > On Mon, Oct 15, 2012 at 07:32:33PM -0600, Paul Walmsley wrote: > > The OMAP watchdog timer driver directly calls a function exported by > > code in arch/arm/mach-omap2. This is not good; it tightly couples > > this driver to the mach-omap2 integration code. Instead, add a > > temporary platform_data function pointer to abstract this function > > call. A subsequent patch will convert the watchdog driver to use this > > function pointer. > > Why a function is needed? Reset cause won't change until the next reset, > so it should be enough to read it once during the init and store it into > the platform data. Once the PRM/CM drivers and DT conversion is complete, this driver won't have any platform_data. There won't be any watchdog driver initialization code in arch/arm/mach-omap2. At that point in time, the driver will just call a function from the PRM driver that provides the reset source data. As you say, the watchdog can certainly do this from its probe code; it would be more efficient. Just didn't want to make that change now; seems like it would be best done as a separate optimization patch. - Paul
* Paul Walmsley <paul@pwsan.com> [121025 11:53]: > Terve, > > On Thu, 25 Oct 2012, Aaro Koskinen wrote: > > > On Mon, Oct 15, 2012 at 07:32:33PM -0600, Paul Walmsley wrote: > > > The OMAP watchdog timer driver directly calls a function exported by > > > code in arch/arm/mach-omap2. This is not good; it tightly couples > > > this driver to the mach-omap2 integration code. Instead, add a > > > temporary platform_data function pointer to abstract this function > > > call. A subsequent patch will convert the watchdog driver to use this > > > function pointer. > > > > Why a function is needed? Reset cause won't change until the next reset, > > so it should be enough to read it once during the init and store it into > > the platform data. > > Once the PRM/CM drivers and DT conversion is complete, this driver won't > have any platform_data. There won't be any watchdog driver initialization > code in arch/arm/mach-omap2. At that point in time, the driver will just > call a function from the PRM driver that provides the reset source data. > > As you say, the watchdog can certainly do this from its probe code; it > would be more efficient. Just didn't want to make that change now; seems > like it would be best done as a separate optimization patch. Ideally we'd have some Linux generic function to implement in the PRM/CM drivers for getting the bootreason. But I guess we don't? Regards, Tony
On Thu, 25 Oct 2012, Tony Lindgren wrote: > Ideally we'd have some Linux generic function to implement in the PRM/CM > drivers for getting the bootreason. But I guess we don't? Do you know of one, apart from WDIOC_GETBOOTSTATUS? - Paul
* Paul Walmsley <paul@pwsan.com> [121025 12:11]: > On Thu, 25 Oct 2012, Tony Lindgren wrote: > > > Ideally we'd have some Linux generic function to implement in the PRM/CM > > drivers for getting the bootreason. But I guess we don't? > > Do you know of one, apart from WDIOC_GETBOOTSTATUS? I wonder if we can now with multiple watchdogs supported to have also PRM/CM implement omap_prcm_wdt_ioctl() that just handles WDIOC_GETBOOTSTATUS and then just remove that handling from the other omap watchdog drivers? Regards, Tony
On Thu, 25 Oct 2012, Tony Lindgren wrote: > I wonder if we can now with multiple watchdogs supported to > have also PRM/CM implement omap_prcm_wdt_ioctl() that just > handles WDIOC_GETBOOTSTATUS and then just remove that handling > from the other omap watchdog drivers? The watchdog ioctl code needs access to internal watchdog state and functions, for example WDIOC_KEEPALIVE and WDIOC_SETTIMEOUT. How would the approach you're thinking of implement those operations? - Paul
* Paul Walmsley <paul@pwsan.com> [121025 12:32]: > On Thu, 25 Oct 2012, Tony Lindgren wrote: > > > I wonder if we can now with multiple watchdogs supported to > > have also PRM/CM implement omap_prcm_wdt_ioctl() that just > > handles WDIOC_GETBOOTSTATUS and then just remove that handling > > from the other omap watchdog drivers? > > The watchdog ioctl code needs access to internal watchdog state and > functions, for example WDIOC_KEEPALIVE and WDIOC_SETTIMEOUT. How would > the approach you're thinking of implement those operations? I have not looked how the watchdog subsystem handles multiple watchdogs, but.. Can't we have the omap watchdog and twl watchdog return -ENOTSUPP for WDIOC_GETBOOTSTATUS and have the watchdog core fail over to the third wathcdog omap_prcm_wdt_ioctl() that only implements WDIOC_GETBOOTSTATUS? Or something like that. Regards, Tony
* Tony Lindgren <tony@atomide.com> [121025 12:35]: > * Paul Walmsley <paul@pwsan.com> [121025 12:32]: > > On Thu, 25 Oct 2012, Tony Lindgren wrote: > > > > > I wonder if we can now with multiple watchdogs supported to > > > have also PRM/CM implement omap_prcm_wdt_ioctl() that just > > > handles WDIOC_GETBOOTSTATUS and then just remove that handling > > > from the other omap watchdog drivers? > > > > The watchdog ioctl code needs access to internal watchdog state and > > functions, for example WDIOC_KEEPALIVE and WDIOC_SETTIMEOUT. How would > > the approach you're thinking of implement those operations? > > I have not looked how the watchdog subsystem handles multiple > watchdogs, but.. Can't we have the omap watchdog and twl watchdog > return -ENOTSUPP for WDIOC_GETBOOTSTATUS and have the watchdog > core fail over to the third wathcdog omap_prcm_wdt_ioctl() that > only implements WDIOC_GETBOOTSTATUS? Or something like that. After poking around a bit, probably the way to do this would be to have watchdog core bootstatus in addition to the bootstatus in struct watchdog_device? Then the omap_prcm watchdog could just initialize the watchdog core bootstatus, and the other omap watchdog drivers would just return the watchdog core bootstatus. Not that it's related to this patchset, just trying to figure out how it could be done in a generic way :) Regards, Tony
On Thu, 25 Oct 2012, Tony Lindgren wrote: > I have not looked how the watchdog subsystem handles multiple > watchdogs, but.. In the new watchdog core code, each watchdog driver gets a separate /dev/watchdog* character device. The ioctls are called on those device nodes. [ As an aside, neither the OMAP watchdog driver, nor the TWL watchdog driver have been updated to use the new watchdog core code. So they both can't be loaded at the same time until one or both are fixed. ] > Can't we have the omap watchdog and twl watchdog return -ENOTSUPP for > WDIOC_GETBOOTSTATUS and have the watchdog core fail over to the third > wathcdog omap_prcm_wdt_ioctl() that only implements WDIOC_GETBOOTSTATUS? > Or something like that. Sounds like a question better asked of Alan Cox and Wim, who wrote the watchdog core code. Two other observations: - It's possible that two different watchdogs could report different boot reasons. The TWL might store its own watchdog boot reason which could be different from what's reported via the OMAP PRM. For example, the TWL can record a thermal shutdown reset event, STS_BOOT.TS, which wouldn't be reflected in the PRM reset source data, which would just see some kind of external reset or power-off event. - The PRM doesn't contain a hardware watchdog, so not sure it makes sense to create a watchdog driver for it. - Paul
Hi, On Thu, Oct 25, 2012 at 07:57:31PM +0000, Paul Walmsley wrote: > [ As an aside, neither the OMAP watchdog driver, nor the TWL watchdog > driver have been updated to use the new watchdog core code. So they both > can't be loaded at the same time until one or both are fixed. ] FYI, this is being done currently: - OMAP wdt: https://lkml.org/lkml/2012/10/10/402 - TWL wdt: http://marc.info/?l=linux-omap&m=134734329319385&w=2 A.
On Thu, 25 Oct 2012, Aaro Koskinen wrote: > On Thu, Oct 25, 2012 at 07:57:31PM +0000, Paul Walmsley wrote: > > [ As an aside, neither the OMAP watchdog driver, nor the TWL watchdog > > driver have been updated to use the new watchdog core code. So they both > > can't be loaded at the same time until one or both are fixed. ] > > FYI, this is being done currently: > > - OMAP wdt: https://lkml.org/lkml/2012/10/10/402 > - TWL wdt: http://marc.info/?l=linux-omap&m=134734329319385&w=2 Thanks, that's good news. - Paul
diff --git a/arch/arm/mach-omap1/devices.c b/arch/arm/mach-omap1/devices.c index d3fec92..c3408e7 100644 --- a/arch/arm/mach-omap1/devices.c +++ b/arch/arm/mach-omap1/devices.c @@ -17,6 +17,8 @@ #include <linux/platform_device.h> #include <linux/spi/spi.h> +#include <linux/platform_data/omap-wd-timer.h> + #include <asm/mach/map.h> #include <plat/tc.h> @@ -439,18 +441,31 @@ static struct resource wdt_resources[] = { }; static struct platform_device omap_wdt_device = { - .name = "omap_wdt", - .id = -1, + .name = "omap_wdt", + .id = -1, .num_resources = ARRAY_SIZE(wdt_resources), .resource = wdt_resources, }; static int __init omap_init_wdt(void) { + struct omap_wd_timer_platform_data pdata; + int ret; + if (!cpu_is_omap16xx()) return -ENODEV; - return platform_device_register(&omap_wdt_device); + pdata.read_reset_sources = omap1_read_reset_sources; + + ret = platform_device_register(&omap_wdt_device); + if (!ret) { + ret = platform_device_add_data(&omap_wdt_device, &pdata, + sizeof(pdata)); + if (ret) + platform_device_del(&omap_wdt_device); + } + + return ret; } subsys_initcall(omap_init_wdt); #endif diff --git a/arch/arm/mach-omap2/devices.c b/arch/arm/mach-omap2/devices.c index c8c2117..2ab2c99 100644 --- a/arch/arm/mach-omap2/devices.c +++ b/arch/arm/mach-omap2/devices.c @@ -644,29 +644,3 @@ static int __init omap2_init_devices(void) return 0; } arch_initcall(omap2_init_devices); - -#if defined(CONFIG_OMAP_WATCHDOG) || defined(CONFIG_OMAP_WATCHDOG_MODULE) -static int __init omap_init_wdt(void) -{ - int id = -1; - struct platform_device *pdev; - struct omap_hwmod *oh; - char *oh_name = "wd_timer2"; - char *dev_name = "omap_wdt"; - - if (!cpu_class_is_omap2() || of_have_populated_dt()) - return 0; - - oh = omap_hwmod_lookup(oh_name); - if (!oh) { - pr_err("Could not look up wd_timer%d hwmod\n", id); - return -EINVAL; - } - - pdev = omap_device_build(dev_name, id, oh, NULL, 0, NULL, 0, 0); - WARN(IS_ERR(pdev), "Can't build omap_device for %s:%s.\n", - dev_name, oh->name); - return 0; -} -subsys_initcall(omap_init_wdt); -#endif diff --git a/arch/arm/mach-omap2/wd_timer.c b/arch/arm/mach-omap2/wd_timer.c index b2f1c67..00ef54c 100644 --- a/arch/arm/mach-omap2/wd_timer.c +++ b/arch/arm/mach-omap2/wd_timer.c @@ -11,10 +11,14 @@ #include <linux/io.h> #include <linux/err.h> +#include <linux/platform_data/omap-wd-timer.h> + #include <plat/omap_hwmod.h> +#include <plat/omap_device.h> #include "wd_timer.h" #include "common.h" +#include "prm.h" /* * In order to avoid any assumptions from bootloader regarding WDT @@ -99,3 +103,32 @@ int omap2_wd_timer_reset(struct omap_hwmod *oh) return (c == MAX_MODULE_SOFTRESET_WAIT) ? -ETIMEDOUT : omap2_wd_timer_disable(oh); } + +static int __init omap_init_wdt(void) +{ + int id = -1; + struct platform_device *pdev; + struct omap_hwmod *oh; + char *oh_name = "wd_timer2"; + char *dev_name = "omap_wdt"; + struct omap_wd_timer_platform_data pdata; + + if (!cpu_class_is_omap2()) + return 0; + + oh = omap_hwmod_lookup(oh_name); + if (!oh) { + pr_err("Could not look up wd_timer%d hwmod\n", id); + return -EINVAL; + } + + pdata.read_reset_sources = prm_read_reset_sources; + + pdev = omap_device_build(dev_name, id, oh, &pdata, + sizeof(struct omap_wd_timer_platform_data), + NULL, 0, 0); + WARN(IS_ERR(pdev), "Can't build omap_device for %s:%s.\n", + dev_name, oh->name); + return 0; +} +subsys_initcall(omap_init_wdt); diff --git a/include/linux/platform_data/omap-wd-timer.h b/include/linux/platform_data/omap-wd-timer.h new file mode 100644 index 0000000..d75f5f8 --- /dev/null +++ b/include/linux/platform_data/omap-wd-timer.h @@ -0,0 +1,38 @@ +/* + * OMAP2+ WDTIMER-specific function prototypes + * + * Copyright (C) 2012 Texas Instruments, Inc. + * Paul Walmsley + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + */ + +#ifndef __LINUX_PLATFORM_DATA_OMAP_WD_TIMER_H +#define __LINUX_PLATFORM_DATA_OMAP_WD_TIMER_H + +#include <linux/types.h> + +/* + * Standardized OMAP reset source bits + * + * This is a subset of the ones listed in arch/arm/mach-omap2/prm.h + * and are the only ones needed in the watchdog driver. + */ +#define OMAP_MPU_WD_RST_SRC_ID_SHIFT 3 + +/** + * struct omap_wd_timer_platform_data - WDTIMER integration to the host SoC + * @read_reset_sources - fn ptr for the SoC to indicate the last reset cause + * + * The function pointed to by @read_reset_sources must return its data + * in a standard format - search for RST_SRC_ID_SHIFT in + * arch/arm/mach-omap2 + */ +struct omap_wd_timer_platform_data { + u32 (*read_reset_sources)(void); +}; + +#endif
The OMAP watchdog timer driver directly calls a function exported by code in arch/arm/mach-omap2. This is not good; it tightly couples this driver to the mach-omap2 integration code. Instead, add a temporary platform_data function pointer to abstract this function call. A subsequent patch will convert the watchdog driver to use this function pointer. This patch also moves the device creation code out of arch/arm/mach-omap2/devices.c and into arch/arm/mach-omap2/wd_timer.c. This is another step towards the removal of arch/arm/mach-omap2/devices.c. Signed-off-by: Paul Walmsley <paul@pwsan.com> Cc: Wim Van Sebroeck <wim@iguana.be> --- arch/arm/mach-omap1/devices.c | 21 +++++++++++++-- arch/arm/mach-omap2/devices.c | 26 ------------------ arch/arm/mach-omap2/wd_timer.c | 33 +++++++++++++++++++++++ include/linux/platform_data/omap-wd-timer.h | 38 +++++++++++++++++++++++++++ 4 files changed, 89 insertions(+), 29 deletions(-) create mode 100644 include/linux/platform_data/omap-wd-timer.h