From: Fu Wei fu.wei@linaro.org
This driver is designed for ARM AArch64 HW originally. Now It's tested on Foundation model with Linux watchdog deamon. It support two stages watch period, and will trigger a SPI after first timeout.
Fu Wei (1): Watchdog: add SBSA watchdog driver (1)use linux kernel watchdog framework (2)support getting info from ACPI GTDT table
drivers/watchdog/Kconfig | 10 + drivers/watchdog/Makefile | 1 + drivers/watchdog/sbsa_gwdt.c | 805 +++++++++++++++++++++++++++++++++++++++++++ include/linux/sbsa_gwdt.h | 134 +++++++ 4 files changed, 950 insertions(+) create mode 100644 drivers/watchdog/sbsa_gwdt.c create mode 100644 include/linux/sbsa_gwdt.h
From: Fu Wei fu.wei@linaro.org
--- drivers/watchdog/Kconfig | 10 + drivers/watchdog/Makefile | 1 + drivers/watchdog/sbsa_gwdt.c | 805 +++++++++++++++++++++++++++++++++++++++++++ include/linux/sbsa_gwdt.h | 134 +++++++ 4 files changed, 950 insertions(+)
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index 16f2023..26cd48f 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -152,6 +152,16 @@ 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 ARM64 && ARM_AMBA + select WATCHDOG_CORE + help + ARM SBSA Generic Watchdog timer. This has two Watchdog Signal(WS0/WS1), + will trigger a warnning interrupt when the first timeout is reached; + will reboot your system when the second timeout is reached. + More details: DEN0029B - Server Base System Architecture (SBSA) + config AT91RM9200_WATCHDOG tristate "AT91RM9200 watchdog" depends on SOC_AT91RM9200 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..fe773a3 --- /dev/null +++ b/drivers/watchdog/sbsa_gwdt.c @@ -0,0 +1,805 @@ +/* + * SBSA(Server Base System Architecture) Generic Watchdog driver + * + * Copyright (c) 2015, Linaro Ltd. + * Author: Fu Wei fu.wei@linaro.org + * + * 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. + * But 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 with the system counter + * at 400MHz. + * And the second watch period is set by WOR which will be loaded + * automatically by hardware, when WS0 is triggered. + * 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 <linux/module.h> +#include <linux/moduleparam.h> +#include <linux/uaccess.h> +#include <linux/io.h> +#include <linux/interrupt.h> +#include <linux/watchdog.h> +#include <linux/platform_device.h> +#include <linux/acpi.h> +#include <linux/sbsa_gwdt.h> + +#include <asm/arch_timer.h> + +/* For the second watch period, + * the timeout value comes from WOR(a 32bit wide. register). + * This gives a maximum watch period of around 10s + * at a maximum system counter frequency. + * + * The System Counter shall run at maximum of 400MHz. + * (Server Base System Architecture ARM-DEN-0029 Version 2.3) + */ +#define DEFAULT_TIMEOUT 15 /* seconds, the 1st + 2nd watch period*/ +#define DEFAULT_PRETIMEOUT 5 /* seconds, the 2nd watch period*/ + +/*store the System Counter clock frequency, in Hz.*/ +static u32 sbsa_gwdt_rate; + +static unsigned int timeout = DEFAULT_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 = DEFAULT_PRETIMEOUT; +module_param(pretimeout, uint, 0); +MODULE_PARM_DESC(pretimeout, + "Watchdog pretimeout in seconds. (>=0, default=" + __MODULE_STRING(DEFAULT_PRETIMEOUT) ")"); + +static int preaction = SBSA_GWDT_PREACT_NONE; +module_param(preaction, int, 0); +MODULE_PARM_DESC(preaction, + "The action which Kernel will take, once get WS0 (default=" + __MODULE_STRING(SBSA_GWDT_PREACT_NONE) ")"); + +static bool nowayout = WATCHDOG_NOWAYOUT; +module_param(nowayout, bool, 0); +MODULE_PARM_DESC(nowayout, + "Watchdog cannot be stopped once started (default=" + __MODULE_STRING(WATCHDOG_NOWAYOUT) ")"); + +/* + * Architected system timer support. + */ +static __always_inline +void sbsa_gwdt_reg_write(enum sbsa_gwdt_frame frame, unsigned int reg, u32 val, + struct watchdog_device *wdd) +{ + struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd); + struct sbsa_gwdt_device_data *device_data = &gwdt->device_data; + + switch (frame) { + case SBSA_GWDT_RF: + writel_relaxed(val, device_data->refresh_base + reg); + break; + case SBSA_GWDT_CF: + writel_relaxed(val, device_data->control_base + reg); + break; + default: + pr_err("sbsa_gwdt: reg write fail(frame not available) \n"); + } +} + +static __always_inline +u32 sbsa_gwdt_reg_read(enum sbsa_gwdt_frame frame, unsigned int reg, + struct watchdog_device *wdd) +{ + u32 val; + + struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd); + struct sbsa_gwdt_device_data *device_data = &gwdt->device_data; + + switch (frame) { + case SBSA_GWDT_RF: + val = readl_relaxed(device_data->refresh_base + reg); + break; + case SBSA_GWDT_CF: + val = readl_relaxed(device_data->control_base + reg); + break; + default: + pr_err("sbsa_gwdt: reg read fail(frame not available)\n"); + } + + return val; +} + +/* + * help founctions for accessing 64bit WCV register + * mutex_lock must be called prior to calling this function. + */ +static __always_inline u64 sbsa_gwdt_get_wcv(struct watchdog_device *wdd) +{ + u32 wcv_lo, wcv_hi; + + wcv_lo = sbsa_gwdt_reg_read(SBSA_GWDT_CF, SBSA_GWDT_WCV_LO, wdd); + wcv_hi = sbsa_gwdt_reg_read(SBSA_GWDT_CF, SBSA_GWDT_WCV_HI, wdd); + + return (((u64)wcv_hi << 32) | wcv_lo); +} + +void __always_inline sbsa_gwdt_set_wcv(struct watchdog_device *wdd, u64 value) +{ + u32 wcv_lo, wcv_hi; + wcv_lo = (u32)(value & U32_MAX); + wcv_hi = (u32)((value >> 32) & U32_MAX); + + sbsa_gwdt_reg_write(SBSA_GWDT_CF, SBSA_GWDT_WCV_LO, wcv_lo, wdd); + sbsa_gwdt_reg_write(SBSA_GWDT_CF, SBSA_GWDT_WCV_HI, wcv_hi, wdd); + + if (sbsa_gwdt_get_wcv(wdd) != value) + pr_err("sbsa_gwdt: set wcv fail!\n"); + + return; +} + +void __always_inline reload_timeout_to_wcv(struct watchdog_device *wdd) +{ + struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd); + u64 cntvct, wcv; + /* refresh the wcv */ + cntvct = arch_counter_get_cntvct(); + wcv = cntvct + (wdd->timeout - gwdt->pretimeout) * sbsa_gwdt_rate; + sbsa_gwdt_set_wcv(wdd, wcv); + + return; +} +/* Use the following function to set the limit of timeout + * after updating pretimeout */ +void __always_inline sbsa_gwdt_set_timeout_limits(struct sbsa_gwdt *gwdt) +{ + struct watchdog_device *wdd = &gwdt->wdd; + unsigned int first_period_max = (U64_MAX / sbsa_gwdt_rate); + + wdd->min_timeout = gwdt->pretimeout + 1; + wdd->max_timeout = min((gwdt->pretimeout + first_period_max), + max_timeout); + + return; +} + +/* Use the following function to check if a pretimeout value is invalid */ +static inline bool watchdog_pretimeout_invalid(struct sbsa_gwdt *gwdt, + unsigned int t) +{ + return ((gwdt->max_pretimeout != 0) && (t > gwdt->max_pretimeout)); +} + +static int sbsa_gwdt_set_timeout(struct watchdog_device *wdd, + unsigned int timeout) +{ + if (watchdog_timeout_invalid(wdd, timeout)) { + pr_err("sbsa_gwdt: timeout %d is out of range(%d~%d), unchanged\n", + timeout, wdd->min_timeout, wdd->max_timeout); + return -EINVAL; + } + wdd->timeout = timeout; + /* refresh the wcv */ + reload_timeout_to_wcv(wdd); + + 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; + + if (watchdog_pretimeout_invalid(gwdt, pretimeout)) { + pr_err("sbsa_gwdt: pretimeout %d is out of range(0~%d),skip\n", + pretimeout, gwdt->max_pretimeout); + return -EINVAL; + } + gwdt->pretimeout = pretimeout; + sbsa_gwdt_set_timeout_limits(gwdt); + + /* refresh the WOR, that will cause an explicit watchdog refresh */ + wor = pretimeout * sbsa_gwdt_rate; + sbsa_gwdt_reg_write(SBSA_GWDT_CF, SBSA_GWDT_WOR, wor, wdd); + + return 0; +} + +static int sbsa_gwdt_set_preaction(struct sbsa_gwdt *gwdt, + int preaction) +{ + if ((preaction >= SBSA_GWDT_PREACT_END) || + (preaction < SBSA_GWDT_PREACT_NONE)) { + pr_err("sbsa_gwdt: invalid preaction %d\n", preaction); + return -EINVAL; + } + gwdt->preaction = preaction; + return 0; +} + +static unsigned int sbsa_gwdt_get_timeleft(struct watchdog_device *wdd) +{ + u64 timeleft = sbsa_gwdt_get_wcv(wdd) - arch_counter_get_cntvct(); + + return (unsigned int)(timeleft / sbsa_gwdt_rate); +} + +static int sbsa_gwdt_start(struct watchdog_device *wdd) +{ + struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd); + struct sbsa_gwdt_device_data *device_data = &gwdt->device_data; + + u32 status; + int ret; + + ret = sbsa_gwdt_set_timeout(wdd, wdd->timeout); + if (ret) + return ret; + + mutex_lock(&device_data->lock); + status = sbsa_gwdt_reg_read(SBSA_GWDT_CF, SBSA_GWDT_WCS, wdd); + status |= SBSA_GWDT_WCS_EN; + /* writing WCS will cause an explicit watchdog refresh */ + sbsa_gwdt_reg_write(SBSA_GWDT_CF, SBSA_GWDT_WCS, status, wdd); + status = sbsa_gwdt_reg_read(SBSA_GWDT_CF, SBSA_GWDT_WCS, wdd); + mutex_unlock(&device_data->lock); + + /* Check if the watchdog was enabled */ + if (!(status & SBSA_GWDT_WCS_EN)) + return -EACCES; + + return 0; +} + +static int sbsa_gwdt_stop(struct watchdog_device *wdd) +{ + struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd); + struct sbsa_gwdt_device_data *device_data = &gwdt->device_data; + + u32 status; + + mutex_lock(&device_data->lock); + status = sbsa_gwdt_reg_read(SBSA_GWDT_CF, SBSA_GWDT_WCS, wdd); + status &= ~SBSA_GWDT_WCS_EN; + /* writing WCS will cause an explicit watchdog refresh */ + sbsa_gwdt_reg_write(SBSA_GWDT_CF, SBSA_GWDT_WCS, status, wdd); + status = sbsa_gwdt_reg_read(SBSA_GWDT_CF, SBSA_GWDT_WCS, wdd); + mutex_unlock(&device_data->lock); + + /* Check if the watchdog was disabled */ + if (status & SBSA_GWDT_WCS_EN) + return -EACCES; + + return 0; +} + +static int sbsa_gwdt_keepalive(struct watchdog_device *wdd) +{ + struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd); + struct sbsa_gwdt_device_data *device_data = &gwdt->device_data; + + mutex_lock(&device_data->lock); + /* Writing WRR for an explicit watchdog refresh + * You can write anyting(like 0xc0ffee) + */ + sbsa_gwdt_reg_write(SBSA_GWDT_RF, SBSA_GWDT_WRR, 0xc0ffee, wdd); + mutex_unlock(&device_data->lock); + + reload_timeout_to_wcv(wdd); + return 0; +} + +static long sbsa_gwdt_ioctl(struct watchdog_device *wdd, unsigned int cmd, + unsigned long arg) +{ + struct sbsa_gwdt *gwdt = watchdog_get_drvdata(wdd); + void __user *argp = (void __user *)arg; + int ret = -ENOIOCTLCMD; + unsigned int __user *p = argp; + unsigned int new_value; + int __user *preaction = argp; + int new_preaction; + /* FIXME: what else do we need here?? */ + switch (cmd) { + case WDIOC_SETPRETIMEOUT: + if (get_user(new_value, p)) + return -EFAULT; + ret = sbsa_gwdt_set_pretimeout(wdd, new_value); + break; + case WDIOC_GETPRETIMEOUT: + ret = put_user(gwdt->pretimeout, (unsigned int __user *)arg); + break; + case SBSA_GWDIOC_SETPREACTION: + if (get_user(new_preaction, preaction)) + return -EFAULT; + ret = sbsa_gwdt_set_preaction(gwdt, new_preaction); + break; + case SBSA_GWDIOC_GETPREACTION: + ret = put_user(gwdt->preaction, (int __user *)arg); + break; + } + + return ret; +} + +/* +static int sbsa_gwdt_ref(struct watchdog_device *wdd) +{ + struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd); + struct sbsa_gwdt_device_data *device_data = &gwdt->device_data; + + + return; +} + +static int sbsa_gwdt_unref(struct watchdog_device *wdd) +{ + struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd); + struct sbsa_gwdt_device_data *device_data = &gwdt->device_data; + + + return; +} +*/ + +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; + unsigned int left; + + pr_debug("SBSA Watchdog: in %s\n", __func__); + + status = sbsa_gwdt_reg_read(SBSA_GWDT_CF, SBSA_GWDT_WCS, wdd); + + if (status & SBSA_GWDT_WCS_WS0) { + if (printk_ratelimit()) { + pr_crit("SBSA Watchdog WS0 is triggered\n"); + left = sbsa_gwdt_get_timeleft(wdd); + pr_crit("please try to feed the dog in %ds!\n", left); + } + /* FIXME:anything we can do here? + * send_signal(sig, info, p, group); ?? + */ + if (gwdt->preaction == SBSA_GWDT_PREACT_FEEDDOG) { + sbsa_gwdt_keepalive(wdd); + pr_warn("kernel keep system running by feeding dog!\n"); + } else if (gwdt->preaction == SBSA_GWDT_PREACT_PANIC) { + 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, + .ioctl = sbsa_gwdt_ioctl, + .get_timeleft = sbsa_gwdt_get_timeleft, +/* + .status = sbsa_gwdt_status, + .ref = sbsa_gwdt_ref, + .unref = sbsa_gwdt_unref, +*/ +}; + + +static int sbsa_gwdt_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + + struct sbsa_gwdt *gwdt; + struct sbsa_gwdt_device_data *device_data; + + struct watchdog_device *wdd; + + struct resource *res; + void *rf_base = NULL, *cf_base = NULL; + int irq; + u32 status, flags; + u32 w_iidr, idr; + + int ret = 0; + + /* + * Try to determine the frequency from the cp15 interface + */ + sbsa_gwdt_rate = arch_timer_get_cntfrq(); + /* Check the system counter frequency. */ + if (!sbsa_gwdt_rate) + dev_err(dev, "System Counter frequency not available\n"); + + gwdt = devm_kzalloc(dev, sizeof(*gwdt), GFP_KERNEL); + if (!gwdt) + return -ENOMEM; + + wdd = &gwdt->wdd; + wdd->parent = dev; + device_data = &gwdt->device_data; + mutex_init(&device_data->lock); + + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, + SBSA_GWDT_RF_RES_NAME); + 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, + SBSA_GWDT_CF_RES_NAME); + cf_base = devm_ioremap_resource(dev, res); + if (IS_ERR(rf_base)) + return PTR_ERR(rf_base); + + res = platform_get_resource_byname(pdev, IORESOURCE_IRQ, + SBSA_GWDT_IRQ_RES_NAME); + if (!res) { + dev_err(dev, "required ws0 interrupt missing\n"); + return -ENXIO; + } + irq = res->start; + flags = res->flags; + + device_data->refresh_base = rf_base; + device_data->control_base = cf_base; + device_data->irq = irq; + device_data->flags = flags; + + ret = devm_request_irq(dev, irq, sbsa_gwdt_interrupt, + IRQF_TIMER, pdev->name, gwdt); + if (ret) { + dev_err(dev, "unable to request IRQ %d, disabling device\n", + irq); + return ret; + } + + /* get device information from refresh/control frame */ + w_iidr = sbsa_gwdt_reg_read(SBSA_GWDT_RF, SBSA_GWDT_W_IIDR, &gwdt->wdd); + idr = sbsa_gwdt_reg_read(SBSA_GWDT_RF, SBSA_GWDT_IDR, &gwdt->wdd); + dev_info(dev, "W_IIDR %x IDR %x in RF.\n", w_iidr, idr); + +/* + w_iidr = sbsa_gwdt_reg_read(SBSA_GWDT_CF, SBSA_GWDT_W_IIDR, &gwdt->wdd); + idr = sbsa_gwdt_reg_read(SBSA_GWDT_CF, SBSA_GWDT_IDR, &gwdt->wdd); + dev_info(dev, "sbsa_gwdt: W_IIDR %x IDR %x in CF.\n", w_iidr, idr); +*/ + + /* update device information to device_data for future use*/ + device_data->info.pid = SBSA_GWDT_W_IIDR_PID(w_iidr); + device_data->info.arch_ver = SBSA_GWDT_W_IIDR_ARCH_VER(w_iidr); + device_data->info.revision = SBSA_GWDT_W_IIDR_REV(w_iidr); + device_data->info.vid_bank = SBSA_GWDT_W_IIDR_VID_BANK(w_iidr); + device_data->info.vid = SBSA_GWDT_W_IIDR_VID(w_iidr); + + wdd->info = &sbsa_gwdt_info; +// wdd->info->firmware_version = w_iidr; + wdd->ops = &sbsa_gwdt_ops; + + watchdog_set_drvdata(wdd, gwdt); + +// mutex_lock(&device_data->lock); + status = sbsa_gwdt_reg_read(SBSA_GWDT_CF, SBSA_GWDT_WCS, wdd); +// mutex_unlock(&device_data->lock); + if (status & SBSA_GWDT_WCS_WS1) { + dev_warn(dev, "System was reseted by SBSA Watchdog, WCS %x\n", + status); + wdd->bootstatus |= WDIOF_CARDRESET; + gwdt->wcv_dump = sbsa_gwdt_get_wcv(wdd); + } + +// nowayout = false; + watchdog_set_nowayout(wdd, nowayout); + + gwdt->max_pretimeout = min((U32_MAX / sbsa_gwdt_rate), + (max_timeout - 1)); + gwdt->preaction = preaction; + sbsa_gwdt_set_pretimeout(wdd, pretimeout); + sbsa_gwdt_set_timeout(wdd, timeout); + + platform_set_drvdata(pdev, gwdt); + ret = watchdog_register_device(wdd); + if (ret) + return ret; + + /* Check if watchdog is already enabled */ + if (status & SBSA_GWDT_WCS_EN) { + dev_warn(dev, "already enabled!\n"); + sbsa_gwdt_keepalive(wdd); + } + + dev_info(dev, "registered with %ds timeout, %ds pretimeout\n", + wdd->timeout, gwdt->pretimeout); + + 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); + struct watchdog_device *wdd = &gwdt->wdd; + int ret = 0; + + if (!nowayout) + ret = sbsa_gwdt_stop(wdd); + watchdog_unregister_device(wdd); + + return ret; +} + +#ifdef CONFIG_PM +/* Disable watchdog if it is active during suspend */ +static int sbsa_gwdt_suspend(struct platform_device *pdev, + pm_message_t message) +{ + struct sbsa_gwdt *gwdt = platform_get_drvdata(pdev); + struct sbsa_gwdt_device_data *device_data = &gwdt->device_data; + struct watchdog_device *wdd = &gwdt->wdd; + + mutex_lock(&device_data->lock); + gwdt->pm_status_store = sbsa_gwdt_reg_read(SBSA_GWDT_CF, SBSA_GWDT_WCS, + wdd); + mutex_unlock(&device_data->lock); + + if (gwdt->pm_status_store & SBSA_GWDT_WCS_EN) + return sbsa_gwdt_stop(wdd); + + return 0; +} + +/* Enable watchdog and configure it if necessary */ +static int sbsa_gwdt_resume(struct platform_device *pdev) +{ + struct sbsa_gwdt *gwdt = platform_get_drvdata(pdev); + struct watchdog_device *wdd = &gwdt->wdd; + + /* + * If watchdog was stopped before suspend be sure it gets disabled + * again, for the case BIOS has enabled it during resume + */ + if (gwdt->pm_status_store & SBSA_GWDT_WCS_EN) + return sbsa_gwdt_start(wdd); + else + return sbsa_gwdt_stop(wdd); +} +#else +#define sbsa_gwdt_suspend NULL +#define sbsa_gwdt_resume NULL +#endif + +/* +static struct dev_pm_ops smc_drv_pm_ops = { + .suspend = sbsa_gwdt_suspend, + .resume = 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 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, + .suspend = sbsa_gwdt_suspend, + .resume = sbsa_gwdt_resume, + +}; + +module_platform_driver(sbsa_gwdt_driver); + +#ifdef CONFIG_ACPI +static int __init map_generic_timer_interrupt(u32 interrupt, u32 flags) +{ + int trigger, polarity; + + if (!interrupt) + return 0; + + trigger = (flags & ACPI_GTDT_INTERRUPT_MODE) ? ACPI_EDGE_SENSITIVE + : ACPI_LEVEL_SENSITIVE; + + polarity = (flags & ACPI_GTDT_INTERRUPT_POLARITY) ? ACPI_ACTIVE_LOW + : ACPI_ACTIVE_HIGH; + + return acpi_register_gsi(NULL, interrupt, trigger, polarity); +} + +static void __always_inline *acpi_gtdt_subtable(struct acpi_table_gtdt *gtdt) +{ + if (!gtdt->platform_timer_count) + return NULL; + + return ((void *)gtdt + (gtdt->platform_timer_offset)); +} +static int __always_inline acpi_gtdt_set_resource_irq_flags(u32 timer_flags) +{ + int flags; + + switch (timer_flags & + (ACPI_GTDT_INTERRUPT_MODE | ACPI_GTDT_INTERRUPT_POLARITY)) { + case (ACPI_GTDT_INTERRUPT_MODE | ACPI_GTDT_INTERRUPT_POLARITY): + flags = IORESOURCE_IRQ_LOWEDGE | IORESOURCE_IRQ; + pr_info("sbsa_gwdt: device irq flags is low edge"); + break; + case ACPI_GTDT_INTERRUPT_MODE: + flags = IORESOURCE_IRQ_HIGHEDGE | IORESOURCE_IRQ; + pr_info("sbsa_gwdt: device irq flags is high edge"); + break; + case ACPI_GTDT_INTERRUPT_POLARITY: + flags = IORESOURCE_IRQ_LOWLEVEL | IORESOURCE_IRQ; + pr_info("sbsa_gwdt: device irq flags is low level"); + break; + default: + flags = IORESOURCE_IRQ_HIGHLEVEL | IORESOURCE_IRQ; + pr_info("sbsa_gwdt: device irq flags is high level"); + } + pr_info("sbsa_gwdt: device irq flags:%d\n", flags); + return flags; +} + +static int __init sbsa_gwdt_add_platform_device(struct acpi_gtdt_header *table, + int index) +{ + struct acpi_gtdt_watchdog *watchdog; + int irq, gsi; + int err; + u32 flags; + void *rf_base_phy = NULL, *cf_base_phy = NULL; + struct platform_device *pdev; + struct resource *res; + + watchdog = container_of(table, struct acpi_gtdt_watchdog, header); + + /* get SBSA Generic Watchdog device information from GTDT */ + rf_base_phy = (void *)watchdog->refresh_frame_address; + cf_base_phy = (void *)watchdog->control_frame_address; + irq = watchdog->timer_interrupt; + flags = watchdog->timer_flags; + + pr_info("sbsa_gwdt: found a device @ 0x%p/0x%p irq:%d flags:%x\n", + rf_base_phy, cf_base_phy, irq, flags); + if (!rf_base_phy || !cf_base_phy || !irq) { + pr_err("sbsa_gwdt: fail to get the device info from GTDT.\n"); + return -EINVAL; + } + + gsi = map_generic_timer_interrupt(irq, flags); + + pdev = platform_device_alloc("sbsa_gwdt", index); + if (!pdev) + return -ENOMEM; + + res = kcalloc(SBSA_GWDT_RES_NUMBER, sizeof(*res), GFP_KERNEL); + if (!res) + goto err_free_device; + + res[SBSA_GWDT_RES_RF].start = (resource_size_t)rf_base_phy; + res[SBSA_GWDT_RES_RF].end = (resource_size_t)(rf_base_phy + SZ_64K - 1); + res[SBSA_GWDT_RES_RF].name = SBSA_GWDT_RF_RES_NAME; + res[SBSA_GWDT_RES_RF].flags = IORESOURCE_MEM; + + res[SBSA_GWDT_RES_CF].start = (resource_size_t)cf_base_phy; + res[SBSA_GWDT_RES_CF].end = (resource_size_t)(cf_base_phy + SZ_64K - 1); + res[SBSA_GWDT_RES_CF].name = SBSA_GWDT_CF_RES_NAME; + res[SBSA_GWDT_RES_CF].flags = IORESOURCE_MEM; + + res[SBSA_GWDT_RES_IRQ].start = res[SBSA_GWDT_RES_IRQ].end = gsi; + res[SBSA_GWDT_RES_IRQ].name = SBSA_GWDT_IRQ_RES_NAME; + res[SBSA_GWDT_RES_IRQ].flags = acpi_gtdt_set_resource_irq_flags(flags); + + err = platform_device_add_resources(pdev, res, SBSA_GWDT_RES_NUMBER); + if (err) + goto err_free_res; + + err = platform_device_add(pdev); + if (err) + goto err_free_res; + + return 0; + +err_free_res: + kfree(res); + +err_free_device: + platform_device_put(pdev); + return err; + +} + +/* Initialize SBSA generic Watchdog platform device info from GTDT */ +static int __init sbsa_gwdt_acpi_init(struct acpi_table_header *table) +{ + struct acpi_table_gtdt *gtdt; + int i, timer_count, gwdt_count = 0; + struct acpi_gtdt_header *header; + void __iomem *gtdt_subtable = NULL; + + gtdt = container_of(table, struct acpi_table_gtdt, header); + + timer_count = gtdt->platform_timer_count; + + gtdt_subtable = acpi_gtdt_subtable(gtdt); + if (!gtdt_subtable) { + pr_warn("sbsa gwdt: No Platform Timer structures!\n"); + return -EINVAL; + } + + for (i = 0; i < timer_count; i++) { + header = (struct acpi_gtdt_header *)gtdt_subtable; + if (header->type == ACPI_GTDT_TYPE_WATCHDOG) { + if (sbsa_gwdt_add_platform_device(header, i)) + pr_err("sbsa gwdt: adding sbsa_gwdt%d " + "platform device fail! subtable: %d\n", + gwdt_count, i); + gwdt_count++; + } + gtdt_subtable += header->length; + } + + return 0; +} + +/* Initialize the SBSA Generic Watchdog presented in GTDT */ +static int __init sbsa_gwdt_init(void) +{ + if (acpi_disabled) + return 0; + + return acpi_table_parse(ACPI_SIG_GTDT, sbsa_gwdt_acpi_init); +} + +arch_initcall(sbsa_gwdt_init); +#endif + +MODULE_DESCRIPTION("SBSA Generic Watchdog Driver"); +MODULE_AUTHOR("Fu Wei fu.wei@linaro.org"); +MODULE_LICENSE("GPL v2"); diff --git a/include/linux/sbsa_gwdt.h b/include/linux/sbsa_gwdt.h new file mode 100644 index 0000000..58425ef --- /dev/null +++ b/include/linux/sbsa_gwdt.h @@ -0,0 +1,134 @@ +/* + * SBSA(Server Base System Architecture) Generic Watchdog driver definitions + * + * Copyright (c) 2015, Linaro Ltd. + * Author: Fu Wei fu.wei@linaro.org + * + * 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. + */ + +#ifndef _SBSA_GENERIC_WATCHDOG_H_ +#define _SBSA_GENERIC_WATCHDOG_H_ + +/* SBSA Generic Watchdog register definitions */ +enum sbsa_gwdt_frame { + SBSA_GWDT_RF, + SBSA_GWDT_CF, +}; + +/* 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 (1 << 0) +#define SBSA_GWDT_WCS_WS0 (1 << 1) +#define SBSA_GWDT_WCS_WS1 (1 << 2) + +/* Watchdog Interface Identification Register */ +#define SBSA_GWDT_W_IIDR_PID(x) ((x >> 20) & 0xfff) +#define SBSA_GWDT_W_IIDR_ARCH_VER(x) ((x >> 16) & 0xf) +#define SBSA_GWDT_W_IIDR_REV(x) ((x >> 12) & 0xf) +#define SBSA_GWDT_W_IIDR_IMPLEMENTER(x) (x & 0xfff) +#define SBSA_GWDT_W_IIDR_VID_BANK(x) ((x >> 8) & 0xf) +#define SBSA_GWDT_W_IIDR_VID(x) (x & 0x7f) + +/* Watchdog Identification Register */ +#define SBSA_GWDT_IDR_W_PIDR2 (0xfe8) +#define SBSA_GWDT_IDR_W_PIDR2_ARCH_VER(x) ((x >> 4) & 0xf) + +#define SBSA_GWDT_RF_RES_NAME "refresh_frame" +#define SBSA_GWDT_CF_RES_NAME "control_frame" +#define SBSA_GWDT_IRQ_RES_NAME "wd0" + +#define SBSA_GWDT_WS1_KERNEL_KEEPER IS_BUILTIN(CONFIG_SBSA_GWDT_WS1_KERNEL_KEEPER) + +#define SBSA_GWD_IOCTL_BASE ('S') +#define SBSA_GWDIOC_GETPREACTION _IOR(SBSA_GWD_IOCTL_BASE, 0, int) +#define SBSA_GWDIOC_SETPREACTION _IOWR(SBSA_GWD_IOCTL_BASE, 1, int) + +enum { + SBSA_GWDT_RES_RF = 0, + SBSA_GWDT_RES_CF, + SBSA_GWDT_RES_IRQ, + SBSA_GWDT_RES_NUMBER, +}; + +/* Operations that can be performed on a pretimout. */ +enum { + SBSA_GWDT_PREACT_NONE = 0, /* just print a warning */ + SBSA_GWDT_PREACT_PANIC, /* trigger a panic to print some + * kernel info, if the kernel support + * kexec/kdump, this is more useful. + */ + SBSA_GWDT_PREACT_FEEDDOG, /* feed the dog, if kernel is alive. */ + SBSA_GWDT_PREACT_END, /* feed the dog, if kernel is alive. */ +}; + +/** + * struct sbsa_gwdt_info - SBSA Generic Watchdog device hardware information + * @pid: GWDT ProductID + * @arch_ver: GWDT architecture version + * @revision: GWDT revision number + * @vid_bank: GWDT The JEP106 continuation code of the implementer + * @vid: GWDT The JEP106 identity code of the implementer + * @refresh_pa: the watchdog refresh frame(PA) + * @control_pa: the watchdog control frame(PA) + */ +struct sbsa_gwdt_info { + unsigned int pid; + unsigned int arch_ver; + unsigned int revision; + unsigned int vid_bank; + unsigned int vid; + void *refresh_pa; + void *control_pa; +}; + +/** + * struct sbsa_gwdt_device_data - Internal representation of the SBSA GWDT + * @refresh_base: VA of the watchdog refresh frame + * @control_base: VA of the watchdog control frame + * @irq: Watchdog Timer GSIV (WS0) + * @flags: Watchdog Timer Flags + * @dev: Pointer to kernel device structure + * @info: SBSA Generic Watchdog info structure + * @lock: GWDT mutex + */ +struct sbsa_gwdt_device_data { + void __iomem *refresh_base; + void __iomem *control_base; + int irq; + u32 flags; + struct device *dev; + struct sbsa_gwdt_info info; + struct mutex lock; +}; + +struct sbsa_gwdt { + struct sbsa_gwdt_device_data device_data; + struct watchdog_device wdd; + unsigned int pretimeout; /* seconds */ +// unsigned int min_pretimeout; + unsigned int max_pretimeout; + int preaction; + u64 wcv_dump; +#ifdef CONFIG_PM + u8 pm_status_store; +#endif +}; + +#define to_sbsa_gwdt(e) container_of(e, struct sbsa_gwdt, wdd) + +#endif /* _SBSA_GENERIC_WATCHDOG_H_ */
On Wednesday 29 April 2015 01:49:53 fu.wei@linaro.org wrote:
From: Fu Wei fu.wei@linaro.org
This needs a changelog. Please explain what this is good for here.
+/*
- Architected system timer support.
- */
+static __always_inline
Just drop the __always_inline here, unless it's broken to do so (in which case you should add a comment). The compiler will inline static functions by itself, and you can use 'inline' to annotate the fact that you expect the function to be inlined.
+void sbsa_gwdt_reg_write(enum sbsa_gwdt_frame frame, unsigned int reg, u32 val,
struct watchdog_device *wdd)
+{
- struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
- struct sbsa_gwdt_device_data *device_data = &gwdt->device_data;
- switch (frame) {
- case SBSA_GWDT_RF:
writel_relaxed(val, device_data->refresh_base + reg);
break;
- case SBSA_GWDT_CF:
writel_relaxed(val, device_data->control_base + reg);
break;
- default:
pr_err("sbsa_gwdt: reg write fail(frame not available) \n");
- }
+}
[coding style]
How about having two separate functions to do this, it seems that would simplify both the wrapper and its callers.
+static long sbsa_gwdt_ioctl(struct watchdog_device *wdd, unsigned int cmd,
unsigned long arg)
+{
- struct sbsa_gwdt *gwdt = watchdog_get_drvdata(wdd);
- void __user *argp = (void __user *)arg;
- int ret = -ENOIOCTLCMD;
- unsigned int __user *p = argp;
- unsigned int new_value;
- int __user *preaction = argp;
- int new_preaction;
- /* FIXME: what else do we need here?? */
Answer: as little as possible.
- switch (cmd) {
- case WDIOC_SETPRETIMEOUT:
if (get_user(new_value, p))
return -EFAULT;
ret = sbsa_gwdt_set_pretimeout(wdd, new_value);
break;
- case WDIOC_GETPRETIMEOUT:
ret = put_user(gwdt->pretimeout, (unsigned int __user *)arg);
break;
- case SBSA_GWDIOC_SETPREACTION:
if (get_user(new_preaction, preaction))
return -EFAULT;
ret = sbsa_gwdt_set_preaction(gwdt, new_preaction);
break;
- case SBSA_GWDIOC_GETPREACTION:
ret = put_user(gwdt->preaction, (int __user *)arg);
break;
- }
- return ret;
+}
Don't introduce driver specific commands here. Instead, make a separate patch to add new generic callbacks so the user API is shared with all drivers.
It would be helpful to add support for the WDIOC_SETPRETIMEOUT and WDIOC_GETPRETIMEOUT calls in the watchdog core as well.
+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;
- unsigned int left;
- pr_debug("SBSA Watchdog: in %s\n", __func__);
- status = sbsa_gwdt_reg_read(SBSA_GWDT_CF, SBSA_GWDT_WCS, wdd);
- if (status & SBSA_GWDT_WCS_WS0) {
if (printk_ratelimit()) {
pr_crit("SBSA Watchdog WS0 is triggered\n");
left = sbsa_gwdt_get_timeleft(wdd);
pr_crit("please try to feed the dog in %ds!\n", left);
}
/* FIXME:anything we can do here?
* send_signal(sig, info, p, group); ??
*/
if (gwdt->preaction == SBSA_GWDT_PREACT_FEEDDOG) {
sbsa_gwdt_keepalive(wdd);
pr_warn("kernel keep system running by feeding dog!\n");
What is the point of this? Is there any realistic use case for setting SBSA_GWDT_PREACT_FEEDDOG?
} else if (gwdt->preaction == SBSA_GWDT_PREACT_PANIC) {
panic("SBSA Watchdog pre-timeout");
}
Same question here: what is this good for?
+static int sbsa_gwdt_probe(struct platform_device *pdev) +{
- struct device *dev = &pdev->dev;
- struct sbsa_gwdt *gwdt;
- struct sbsa_gwdt_device_data *device_data;
- struct watchdog_device *wdd;
- struct resource *res;
- void *rf_base = NULL, *cf_base = NULL;
Don't initialize local variables to NULL unless you have to use that value later.
- res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
SBSA_GWDT_RF_RES_NAME);
- 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,
SBSA_GWDT_CF_RES_NAME);
- cf_base = devm_ioremap_resource(dev, res);
- if (IS_ERR(rf_base))
return PTR_ERR(rf_base);
- res = platform_get_resource_byname(pdev, IORESOURCE_IRQ,
SBSA_GWDT_IRQ_RES_NAME);
Please open-code the resource names here, the macros make it harder to read what you do here.
Use platform_get_irq() here to allow deferred probing of the irqchip.
+static const struct of_device_id sbsa_gwdt_of_match[] = {
- { .compatible = "arm,sbsa_gwdt", },
- {},
+}; +MODULE_DEVICE_TABLE(of, sbsa_gwdt_of_match);
Please add a binding document for this that describes the mandatory properties and resources.
+#ifdef CONFIG_ACPI +static int __init map_generic_timer_interrupt(u32 interrupt, u32 flags) +{
- int trigger, polarity;
The ACPI parts of this patch should probably be in a separate file, and submitted as a separate patch.
+static int __always_inline acpi_gtdt_set_resource_irq_flags(u32 timer_flags) +{
- int flags;
- switch (timer_flags &
(ACPI_GTDT_INTERRUPT_MODE | ACPI_GTDT_INTERRUPT_POLARITY)) {
- case (ACPI_GTDT_INTERRUPT_MODE | ACPI_GTDT_INTERRUPT_POLARITY):
flags = IORESOURCE_IRQ_LOWEDGE | IORESOURCE_IRQ;
pr_info("sbsa_gwdt: device irq flags is low edge");
break;
- case ACPI_GTDT_INTERRUPT_MODE:
flags = IORESOURCE_IRQ_HIGHEDGE | IORESOURCE_IRQ;
pr_info("sbsa_gwdt: device irq flags is high edge");
break;
- case ACPI_GTDT_INTERRUPT_POLARITY:
flags = IORESOURCE_IRQ_LOWLEVEL | IORESOURCE_IRQ;
pr_info("sbsa_gwdt: device irq flags is low level");
break;
- default:
flags = IORESOURCE_IRQ_HIGHLEVEL | IORESOURCE_IRQ;
pr_info("sbsa_gwdt: device irq flags is high level");
- }
- pr_info("sbsa_gwdt: device irq flags:%d\n", flags);
- return flags;
+}
This all looks like code that should be handled by the ACPI core, and not be driver specific.
+/* Initialize the SBSA Generic Watchdog presented in GTDT */ +static int __init sbsa_gwdt_init(void) +{
- if (acpi_disabled)
return 0;
- return acpi_table_parse(ACPI_SIG_GTDT, sbsa_gwdt_acpi_init);
+}
+arch_initcall(sbsa_gwdt_init); +#endif
You can't have arch_initcall() and module_platform_driver() in the same file, it prevents you from making the driver a loadable module.
diff --git a/include/linux/sbsa_gwdt.h b/include/linux/sbsa_gwdt.h new file mode 100644 index 0000000..58425ef --- /dev/null +++ b/include/linux/sbsa_gwdt.h +/* 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 (1 << 0) +#define SBSA_GWDT_WCS_WS0 (1 << 1) +#define SBSA_GWDT_WCS_WS1 (1 << 2)
Put all the register definitions into the driver itself. Only put things in a global header that need to be shared between multiple drivers. In case of a watchdog driver, that should be nothing.
+#define SBSA_GWDT_WS1_KERNEL_KEEPER IS_BUILTIN(CONFIG_SBSA_GWDT_WS1_KERNEL_KEEPER)
+#define SBSA_GWD_IOCTL_BASE ('S') +#define SBSA_GWDIOC_GETPREACTION _IOR(SBSA_GWD_IOCTL_BASE, 0, int) +#define SBSA_GWDIOC_SETPREACTION _IOWR(SBSA_GWD_IOCTL_BASE, 1, int)
ioctl definitions have to be in include/uapi/linux/ so they are visible to user space programs. Just put them into include/uapi/linux/watchdog.h and remove the SBSA_ prefix in the command and data structure names to make it easier to reuse these for other drivers.
Arnd
Hi Arnd,
First of all, Great thanks for reviewing my patch! I greatly appreciate it. all the feedback and comment inline below :
On 29 April 2015 at 03:18, Arnd Bergmann arnd@arndb.de wrote:
On Wednesday 29 April 2015 01:49:53 fu.wei@linaro.org wrote:
From: Fu Wei fu.wei@linaro.org
This needs a changelog. Please explain what this is good for here.
Thanks , will do
+/*
- Architected system timer support.
- */
+static __always_inline
Just drop the __always_inline here, unless it's broken to do so (in which case you should add a comment). The compiler will inline static functions by itself, and you can use 'inline' to annotate the fact that you expect the function to be inlined.
Thanks, fixed it. :-)
+void sbsa_gwdt_reg_write(enum sbsa_gwdt_frame frame, unsigned int reg,
u32 val,
struct watchdog_device *wdd)
+{
struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
struct sbsa_gwdt_device_data *device_data = &gwdt->device_data;
switch (frame) {
case SBSA_GWDT_RF:
writel_relaxed(val, device_data->refresh_base + reg);
break;
case SBSA_GWDT_CF:
writel_relaxed(val, device_data->control_base + reg);
break;
default:
pr_err("sbsa_gwdt: reg write fail(frame not available)
\n");
}
+}
[coding style]
How about having two separate functions to do this, it seems that would simplify both the wrapper and its callers.
yes, good idea, fixed it :-)
+static long sbsa_gwdt_ioctl(struct watchdog_device *wdd, unsigned int
cmd,
unsigned long arg)
+{
struct sbsa_gwdt *gwdt = watchdog_get_drvdata(wdd);
void __user *argp = (void __user *)arg;
int ret = -ENOIOCTLCMD;
unsigned int __user *p = argp;
unsigned int new_value;
int __user *preaction = argp;
int new_preaction;
/* FIXME: what else do we need here?? */
Answer: as little as possible.
Ok, Thanks
switch (cmd) {
case WDIOC_SETPRETIMEOUT:
if (get_user(new_value, p))
return -EFAULT;
ret = sbsa_gwdt_set_pretimeout(wdd, new_value);
break;
case WDIOC_GETPRETIMEOUT:
ret = put_user(gwdt->pretimeout, (unsigned int __user
*)arg);
break;
case SBSA_GWDIOC_SETPREACTION:
if (get_user(new_preaction, preaction))
return -EFAULT;
ret = sbsa_gwdt_set_preaction(gwdt, new_preaction);
break;
case SBSA_GWDIOC_GETPREACTION:
ret = put_user(gwdt->preaction, (int __user *)arg);
break;
}
return ret;
+}
Don't introduce driver specific commands here. Instead, make a separate patch to add new generic callbacks so the user API is shared with all drivers.
yes, that makes sense, I will make a separate patch for that, how about sysfs interface?
It would be helpful to add support for the WDIOC_SETPRETIMEOUT and WDIOC_GETPRETIMEOUT calls in the watchdog core as well.
yes, there is a define in watchdog.h, but haven't been implemented in watchdog_dev.c only the kempld_wdt.c (which has two stages timeout also)use this , so I do the same way.
So for this driver, I this we can keep WDIOC_*PRETIMEOUT, and move SBSA_GWDIOC_*PREACTION to somewhere else(maybe remove?)
any suggestion ? thought?
+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;
unsigned int left;
pr_debug("SBSA Watchdog: in %s\n", __func__);
status = sbsa_gwdt_reg_read(SBSA_GWDT_CF, SBSA_GWDT_WCS, wdd);
if (status & SBSA_GWDT_WCS_WS0) {
if (printk_ratelimit()) {
pr_crit("SBSA Watchdog WS0 is triggered\n");
left = sbsa_gwdt_get_timeleft(wdd);
pr_crit("please try to feed the dog in %ds!\n", left);
}
/* FIXME:anything we can do here?
* send_signal(sig, info, p, group); ??
*/
if (gwdt->preaction == SBSA_GWDT_PREACT_FEEDDOG) {
sbsa_gwdt_keepalive(wdd);
pr_warn("kernel keep system running by feeding
dog!\n");
What is the point of this? Is there any realistic use case for setting SBSA_GWDT_PREACT_FEEDDOG?
} else if (gwdt->preaction == SBSA_GWDT_PREACT_PANIC) {
panic("SBSA Watchdog pre-timeout");
}
Same question here: what is this good for?
I am not sure what is that best operation to do in this interrupt routine, so I offer some options to user. but we can discuss this and just keep one operation here(so we can delete SBSA_GWDIOC_*PREACTION as well)
now , we have tree option: (1) just a warning to tell user space : you need to feed the dog ( but what is the best way to sent this message? by signal ?) (2) feed the dog in the driver, this make sure kernel is alive, if not, WS1 will be triggered (3)panic() (if we have kdump, we can tell what is wrong with kernel or app), backup and reboot. by this way , we have time to backup important date and debug instead of reset directly like normal WDT.
+static int sbsa_gwdt_probe(struct platform_device *pdev) +{
struct device *dev = &pdev->dev;
struct sbsa_gwdt *gwdt;
struct sbsa_gwdt_device_data *device_data;
struct watchdog_device *wdd;
struct resource *res;
void *rf_base = NULL, *cf_base = NULL;
Don't initialize local variables to NULL unless you have to use that value later.
OK, thanks , fixed it
res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
SBSA_GWDT_RF_RES_NAME);
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,
SBSA_GWDT_CF_RES_NAME);
cf_base = devm_ioremap_resource(dev, res);
if (IS_ERR(rf_base))
return PTR_ERR(rf_base);
res = platform_get_resource_byname(pdev, IORESOURCE_IRQ,
SBSA_GWDT_IRQ_RES_NAME);
Please open-code the resource names here, the macros make it harder to read what you do here.
OK, thanks , fixed it
Use platform_get_irq() here to allow deferred probing of the irqchip.
I tried to used that API before, but I found out I need not only a irq number, but also flags, so I turn to use resource. any thing we can do to improve this ? any thought?
+static const struct of_device_id sbsa_gwdt_of_match[] = {
{ .compatible = "arm,sbsa_gwdt", },
{},
+}; +MODULE_DEVICE_TABLE(of, sbsa_gwdt_of_match);
Please add a binding document for this that describes the mandatory properties and resources.
OK , will do in this patchset, right?
+#ifdef CONFIG_ACPI +static int __init map_generic_timer_interrupt(u32 interrupt, u32 flags) +{
int trigger, polarity;
The ACPI parts of this patch should probably be in a separate file, and submitted as a separate patch.
OK, np, I can separate it . :-) I put this code here , because only this driver will use SBSA watchdog info in GTDT for now, but I am happy if we can move this code to ACPI driver :-)
+static int __always_inline acpi_gtdt_set_resource_irq_flags(u32
timer_flags)
+{
int flags;
switch (timer_flags &
(ACPI_GTDT_INTERRUPT_MODE | ACPI_GTDT_INTERRUPT_POLARITY))
{
case (ACPI_GTDT_INTERRUPT_MODE | ACPI_GTDT_INTERRUPT_POLARITY):
flags = IORESOURCE_IRQ_LOWEDGE | IORESOURCE_IRQ;
pr_info("sbsa_gwdt: device irq flags is low edge");
break;
case ACPI_GTDT_INTERRUPT_MODE:
flags = IORESOURCE_IRQ_HIGHEDGE | IORESOURCE_IRQ;
pr_info("sbsa_gwdt: device irq flags is high edge");
break;
case ACPI_GTDT_INTERRUPT_POLARITY:
flags = IORESOURCE_IRQ_LOWLEVEL | IORESOURCE_IRQ;
pr_info("sbsa_gwdt: device irq flags is low level");
break;
default:
flags = IORESOURCE_IRQ_HIGHLEVEL | IORESOURCE_IRQ;
pr_info("sbsa_gwdt: device irq flags is high level");
}
pr_info("sbsa_gwdt: device irq flags:%d\n", flags);
return flags;
+}
This all looks like code that should be handled by the ACPI core, and not be driver specific.
yes, you are right , this is a draft, so I think the best way to do is move this acpi code to acpi driver. any thought?
+/* Initialize the SBSA Generic Watchdog presented in GTDT */ +static int __init sbsa_gwdt_init(void) +{
if (acpi_disabled)
return 0;
return acpi_table_parse(ACPI_SIG_GTDT, sbsa_gwdt_acpi_init);
+}
+arch_initcall(sbsa_gwdt_init); +#endif
You can't have arch_initcall() and module_platform_driver() in the same file, it prevents you from making the driver a loadable module.
diff --git a/include/linux/sbsa_gwdt.h b/include/linux/sbsa_gwdt.h new file mode 100644 index 0000000..58425ef --- /dev/null +++ b/include/linux/sbsa_gwdt.h +/* 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 (1 << 0) +#define SBSA_GWDT_WCS_WS0 (1 << 1) +#define SBSA_GWDT_WCS_WS1 (1 << 2)
Put all the register definitions into the driver itself. Only put things in a global header that need to be shared between multiple drivers. In case of a watchdog driver, that should be nothing.
yes, good point , Thanks !
+#define SBSA_GWDT_WS1_KERNEL_KEEPER
IS_BUILTIN(CONFIG_SBSA_GWDT_WS1_KERNEL_KEEPER)
+#define SBSA_GWD_IOCTL_BASE ('S') +#define SBSA_GWDIOC_GETPREACTION
_IOR(SBSA_GWD_IOCTL_BASE, 0, int)
+#define SBSA_GWDIOC_SETPREACTION
_IOWR(SBSA_GWD_IOCTL_BASE, 1, int)
ioctl definitions have to be in include/uapi/linux/ so they are visible to user space programs. Just put them into include/uapi/linux/watchdog.h and remove the SBSA_ prefix in the command and data structure names to make it easier to reuse these for other drivers.
yes, good idea. but just like I said , I am not sure if we need this "PREACTION" code, if so , I will do this way.
Great thanks for your help, Arnd, and some of your suggestion will go into my next patch(V2). but we still need to decide : (1) do we need "PREACTION" code to give user option for the first timeout (2) what is the best way to handle WS0 ? (3) move ACPI GTDT code to ACPI driver, is that OK for everyone?
any thought and suggestion is most welcome! Great thanks for your time to review my draft patch.
Arnd
On Friday 01 May 2015 16:22:08 Fu Wei wrote:
switch (cmd) {
case WDIOC_SETPRETIMEOUT:
if (get_user(new_value, p))
return -EFAULT;
ret = sbsa_gwdt_set_pretimeout(wdd, new_value);
break;
case WDIOC_GETPRETIMEOUT:
ret = put_user(gwdt->pretimeout, (unsigned int __user
*)arg);
break;
case SBSA_GWDIOC_SETPREACTION:
if (get_user(new_preaction, preaction))
return -EFAULT;
ret = sbsa_gwdt_set_preaction(gwdt, new_preaction);
break;
case SBSA_GWDIOC_GETPREACTION:
ret = put_user(gwdt->preaction, (int __user *)arg);
break;
}
return ret;
+}
Don't introduce driver specific commands here. Instead, make a separate patch to add new generic callbacks so the user API is shared with all drivers.
yes, that makes sense, I will make a separate patch for that, how about sysfs interface?
I think an ioctl interface makes most sense here if we want something to be configured, for consistency with the existing interfaces.
However, we should try to avoid adding driver specific interfaces for anything that can be part of the core.
It would be helpful to add support for the WDIOC_SETPRETIMEOUT and WDIOC_GETPRETIMEOUT calls in the watchdog core as well.
yes, there is a define in watchdog.h, but haven't been implemented in watchdog_dev.c only the kempld_wdt.c (which has two stages timeout also)use this , so I do the same way.
So for this driver, I this we can keep WDIOC_*PRETIMEOUT, and move SBSA_GWDIOC_*PREACTION to somewhere else(maybe remove?)
any suggestion ? thought?
I see two drivers that implement WDIOC_SETPRETIMEOUT. My guess is that the previous authors did not see it worthwhile to implement it in the core if there are only one or two users. However, now that there are three drivers including this one, it's clear that the support belongs into the core with a separate callback instead of a custom ioctl implementation.
+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;
unsigned int left;
pr_debug("SBSA Watchdog: in %s\n", __func__);
status = sbsa_gwdt_reg_read(SBSA_GWDT_CF, SBSA_GWDT_WCS, wdd);
if (status & SBSA_GWDT_WCS_WS0) {
if (printk_ratelimit()) {
pr_crit("SBSA Watchdog WS0 is triggered\n");
left = sbsa_gwdt_get_timeleft(wdd);
pr_crit("please try to feed the dog in %ds!\n", left);
}
/* FIXME:anything we can do here?
* send_signal(sig, info, p, group); ??
*/
if (gwdt->preaction == SBSA_GWDT_PREACT_FEEDDOG) {
sbsa_gwdt_keepalive(wdd);
pr_warn("kernel keep system running by feeding
dog!\n");
What is the point of this? Is there any realistic use case for setting SBSA_GWDT_PREACT_FEEDDOG?
} else if (gwdt->preaction == SBSA_GWDT_PREACT_PANIC) {
panic("SBSA Watchdog pre-timeout");
}
Same question here: what is this good for?
I am not sure what is that best operation to do in this interrupt routine, so I offer some options to user. but we can discuss this and just keep one operation here(so we can delete SBSA_GWDIOC_*PREACTION as well)
now , we have tree option: (1) just a warning to tell user space : you need to feed the dog ( but what is the best way to sent this message? by signal ?)
This is very unlikely to ever be seen by a real user, and the message will otherwise just be lost.
(2) feed the dog in the driver, this make sure kernel is alive, if not, WS1 will be triggered
This seems counterproductive, because it prevents the watchdog hardware to do its job of rebooting the system when user space is unresponsive.
(3)panic() (if we have kdump, we can tell what is wrong with kernel or app), backup and reboot. by this way , we have time to backup important date and debug instead of reset directly like normal WDT.
I can see value of this one, but if the other two are not needed, we can avoid adding an interface to configure the behavior and always do it this way if the pretimeout is set, or default to doing nothing when it is not set.
Use platform_get_irq() here to allow deferred probing of the irqchip.
I tried to used that API before, but I found out I need not only a irq number, but also flags, so I turn to use resource. any thing we can do to improve this ? any thought?
I don't see where in your driver you actually use the flags, but I also don't fully understand the ACPI portion. It seems that the probe function just sets a variable in the watchdog specific structure, but that one is never used.
+static const struct of_device_id sbsa_gwdt_of_match[] = {
{ .compatible = "arm,sbsa_gwdt", },
{},
+}; +MODULE_DEVICE_TABLE(of, sbsa_gwdt_of_match);
Please add a binding document for this that describes the mandatory properties and resources.
OK , will do in this patchset, right?
Yes, but as a separate patch.
Great thanks for your help, Arnd, and some of your suggestion will go into my next patch(V2). but we still need to decide : (1) do we need "PREACTION" code to give user option for the first timeout
My impression is that we should leave it out for now. Adding user space interfaces is complicated, and if someone absolutely needs it later, we can add it then.
Arnd
Hi Arnd,
Great thanks for your feedback all the feedback and comment inline below :
On 1 May 2015 at 19:26, Arnd Bergmann arnd@arndb.de wrote:
On Friday 01 May 2015 16:22:08 Fu Wei wrote:
switch (cmd) {
case WDIOC_SETPRETIMEOUT:
if (get_user(new_value, p))
return -EFAULT;
ret = sbsa_gwdt_set_pretimeout(wdd, new_value);
break;
case WDIOC_GETPRETIMEOUT:
ret = put_user(gwdt->pretimeout, (unsigned int __user
*)arg);
break;
case SBSA_GWDIOC_SETPREACTION:
if (get_user(new_preaction, preaction))
return -EFAULT;
ret = sbsa_gwdt_set_preaction(gwdt, new_preaction);
break;
case SBSA_GWDIOC_GETPREACTION:
ret = put_user(gwdt->preaction, (int __user *)arg);
break;
}
return ret;
+}
Don't introduce driver specific commands here. Instead, make a separate patch to add new generic callbacks so the user API is shared with all
drivers.
yes, that makes sense, I will make a separate patch for that, how about sysfs interface?
I think an ioctl interface makes most sense here if we want something to be configured, for consistency with the existing interfaces.
However, we should try to avoid adding driver specific interfaces for anything that can be part of the core.
OK, let's just delete PREACTION first :-)
It would be helpful to add support for the WDIOC_SETPRETIMEOUT and WDIOC_GETPRETIMEOUT calls in the watchdog core as well.
yes, there is a define in watchdog.h, but haven't been implemented in watchdog_dev.c only the kempld_wdt.c (which has two stages timeout also)use this , so I
do
the same way.
So for this driver, I this we can keep WDIOC_*PRETIMEOUT, and move SBSA_GWDIOC_*PREACTION to somewhere else(maybe remove?)
any suggestion ? thought?
I see two drivers that implement WDIOC_SETPRETIMEOUT. My guess is that the previous authors did not see it worthwhile to implement it in the core if there are only one or two users. However, now that there are three drivers including this one, it's clear that the support belongs into the core with a separate callback instead of a custom ioctl implementation.
yes, I think, we can make a separate patch for this later to clear it.
+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;
unsigned int left;
pr_debug("SBSA Watchdog: in %s\n", __func__);
status = sbsa_gwdt_reg_read(SBSA_GWDT_CF, SBSA_GWDT_WCS, wdd);
if (status & SBSA_GWDT_WCS_WS0) {
if (printk_ratelimit()) {
pr_crit("SBSA Watchdog WS0 is triggered\n");
left = sbsa_gwdt_get_timeleft(wdd);
pr_crit("please try to feed the dog in %ds!\n",
left);
}
/* FIXME:anything we can do here?
* send_signal(sig, info, p, group); ??
*/
if (gwdt->preaction == SBSA_GWDT_PREACT_FEEDDOG) {
sbsa_gwdt_keepalive(wdd);
pr_warn("kernel keep system running by feeding
dog!\n");
What is the point of this? Is there any realistic use case for setting SBSA_GWDT_PREACT_FEEDDOG?
} else if (gwdt->preaction == SBSA_GWDT_PREACT_PANIC) {
panic("SBSA Watchdog pre-timeout");
}
Same question here: what is this good for?
I am not sure what is that best operation to do in this interrupt
routine,
so I offer some options to user. but we can discuss this and just keep
one
operation here(so we can delete SBSA_GWDIOC_*PREACTION as well)
now , we have tree option: (1) just a warning to tell user space : you need to feed the dog ( but
what
is the best way to sent this message? by signal ?)
This is very unlikely to ever be seen by a real user, and the message will otherwise just be lost.
(2) feed the dog in the driver, this make sure kernel is alive, if not,
WS1
will be triggered
This seems counterproductive, because it prevents the watchdog hardware to do its job of rebooting the system when user space is unresponsive.
the purpose to do this way is : if kernel can still feed the dog , that means kernel is still alive, app is dead. so we can just kill the app, and keep kernel running for a while. but user need to be warned do you think it is make sense ?
(3)panic() (if we have kdump, we can tell what is wrong with kernel or app), backup and reboot. by this way , we have time to backup important date and debug instead of reset directly like normal WDT.
I can see value of this one, but if the other two are not needed, we can avoid adding an interface to configure the behavior and always do it this way if the pretimeout is set, or default to doing nothing when it is not set.
Good idea , let's do this way first ?
Use platform_get_irq() here to allow deferred probing of the irqchip.
I tried to used that API before, but I found out I need not only a irq number, but also flags, so I turn to use resource. any thing we can do to improve this ? any thought?
I don't see where in your driver you actually use the flags, but I also don't fully understand the ACPI portion. It seems that the probe function just sets a variable in the watchdog specific structure, but that one is never used.
yes, for now , It's just for ACPI, but maybe we need the flags to config GIC? so maybe I do this way first, then if the flags are unnecessary in the future, I will turn to use platform_get_irq() .
+static const struct of_device_id sbsa_gwdt_of_match[] = {
{ .compatible = "arm,sbsa_gwdt", },
{},
+}; +MODULE_DEVICE_TABLE(of, sbsa_gwdt_of_match);
Please add a binding document for this that describes the mandatory properties and resources.
OK , will do in this patchset, right?
Yes, but as a separate patch.
yes, done in my v2
Great thanks for your help, Arnd, and some of your suggestion will go
into
my next patch(V2). but we still need to decide : (1) do we need "PREACTION" code to give user option for the first timeout
My impression is that we should leave it out for now. Adding user space interfaces is complicated, and if someone absolutely needs it later, we can add it then.
OK , np , doing that way
Arnd
On Monday 04 May 2015 17:02:28 Fu Wei wrote:
On 1 May 2015 at 19:26, Arnd Bergmann arnd@arndb.de wrote:
On Friday 01 May 2015 16:22:08 Fu Wei wrote:
now , we have tree option: (1) just a warning to tell user space : you need to feed the dog ( but
what
is the best way to sent this message? by signal ?)
This is very unlikely to ever be seen by a real user, and the message will otherwise just be lost.
(2) feed the dog in the driver, this make sure kernel is alive, if not,
WS1
will be triggered
This seems counterproductive, because it prevents the watchdog hardware to do its job of rebooting the system when user space is unresponsive.
the purpose to do this way is : if kernel can still feed the dog , that means kernel is still alive, app is dead. so we can just kill the app, and keep kernel running for a while. but user need to be warned do you think it is make sense ?
No, that's not how the watchdog subsystem works. The idea is that the watchdog tries to probe all kinds of actions periodically (possibly network, disk, or some IPC) and only feeds the watchdog if all of them are functioning correctly. If the user space fails to notify the kernel, the assumption is that the system is not in a working state and needs to be reset. A bug in the watchdog program however is not an unlikely error scenario we need to worry about: even if that happens, the safe action to do is still to reboot the system because we can no longer tell if the system is working reliably.
(3)panic() (if we have kdump, we can tell what is wrong with kernel or app), backup and reboot. by this way , we have time to backup important date and debug instead of reset directly like normal WDT.
I can see value of this one, but if the other two are not needed, we can avoid adding an interface to configure the behavior and always do it this way if the pretimeout is set, or default to doing nothing when it is not set.
Good idea , let's do this way first ?
Ok.
Use platform_get_irq() here to allow deferred probing of the irqchip.
I tried to used that API before, but I found out I need not only a irq number, but also flags, so I turn to use resource. any thing we can do to improve this ? any thought?
I don't see where in your driver you actually use the flags, but I also don't fully understand the ACPI portion. It seems that the probe function just sets a variable in the watchdog specific structure, but that one is never used.
yes, for now , It's just for ACPI, but maybe we need the flags to config GIC? so maybe I do this way first, then if the flags are unnecessary in the future, I will turn to use platform_get_irq() .
platform_get_irq() guarantees that the flags are set up correctly in the GIC, and that should be the case for both ACPI and DT, a driver should never set that up manually. Just use it now.
Arnd
Hi Arnd,
Great thanks, I am using platform_get_irq() right now, and for ws0 routine, I am using panic()
On 4 May 2015 at 18:22, Arnd Bergmann arnd@arndb.de wrote:
On Monday 04 May 2015 17:02:28 Fu Wei wrote:
On 1 May 2015 at 19:26, Arnd Bergmann arnd@arndb.de wrote:
On Friday 01 May 2015 16:22:08 Fu Wei wrote:
now , we have tree option: (1) just a warning to tell user space : you need to feed the dog (
but
what
is the best way to sent this message? by signal ?)
This is very unlikely to ever be seen by a real user, and the message
will
otherwise just be lost.
(2) feed the dog in the driver, this make sure kernel is alive, if
not,
WS1
will be triggered
This seems counterproductive, because it prevents the watchdog
hardware to
do its job of rebooting the system when user space is unresponsive.
the purpose to do this way is : if kernel can still feed the dog , that means kernel is still alive, app is dead. so we can just kill the app,
and
keep kernel running for a while. but user need to be warned do you think it is make sense ?
No, that's not how the watchdog subsystem works. The idea is that the watchdog tries to probe all kinds of actions periodically (possibly network, disk, or some IPC) and only feeds the watchdog if all of them are functioning correctly. If the user space fails to notify the kernel, the assumption is that the system is not in a working state and needs to be reset. A bug in the watchdog program however is not an unlikely error scenario we need to worry about: even if that happens, the safe action to do is still to reboot the system because we can no longer tell if the system is working reliably.
(3)panic() (if we have kdump, we can tell what is wrong with kernel
or
app), backup and reboot. by this way , we have time to backup
important
date and debug instead of reset directly like normal WDT.
I can see value of this one, but if the other two are not needed, we can avoid adding an interface to configure the behavior and always do it this way if the pretimeout is set, or default to doing nothing when it is not set.
Good idea , let's do this way first ?
Ok.
Use platform_get_irq() here to allow deferred probing of the
irqchip.
I tried to used that API before, but I found out I need not only a
irq
number, but also flags, so I turn to use resource. any thing we can do to improve this ? any thought?
I don't see where in your driver you actually use the flags, but I also don't fully understand the ACPI portion. It seems that the probe
function
just sets a variable in the watchdog specific structure, but that one is never used.
yes, for now , It's just for ACPI, but maybe we need the flags to config GIC? so maybe I do this way first, then if the flags are unnecessary in the future, I will turn to use platform_get_irq() .
platform_get_irq() guarantees that the flags are set up correctly in the GIC, and that should be the case for both ACPI and DT, a driver should never set that up manually. Just use it now.
Arnd
Hi Arnd, I have sent my new patchset to linaro-acpi list, please help me review it. See if the updates can cover all your comment :-) any suggestion is most welcome! Great thanks for your time.
On 4 May 2015 at 18:40, Fu Wei fu.wei@linaro.org wrote:
Hi Arnd,
Great thanks, I am using platform_get_irq() right now, and for ws0 routine, I am using panic()
On 4 May 2015 at 18:22, Arnd Bergmann arnd@arndb.de wrote:
On Monday 04 May 2015 17:02:28 Fu Wei wrote:
On 1 May 2015 at 19:26, Arnd Bergmann arnd@arndb.de wrote:
On Friday 01 May 2015 16:22:08 Fu Wei wrote:
now , we have tree option: (1) just a warning to tell user space : you need to feed the dog (
but
what
is the best way to sent this message? by signal ?)
This is very unlikely to ever be seen by a real user, and the message
will
otherwise just be lost.
(2) feed the dog in the driver, this make sure kernel is alive, if
not,
WS1
will be triggered
This seems counterproductive, because it prevents the watchdog
hardware to
do its job of rebooting the system when user space is unresponsive.
the purpose to do this way is : if kernel can still feed the dog , that means kernel is still alive, app is dead. so we can just kill the app,
and
keep kernel running for a while. but user need to be warned do you think it is make sense ?
No, that's not how the watchdog subsystem works. The idea is that the watchdog tries to probe all kinds of actions periodically (possibly network, disk, or some IPC) and only feeds the watchdog if all of them are functioning correctly. If the user space fails to notify the kernel, the assumption is that the system is not in a working state and needs to be reset. A bug in the watchdog program however is not an unlikely error scenario we need to worry about: even if that happens, the safe action to do is still to reboot the system because we can no longer tell if the system is working reliably.
(3)panic() (if we have kdump, we can tell what is wrong with kernel
or
app), backup and reboot. by this way , we have time to backup
important
date and debug instead of reset directly like normal WDT.
I can see value of this one, but if the other two are not needed, we can avoid adding an interface to configure the behavior and always do it this way if the pretimeout is set, or default to doing nothing when it is not set.
Good idea , let's do this way first ?
Ok.
Use platform_get_irq() here to allow deferred probing of the
irqchip.
I tried to used that API before, but I found out I need not only a
irq
number, but also flags, so I turn to use resource. any thing we can do to improve this ? any thought?
I don't see where in your driver you actually use the flags, but I
also
don't fully understand the ACPI portion. It seems that the probe
function
just sets a variable in the watchdog specific structure, but that one is never used.
yes, for now , It's just for ACPI, but maybe we need the flags to config GIC? so maybe I do this way first, then if the flags are unnecessary in the future, I will turn to use platform_get_irq() .
platform_get_irq() guarantees that the flags are set up correctly in the GIC, and that should be the case for both ACPI and DT, a driver should never set that up manually. Just use it now.
Arnd
-- 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