On Sat, Oct 21, 2017 at 9:47 AM, Guenter Roeck <linux@xxxxxxxxxxxx> wrote: > On Wed, Oct 18, 2017 at 10:01:35AM -0700, Andrey Smirnov wrote: >> This driver provides access to RAVE SP watchdog functionality. >> >> Cc: linux-kernel@xxxxxxxxxxxxxxx >> Cc: linux-watchdog@xxxxxxxxxxxxxxx >> Cc: cphealy@xxxxxxxxx >> Cc: Lucas Stach <l.stach@xxxxxxxxxxxxxx> >> Cc: Nikita Yushchenko <nikita.yoush@xxxxxxxxxxxxxxxxxx> >> Cc: Lee Jones <lee.jones@xxxxxxxxxx> >> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> >> Cc: Pavel Machek <pavel@xxxxxx> >> Cc: Andy Shevchenko <andy.shevchenko@xxxxxxxxx> >> Cc: Guenter Roeck <linux@xxxxxxxxxxxx> >> Cc: Rob Herring <robh@xxxxxxxxxx> >> Cc: Johan Hovold <johan@xxxxxxxxxx> >> Signed-off-by: Nikita Yushchenko <nikita.yoush@xxxxxxxxxxxxxxxxxx> >> Signed-off-by: Andrey Smirnov <andrew.smirnov@xxxxxxxxx> >> --- >> drivers/watchdog/Kconfig | 7 + >> drivers/watchdog/Makefile | 1 + >> drivers/watchdog/rave-sp-wdt.c | 310 +++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 318 insertions(+) >> create mode 100644 drivers/watchdog/rave-sp-wdt.c >> >> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig >> index c722cbfdc7e6..533a72248cd1 100644 >> --- a/drivers/watchdog/Kconfig >> +++ b/drivers/watchdog/Kconfig >> @@ -223,6 +223,13 @@ config ZIIRAVE_WATCHDOG >> To compile this driver as a module, choose M here: the >> module will be called ziirave_wdt. >> >> +config RAVE_SP_WATCHDOG >> + tristate "RAVE SP Watchdog timer" >> + depends on RAVE_SP_CORE >> + select WATCHDOG_CORE >> + help >> + Support for the watchdog on RAVE SP device. >> + >> # ALPHA Architecture >> >> # ARM Architecture >> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile >> index 56adf9fa67d0..5c9556c09f6e 100644 >> --- a/drivers/watchdog/Makefile >> +++ b/drivers/watchdog/Makefile >> @@ -223,3 +223,4 @@ obj-$(CONFIG_MAX77620_WATCHDOG) += max77620_wdt.o >> obj-$(CONFIG_ZIIRAVE_WATCHDOG) += ziirave_wdt.o >> obj-$(CONFIG_SOFT_WATCHDOG) += softdog.o >> obj-$(CONFIG_MENF21BMC_WATCHDOG) += menf21bmc_wdt.o >> +obj-$(CONFIG_RAVE_SP_WATCHDOG) += rave-sp-wdt.o >> diff --git a/drivers/watchdog/rave-sp-wdt.c b/drivers/watchdog/rave-sp-wdt.c >> new file mode 100644 >> index 000000000000..86fdc7fb1f92 >> --- /dev/null >> +++ b/drivers/watchdog/rave-sp-wdt.c >> @@ -0,0 +1,310 @@ >> +/* >> + * rave-sp-wdt.c - Watchdog driver preseing in RAVE SP > > present ? > Yup, my bad, will fix in v9. >> + * >> + * Copyright (C) 2017 Zodiac Inflight Innovation >> + * >> + * This program is free software; you can redistribute it and/or >> + * modify it under the terms of the GNU General Public License as >> + * published by the Free Software Foundation; either version 2 of >> + * the License, or (at your option) any later version. >> + * >> + * 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. >> + * >> + * You should have received a copy of the GNU General Public License >> + * along with this program; if not, see <http://www.gnu.org/licenses/>. >> + */ >> + >> +#include <linux/delay.h> >> +#include <linux/kernel.h> >> +#include <linux/module.h> >> +#include <linux/of_device.h> >> +#include <linux/platform_device.h> >> +#include <linux/rave-sp.h> >> +#include <linux/watchdog.h> >> +#include <linux/nvmem-consumer.h> >> +#include <linux/reboot.h> >> +#include <linux/slab.h> > > Alphabetic order please. > OK, will do. >> + >> +enum { >> + RAVE_SP_RESET_BYTE = 1, >> + RAVE_SP_RESET_REASON_NORMAL = 0, >> + RAVE_SP_RESET_DELAY_MS = 500, >> +}; >> + >> +struct rave_sp_wdt_variant { >> + unsigned int max_timeout; >> + unsigned int min_timeout; >> + >> + int (*configure)(struct watchdog_device *); >> + int (*restart)(struct watchdog_device *); >> +}; >> + >> +struct rave_sp_wdt { >> + struct watchdog_device wdd; >> + struct rave_sp *sp; >> + const struct rave_sp_wdt_variant *variant; >> + struct notifier_block reboot_notifier; >> +}; >> + >> +static struct rave_sp_wdt *to_rave_sp_wdt(struct watchdog_device *wdd) >> +{ >> + return container_of(wdd, struct rave_sp_wdt, wdd); >> +} >> + >> +static int __rave_sp_wdt_exec(struct watchdog_device *wdd, >> + void *data, size_t data_size, >> + void *reply, size_t reply_size) >> +{ >> + return rave_sp_exec(to_rave_sp_wdt(wdd)->sp, >> + data, data_size, reply, reply_size); >> +} > > This extra function is only called once and thus quite pointless. > True. Will remove in v9. >> + >> +static int rave_sp_wdt_exec(struct watchdog_device *wdd, void *data, >> + size_t data_size) >> +{ >> + return __rave_sp_wdt_exec(wdd, data, data_size, NULL, 0); >> +} >> + >> + > > Please run checkpatch --strict and fix what it reports. > Sure, will do. >> +static int rave_sp_wdt_legacy_configure(struct watchdog_device *wdd) >> +{ >> + const bool enable = watchdog_hw_running(wdd); >> + u8 cmd[] = { >> + [0] = RAVE_SP_CMD_SW_WDT, >> + [1] = 0, >> + [2] = 0, >> + [3] = !!enable, > > Useless !!. It converts a boolean to a boolean. Besides, isn't it always > true anyway ? Good point, will remove the "!!". This function also is called as .stop method (via rave_sp_wdt_configure) for corresponding "watchdog_device" and I rely on code in watchdog_stop() to clear WDOG_HW_RUNNING. Is this not a correct thing to do? > >> + [4] = enable ? wdd->timeout : 0, >> + }; > > Interesting; checkpatch doesn't require an empty line after variable > declarations anymore ? Lets keep it anyway for consistency. > OK, will fix in v9. >> + return rave_sp_wdt_exec(wdd, cmd, sizeof(cmd)); >> +} >> + >> +static int rave_sp_wdt_rdu_configure(struct watchdog_device *wdd) >> +{ >> + u8 cmd[] = { >> + [0] = RAVE_SP_CMD_SW_WDT, >> + [1] = 0, >> + [2] = watchdog_hw_running(wdd), > > Nitpick, but isn't this always true ? There is no stop function, > and HW_RUNNING is set before this function is called. > Same as above. Let me know if relying on HW_RUNNING being cleared by watchdog_stop() is incorrect. >> + [3] = (u8) wdd->timeout, >> + [4] = (u8) (wdd->timeout >> 8), >> + }; >> + return rave_sp_wdt_exec(wdd, cmd, sizeof(cmd)); >> +} >> + >> +static int rave_sp_wdt_configure(struct watchdog_device *wdd) >> +{ >> + return to_rave_sp_wdt(wdd)->variant->configure(wdd); >> +} >> + >> +static int rave_sp_wdt_legacy_restart(struct watchdog_device *wdd) >> +{ >> + u8 cmd[] = { >> + [0] = RAVE_SP_CMD_RESET, >> + [1] = 0, >> + [2] = RAVE_SP_RESET_BYTE >> + }; >> + return rave_sp_wdt_exec(wdd, cmd, sizeof(cmd)); >> +} >> + >> +static int rave_sp_wdt_rdu_restart(struct watchdog_device *wdd) >> +{ >> + u8 cmd[] = { >> + [0] = RAVE_SP_CMD_RESET, >> + [1] = 0, >> + [2] = RAVE_SP_RESET_BYTE, >> + [3] = RAVE_SP_RESET_REASON_NORMAL >> + }; >> + return rave_sp_wdt_exec(wdd, cmd, sizeof(cmd)); >> +} >> + >> +static int rave_sp_wdt_reboot_notifier(struct notifier_block *nb, >> + unsigned long action, void *data) >> +{ >> + /* >> + * Restart handler is called in atomic context which means we >> + * can't commuicate to SP via UART. Luckily for use SP will > > communicate > My bad, will fix in v9. >> + * wait 500ms before actually resetting us, so we ask it to do >> + * so here and let the rest of the system go on wrapping >> + * things up. >> + */ >> + if (action == SYS_DOWN || action == SYS_HALT) { >> + struct rave_sp_wdt *sp_wd = >> + container_of(nb, struct rave_sp_wdt, reboot_notifier); >> + >> + const int ret = sp_wd->variant->restart(&sp_wd->wdd); >> + >> + if (ret < 0) >> + dev_err(sp_wd->wdd.parent, >> + "Failed to issue restart command (%d)", ret); >> + return NOTIFY_OK; >> + } >> + >> + return NOTIFY_DONE; >> +} >> + >> +static int rave_sp_wdt_restart(struct watchdog_device *wdd, >> + unsigned long action, void *data) >> +{ >> + /* >> + * The actual work was done by reboot notifier above. SP >> + * firmware waits 500 ms before issuing reset, so let's hang >> + * here for one second and hopefuly we'd never reach that > > RAVE_SP_RESET_DELAY_MS is 500(ms), not one second. > Sorry, meant to wait for twice that delay, hence one second, but wrote incorrect code. Will fix in v9. >> + * return statement >> + */ >> + mdelay(RAVE_SP_RESET_DELAY_MS); >> + return -EIO; >> +} >> + >> +static int rave_sp_wdt_start(struct watchdog_device *wdd) >> +{ >> + set_bit(WDOG_HW_RUNNING, &wdd->status); >> + return rave_sp_wdt_configure(wdd); >> +} >> + >> +static int rave_sp_wdt_set_timeout(struct watchdog_device *wdd, >> + unsigned int timeout) >> +{ >> + wdd->timeout = timeout; >> + return rave_sp_wdt_configure(wdd); >> +} >> + >> +static int rave_sp_wdt_ping(struct watchdog_device *wdd) >> +{ >> + u8 cmd[] = { >> + [0] = RAVE_SP_CMD_PET_WDT, >> + [1] = 0, >> + }; >> + >> + return rave_sp_wdt_exec(wdd, cmd, sizeof(cmd)); >> +} >> + >> +static const struct watchdog_info rave_sp_wdt_info = { >> + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE, >> + .identity = "RAVE SP Watchdog", >> +}; >> + >> +static const struct watchdog_ops rave_sp_wdt_ops = { >> + .owner = THIS_MODULE, >> + .start = rave_sp_wdt_start, >> + .stop = rave_sp_wdt_configure, >> + .ping = rave_sp_wdt_ping, >> + .set_timeout = rave_sp_wdt_set_timeout, >> + .restart = rave_sp_wdt_restart, >> +}; >> + >> +static const struct of_device_id rave_sp_wdt_of_match[] = { >> + { .compatible = "zii,rave-sp-watchdog" }, >> + {} >> +}; >> + >> +static const struct rave_sp_wdt_variant rave_sp_wdt_legacy = { >> + .max_timeout = 255, >> + .min_timeout = 1, >> + .configure = rave_sp_wdt_legacy_configure, >> + .restart = rave_sp_wdt_legacy_restart, >> +}; >> + >> +static const struct rave_sp_wdt_variant rave_sp_wdt_rdu = { >> + .max_timeout = 180, >> + .min_timeout = 60, >> + .configure = rave_sp_wdt_rdu_configure, >> + .restart = rave_sp_wdt_rdu_restart, >> +}; >> + >> +static const struct of_device_id rave_sp_wdt_variants[] = { >> + { .compatible = COMPATIBLE_RAVE_SP_NIU, .data = &rave_sp_wdt_legacy }, >> + { .compatible = COMPATIBLE_RAVE_SP_MEZZ, .data = &rave_sp_wdt_legacy }, >> + { .compatible = COMPATIBLE_RAVE_SP_ESB, .data = &rave_sp_wdt_legacy }, >> + { .compatible = COMPATIBLE_RAVE_SP_RDU1, .data = &rave_sp_wdt_rdu }, >> + { .compatible = COMPATIBLE_RAVE_SP_RDU2, .data = &rave_sp_wdt_rdu }, >> + { /* sentinel */ } >> +}; >> + >> +static int rave_sp_wdt_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + const struct of_device_id *id; >> + struct watchdog_device *wdd; >> + struct rave_sp_wdt *sp_wd; >> + struct nvmem_cell *cell; >> + __le16 timeout = 0; >> + int ret; >> + >> + id = of_match_device(rave_sp_wdt_variants, dev->parent); >> + if (WARN_ON(!id)) >> + return -ENODEV; >> + >> + sp_wd = devm_kzalloc(dev, sizeof(*sp_wd), GFP_KERNEL); >> + if (!sp_wd) >> + return -ENOMEM; >> + >> + sp_wd->variant = id->data; >> + sp_wd->sp = dev_get_drvdata(dev->parent); >> + >> + if (WARN_ON(!sp_wd->sp)) >> + return -ENODEV; >> + > Not sure I understand the value of those tracebacks. Presumably the driver > is instantiated through its parent. Is there precedent that other drivers > in the same situation do this ? > No precedent. I was just being paranoid. Will remove in v9. Thanks, Andrey Smirnov -- To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
