lkml.org 
[lkml]   [2024]   [Apr]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [RFC PATCH v2 5/6] watchdog: ROHM BD96801 PMIC WDG driver
From
Hi George,

Thanks for the input! Reviewing is very much appreciated :)

On 4/17/24 07:29, George Cherian wrote:
> On 2024-04-12 at 16:52:46, Matti Vaittinen (mazziesaccount@gmail.com) 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>
>>
>> ---
>> Revision history:
>> RFCv1 => RFCv2:
>> - remove always running
>> - add IRQ handling
>> - call emergency_restart()
>> - drop MODULE_ALIAS and add MODULE_DEVICE_TABLE
>> ---
>> drivers/watchdog/Kconfig | 13 ++
>> drivers/watchdog/Makefile | 1 +
>> drivers/watchdog/bd96801_wdt.c | 389 +++++++++++++++++++++++++++++++++
>> 3 files changed, 403 insertions(+)
>> create mode 100644 drivers/watchdog/bd96801_wdt.c
>>4
>> +
>> +static const struct watchdog_ops bd96801_wdt_ops = {
>> + .start = bd96801_wdt_start,
>> + .stop = bd96801_wdt_stop,
>> + .ping = bd96801_wdt_ping,
>> +};
> Is there no way to setup a timeout to the WDOG device from userspace?
>

For the BD96801 hardware? Currently no. (See below)

>> +
>> +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");
> Why is that timeout need to be configured from a devicce-tree property?
> set_timeout/get_timeout can't be done for this device?

The BD96801 supports 32 different "slow detection" timeouts - only 1 of
which is guaranteed to be greater than 1s (1.8432 Sec). The timeout
setting interface uses seconds.

Furthermore, I think that the HW heartbeat is not all that meaningful to
the user-space. What is meaningful to user-space is how often the
userland daemon needs to write the watchdog file to keep system alive.
This is different thing. With the sub-second HW timeouts it is better
the kernel keeps feeding the watchdog based on a timer (adjusted to meet
the HW requirements) and can thus require the userspace to ping less
frequently than the HW heartbeat.

>> + 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;
>> +}
>> +
>> +extern void emergency_restart(void);
>> +static irqreturn_t bd96801_irq_hnd(int irq, void *data)
>> +{
>> + emergency_restart();
> In case of a full system hang will this function get executed?

Maybe, maybe not. It is probably still the best we can do if the
watchdog has not been configured to kill the power by HW. Or, do you
have some other suggestion? (The emergency_restart() was actually
suggested by Guenter during the v1 review).

>> + return IRQ_NONE;
>> +}

Yours,
-- Matti

--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~


\
 
 \ /
  Last update: 2024-04-17 09:36    [W:0.045 / U:0.592 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site