Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <512ac643-c487-ed83-9bf5-242f4890c680@linaro.org>
Date:   Fri, 14 Apr 2023 14:28:13 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.1
Subject: Re: [EXT] Re: [PATCH 2/2] Watchdog: Add octeontx2 GTI watchdog driver
To:     Bharat Bhushan <bbhushan2@marvell.com>,
        "wim@linux-watchdog.org" <wim@linux-watchdog.org>,
        "linux@roeck-us.net" <linux@roeck-us.net>,
        "robh+dt@kernel.org" <robh+dt@kernel.org>,
        "krzysztof.kozlowski+dt@linaro.org" 
        <krzysztof.kozlowski+dt@linaro.org>,
        "linux-watchdog@vger.kernel.org" <linux-watchdog@vger.kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
References: <20230414102342.23696-1-bbhushan2@marvell.com>
 <20230414102342.23696-2-bbhushan2@marvell.com>
 <832d7830-be98-243c-ebf7-c23ae51e4ccc@linaro.org>
 <DM5PR1801MB1883F11EF48041C7B9EF3D9EE3999@DM5PR1801MB1883.namprd18.prod.outlook.com>
Content-Language: en-US
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <DM5PR1801MB1883F11EF48041C7B9EF3D9EE3999@DM5PR1801MB1883.namprd18.prod.outlook.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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