Re: [RFC PATCH v2 5/6] watchdog: ROHM BD96801 PMIC WDG driver

[Matti Vaittinen <mazziesaccount@gmail.com>]

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! ~~