Message-ID: <279336b3-f28d-48ee-a10f-47abba7b0b89@gmail.com>
Date: Wed, 3 Apr 2024 09:34:35 +0300
From: Matti Vaittinen <mazziesaccount@...il.com>
To: Guenter Roeck <linux@...ck-us.net>
Cc: Matti Vaittinen <matti.vaittinen@...rohmeurope.com>,
 Lee Jones <lee@...nel.org>, Wim Van Sebroeck <wim@...ux-watchdog.org>,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-watchdog@...r.kernel.org
Subject: Re: [RFC PATCH 5/6] watchdog: ROHM BD96801 PMIC WDG driver

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

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

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

Powered by blists - more mailing lists