On 14/04/2023 13:44, Bharat Bhushan wrote:
> Please see inline
>
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
>> Sent: Friday, April 14, 2023 4:58 PM
>> To: Bharat Bhushan <bbhushan2@xxxxxxxxxxx>; wim@xxxxxxxxxxxxxxxxxx;
>> linux@xxxxxxxxxxxx; robh+dt@xxxxxxxxxx; krzysztof.kozlowski+dt@xxxxxxxxxx;
>> linux-watchdog@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-
>> kernel@xxxxxxxxxxxxxxx
>> 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@xxxxxxxxxxx>
>>> ---
>>> 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
