Re: [PATCH v4] watchdog: qcom_wdt: set WDOG_HW_RUNNING bit when appropriate

[Kathiravan T <kathirav@codeaurora.org>]

On 10/31/2020 7:38 PM, Guenter Roeck wrote:
> On 10/31/20 5:11 AM, Robert Marko wrote:
>> If the watchdog hardware is enabled/running during boot, e.g.
>> due to a boot loader configuring it, we must tell the
>> watchdog framework about this fact so that it can ping the
>> watchdog until userspace opens the device and takes over
>> control.
>>
>> Do so using the WDOG_HW_RUNNING flag that exists for exactly
>> that use-case.
>>
>> Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>

Thanks for addressing the comments and now the patch looks good to me. 
One more suggestion, can we make the initcall level of the driver to 
subsys_initcall_sync so that the driver gets registered immediately 
after the watchdog_core is registered and watchdog_core starts pinging 
the WDT?

>> Cc: Luka Perkov <luka.perkov@sartura.hr>
>> ---
>> Changes in v4:
>> * Use QCOM_WDT_ENABLE macro
>>
>> Changes in v3:
>> * Drop call to stop as start already does it
>> * Update commit message
>>
>> Changes in v2:
>> * Correct authorship
>>
>>   drivers/watchdog/qcom-wdt.c | 18 ++++++++++++++++++
>>   1 file changed, 18 insertions(+)
>>
>> diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
>> index ab7465d186fd..07d399c4edc4 100644
>> --- a/drivers/watchdog/qcom-wdt.c
>> +++ b/drivers/watchdog/qcom-wdt.c
>> @@ -152,6 +152,13 @@ static int qcom_wdt_restart(struct watchdog_device *wdd, unsigned long action,
>>   	return 0;
>>   }
>>   
>> +static int qcom_wdt_is_running(struct watchdog_device *wdd)
>> +{
>> +	struct qcom_wdt *wdt = to_qcom_wdt(wdd);
>> +
>> +	return (readl(wdt_addr(wdt, WDT_EN)) & QCOM_WDT_ENABLE);
>> +}
>> +
>>   static const struct watchdog_ops qcom_wdt_ops = {
>>   	.start		= qcom_wdt_start,
>>   	.stop		= qcom_wdt_stop,
>> @@ -294,6 +301,17 @@ static int qcom_wdt_probe(struct platform_device *pdev)
>>   	wdt->wdd.timeout = min(wdt->wdd.max_timeout, 30U);
>>   	watchdog_init_timeout(&wdt->wdd, 0, dev);
>>   
>> +	/*
>> +	 * If WDT is already running, call WDT start which
>> +	 * will stop the WDT, set timeouts as bootloader
>> +	 * might use different ones and set running bit
>> +	 * to inform the WDT subsystem to ping the WDT
>> +	 */
>> +	if (qcom_wdt_is_running(&wdt->wdd)) {
>> +		qcom_wdt_start(&wdt->wdd);
>> +		set_bit(WDOG_HW_RUNNING, &wdt->wdd.status);
>> +	}
>> +
>>   	ret = devm_watchdog_register_device(dev, &wdt->wdd);
>>   	if (ret)
>>   		return ret;
>>
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation