Re: [EXT] Re: [PATCH 2/2] Watchdog: Add octeontx2 GTI watchdog driver

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 14/04/2023 13:44, Bharat Bhushan wrote:
> Please see inline 
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> Sent: Friday, April 14, 2023 4:58 PM
>> To: Bharat Bhushan <bbhushan2@marvell.com>; wim@linux-watchdog.org;
>> linux@roeck-us.net; robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org;
>> linux-watchdog@vger.kernel.org; devicetree@vger.kernel.org; linux-
>> kernel@vger.kernel.org
>> Subject: [EXT] Re: [PATCH 2/2] Watchdog: Add octeontx2 GTI watchdog driver
>>
>> External Email
>>
>> ----------------------------------------------------------------------
>> On 14/04/2023 12:23, Bharat Bhushan wrote:
>>> GTI watchdog timer are programmed in "interrupt + del3t + reset mode"
>>> and del3t traps are not enabled.
>>> GTI watchdog exception flow is:
>>>  - 1st timer expiration generates watchdog interrupt.
>>>  - 2nd timer expiration is ignored
>>>  - On 3rd timer expiration will trigger a system-wide core reset.
>>>
>>> Signed-off-by: Bharat Bhushan <bbhushan2@marvell.com>
>>> ---
>>>  drivers/watchdog/Kconfig         |   9 ++
>>>  drivers/watchdog/Makefile        |   1 +
>>>  drivers/watchdog/octeontx2_wdt.c | 238
>>> +++++++++++++++++++++++++++++++
>>>  3 files changed, 248 insertions(+)
>>>  create mode 100644 drivers/watchdog/octeontx2_wdt.c
>>>
>>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index
>>> f0872970daf9..31ff282c62ad 100644
>>> --- a/drivers/watchdog/Kconfig
>>> +++ b/drivers/watchdog/Kconfig
>>> @@ -2212,4 +2212,13 @@ config KEEMBAY_WATCHDOG
>>>  	  To compile this driver as a module, choose M here: the
>>>  	  module will be called keembay_wdt.
>>>
>>> +config OCTEONTX2_WATCHDOG
>>> +	tristate "OCTEONTX2 Watchdog driver"
>>> +	depends on ARCH_THUNDER || (COMPILE_TEST && 64BIT)
>>
>> Why it cannot be compile tested on 32-bit?
> 
> Used in 64 bit configuration but no harm getting compile tested for 32bit.
> Will change
> 
>>
>>> +	help
>>> +	 OCTEONTX2 GTI hardware supports watchdog timer. This watchdog
>> timer are
>>> +	 programmed in "interrupt + del3t + reset" mode. On first expiry it will
>>> +	 generate interrupt. Second expiry (del3t) is ignored and system will reset
>>> +	 on final timer expiry.
>>> +
>>>  endif # WATCHDOG
>>> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
>>> index 9cbf6580f16c..aa1b813ad1f9 100644
>>> --- a/drivers/watchdog/Makefile
>>> +++ b/drivers/watchdog/Makefile
>>> @@ -230,3 +230,4 @@ obj-$(CONFIG_MENZ069_WATCHDOG) +=
>> menz69_wdt.o
>>>  obj-$(CONFIG_RAVE_SP_WATCHDOG) += rave-sp-wdt.o
>>>  obj-$(CONFIG_STPMIC1_WATCHDOG) += stpmic1_wdt.o
>>>  obj-$(CONFIG_SL28CPLD_WATCHDOG) += sl28cpld_wdt.o
>>> +obj-$(CONFIG_OCTEONTX2_WATCHDOG) += octeontx2_wdt.o
>>
>> Please tell me that you added it in some reasonable place, not just at the end...
>> The same in Kconfig.
> 
> Is it alphabetical order, any suggestion?

'O' is not after 'S', thus for sure you did not add it in alphabetical
order.

Or what is a question? If so, then yes, usually we try to keep
alphabetical order. Anyway adding entries to the end is conflictprone.


>>> +	dev_info(dev, "Watchdog enabled (timeout=%d sec)\n", wdog_dev-
>>> timeout);
>>> +	return 0;
>>> +}
>>> +
>>> +static int octeontx2_wdt_remove(struct platform_device *pdev) {
>>> +	struct octeontx2_wdt_priv *priv = platform_get_drvdata(pdev);
>>> +
>>> +	if (priv->irq)
>>
>> Is it possible?

You did not reply, so I assume it is not possible. Drop.

>>
>>> +		free_irq(priv->irq, priv);
>>
>> Anyway, your order of cleanup is a bit surprising. It is expected to be reversed
>> from probe. In probe() you requested IRQs before watchdog, but cleanup will be
>> done before watchdog release? This does not look right.
> 
> Watchdog release happen outside this driver as we used devm_*. 

I know, this is why I raised the question. Don't repeat the obvious but
rather address the problem mentioned here.


> Will convert request_irq to devm_request_irq(). 

If interrupt is not shared, then looks like correct approach.

Best regards,
Krzysztof