From: Guenter Roeck <linux@roeck-us.net>
To: vijayakannan.ayyathurai@intel.com
Cc: wim@linux-watchdog.org, robh+dt@kernel.org,
linux-watchdog@vger.kernel.org, devicetree@vger.kernel.org,
andriy.shevchenko@linux.intel.com, mgross@linux.intel.com,
wan.ahmad.zainie.wan.mohamad@intel.com,
lakshmi.bai.raja.subramanian@intel.com
Subject: Re: [PATCH v2 1/2] watchdog: Add watchdog driver for Intel Keembay Soc
Date: Mon, 30 Nov 2020 14:05:38 -0800 [thread overview]
Message-ID: <20201130220538.GA42581@roeck-us.net> (raw)
In-Reply-To: <870c2fda29b290ee6b9f88b15bd1f173bfad8723.1605028524.git.vijayakannan.ayyathurai@intel.com>
On Wed, Nov 11, 2020 at 01:53:07AM +0800, vijayakannan.ayyathurai@intel.com wrote:
> From: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com>
>
> Intel Keembay Soc requires watchdog timer support.
> Add watchdog driver to enable this.
>
> Signed-off-by: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com>
> Acked-by: Mark Gross <mgross@linux.intel.com>
> Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> drivers/watchdog/Kconfig | 13 ++
> drivers/watchdog/Makefile | 1 +
> drivers/watchdog/keembay_wdt.c | 288 +++++++++++++++++++++++++++++++++
> 3 files changed, 302 insertions(+)
> create mode 100644 drivers/watchdog/keembay_wdt.c
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index fd7968635e6d..f412cf2d0f1a 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -2163,4 +2163,17 @@ config USBPCWATCHDOG
>
> Most people will say N.
>
> +config KEEMBAY_WATCHDOG
> + tristate "Intel Keem Bay SoC non-secure watchdog"
> + depends on ARCH_KEEMBAY || (ARM64 && COMPILE_TEST)
> + select WATCHDOG_CORE
> + help
> + This option enable support for an In-secure watchdog timer driver for
> + Intel Keem Bay SoC. This WDT has a 32 bit timer and decrements in every
> + count unit. An interrupt will be triggered, when the count crosses
> + the thershold configured in the register.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called keembay_wdt.
> +
> endif # WATCHDOG
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 071a2e50be98..f6f9f434f407 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -146,6 +146,7 @@ obj-$(CONFIG_INTEL_MEI_WDT) += mei_wdt.o
> obj-$(CONFIG_NI903X_WDT) += ni903x_wdt.o
> obj-$(CONFIG_NIC7018_WDT) += nic7018_wdt.o
> obj-$(CONFIG_MLX_WDT) += mlx_wdt.o
> +obj-$(CONFIG_KEEMBAY_WATCHDOG) += keembay_wdt.o
>
> # M68K Architecture
> obj-$(CONFIG_M54xx_WATCHDOG) += m54xx_wdt.o
> diff --git a/drivers/watchdog/keembay_wdt.c b/drivers/watchdog/keembay_wdt.c
> new file mode 100644
> index 000000000000..1d08c7f0f16c
> --- /dev/null
> +++ b/drivers/watchdog/keembay_wdt.c
> @@ -0,0 +1,288 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Watchdog driver for Intel Keem Bay non-secure watchdog.
> + *
> + * Copyright (C) 2020 Intel Corporation
> + */
> +
> +#include <linux/arm-smccc.h>
> +#include <linux/bits.h>
> +#include <linux/clk.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/limits.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/platform_device.h>
> +#include <linux/reboot.h>
> +#include <linux/watchdog.h>
> +
> +/* Non-secure watchdog register offsets */
> +#define TIM_WATCHDOG 0x0
> +#define TIM_WATCHDOG_INT_THRES 0x4
> +#define TIM_WDOG_EN 0x8
> +#define TIM_SAFE 0xc
> +
> +#define WDT_ISR_MASK GENMASK(9, 8)
> +#define WDT_ISR_CLEAR 0x8200ff18
> +#define WDT_UNLOCK 0xf1d0dead
> +#define WDT_LOAD_MAX U32_MAX
> +#define WDT_LOAD_MIN 1
> +#define WDT_TIMEOUT 5
> +
> +static unsigned int timeout = WDT_TIMEOUT;
> +module_param(timeout, int, 0);
> +MODULE_PARM_DESC(timeout, "Watchdog timeout period in seconds (default = "
> + __MODULE_STRING(WDT_TIMEOUT) ")");
> +
> +static bool nowayout = WATCHDOG_NOWAYOUT;
> +module_param(nowayout, bool, 0);
> +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default = "
> + __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> +
> +struct keembay_wdt {
> + struct watchdog_device wdd;
> + struct clk *clk;
> + unsigned int rate;
> + int to_irq;
> + int th_irq;
> + void __iomem *base;
> +};
> +
> +static inline u32 keembay_wdt_readl(struct keembay_wdt *wdt, u32 offset)
> +{
> + return readl(wdt->base + offset);
> +}
> +
> +static inline void keembay_wdt_writel(struct keembay_wdt *wdt,
> + u32 offset, u32 val)
> +{
> + writel(WDT_UNLOCK, wdt->base + TIM_SAFE);
> + writel(val, wdt->base + offset);
> +}
> +
> +static void keembay_wdt_set_timeout_reg(struct watchdog_device *wdog, bool ping)
> +{
> + struct keembay_wdt *wdt = watchdog_get_drvdata(wdog);
> + u32 th_val = 0;
> +
> + if (ping)
> + keembay_wdt_writel(wdt, TIM_WATCHDOG, wdog->timeout * wdt->rate);
> +
> + if (!ping && wdog->pretimeout) {
> + th_val = wdog->timeout - wdog->pretimeout;
> + keembay_wdt_writel(wdt, TIM_WATCHDOG_INT_THRES, th_val * wdt->rate);
> + }
> +
> + if (!ping)
> + keembay_wdt_writel(wdt, TIM_WATCHDOG, wdog->timeout * wdt->rate);
I am a bit at loss here. This seems unnecessarily complex. Why not just the following ?
if (!ping && wdog->pretimeout) {
th_val = wdog->timeout - wdog->pretimeout;
keembay_wdt_writel(wdt, TIM_WATCHDOG_INT_THRES, th_val * wdt->rate);
}
keembay_wdt_writel(wdt, TIM_WATCHDOG, wdog->timeout * wdt->rate);
Thanks,
Guenter
next prev parent reply other threads:[~2020-11-30 22:06 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-10 17:53 [PATCH v2 0/2] Add drivers for Intel Keem Bay SoC watchdog vijayakannan.ayyathurai
2020-11-10 17:53 ` [PATCH v2 1/2] watchdog: Add watchdog driver for Intel Keembay Soc vijayakannan.ayyathurai
2020-11-30 22:05 ` Guenter Roeck [this message]
2020-12-01 6:19 ` Ayyathurai, Vijayakannan
2020-12-01 12:22 ` Guenter Roeck
2020-11-10 17:53 ` [PATCH v2 2/2] dt-bindings: watchdog: Add bindings for Intel Keem Bay SoC vijayakannan.ayyathurai
2020-11-11 20:03 ` Rob Herring
2020-11-12 3:10 ` Ayyathurai, Vijayakannan
2020-11-11 2:08 ` [PATCH v2 0/2] Add drivers for Intel Keem Bay SoC watchdog mark gross
2020-11-23 4:47 ` Ayyathurai, Vijayakannan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201130220538.GA42581@roeck-us.net \
--to=linux@roeck-us.net \
--cc=andriy.shevchenko@linux.intel.com \
--cc=devicetree@vger.kernel.org \
--cc=lakshmi.bai.raja.subramanian@intel.com \
--cc=linux-watchdog@vger.kernel.org \
--cc=mgross@linux.intel.com \
--cc=robh+dt@kernel.org \
--cc=vijayakannan.ayyathurai@intel.com \
--cc=wan.ahmad.zainie.wan.mohamad@intel.com \
--cc=wim@linux-watchdog.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).