lkml.org 
[lkml]   [2023]   [Apr]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [EXT] Re: [PATCH 2/2] Watchdog: Add octeontx2 GTI watchdog driver
From
On 4/17/23 04:06, Bharat Bhushan wrote:
> Please see inline
>
>> -----Original Message-----
>> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
>> Sent: Friday, April 14, 2023 8:05 PM
>> To: Bharat Bhushan <bbhushan2@marvell.com>
>> Cc: wim@linux-watchdog.org; 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 Fri, Apr 14, 2023 at 03:53:42PM +0530, 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.
>>>
>>
>> This means the interrupt should not result in a panic, the programmed timeout
>> value should be considered a pretimeout which is set to (timeout / 3), and the
>> interrupt handler should call watchdog_notify_pretimeout().
>>
>> Either case, the above should be documented in the driver but does not make
>> much if any sense as patch description. If not, what are the other modes, and
>> why is this mode used instead of any of those modes ?
>
> Hardware supports following mode of operation:
> 1) Interrupt Only:
> This will generate the interrupt to arm core whenever timeout happens.
>
> 2) Interrupt + del3t (Interrupt to firmware (SCP processor)).
> This will generate interrupt to arm core whenever 1st timeout happens
> This will generate interrupt to SCP processor whenever 2nd timeout happens
>
> 3) Interrupt + Interrupt to SCP processor (called delt3t) + reboot.
> This will generate interrupt to arm core whenever 1st timeout happens
> This will generate interrupt to SCP processor whenever 2nd timeout happens, if interrupt is configured.
> This will reboot on 3rd timeout.
>
> We are going with mode-3 above so that system can reboot in case a hardware hang. Also h/w is configured not to generate SCP interrupt, so effectively 2nd timeout is ignored within hardware.
>
> Software is supposed to poke within 1st timeout. If poke does not happen then it will receive interrupt, interrupt handler will do panic.
> But for some reason if processor can not take interrupt then system will reboot on 3rd timeout.
>
Then, as I said, the first timeout shopuld be modeled as pretimeout.

Thanks,
Guenter

>>
>>> 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)
>>> + 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.
>>> +
>>
>> The above should be part of the in-driver documentation but those details
>> should not be in Kconfig.
>
> Ok,
>
>>
>> Is the documentation available in public ? I'd like to check if this mode, especially
>> the ignored del3t mode, really makes sense.
>
> Documentation is not public. Provided some description above, let me know if I need to provide some more details.
>
>>
>>> 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
>>> diff --git a/drivers/watchdog/octeontx2_wdt.c
>>> b/drivers/watchdog/octeontx2_wdt.c
>>> new file mode 100644
>>> index 000000000000..7b78a092e83f
>>> --- /dev/null
>>> +++ b/drivers/watchdog/octeontx2_wdt.c
>>> @@ -0,0 +1,238 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/* Marvell Octeontx2 Watchdog driver
>>> + *
>>> + * Copyright (C) 2023 Marvell International Ltd.
>>> + *
>>> + * This program is free software; you can redistribute it and/or
>>> +modify
>>> + * it under the terms of the GNU General Public License version 2 as
>>> + * published by the Free Software Foundation.
>>> + */
>>> +
>>> +#include <linux/module.h>
>>> +#include <linux/cpu.h>
>>> +#include <linux/interrupt.h>
>>> +#include <linux/of_platform.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/watchdog.h>
>>> +#include <linux/sched/debug.h>
>>
>> What is this include for ?
>
> Taken from other driver when started coding this one. Will remove
>
>>
>>> +
>>> +#include <asm/arch_timer.h>
>>> +
>>> +/* GTI CWD Watchdog Registers */
>>> +#define GTI_CWD_GLOBAL_WDOG_IDX 63
>>> +#define GTI_CWD_WDOG (0x8 *
>> GTI_CWD_GLOBAL_WDOG_IDX)
>>> +#define GTI_CWD_WDOG_MODE_INT_DEL3T_RST 0x3
>>> +#define GTI_CWD_WDOG_MODE_MASK 0x3
>>> +#define GTI_CWD_WDOG_LEN_SHIFT 4
>>> +#define GTI_CWD_WDOG_CNT_SHIFT 20
>>> +
>>> +/* GTI Per-core Watchdog Interrupt Register */
>>> +#define GTI_CWD_INT 0x200
>>> +#define GTI_CWD_INT_PENDING_STATUS (1ULL <<
>> GTI_CWD_GLOBAL_WDOG_IDX)
>>> +
>>> +/* GTI Per-core Watchdog Interrupt Enable Clear Register */
>>> +#define GTI_CWD_INT_ENA_CLR 0x210
>>> +#define GTI_CWD_INT_ENA_CLR_VAL (1ULL <<
>> GTI_CWD_GLOBAL_WDOG_IDX)
>>> +
>>> +/* GTI Per-core Watchdog Interrupt Enable Set Register */
>>> +#define GTI_CWD_INT_ENA_SET 0x218
>>> +#define GTI_CWD_INT_ENA_SET_VAL (1ULL <<
>> GTI_CWD_GLOBAL_WDOG_IDX)
>>> +
>>> +/* GTI Per-core Watchdog Poke Registers */
>>> +#define GTI_CWD_POKE (0x10000 + 0x8 *
>> GTI_CWD_GLOBAL_WDOG_IDX)
>>> +#define GTI_CWD_POKE_VAL (1ULL << GTI_CWD_GLOBAL_WDOG_IDX)
>>> +
>>> +struct octeontx2_wdt_priv {
>>> + struct watchdog_device wdev;
>>> + void __iomem *base;
>>> + u64 clock_freq;
>>> + int irq;
>>> +};
>>> +
>>> +static irqreturn_t octeontx2_wdt_interrupt(int irq, void *data) {
>>> + panic("Kernel Watchdog");
>>> + return IRQ_HANDLED;
>>> +}
>>> +
>>> +static int octeontx2_wdt_ping(struct watchdog_device *wdev) {
>>> + struct octeontx2_wdt_priv *priv = watchdog_get_drvdata(wdev);
>>> +
>>> + writeq(GTI_CWD_POKE_VAL, priv->base + GTI_CWD_POKE);
>>> + return 0;
>>> +}
>>> +
>>> +static int octeontx2_wdt_start(struct watchdog_device *wdev) {
>>> + struct octeontx2_wdt_priv *priv = watchdog_get_drvdata(wdev);
>>> + u64 regval;
>>> +
>>> + set_bit(WDOG_HW_RUNNING, &wdev->status);
>>> +
>>> + /* Clear any pending interrupt */
>>> + writeq(GTI_CWD_INT_PENDING_STATUS, priv->base + GTI_CWD_INT);
>>> +
>>> + /* Enable Interrupt */
>>> + writeq(GTI_CWD_INT_ENA_SET_VAL, priv->base +
>> GTI_CWD_INT_ENA_SET);
>>> +
>>> + /* Set (Interrupt + SCP interrupt (DEL3T) + core domain reset) Mode */
>>> + regval = readq(priv->base + GTI_CWD_WDOG);
>>> + regval |= GTI_CWD_WDOG_MODE_INT_DEL3T_RST;
>>> + writeq(regval, priv->base + GTI_CWD_WDOG);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int octeontx2_wdt_stop(struct watchdog_device *wdev) {
>>> + struct octeontx2_wdt_priv *priv = watchdog_get_drvdata(wdev);
>>> + u64 regval;
>>> +
>>> + /* Disable Interrupt */
>>> + writeq(GTI_CWD_INT_ENA_CLR_VAL, priv->base +
>> GTI_CWD_INT_ENA_CLR);
>>> +
>>> + /* Set GTI_CWD_WDOG.Mode = 0 to stop the timer */
>>> + regval = readq(priv->base + GTI_CWD_WDOG);
>>> + regval &= ~GTI_CWD_WDOG_MODE_MASK;
>>> + writeq(regval, priv->base + GTI_CWD_WDOG);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int octeontx2_wdt_settimeout(struct watchdog_device *wdev,
>>> + unsigned int timeout)
>>> +{
>>> + struct octeontx2_wdt_priv *priv = watchdog_get_drvdata(wdev);
>>> + u64 timeout_wdog, regval;
>>> +
>>> + /* Update new timeout */
>>> + wdev->timeout = timeout;
>>> +
>>> + /* Get clock cycles from timeout in second */
>>> + timeout_wdog = (u64)timeout * priv->clock_freq;
>>> +
>>> + /* Watchdog counts in 1024 cycle steps */
>>> + timeout_wdog = timeout_wdog >> 10;
>>> +
>>> + /*
>>> + * Hardware allows programming of upper 16-bits of 24-bits cycles
>>> + * Round up and use upper 16-bits only.
>>> + * Set max if timeout more than h/w supported
>>
>> This should be covered when setting max_timeout or max_hw_heartbeat_ms.
>> Setting the actual timeout to a value smaller than configured may result in the
>> watchdog firing before its configured timeout expires.
>
> wdog_dev->max_timeout = 16 is set based on above description, will move this comment there.
>
> Just to explain why I added above comment here and below check, that was to ensure that if user provides timeout more than " wdog_dev->max_timeout" then program wdog_dev->max_timeout only.
> But now looking at the code "set_timeout()" will be called only if timeout is valid ( timeout is <= wdog_dev->max_timeout)
>
> So above comment is not valid, while below check can also be removed.
>
>>
>>> + */
>>> + timeout_wdog = (timeout_wdog + 0xff) >> 8;
>>> + if (timeout_wdog >= 0x10000)
>>> + timeout_wdog = 0xffff;
>>> +
>>> + /*
>>> + * GTI_CWD_WDOG.LEN have only upper 16-bits of 24-bits
>>> + * GTI_CWD_WDOG.CNT, need addition shift of 8.
>>> + */
>>> + regval = readq(priv->base + GTI_CWD_WDOG);
>>> + regval &= GTI_CWD_WDOG_MODE_MASK;
>>> + regval |= ((timeout_wdog) << (GTI_CWD_WDOG_CNT_SHIFT + 8)) |
>>> + (timeout_wdog << GTI_CWD_WDOG_LEN_SHIFT);
>>
>> () around timeout is unnecessary. Why does the timeout need to be programmed
>> twice into the register ? The shift values are odd.
>> Are you sure this does what you expect it to do ?
>
> This register have two timeouts:
> GTI_CWD_WDOG.CNT[43:20] (24bit) = this is something decrementing with a frequency.
> GTI_CWD_WDOG.LEN[19:4] (16bit) = this is something loaded in upper 16 bits of GTI_CWD_WDOG.CNT when poke happens.
>
> Shift looks odd but it works as expected.
>
>>
>>> + writeq(regval, priv->base + GTI_CWD_WDOG);
>>> + return 0;
>>> +}
>>> +
>>> +static const struct watchdog_info octeontx2_wdt_ident = {
>>> + .identity = "OcteonTX2 watchdog",
>>> + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
>> WDIOF_MAGICCLOSE |
>>> + WDIOF_CARDRESET,
>>> +};
>>> +
>>> +static const struct watchdog_ops octeontx2_wdt_ops = {
>>> + .owner = THIS_MODULE,
>>> + .start = octeontx2_wdt_start,
>>> + .stop = octeontx2_wdt_stop,
>>> + .ping = octeontx2_wdt_ping,
>>> + .set_timeout = octeontx2_wdt_settimeout, };
>>> +
>>> +static int octeontx2_wdt_probe(struct platform_device *pdev) {
>>> + struct octeontx2_wdt_priv *priv;
>>> + struct device *dev = &pdev->dev;
>>> + struct watchdog_device *wdog_dev;
>>> + int irq;
>>> + int err;
>>> +
>>> + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
>>> + if (!priv)
>>> + return -ENOMEM;
>>> +
>>> + priv->base = devm_platform_ioremap_resource(pdev, 0);
>>> + if (IS_ERR(priv->base))
>>> + return dev_err_probe(&pdev->dev, PTR_ERR(priv->base),
>>> + "reg property not valid/found\n");
>>> +
>>> + priv->clock_freq = arch_timer_get_cntfrq();
>>> +
>>> + wdog_dev = &priv->wdev;
>>> + wdog_dev->info = &octeontx2_wdt_ident,
>>> + wdog_dev->ops = &octeontx2_wdt_ops,
>>> + wdog_dev->parent = dev;
>>> + wdog_dev->min_timeout = 1;
>>> + wdog_dev->max_timeout = 16;
>>
>> Setting max_timeout makes max_hw_heartbeat_ms useless. Use only
>> max_hw_heartbeat_ms to enable larger soft timeouts, or use only max_timeout
>> to set the hard limit, but not both.
>
> Okay, thanks for details
>
>>
>>> + wdog_dev->max_hw_heartbeat_ms = 16000;
>>> + wdog_dev->timeout = 8;
>>> +
>>> + irq = platform_get_irq(pdev, 0);
>>> + if (irq < 0) {
>>> + dev_err(&pdev->dev, "IRQ resource not found\n");
>>> + return -ENODEV;
>>> + }
>>> +
>>> + err = request_irq(irq, octeontx2_wdt_interrupt, 0, pdev->name, priv);
>>> + if (err) {
>>> + dev_err(dev, "cannot register interrupt handler %d\n", err);
>>> + return err;
>>> + }
>>
>> Use devm_request_irq() and request the interrupt after registering the watchdog.
>
> Okay,
>
>>
>>> +
>>> + priv->irq = irq;
>>> + watchdog_set_drvdata(wdog_dev, priv);
>>> + platform_set_drvdata(pdev, priv);
>>> + watchdog_init_timeout(wdog_dev, wdog_dev->timeout, dev);
>>
>> watchdog_init_timeout sets wdog_dev->timeout, so this is pointless.
>> Calling watchdog_init_timeout() only makes sense if the parameter is either a
>> timeout module parameter or 0 and the idea is to use a value from devicetree if
>> configured.
>
> Okay, thanks again for detail
>
> Regards
> -Bharat
>
>>
>>> + octeontx2_wdt_settimeout(wdog_dev, wdog_dev->timeout);
>>> + watchdog_stop_on_reboot(wdog_dev);
>>> + watchdog_stop_on_unregister(wdog_dev);
>>> +
>>> + err = devm_watchdog_register_device(dev, wdog_dev);
>>> + if (err) {
>>> + free_irq(irq, priv);
>>> + return err;
>>> + }
>>> +
>>> + 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)
>>> + free_irq(priv->irq, priv);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static const struct of_device_id octeontx2_wdt_of_match[] = {
>>> + { .compatible = "marvell-octeontx2-wdt", },
>>> + { },
>>> +};
>>> +MODULE_DEVICE_TABLE(of, octeontx2_wdt_of_match);
>>> +
>>> +static struct platform_driver octeontx2_wdt_driver = {
>>> + .driver = {
>>> + .name = "octeontx2-wdt",
>>> + .of_match_table = octeontx2_wdt_of_match,
>>> + },
>>> + .probe = octeontx2_wdt_probe,
>>> + .remove = octeontx2_wdt_remove,
>>> +};
>>> +module_platform_driver(octeontx2_wdt_driver);
>>> +
>>> +MODULE_AUTHOR("Bharat Bhushan <bbhushan2@marvell.com>");
>>> +MODULE_DESCRIPTION("OcteonTX2 watchdog driver");
>>> --
>>> 2.17.1
>>>

\
 
 \ /
  Last update: 2023-04-17 16:11    [W:0.052 / U:0.280 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site