| Message ID | f8e743a6c49607de0dd7a27778383477e051b130.1712058690.git.mazziesaccount@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Support ROHM BD96801 scalable PMIC | expand |
On 02/04/2024 15:11, Matti Vaittinen wrote: > Introduce driver for WDG block on ROHM BD96801 scalable PMIC. > > This driver only supports watchdog with I2C feeding and delayed > response detection. Whether the watchdog toggles PRSTB pin or > just causes an interrupt can be configured via device-tree. > > The BD96801 PMIC HW supports also window watchdog (too early ... > + > +static struct platform_driver bd96801_wdt = { > + .driver = { > + .name = "bd96801-wdt" > + }, > + .probe = bd96801_wdt_probe, > +}; > +module_platform_driver(bd96801_wdt); > + > +MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>"); > +MODULE_DESCRIPTION("BD96801 watchdog driver"); > +MODULE_LICENSE("GPL"); > +MODULE_ALIAS("platform:bd96801-wdt"); Same comment on alias. Please use ID table. Best regards, Krzysztof
On Tue, Apr 02, 2024 at 04:11:41PM +0300, Matti Vaittinen wrote: > Introduce driver for WDG block on ROHM BD96801 scalable PMIC. > > This driver only supports watchdog with I2C feeding and delayed > response detection. Whether the watchdog toggles PRSTB pin or > just causes an interrupt can be configured via device-tree. > > The BD96801 PMIC HW supports also window watchdog (too early > feeding detection) and Q&A mode. These are not supported by > this driver. > > Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> > --- > drivers/watchdog/Kconfig | 13 ++ > drivers/watchdog/Makefile | 1 + > drivers/watchdog/bd96801_wdt.c | 375 +++++++++++++++++++++++++++++++++ > 3 files changed, 389 insertions(+) > create mode 100644 drivers/watchdog/bd96801_wdt.c > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig > index 6bee137cfbe0..d97e735e1faa 100644 > --- a/drivers/watchdog/Kconfig > +++ b/drivers/watchdog/Kconfig > @@ -181,6 +181,19 @@ config BD957XMUF_WATCHDOG > watchdog. Alternatively say M to compile the driver as a module, > which will be called bd9576_wdt. > > +config BD96801_WATCHDOG > + tristate "ROHM BD96801 PMIC Watchdog" > + depends on MFD_ROHM_BD96801 > + select WATCHDOG_CORE > + help > + Support for the watchdog in the ROHM BD96801 PMIC. Watchdog can be > + configured to only generate IRQ or to trigger system reset via reset > + pin. > + > + Say Y here to include support for the ROHM BD96801 watchdog. > + Alternatively say M to compile the driver as a module, > + which will be called bd96801_wdt. > + > config CROS_EC_WATCHDOG > tristate "ChromeOS EC-based watchdog" > select WATCHDOG_CORE > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile > index 3710c218f05e..31bc94436c81 100644 > --- a/drivers/watchdog/Makefile > +++ b/drivers/watchdog/Makefile > @@ -217,6 +217,7 @@ obj-$(CONFIG_XEN_WDT) += xen_wdt.o > > # Architecture Independent > obj-$(CONFIG_BD957XMUF_WATCHDOG) += bd9576_wdt.o > +obj-$(CONFIG_BD96801_WATCHDOG) += bd96801_wdt.o > obj-$(CONFIG_CROS_EC_WATCHDOG) += cros_ec_wdt.o > obj-$(CONFIG_DA9052_WATCHDOG) += da9052_wdt.o > obj-$(CONFIG_DA9055_WATCHDOG) += da9055_wdt.o > diff --git a/drivers/watchdog/bd96801_wdt.c b/drivers/watchdog/bd96801_wdt.c > new file mode 100644 > index 000000000000..cb2b526ecc21 > --- /dev/null > +++ b/drivers/watchdog/bd96801_wdt.c > @@ -0,0 +1,375 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (C) 2022 ROHM Semiconductors > + * > + * ROHM BD96801 watchdog driver > + */ > + > +#include <linux/kernel.h> > +#include <linux/mfd/rohm-bd96801.h> > +#include <linux/mfd/rohm-generic.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> > +#include <linux/watchdog.h> > + > +static bool nowayout; > +module_param(nowayout, bool, 0); > +MODULE_PARM_DESC(nowayout, > + "Watchdog cannot be stopped once started (default=\"false\")"); > + > +#define BD96801_WD_TMO_SHORT_MASK 0x70 > +#define BD96801_WD_RATIO_MASK 0x3 > +#define BD96801_WD_TYPE_MASK 0x4 > +#define BD96801_WD_TYPE_SLOW 0x4 > +#define BD96801_WD_TYPE_WIN 0x0 > + > +#define BD96801_WD_EN_MASK 0x3 > +#define BD96801_WD_IF_EN 0x1 > +#define BD96801_WD_QA_EN 0x2 > +#define BD96801_WD_DISABLE 0x0 > + > +#define BD96801_WD_ASSERT_MASK 0x8 > +#define BD96801_WD_ASSERT_RST 0x8 > +#define BD96801_WD_ASSERT_IRQ 0x0 > + > +#define BD96801_WD_FEED_MASK 0x1 > +#define BD96801_WD_FEED 0x1 > + > +/* units in uS */ > +#define FASTNG_MIN 3370 > +#define BD96801_WDT_DEFAULT_MARGIN 6905120 > +/* Unit is seconds */ > +#define DEFAULT_TIMEOUT 30 > + > +/* > + * BD96801 WDG supports window mode so the TMO consists of SHORT and LONG > + * timeout values. SHORT time is meaningfull only in window mode where feeding > + * period shorter than SHORT would be an error. LONG time is used to detect if > + * feeding is not occurring within given time limit (SoC SW hangs). The LONG > + * timeout time is a multiple of (2, 4, 8 0r 16 times) the SHORT timeout. > + */ > + > +struct wdtbd96801 { > + struct device *dev; > + struct regmap *regmap; > + bool always_running; > + struct watchdog_device wdt; > +}; > + > +static int bd96801_wdt_ping(struct watchdog_device *wdt) > +{ > + struct wdtbd96801 *w = watchdog_get_drvdata(wdt); > + > + return regmap_update_bits(w->regmap, BD96801_REG_WD_FEED, > + BD96801_WD_FEED_MASK, BD96801_WD_FEED); > +} > + > +static int bd96801_wdt_start(struct watchdog_device *wdt) > +{ > + struct wdtbd96801 *w = watchdog_get_drvdata(wdt); > + int ret; > + > + ret = regmap_update_bits(w->regmap, BD96801_REG_WD_CONF, > + BD96801_WD_EN_MASK, BD96801_WD_IF_EN); > + > + return ret; > +} > + > +static int bd96801_wdt_stop(struct watchdog_device *wdt) > +{ > + struct wdtbd96801 *w = watchdog_get_drvdata(wdt); > + > + if (!w->always_running) > + return regmap_update_bits(w->regmap, BD96801_REG_WD_CONF, > + BD96801_WD_EN_MASK, BD96801_WD_DISABLE); > + set_bit(WDOG_HW_RUNNING, &wdt->status); > + > + return 0; > +} > + > +static const struct watchdog_info bd96801_wdt_info = { > + .options = WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING | > + WDIOF_SETTIMEOUT, > + .identity = "BD96801 Watchdog", > +}; > + > +static const struct watchdog_ops bd96801_wdt_ops = { > + .start = bd96801_wdt_start, > + .stop = bd96801_wdt_stop, > + .ping = bd96801_wdt_ping, > +}; > + > +static int find_closest_fast(int target, int *sel, int *val) > +{ > + int i; > + int window = FASTNG_MIN; > + > + for (i = 0; i < 8 && window < target; i++) > + window <<= 1; > + > + *val = window; > + *sel = i; > + > + if (i == 8) > + return -EINVAL; > + > + return 0; > +} > + > +static int find_closest_slow_by_fast(int fast_val, int *target, int *slowsel) > +{ > + int sel; > + static const int multipliers[] = {2, 4, 8, 16}; > + > + for (sel = 0; sel < ARRAY_SIZE(multipliers) && > + multipliers[sel] * fast_val < *target; sel++) > + ; > + > + if (sel == ARRAY_SIZE(multipliers)) > + return -EINVAL; > + > + *slowsel = sel; > + *target = multipliers[sel] * fast_val; > + > + return 0; > +} > + > +static int find_closest_slow(int *target, int *slow_sel, int *fast_sel) > +{ > + static const int multipliers[] = {2, 4, 8, 16}; > + int i, j; > + int val = 0; > + int window = FASTNG_MIN; > + > + for (i = 0; i < 8; i++) { > + for (j = 0; j < ARRAY_SIZE(multipliers); j++) { > + int slow; > + > + slow = window * multipliers[j]; > + if (slow >= *target && (!val || slow < val)) { > + val = slow; > + *fast_sel = i; > + *slow_sel = j; > + } > + } > + window <<= 1; > + } > + if (!val) > + return -EINVAL; > + > + *target = val; > + > + return 0; > +} > + > +static int bd96801_set_wdt_mode(struct wdtbd96801 *w, int hw_margin, > + int hw_margin_min) > +{ > + int ret, fastng, slowng, type, reg, mask; > + struct device *dev = w->dev; > + > + /* convert to uS */ > + hw_margin *= 1000; > + hw_margin_min *= 1000; > + if (hw_margin_min) { > + int min; > + > + type = BD96801_WD_TYPE_WIN; > + dev_dbg(dev, "Setting type WINDOW 0x%x\n", type); > + ret = find_closest_fast(hw_margin_min, &fastng, &min); > + if (ret) { > + dev_err(dev, "bad WDT window for fast timeout\n"); > + return ret; > + } > + > + ret = find_closest_slow_by_fast(min, &hw_margin, &slowng); > + if (ret) { > + dev_err(dev, "bad WDT window\n"); > + return ret; > + } > + w->wdt.min_hw_heartbeat_ms = min / 1000; > + } else { > + type = BD96801_WD_TYPE_SLOW; > + dev_dbg(dev, "Setting type SLOW 0x%x\n", type); > + ret = find_closest_slow(&hw_margin, &slowng, &fastng); > + if (ret) { > + dev_err(dev, "bad WDT window\n"); > + return ret; > + } > + } > + > + w->wdt.max_hw_heartbeat_ms = hw_margin / 1000; > + > + fastng <<= ffs(BD96801_WD_TMO_SHORT_MASK) - 1; > + > + reg = slowng | fastng; > + mask = BD96801_WD_RATIO_MASK | BD96801_WD_TMO_SHORT_MASK; > + ret = regmap_update_bits(w->regmap, BD96801_REG_WD_TMO, > + mask, reg); > + if (ret) > + return ret; > + > + ret = regmap_update_bits(w->regmap, BD96801_REG_WD_CONF, > + BD96801_WD_TYPE_MASK, type); > + > + return ret; > +} > + > +static int bd96801_set_heartbeat_from_hw(struct wdtbd96801 *w, > + unsigned int conf_reg) > +{ > + int ret; > + unsigned int val, sel, fast; > + > + /* > + * The BD96801 supports a somewhat peculiar QA-mode, which we do not > + * support in this driver. If the QA-mode is enabled then we just > + * warn and bail-out. > + */ > + if ((conf_reg & BD96801_WD_EN_MASK) != BD96801_WD_IF_EN) { > + dev_warn(w->dev, "watchdog set to Q&A mode - exiting\n"); > + return -EINVAL; > + } > + > + ret = regmap_read(w->regmap, BD96801_REG_WD_TMO, &val); > + if (ret) > + return ret; > + > + sel = val & BD96801_WD_TMO_SHORT_MASK; > + sel >>= ffs(BD96801_WD_TMO_SHORT_MASK) - 1; > + fast = FASTNG_MIN << sel; > + > + sel = (val & BD96801_WD_RATIO_MASK) + 1; > + w->wdt.max_hw_heartbeat_ms = (fast << sel) / USEC_PER_MSEC; > + > + if ((conf_reg & BD96801_WD_TYPE_MASK) == BD96801_WD_TYPE_WIN) > + w->wdt.min_hw_heartbeat_ms = fast / USEC_PER_MSEC; > + > + return 0; > +} > + > +static int init_wdg_hw(struct wdtbd96801 *w) > +{ > + u32 hw_margin[2]; > + int count, ret; > + u32 hw_margin_max = BD96801_WDT_DEFAULT_MARGIN, hw_margin_min = 0; > + > + count = device_property_count_u32(w->dev->parent, "rohm,hw-timeout-ms"); > + if (count < 0 && count != -EINVAL) > + return count; > + > + if (count > 0) { > + if (count > ARRAY_SIZE(hw_margin)) > + return -EINVAL; > + > + ret = device_property_read_u32_array(w->dev->parent, > + "rohm,hw-timeout-ms", > + &hw_margin[0], count); > + if (ret < 0) > + return ret; > + > + if (count == 1) > + hw_margin_max = hw_margin[0]; > + > + if (count == 2) { > + hw_margin_max = hw_margin[1]; > + hw_margin_min = hw_margin[0]; > + } > + } > + > + ret = bd96801_set_wdt_mode(w, hw_margin_max, hw_margin_min); > + if (ret) > + return ret; > + > + ret = device_property_match_string(w->dev->parent, "rohm,wdg-action", > + "prstb"); > + if (ret >= 0) { > + ret = regmap_update_bits(w->regmap, BD96801_REG_WD_CONF, > + BD96801_WD_ASSERT_MASK, > + BD96801_WD_ASSERT_RST); > + return ret; > + } > + > + ret = device_property_match_string(w->dev->parent, "rohm,wdg-action", > + "intb-only"); > + if (ret >= 0) { > + ret = regmap_update_bits(w->regmap, BD96801_REG_WD_CONF, > + BD96801_WD_ASSERT_MASK, > + BD96801_WD_ASSERT_IRQ); > + return ret; > + } I don't see the devicetree bindings documented in the series. I am also a bit surprised that the interrupt isn't handled in the driver. Please explain. > + > + return 0; > +} > + > +static int bd96801_wdt_probe(struct platform_device *pdev) > +{ > + struct wdtbd96801 *w; > + int ret; > + unsigned int val; > + > + w = devm_kzalloc(&pdev->dev, sizeof(*w), GFP_KERNEL); > + if (!w) > + return -ENOMEM; > + > + w->regmap = dev_get_regmap(pdev->dev.parent, NULL); dev_get_regmap() can return NULL. > + w->dev = &pdev->dev; > + > + w->wdt.info = &bd96801_wdt_info; > + w->wdt.ops = &bd96801_wdt_ops; > + w->wdt.parent = pdev->dev.parent; > + w->wdt.timeout = DEFAULT_TIMEOUT; > + watchdog_set_drvdata(&w->wdt, w); > + > + w->always_running = device_property_read_bool(pdev->dev.parent, > + "always-running"); > + Without documentation, it looks like the always-running (from linux,wdt-gpio.yaml) may be abused. Its defined meaning is "the watchdog is always running and can not be stopped". Its use here seems to be "start watchdog when loading the module and prevent it from being stopped". Oh well, looks like the abuse was introduced with bd9576_wdt. That doesn't make it better. At the very least it needs to be documented that the property does not have the intended (documented) meaning. > + ret = regmap_read(w->regmap, BD96801_REG_WD_CONF, &val); > + if (ret) > + return dev_err_probe(&pdev->dev, ret, > + "Failed to get the watchdog state\n"); > + > + /* > + * If the WDG is already enabled we assume it is configured by boot. > + * In this case we just update the hw-timeout based on values set to > + * the timeout / mode registers and leave the hardware configs > + * untouched. > + */ > + if ((val & BD96801_WD_EN_MASK) != BD96801_WD_DISABLE) { > + dev_dbg(&pdev->dev, "watchdog was running during probe\n"); > + ret = bd96801_set_heartbeat_from_hw(w, val); > + if (ret) > + return ret; > + > + set_bit(WDOG_HW_RUNNING, &w->wdt.status); > + } else { > + /* If WDG is not running so we will initializate it */ > + ret = init_wdg_hw(w); > + if (ret) > + return ret; > + } > + > + watchdog_init_timeout(&w->wdt, 0, pdev->dev.parent); > + watchdog_set_nowayout(&w->wdt, nowayout); > + watchdog_stop_on_reboot(&w->wdt); > + > + if (w->always_running) > + bd96801_wdt_start(&w->wdt); I think this needs to set WDOG_HW_RUNNING or the watchdog will trigger a reboot if the watchdog device is not opened and the watchdog wasn't already running when the module was loaded. That makes me wonder what happens if the property is set and the watchdog daemon isn't started in the bd9576_wdt driver. > + > + return devm_watchdog_register_device(&pdev->dev, &w->wdt); > +} > + > +static struct platform_driver bd96801_wdt = { > + .driver = { > + .name = "bd96801-wdt" > + }, > + .probe = bd96801_wdt_probe, > +}; > +module_platform_driver(bd96801_wdt); > + > +MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>"); > +MODULE_DESCRIPTION("BD96801 watchdog driver"); > +MODULE_LICENSE("GPL"); > +MODULE_ALIAS("platform:bd96801-wdt"); > -- > 2.43.2 > > > -- > Matti Vaittinen, Linux device drivers > ROHM Semiconductors, Finland SWDC > Kiviharjunlenkki 1E > 90220 OULU > FINLAND > > ~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~ > Simon says - in Latin please. > ~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~ > Thanks to Simon Glass for the translation =]
Hi Guenter, First of all, thanks for the review. It was quick! Especially when we speak of a RFC series. Very much appreciated. On 4/2/24 20:11, Guenter Roeck wrote: > On Tue, Apr 02, 2024 at 04:11:41PM +0300, Matti Vaittinen wrote >> +static int init_wdg_hw(struct wdtbd96801 *w) >> +{ >> + u32 hw_margin[2]; >> + int count, ret; >> + u32 hw_margin_max = BD96801_WDT_DEFAULT_MARGIN, hw_margin_min = 0; >> + >> + count = device_property_count_u32(w->dev->parent, "rohm,hw-timeout-ms"); >> + if (count < 0 && count != -EINVAL) >> + return count; >> + >> + if (count > 0) { >> + if (count > ARRAY_SIZE(hw_margin)) >> + return -EINVAL; >> + >> + ret = device_property_read_u32_array(w->dev->parent, >> + "rohm,hw-timeout-ms", >> + &hw_margin[0], count); >> + if (ret < 0) >> + return ret; >> + >> + if (count == 1) >> + hw_margin_max = hw_margin[0]; >> + >> + if (count == 2) { >> + hw_margin_max = hw_margin[1]; >> + hw_margin_min = hw_margin[0]; >> + } >> + } >> + >> + ret = bd96801_set_wdt_mode(w, hw_margin_max, hw_margin_min); >> + if (ret) >> + return ret; >> + >> + ret = device_property_match_string(w->dev->parent, "rohm,wdg-action", >> + "prstb"); >> + if (ret >= 0) { >> + ret = regmap_update_bits(w->regmap, BD96801_REG_WD_CONF, >> + BD96801_WD_ASSERT_MASK, >> + BD96801_WD_ASSERT_RST); >> + return ret; >> + } >> + >> + ret = device_property_match_string(w->dev->parent, "rohm,wdg-action", >> + "intb-only"); >> + if (ret >= 0) { >> + ret = regmap_update_bits(w->regmap, BD96801_REG_WD_CONF, >> + BD96801_WD_ASSERT_MASK, >> + BD96801_WD_ASSERT_IRQ); >> + return ret; >> + } > > I don't see the devicetree bindings documented in the series. Seems like I have missed this WDG binding. But after reading your comment below, I am wondering if I should just drop the binding and default to "prstb" (shutdown should the feeding be skipped) - and leave the "intb-only" case for one who actually needs such. > I am also a bit surprised that the interrupt isn't handled in the driver. > Please explain. Basically, I just had no idea what the IRQ should do in the generic case. If we get an interrupt, it means the WDG feeding has failed. My thinking is that, what should happen is forced reset. I don't see how that can be done in reliably manner from an IRQ handler. When the "prstb WDG action" is set (please, see the above DT binding handling), the PMIC shall shut down power outputs. This should get the watchdog's job done. With the "intb-only"-option, PMIC will not turn off the power. I'd expect there to be some external HW connection which handles the reset by HW. After all this being said, I wonder if I should just unconditionally configure the PMIC to always turn off the power (prstb option) should the feeding fail? Or do someone have some suggestion what the IRQ handler should do (except maybe print an error msg)? > >> + >> + return 0; >> +} >> + >> +static int bd96801_wdt_probe(struct platform_device *pdev) >> +{ >> + struct wdtbd96801 *w; >> + int ret; >> + unsigned int val; >> + >> + w = devm_kzalloc(&pdev->dev, sizeof(*w), GFP_KERNEL); >> + if (!w) >> + return -ENOMEM; >> + >> + w->regmap = dev_get_regmap(pdev->dev.parent, NULL); > > dev_get_regmap() can return NULL. > >> + w->dev = &pdev->dev; >> + >> + w->wdt.info = &bd96801_wdt_info; >> + w->wdt.ops = &bd96801_wdt_ops; >> + w->wdt.parent = pdev->dev.parent; >> + w->wdt.timeout = DEFAULT_TIMEOUT; >> + watchdog_set_drvdata(&w->wdt, w); >> + >> + w->always_running = device_property_read_bool(pdev->dev.parent, >> + "always-running"); >> + > Without documentation, it looks like the always-running (from > linux,wdt-gpio.yaml) may be abused. Its defined meaning is > "the watchdog is always running and can not be stopped". Its > use here seems to be "start watchdog when loading the module and > prevent it from being stopped". Yes. You're right. > Oh well, looks like the abuse was introduced with bd9576_wdt. That > doesn't make it better. At the very least it needs to be documented > that the property does not have the intended (documented) meaning. I can raise my hand for a sign of an error here. I've been misreading the intended meaning of the always-running. Not sure if I've picked it from another driver (maybe GPIO watchdog), or if I've just misinterpreted the binding docs. Do you suggest me to add a note in the BD9576 binding doc (there is no BD9576 specific binding doc for watchdog. I wonder whether this warrants adding one under watchdog or if the note can be added under Documentation/devicetree/bindings/mfd/rohm,bd9576...). >> + ret = regmap_read(w->regmap, BD96801_REG_WD_CONF, &val); >> + if (ret) >> + return dev_err_probe(&pdev->dev, ret, >> + "Failed to get the watchdog state\n"); >> + >> + /* >> + * If the WDG is already enabled we assume it is configured by boot. >> + * In this case we just update the hw-timeout based on values set to >> + * the timeout / mode registers and leave the hardware configs >> + * untouched. >> + */ >> + if ((val & BD96801_WD_EN_MASK) != BD96801_WD_DISABLE) { >> + dev_dbg(&pdev->dev, "watchdog was running during probe\n"); >> + ret = bd96801_set_heartbeat_from_hw(w, val); >> + if (ret) >> + return ret; >> + >> + set_bit(WDOG_HW_RUNNING, &w->wdt.status); >> + } else { >> + /* If WDG is not running so we will initializate it */ >> + ret = init_wdg_hw(w); >> + if (ret) >> + return ret; >> + } >> + >> + watchdog_init_timeout(&w->wdt, 0, pdev->dev.parent); >> + watchdog_set_nowayout(&w->wdt, nowayout); >> + watchdog_stop_on_reboot(&w->wdt); >> + >> + if (w->always_running) >> + bd96801_wdt_start(&w->wdt); > > I think this needs to set WDOG_HW_RUNNING or the watchdog will trigger > a reboot if the watchdog device is not opened and the watchdog wasn't > already running when the module was loaded. I believe you're right. Seems I haven't properly tested this path. > That makes me wonder what happens if the property is set and the > watchdog daemon isn't started in the bd9576_wdt driver. My assumption is the dog starts barking. I'll see if I find the BD9576 break-out board from one of my boxes to wire it up and test this. If the always-running is not working it might be justified to just drop it from both drivers. I believe it'd be indication that no-one is really using the always-running with the upstream driver. Thanks a ton! Yours, -- Matti
On Wed, Apr 03, 2024 at 09:34:35AM +0300, Matti Vaittinen wrote: > Hi Guenter, > > First of all, thanks for the review. It was quick! Especially when we speak > of a RFC series. Very much appreciated. > > On 4/2/24 20:11, Guenter Roeck wrote: > > On Tue, Apr 02, 2024 at 04:11:41PM +0300, Matti Vaittinen wrote >> +static int init_wdg_hw(struct wdtbd96801 *w) > > > +{ > > > + u32 hw_margin[2]; > > > + int count, ret; > > > + u32 hw_margin_max = BD96801_WDT_DEFAULT_MARGIN, hw_margin_min = 0; > > > + > > > + count = device_property_count_u32(w->dev->parent, "rohm,hw-timeout-ms"); > > > + if (count < 0 && count != -EINVAL) > > > + return count; > > > + > > > + if (count > 0) { > > > + if (count > ARRAY_SIZE(hw_margin)) > > > + return -EINVAL; > > > + > > > + ret = device_property_read_u32_array(w->dev->parent, > > > + "rohm,hw-timeout-ms", > > > + &hw_margin[0], count); > > > + if (ret < 0) > > > + return ret; > > > + > > > + if (count == 1) > > > + hw_margin_max = hw_margin[0]; > > > + > > > + if (count == 2) { > > > + hw_margin_max = hw_margin[1]; > > > + hw_margin_min = hw_margin[0]; > > > + } > > > + } > > > + > > > + ret = bd96801_set_wdt_mode(w, hw_margin_max, hw_margin_min); > > > + if (ret) > > > + return ret; > > > + > > > + ret = device_property_match_string(w->dev->parent, "rohm,wdg-action", > > > + "prstb"); > > > + if (ret >= 0) { > > > + ret = regmap_update_bits(w->regmap, BD96801_REG_WD_CONF, > > > + BD96801_WD_ASSERT_MASK, > > > + BD96801_WD_ASSERT_RST); > > > + return ret; > > > + } > > > + > > > + ret = device_property_match_string(w->dev->parent, "rohm,wdg-action", > > > + "intb-only"); > > > + if (ret >= 0) { > > > + ret = regmap_update_bits(w->regmap, BD96801_REG_WD_CONF, > > > + BD96801_WD_ASSERT_MASK, > > > + BD96801_WD_ASSERT_IRQ); > > > + return ret; > > > + } > > > > I don't see the devicetree bindings documented in the series. > > Seems like I have missed this WDG binding. But after reading your comment > below, I am wondering if I should just drop the binding and default to > "prstb" (shutdown should the feeding be skipped) - and leave the "intb-only" > case for one who actually needs such. > > > I am also a bit surprised that the interrupt isn't handled in the driver. > > Please explain. > > Basically, I just had no idea what the IRQ should do in the generic case. If > we get an interrupt, it means the WDG feeding has failed. My thinking is > that, what should happen is forced reset. I don't see how that can be done > in reliably manner from an IRQ handler. > > When the "prstb WDG action" is set (please, see the above DT binding > handling), the PMIC shall shut down power outputs. This should get the > watchdog's job done. > > With the "intb-only"-option, PMIC will not turn off the power. I'd expect > there to be some external HW connection which handles the reset by HW. > > After all this being said, I wonder if I should just unconditionally > configure the PMIC to always turn off the power (prstb option) should the > feeding fail? Or do someone have some suggestion what the IRQ handler should > do (except maybe print an error msg)? > Other watchdog drivers call emergency_restart() if the watchdog times out and triggers an interrupt. Are you saying this won't work for this system ? If so, please explain. Thanks, Guenter
On 4/3/24 15:41, Guenter Roeck wrote: > On Wed, Apr 03, 2024 at 09:34:35AM +0300, Matti Vaittinen wrote: >> Hi Guenter, >> >> First of all, thanks for the review. It was quick! Especially when we speak >> of a RFC series. Very much appreciated. >> >> On 4/2/24 20:11, Guenter Roeck wrote: >>> On Tue, Apr 02, 2024 at 04:11:41PM +0300, Matti Vaittinen wrote >> +static int init_wdg_hw(struct wdtbd96801 *w) >>>> +{ >>>> + u32 hw_margin[2]; >>>> + int count, ret; >>>> + u32 hw_margin_max = BD96801_WDT_DEFAULT_MARGIN, hw_margin_min = 0; >>>> + >>>> + count = device_property_count_u32(w->dev->parent, "rohm,hw-timeout-ms"); >>>> + if (count < 0 && count != -EINVAL) >>>> + return count; >>>> + >>>> + if (count > 0) { >>>> + if (count > ARRAY_SIZE(hw_margin)) >>>> + return -EINVAL; >>>> + >>>> + ret = device_property_read_u32_array(w->dev->parent, >>>> + "rohm,hw-timeout-ms", >>>> + &hw_margin[0], count); >>>> + if (ret < 0) >>>> + return ret; >>>> + >>>> + if (count == 1) >>>> + hw_margin_max = hw_margin[0]; >>>> + >>>> + if (count == 2) { >>>> + hw_margin_max = hw_margin[1]; >>>> + hw_margin_min = hw_margin[0]; >>>> + } >>>> + } >>>> + >>>> + ret = bd96801_set_wdt_mode(w, hw_margin_max, hw_margin_min); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + ret = device_property_match_string(w->dev->parent, "rohm,wdg-action", >>>> + "prstb"); >>>> + if (ret >= 0) { >>>> + ret = regmap_update_bits(w->regmap, BD96801_REG_WD_CONF, >>>> + BD96801_WD_ASSERT_MASK, >>>> + BD96801_WD_ASSERT_RST); >>>> + return ret; >>>> + } >>>> + >>>> + ret = device_property_match_string(w->dev->parent, "rohm,wdg-action", >>>> + "intb-only"); >>>> + if (ret >= 0) { >>>> + ret = regmap_update_bits(w->regmap, BD96801_REG_WD_CONF, >>>> + BD96801_WD_ASSERT_MASK, >>>> + BD96801_WD_ASSERT_IRQ); >>>> + return ret; >>>> + } >>> >>> I don't see the devicetree bindings documented in the series. >> >> Seems like I have missed this WDG binding. But after reading your comment >> below, I am wondering if I should just drop the binding and default to >> "prstb" (shutdown should the feeding be skipped) - and leave the "intb-only" >> case for one who actually needs such. >> >>> I am also a bit surprised that the interrupt isn't handled in the driver. >>> Please explain. >> >> Basically, I just had no idea what the IRQ should do in the generic case. If >> we get an interrupt, it means the WDG feeding has failed. My thinking is >> that, what should happen is forced reset. I don't see how that can be done >> in reliably manner from an IRQ handler. >> >> When the "prstb WDG action" is set (please, see the above DT binding >> handling), the PMIC shall shut down power outputs. This should get the >> watchdog's job done. >> >> With the "intb-only"-option, PMIC will not turn off the power. I'd expect >> there to be some external HW connection which handles the reset by HW. >> >> After all this being said, I wonder if I should just unconditionally >> configure the PMIC to always turn off the power (prstb option) should the >> feeding fail? Or do someone have some suggestion what the IRQ handler should >> do (except maybe print an error msg)? >> > > Other watchdog drivers call emergency_restart() if the watchdog times out > and triggers an interrupt. Are you saying this won't work for this system ? > If so, please explain. > Thanks Guenter. If it works with systems using other devices, then it should work (to the same extent) with systems using this PMIC. Thanks. I'll add the IRQ handling to next version - but it may take a while as I'm currently having some problems with the IRQs in general, and because I'll wait for feedback from Mark to the regulator part. Yours, -- Matti
On Wed, Apr 03, 2024 at 03:47:12PM +0300, Matti Vaittinen wrote: > > > > Other watchdog drivers call emergency_restart() if the watchdog times out > > and triggers an interrupt. Are you saying this won't work for this system ? > > If so, please explain. > > > > Thanks Guenter. If it works with systems using other devices, then it should > work (to the same extent) with systems using this PMIC. Thanks. > You might also consider to just call panic(). What is what we do if the pretimeout panic governor is enabled. That makes me wonder if it would make sense to introduce watchdog timeout governors, similar to the existing pretimeout governors. Maybe if I ever find the time to do it ... Guenter > I'll add the IRQ handling to next version - but it may take a while as I'm > currently having some problems with the IRQs in general, and because I'll > wait for feedback from Mark to the regulator part. > > Yours, > -- Matti > > -- > Matti Vaittinen > Linux kernel developer at ROHM Semiconductors > Oulu Finland > > ~~ When things go utterly wrong vim users can always type :help! ~~ >
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index 6bee137cfbe0..d97e735e1faa 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -181,6 +181,19 @@ config BD957XMUF_WATCHDOG watchdog. Alternatively say M to compile the driver as a module, which will be called bd9576_wdt. +config BD96801_WATCHDOG + tristate "ROHM BD96801 PMIC Watchdog" + depends on MFD_ROHM_BD96801 + select WATCHDOG_CORE + help + Support for the watchdog in the ROHM BD96801 PMIC. Watchdog can be + configured to only generate IRQ or to trigger system reset via reset + pin. + + Say Y here to include support for the ROHM BD96801 watchdog. + Alternatively say M to compile the driver as a module, + which will be called bd96801_wdt. + config CROS_EC_WATCHDOG tristate "ChromeOS EC-based watchdog" select WATCHDOG_CORE diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile index 3710c218f05e..31bc94436c81 100644 --- a/drivers/watchdog/Makefile +++ b/drivers/watchdog/Makefile @@ -217,6 +217,7 @@ obj-$(CONFIG_XEN_WDT) += xen_wdt.o # Architecture Independent obj-$(CONFIG_BD957XMUF_WATCHDOG) += bd9576_wdt.o +obj-$(CONFIG_BD96801_WATCHDOG) += bd96801_wdt.o obj-$(CONFIG_CROS_EC_WATCHDOG) += cros_ec_wdt.o obj-$(CONFIG_DA9052_WATCHDOG) += da9052_wdt.o obj-$(CONFIG_DA9055_WATCHDOG) += da9055_wdt.o diff --git a/drivers/watchdog/bd96801_wdt.c b/drivers/watchdog/bd96801_wdt.c new file mode 100644 index 000000000000..cb2b526ecc21 --- /dev/null +++ b/drivers/watchdog/bd96801_wdt.c @@ -0,0 +1,375 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (C) 2022 ROHM Semiconductors + * + * ROHM BD96801 watchdog driver + */ + +#include <linux/kernel.h> +#include <linux/mfd/rohm-bd96801.h> +#include <linux/mfd/rohm-generic.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> +#include <linux/watchdog.h> + +static bool nowayout; +module_param(nowayout, bool, 0); +MODULE_PARM_DESC(nowayout, + "Watchdog cannot be stopped once started (default=\"false\")"); + +#define BD96801_WD_TMO_SHORT_MASK 0x70 +#define BD96801_WD_RATIO_MASK 0x3 +#define BD96801_WD_TYPE_MASK 0x4 +#define BD96801_WD_TYPE_SLOW 0x4 +#define BD96801_WD_TYPE_WIN 0x0 + +#define BD96801_WD_EN_MASK 0x3 +#define BD96801_WD_IF_EN 0x1 +#define BD96801_WD_QA_EN 0x2 +#define BD96801_WD_DISABLE 0x0 + +#define BD96801_WD_ASSERT_MASK 0x8 +#define BD96801_WD_ASSERT_RST 0x8 +#define BD96801_WD_ASSERT_IRQ 0x0 + +#define BD96801_WD_FEED_MASK 0x1 +#define BD96801_WD_FEED 0x1 + +/* units in uS */ +#define FASTNG_MIN 3370 +#define BD96801_WDT_DEFAULT_MARGIN 6905120 +/* Unit is seconds */ +#define DEFAULT_TIMEOUT 30 + +/* + * BD96801 WDG supports window mode so the TMO consists of SHORT and LONG + * timeout values. SHORT time is meaningfull only in window mode where feeding + * period shorter than SHORT would be an error. LONG time is used to detect if + * feeding is not occurring within given time limit (SoC SW hangs). The LONG + * timeout time is a multiple of (2, 4, 8 0r 16 times) the SHORT timeout. + */ + +struct wdtbd96801 { + struct device *dev; + struct regmap *regmap; + bool always_running; + struct watchdog_device wdt; +}; + +static int bd96801_wdt_ping(struct watchdog_device *wdt) +{ + struct wdtbd96801 *w = watchdog_get_drvdata(wdt); + + return regmap_update_bits(w->regmap, BD96801_REG_WD_FEED, + BD96801_WD_FEED_MASK, BD96801_WD_FEED); +} + +static int bd96801_wdt_start(struct watchdog_device *wdt) +{ + struct wdtbd96801 *w = watchdog_get_drvdata(wdt); + int ret; + + ret = regmap_update_bits(w->regmap, BD96801_REG_WD_CONF, + BD96801_WD_EN_MASK, BD96801_WD_IF_EN); + + return ret; +} + +static int bd96801_wdt_stop(struct watchdog_device *wdt) +{ + struct wdtbd96801 *w = watchdog_get_drvdata(wdt); + + if (!w->always_running) + return regmap_update_bits(w->regmap, BD96801_REG_WD_CONF, + BD96801_WD_EN_MASK, BD96801_WD_DISABLE); + set_bit(WDOG_HW_RUNNING, &wdt->status); + + return 0; +} + +static const struct watchdog_info bd96801_wdt_info = { + .options = WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING | + WDIOF_SETTIMEOUT, + .identity = "BD96801 Watchdog", +}; + +static const struct watchdog_ops bd96801_wdt_ops = { + .start = bd96801_wdt_start, + .stop = bd96801_wdt_stop, + .ping = bd96801_wdt_ping, +}; + +static int find_closest_fast(int target, int *sel, int *val) +{ + int i; + int window = FASTNG_MIN; + + for (i = 0; i < 8 && window < target; i++) + window <<= 1; + + *val = window; + *sel = i; + + if (i == 8) + return -EINVAL; + + return 0; +} + +static int find_closest_slow_by_fast(int fast_val, int *target, int *slowsel) +{ + int sel; + static const int multipliers[] = {2, 4, 8, 16}; + + for (sel = 0; sel < ARRAY_SIZE(multipliers) && + multipliers[sel] * fast_val < *target; sel++) + ; + + if (sel == ARRAY_SIZE(multipliers)) + return -EINVAL; + + *slowsel = sel; + *target = multipliers[sel] * fast_val; + + return 0; +} + +static int find_closest_slow(int *target, int *slow_sel, int *fast_sel) +{ + static const int multipliers[] = {2, 4, 8, 16}; + int i, j; + int val = 0; + int window = FASTNG_MIN; + + for (i = 0; i < 8; i++) { + for (j = 0; j < ARRAY_SIZE(multipliers); j++) { + int slow; + + slow = window * multipliers[j]; + if (slow >= *target && (!val || slow < val)) { + val = slow; + *fast_sel = i; + *slow_sel = j; + } + } + window <<= 1; + } + if (!val) + return -EINVAL; + + *target = val; + + return 0; +} + +static int bd96801_set_wdt_mode(struct wdtbd96801 *w, int hw_margin, + int hw_margin_min) +{ + int ret, fastng, slowng, type, reg, mask; + struct device *dev = w->dev; + + /* convert to uS */ + hw_margin *= 1000; + hw_margin_min *= 1000; + if (hw_margin_min) { + int min; + + type = BD96801_WD_TYPE_WIN; + dev_dbg(dev, "Setting type WINDOW 0x%x\n", type); + ret = find_closest_fast(hw_margin_min, &fastng, &min); + if (ret) { + dev_err(dev, "bad WDT window for fast timeout\n"); + return ret; + } + + ret = find_closest_slow_by_fast(min, &hw_margin, &slowng); + if (ret) { + dev_err(dev, "bad WDT window\n"); + return ret; + } + w->wdt.min_hw_heartbeat_ms = min / 1000; + } else { + type = BD96801_WD_TYPE_SLOW; + dev_dbg(dev, "Setting type SLOW 0x%x\n", type); + ret = find_closest_slow(&hw_margin, &slowng, &fastng); + if (ret) { + dev_err(dev, "bad WDT window\n"); + return ret; + } + } + + w->wdt.max_hw_heartbeat_ms = hw_margin / 1000; + + fastng <<= ffs(BD96801_WD_TMO_SHORT_MASK) - 1; + + reg = slowng | fastng; + mask = BD96801_WD_RATIO_MASK | BD96801_WD_TMO_SHORT_MASK; + ret = regmap_update_bits(w->regmap, BD96801_REG_WD_TMO, + mask, reg); + if (ret) + return ret; + + ret = regmap_update_bits(w->regmap, BD96801_REG_WD_CONF, + BD96801_WD_TYPE_MASK, type); + + return ret; +} + +static int bd96801_set_heartbeat_from_hw(struct wdtbd96801 *w, + unsigned int conf_reg) +{ + int ret; + unsigned int val, sel, fast; + + /* + * The BD96801 supports a somewhat peculiar QA-mode, which we do not + * support in this driver. If the QA-mode is enabled then we just + * warn and bail-out. + */ + if ((conf_reg & BD96801_WD_EN_MASK) != BD96801_WD_IF_EN) { + dev_warn(w->dev, "watchdog set to Q&A mode - exiting\n"); + return -EINVAL; + } + + ret = regmap_read(w->regmap, BD96801_REG_WD_TMO, &val); + if (ret) + return ret; + + sel = val & BD96801_WD_TMO_SHORT_MASK; + sel >>= ffs(BD96801_WD_TMO_SHORT_MASK) - 1; + fast = FASTNG_MIN << sel; + + sel = (val & BD96801_WD_RATIO_MASK) + 1; + w->wdt.max_hw_heartbeat_ms = (fast << sel) / USEC_PER_MSEC; + + if ((conf_reg & BD96801_WD_TYPE_MASK) == BD96801_WD_TYPE_WIN) + w->wdt.min_hw_heartbeat_ms = fast / USEC_PER_MSEC; + + return 0; +} + +static int init_wdg_hw(struct wdtbd96801 *w) +{ + u32 hw_margin[2]; + int count, ret; + u32 hw_margin_max = BD96801_WDT_DEFAULT_MARGIN, hw_margin_min = 0; + + count = device_property_count_u32(w->dev->parent, "rohm,hw-timeout-ms"); + if (count < 0 && count != -EINVAL) + return count; + + if (count > 0) { + if (count > ARRAY_SIZE(hw_margin)) + return -EINVAL; + + ret = device_property_read_u32_array(w->dev->parent, + "rohm,hw-timeout-ms", + &hw_margin[0], count); + if (ret < 0) + return ret; + + if (count == 1) + hw_margin_max = hw_margin[0]; + + if (count == 2) { + hw_margin_max = hw_margin[1]; + hw_margin_min = hw_margin[0]; + } + } + + ret = bd96801_set_wdt_mode(w, hw_margin_max, hw_margin_min); + if (ret) + return ret; + + ret = device_property_match_string(w->dev->parent, "rohm,wdg-action", + "prstb"); + if (ret >= 0) { + ret = regmap_update_bits(w->regmap, BD96801_REG_WD_CONF, + BD96801_WD_ASSERT_MASK, + BD96801_WD_ASSERT_RST); + return ret; + } + + ret = device_property_match_string(w->dev->parent, "rohm,wdg-action", + "intb-only"); + if (ret >= 0) { + ret = regmap_update_bits(w->regmap, BD96801_REG_WD_CONF, + BD96801_WD_ASSERT_MASK, + BD96801_WD_ASSERT_IRQ); + return ret; + } + + return 0; +} + +static int bd96801_wdt_probe(struct platform_device *pdev) +{ + struct wdtbd96801 *w; + int ret; + unsigned int val; + + w = devm_kzalloc(&pdev->dev, sizeof(*w), GFP_KERNEL); + if (!w) + return -ENOMEM; + + w->regmap = dev_get_regmap(pdev->dev.parent, NULL); + w->dev = &pdev->dev; + + w->wdt.info = &bd96801_wdt_info; + w->wdt.ops = &bd96801_wdt_ops; + w->wdt.parent = pdev->dev.parent; + w->wdt.timeout = DEFAULT_TIMEOUT; + watchdog_set_drvdata(&w->wdt, w); + + w->always_running = device_property_read_bool(pdev->dev.parent, + "always-running"); + + ret = regmap_read(w->regmap, BD96801_REG_WD_CONF, &val); + if (ret) + return dev_err_probe(&pdev->dev, ret, + "Failed to get the watchdog state\n"); + + /* + * If the WDG is already enabled we assume it is configured by boot. + * In this case we just update the hw-timeout based on values set to + * the timeout / mode registers and leave the hardware configs + * untouched. + */ + if ((val & BD96801_WD_EN_MASK) != BD96801_WD_DISABLE) { + dev_dbg(&pdev->dev, "watchdog was running during probe\n"); + ret = bd96801_set_heartbeat_from_hw(w, val); + if (ret) + return ret; + + set_bit(WDOG_HW_RUNNING, &w->wdt.status); + } else { + /* If WDG is not running so we will initializate it */ + ret = init_wdg_hw(w); + if (ret) + return ret; + } + + watchdog_init_timeout(&w->wdt, 0, pdev->dev.parent); + watchdog_set_nowayout(&w->wdt, nowayout); + watchdog_stop_on_reboot(&w->wdt); + + if (w->always_running) + bd96801_wdt_start(&w->wdt); + + return devm_watchdog_register_device(&pdev->dev, &w->wdt); +} + +static struct platform_driver bd96801_wdt = { + .driver = { + .name = "bd96801-wdt" + }, + .probe = bd96801_wdt_probe, +}; +module_platform_driver(bd96801_wdt); + +MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>"); +MODULE_DESCRIPTION("BD96801 watchdog driver"); +MODULE_LICENSE("GPL"); +MODULE_ALIAS("platform:bd96801-wdt");
Introduce driver for WDG block on ROHM BD96801 scalable PMIC. This driver only supports watchdog with I2C feeding and delayed response detection. Whether the watchdog toggles PRSTB pin or just causes an interrupt can be configured via device-tree. The BD96801 PMIC HW supports also window watchdog (too early feeding detection) and Q&A mode. These are not supported by this driver. Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> --- drivers/watchdog/Kconfig | 13 ++ drivers/watchdog/Makefile | 1 + drivers/watchdog/bd96801_wdt.c | 375 +++++++++++++++++++++++++++++++++ 3 files changed, 389 insertions(+) create mode 100644 drivers/watchdog/bd96801_wdt.c