[v3,5/6] Watchdog: introduce ARM SBSA watchdog driver

From: Fu Wei <fu.wei@linaro.org>

This driver bases on linux kernel watchdog framework, and
use "pretimeout" in the framework. It supports getting timeout and
pretimeout from parameter and FDT at the driver init stage.
In first timeout, the interrupt routine run panic to save
system context.

Acked-by: Arnd Bergmann <arnd@arndb.de>
Tested-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
Signed-off-by: Fu Wei <fu.wei@linaro.org>
---
 drivers/watchdog/Kconfig     |  11 +
 drivers/watchdog/Makefile    |   1 +
 drivers/watchdog/sbsa_gwdt.c | 474 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 486 insertions(+)
 create mode 100644 drivers/watchdog/sbsa_gwdt.c

Hi Guenter,
Great thanks,  feedback inline

On 26 May 2015 at 03:39, Guenter Roeck <linux@roeck-us.net> wrote:
> On 05/25/2015 03:03 AM, fu.wei@linaro.org wrote:
>>
>> From: Fu Wei <fu.wei@linaro.org>
>>
>> This driver bases on linux kernel watchdog framework, and
>> use "pretimeout" in the framework. It supports getting timeout and
>> pretimeout from parameter and FDT at the driver init stage.
>> In first timeout, the interrupt routine run panic to save
>> system context.
>>
>> Acked-by: Arnd Bergmann <arnd@arndb.de>
>> Tested-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
>> Signed-off-by: Fu Wei <fu.wei@linaro.org>
>> ---
>
>
> Comments inline.
>
> Thanks,
> Guenter
>
>
>>   drivers/watchdog/Kconfig     |  11 +
>>   drivers/watchdog/Makefile    |   1 +
>>   drivers/watchdog/sbsa_gwdt.c | 474
>> +++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 486 insertions(+)
>>   create mode 100644 drivers/watchdog/sbsa_gwdt.c
>>
>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>> index e5e7c55..554f18a 100644
>> --- a/drivers/watchdog/Kconfig
>> +++ b/drivers/watchdog/Kconfig
>> @@ -152,6 +152,17 @@ config ARM_SP805_WATCHDOG
>>           ARM Primecell SP805 Watchdog timer. This will reboot your system
>> when
>>           the timeout is reached.
>>
>> +config ARM_SBSA_WATCHDOG
>> +       tristate "ARM SBSA Generic Watchdog"
>> +       depends on ARM64
>> +       depends on ARM_ARCH_TIMER
>> +       select WATCHDOG_CORE
>> +       help
>> +         ARM SBSA Generic Watchdog. This watchdog has two Watchdog
>> timeouts.
>> +         The first timeout will trigger a panic; the second timeout will
>> +         trigger a system reset.
>> +         More details: ARM DEN0029B - Server Base System Architecture
>> (SBSA)
>> +
>>   config AT91RM9200_WATCHDOG
>>         tristate "AT91RM9200 watchdog"
>>         depends on SOC_AT91RM9200 && MFD_SYSCON
>> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
>> index 5c19294..471f1b7c 100644
>> --- a/drivers/watchdog/Makefile
>> +++ b/drivers/watchdog/Makefile
>> @@ -30,6 +30,7 @@ obj-$(CONFIG_USBPCWATCHDOG) += pcwd_usb.o
>>
>>   # ARM Architecture
>>   obj-$(CONFIG_ARM_SP805_WATCHDOG) += sp805_wdt.o
>> +obj-$(CONFIG_ARM_SBSA_WATCHDOG) += sbsa_gwdt.o
>>   obj-$(CONFIG_AT91RM9200_WATCHDOG) += at91rm9200_wdt.o
>>   obj-$(CONFIG_AT91SAM9X_WATCHDOG) += at91sam9_wdt.o
>>   obj-$(CONFIG_CADENCE_WATCHDOG) += cadence_wdt.o
>> diff --git a/drivers/watchdog/sbsa_gwdt.c b/drivers/watchdog/sbsa_gwdt.c
>> new file mode 100644
>> index 0000000..0d1aff1
>> --- /dev/null
>> +++ b/drivers/watchdog/sbsa_gwdt.c
>> @@ -0,0 +1,474 @@
>> +/*
>> + * SBSA(Server Base System Architecture) Generic Watchdog driver
>> + *
>> + * Copyright (c) 2015, Linaro Ltd.
>> + * Author: Fu Wei <fu.wei@linaro.org>
>> + *         Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License 2 as published
>> + * by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * Note: This SBSA Generic watchdog driver is compatible with
>> + *       the pretimeout concept of Linux kernel.
>> + *       The timeout and pretimeout are set by the different REGs.
>> + *       The first watch period is set by writing WCV directly,
>> + *       that can support more than 10s timeout at the maximum
>> + *       system counter frequency.
>> + *       The second watch period is set by WOR(32bit) which will be
>> loaded
>> + *       automatically by hardware, when WS0 is triggered.
>> + *       This gives a maximum watch period of around 10s at the maximum
>> + *       system counter frequency.
>> + *       The System Counter shall run at maximum of 400MHz.
>> + *       More details: ARM DEN0029B - Server Base System Architecture
>> (SBSA)
>> + *
>> + * Kernel/API:                         P---------| pretimeout
>> + *               |-------------------------------T timeout
>> + * SBSA GWDT:                          P--WOR---WS1 pretimeout
>> + *               |-------WCV----------WS0~~~~~~~~T timeout
>> + */
>> +
>> +#include <linux/io.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/module.h>
>> +#include <linux/moduleparam.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/uaccess.h>
>> +#include <linux/watchdog.h>
>> +#include <asm/arch_timer.h>
>> +
>> +/* SBSA Generic Watchdog register definitions */
>> +/* refresh frame */
>> +#define SBSA_GWDT_WRR                          0x000
>> +
>> +/* control frame */
>> +#define SBSA_GWDT_WCS                          0x000
>> +#define SBSA_GWDT_WOR                          0x008
>> +#define SBSA_GWDT_WCV_LO                       0x010
>> +#define SBSA_GWDT_WCV_HI                       0x014
>> +
>> +/* refresh/control frame */
>> +#define SBSA_GWDT_W_IIDR                       0xfcc
>> +#define SBSA_GWDT_IDR                          0xfd0
>> +
>> +/* Watchdog Control and Status Register */
>> +#define SBSA_GWDT_WCS_EN                       BIT(0)
>> +#define SBSA_GWDT_WCS_WS0                      BIT(1)
>> +#define SBSA_GWDT_WCS_WS1                      BIT(2)
>> +
>> +/**
>> + * struct sbsa_gwdt - Internal representation of the SBSA GWDT
>> + * @wdd:               kernel watchdog_device structure
>> + * @clk:               store the System Counter clock frequency, in Hz.
>> + * @refresh_base:      Virtual address of the watchdog refresh frame
>> + * @control_base:      Virtual address of the watchdog control frame
>> + */
>> +struct sbsa_gwdt {
>> +       struct watchdog_device  wdd;
>> +       u32                     clk;
>> +       void __iomem            *refresh_base;
>> +       void __iomem            *control_base;
>> +};
>> +
>> +#define to_sbsa_gwdt(e) container_of(e, struct sbsa_gwdt, wdd)
>> +
>> +#define DEFAULT_TIMEOUT                10 /* seconds, the 1st + 2nd watch
>> periods*/
>
>
> That is a bit low for a default. Is that on purpose ?
> Most drivers use 30 or 60 seconds.

That is not on purpose. will fixed them :
#define DEFAULT_TIMEOUT 30 /* seconds, the 1st + 2nd watch periods*/
#define DEFAULT_PRETIMEOUT 10 /* seconds, the 2nd watch period*/

>
>> +#define DEFAULT_PRETIMEOUT     5  /* seconds, the 2nd watch period*/
>> +
>> +static unsigned int timeout_param;
>> +module_param(timeout_param, uint, 0);
>> +MODULE_PARM_DESC(timeout_param,
>> +                "Watchdog timeout in seconds. (>=0, default="
>> +                __MODULE_STRING(DEFAULT_TIMEOUT) ")");
>> +
>
> Why _param in the module parameter names ? Seems to be redundant.

sorry, I shouldn't add _param , fixed it

>
>> +static unsigned int max_timeout_param = UINT_MAX;
>> +module_param(max_timeout_param, uint, 0);
>> +MODULE_PARM_DESC(max_timeout_param,
>> +                "Watchdog max timeout in seconds. (>=0, default="
>> +                __MODULE_STRING(UINT_MAX) ")");
>> +
>
>
> Why do we want or need this parameter and max_pretimeout ? Those
> are determined by the hardware.

Sorry, I thought we can also allow user setup a max value when insmod
the module or by boot cmdline. but I was wrong, have deleted them.

>
>
>> +static unsigned int pretimeout_param;
>> +module_param(pretimeout_param, uint, 0);
>> +MODULE_PARM_DESC(pretimeout_param,
>> +                "Watchdog pretimeout in seconds. (>=0, default="
>> +                __MODULE_STRING(DEFAULT_PRETIMEOUT) ")");
>> +
>> +static unsigned int max_pretimeout_param = U32_MAX;
>> +module_param(max_pretimeout_param, uint, 0);
>> +MODULE_PARM_DESC(max_pretimeout_param,
>> +                "Watchdog max pretimeout in seconds. (>=0, default="
>> +                __MODULE_STRING(U32_MAX) ")");
>> +
>> +static bool nowayout = WATCHDOG_NOWAYOUT;
>> +module_param(nowayout, bool, S_IRUGO);
>> +MODULE_PARM_DESC(nowayout,
>> +                "Watchdog cannot be stopped once started (default="
>> +                __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
>> +
>> +/*
>> + * help functions for accessing 32bit registers of SBSA Generic Watchdog
>> + */
>> +static void sbsa_gwdt_cf_write(unsigned int reg, u32 val,
>> +                              struct watchdog_device *wdd)
>> +{
>> +       struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
>> +
>> +       writel_relaxed(val, gwdt->control_base + reg);
>> +}
>> +
>> +static void sbsa_gwdt_rf_write(unsigned int reg, u32 val,
>> +                              struct watchdog_device *wdd)
>> +{
>> +       struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
>> +
>> +       writel_relaxed(val, gwdt->refresh_base + reg);
>> +}
>> +
>> +static u32 sbsa_gwdt_cf_read(unsigned int reg, struct watchdog_device
>> *wdd)
>> +{
>> +       struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
>> +
>> +       return readl_relaxed(gwdt->control_base + reg);
>> +}
>> +
>> +/*
>> + * help functions for accessing 64bit WCV register
>> + */
>> +static u64 sbsa_gwdt_get_wcv(struct watchdog_device *wdd)
>> +{
>> +       u32 wcv_lo, wcv_hi;
>> +
>> +       do {
>> +               wcv_hi = sbsa_gwdt_cf_read(SBSA_GWDT_WCV_HI, wdd);
>> +               wcv_lo = sbsa_gwdt_cf_read(SBSA_GWDT_WCV_LO, wdd);
>> +       } while (wcv_hi != sbsa_gwdt_cf_read(SBSA_GWDT_WCV_HI, wdd));
>> +
>> +       return (((u64)wcv_hi << 32) | wcv_lo);
>> +}
>> +
>> +static void sbsa_gwdt_set_wcv(struct watchdog_device *wdd, u64 value)
>> +{
>> +       u32 wcv_lo, wcv_hi;
>> +
>> +       wcv_lo = value & U32_MAX;
>> +       wcv_hi = (value >> 32) & U32_MAX;
>> +
>> +       sbsa_gwdt_cf_write(SBSA_GWDT_WCV_HI, wcv_hi, wdd);
>> +       sbsa_gwdt_cf_write(SBSA_GWDT_WCV_LO, wcv_lo, wdd);
>> +}
>> +
>> +static void reload_timeout_to_wcv(struct watchdog_device *wdd)
>> +{
>> +       struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
>> +       u64 wcv;
>> +
>> +       wcv = arch_counter_get_cntvct() +
>> +               (u64)(wdd->timeout - wdd->pretimeout) * gwdt->clk;
>> +
>> +       sbsa_gwdt_set_wcv(wdd, wcv);
>> +}
>> +
>> +/*
>> + * Use the following function to update the timeout limits
>> + * after updating pretimeout
>> + */
>> +static void sbsa_gwdt_update_limits(struct watchdog_device *wdd)
>> +{
>> +       struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
>> +       u64 first_period_max = U64_MAX;
>> +
>> +       do_div(first_period_max, gwdt->clk);
>> +
>> +       wdd->min_timeout = wdd->pretimeout + 1;
>> +       wdd->max_timeout = min((uint)(wdd->pretimeout + first_period_max),
>> +                              wdd->max_timeout);
>> +}
>> +
>
>
> After understanding the watchdog a bit better now, I think you should drop
> those updates and set
>         min_timeout = 1
>         max_timeout = min(UINT_MAX, U64_MAX / clk)
>         min_pretimeout = 0
>         max_pretimeout = U32_MAX / clk
>
> and then ensure that pretimeout < timeout at runtime (if possible in
> the infrastructure code).
>
> Even at maximum clock rate, max_timeout is so large that anything else
> is really overkill.

you are right, fixed it, thanks

>
>> +static int sbsa_gwdt_set_timeout(struct watchdog_device *wdd,
>> +                                unsigned int timeout)
>> +{
>> +       wdd->timeout = timeout;
>> +
>> +       return 0;
>> +}
>> +
>> +static int sbsa_gwdt_set_pretimeout(struct watchdog_device *wdd,
>> +                                   unsigned int pretimeout)
>> +{
>> +       struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
>> +       u32 wor;
>> +
>> +       wdd->pretimeout = pretimeout;
>> +       sbsa_gwdt_update_limits(wdd);
>> +
>> +       if (!pretimeout)
>> +               /* gives sbsa_gwdt_start a chance to setup timeout */
>> +               wor = gwdt->clk;
>> +       else
>> +               wor = pretimeout * gwdt->clk;
>> +
>
>
> So in practice you always have a pretimeout of at least one second,
> correct ? That kind of violates the ABI, which says that the pretimeout
> should be disabled if set to 0. Is there anything we can do about that ?
>
> What exactly happens if WOR = 0 ? Doesn't that just mean that the second
> timeout will happen immediately, and isn't that what we would want if
> pretimeout = 0

Accroding to SBSA, in theory,
if WOR == 0, once we write WRR(refresh), we will get WS0 and WS1 immediately .
that means timeout == 0.

So I will add more comment here to explain this.

>
>> +       /* refresh the WOR, that will cause an explicit watchdog refresh
>> */
>
>
> Except that we use wcv which needs a manual refresh, so this is a bit
> misleading, isn't it ?

if we setup  wcv manually, we maybe don't need a manual refresh.
I will test it later.

>
>
>> +       sbsa_gwdt_cf_write(SBSA_GWDT_WOR, wor, wdd);
>> +
>> +       return 0;
>> +}
>> +
>> +static unsigned int sbsa_gwdt_get_timeleft(struct watchdog_device *wdd)
>> +{
>> +       struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
>> +       u64 timeleft = sbsa_gwdt_get_wcv(wdd) - arch_counter_get_cntvct();
>> +
>> +       do_div(timeleft, gwdt->clk);
>> +
>> +       return timeleft;
>> +}
>> +
>> +static int sbsa_gwdt_start(struct watchdog_device *wdd)
>> +{
>> +       /* Force refresh */
>> +       sbsa_gwdt_rf_write(SBSA_GWDT_WRR, 0xc0ffee, wdd);
>> +       /* writing WCS will cause an explicit watchdog refresh */
>> +       sbsa_gwdt_cf_write(SBSA_GWDT_WCS, SBSA_GWDT_WCS_EN, wdd);
>> +
>> +       reload_timeout_to_wcv(wdd);
>> +
>> +       return 0;
>> +}
>> +
>> +static int sbsa_gwdt_stop(struct watchdog_device *wdd)
>> +{
>> +       /* Force refresh */
>> +       sbsa_gwdt_rf_write(SBSA_GWDT_WRR, 0xc0ffee, wdd);
>> +       /* writing WCS will cause an explicit watchdog refresh */
>> +       sbsa_gwdt_cf_write(SBSA_GWDT_WCS, 0, wdd);
>> +
>> +       return 0;
>> +}
>> +
>> +static int sbsa_gwdt_keepalive(struct watchdog_device *wdd)
>> +{
>> +       /*
>> +        * Writing WRR for an explicit watchdog refresh
>> +        * You can write anyting(like 0xc0ffee)
>> +        */
>> +       sbsa_gwdt_rf_write(SBSA_GWDT_WRR, 0xc0ffee, wdd);
>
>
> Should that happen after writing wcv ?

no, if so, WOR + current_time will be loaded into wcv. I put this
before writing wcv on purpose.
maybe we can just update wcv without a force refresh.

will test it later


>
>
>> +
>> +       reload_timeout_to_wcv(wdd);
>> +
>> +       return 0;
>> +}
>> +
>> +static irqreturn_t sbsa_gwdt_interrupt(int irq, void *dev_id)
>> +{
>> +       struct sbsa_gwdt *gwdt = (struct sbsa_gwdt *)dev_id;
>> +       struct watchdog_device *wdd = &gwdt->wdd;
>> +       u32 status;
>> +
>> +       status = sbsa_gwdt_cf_read(SBSA_GWDT_WCS, wdd);
>> +
>> +       if (status & SBSA_GWDT_WCS_WS0)
>> +               panic("SBSA Watchdog pre-timeout");
>> +
>> +       return IRQ_HANDLED;
>> +}
>> +
>> +static struct watchdog_info sbsa_gwdt_info = {
>> +       .identity       = "SBSA Generic Watchdog",
>> +       .options        = WDIOF_SETTIMEOUT |
>> +                         WDIOF_KEEPALIVEPING |
>> +                         WDIOF_MAGICCLOSE |
>> +                         WDIOF_PRETIMEOUT |
>> +                         WDIOF_CARDRESET,
>> +};
>> +
>> +static struct watchdog_ops sbsa_gwdt_ops = {
>> +       .owner          = THIS_MODULE,
>> +       .start          = sbsa_gwdt_start,
>> +       .stop           = sbsa_gwdt_stop,
>> +       .ping           = sbsa_gwdt_keepalive,
>> +       .set_timeout    = sbsa_gwdt_set_timeout,
>> +       .set_pretimeout = sbsa_gwdt_set_pretimeout,
>> +       .get_timeleft   = sbsa_gwdt_get_timeleft,
>> +};
>> +
>> +static int sbsa_gwdt_probe(struct platform_device *pdev)
>> +{
>> +       struct device *dev = &pdev->dev;
>> +       struct sbsa_gwdt *gwdt;
>> +       struct watchdog_device *wdd;
>> +       struct resource *res;
>> +       void *rf_base, *cf_base;
>> +       int irq;
>> +       u32 clk, status;
>> +       int ret = 0;
>> +       u64 first_period_max = U64_MAX;
>> +
>> +       /*
>> +        * Get the frequency of system counter from
>> +        * the cp15 interface of ARM Generic timer
>> +        */
>> +       clk = arch_timer_get_cntfrq();
>
>
> That can not return 0, presumably, from looking into its implementation.
> And the system should panic if it is ever 0.
>
>> +       if (!clk) {
>
>
> Given that, I don't think this check is necessary.

yes, you are right , I have got your point, and improved it .

>
>
>> +               dev_err(dev, "System Counter frequency not available\n");
>> +               return -EINVAL;
>> +       }
>> +
>> +       gwdt = devm_kzalloc(dev, sizeof(*gwdt), GFP_KERNEL);
>> +       if (!gwdt)
>> +               return -ENOMEM;
>> +
>> +       res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>> "refresh");
>> +       rf_base = devm_ioremap_resource(dev, res);
>> +       if (IS_ERR(rf_base))
>> +               return PTR_ERR(rf_base);
>> +
>> +       res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>> "control");
>> +       cf_base = devm_ioremap_resource(dev, res);
>> +       if (IS_ERR(cf_base))
>> +               return PTR_ERR(cf_base);
>> +
>> +       irq = platform_get_irq_byname(pdev, "ws0");
>> +       if (irq < 0) {
>> +               dev_err(dev, "unable to get ws0 interrupt.\n");
>> +               return irq;
>> +       }
>> +
>> +       gwdt->refresh_base = rf_base;
>> +       gwdt->control_base = cf_base;
>> +       gwdt->clk = clk;
>> +
>> +       platform_set_drvdata(pdev, gwdt);
>> +
>> +       wdd = &gwdt->wdd;
>> +       wdd->parent = dev;
>> +       wdd->info = &sbsa_gwdt_info;
>> +       wdd->ops = &sbsa_gwdt_ops;
>> +       watchdog_set_drvdata(wdd, gwdt);
>> +       watchdog_set_nowayout(wdd, nowayout);
>> +
>> +       status = sbsa_gwdt_cf_read(SBSA_GWDT_WCS, wdd);
>> +       if (status & SBSA_GWDT_WCS_WS1) {
>> +               dev_warn(dev, "System reset by WDT(WCS: %x, WCV: %llx)\n",
>> +                        status, sbsa_gwdt_get_wcv(wdd));
>
>
> Does this message (specifically the WCS / WCV values) have any
> useful meaning for the user ?

I think so, according to SBSA spec:
If WS0 is asserted and a timeout refresh occurs then the following must occur:
    If the system is compliant to SBSA level 2 or higher the compare
value must retain its current value. This means that the compare value
records the time that WS1 is asserted.

So, I think WCV can log the time when system reset by WDT, that may
help user figure out the problem. but WCS can be delete I think.


>
>> +               wdd->bootstatus |= WDIOF_CARDRESET;
>> +       }
>> +       /* Check if watchdog is already enabled */
>> +       if (status & SBSA_GWDT_WCS_EN) {
>> +               dev_warn(dev, "already enabled! WCS:0x%x\n", status);
>> +               sbsa_gwdt_keepalive(wdd);
>
>
> You have not configured wdd->timeout and wdd->pretimeout here,
> yet the function calls reload_timeout_to_wcv which needs it.

Ah, sorry, that is a bug, have reordered the code, Thanks

>
>> +       }
>> +
>> +       wdd->min_pretimeout = 0;
>> +       wdd->max_pretimeout = min(U32_MAX / clk, max_pretimeout_param);
>> +       wdd->min_timeout = 1;
>> +       do_div(first_period_max, clk);
>> +       wdd->max_timeout = min((uint)(wdd->pretimeout + first_period_max),
>> +                              max_timeout_param);
>> +
>> +       wdd->pretimeout = DEFAULT_PRETIMEOUT;
>> +       wdd->timeout = DEFAULT_TIMEOUT;
>> +       watchdog_init_timeouts(wdd, pretimeout_param, timeout_param, dev);
>> +       /* update pretimeout to WOR */
>> +       sbsa_gwdt_cf_write(SBSA_GWDT_WOR, wdd->pretimeout * clk, wdd);
>
>
> This is interesting because you set WOR to 0 here if pretimeout is 0.
> Yet, when pretimeout is updated later on, you always set it to at least
> one second. Any reason for doing this differently ?
>
> If the above works, and presumably you have tested it, I don't see why
> it would be necessary to set WOR to a value larger than 0 when updating
> pretimeout.

Ah I should tested more in extreme case, but here, I can use
sbsa_gwdt_set_pretimeout directly.


>
>> +
>> +       ret = devm_request_irq(dev, irq, sbsa_gwdt_interrupt, IRQF_TIMER,
>> +                              pdev->name, gwdt);
>> +       if (ret) {
>> +               dev_err(dev, "unable to request IRQ %d\n", irq);
>> +               return ret;
>> +       }
>> +
>> +       ret = watchdog_register_device(wdd);
>> +       if (ret)
>> +               return ret;
>> +
>> +       dev_info(dev, "Initialized with %ds timeout, %ds pretimeout @
>> %uHz\n",
>> +                wdd->timeout, wdd->pretimeout, gwdt->clk);
>
>
> 400000Hz reads a bit odd. How about a space between the number and Hz ?

np, fixed it , thanks :-)

>
>> +
>> +       return 0;
>> +}
>> +
>> +static void sbsa_gwdt_shutdown(struct platform_device *pdev)
>> +{
>> +       struct sbsa_gwdt *gwdt = platform_get_drvdata(pdev);
>> +
>> +       sbsa_gwdt_stop(&gwdt->wdd);
>> +}
>> +
>> +static int sbsa_gwdt_remove(struct platform_device *pdev)
>> +{
>> +       struct sbsa_gwdt *gwdt = platform_get_drvdata(pdev);
>> +       int ret = 0;
>> +
>> +       if (!nowayout)
>> +               ret = sbsa_gwdt_stop(&gwdt->wdd);
>> +
>
> I don't think you should do anything here. The driver can only be removed
> if closed, and it that case the watchdog will already have been stopped,
> or if nowayout was set it won't be stopped. Either case it is already in
> the state we want it to be in.

Sorry, I should delete it. Fixed

>
>
>> +       watchdog_unregister_device(&gwdt->wdd);
>> +
>> +       return ret;
>> +}
>> +
>> +/* Disable watchdog if it is active during suspend */
>> +static int __maybe_unused sbsa_gwdt_suspend(struct device *dev)
>> +{
>> +       struct sbsa_gwdt *gwdt = dev_get_drvdata(dev);
>> +
>> +       if (watchdog_active(&gwdt->wdd))
>> +               sbsa_gwdt_stop(&gwdt->wdd);
>> +
>> +       return 0;
>> +}
>> +
>> +/* Enable watchdog and configure it if necessary */
>> +static int __maybe_unused sbsa_gwdt_resume(struct device *dev)
>> +{
>> +       struct sbsa_gwdt *gwdt = dev_get_drvdata(dev);
>> +
>> +       if (watchdog_active(&gwdt->wdd))
>> +               sbsa_gwdt_start(&gwdt->wdd);
>> +
>> +       return 0;
>> +}
>> +
>> +static const struct dev_pm_ops sbsa_gwdt_pm_ops = {
>> +       SET_SYSTEM_SLEEP_PM_OPS(sbsa_gwdt_suspend, sbsa_gwdt_resume)
>> +};
>> +
>> +static const struct of_device_id sbsa_gwdt_of_match[] = {
>> +       { .compatible = "arm,sbsa-gwdt", },
>> +       {},
>> +};
>> +MODULE_DEVICE_TABLE(of, sbsa_gwdt_of_match);
>> +
>> +static const struct platform_device_id sbsa_gwdt_pdev_match[] = {
>> +       { .name = "sbsa-gwdt", },
>> +       {},
>> +};
>> +MODULE_DEVICE_TABLE(platform, sbsa_gwdt_pdev_match);
>> +
>> +static struct platform_driver sbsa_gwdt_driver = {
>> +       .driver = {
>> +               .name = "sbsa-gwdt",
>> +               .pm = &sbsa_gwdt_pm_ops,
>> +               .of_match_table = sbsa_gwdt_of_match,
>> +       },
>> +       .probe = sbsa_gwdt_probe,
>> +       .remove = sbsa_gwdt_remove,
>> +       .shutdown = sbsa_gwdt_shutdown,
>> +       .id_table = sbsa_gwdt_pdev_match,
>> +};
>> +
>> +module_platform_driver(sbsa_gwdt_driver);
>> +
>> +MODULE_DESCRIPTION("SBSA Generic Watchdog Driver");
>> +MODULE_VERSION("v1.0");
>> +MODULE_AUTHOR("Fu Wei <fu.wei@linaro.org>");
>> +MODULE_AUTHOR("Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>");
>> +MODULE_LICENSE("GPL v2");
>>
>

Hi Timur,

On 27 May 2015 at 00:50, Timur Tabi <timur@codeaurora.org> wrote:
> On 05/25/2015 05:03 AM, fu.wei@linaro.org wrote:
>
>> +/*
>> + * help functions for accessing 32bit registers of SBSA Generic Watchdog
>> + */
>> +static void sbsa_gwdt_cf_write(unsigned int reg, u32 val,
>> +                              struct watchdog_device *wdd)
>> +{
>> +       struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
>> +
>> +       writel_relaxed(val, gwdt->control_base + reg);
>> +}
>> +
>> +static void sbsa_gwdt_rf_write(unsigned int reg, u32 val,
>> +                              struct watchdog_device *wdd)
>> +{
>> +       struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
>> +
>> +       writel_relaxed(val, gwdt->refresh_base + reg);
>> +}
>> +
>> +static u32 sbsa_gwdt_cf_read(unsigned int reg, struct watchdog_device
>> *wdd)
>> +{
>> +       struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
>> +
>> +       return readl_relaxed(gwdt->control_base + reg);
>> +}
>
>
> I don't understand the value of these functions.  You're just adding
> overhead to each read and write by dereferencing wdd every time.  I would
> get rid of them and just call readl_relaxed() and writel_relaxed() directly.

yes, that makes sense, sometimes , I also feel these functions are a
little redundant,
let me see if I can improve it.

>
>> +/*
>> + * help functions for accessing 64bit WCV register
>> + */
>> +static u64 sbsa_gwdt_get_wcv(struct watchdog_device *wdd)
>> +{
>> +       u32 wcv_lo, wcv_hi;
>> +
>> +       do {
>> +               wcv_hi = sbsa_gwdt_cf_read(SBSA_GWDT_WCV_HI, wdd);
>> +               wcv_lo = sbsa_gwdt_cf_read(SBSA_GWDT_WCV_LO, wdd);
>> +       } while (wcv_hi != sbsa_gwdt_cf_read(SBSA_GWDT_WCV_HI, wdd));
>
>
> Please add a comment indicating that you're trying to read WCV atomically.

OK , that makes sense

>
>> +
>> +       return (((u64)wcv_hi << 32) | wcv_lo);
>> +}
>
>
> How about defining this macro:
>
> #define make64(high, low) (((u64)(high) << 32) | (low))
>
> and using it instead?  That makes the code easier to read.

good idea, but it's  just  used once, not sure if it's worthy
Actually, I have seen some macro in some driver, but not in kernel header file.

>
>> +
>> +static void sbsa_gwdt_set_wcv(struct watchdog_device *wdd, u64 value)
>> +{
>> +       u32 wcv_lo, wcv_hi;
>> +
>> +       wcv_lo = value & U32_MAX;
>> +       wcv_hi = (value >> 32) & U32_MAX;
>
>
> Use upper_32_bits() and lower_32_bits() instead.

cool, thanks ,  fixed it

>
>> +
>> +       sbsa_gwdt_cf_write(SBSA_GWDT_WCV_HI, wcv_hi, wdd);
>> +       sbsa_gwdt_cf_write(SBSA_GWDT_WCV_LO, wcv_lo, wdd);
>> +}
>> +
>> +static void reload_timeout_to_wcv(struct watchdog_device *wdd)
>
>
> This should be sbsa_gwdt_reload_timeout_to_wcv()
>
>> +{
>> +       struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
>> +       u64 wcv;
>> +
>> +       wcv = arch_counter_get_cntvct() +
>> +               (u64)(wdd->timeout - wdd->pretimeout) * gwdt->clk;
>> +
>> +       sbsa_gwdt_set_wcv(wdd, wcv);
>
>
> Shouldn't you program WCV and WOR together?

why? WOR just for pretimeout in this driver.

>
>> +static int sbsa_gwdt_set_pretimeout(struct watchdog_device *wdd,
>> +                                   unsigned int pretimeout)
>> +{
>> +       struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
>> +       u32 wor;
>> +
>> +       wdd->pretimeout = pretimeout;
>> +       sbsa_gwdt_update_limits(wdd);
>> +
>> +       if (!pretimeout)
>> +               /* gives sbsa_gwdt_start a chance to setup timeout */
>> +               wor = gwdt->clk;
>> +       else
>> +               wor = pretimeout * gwdt->clk;
>> +
>> +       /* refresh the WOR, that will cause an explicit watchdog refresh
>> */
>> +       sbsa_gwdt_cf_write(SBSA_GWDT_WOR, wor, wdd);
>
>
> Why not just ping the watchdog explicitely?

we just setup WOR, but we don't need to load pretimeout to WCV now, right ?

>
>> +static irqreturn_t sbsa_gwdt_interrupt(int irq, void *dev_id)
>> +{
>> +       struct sbsa_gwdt *gwdt = (struct sbsa_gwdt *)dev_id;
>> +       struct watchdog_device *wdd = &gwdt->wdd;
>> +       u32 status;
>> +
>> +       status = sbsa_gwdt_cf_read(SBSA_GWDT_WCS, wdd);
>> +
>> +       if (status & SBSA_GWDT_WCS_WS0)
>
>
> This should always be true.  Instead of reading WCS, I think you should just
> panic().

I thinks I need to confirm it , in case this has been cleaned.

>
>> +static int sbsa_gwdt_probe(struct platform_device *pdev)
>> +{
>> +       struct device *dev = &pdev->dev;
>> +       struct sbsa_gwdt *gwdt;
>> +       struct watchdog_device *wdd;
>> +       struct resource *res;
>> +       void *rf_base, *cf_base;
>> +       int irq;
>> +       u32 clk, status;
>> +       int ret = 0;
>> +       u64 first_period_max = U64_MAX;
>> +
>> +       /*
>> +        * Get the frequency of system counter from
>> +        * the cp15 interface of ARM Generic timer
>> +        */
>> +       clk = arch_timer_get_cntfrq();
>> +       if (!clk) {
>
>
> You have
>
>         depends on ARM_ARCH_TIMER
>
> in your Kconfig, so you don't need to check the return of
> arch_timer_get_cntfrq().  It can never be zero.
>
> Also, I would not use the variable name 'clk', because that's usually used
> for a "struct clk" object.  I would call this "freq" instead.

yes, I have fixed it .

>
>
> --
> Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the
> Code Aurora Forum, a Linux Foundation Collaborative Project.

Hi Timur

On 29 May 2015 at 21:28, Timur Tabi <timur@codeaurora.org> wrote:
> Fu Wei wrote:
>>>
>>> This should always be true.  Instead of reading WCS, I think you should
>>> just
>>> >panic().
>
>
>> I thinks I need to confirm it , in case this has been cleaned.
>
>
> I don't see how it's possible for you to receive the interrupt and have WCS
> be cleared.

It is a SPI, every CPU can get it,
But maybe I miss something, but please let me know if other CPU can
not get the interrupt.

>
> --
> Sent by an employee of the Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the
> Code Aurora Forum, hosted by The Linux Foundation.

Hi Guenter,



On 29 May 2015 at 22:54, Guenter Roeck <linux@roeck-us.net> wrote:
> On 05/29/2015 02:11 AM, Fu Wei wrote:
> [ ... ]
>
>>>> +
>>>> +       status = sbsa_gwdt_cf_read(SBSA_GWDT_WCS, wdd);
>>>> +       if (status & SBSA_GWDT_WCS_WS1) {
>>>> +               dev_warn(dev, "System reset by WDT(WCS: %x, WCV:
>>>> %llx)\n",
>>>> +                        status, sbsa_gwdt_get_wcv(wdd));
>>>
>>>
>>>
>>> Does this message (specifically the WCS / WCV values) have any
>>> useful meaning for the user ?
>>
>>
>> I think so, according to SBSA spec:
>> If WS0 is asserted and a timeout refresh occurs then the following must
>> occur:
>>      If the system is compliant to SBSA level 2 or higher the compare
>> value must retain its current value. This means that the compare value
>> records the time that WS1 is asserted.
>>
>> So, I think WCV can log the time when system reset by WDT, that may
>> help user figure out the problem. but WCS can be delete I think.
>>
>
> How would that help ? It is just a number with no reference to anything.

This number is the time, when system reset by watchdog
WCV : the number of clock, from system boot  to system reset.
Is that worthy to be logged? any suggestion ? :-)

>
> Thanks,
> Guenter
>

Hi Timur

On 29 May 2015 at 23:46, Timur Tabi <timur@codeaurora.org> wrote:
> On 05/29/2015 09:32 AM, Fu Wei wrote:
>>
>> It is a SPI, every CPU can get it,
>> But maybe I miss something, but please let me know if other CPU can
>> not get the interrupt.
>
>
> There's only one watchdog device, so there's only one interrupt.  I don't
> know which CPU will get the interrupt, but the watchdog is not a per-CPU
> device.
>

OK, first of all, I know, there maybe only watchdog in the system, but
the Interrupter is SPI,
that means maybe some other device can also trigger the same
Interrupter or maybe(in SMP) other CPU will handle it.

If this interrupter is triggered, that means system has goes wrong,
who knows what is wrong ,
I have to make sure that  system get into that routine ,because of the
WS0, if not I won't do panic.

And in a interrupter routine , checking the Interrupter status
register is right way to do.


> --
> Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the
> Code Aurora Forum, a Linux Foundation Collaborative Project.

Hi Guenter , Timur

On 30 May 2015 at 06:10, Guenter Roeck <linux@roeck-us.net> wrote:
> On 05/29/2015 08:46 AM, Timur Tabi wrote:
>>
>> On 05/29/2015 09:32 AM, Fu Wei wrote:
>>>
>>> It is a SPI, every CPU can get it,
>>> But maybe I miss something, but please let me know if other CPU can
>>> not get the interrupt.
>>
>>
>> There's only one watchdog device, so there's only one interrupt.  I don't
>> know which CPU will get the interrupt, but the watchdog is not a per-CPU
>> device.
>>
> Plus, that one interrupt is not shared, and the driver returns
> IRQ_HANDLED even if the bit is not set. So _something_ is definitely
> wrong. Either the interrupt is shared, then it needs to be requested
> as shared and the handler should only return IRQ_HANDLED if it actually
> handles the interrupt. Or it is not shared and the handler should always
> handle it.

I have thought about this again, For now, I did not find any reason to
keep that "if (status & SBSA_GWDT_WCS_WS0)"

So you are right, I should delete it.

and for IRQF_TIMER,  I will delete it.

Thanks for your correction.

>
> Guenter
>