| Message ID | 1432197156-16947-7-git-send-email-fu.wei@linaro.org |
|---|---|
| State | New |
| Headers | show |
Hi Guenter, Thanks for review :-) On 21 May 2015 at 18:34, Guenter Roeck <linux@roeck-us.net> wrote: > On 05/21/2015 01:32 AM, fu.wei@linaro.org wrote: >> >> From: Fu Wei <fu.wei@linaro.org> >> >> This driver bases on linux kernel watchdog framework, and >> use "pretimeout" in the framework. It supports getting timeout and >> pretimeout from parameter and FDT at the driver init stage. >> In first timeout(WS0), the interrupt routine run panic to save >> system context. >> >> Signed-off-by: Fu Wei <fu.wei@linaro.org> >> --- >> drivers/watchdog/Kconfig | 12 ++ >> drivers/watchdog/Makefile | 1 + >> drivers/watchdog/sbsa_gwdt.c | 476 >> +++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 489 insertions(+) >> create mode 100644 drivers/watchdog/sbsa_gwdt.c >> >> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig >> index e5e7c55..25a0df1 100644 >> --- a/drivers/watchdog/Kconfig >> +++ b/drivers/watchdog/Kconfig >> @@ -152,6 +152,18 @@ config ARM_SP805_WATCHDOG >> ARM Primecell SP805 Watchdog timer. This will reboot your system >> when >> the timeout is reached. >> >> +config ARM_SBSA_WATCHDOG >> + tristate "ARM SBSA Generic Watchdog" >> + depends on ARM || ARM64 || COMPILE_TEST >> + depends on ARM_ARCH_TIMER >> + select WATCHDOG_CORE >> + help >> + ARM SBSA Generic Watchdog timer. This has two Watchdog Signals >> + (WS0/WS1), will trigger a warning interrupt(do panic) at the >> first >> + timeout(WS0); will reboot your system when the second >> timeout(WS1) >> + is reached. > > > I think it would be better to keep the WS0/WS1 out of the help text. > It is an implementation detail, and no user will be able to make sense of > it. I don't mind to delete it , but those are in SBSA spec, is that an implementation detail ? Sorry , just try to re-confirm it :-) > > >> + More details: DEN0029B - Server Base System Architecture (SBSA) >> + >> config AT91RM9200_WATCHDOG >> tristate "AT91RM9200 watchdog" >> depends on SOC_AT91RM9200 && MFD_SYSCON >> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile >> index 5c19294..471f1b7c 100644 >> --- a/drivers/watchdog/Makefile >> +++ b/drivers/watchdog/Makefile >> @@ -30,6 +30,7 @@ obj-$(CONFIG_USBPCWATCHDOG) += pcwd_usb.o >> >> # ARM Architecture >> obj-$(CONFIG_ARM_SP805_WATCHDOG) += sp805_wdt.o >> +obj-$(CONFIG_ARM_SBSA_WATCHDOG) += sbsa_gwdt.o >> obj-$(CONFIG_AT91RM9200_WATCHDOG) += at91rm9200_wdt.o >> obj-$(CONFIG_AT91SAM9X_WATCHDOG) += at91sam9_wdt.o >> obj-$(CONFIG_CADENCE_WATCHDOG) += cadence_wdt.o >> diff --git a/drivers/watchdog/sbsa_gwdt.c b/drivers/watchdog/sbsa_gwdt.c >> new file mode 100644 >> index 0000000..4ebe7c3 >> --- /dev/null >> +++ b/drivers/watchdog/sbsa_gwdt.c >> @@ -0,0 +1,476 @@ >> +/* >> + * SBSA(Server Base System Architecture) Generic Watchdog driver >> + * >> + * Copyright (c) 2015, Linaro Ltd. >> + * Author: Fu Wei <fu.wei@linaro.org> >> + * Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License 2 as published >> + * by the Free Software Foundation. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + * >> + * Note: This SBSA Generic watchdog driver is compatible with >> + * the pretimeout concept of Linux kernel. >> + * The timeout and pretimeout are set by the different REGs. >> + * The first watch period is set by writing WCV directly, >> + * that can support more than 10s timeout at the maximum >> + * system counter frequency. >> + * The second watch period is set by WOR(32bit) which will be >> loaded >> + * automatically by hardware, when WS0 is triggered. >> + * This gives a maximum watch period of around 10s at the maximum >> + * system counter frequency. >> + * The System Counter shall run at maximum of 400MHz. >> + * More details: DEN0029B - Server Base System Architecture (SBSA) >> + * >> + * Kernel/API: P---------| pretimeout >> + * |-------------------------------T timeout >> + * SBSA GWDT: P--WOR---WS1 pretimeout >> + * |-------WCV----------WS0~~~~~~~~T timeout >> + */ >> + >> +#include <asm/arch_timer.h> >> + >> +#include <linux/acpi.h> >> +#include <linux/io.h> >> +#include <linux/interrupt.h> >> +#include <linux/module.h> >> +#include <linux/moduleparam.h> >> +#include <linux/of.h> >> +#include <linux/of_device.h> >> +#include <linux/platform_device.h> >> +#include <linux/uaccess.h> >> +#include <linux/watchdog.h> >> + >> +/* SBSA Generic Watchdog register definitions */ >> +/* refresh frame */ >> +#define SBSA_GWDT_WRR 0x000 >> + >> +/* control frame */ >> +#define SBSA_GWDT_WCS 0x000 >> +#define SBSA_GWDT_WOR 0x008 >> +#define SBSA_GWDT_WCV_LO 0x010 >> +#define SBSA_GWDT_WCV_HI 0x014 >> + >> +/* refresh/control frame */ >> +#define SBSA_GWDT_W_IIDR 0xfcc >> +#define SBSA_GWDT_IDR 0xfd0 >> + >> +/* Watchdog Control and Status Register */ >> +#define SBSA_GWDT_WCS_EN BIT(0) >> +#define SBSA_GWDT_WCS_WS0 BIT(1) >> +#define SBSA_GWDT_WCS_WS1 BIT(2) >> + >> +/** >> + * struct sbsa_gwdt - Internal representation of the SBSA GWDT >> + * @wdd: kernel watchdog_device structure >> + * @clk: store the System Counter clock frequency, in Hz. >> + * @refresh_base: Virtual address of the watchdog refresh frame >> + * @control_base: Virtual address of the watchdog control frame >> + */ >> +struct sbsa_gwdt { >> + struct watchdog_device wdd; >> + u32 clk; >> + void __iomem *refresh_base; >> + void __iomem *control_base; >> +}; >> + >> +#define to_sbsa_gwdt(e) container_of(e, struct sbsa_gwdt, wdd) >> + >> +#define DEFAULT_TIMEOUT 10 /* seconds, the 1st + 2nd watch >> periods*/ >> +#define DEFAULT_PRETIMEOUT 5 /* seconds, the 2nd watch period*/ >> + >> +static unsigned int timeout; >> +module_param(timeout, uint, 0); >> +MODULE_PARM_DESC(timeout, >> + "Watchdog timeout in seconds. (>=0, default=" >> + __MODULE_STRING(DEFAULT_TIMEOUT) ")"); >> + >> +static unsigned int max_timeout = UINT_MAX; >> +module_param(max_timeout, uint, 0); >> +MODULE_PARM_DESC(max_timeout, >> + "Watchdog max timeout in seconds. (>=0, default=" >> + __MODULE_STRING(UINT_MAX) ")"); >> + >> +static unsigned int pretimeout; >> +module_param(pretimeout, uint, 0); >> +MODULE_PARM_DESC(pretimeout, >> + "Watchdog pretimeout in seconds. (>=0, default=" >> + __MODULE_STRING(DEFAULT_PRETIMEOUT) ")"); >> + >> +static unsigned int max_pretimeout = U32_MAX; >> +module_param(max_pretimeout, uint, 0); >> +MODULE_PARM_DESC(max_pretimeout, >> + "Watchdog max pretimeout in seconds. (>=0, default=" >> + __MODULE_STRING(U32_MAX) ")"); >> + >> +static bool nowayout = WATCHDOG_NOWAYOUT; >> +module_param(nowayout, bool, S_IRUGO); >> +MODULE_PARM_DESC(nowayout, >> + "Watchdog cannot be stopped once started (default=" >> + __MODULE_STRING(WATCHDOG_NOWAYOUT) ")"); >> + >> +/* >> + * Architected system timer support. > > > Architected ? Sounds a bit odd. Not sure if this comment serves a real > purpose. Ah, sorry , this is comment is a legacy , will update it . :-) > > >> + */ >> +static void sbsa_gwdt_cf_write(unsigned int reg, u32 val, >> + struct watchdog_device *wdd) >> +{ >> + struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd); >> + >> + writel_relaxed(val, gwdt->control_base + reg); >> +} >> + >> +static void sbsa_gwdt_rf_write(unsigned int reg, u32 val, >> + struct watchdog_device *wdd) >> +{ >> + struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd); >> + >> + writel_relaxed(val, gwdt->refresh_base + reg); >> +} >> + >> +static u32 sbsa_gwdt_cf_read(unsigned int reg, struct watchdog_device >> *wdd) >> +{ >> + struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd); >> + >> + return readl_relaxed(gwdt->control_base + reg); >> +} >> + >> +/* >> + * help founctions for accessing 64bit WCV register > > > functions Ah , typo , sorry > > >> + */ >> +static u64 sbsa_gwdt_get_wcv(struct watchdog_device *wdd) >> +{ >> + u32 wcv_lo, wcv_hi; >> + >> + do { >> + wcv_hi = sbsa_gwdt_cf_read(SBSA_GWDT_WCV_HI, wdd); >> + wcv_lo = sbsa_gwdt_cf_read(SBSA_GWDT_WCV_LO, wdd); >> + } while (wcv_hi != sbsa_gwdt_cf_read(SBSA_GWDT_WCV_HI, wdd)); >> + >> + return (((u64)wcv_hi << 32) | wcv_lo); >> +} >> + >> +static void sbsa_gwdt_set_wcv(struct watchdog_device *wdd, u64 value) >> +{ >> + u32 wcv_lo, wcv_hi; >> + >> + wcv_lo = value & U32_MAX; >> + wcv_hi = (value >> 32) & U32_MAX; >> + >> + sbsa_gwdt_cf_write(SBSA_GWDT_WCV_HI, wcv_hi, wdd); >> + sbsa_gwdt_cf_write(SBSA_GWDT_WCV_LO, wcv_lo, wdd); >> +} >> + >> +static void reload_timeout_to_wcv(struct watchdog_device *wdd) >> +{ >> + struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd); >> + u64 wcv; >> + >> + wcv = arch_counter_get_cntvct() + >> + (u64)(wdd->timeout - wdd->pretimeout) * gwdt->clk; >> + >> + sbsa_gwdt_set_wcv(wdd, wcv); >> +} >> + >> +/* >> + * Use the following function to set the limit of timeout > > > s/limit of timeout/timeout limits/ Thanks , will fix it > > >> + * after updating pretimeout >> + */ >> +static void sbsa_gwdt_update_limits(struct watchdog_device *wdd) >> +{ >> + struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd); >> + u64 first_period_max = U64_MAX; >> + >> + do_div(first_period_max, gwdt->clk); >> + >> + wdd->min_timeout = wdd->pretimeout + 1; >> + wdd->max_timeout = min((uint)(wdd->pretimeout + first_period_max), >> + wdd->max_timeout); >> +} >> + >> +static int sbsa_gwdt_set_timeout(struct watchdog_device *wdd, >> + unsigned int timeout) >> +{ >> + wdd->timeout = timeout; >> + >> + return 0; >> +} >> + >> +static int sbsa_gwdt_set_pretimeout(struct watchdog_device *wdd, >> + unsigned int pretimeout) >> +{ >> + struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd); >> + u32 wor; >> + >> + wdd->pretimeout = pretimeout; >> + sbsa_gwdt_update_limits(wdd); >> + >> + if (!pretimeout) >> + /* gives sbsa_gwdt_start a chance to setup timeout */ >> + wor = gwdt->clk; >> + else >> + wor = pretimeout * gwdt->clk; >> + >> + /* refresh the WOR, that will cause an explicit watchdog refresh >> */ >> + sbsa_gwdt_cf_write(SBSA_GWDT_WOR, wor, wdd); >> + >> + return 0; >> +} >> + >> +static unsigned int sbsa_gwdt_get_timeleft(struct watchdog_device *wdd) >> +{ >> + struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd); >> + u64 timeleft = sbsa_gwdt_get_wcv(wdd) - arch_counter_get_cntvct(); >> + > > > Still not happy about the use of arch_counter_get_cntvct > instead of using the clock subsystem. I am quite sure this could be done, > possibly through arch_sys_counter, though at this point I am getting wary > of bringing it up, so I guess I'll just let it go. yes, It is the code I should check again, will sent more feedback once I get better idea > > >> + do_div(timeleft, gwdt->clk); >> + >> + return timeleft; >> +} >> + >> +static int sbsa_gwdt_start(struct watchdog_device *wdd) >> +{ >> + /* Force refresh */ >> + sbsa_gwdt_rf_write(SBSA_GWDT_WRR, 0xc0ffee, wdd); >> + /* writing WCS will cause an explicit watchdog refresh */ >> + sbsa_gwdt_cf_write(SBSA_GWDT_WCS, SBSA_GWDT_WCS_EN, wdd); >> + >> + reload_timeout_to_wcv(wdd); >> + >> + return 0; >> +} >> + >> +static int sbsa_gwdt_stop(struct watchdog_device *wdd) >> +{ >> + /* Force refresh */ >> + sbsa_gwdt_rf_write(SBSA_GWDT_WRR, 0xc0ffee, wdd); >> + /* writing WCS will cause an explicit watchdog refresh */ >> + sbsa_gwdt_cf_write(SBSA_GWDT_WCS, 0, wdd); >> + >> + return 0; >> +} >> + >> +static int sbsa_gwdt_keepalive(struct watchdog_device *wdd) >> +{ >> + /* >> + * Writing WRR for an explicit watchdog refresh >> + * You can write anyting(like 0xc0ffee) >> + */ >> + sbsa_gwdt_rf_write(SBSA_GWDT_WRR, 0xc0ffee, wdd); >> + >> + reload_timeout_to_wcv(wdd); >> + >> + return 0; >> +} >> + >> +static irqreturn_t sbsa_gwdt_interrupt(int irq, void *dev_id) >> +{ >> + struct sbsa_gwdt *gwdt = (struct sbsa_gwdt *)dev_id; >> + struct watchdog_device *wdd = &gwdt->wdd; >> + u32 status; >> + >> + status = sbsa_gwdt_cf_read(SBSA_GWDT_WCS, wdd); >> + >> + if (status & SBSA_GWDT_WCS_WS0) >> + panic("SBSA Watchdog pre-timeout"); >> + >> + return IRQ_HANDLED; >> +} >> + >> +static struct watchdog_info sbsa_gwdt_info = { >> + .identity = "SBSA Generic Watchdog", >> + .options = WDIOF_SETTIMEOUT | >> + WDIOF_KEEPALIVEPING | >> + WDIOF_MAGICCLOSE | >> + WDIOF_PRETIMEOUT | >> + WDIOF_CARDRESET, >> +}; >> + >> +static struct watchdog_ops sbsa_gwdt_ops = { >> + .owner = THIS_MODULE, >> + .start = sbsa_gwdt_start, >> + .stop = sbsa_gwdt_stop, >> + .ping = sbsa_gwdt_keepalive, >> + .set_timeout = sbsa_gwdt_set_timeout, >> + .set_pretimeout = sbsa_gwdt_set_pretimeout, >> + .get_timeleft = sbsa_gwdt_get_timeleft, >> +}; >> + >> +static int sbsa_gwdt_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + struct sbsa_gwdt *gwdt; >> + struct watchdog_device *wdd; >> + struct resource *res; >> + void *rf_base, *cf_base; >> + int irq; >> + u32 clk, status; >> + int ret = 0; >> + u64 first_period_max = U64_MAX; >> + >> + /* >> + * Try to determine the frequency from the arch_timer interface >> + */ >> + clk = arch_timer_get_rate(); > > > arch_timer_get_rate() does not seem to be exported. Did you try to build > the driver as module ? yes, I have built it as a ko module, that is why I made a patch to export this interface in the first patch of this patchset but I will confirm it again :-) > > >> + if (!clk) { >> + dev_err(dev, "System Counter frequency not available\n"); >> + return -EINVAL; >> + } >> + >> + gwdt = devm_kzalloc(dev, sizeof(*gwdt), GFP_KERNEL); >> + if (!gwdt) >> + return -ENOMEM; >> + >> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, >> "refresh"); >> + rf_base = devm_ioremap_resource(dev, res); >> + if (IS_ERR(rf_base)) >> + return PTR_ERR(rf_base); >> + >> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, >> "control"); >> + cf_base = devm_ioremap_resource(dev, res); >> + if (IS_ERR(rf_base)) >> + return PTR_ERR(rf_base); >> + >> + irq = platform_get_irq_byname(pdev, "ws0"); >> + if (irq < 0) { >> + dev_err(dev, "unable to get ws0 interrupt.\n"); >> + return irq; >> + } >> + >> + gwdt->refresh_base = rf_base; >> + gwdt->control_base = cf_base; >> + gwdt->clk = clk; >> + >> + platform_set_drvdata(pdev, gwdt); >> + >> + wdd = &gwdt->wdd; >> + wdd->parent = dev; >> + wdd->info = &sbsa_gwdt_info; >> + wdd->ops = &sbsa_gwdt_ops; >> + watchdog_set_drvdata(wdd, gwdt); >> + watchdog_set_nowayout(wdd, nowayout); >> + >> + status = sbsa_gwdt_cf_read(SBSA_GWDT_WCS, wdd); >> + if (status & SBSA_GWDT_WCS_WS1) { >> + dev_warn(dev, "System reset by WDT(WCS: %x, WCV: %llx)\n", >> + status, sbsa_gwdt_get_wcv(wdd)); >> + wdd->bootstatus |= WDIOF_CARDRESET; >> + } >> + /* Check if watchdog is already enabled */ >> + if (status & SBSA_GWDT_WCS_EN) { >> + dev_warn(dev, "already enabled! WCS:0x%x\n", status); >> + sbsa_gwdt_keepalive(wdd); >> + } >> + >> + wdd->min_pretimeout = 0; >> + wdd->max_pretimeout = min(U32_MAX / clk, max_pretimeout); >> + wdd->min_timeout = 1; >> + do_div(first_period_max, clk); >> + wdd->max_timeout = min((uint)(wdd->pretimeout + first_period_max), >> + max_timeout); >> + >> + wdd->pretimeout = DEFAULT_PRETIMEOUT; >> + wdd->timeout = DEFAULT_TIMEOUT; >> + watchdog_init_timeouts(wdd, pretimeout, timeout, >> + sbsa_gwdt_update_limits, dev); >> + /* update pretimeout to WOR */ >> + sbsa_gwdt_cf_write(SBSA_GWDT_WOR, wdd->pretimeout * clk, wdd); >> + >> + ret = devm_request_irq(dev, irq, sbsa_gwdt_interrupt, IRQF_TIMER, >> + pdev->name, gwdt); >> + if (ret) { >> + dev_err(dev, "unable to request IRQ %d\n", irq); >> + return ret; >> + } >> + >> + ret = watchdog_register_device(wdd); >> + if (ret) >> + return ret; >> + >> + dev_info(dev, "Initialized with %ds timeout, %ds pretimeout @ >> %uHz\n", >> + wdd->timeout, wdd->pretimeout, gwdt->clk); >> + >> + return 0; >> +} >> + >> +static void sbsa_gwdt_shutdown(struct platform_device *pdev) >> +{ >> + struct sbsa_gwdt *gwdt = platform_get_drvdata(pdev); >> + >> + sbsa_gwdt_stop(&gwdt->wdd); >> +} >> + >> +static int sbsa_gwdt_remove(struct platform_device *pdev) >> +{ >> + struct sbsa_gwdt *gwdt = platform_get_drvdata(pdev); >> + int ret = 0; >> + >> + if (!nowayout) >> + ret = sbsa_gwdt_stop(&gwdt->wdd); >> + >> + watchdog_unregister_device(&gwdt->wdd); >> + >> + return ret; >> +} >> + >> +/* Disable watchdog if it is active during suspend */ >> +static int __maybe_unused sbsa_gwdt_suspend(struct device *dev) >> +{ >> + struct sbsa_gwdt *gwdt = dev_get_drvdata(dev); >> + >> + if (watchdog_active(&gwdt->wdd)) >> + sbsa_gwdt_stop(&gwdt->wdd); >> + >> + return 0; >> +} >> + >> +/* Enable watchdog and configure it if necessary */ >> +static int __maybe_unused sbsa_gwdt_resume(struct device *dev) >> +{ >> + struct sbsa_gwdt *gwdt = dev_get_drvdata(dev); >> + >> + if (watchdog_active(&gwdt->wdd)) >> + sbsa_gwdt_start(&gwdt->wdd); >> + >> + return 0; >> +} >> + >> +static const struct dev_pm_ops sbsa_gwdt_pm_ops = { >> + SET_SYSTEM_SLEEP_PM_OPS(sbsa_gwdt_suspend, sbsa_gwdt_resume) >> +}; >> + >> +static const struct of_device_id sbsa_gwdt_of_match[] = { >> + { .compatible = "arm,sbsa-gwdt", }, >> + {}, >> +}; >> +MODULE_DEVICE_TABLE(of, sbsa_gwdt_of_match); >> + >> +static const struct platform_device_id sbsa_gwdt_pdev_match[] = { >> + { .name = "sbsa-gwdt", }, >> + {}, >> +}; >> +MODULE_DEVICE_TABLE(platform, sbsa_gwdt_pdev_match); >> + >> +static struct platform_driver sbsa_gwdt_driver = { >> + .driver = { >> + .name = "sbsa-gwdt", >> + .pm = &sbsa_gwdt_pm_ops, >> + .of_match_table = sbsa_gwdt_of_match, >> + }, >> + .probe = sbsa_gwdt_probe, >> + .remove = sbsa_gwdt_remove, >> + .shutdown = sbsa_gwdt_shutdown, >> + .id_table = sbsa_gwdt_pdev_match, >> +}; >> + >> +module_platform_driver(sbsa_gwdt_driver); >> + >> +MODULE_DESCRIPTION("SBSA Generic Watchdog Driver"); >> +MODULE_VERSION("v1.0"); >> +MODULE_AUTHOR("Fu Wei <fu.wei@linaro.org>"); >> +MODULE_AUTHOR("Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>"); >> +MODULE_LICENSE("GPL v2"); >> >
Hi Guenter, Great thanks :-) On 21 May 2015 at 23:18, Guenter Roeck <linux@roeck-us.net> wrote: > On Thu, May 21, 2015 at 07:08:34PM +0800, Fu Wei wrote: >> Hi Guenter, >> >> Thanks for review :-) >> >> On 21 May 2015 at 18:34, Guenter Roeck <linux@roeck-us.net> wrote: >> > On 05/21/2015 01:32 AM, fu.wei@linaro.org wrote: >> >> >> >> From: Fu Wei <fu.wei@linaro.org> >> >> >> >> This driver bases on linux kernel watchdog framework, and >> >> use "pretimeout" in the framework. It supports getting timeout and >> >> pretimeout from parameter and FDT at the driver init stage. >> >> In first timeout(WS0), the interrupt routine run panic to save >> >> system context. >> >> >> >> Signed-off-by: Fu Wei <fu.wei@linaro.org> >> >> --- >> >> drivers/watchdog/Kconfig | 12 ++ >> >> drivers/watchdog/Makefile | 1 + >> >> drivers/watchdog/sbsa_gwdt.c | 476 >> >> +++++++++++++++++++++++++++++++++++++++++++ >> >> 3 files changed, 489 insertions(+) >> >> create mode 100644 drivers/watchdog/sbsa_gwdt.c >> >> >> >> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig >> >> index e5e7c55..25a0df1 100644 >> >> --- a/drivers/watchdog/Kconfig >> >> +++ b/drivers/watchdog/Kconfig >> >> @@ -152,6 +152,18 @@ config ARM_SP805_WATCHDOG >> >> ARM Primecell SP805 Watchdog timer. This will reboot your system >> >> when >> >> the timeout is reached. >> >> >> >> +config ARM_SBSA_WATCHDOG >> >> + tristate "ARM SBSA Generic Watchdog" >> >> + depends on ARM || ARM64 || COMPILE_TEST >> >> + depends on ARM_ARCH_TIMER >> >> + select WATCHDOG_CORE >> >> + help >> >> + ARM SBSA Generic Watchdog timer. This has two Watchdog Signals >> >> + (WS0/WS1), will trigger a warning interrupt(do panic) at the >> >> first >> >> + timeout(WS0); will reboot your system when the second >> >> timeout(WS1) >> >> + is reached. >> > >> > >> > I think it would be better to keep the WS0/WS1 out of the help text. >> > It is an implementation detail, and no user will be able to make sense of >> > it. >> >> I don't mind to delete it , but those are in SBSA spec, is that an >> implementation detail ? >> > I think so. > > Ask yourself - assuming you are not involved in the development of this driver, > and you read this help text. Is the help text going to help you to know about > WS0 and WS1 ? Why not just something like the following, if you want to be > verbose ? > > ARM SBSA Generic Watchdog. This watchdog has two Watchdog timeouts. > The first timeout will trigger a panic; the second timeout will trigger > a system reset. Great thanks, that is better, yes, If I never read SBSA spec, I don't know WS is watchdog signal :-) > > Anyway, not worth arguing about. yes, agree , Thanks for the example "help" :-) > >> >> + >> >> + /* >> >> + * Try to determine the frequency from the arch_timer interface >> >> + */ >> >> + clk = arch_timer_get_rate(); >> > >> > >> > arch_timer_get_rate() does not seem to be exported. Did you try to build >> > the driver as module ? >> >> yes, I have built it as a ko module, that is why I made a patch to >> export this interface in the first patch of this patchset >> >> but I will confirm it again :-) >> > Guess I'll give it a try myself. I don't really understand how this > can work unless arch_timer_get_rate() is exported in your tree. yes, I have exported it , I think it make sense to export it. Because other driver maybe need to get system counter info Do you agree ? :-) > > Guenter
Hi Guenter, On 21 May 2015 at 23:59, Guenter Roeck <linux@roeck-us.net> wrote: > On Thu, May 21, 2015 at 11:46:53PM +0800, Fu Wei wrote: >> Hi Guenter, >> > [ ... ] > >> >> >> + >> >> >> + /* >> >> >> + * Try to determine the frequency from the arch_timer interface >> >> >> + */ >> >> >> + clk = arch_timer_get_rate(); >> >> > >> >> > >> >> > arch_timer_get_rate() does not seem to be exported. Did you try to build >> >> > the driver as module ? >> >> >> >> yes, I have built it as a ko module, that is why I made a patch to >> >> export this interface in the first patch of this patchset >> >> >> >> but I will confirm it again :-) >> >> >> > Guess I'll give it a try myself. I don't really understand how this >> > can work unless arch_timer_get_rate() is exported in your tree. >> >> yes, I have exported it , I think it make sense to export it. >> Because other driver maybe need to get system counter info >> >> Do you agree ? :-) >> > I don't think it is for me to agree or not. The arm maintainers > will need to be involved. You can not just export such a function > without maintainer Ack. yes, I have added Mark Rutland in the CC list when I sent this patchset . He added the "arch_timer_get_rate();" in the arch timer driver. See if he can provide some suggestion. :-) maybe I should add some more arm arch timer maintainers to get more help on this :-) > > Having said that, my personal preference would be for the counter > and rate to be exported through the clock subsystem (ie with > clk_get_rate). But that would still not provide the current counter > value, so maybe that isn't even possible. I will try to make a patch for this, If the arm maintainers don't like exporting "arch_timer_get_rate();" But your thought is good, the clk_get_rate is the best way to do for now > > Thanks, > Guenter
Hi Timur. On 22 May 2015 at 00:33, Timur Tabi <timur@codeaurora.org> wrote: > On 05/21/2015 11:12 AM, Fu Wei wrote: >>> >>> > >>> >Having said that, my personal preference would be for the counter >>> >and rate to be exported through the clock subsystem (ie with >>> >clk_get_rate). But that would still not provide the current counter >>> >value, so maybe that isn't even possible. >> >> I will try to make a patch for this, If the arm maintainers don't like >> exporting "arch_timer_get_rate();" >> >> But your thought is good, the clk_get_rate is the best way to do for now > > > The rate isn't the problem. It's the current timestamp counter. The only > way to get that is with either arch_counter_get_cntvct() or > arch_timer_read_counter(). I'm not sure which of the two functions is > better. However, arch_timer_read_counter() is really just a function > pointer that points to arch_counter_get_cntvct(). > > Also, clk_get_rate() only works if you have a real clk object. I've said > this before many times, but on my ACPI platform, there are no clk objects. > Clocks are handled by UEFI. Thanks for your info, I think : right now , do what we can do, export the interface of arch_timer, If the arm maintainers don't like this way, we can find another way later or find a option as plan B. > > -- > Qualcomm Innovation Center, Inc. > The Qualcomm Innovation Center, Inc. is a member of the > Code Aurora Forum, a Linux Foundation Collaborative Project.
On 2015年05月21日 16:32, fu.wei@linaro.org wrote: > From: Fu Wei <fu.wei@linaro.org> > > This driver bases on linux kernel watchdog framework, and > use "pretimeout" in the framework. It supports getting timeout and > pretimeout from parameter and FDT at the driver init stage. > In first timeout(WS0), the interrupt routine run panic to save > system context. > > Signed-off-by: Fu Wei <fu.wei@linaro.org> > --- > drivers/watchdog/Kconfig | 12 ++ > drivers/watchdog/Makefile | 1 + > drivers/watchdog/sbsa_gwdt.c | 476 +++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 489 insertions(+) > create mode 100644 drivers/watchdog/sbsa_gwdt.c > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig > index e5e7c55..25a0df1 100644 > --- a/drivers/watchdog/Kconfig > +++ b/drivers/watchdog/Kconfig > @@ -152,6 +152,18 @@ config ARM_SP805_WATCHDOG > ARM Primecell SP805 Watchdog timer. This will reboot your system when > the timeout is reached. > > +config ARM_SBSA_WATCHDOG > + tristate "ARM SBSA Generic Watchdog" > + depends on ARM || ARM64 || COMPILE_TEST SBSA is for ARMv8-A based (64-bit) servers, no need to depends on ARM, and why we depends on COMPILE_TEST? > + depends on ARM_ARCH_TIMER > + select WATCHDOG_CORE > + help > + ARM SBSA Generic Watchdog timer. This has two Watchdog Signals > + (WS0/WS1), will trigger a warning interrupt(do panic) at the first > + timeout(WS0); will reboot your system when the second timeout(WS1) > + is reached. > + More details: DEN0029B - Server Base System Architecture (SBSA) > + > config AT91RM9200_WATCHDOG > tristate "AT91RM9200 watchdog" > depends on SOC_AT91RM9200 && MFD_SYSCON > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile > index 5c19294..471f1b7c 100644 > --- a/drivers/watchdog/Makefile > +++ b/drivers/watchdog/Makefile > @@ -30,6 +30,7 @@ obj-$(CONFIG_USBPCWATCHDOG) += pcwd_usb.o > > # ARM Architecture > obj-$(CONFIG_ARM_SP805_WATCHDOG) += sp805_wdt.o > +obj-$(CONFIG_ARM_SBSA_WATCHDOG) += sbsa_gwdt.o > obj-$(CONFIG_AT91RM9200_WATCHDOG) += at91rm9200_wdt.o > obj-$(CONFIG_AT91SAM9X_WATCHDOG) += at91sam9_wdt.o > obj-$(CONFIG_CADENCE_WATCHDOG) += cadence_wdt.o > diff --git a/drivers/watchdog/sbsa_gwdt.c b/drivers/watchdog/sbsa_gwdt.c > new file mode 100644 > index 0000000..4ebe7c3 > --- /dev/null > +++ b/drivers/watchdog/sbsa_gwdt.c > @@ -0,0 +1,476 @@ > +/* > + * SBSA(Server Base System Architecture) Generic Watchdog driver > + * > + * Copyright (c) 2015, Linaro Ltd. > + * Author: Fu Wei <fu.wei@linaro.org> > + * Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License 2 as published > + * by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * Note: This SBSA Generic watchdog driver is compatible with > + * the pretimeout concept of Linux kernel. > + * The timeout and pretimeout are set by the different REGs. > + * The first watch period is set by writing WCV directly, > + * that can support more than 10s timeout at the maximum > + * system counter frequency. > + * The second watch period is set by WOR(32bit) which will be loaded > + * automatically by hardware, when WS0 is triggered. > + * This gives a maximum watch period of around 10s at the maximum > + * system counter frequency. > + * The System Counter shall run at maximum of 400MHz. > + * More details: DEN0029B - Server Base System Architecture (SBSA) > + * > + * Kernel/API: P---------| pretimeout > + * |-------------------------------T timeout > + * SBSA GWDT: P--WOR---WS1 pretimeout > + * |-------WCV----------WS0~~~~~~~~T timeout > + */ > + > +#include <asm/arch_timer.h> please put it under #include <linux/*.h>, this is the convention of order head files. > + > +#include <linux/acpi.h> Will we add any acpi realted code in this patch? if not, please remove this head file. > +#include <linux/io.h> > +#include <linux/interrupt.h> > +#include <linux/module.h> > +#include <linux/moduleparam.h> > +#include <linux/of.h> > +#include <linux/of_device.h> > +#include <linux/platform_device.h> > +#include <linux/uaccess.h> > +#include <linux/watchdog.h> > + > +/* SBSA Generic Watchdog register definitions */ > +/* refresh frame */ > +#define SBSA_GWDT_WRR 0x000 > + > +/* control frame */ > +#define SBSA_GWDT_WCS 0x000 > +#define SBSA_GWDT_WOR 0x008 > +#define SBSA_GWDT_WCV_LO 0x010 > +#define SBSA_GWDT_WCV_HI 0x014 > + > +/* refresh/control frame */ > +#define SBSA_GWDT_W_IIDR 0xfcc > +#define SBSA_GWDT_IDR 0xfd0 > + > +/* Watchdog Control and Status Register */ > +#define SBSA_GWDT_WCS_EN BIT(0) > +#define SBSA_GWDT_WCS_WS0 BIT(1) > +#define SBSA_GWDT_WCS_WS1 BIT(2) > + > +/** > + * struct sbsa_gwdt - Internal representation of the SBSA GWDT > + * @wdd: kernel watchdog_device structure > + * @clk: store the System Counter clock frequency, in Hz. > + * @refresh_base: Virtual address of the watchdog refresh frame > + * @control_base: Virtual address of the watchdog control frame > + */ > +struct sbsa_gwdt { > + struct watchdog_device wdd; > + u32 clk; > + void __iomem *refresh_base; > + void __iomem *control_base; > +}; > + > +#define to_sbsa_gwdt(e) container_of(e, struct sbsa_gwdt, wdd) > + > +#define DEFAULT_TIMEOUT 10 /* seconds, the 1st + 2nd watch periods*/ > +#define DEFAULT_PRETIMEOUT 5 /* seconds, the 2nd watch period*/ > + > +static unsigned int timeout; > +module_param(timeout, uint, 0); > +MODULE_PARM_DESC(timeout, > + "Watchdog timeout in seconds. (>=0, default=" > + __MODULE_STRING(DEFAULT_TIMEOUT) ")"); > + > +static unsigned int max_timeout = UINT_MAX; > +module_param(max_timeout, uint, 0); > +MODULE_PARM_DESC(max_timeout, > + "Watchdog max timeout in seconds. (>=0, default=" > + __MODULE_STRING(UINT_MAX) ")"); > + > +static unsigned int pretimeout; > +module_param(pretimeout, uint, 0); > +MODULE_PARM_DESC(pretimeout, > + "Watchdog pretimeout in seconds. (>=0, default=" > + __MODULE_STRING(DEFAULT_PRETIMEOUT) ")"); > + > +static unsigned int max_pretimeout = U32_MAX; > +module_param(max_pretimeout, uint, 0); > +MODULE_PARM_DESC(max_pretimeout, > + "Watchdog max pretimeout in seconds. (>=0, default=" > + __MODULE_STRING(U32_MAX) ")"); > + > +static bool nowayout = WATCHDOG_NOWAYOUT; > +module_param(nowayout, bool, S_IRUGO); > +MODULE_PARM_DESC(nowayout, > + "Watchdog cannot be stopped once started (default=" > + __MODULE_STRING(WATCHDOG_NOWAYOUT) ")"); > + > +/* > + * Architected system timer support. > + */ > +static void sbsa_gwdt_cf_write(unsigned int reg, u32 val, > + struct watchdog_device *wdd) > +{ > + struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd); > + > + writel_relaxed(val, gwdt->control_base + reg); > +} > + > +static void sbsa_gwdt_rf_write(unsigned int reg, u32 val, > + struct watchdog_device *wdd) > +{ > + struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd); > + > + writel_relaxed(val, gwdt->refresh_base + reg); > +} > + > +static u32 sbsa_gwdt_cf_read(unsigned int reg, struct watchdog_device *wdd) > +{ > + struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd); > + > + return readl_relaxed(gwdt->control_base + reg); > +} > + > +/* > + * help founctions for accessing 64bit WCV register > + */ > +static u64 sbsa_gwdt_get_wcv(struct watchdog_device *wdd) > +{ > + u32 wcv_lo, wcv_hi; > + > + do { > + wcv_hi = sbsa_gwdt_cf_read(SBSA_GWDT_WCV_HI, wdd); > + wcv_lo = sbsa_gwdt_cf_read(SBSA_GWDT_WCV_LO, wdd); > + } while (wcv_hi != sbsa_gwdt_cf_read(SBSA_GWDT_WCV_HI, wdd)); > + > + return (((u64)wcv_hi << 32) | wcv_lo); > +} > + > +static void sbsa_gwdt_set_wcv(struct watchdog_device *wdd, u64 value) > +{ > + u32 wcv_lo, wcv_hi; > + > + wcv_lo = value & U32_MAX; > + wcv_hi = (value >> 32) & U32_MAX; > + > + sbsa_gwdt_cf_write(SBSA_GWDT_WCV_HI, wcv_hi, wdd); > + sbsa_gwdt_cf_write(SBSA_GWDT_WCV_LO, wcv_lo, wdd); > +} > + > +static void reload_timeout_to_wcv(struct watchdog_device *wdd) > +{ > + struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd); > + u64 wcv; > + > + wcv = arch_counter_get_cntvct() + > + (u64)(wdd->timeout - wdd->pretimeout) * gwdt->clk; > + > + sbsa_gwdt_set_wcv(wdd, wcv); > +} > + > +/* > + * Use the following function to set the limit of timeout > + * after updating pretimeout > + */ > +static void sbsa_gwdt_update_limits(struct watchdog_device *wdd) > +{ > + struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd); > + u64 first_period_max = U64_MAX; > + > + do_div(first_period_max, gwdt->clk); > + > + wdd->min_timeout = wdd->pretimeout + 1; > + wdd->max_timeout = min((uint)(wdd->pretimeout + first_period_max), > + wdd->max_timeout); > +} > + > +static int sbsa_gwdt_set_timeout(struct watchdog_device *wdd, > + unsigned int timeout) > +{ > + wdd->timeout = timeout; > + > + return 0; > +} > + > +static int sbsa_gwdt_set_pretimeout(struct watchdog_device *wdd, > + unsigned int pretimeout) > +{ > + struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd); > + u32 wor; > + > + wdd->pretimeout = pretimeout; > + sbsa_gwdt_update_limits(wdd); > + > + if (!pretimeout) > + /* gives sbsa_gwdt_start a chance to setup timeout */ > + wor = gwdt->clk; > + else > + wor = pretimeout * gwdt->clk; > + > + /* refresh the WOR, that will cause an explicit watchdog refresh */ > + sbsa_gwdt_cf_write(SBSA_GWDT_WOR, wor, wdd); > + > + return 0; > +} > + > +static unsigned int sbsa_gwdt_get_timeleft(struct watchdog_device *wdd) > +{ > + struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd); > + u64 timeleft = sbsa_gwdt_get_wcv(wdd) - arch_counter_get_cntvct(); > + > + do_div(timeleft, gwdt->clk); > + > + return timeleft; > +} > + > +static int sbsa_gwdt_start(struct watchdog_device *wdd) > +{ > + /* Force refresh */ > + sbsa_gwdt_rf_write(SBSA_GWDT_WRR, 0xc0ffee, wdd); > + /* writing WCS will cause an explicit watchdog refresh */ > + sbsa_gwdt_cf_write(SBSA_GWDT_WCS, SBSA_GWDT_WCS_EN, wdd); > + > + reload_timeout_to_wcv(wdd); > + > + return 0; > +} > + > +static int sbsa_gwdt_stop(struct watchdog_device *wdd) > +{ > + /* Force refresh */ > + sbsa_gwdt_rf_write(SBSA_GWDT_WRR, 0xc0ffee, wdd); > + /* writing WCS will cause an explicit watchdog refresh */ > + sbsa_gwdt_cf_write(SBSA_GWDT_WCS, 0, wdd); > + > + return 0; > +} > + > +static int sbsa_gwdt_keepalive(struct watchdog_device *wdd) > +{ > + /* > + * Writing WRR for an explicit watchdog refresh > + * You can write anyting(like 0xc0ffee) > + */ > + sbsa_gwdt_rf_write(SBSA_GWDT_WRR, 0xc0ffee, wdd); > + > + reload_timeout_to_wcv(wdd); > + > + return 0; > +} > + > +static irqreturn_t sbsa_gwdt_interrupt(int irq, void *dev_id) > +{ > + struct sbsa_gwdt *gwdt = (struct sbsa_gwdt *)dev_id; > + struct watchdog_device *wdd = &gwdt->wdd; > + u32 status; > + > + status = sbsa_gwdt_cf_read(SBSA_GWDT_WCS, wdd); > + > + if (status & SBSA_GWDT_WCS_WS0) > + panic("SBSA Watchdog pre-timeout"); > + > + return IRQ_HANDLED; > +} > + > +static struct watchdog_info sbsa_gwdt_info = { > + .identity = "SBSA Generic Watchdog", > + .options = WDIOF_SETTIMEOUT | > + WDIOF_KEEPALIVEPING | > + WDIOF_MAGICCLOSE | > + WDIOF_PRETIMEOUT | > + WDIOF_CARDRESET, > +}; > + > +static struct watchdog_ops sbsa_gwdt_ops = { > + .owner = THIS_MODULE, > + .start = sbsa_gwdt_start, > + .stop = sbsa_gwdt_stop, > + .ping = sbsa_gwdt_keepalive, > + .set_timeout = sbsa_gwdt_set_timeout, > + .set_pretimeout = sbsa_gwdt_set_pretimeout, > + .get_timeleft = sbsa_gwdt_get_timeleft, > +}; > + > +static int sbsa_gwdt_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct sbsa_gwdt *gwdt; > + struct watchdog_device *wdd; > + struct resource *res; > + void *rf_base, *cf_base; > + int irq; > + u32 clk, status; > + int ret = 0; > + u64 first_period_max = U64_MAX; > + > + /* > + * Try to determine the frequency from the arch_timer interface > + */ > + clk = arch_timer_get_rate(); > + if (!clk) { > + dev_err(dev, "System Counter frequency not available\n"); > + return -EINVAL; > + } > + > + gwdt = devm_kzalloc(dev, sizeof(*gwdt), GFP_KERNEL); > + if (!gwdt) > + return -ENOMEM; > + > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "refresh"); > + rf_base = devm_ioremap_resource(dev, res); > + if (IS_ERR(rf_base)) > + return PTR_ERR(rf_base); > + > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "control"); > + cf_base = devm_ioremap_resource(dev, res); > + if (IS_ERR(rf_base)) > + return PTR_ERR(rf_base); > + > + irq = platform_get_irq_byname(pdev, "ws0"); > + if (irq < 0) { > + dev_err(dev, "unable to get ws0 interrupt.\n"); > + return irq; > + } > + > + gwdt->refresh_base = rf_base; > + gwdt->control_base = cf_base; > + gwdt->clk = clk; > + > + platform_set_drvdata(pdev, gwdt); > + > + wdd = &gwdt->wdd; > + wdd->parent = dev; > + wdd->info = &sbsa_gwdt_info; > + wdd->ops = &sbsa_gwdt_ops; > + watchdog_set_drvdata(wdd, gwdt); > + watchdog_set_nowayout(wdd, nowayout); > + > + status = sbsa_gwdt_cf_read(SBSA_GWDT_WCS, wdd); > + if (status & SBSA_GWDT_WCS_WS1) { > + dev_warn(dev, "System reset by WDT(WCS: %x, WCV: %llx)\n", > + status, sbsa_gwdt_get_wcv(wdd)); > + wdd->bootstatus |= WDIOF_CARDRESET; > + } > + /* Check if watchdog is already enabled */ > + if (status & SBSA_GWDT_WCS_EN) { > + dev_warn(dev, "already enabled! WCS:0x%x\n", status); > + sbsa_gwdt_keepalive(wdd); > + } > + > + wdd->min_pretimeout = 0; > + wdd->max_pretimeout = min(U32_MAX / clk, max_pretimeout); > + wdd->min_timeout = 1; > + do_div(first_period_max, clk); > + wdd->max_timeout = min((uint)(wdd->pretimeout + first_period_max), > + max_timeout); > + > + wdd->pretimeout = DEFAULT_PRETIMEOUT; > + wdd->timeout = DEFAULT_TIMEOUT; > + watchdog_init_timeouts(wdd, pretimeout, timeout, > + sbsa_gwdt_update_limits, dev); > + /* update pretimeout to WOR */ > + sbsa_gwdt_cf_write(SBSA_GWDT_WOR, wdd->pretimeout * clk, wdd); > + > + ret = devm_request_irq(dev, irq, sbsa_gwdt_interrupt, IRQF_TIMER, > + pdev->name, gwdt); > + if (ret) { > + dev_err(dev, "unable to request IRQ %d\n", irq); > + return ret; > + } > + > + ret = watchdog_register_device(wdd); > + if (ret) > + return ret; > + > + dev_info(dev, "Initialized with %ds timeout, %ds pretimeout @ %uHz\n", > + wdd->timeout, wdd->pretimeout, gwdt->clk); > + > + return 0; > +} > + > +static void sbsa_gwdt_shutdown(struct platform_device *pdev) > +{ > + struct sbsa_gwdt *gwdt = platform_get_drvdata(pdev); > + > + sbsa_gwdt_stop(&gwdt->wdd); > +} > + > +static int sbsa_gwdt_remove(struct platform_device *pdev) > +{ > + struct sbsa_gwdt *gwdt = platform_get_drvdata(pdev); > + int ret = 0; > + > + if (!nowayout) > + ret = sbsa_gwdt_stop(&gwdt->wdd); > + > + watchdog_unregister_device(&gwdt->wdd); > + > + return ret; > +} > + > +/* Disable watchdog if it is active during suspend */ > +static int __maybe_unused sbsa_gwdt_suspend(struct device *dev) > +{ > + struct sbsa_gwdt *gwdt = dev_get_drvdata(dev); > + > + if (watchdog_active(&gwdt->wdd)) > + sbsa_gwdt_stop(&gwdt->wdd); > + > + return 0; > +} > + > +/* Enable watchdog and configure it if necessary */ > +static int __maybe_unused sbsa_gwdt_resume(struct device *dev) > +{ > + struct sbsa_gwdt *gwdt = dev_get_drvdata(dev); > + > + if (watchdog_active(&gwdt->wdd)) > + sbsa_gwdt_start(&gwdt->wdd); > + > + return 0; > +} > + > +static const struct dev_pm_ops sbsa_gwdt_pm_ops = { > + SET_SYSTEM_SLEEP_PM_OPS(sbsa_gwdt_suspend, sbsa_gwdt_resume) > +}; > + > +static const struct of_device_id sbsa_gwdt_of_match[] = { > + { .compatible = "arm,sbsa-gwdt", }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, sbsa_gwdt_of_match); > + > +static const struct platform_device_id sbsa_gwdt_pdev_match[] = { > + { .name = "sbsa-gwdt", }, So this is because we need to auto load the module if those resources are parsed from ACPI table, because there is no acpi_device_id for ACPI tables, right? If so, please add related code in the ACPI patch, and comment on it please :) Thanks Hanjun -- To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2015年05月22日 23:01, Guenter Roeck wrote: > On Fri, May 22, 2015 at 04:55:04PM +0200, Arnd Bergmann wrote: >> On Friday 22 May 2015 22:50:30 Hanjun Guo wrote: >>>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig >>>> index e5e7c55..25a0df1 100644 >>>> --- a/drivers/watchdog/Kconfig >>>> +++ b/drivers/watchdog/Kconfig >>>> @@ -152,6 +152,18 @@ config ARM_SP805_WATCHDOG >>>> ARM Primecell SP805 Watchdog timer. This will reboot your system when >>>> the timeout is reached. >>>> >>>> +config ARM_SBSA_WATCHDOG >>>> + tristate "ARM SBSA Generic Watchdog" >>>> + depends on ARM || ARM64 || COMPILE_TEST >>> >>> SBSA is for ARMv8-A based (64-bit) servers, no need to depends on ARM, >>> and why we depends on COMPILE_TEST? >>> >> >> I think it's a reasonable assumption that someone will sooner or later >> put that hardware into an ARM32 machine, or run a 32-bit kernel on >> a chip that has it. >> >> While SBSA requires this watchdog device, nothing prevents SoC >> manufacturers from using the same design in something that is not >> a server. From this point of view, I agree that SBSA watchdog design may used in other ARM SoCs in the future, but how about add it back when this kind of hardware showing up? >> > Tricky, though. Since teh driver uses arm specific clock functions, > I don't think this can compile on a non-arm machine. Since it depends on ARM64/ARM, we can temporary release from that now :) Thanks Hanjun -- To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Guenter, On 22 May 2015 at 23:01, Guenter Roeck <linux@roeck-us.net> wrote: > On Fri, May 22, 2015 at 04:55:04PM +0200, Arnd Bergmann wrote: >> On Friday 22 May 2015 22:50:30 Hanjun Guo wrote: >> > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig >> > > index e5e7c55..25a0df1 100644 >> > > --- a/drivers/watchdog/Kconfig >> > > +++ b/drivers/watchdog/Kconfig >> > > @@ -152,6 +152,18 @@ config ARM_SP805_WATCHDOG >> > > ARM Primecell SP805 Watchdog timer. This will reboot your system when >> > > the timeout is reached. >> > > >> > > +config ARM_SBSA_WATCHDOG >> > > + tristate "ARM SBSA Generic Watchdog" >> > > + depends on ARM || ARM64 || COMPILE_TEST >> > >> > SBSA is for ARMv8-A based (64-bit) servers, no need to depends on ARM, >> > and why we depends on COMPILE_TEST? >> > >> >> I think it's a reasonable assumption that someone will sooner or later >> put that hardware into an ARM32 machine, or run a 32-bit kernel on >> a chip that has it. >> >> While SBSA requires this watchdog device, nothing prevents SoC >> manufacturers from using the same design in something that is not >> a server. >> > Tricky, though. Since teh driver uses arm specific clock functions, > I don't think this can compile on a non-arm machine. yes, According to SBSA spec, the clock source of SBSAwatchdog is system counter which is a part of arm arch timer. So I think SBSA watchdog always goes with ARM. ARM arch timer has been used on ARM32 and ARM64, so SBSA also can be used on ARM32. Just there is not a ARM32 hardware has that IP core right now. > > Guenter
Hi Timur, On 21 May 2015 at 23:42, Timur Tabi <timur@codeaurora.org> wrote: > On 05/21/2015 03:32 AM, fu.wei@linaro.org wrote: > >> +static void reload_timeout_to_wcv(struct watchdog_device *wdd) >> +{ >> + struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd); >> + u64 wcv; >> + >> + wcv = arch_counter_get_cntvct() + >> + (u64)(wdd->timeout - wdd->pretimeout) * gwdt->clk; >> + >> + sbsa_gwdt_set_wcv(wdd, wcv); >> +} > > > ... > >> +static int sbsa_gwdt_set_timeout(struct watchdog_device *wdd, >> + unsigned int timeout) >> +{ >> + wdd->timeout = timeout; >> + >> + return 0; >> +} > > > ... > >> +static irqreturn_t sbsa_gwdt_interrupt(int irq, void *dev_id) >> +{ >> + struct sbsa_gwdt *gwdt = (struct sbsa_gwdt *)dev_id; >> + struct watchdog_device *wdd = &gwdt->wdd; >> + u32 status; >> + >> + status = sbsa_gwdt_cf_read(SBSA_GWDT_WCS, wdd); >> + >> + if (status & SBSA_GWDT_WCS_WS0) >> + panic("SBSA Watchdog pre-timeout"); >> + >> + return IRQ_HANDLED; >> +} > > > There's one thing I don't understand about your driver. The 'timeout' value > from the kernel is supposed to to be the number of seconds until the system > reboots. You are programming the WCV with that value, which means that the > WS0 interrupt will fire when the timeout expires the first time. However, > you don't reboot the system during this interrupt. The "panic" will cause > the system to halt, but not reboot. Instead, it will just sit there. the "panic" is not just halt the system, please check the code : (1)It can trigger Kdump (not just print the panic message), if you enable this in the config. that can help server administrator to figure out why the system goes wrong. (2)panic also can trigger a reboot, if you set up "panic timeout". Obviously, it won't just sit there, it can help user figure out the problem. At the beginning, I would like to make the first signal more useful, but for simplifying the first version of driver , I decide to use panic(). but if there is better "alerts" for a ARM server , I will go on maintaining this driver to make WS0 more useful. > You're waiting for the WS1 timeout for the system to reboot, but this is not > a clean reboot, and it occurs at 2*timeout seconds. > > That's why I like my driver better. It doesn't have any of this pretimeout > stuff, and when the timeout expires during the WS0 interrupt, it calls > emergency_restart() which reboots the system properly. The WS1 hard reset > is used as a "backup" reset in case emergency_restart() fails. OK, If you think so, I hope you can read the SBSA spec more carefully For the watchdog signal (WS0/WS1), SBSA say: "The initial signal is typically wired to an interrupt and alerts the system. The system can attempt to take corrective action that includes refreshing the watchdog within the second watch period. If the refresh is successful the system returns to the previous normal operation. If it fails then the second watch period expires and a second signal is generated. The signal is fed to a higher agent as an interrupt or reset for it to take executive action." So WS0 is a warning, but not a reset. the WS1 maybe a reset, or a interrupt to higher agent. That is different from a normal watchdog use before. the two stage of WS are not just for reset , at least the first one is definitely not a reset. and the second one is not a backup. If you make SBSA watchdog work like a normal watchdog,: (1)why we need a new driver and new device? you can just use SP805 in the system. (2) why we need a two stages? ( if the second hardware reset signal can work more reliably , why use emergency_restart() which is a software reset, does it clean the system and do some useful backup or sync? ) the only useful thing done by emergency_restart() is kmsg_dump(KMSG_DUMP_EMERG);) (3)why the first WS is connect to a interrupt, but not a reset signal(I believe the direct reset signal is far more reliable than a interrupt to trigger a software reset ) And because of WS0 is a warning, so I decide to use a existing watchdog concept "pretimeout": ----------------- Pretimeouts: Some watchdog timers can be set to have a trigger go off before the actual time they will reset the system. This can be done with an NMI, interrupt, or other mechanism. This allows Linux to record useful information (like panic information and kernel coredumps) before it resets. ----------------- > > -- > Qualcomm Innovation Center, Inc. > The Qualcomm Innovation Center, Inc. is a member of the > Code Aurora Forum, a Linux Foundation Collaborative Project.
Hi Timur, On 24 May 2015 at 00:28, Fu Wei <fu.wei@linaro.org> wrote: > Hi Timur, > > > > On 21 May 2015 at 23:42, Timur Tabi <timur@codeaurora.org> wrote: >> On 05/21/2015 03:32 AM, fu.wei@linaro.org wrote: >> >>> +static void reload_timeout_to_wcv(struct watchdog_device *wdd) >>> +{ >>> + struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd); >>> + u64 wcv; >>> + >>> + wcv = arch_counter_get_cntvct() + >>> + (u64)(wdd->timeout - wdd->pretimeout) * gwdt->clk; >>> + >>> + sbsa_gwdt_set_wcv(wdd, wcv); >>> +} >> >> >> ... >> >>> +static int sbsa_gwdt_set_timeout(struct watchdog_device *wdd, >>> + unsigned int timeout) >>> +{ >>> + wdd->timeout = timeout; >>> + >>> + return 0; >>> +} >> >> >> ... >> >>> +static irqreturn_t sbsa_gwdt_interrupt(int irq, void *dev_id) >>> +{ >>> + struct sbsa_gwdt *gwdt = (struct sbsa_gwdt *)dev_id; >>> + struct watchdog_device *wdd = &gwdt->wdd; >>> + u32 status; >>> + >>> + status = sbsa_gwdt_cf_read(SBSA_GWDT_WCS, wdd); >>> + >>> + if (status & SBSA_GWDT_WCS_WS0) >>> + panic("SBSA Watchdog pre-timeout"); >>> + >>> + return IRQ_HANDLED; >>> +} >> >> >> There's one thing I don't understand about your driver. The 'timeout' value >> from the kernel is supposed to to be the number of seconds until the system >> reboots. You are programming the WCV with that value, which means that the >> WS0 interrupt will fire when the timeout expires the first time. However, >> you don't reboot the system during this interrupt. The "panic" will cause >> the system to halt, but not reboot. Instead, it will just sit there. > > the "panic" is not just halt the system, please check the code : > (1)It can trigger Kdump (not just print the panic message), if you > enable this in the config. that can help server administrator to > figure out why the system goes wrong. > (2)panic also can trigger a reboot, if you set up "panic timeout". > > Obviously, it won't just sit there, it can help user figure out the problem. > > At the beginning, I would like to make the first signal more useful, > but for simplifying the first version of driver , I decide to use > panic(). but if there is better "alerts" for a ARM server , I will go > on maintaining this driver to make WS0 more useful. > >> You're waiting for the WS1 timeout for the system to reboot, but this is not >> a clean reboot, and it occurs at 2*timeout seconds. >> >> That's why I like my driver better. It doesn't have any of this pretimeout >> stuff, and when the timeout expires during the WS0 interrupt, it calls >> emergency_restart() which reboots the system properly. The WS1 hard reset >> is used as a "backup" reset in case emergency_restart() fails. > > OK, If you think so, I hope you can read the SBSA spec more carefully > For the watchdog signal (WS0/WS1), SBSA say: > "The initial signal is typically wired to an interrupt and alerts the > system. The system can attempt to take corrective > action that includes refreshing the watchdog within the second watch > period. If the refresh is successful the > system returns to the previous normal operation. From here, you can see, even a panic is not good enough. we even can refreshing the watchdog. But for simplifying the driver, I think, at least, panic() can help user to backup system context, it is very helpful for a server administrator. Because server should be very stable and important , if its software goes wrong, we must figure out the problem, we can not let it happen again. but in WS0 interrupt routine , just simply restart , it is not a server watchdog should do. > If it fails then the > second watch period expires and a second > signal is generated. The signal is fed to a higher agent as an > interrupt or reset for it to take executive action." > > So WS0 is a warning, but not a reset. the WS1 maybe a reset, or a > interrupt to higher agent. > > That is different from a normal watchdog use before. the two stage of > WS are not just for reset , at least the first one is definitely not a > reset. and the second one is not a backup. > > If you make SBSA watchdog work like a normal watchdog,: > (1)why we need a new driver and new device? you can just use SP805 in > the system. > (2) why we need a two stages? ( if the second hardware reset signal > can work more reliably , why use emergency_restart() which is a > software reset, does it clean the system and do some useful backup or > sync? ) > the only useful thing done by emergency_restart() is > kmsg_dump(KMSG_DUMP_EMERG);) > (3)why the first WS is connect to a interrupt, but not a reset > signal(I believe the direct reset signal is far more reliable than a > interrupt to trigger a software reset ) > > And because of WS0 is a warning, so I decide to use a existing > watchdog concept "pretimeout": > ----------------- > Pretimeouts: > > Some watchdog timers can be set to have a trigger go off before the > actual time they will reset the system. This can be done with an NMI, > interrupt, or other mechanism. This allows Linux to record useful > information (like panic information and kernel coredumps) before it > resets. > ----------------- > >> >> -- >> Qualcomm Innovation Center, Inc. >> The Qualcomm Innovation Center, Inc. is a member of the >> Code Aurora Forum, a Linux Foundation Collaborative Project. > > > > -- > Best regards, > > Fu Wei > Software Engineer > Red Hat Software (Beijing) Co.,Ltd.Shanghai Branch > Ph: +86 21 61221326(direct) > Ph: +86 186 2020 4684 (mobile) > Room 1512, Regus One Corporate Avenue,Level 15, > One Corporate Avenue,222 Hubin Road,Huangpu District, > Shanghai,China 200021
Hi Timur, On 23 May 2015 at 23:08, Timur Tabi <timur@codeaurora.org> wrote: > Arnd Bergmann wrote: >> >> I think it's a reasonable assumption that someone will sooner or later >> put that hardware into an ARM32 machine, > > > I'm going to have to disagree. If they haven't done it by now, I can't > imagine any ARM SOC vendor creating a 32-bit ARM SOC with an SBSA watchdog > in it. I can imagine a vendor trying to repurpose an existing 32-bit ARM > SOC for the server market, but that SOC won't have an SBSA watchdog in it. I will agree with you on this, ONLY IF a people can represent ARM and all chip vendors say publicly: " We never ever use SBSA watchdog IP core on ARM32!" or " SBSA watchdog IP core is imcompatible with ARM32" Although the SBSA is all about ARMv8, but in "5 APPENDIX A: GENERIC WATCHDOG", it doesn't say "this is only for ARMv8". and its clock source "system counter" and arm_arch_timer have been in ARM32 Soc for years, and all the regs in SBSA watchdog is 32bit. I can't see why we can not do that, unless I miss something. I wonder why you are so sure "that SOC won't have an SBSA watchdog in it." any documentation ? Sorry, I am not a chip design engineer, I can't see why 32-bit ARM won't have an SBSA watchdog in it. > >> or run a 32-bit kernel on >> >> a chip that has it. > > > That might happen, but I would be very surprised, and I would need to be > convinced that it's useful. > > -- > Sent by an employee of the Qualcomm Innovation Center, Inc. > The Qualcomm Innovation Center, Inc. is a member of the > Code Aurora Forum, hosted by The Linux Foundation.
Hi Timur On 24 May 2015 at 02:37, Timur Tabi <timur@codeaurora.org> wrote: > Guenter Roeck wrote: >> >> I think it is quite unfortunate that the specification is not public. >> We have heard many statements about what is in the spec or not. > > > All you need to do is go to > http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0029b/index.html, > get a free ARM account, and download the spec. > Great thanks for providing this info! > > -- > Sent by an employee of the Qualcomm Innovation Center, Inc. > The Qualcomm Innovation Center, Inc. is a member of the > Code Aurora Forum, hosted by The Linux Foundation.
Hi Timur On 24 May 2015 at 02:40, Timur Tabi <timur@codeaurora.org> wrote: > Fu Wei wrote: >> >> I wonder why you are so sure "that SOC won't have an SBSA watchdog in >> it." any documentation ? >> Sorry, I am not a chip design engineer, I can't see why 32-bit ARM >> won't have an SBSA watchdog in it. > > > Because there's no market for it. I'm not talking about what's > theoretically possible. I'm only talking about what makes sense and what > will actually happen. And I'm quite certain that we will never see an > actual 32-bit ARM SOC with an SBSA watchdog device in it. Why are you quite certain? any info you can kindly share here? > > Therefore, it makes no sense to complicated the code so that we can support > an SOC that will never exist. yes, that is a good reason! > > So can we PLEASE stop talking about 32-bit ARM support? why? we are just trying to figure out: Do we need to add ARM in "depends on" for SBSA watchdog driver? If some one suggests this, we need to figure out. > > > -- > Sent by an employee of the Qualcomm Innovation Center, Inc. > The Qualcomm Innovation Center, Inc. is a member of the > Code Aurora Forum, hosted by The Linux Foundation.
Hi Guenter, Great thanks for your suggestion and review, I have some questions below , would you please help me out? On 24 May 2015 at 03:51, Guenter Roeck <linux@roeck-us.net> wrote: > On 05/23/2015 11:37 AM, Timur Tabi wrote: >> >> Guenter Roeck wrote: >>> >>> I think it is quite unfortunate that the specification is not public. >>> We have heard many statements about what is in the spec or not. >> >> >> All you need to do is go to >> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0029b/index.html, >> get a free ARM account, and download the spec. >> > That helps - thanks a lot! > > Folks, please correct me if my understanding of the specification > is wrong. > > 1) Pretimeout > > The document suggests that > WS1 = WS0 * 2 Are you saying: the first timeout == the second timeout |-------------WS0 |-------------WS1 Sorry, could you let me know where is that suggestion?? I have checked the SBSA again, but I can not find it. Maybe I really miss this part. > is in fact correct. In essence, there is just one counter, > not two. This means that a separate pretimeout does not really > make sense, since in practice the timeout would always be > twice the pretimeout, Yes, you are right, if we only use "WOR", then the first timeout == the second timeout > and changing just one without affecting > the other is not really possible. although there is only one counter, and it is 32 bits wide. In SBSA, we can see this: ------- Note: the watchdog offset register is 32 bits wide. This gives a maximum watch period of around 10s at a system counter frequency of 400MHz. If a larger watch period is required then the compare value can be programmed directly into the compare value register. ------- So for the first timeout, we can set the compare value register(WCV). Then the two timeouts are different. and the first timeout has not 10s(@400MHz) limit. just the the second timeout must use "WOR". > > 2) 64 bit WCV register > > The specification is not clear on how to read this register. > It clearly states that it is comprised of two 32-bit registers, > but it does not specify if a single 64-bit read would be atomic, > or if it would be split into two separate 32-bit operations. > This leaves room for interpretation by the implementer, and > may result in bad values if the implementation changes the > counter from, say, 0x000010000 to 0x0000ffff while the value > is read. > > My interpretation of this is that it would be safer to read two > 32-bit values and ensure that the values are consistent > instead of relying on the assumption that a single 64-bit read > would be atomic. yes, thanks for pointing it out. I found this problem in my first upstream patchset. So in this patchset, there is not that problem now: ------- do { wcv_hi = sbsa_gwdt_cf_read(SBSA_GWDT_WCV_HI, wdd); wcv_lo = sbsa_gwdt_cf_read(SBSA_GWDT_WCV_LO, wdd); } while (wcv_hi != sbsa_gwdt_cf_read(SBSA_GWDT_WCV_HI, wdd)); ------- > > 3) ACPI vs. FDT > > The specification does not say anything about ACPI or FDT support > except that it assumes that there are "System description data > structures such as ACPI or FDT". Given that, the driver should > support both. yes, this patchset has fully support ACPI and FDT, and has been successfully tested on (1)Foundation model (2)AMD Seattle B0 Soc > > 4) ARM vs. ARM64 > > SBSA clearly states that any CPU supporting it shall be ARM v8 > compliant (4.1.1, CPU architecture). Personally I think the > discussion around supporting the driver on ARM/ARM64 or ARM64 > only is a bit pointless, especially since being able to build > it on ARM doesn't really hurt, even though there is currently > no HW supporting it. > > Overall I must admit that I don't really understand why this is > such a contentious issue. I think that is not a contentious issue. Just some one has that suggestion, then we discussed it. So I am going to delete ARM support. If we have this hardware in the future, we can add it by then. > > 5) Revision support > > While it is difficult to predict the future, it is common practice > in the industry to make future revisions of a standard (and chip) > as much backward compatible as possible, and to only add functionality. > As such, I don't see a reason to restrict support to revision 0 > of the standard. Totally agree, there is not this problem in my patchset. > > 6) A note about driver messages > > Implementation defined registers are just that, implementation > defined registers. I don't think it makes sense to display any > of those, not even for debugging purposes. in this patchset, I have totally deleted all the debug message, only keep: (1)3 error messages (2)2 warning messages reason: 1. if system reset by watchdog, we need to inform user (WCS: store watchdog status, WCV: store the timestamp of the last reboot, maybe these can provide some basic info of reboot ) 2. if watchdog has been enabled before register, there is something wrong we need to inform user. (WCS: store watchdog status) (3)1 info message reason: if watchdog has been successfully registered, we log it. Please let me know if there is any redundant massage, I will fix it. > > --- > > Again, please correct me if any of those statements is wrong. > When doing so, please reference the specification, to make sure > that we all know what we are talking about. Great thanks for your help , those help me a lot. So, for now , this only big problem in my patchset is "pretimeout", but I have some doubt above, would you help me out ? :-) Thanks > > My hope is that we can use this as a starting point to converge > on a single driver. > > Thanks, > Guenter >
Hi Guenter, On 24 May 2015 at 04:01, Guenter Roeck <linux@roeck-us.net> wrote: > On 05/23/2015 12:40 PM, Timur Tabi wrote: > [ ... ] >> >> >> I use emergency_restart(), because the watchdog-api.txt documentation says >> this: >> >> "If userspace fails (RAM error, kernel bug, whatever), the >> notifications cease to occur, and the hardware watchdog will reset the >> system (causing a reboot) after the timeout occurs." >> >> Maybe I'm reading this too literally, but to me this means that when the >> timeout expires, the system has to reset immediately. >> >> However, maybe panic() is better, since it can do the same thing and more. >> > > I have a specific requirement at work to have watchdog expiration > (not this watchdog, this is different HW) result in a panic, specifically > to enable crashdump support and thus post-mortem analysis. > > I had not thought about this use case myself, and I had always wondered > why watchdog driver implementers would choose to call panic() after an > interrupt or NMI. But we live and learn, so now I finally understand. > > In the pretimeout/timeout world, the pretimeout would (typically) > result in a panic, and the timeout would result in a reset. So one > would set the timer register to 10s for 10s pretimeout and 20s timeout. > > However, the pretimeout concept assumes that there are two timers > which can be set independently. As you had pointed out earlier, > and as the specification seems to confirm, that is not the case here. Sorry, in Documentation/watchdog/watchdog-api.txt, I can not get the info about " the pretimeout concept assumes that there are two timers which can be set independently." Could you kindly point out where is the assumption. I thinks in kernel documentation, that meams "one watchdog has two timeout stages", maybe I miss something. Could you help me out? > As such, I don't really understand why and how the pretimeout / timeout > concept would add any value here and not just make things more > complicated than necessary. Maybe I am just missing something. If pretimeout concept assumes that there are two timers, I misunderstand the "pretimeout", then I will delete the pretimeout immediately. > > Thanks, > Guenter >
Hi Guenter, On 24 May 2015 at 04:44, Guenter Roeck <linux@roeck-us.net> wrote: > On 05/23/2015 01:27 PM, Timur Tabi wrote: >> >> Guenter Roeck wrote: >>> >>> >>> However, the pretimeout concept assumes that there are two timers >>> which can be set independently. As you had pointed out earlier, >>> and as the specification seems to confirm, that is not the case here. >>> As such, I don't really understand why and how the pretimeout / timeout >>> concept would add any value here and not just make things more >>> complicated than necessary. Maybe I am just missing something. >> >> >> It might be possible to load a new value into the WOR register after the >> WS0 interrupt occurs. That is, in the interrupt handler, we can do >> something like this: >> >> if (status & SBSA_GWDT_WCS_WS0) >> // write new WOR value, >> // then ping watchdog so that it's loaded >> >> I'm not convinced that it's worth it, however. It would require >> interrupts to still be working when WS0 times out, which somewhat defeats >> the purpose of a watchdog. >> > > If I understand the specification correctly, reloading the register > would result in another WS0, not in WS1. That isn't really what we > would want to happen. Yes, you are 100% correct. In SBSA: --------------- An explicit watchdog refresh occurs when one of a number of different events occur: (1) The Watchdog Refresh Register is written. (2) The Watchdog Offset Register is written. (3) The Watchdog Control and Status register is written. In the case of an explicit refresh the Watchdog Signals are cleared. --------------- So, for the second timeout, we can only use WOR.(but maybe we can write WCV in the WS0 routine to make the second timeout longer, if we really need a long time for panic) If we write WOR , that will clean WCS, then the system go back to first timeout stage. But for the first timeout, we can use WOR (that means the two timeout stages will be the same, in another world, WS0==WS1*2 ) or we can write WCV directly. And writing WCV will not trigger an explicit watchdog refresh, that is what I am doing to set up pretimeout. > > Reloading the register would normally be done in the crashdump kernel, > if it is loaded, to give it time to actually take the crashdump. > But that is post-restart, not pre-restart. > > Thanks, > Guenter >
Hi Guenter, Great thanks for your time, you provide your feedback in your weekend, I am so appreciate that. my feedback inline below On 24 May 2015 at 22:06, Guenter Roeck <linux@roeck-us.net> wrote: > On 05/24/2015 02:58 AM, Fu Wei wrote: >> >> Hi Guenter, >> >> Great thanks for your suggestion and review, >> I have some questions below , would you please help me out? >> >> On 24 May 2015 at 03:51, Guenter Roeck <linux@roeck-us.net> wrote: >>> >>> On 05/23/2015 11:37 AM, Timur Tabi wrote: >>>> >>>> >>>> Guenter Roeck wrote: >>>>> >>>>> >>>>> I think it is quite unfortunate that the specification is not public. >>>>> We have heard many statements about what is in the spec or not. >>>> >>>> >>>> >>>> All you need to do is go to >>>> >>>> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0029b/index.html, >>>> get a free ARM account, and download the spec. >>>> >>> That helps - thanks a lot! >>> >>> Folks, please correct me if my understanding of the specification >>> is wrong. >>> >>> 1) Pretimeout >>> >>> The document suggests that >>> WS1 = WS0 * 2 >> >> >> Are you saying: the first timeout == the second timeout >> >> |-------------WS0 >> |-------------WS1 >> >> Sorry, could you let me know where is that suggestion?? >> I have checked the SBSA again, but I can not find it. >> Maybe I really miss this part. >> > > The document states: > > "If both watchdog signals are deasserted and a timeout refresh occurs then > WS0 is asserted. > If WS0 is asserted and a timeout refresh occurs then WS1 is asserted." yes, this just describe how the WS0/WS1 are deasserted or asserted. but it doesn't suggest WS1 = WS0 * 2. Of course, If we only config WOR in the driver, we get WS1 = WS0 * 2. If we delete pretimeout concept from this driver Then what is the timeout, |-------------WS0 or |-------------WS0-------------WS1 ? If we only config WOR , the timeout for two stage will be the same I think the first stage should be called timeout, the user get a panic in timeout(the first stage) Then we can tell user, in this driver, if first timeout is triggered but fail, we can wait for the same time for reboot. but the second timeout is still like a pretimeout, why not just use it ? IMO, the key point of SBSA watchdog is two signal, and the first one is interrupt, the second one maybe a hardware reset OR a interrupt if we drop "pretimeout", the watchdog just acts like a common watchdog (just like SP805) Now we assume that the second signal is a reset, but according to SBSA, that maybe a interrupt. If that happens on a Soc(that will happen), how to deal with that? what is the timeout between WS0 and WS1? If the two signals are all connect to interrupts, I think we still need a pretimeout. why not use pretimeout right now. that will be more easy to maintain the driver. > > There is no mention that programming both WOR and WCV would or even > could result in different timeouts for the two watchdog signals. yes, they don't mention it. using WCV to config the first stage and WOR for the second stage is my idea to solve the two stage timeouts. Because by this way, we can make the first stage longer(has not that 10s(@400MHz) limit), and we can provide the full feature of SBSA watchdog. as I have mentioned above, If the two signals are all connect to interrupts, we don't need to tell user : the two stage timeout must be the same and 10s(@400MHz) limit, > >>> is in fact correct. In essence, there is just one counter, >>> not two. This means that a separate pretimeout does not really >>> make sense, since in practice the timeout would always be >>> twice the pretimeout, >> >> >> Yes, you are right, if we only use "WOR", then the first timeout == >> the second timeout >> >>> and changing just one without affecting >>> the other is not really possible. >> >> >> although there is only one counter, and it is 32 bits wide. >> In SBSA, we can see this: >> ------- >> Note: the watchdog offset register is 32 bits wide. This gives a >> maximum watch period of around 10s at a system counter frequency of >> 400MHz. >> If a larger watch period is required then the compare value can be >> programmed directly into the compare value register. >> ------- >> So for the first timeout, we can set the compare value register(WCV). >> Then the two timeouts are different. and the first timeout has not >> 10s(@400MHz) limit. >> just the the second timeout must use "WOR". >> > > That is not how I read the specification. It only talks about > "timeout refresh". There is no indication that using both registers > would have the impact you describe. > > Since there is no mention of different WS0 and WS1 timeout periods > in the specification, I am concerned that even _if_ a specific > implementation supports such different timeouts, it would be > implementation specific. yes, you are right, they don't mention, but they can be different, that is the way I use. I just put limit on user , say: the two timeout must be the same. I hope my driver can provide the full feature of hardware. but they make driver a little more complicated than the one which simply config WOR. I have to say , another one make SBSA watchdog become a watchdog. > > Maybe you and/or Timur can test this on the real systems you > have access to. It should be quite straightforward to test - > have the interrupt handler only print a message, program some > values into the watchdog, and see when WS0 and WS1 occur. yes, the first version of my driver has printed the timeleft out, I can have different timeouts and pretimeout > > Thanks, > Guenter >
Hi Guenter, Thanks for your feedback On 24 May 2015 at 22:15, Guenter Roeck <linux@roeck-us.net> wrote: > On 05/24/2015 03:15 AM, Fu Wei wrote: >> >> Hi Guenter, >> >> On 24 May 2015 at 04:01, Guenter Roeck <linux@roeck-us.net> wrote: >>> >>> On 05/23/2015 12:40 PM, Timur Tabi wrote: >>> [ ... ] >>>> >>>> >>>> >>>> I use emergency_restart(), because the watchdog-api.txt documentation >>>> says >>>> this: >>>> >>>> "If userspace fails (RAM error, kernel bug, whatever), the >>>> notifications cease to occur, and the hardware watchdog will reset the >>>> system (causing a reboot) after the timeout occurs." >>>> >>>> Maybe I'm reading this too literally, but to me this means that when the >>>> timeout expires, the system has to reset immediately. >>>> >>>> However, maybe panic() is better, since it can do the same thing and >>>> more. >>>> >>> >>> I have a specific requirement at work to have watchdog expiration >>> (not this watchdog, this is different HW) result in a panic, specifically >>> to enable crashdump support and thus post-mortem analysis. >>> >>> I had not thought about this use case myself, and I had always wondered >>> why watchdog driver implementers would choose to call panic() after an >>> interrupt or NMI. But we live and learn, so now I finally understand. >>> >>> In the pretimeout/timeout world, the pretimeout would (typically) >>> result in a panic, and the timeout would result in a reset. So one >>> would set the timer register to 10s for 10s pretimeout and 20s timeout. >>> >>> However, the pretimeout concept assumes that there are two timers >>> which can be set independently. As you had pointed out earlier, >>> and as the specification seems to confirm, that is not the case here. >> >> >> Sorry, in Documentation/watchdog/watchdog-api.txt, I can not get the >> info about " the pretimeout concept assumes that there are two timers >> which can be set independently." >> Could you kindly point out where is the assumption. >> >> I thinks in kernel documentation, that meams "one watchdog has two >> timeout stages", maybe I miss something. Could you help me out? >> > My apologies. Terminology problem; see below. > > Note that the pretimeout, as documented, is a difference to the real > timeout, not an absolute time (which I had not realized before). > > "Note that the pretimeout is the number of seconds before the time > when the timeout will go off. It is not the number of seconds until > the pretimeout. So, for instance, if you set the timeout to 60 seconds > and the pretimeout to 10 seconds, the pretimeout will go off in 50 > seconds. Setting a pretimeout to zero disables it." yes , this patchset is designed for this pretimeout concept > >>> As such, I don't really understand why and how the pretimeout / timeout >>> concept would add any value here and not just make things more >>> complicated than necessary. Maybe I am just missing something. >> >> >> If pretimeout concept assumes that there are two timers, I >> misunderstand the "pretimeout", then I will delete the pretimeout >> immediately. >> > I think I used the wrong term. I should have said something like > "two distinct timeout values". Actually, I have added my thought at the head of sbsa_gwdt.c as a comment : * * Note: This SBSA Generic watchdog driver is compatible with * the pretimeout concept of Linux kernel. * The timeout and pretimeout are set by the different REGs. * The first watch period is set by writing WCV directly, * that can support more than 10s timeout at the maximum * system counter frequency. * The second watch period is set by WOR(32bit) which will be loaded * automatically by hardware, when WS0 is triggered. * This gives a maximum watch period of around 10s at the maximum * system counter frequency. * The System Counter shall run at maximum of 400MHz. * More details: DEN0029B - Server Base System Architecture (SBSA) * * Kernel/API: P---------| pretimeout * |-------------------------------T timeout * SBSA GWDT: P--WOR---WS1 pretimeout * |-------WCV----------WS0~~~~~~~~T timeout */ > > Guenter >
Hi Timur, I have said this before: in the first timeout, just panic() maybe not enough, in [RFC] version of my patchset, I offer some option as "preaction" to use, but for simplifying the first version of driver, I have deleted them. but at least, panic() is far more useful than a simple reset. at least, it can provide the context of the crashed system to admin. If you want to warn user space, that will make driver more complicated, I don't think that is a good choose for a first version. but we can find a way to improve this later On 24 May 2015 at 23:02, Timur Tabi <timur@codeaurora.org> wrote: > Fu Wei wrote: >> >> If pretimeout concept assumes that there are two timers, I >> misunderstand the "pretimeout", then I will delete the pretimeout >> immediately. > > > In my opinion, calling panic() on a pre-timeout is not useful, because > that's really just a normal timeout. If there were a way to "warn" user > space that a timeout is about to occur, without a panic or reset, then that > might be useful. But as far as I can see, all you're doing is redefining > the word "timeout". > > > -- > Sent by an employee of the Qualcomm Innovation Center, Inc. > The Qualcomm Innovation Center, Inc. is a member of the > Code Aurora Forum, hosted by The Linux Foundation.
Hi Timur, Sorry, how do you trigger a panic if WS1 ties to reset ?? don't know what is your question could you make it simple: 1) calling panic() on pre-timeout (WS0????) 2) calling panic() on timeout (WS1????) in "first version" , I just print the timeleft for WS1 or panic when WS0 occurs. On 25 May 2015 at 00:13, Timur Tabi <timur@codeaurora.org> wrote: > Fu Wei wrote: >> >> in the first timeout, just panic() maybe not enough, in [RFC] >> version of my patchset, I offer some option as "preaction" to use, but >> for simplifying the first version of driver, I have deleted them. >> but at least, panic() is far more useful than a simple reset. at >> least, it can provide the context of the crashed system to admin. > > > My point is that there is very little difference between > > 1) calling panic() on pre-timeout > 2) calling panic() on timeout > > In both cases, the system will panic. The watchdog API says that the system > should reset when a timeout occurs, so you cannot call panic() before the > timeout expires. > >> If you want to warn user space, that will make driver more >> complicated, I don't think that is a good choose for a first version. >> but we can find a way to improve this later > > In my opinion, this "first version" is not useful. I would like to see a > pre-timeout feature that does not panic or reset when a pre-timeout occurs. > > > -- > Sent by an employee of the Qualcomm Innovation Center, Inc. > The Qualcomm Innovation Center, Inc. is a member of the > Code Aurora Forum, hosted by The Linux Foundation.
Hi Guenter On 25 May 2015 at 00:29, Guenter Roeck <linux@roeck-us.net> wrote: > On 05/24/2015 09:13 AM, Timur Tabi wrote: >> >> Fu Wei wrote: >>> >>> in the first timeout, just panic() maybe not enough, in [RFC] >>> version of my patchset, I offer some option as "preaction" to use, but >>> for simplifying the first version of driver, I have deleted them. >>> but at least, panic() is far more useful than a simple reset. at >>> least, it can provide the context of the crashed system to admin. >> >> >> My point is that there is very little difference between >> >> 1) calling panic() on pre-timeout >> 2) calling panic() on timeout >> > > The assumption would be that the second timeout doesn't cause a panic > but a system reset. > >> In both cases, the system will panic. The watchdog API says that the >> system should reset when a timeout occurs, so you cannot call panic() before >> the timeout expires. >> >> > If you want to warn user space, that will make driver more >> > complicated, I don't think that is a good choose for a first version. >> > but we can find a way to improve this later >> >> In my opinion, this "first version" is not useful. I would like to see a >> pre-timeout feature that does not panic or reset when a pre-timeout occurs. >> > > The current watchdog API suggests that the pretimeout "allows Linux > to record useful information (like panic information and kernel > coredumps) before it resets". The call to panic() would be the > means to make this happen. Yes, that is what I mean. Great thanks for your explanation. :-) > > Are you suggesting to change this definition ? What should it do > instead in your opinion ? > > Thanks, > Guenter >
Hi Guenter, On 25 May 2015 at 00:23, Guenter Roeck <linux@roeck-us.net> wrote: > On 05/24/2015 08:50 AM, Fu Wei wrote: > [ ...] > >> Actually, I have added my thought at the head of sbsa_gwdt.c as a comment >> : >> >> * >> * Note: This SBSA Generic watchdog driver is compatible with >> * the pretimeout concept of Linux kernel. >> * The timeout and pretimeout are set by the different REGs. >> * The first watch period is set by writing WCV directly, >> * that can support more than 10s timeout at the maximum >> * system counter frequency. >> * The second watch period is set by WOR(32bit) which will be >> loaded >> * automatically by hardware, when WS0 is triggered. >> * This gives a maximum watch period of around 10s at the maximum >> * system counter frequency. >> * The System Counter shall run at maximum of 400MHz. >> * More details: DEN0029B - Server Base System Architecture (SBSA) >> * >> * Kernel/API: P---------| pretimeout >> * |-------------------------------T timeout >> * SBSA GWDT: P--WOR---WS1 pretimeout >> * |-------WCV----------WS0~~~~~~~~T timeout >> */ >> > > Yes, but do we actually _know_ that it works that way, ie that WCV > drives WS0 and that WOR drives WS1 ? Unless I am missing something, > the specification doesn't say that, and it would have been a really > easy statement to make if that was the intent. yes, Suravee has tested it on Seattle B0 Soc, that works. But hope Suravee can provide more info about test, I will ping him later. According to SBSA, that WCV and WOR can both drive WS1 and WS0 the timeout and pretimeout in my patchset have been tested on Seattle B0 and Foundation model. > > My concern here is that the above behavior is not spelled out in > the document, meaning it is up to interpretation by the hardware > engineer implementing it, to the point where it appears that not > even two software engineers can agree how it is supposed to work. > Which is a really bad starting point :-(. Is that a real hardware teat can prove it works? or actually, SBSA say that it should work: ----------------- Note: the watchdog offset register is 32 bits wide. This gives a maximum watch period of around 10s at a system counter frequency of 400MHz. If a larger watch period is required then the compare value can be programmed directly into the compare value register. ----------------- offset register == WOR compare value register == WCV > > Thanks, > Guenter > > > -- > To unsubscribe from this list: send the line "unsubscribe devicetree" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Timur On 25 May 2015 at 00:44, Timur Tabi <timur@codeaurora.org> wrote: > Guenter Roeck wrote: > >> The current watchdog API suggests that the pretimeout "allows Linux >> to record useful information (like panic information and kernel >> coredumps) before it resets". The call to panic() would be the >> means to make this happen. > > > Now that I think about it, that does make sense. > > However, if a pre-timeout is not set, then the driver should never call > panic(). That means that the interrupt handler should only be registered if > sbsa_gwdt_set_pretimeout() is called. I don't know why you want to do this tricky way. you can always register the interrupt handler, if pre-timeout is 0, system will just trigger WS1 right after WS0 > > > -- > Sent by an employee of the Qualcomm Innovation Center, Inc. > The Qualcomm Innovation Center, Inc. is a member of the > Code Aurora Forum, hosted by The Linux Foundation.
Hi Guenter, On 25 May 2015 at 00:58, Guenter Roeck <linux@roeck-us.net> wrote: > On 05/24/2015 09:47 AM, Fu Wei wrote: >> >> Hi Guenter, >> >> On 25 May 2015 at 00:23, Guenter Roeck <linux@roeck-us.net> wrote: >>> >>> On 05/24/2015 08:50 AM, Fu Wei wrote: >>> [ ...] >>> >>>> Actually, I have added my thought at the head of sbsa_gwdt.c as a >>>> comment >>>> : >>>> >>>> * >>>> * Note: This SBSA Generic watchdog driver is compatible with >>>> * the pretimeout concept of Linux kernel. >>>> * The timeout and pretimeout are set by the different REGs. >>>> * The first watch period is set by writing WCV directly, >>>> * that can support more than 10s timeout at the maximum >>>> * system counter frequency. >>>> * The second watch period is set by WOR(32bit) which will be >>>> loaded >>>> * automatically by hardware, when WS0 is triggered. >>>> * This gives a maximum watch period of around 10s at the >>>> maximum >>>> * system counter frequency. >>>> * The System Counter shall run at maximum of 400MHz. >>>> * More details: DEN0029B - Server Base System Architecture >>>> (SBSA) >>>> * >>>> * Kernel/API: P---------| pretimeout >>>> * |-------------------------------T timeout >>>> * SBSA GWDT: P--WOR---WS1 pretimeout >>>> * |-------WCV----------WS0~~~~~~~~T timeout >>>> */ >>>> >>> >>> Yes, but do we actually _know_ that it works that way, ie that WCV >>> drives WS0 and that WOR drives WS1 ? Unless I am missing something, >>> the specification doesn't say that, and it would have been a really >>> easy statement to make if that was the intent. >> >> >> yes, Suravee has tested it on Seattle B0 Soc, that works. >> But hope Suravee can provide more info about test, I will ping him later. >> >> According to SBSA, that WCV and WOR can both drive WS1 and WS0 >> >> the timeout and pretimeout in my patchset have been tested on Seattle >> B0 and Foundation model. >> >>> >>> My concern here is that the above behavior is not spelled out in >>> the document, meaning it is up to interpretation by the hardware >>> engineer implementing it, to the point where it appears that not >>> even two software engineers can agree how it is supposed to work. >>> Which is a really bad starting point :-(. >> >> >> Is that a real hardware teat can prove it works? >> >> or actually, SBSA say that it should work: >> ----------------- >> Note: the watchdog offset register is 32 bits wide. This gives a >> maximum watch period of around 10s at a system >> counter frequency of 400MHz. If a larger watch period is required then >> the compare value can be programmed >> directly into the compare value register. >> ----------------- >> offset register == WOR >> compare value register == WCV >> > > Where does it say that WCV shall be associated with WS0 and that WOR > shall be associated with WS1 ? Guess I am missing that part. no, it doesn't say WCV shall be associated with WS0, that is my driver doing. but WCV can control both WS1/WS0. WOR is just a auto-reload value. I think maybe you can read SBSA 2.3 Page 23, The pseudocode can explain everything. :-) Great thanks for your time > > > Thanks, > Guenter > > -- > To unsubscribe from this list: send the line "unsubscribe devicetree" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Timur, On 25 May 2015 at 01:19, Timur Tabi <timur@codeaurora.org> wrote: > Fu Wei wrote: >> >> I don't know why you want to do this tricky way. you can always >> register the interrupt handler, >> if pre-timeout is 0, system will just trigger WS1 right after WS0 > > > But that only works if the pre-timeout and timeout can be programmed to > separate values. And as Guenter says, the SBSA may not guarantee that. In my driver patch, the first stage, I am using WCV, for the second stage I am using WOR, that have been tested on real hardware. would you please read the pseudocode in Page 23 of SBSA 2.3 > > > -- > Sent by an employee of the Qualcomm Innovation Center, Inc. > The Qualcomm Innovation Center, Inc. is a member of the > Code Aurora Forum, hosted by The Linux Foundation.
Hi, Guenter, On 25 May 2015 at 01:32, Guenter Roeck <linux@roeck-us.net> wrote: > On 05/24/2015 10:19 AM, Timur Tabi wrote: >> >> Fu Wei wrote: >>> >>> I don't know why you want to do this tricky way. you can always >>> register the interrupt handler, >>> if pre-timeout is 0, system will just trigger WS1 right after WS0 >> >> >> But that only works if the pre-timeout and timeout can be programmed to >> separate values. And as Guenter says, the SBSA may not guarantee that. >> > > The pseudo-code in the specification suggests that if WCV is configured, > WS0 = WCV > WS1 = WCV + WOR > > Assuming that the implementation follows the pseudo-code in the > specification, > we would have separately programmable values. Since the pretimeout (per ABI) > is the difference in seconds to the timeout, and not an absolute value, > we would have to program the registers as follows. > > WCV = timeout - pretimeout; > WOR = pretimeout; yes, this patchset is doing this way. > > Does this make sense ? > > Thanks, > Guenter >
Hi Guenter, On 25 May 2015 at 01:47, Timur Tabi <timur@codeaurora.org> wrote: > Guenter Roeck wrote: >> >> >> The pseudo-code in the specification suggests that if WCV is configured, >> WS0 = WCV >> WS1 = WCV + WOR >> >> Assuming that the implementation follows the pseudo-code in the >> specification, >> we would have separately programmable values. Since the pretimeout (per >> ABI) >> is the difference in seconds to the timeout, and not an absolute value, >> we would have to program the registers as follows. >> >> WCV = timeout - pretimeout; >> WOR = pretimeout; >> >> Does this make sense ? And actually all the patchset I sent to upstream are all follow this design, and have been test on Foundation model and Seattle B0 > > > Yes, thank you. I will test this on my hardware. > > > -- > Sent by an employee of the Qualcomm Innovation Center, Inc. > The Qualcomm Innovation Center, Inc. is a member of the > Code Aurora Forum, hosted by The Linux Foundation.
Hi Guenter, On 21 May 2015 at 23:28, Guenter Roeck <linux@roeck-us.net> wrote: > On Thu, May 21, 2015 at 08:09:02AM -0500, Timur Tabi wrote: >> Guenter Roeck wrote: >> >> >> >>+static unsigned int sbsa_gwdt_get_timeleft(struct watchdog_device *wdd) >> >>+{ >> >>+ struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd); >> >>+ u64 timeleft = sbsa_gwdt_get_wcv(wdd) - arch_counter_get_cntvct(); >> >>+ >> > >> >Still not happy about the use of arch_counter_get_cntvct >> >instead of using the clock subsystem. I am quite sure this could be done, >> >possibly through arch_sys_counter, though at this point I am getting wary >> >of bringing it up, so I guess I'll just let it go. >> >> You made the same comment with my driver, and I keep asking for >> clarification. The clk_get_sys() API does not work on my system, because >> there are not clocks defined. That must be an ACPI limitation that I can't >> fix. >> > Would it be possible to define such clocks ? > >> The alternative to arch_counter_get_cntvct() is arch_timer_read_counter(), >> which is not exported. So we have two choices, >> >> 1) Continue to use arch_counter_get_cntvct(), which works on all ARM64 >> platforms that this driver supports anyway >> >> 2) Export arch_timer_read_counter() >> >> I prefer option #1. >> > Do we have any feedback from the arm maintainers ? > > My problem is that I don't want to be the first one to permit using > those functions outside architecture and clock code. If we do this, > we should get an Ack from an arm maintainer specifically for the use > of arch_counter_get_cntvct() and arch_timer_get_rate(). IMO, we may need to use arch_timer_read_counter. Reason: Once the Linux kernel has KVM support and is in hyp mode. we may need to use arch_counter_get_cntpct. So arch_timer driver have helped us to make arch_timer_read_counter point to the right function. and arch_timer_read_counter is in the include/clocksource/arm_arch_timer.h, just like arch_timer_get_rate as a interface > > Thanks, > Guenter > -- > To unsubscribe from this list: send the line "unsubscribe devicetree" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Timur. On 25 May 2015 at 11:46, Timur Tabi <timur@codeaurora.org> wrote: > Fu Wei wrote: >> >> Once the Linux kernel has KVM support and is in hyp mode. we >> may need to use arch_counter_get_cntpct. > > > arch_counter_get_cntpct() does not appear to be valid for ARM64: > > http://lxr.free-electrons.com/source/arch/arm64/include/asm/arch_timer.h#L108 Great thank for correction, that make sense. > > > -- > Sent by an employee of the Qualcomm Innovation Center, Inc. > The Qualcomm Innovation Center, Inc. is a member of the > Code Aurora Forum, hosted by The Linux Foundation.
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index e5e7c55..25a0df1 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -152,6 +152,18 @@ config ARM_SP805_WATCHDOG ARM Primecell SP805 Watchdog timer. This will reboot your system when the timeout is reached. +config ARM_SBSA_WATCHDOG + tristate "ARM SBSA Generic Watchdog" + depends on ARM || ARM64 || COMPILE_TEST + depends on ARM_ARCH_TIMER + select WATCHDOG_CORE + help + ARM SBSA Generic Watchdog timer. This has two Watchdog Signals + (WS0/WS1), will trigger a warning interrupt(do panic) at the first + timeout(WS0); will reboot your system when the second timeout(WS1) + is reached. + More details: DEN0029B - Server Base System Architecture (SBSA) + config AT91RM9200_WATCHDOG tristate "AT91RM9200 watchdog" depends on SOC_AT91RM9200 && MFD_SYSCON diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile index 5c19294..471f1b7c 100644 --- a/drivers/watchdog/Makefile +++ b/drivers/watchdog/Makefile @@ -30,6 +30,7 @@ obj-$(CONFIG_USBPCWATCHDOG) += pcwd_usb.o # ARM Architecture obj-$(CONFIG_ARM_SP805_WATCHDOG) += sp805_wdt.o +obj-$(CONFIG_ARM_SBSA_WATCHDOG) += sbsa_gwdt.o obj-$(CONFIG_AT91RM9200_WATCHDOG) += at91rm9200_wdt.o obj-$(CONFIG_AT91SAM9X_WATCHDOG) += at91sam9_wdt.o obj-$(CONFIG_CADENCE_WATCHDOG) += cadence_wdt.o diff --git a/drivers/watchdog/sbsa_gwdt.c b/drivers/watchdog/sbsa_gwdt.c new file mode 100644 index 0000000..4ebe7c3 --- /dev/null +++ b/drivers/watchdog/sbsa_gwdt.c @@ -0,0 +1,476 @@ +/* + * SBSA(Server Base System Architecture) Generic Watchdog driver + * + * Copyright (c) 2015, Linaro Ltd. + * Author: Fu Wei <fu.wei@linaro.org> + * Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License 2 as published + * by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * Note: This SBSA Generic watchdog driver is compatible with + * the pretimeout concept of Linux kernel. + * The timeout and pretimeout are set by the different REGs. + * The first watch period is set by writing WCV directly, + * that can support more than 10s timeout at the maximum + * system counter frequency. + * The second watch period is set by WOR(32bit) which will be loaded + * automatically by hardware, when WS0 is triggered. + * This gives a maximum watch period of around 10s at the maximum + * system counter frequency. + * The System Counter shall run at maximum of 400MHz. + * More details: DEN0029B - Server Base System Architecture (SBSA) + * + * Kernel/API: P---------| pretimeout + * |-------------------------------T timeout + * SBSA GWDT: P--WOR---WS1 pretimeout + * |-------WCV----------WS0~~~~~~~~T timeout + */ + +#include <asm/arch_timer.h> + +#include <linux/acpi.h> +#include <linux/io.h> +#include <linux/interrupt.h> +#include <linux/module.h> +#include <linux/moduleparam.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/platform_device.h> +#include <linux/uaccess.h> +#include <linux/watchdog.h> + +/* SBSA Generic Watchdog register definitions */ +/* refresh frame */ +#define SBSA_GWDT_WRR 0x000 + +/* control frame */ +#define SBSA_GWDT_WCS 0x000 +#define SBSA_GWDT_WOR 0x008 +#define SBSA_GWDT_WCV_LO 0x010 +#define SBSA_GWDT_WCV_HI 0x014 + +/* refresh/control frame */ +#define SBSA_GWDT_W_IIDR 0xfcc +#define SBSA_GWDT_IDR 0xfd0 + +/* Watchdog Control and Status Register */ +#define SBSA_GWDT_WCS_EN BIT(0) +#define SBSA_GWDT_WCS_WS0 BIT(1) +#define SBSA_GWDT_WCS_WS1 BIT(2) + +/** + * struct sbsa_gwdt - Internal representation of the SBSA GWDT + * @wdd: kernel watchdog_device structure + * @clk: store the System Counter clock frequency, in Hz. + * @refresh_base: Virtual address of the watchdog refresh frame + * @control_base: Virtual address of the watchdog control frame + */ +struct sbsa_gwdt { + struct watchdog_device wdd; + u32 clk; + void __iomem *refresh_base; + void __iomem *control_base; +}; + +#define to_sbsa_gwdt(e) container_of(e, struct sbsa_gwdt, wdd) + +#define DEFAULT_TIMEOUT 10 /* seconds, the 1st + 2nd watch periods*/ +#define DEFAULT_PRETIMEOUT 5 /* seconds, the 2nd watch period*/ + +static unsigned int timeout; +module_param(timeout, uint, 0); +MODULE_PARM_DESC(timeout, + "Watchdog timeout in seconds. (>=0, default=" + __MODULE_STRING(DEFAULT_TIMEOUT) ")"); + +static unsigned int max_timeout = UINT_MAX; +module_param(max_timeout, uint, 0); +MODULE_PARM_DESC(max_timeout, + "Watchdog max timeout in seconds. (>=0, default=" + __MODULE_STRING(UINT_MAX) ")"); + +static unsigned int pretimeout; +module_param(pretimeout, uint, 0); +MODULE_PARM_DESC(pretimeout, + "Watchdog pretimeout in seconds. (>=0, default=" + __MODULE_STRING(DEFAULT_PRETIMEOUT) ")"); + +static unsigned int max_pretimeout = U32_MAX; +module_param(max_pretimeout, uint, 0); +MODULE_PARM_DESC(max_pretimeout, + "Watchdog max pretimeout in seconds. (>=0, default=" + __MODULE_STRING(U32_MAX) ")"); + +static bool nowayout = WATCHDOG_NOWAYOUT; +module_param(nowayout, bool, S_IRUGO); +MODULE_PARM_DESC(nowayout, + "Watchdog cannot be stopped once started (default=" + __MODULE_STRING(WATCHDOG_NOWAYOUT) ")"); + +/* + * Architected system timer support. + */ +static void sbsa_gwdt_cf_write(unsigned int reg, u32 val, + struct watchdog_device *wdd) +{ + struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd); + + writel_relaxed(val, gwdt->control_base + reg); +} + +static void sbsa_gwdt_rf_write(unsigned int reg, u32 val, + struct watchdog_device *wdd) +{ + struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd); + + writel_relaxed(val, gwdt->refresh_base + reg); +} + +static u32 sbsa_gwdt_cf_read(unsigned int reg, struct watchdog_device *wdd) +{ + struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd); + + return readl_relaxed(gwdt->control_base + reg); +} + +/* + * help founctions for accessing 64bit WCV register + */ +static u64 sbsa_gwdt_get_wcv(struct watchdog_device *wdd) +{ + u32 wcv_lo, wcv_hi; + + do { + wcv_hi = sbsa_gwdt_cf_read(SBSA_GWDT_WCV_HI, wdd); + wcv_lo = sbsa_gwdt_cf_read(SBSA_GWDT_WCV_LO, wdd); + } while (wcv_hi != sbsa_gwdt_cf_read(SBSA_GWDT_WCV_HI, wdd)); + + return (((u64)wcv_hi << 32) | wcv_lo); +} + +static void sbsa_gwdt_set_wcv(struct watchdog_device *wdd, u64 value) +{ + u32 wcv_lo, wcv_hi; + + wcv_lo = value & U32_MAX; + wcv_hi = (value >> 32) & U32_MAX; + + sbsa_gwdt_cf_write(SBSA_GWDT_WCV_HI, wcv_hi, wdd); + sbsa_gwdt_cf_write(SBSA_GWDT_WCV_LO, wcv_lo, wdd); +} + +static void reload_timeout_to_wcv(struct watchdog_device *wdd) +{ + struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd); + u64 wcv; + + wcv = arch_counter_get_cntvct() + + (u64)(wdd->timeout - wdd->pretimeout) * gwdt->clk; + + sbsa_gwdt_set_wcv(wdd, wcv); +} + +/* + * Use the following function to set the limit of timeout + * after updating pretimeout + */ +static void sbsa_gwdt_update_limits(struct watchdog_device *wdd) +{ + struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd); + u64 first_period_max = U64_MAX; + + do_div(first_period_max, gwdt->clk); + + wdd->min_timeout = wdd->pretimeout + 1; + wdd->max_timeout = min((uint)(wdd->pretimeout + first_period_max), + wdd->max_timeout); +} + +static int sbsa_gwdt_set_timeout(struct watchdog_device *wdd, + unsigned int timeout) +{ + wdd->timeout = timeout; + + return 0; +} + +static int sbsa_gwdt_set_pretimeout(struct watchdog_device *wdd, + unsigned int pretimeout) +{ + struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd); + u32 wor; + + wdd->pretimeout = pretimeout; + sbsa_gwdt_update_limits(wdd); + + if (!pretimeout) + /* gives sbsa_gwdt_start a chance to setup timeout */ + wor = gwdt->clk; + else + wor = pretimeout * gwdt->clk; + + /* refresh the WOR, that will cause an explicit watchdog refresh */ + sbsa_gwdt_cf_write(SBSA_GWDT_WOR, wor, wdd); + + return 0; +} + +static unsigned int sbsa_gwdt_get_timeleft(struct watchdog_device *wdd) +{ + struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd); + u64 timeleft = sbsa_gwdt_get_wcv(wdd) - arch_counter_get_cntvct(); + + do_div(timeleft, gwdt->clk); + + return timeleft; +} + +static int sbsa_gwdt_start(struct watchdog_device *wdd) +{ + /* Force refresh */ + sbsa_gwdt_rf_write(SBSA_GWDT_WRR, 0xc0ffee, wdd); + /* writing WCS will cause an explicit watchdog refresh */ + sbsa_gwdt_cf_write(SBSA_GWDT_WCS, SBSA_GWDT_WCS_EN, wdd); + + reload_timeout_to_wcv(wdd); + + return 0; +} + +static int sbsa_gwdt_stop(struct watchdog_device *wdd) +{ + /* Force refresh */ + sbsa_gwdt_rf_write(SBSA_GWDT_WRR, 0xc0ffee, wdd); + /* writing WCS will cause an explicit watchdog refresh */ + sbsa_gwdt_cf_write(SBSA_GWDT_WCS, 0, wdd); + + return 0; +} + +static int sbsa_gwdt_keepalive(struct watchdog_device *wdd) +{ + /* + * Writing WRR for an explicit watchdog refresh + * You can write anyting(like 0xc0ffee) + */ + sbsa_gwdt_rf_write(SBSA_GWDT_WRR, 0xc0ffee, wdd); + + reload_timeout_to_wcv(wdd); + + return 0; +} + +static irqreturn_t sbsa_gwdt_interrupt(int irq, void *dev_id) +{ + struct sbsa_gwdt *gwdt = (struct sbsa_gwdt *)dev_id; + struct watchdog_device *wdd = &gwdt->wdd; + u32 status; + + status = sbsa_gwdt_cf_read(SBSA_GWDT_WCS, wdd); + + if (status & SBSA_GWDT_WCS_WS0) + panic("SBSA Watchdog pre-timeout"); + + return IRQ_HANDLED; +} + +static struct watchdog_info sbsa_gwdt_info = { + .identity = "SBSA Generic Watchdog", + .options = WDIOF_SETTIMEOUT | + WDIOF_KEEPALIVEPING | + WDIOF_MAGICCLOSE | + WDIOF_PRETIMEOUT | + WDIOF_CARDRESET, +}; + +static struct watchdog_ops sbsa_gwdt_ops = { + .owner = THIS_MODULE, + .start = sbsa_gwdt_start, + .stop = sbsa_gwdt_stop, + .ping = sbsa_gwdt_keepalive, + .set_timeout = sbsa_gwdt_set_timeout, + .set_pretimeout = sbsa_gwdt_set_pretimeout, + .get_timeleft = sbsa_gwdt_get_timeleft, +}; + +static int sbsa_gwdt_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct sbsa_gwdt *gwdt; + struct watchdog_device *wdd; + struct resource *res; + void *rf_base, *cf_base; + int irq; + u32 clk, status; + int ret = 0; + u64 first_period_max = U64_MAX; + + /* + * Try to determine the frequency from the arch_timer interface + */ + clk = arch_timer_get_rate(); + if (!clk) { + dev_err(dev, "System Counter frequency not available\n"); + return -EINVAL; + } + + gwdt = devm_kzalloc(dev, sizeof(*gwdt), GFP_KERNEL); + if (!gwdt) + return -ENOMEM; + + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "refresh"); + rf_base = devm_ioremap_resource(dev, res); + if (IS_ERR(rf_base)) + return PTR_ERR(rf_base); + + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "control"); + cf_base = devm_ioremap_resource(dev, res); + if (IS_ERR(rf_base)) + return PTR_ERR(rf_base); + + irq = platform_get_irq_byname(pdev, "ws0"); + if (irq < 0) { + dev_err(dev, "unable to get ws0 interrupt.\n"); + return irq; + } + + gwdt->refresh_base = rf_base; + gwdt->control_base = cf_base; + gwdt->clk = clk; + + platform_set_drvdata(pdev, gwdt); + + wdd = &gwdt->wdd; + wdd->parent = dev; + wdd->info = &sbsa_gwdt_info; + wdd->ops = &sbsa_gwdt_ops; + watchdog_set_drvdata(wdd, gwdt); + watchdog_set_nowayout(wdd, nowayout); + + status = sbsa_gwdt_cf_read(SBSA_GWDT_WCS, wdd); + if (status & SBSA_GWDT_WCS_WS1) { + dev_warn(dev, "System reset by WDT(WCS: %x, WCV: %llx)\n", + status, sbsa_gwdt_get_wcv(wdd)); + wdd->bootstatus |= WDIOF_CARDRESET; + } + /* Check if watchdog is already enabled */ + if (status & SBSA_GWDT_WCS_EN) { + dev_warn(dev, "already enabled! WCS:0x%x\n", status); + sbsa_gwdt_keepalive(wdd); + } + + wdd->min_pretimeout = 0; + wdd->max_pretimeout = min(U32_MAX / clk, max_pretimeout); + wdd->min_timeout = 1; + do_div(first_period_max, clk); + wdd->max_timeout = min((uint)(wdd->pretimeout + first_period_max), + max_timeout); + + wdd->pretimeout = DEFAULT_PRETIMEOUT; + wdd->timeout = DEFAULT_TIMEOUT; + watchdog_init_timeouts(wdd, pretimeout, timeout, + sbsa_gwdt_update_limits, dev); + /* update pretimeout to WOR */ + sbsa_gwdt_cf_write(SBSA_GWDT_WOR, wdd->pretimeout * clk, wdd); + + ret = devm_request_irq(dev, irq, sbsa_gwdt_interrupt, IRQF_TIMER, + pdev->name, gwdt); + if (ret) { + dev_err(dev, "unable to request IRQ %d\n", irq); + return ret; + } + + ret = watchdog_register_device(wdd); + if (ret) + return ret; + + dev_info(dev, "Initialized with %ds timeout, %ds pretimeout @ %uHz\n", + wdd->timeout, wdd->pretimeout, gwdt->clk); + + return 0; +} + +static void sbsa_gwdt_shutdown(struct platform_device *pdev) +{ + struct sbsa_gwdt *gwdt = platform_get_drvdata(pdev); + + sbsa_gwdt_stop(&gwdt->wdd); +} + +static int sbsa_gwdt_remove(struct platform_device *pdev) +{ + struct sbsa_gwdt *gwdt = platform_get_drvdata(pdev); + int ret = 0; + + if (!nowayout) + ret = sbsa_gwdt_stop(&gwdt->wdd); + + watchdog_unregister_device(&gwdt->wdd); + + return ret; +} + +/* Disable watchdog if it is active during suspend */ +static int __maybe_unused sbsa_gwdt_suspend(struct device *dev) +{ + struct sbsa_gwdt *gwdt = dev_get_drvdata(dev); + + if (watchdog_active(&gwdt->wdd)) + sbsa_gwdt_stop(&gwdt->wdd); + + return 0; +} + +/* Enable watchdog and configure it if necessary */ +static int __maybe_unused sbsa_gwdt_resume(struct device *dev) +{ + struct sbsa_gwdt *gwdt = dev_get_drvdata(dev); + + if (watchdog_active(&gwdt->wdd)) + sbsa_gwdt_start(&gwdt->wdd); + + return 0; +} + +static const struct dev_pm_ops sbsa_gwdt_pm_ops = { + SET_SYSTEM_SLEEP_PM_OPS(sbsa_gwdt_suspend, sbsa_gwdt_resume) +}; + +static const struct of_device_id sbsa_gwdt_of_match[] = { + { .compatible = "arm,sbsa-gwdt", }, + {}, +}; +MODULE_DEVICE_TABLE(of, sbsa_gwdt_of_match); + +static const struct platform_device_id sbsa_gwdt_pdev_match[] = { + { .name = "sbsa-gwdt", }, + {}, +}; +MODULE_DEVICE_TABLE(platform, sbsa_gwdt_pdev_match); + +static struct platform_driver sbsa_gwdt_driver = { + .driver = { + .name = "sbsa-gwdt", + .pm = &sbsa_gwdt_pm_ops, + .of_match_table = sbsa_gwdt_of_match, + }, + .probe = sbsa_gwdt_probe, + .remove = sbsa_gwdt_remove, + .shutdown = sbsa_gwdt_shutdown, + .id_table = sbsa_gwdt_pdev_match, +}; + +module_platform_driver(sbsa_gwdt_driver); + +MODULE_DESCRIPTION("SBSA Generic Watchdog Driver"); +MODULE_VERSION("v1.0"); +MODULE_AUTHOR("Fu Wei <fu.wei@linaro.org>"); +MODULE_AUTHOR("Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>"); +MODULE_LICENSE("GPL v2");