mbox series

[v2,0/2] Add drivers for Intel Keem Bay SoC watchdog

Message ID cover.1605028524.git.vijayakannan.ayyathurai@intel.com
Headers show
Series Add drivers for Intel Keem Bay SoC watchdog | expand

Message

Vijayakannan Ayyathurai Nov. 10, 2020, 5:53 p.m. UTC 
From: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com>

Hi,

This patch set adds the watchdog timer driver support for Intel Keem Bay Soc.

Patch 1 holds the driver and Patch 2 holds the Device Tree
binding documentation.

This driver was tested on the Keem Bay evaluation module board.

Thank you,
Vijay

Changes since v1:
 - Fix indentation error in the dt-bindings file.
 - Use true/false in the second arg of keembay_wdt_set_timeout_reg().
 - Fix the watchdog start sequence.
 - Avoid reduntant timeout register setting.
 - Remove min usage to find actual time at keembay_wdt_set_timeout().
 - Remove timeout configuration boundary check at
   keembay_wdt_set_pretimeout().
 - Use devm_watchdog_register_device() for wdt registration, which
   eventually supports driver unload functionality as well.

Vijayakannan Ayyathurai (2):
  watchdog: Add watchdog driver for Intel Keembay Soc
  dt-bindings: watchdog: Add bindings for Intel Keem Bay SoC

 .../bindings/watchdog/intel,keembay-wdt.yaml  |  57 ++++
 drivers/watchdog/Kconfig                      |  13 +
 drivers/watchdog/Makefile                     |   1 +
 drivers/watchdog/keembay_wdt.c                | 288 ++++++++++++++++++
 4 files changed, 359 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/intel,keembay-wdt.yaml
 create mode 100644 drivers/watchdog/keembay_wdt.c


base-commit: 3650b228f83adda7e5ee532e2b90429c03f7b9ec
prerequisite-patch-id: 822987dcf4c969ef6ac70359b088af06ba39042b
prerequisite-patch-id: 0a348762b660d0d817b8e70cc71647e83173c78c
prerequisite-patch-id: 54c661a006c7362053cb7602448d6c77419d5cf9
prerequisite-patch-id: d140d8534fb828778e0652fe5fcf6282e027f985

Comments

Vijayakannan Ayyathurai Nov. 23, 2020, 4:47 a.m. UTC | #1 
Hi,

> From: Ayyathurai, Vijayakannan <vijayakannan.ayyathurai@intel.com>

> Sent: Wednesday, 11 November, 2020 1:53 AM

> Subject: [PATCH v2 0/2] Add drivers for Intel Keem Bay SoC watchdog

> 

> From: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com>

> 

> Hi,

> 

> This patch set adds the watchdog timer driver support for Intel Keem Bay Soc.

> 

> Patch 1 holds the driver and Patch 2 holds the Device Tree

> binding documentation.

> 

> This driver was tested on the Keem Bay evaluation module board.

> 

> Thank you,

> Vijay

> 

> Changes since v1:

>  - Fix indentation error in the dt-bindings file.

>  - Use true/false in the second arg of keembay_wdt_set_timeout_reg().

>  - Fix the watchdog start sequence.

>  - Avoid reduntant timeout register setting.

>  - Remove min usage to find actual time at keembay_wdt_set_timeout().

>  - Remove timeout configuration boundary check at

>    keembay_wdt_set_pretimeout().

>  - Use devm_watchdog_register_device() for wdt registration, which

>    eventually supports driver unload functionality as well.

> 


Please consider review this patch and let me know 
if there is further feedback.

> Vijayakannan Ayyathurai (2):

>   watchdog: Add watchdog driver for Intel Keembay Soc

>   dt-bindings: watchdog: Add bindings for Intel Keem Bay SoC

> 

>  .../bindings/watchdog/intel,keembay-wdt.yaml  |  57 ++++

>  drivers/watchdog/Kconfig                      |  13 +

>  drivers/watchdog/Makefile                     |   1 +

>  drivers/watchdog/keembay_wdt.c                | 288 ++++++++++++++++++

>  4 files changed, 359 insertions(+)

>  create mode 100644

> Documentation/devicetree/bindings/watchdog/intel,keembay-wdt.yaml

>  create mode 100644 drivers/watchdog/keembay_wdt.c

> 

> 

> base-commit: 3650b228f83adda7e5ee532e2b90429c03f7b9ec

> --

> 2.17.1


Thanks,
Vijay
Guenter Roeck Nov. 30, 2020, 10:05 p.m. UTC | #2 
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
Vijayakannan Ayyathurai Dec. 1, 2020, 6:19 a.m. UTC | #3 
Hi Guenter,

> From: Guenter Roeck <linux@roeck-us.net>

> 

> 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.

> >

> > +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 ?

> 


Sure. I can follow the below way.
Let me know if there is further comments to improve for the next version.

> 	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


Thanks,
Vijay
Guenter Roeck Dec. 1, 2020, 12:22 p.m. UTC | #4 
On Tue, Dec 01, 2020 at 06:19:09AM +0000, Ayyathurai, Vijayakannan wrote:
> Hi Guenter,

> 

> > From: Guenter Roeck <linux@roeck-us.net>

> > 

> > 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.

> > >

> > > +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 ?

> > 

> 

> Sure. I can follow the below way.

> Let me know if there is further comments to improve for the next version.

> 

No, that was all. Driver looks good otherwise.

Guenter

> > 	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

> 

> Thanks,

> Vijay