On Monday 04 May 2015 20:04:44 fu.wei@linaro.org wrote:

From: Fu Wei fu.wei@linaro.org

Signed-off-by: Fu Wei fu.wei@linaro.org

.../devicetree/bindings/watchdog/sbsa-gwdt.txt | 41 ++++++++++++++++++++++ 1 file changed, 41 insertions(+) create mode 100644 Documentation/devicetree/bindings/watchdog/sbsa-gwdt.txt

diff --git a/Documentation/devicetree/bindings/watchdog/sbsa-gwdt.txt b/Documentation/devicetree/bindings/watchdog/sbsa-gwdt.txt new file mode 100644 index 0000000..14c9ec5 --- /dev/null +++ b/Documentation/devicetree/bindings/watchdog/sbsa-gwdt.txt @@ -0,0 +1,41 @@ +* SBSA(Server Base System Architecture) Generic Watchdog

+The SBSA Generic Watchdog Timer is used for resuming system operation +after two stages of timeout. +More details: ARM-DEN-0029 - Server Base System Architecture (SBSA)

+Required properties: +- compatible : Should at least contain "arm,sbsa-gwdt".

+- reg : base physical address of the frames and length of memory mapped region.

+- reg-names : Should contain the resource reg names to show the order of

• the values in "reg".
• Must include the following entries : "refresh_frame", "control_frame".

I'd suggest naming them "refresh" and "control" without the _frame postfix.

+Note that #address-cells, #size-cells, and ranges shall be present to ensure +the CPU can address a frame's registers.

I don't see what you mean here. These properties should only be needed if there are child devices below the watchdog, but there are none.

+watchdog@2a450000 {

• compatible = "arm,sbsa-gwdt";
• #address-cells = <2>;
• #size-cells = <2>;
• reg = <0x0 0x2a450000 0 0x10000

•       0x0 0x2a440000 0 0x10000>;
  

Better write this as

reg = <0x0 0x2a450000 0 0x10000>, <0x0 0x2a440000 0 0x10000>;

for consistency.

• reg-names = "refresh_frame", "control_frame";
• interrupts = <0 27 4>;
• interrupt-names: "ws0"

typo.

• timeout-sec = <10 5>;

+};

Arnd

Hi Guenter,

Great thanks for your feedback, will fix them ASAP. Some feedback and comment inline below :

On 4 May 2015 at 21:42, Guenter Roeck linux@roeck-us.net wrote:

On 05/04/2015 05:04 AM, fu.wei@linaro.org wrote:

...

From: Fu Wei fu.wei@linaro.org

Signed-off-by: Fu Wei fu.wei@linaro.org

Please fix the subject.

Also, "checkpatch --strict" reports

total: 2 errors, 9 warnings, 20 checks, 700 lines checked

which is a bit on the high side for an explicit review. Please run it yourself and fix what it reports.

Sorry, I should do that before I sent it, you won't see those in my next version

Some more comments inline.

Thanks, Guenter

---

...

drivers/watchdog/Kconfig | 10 + drivers/watchdog/Makefile | 1 + drivers/watchdog/sbsa_gwdt.c | 578 +++++++++++++++++++++++++++++++++++++++++++ drivers/watchdog/sbsa_gwdt.h | 99 ++++++++ 4 files changed, 688 insertions(+) create mode 100644 drivers/watchdog/sbsa_gwdt.c create mode 100644 drivers/watchdog/sbsa_gwdt.h

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index e5e7c55..46d6f46 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -152,6 +152,16 @@ 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 && ARM_AMBA
  

•   select WATCHDOG_CORE
  

•   help
  

•     ARM SBSA Generic Watchdog timer. This has two Watchdog
  

Signal(WS0/WS1),

•     will trigger a warnning interrupt(do panic) in the first
  

timeout(WS0);

•     will reboot your system when the second timeout(WS1) is reached.
  

•     More details: 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..feee069 --- /dev/null +++ b/drivers/watchdog/sbsa_gwdt.c @@ -0,0 +1,578 @@ +/*

• • SBSA(Server Base System Architecture) Generic Watchdog driver
• • Copyright (c) 2015, Linaro Ltd.
• • Author: Fu Wei fu.wei@linaro.org
• • 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.
    

• •   But 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 with the system counter
    

• •   at 400MHz.
    

• •   And the second watch period is set by WOR which will be loaded
    

• •   automatically by hardware, when WS0 is triggered.
    

• •   More details: DEN0029B - Server Base System Architecture (SBSA)
    

• • Kernel/API: P---------| pretimeout

• •           |-------------------------------T timeout
    

• • SBSA GWDT: P--WOR---WS1 pretimeout

• •           |-------WCV----------WS0~~~~~~~~T timeout
    

• */

+#include <asm/arch_timer.h>

+#include <linux/acpi.h> +#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 "sbsa_gwdt.h"

+/* For the second watch period,

• • the timeout value comes from WOR(a 32bit wide. register).
• • This gives a maximum watch period of around 10s
• • at a maximum system counter frequency.
• • The System Counter shall run at maximum of 400MHz.
• • (Server Base System Architecture ARM-DEN-0029 Version 2.3)
• */

+#define DEFAULT_TIMEOUT 15 /* seconds, the 1st + 2nd watch period*/ +#define DEFAULT_PRETIMEOUT 5 /* seconds, the 2nd watch period*/

+/*store the System Counter clock frequency, in Hz.*/ +static u32 sbsa_gwdt_rate;

+static unsigned int timeout = 0; +module_param(timeout, uint, S_IRUGO|S_IWUSR); +MODULE_PARM_DESC(timeout,

•   "Watchdog timeout in seconds. (>=0, default="
  

•   __MODULE_STRING(DEFAULT_TIMEOUT) ")");
  

+static unsigned int max_timeout = UINT_MAX; +module_param(max_timeout, uint, S_IRUGO); +MODULE_PARM_DESC(max_timeout,

•   "Watchdog max timeout in seconds. (>=0, default="
  

•   __MODULE_STRING(UINT_MAX) ")");
  

+static unsigned int pretimeout = 0; +module_param(pretimeout, uint, S_IRUGO|S_IWUSR); +MODULE_PARM_DESC(pretimeout,

•   "Watchdog pretimeout in seconds. (>=0, default="
  

•   __MODULE_STRING(DEFAULT_PRETIMEOUT) ")");
  

+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) ")");
  

+/*

• • Architected system timer support.
• */

+static void sbsa_gwdt_cf_write(unsigned int reg, u32 val,

•                           struct watchdog_device *wdd)
  

+{

•   struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
  

•   struct sbsa_gwdt_device_data *device_data = &gwdt->device_data;
  

•   writel_relaxed(val, device_data->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);
  

•   struct sbsa_gwdt_device_data *device_data = &gwdt->device_data;
  

•   writel_relaxed(val, device_data->refresh_base + reg);
  

+}

+static u32 sbsa_gwdt_cf_read(unsigned int reg, struct watchdog_device *wdd) +{

•   struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
  

•   struct sbsa_gwdt_device_data *device_data = &gwdt->device_data;
  

•   return readl_relaxed(device_data->control_base + reg);
  

+}

+static u32 sbsa_gwdt_rf_read(unsigned int reg, struct watchdog_device *wdd) +{

•   struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
  

•   struct sbsa_gwdt_device_data *device_data = &gwdt->device_data;
  

•   return readl_relaxed(device_data->refresh_base + reg);
  

+}

+/*

• • help founctions for accessing 64bit WCV register
• • mutex_lock must be called prior to calling this function.
• */

+static u64 sbsa_gwdt_get_wcv(struct watchdog_device *wdd) +{

•   u32 wcv_lo, wcv_hi;
  

•   wcv_lo = sbsa_gwdt_cf_read(SBSA_GWDT_WCV_LO, wdd);
  

•   wcv_hi = sbsa_gwdt_cf_read(SBSA_GWDT_WCV_HI, wdd);
  

•   return (((u64)wcv_hi << 32) | wcv_lo);
  

+}

+void sbsa_gwdt_set_wcv(struct watchdog_device *wdd, u64 value) +{

•   u32 wcv_lo, wcv_hi;
  

•   wcv_lo = (u32)(value & U32_MAX);
  

•   wcv_hi = (u32)((value >> 32) & U32_MAX);
  
  

Those typecasts are unnecessary.

will fix it , Thanks

...

•   sbsa_gwdt_cf_write(SBSA_GWDT_WCV_LO, wcv_lo, wdd);
  

•   sbsa_gwdt_cf_write(SBSA_GWDT_WCV_HI, wcv_hi, wdd);
  

•   if (sbsa_gwdt_get_wcv(wdd) != value)
  

•           pr_err("sbsa_gwdt: set wcv fail!\n");
  

A function which can have an error should return an error code, not void.

OK , will fix it ,Thanks :-)

•   return;
  

...

+}

+void reload_timeout_to_wcv(struct watchdog_device *wdd) +{

•   struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
  

•   u64 cntvct, wcv;
  

•   /* refresh the wcv */
  

•   cntvct = arch_counter_get_cntvct();
  

•   wcv = cntvct + (wdd->timeout - gwdt->pretimeout) * sbsa_gwdt_rate;
  

•   sbsa_gwdt_set_wcv(wdd, wcv);
  

•   return;
  

+} +/* Use the following function to set the limit of timeout

• • after updating pretimeout */

+void sbsa_gwdt_set_timeout_limits(struct sbsa_gwdt *gwdt) +{

•   struct watchdog_device *wdd = &gwdt->wdd;
  

•   unsigned int first_period_max = (U64_MAX / sbsa_gwdt_rate);
  

•   wdd->min_timeout = gwdt->pretimeout + 1;
  

•   wdd->max_timeout = min((gwdt->pretimeout + first_period_max),
  
  

Please no unnecessary ( ).

yes, you are right , will fix it

•                           max_timeout);
  

...

•   return;
  

+}

+/* Use the following function to check if a pretimeout value is invalid */ +static inline bool watchdog_pretimeout_invalid(struct sbsa_gwdt *gwdt,

•                                       unsigned int t)
  

+{

•   return ((gwdt->max_pretimeout != 0) && (t >
  

gwdt->max_pretimeout));

Please no unnecessary ( ).

yes, will fix it

+}

...

+static int sbsa_gwdt_set_timeout(struct watchdog_device *wdd,

•                            unsigned int timeout)
  

+{

•   if (watchdog_timeout_invalid(wdd, timeout)) {
  

•           pr_err("sbsa_gwdt: timeout %d is out of range(%d~%d),
  

unchanged\n",

•                   timeout, wdd->min_timeout, wdd->max_timeout);
  

•           return -EINVAL;
  

•   }
  
  

The watchdog core already checks if the timeout range is valid. No need to check it again.

yes, sorry, will delete this check here. :-)

•   wdd->timeout = timeout;
  

...

•   /* refresh the wcv */
  

•   reload_timeout_to_wcv(wdd);
  

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

•   if (watchdog_pretimeout_invalid(gwdt, pretimeout)) {
  

•           pr_err("sbsa_gwdt: pretimeout %d is out of
  

range(0~%d),skip\n",

•                   pretimeout, gwdt->max_pretimeout);
  

•           return -EINVAL;
  

•   }
  
  

Same here.

sorry, this is for pretimeout , it's different.

•   gwdt->pretimeout = pretimeout;
  

...

•   sbsa_gwdt_set_timeout_limits(gwdt);
  

•   /* refresh the WOR, that will cause an explicit watchdog refresh
  

*/

•   wor = pretimeout * sbsa_gwdt_rate;
  

•   sbsa_gwdt_cf_write(SBSA_GWDT_WOR, wor, wdd);
  

•   return 0;
  

+}

+static unsigned int sbsa_gwdt_get_timeleft(struct watchdog_device *wdd) +{

•   u64 timeleft = sbsa_gwdt_get_wcv(wdd) - arch_counter_get_cntvct();
  

•   return (unsigned int)(timeleft / sbsa_gwdt_rate);
  

+}

+static int sbsa_gwdt_start(struct watchdog_device *wdd) +{

•   struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
  

•   struct sbsa_gwdt_device_data *device_data = &gwdt->device_data;
  

•   u32 status;
  

•   int ret;
  

•   ret = sbsa_gwdt_set_timeout(wdd, wdd->timeout);
  

•   if (ret)
  

•           return ret;
  

•   mutex_lock(&device_data->lock);
  

•   status = sbsa_gwdt_cf_read(SBSA_GWDT_WCS, wdd);
  

•   status |= SBSA_GWDT_WCS_EN;
  

•   /* writing WCS will cause an explicit watchdog refresh */
  

•   sbsa_gwdt_cf_write(SBSA_GWDT_WCS, status, wdd);
  

•   status = sbsa_gwdt_cf_read(SBSA_GWDT_WCS, wdd);
  

•   mutex_unlock(&device_data->lock);
  

•   /* Check if the watchdog was enabled */
  

•   if (!(status & SBSA_GWDT_WCS_EN))
  

•           return -EACCES;
  

EACCES is "permission denied". This is not the case here.

•   return 0;
  

...

+}

+static int sbsa_gwdt_stop(struct watchdog_device *wdd) +{

•   struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
  

•   struct sbsa_gwdt_device_data *device_data = &gwdt->device_data;
  

•   u32 status;
  

•   mutex_lock(&device_data->lock);
  

•   status = sbsa_gwdt_cf_read(SBSA_GWDT_WCS, wdd);
  

•   status &= ~SBSA_GWDT_WCS_EN;
  

•   /* writing WCS will cause an explicit watchdog refresh */
  

•   sbsa_gwdt_cf_write(SBSA_GWDT_WCS, status, wdd);
  

•   status = sbsa_gwdt_cf_read(SBSA_GWDT_WCS, wdd);
  

•   mutex_unlock(&device_data->lock);
  

•   /* Check if the watchdog was disabled */
  

•   if (status & SBSA_GWDT_WCS_EN)
  

•           return -EACCES;
  
  

Same as above.

For these two, maybe EPERM?

...

•   return 0;
  

+}

+static int sbsa_gwdt_keepalive(struct watchdog_device *wdd) +{

•   struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
  

•   struct sbsa_gwdt_device_data *device_data = &gwdt->device_data;
  

•   mutex_lock(&device_data->lock);
  

•   /* Writing WRR for an explicit watchdog refresh
  

•    * You can write anyting(like 0xc0ffee)
  

•    */
  

•   sbsa_gwdt_rf_write(SBSA_GWDT_WRR, 0xc0ffee, wdd);
  

•   mutex_unlock(&device_data->lock);
  

•   reload_timeout_to_wcv(wdd);
  

•   return 0;
  

+}

+static long sbsa_gwdt_ioctl(struct watchdog_device *wdd, unsigned int cmd,

•                           unsigned long arg)
  

+{

•   struct sbsa_gwdt *gwdt = watchdog_get_drvdata(wdd);
  

•   void __user *argp = (void __user *)arg;
  

•   int ret = -ENOIOCTLCMD;
  

•   unsigned int __user *p = argp;
  

•   unsigned int new_value;
  

•   /* FIXME: what else do we need here?? */
  

•   switch (cmd) {
  

•   case WDIOC_SETPRETIMEOUT:
  

•           if (get_user(new_value, p))
  

•                   return -EFAULT;
  

•           ret = sbsa_gwdt_set_pretimeout(wdd, new_value);
  

•           break;
  

•   case WDIOC_GETPRETIMEOUT:
  

•           ret = put_user(gwdt->pretimeout, (unsigned int __user
  

*)arg);

•           break;
  

•   }
  

•   return ret;
  

+}

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

•   unsigned int left;
  

•   pr_debug("SBSA Watchdog: in %s\n", __func__);
  

•   status = sbsa_gwdt_cf_read(SBSA_GWDT_WCS, wdd);
  

•   if (status & SBSA_GWDT_WCS_WS0) {
  

•           pr_crit("SBSA Watchdog WS0 is triggered\n");
  

•           left = sbsa_gwdt_get_timeleft(wdd);
  

•           pr_crit("please try to feed the dog in %ds!\n", left);
  

Pleae no unnecessary noise.

yes, will simplify it

Really, what do you expect the user to do here ? To be online, watch the kernel log, and do something frantic ?

:-) , that is the legacy code from first version, should be delete or simplifed.

•           /* FIXME:anything we can do here? */
  

...

•           panic("SBSA Watchdog pre-timeout");
  
  

Isn't this a pretimeout ?

yes, in kernel documentaion: ---------------------------------- Pretimeouts:

Some watchdog timers can be set to have a trigger go off before the actual time they will reset the system. This can be done with an NMI, interrupt, or other mechanism. This allows Linux to record useful information (like panic information and kernel coredumps) before it resets. --------------------------------- So I think it makes sense to do a panic here, and if the platform supprot kexec/kdump, it will be trigger automatically

any suggestion?

•   }
  

...

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

•   .ioctl          = sbsa_gwdt_ioctl,
  

•   .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 sbsa_gwdt_device_data *device_data;
  

•   struct watchdog_device *wdd;
  

•   struct resource *res;
  

•   void *rf_base, *cf_base;
  

•   int irq;
  

•   u32 status, flags;
  

•   u32 w_iidr, idr;
  

•   int ret = 0;
  

•   /*
  

•    * Try to determine the frequency from the cp15 interface
  

•    */
  

•   sbsa_gwdt_rate = arch_timer_get_cntfrq();
  

•   /* Check the system counter frequency. */
  

•   if (!sbsa_gwdt_rate)
  

•           dev_err(dev, "System Counter frequency not available\n");
  

•   gwdt = devm_kzalloc(dev, sizeof(*gwdt), GFP_KERNEL);
  

•   if (!gwdt)
  

•           return -ENOMEM;
  

•   wdd = &gwdt->wdd;
  

•   wdd->parent = dev;
  

•   device_data = &gwdt->device_data;
  

•   mutex_init(&device_data->lock);
  

•   res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
  

•                                      "refresh_frame");
  

•   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_frame");
  

•   cf_base = devm_ioremap_resource(dev, res);
  

•   if (IS_ERR(rf_base))
  

•           return PTR_ERR(rf_base);
  

•   irq = platform_get_irq_byname(pdev, "ws0");
  

•   if (IS_ERR(irq)) {
  

•           dev_err(dev, "required ws0 interrupt missing\n");
  

•           return PTR_ERR(irq);
  

•   }
  

•   res = platform_get_resource_byname(pdev, IORESOURCE_IRQ, "ws0");
  

•   if (!res)
  

•           dev_warn(dev, "ws0 interrupt flags missing, ignore\n");
  

•   else
  

•           flags = res->flags;
  

Not really sure why you get ws0 twice. Why don't you just use platform_get_resource_byname() and use it to get the irq number as well ?

yes, I did this before, but some suggestion said it is better to use platform_get_irq. so I am doing this way now but we can discuss more about this , and will check it again

Sure, that may not for for devicetree based initialization, but then you won't have flags available anyway, and always printing that warning on devicetree based systems isn't very useful either.

yes, you are right, will fix it, or maybe pr_debug

•   device_data->refresh_base = rf_base;
  

...

•   device_data->control_base = cf_base;
  

•   device_data->irq = irq;
  

•   device_data->flags = flags;
  

•   ret = devm_request_irq(dev, irq, sbsa_gwdt_interrupt,
  

•                        IRQF_TIMER, pdev->name, gwdt);
  

•   if (ret) {
  

•           dev_err(dev, "unable to request IRQ %d, disabling
  

device\n",

•                   irq);
  

•           return ret;
  

•   }
  

•   /* get device information from refresh/control frame */
  

•   w_iidr = sbsa_gwdt_rf_read(SBSA_GWDT_W_IIDR, &gwdt->wdd);
  

•   idr = sbsa_gwdt_rf_read(SBSA_GWDT_IDR, &gwdt->wdd);
  

•   dev_info(dev, "W_IIDR %x  IDR %x in RF.\n", w_iidr, idr);
  

•   /* update device information to device_data for future use*/
  

•   device_data->info.pid = SBSA_GWDT_W_IIDR_PID(w_iidr);
  

•   device_data->info.arch_ver = SBSA_GWDT_W_IIDR_ARCH_VER(w_iidr);
  

•   device_data->info.revision = SBSA_GWDT_W_IIDR_REV(w_iidr);
  

•   device_data->info.vid_bank = SBSA_GWDT_W_IIDR_VID_BANK(w_iidr);
  

•   device_data->info.vid = SBSA_GWDT_W_IIDR_VID(w_iidr);
  

•   wdd->info = &sbsa_gwdt_info;
  

•   wdd->ops = &sbsa_gwdt_ops;
  

•   watchdog_set_drvdata(wdd, gwdt);
  

•   status = sbsa_gwdt_cf_read(SBSA_GWDT_WCS, wdd);
  

•   if (status & SBSA_GWDT_WCS_WS1) {
  

•           dev_warn(dev, "System was reseted by SBSA Watchdog, WCS
  

%x\n",

•                    status);
  

•           wdd->bootstatus |= WDIOF_CARDRESET;
  

•           gwdt->wcv_dump = sbsa_gwdt_get_wcv(wdd);
  

•   }
  

•   watchdog_set_nowayout(wdd, nowayout);
  

•   gwdt->max_pretimeout = min((U32_MAX / sbsa_gwdt_rate),
  

•                              (max_timeout - 1));
  
  

Please no unnecessary ( ).

yes, will fix it

...

•   if (IS_ENABLED(CONFIG_OF) && pdev->dev.of_node) {
  

•           u32 timeouts[2];
  

•           ret = of_property_read_u32_array(pdev->dev.of_node,
  

•                                            "timeout-sec", timeouts,
  

2);

•           if (ret && timeout && pretimeout) {
  

•                   timeout = timeouts[0];
  

•                   pretimeout = timeouts[1];
  

•           }
  

•   }
  

•   if ((!timeout) || (!pretimeout)) {
  
  

Please no unnecessary ( ).

yes, will fix it

•           dev_info(dev, "get timeouts from DT or param fail."
  

...

•                         "Use DEFAULT.\n");
  

•           timeout = DEFAULT_TIMEOUT;
  

•           pretimeout = DEFAULT_PRETIMEOUT;
  

•   }
  

•   sbsa_gwdt_set_pretimeout(wdd, pretimeout);
  

•   sbsa_gwdt_set_timeout(wdd, timeout);
  

•   platform_set_drvdata(pdev, gwdt);
  

•   ret = watchdog_register_device(wdd);
  

•   if (ret)
  

•           return ret;
  

•   /* Check if watchdog is already enabled */
  

•   if (status & SBSA_GWDT_WCS_EN) {
  

•           dev_warn(dev, "already enabled!\n");
  

•           sbsa_gwdt_keepalive(wdd);
  

•   }
  

•   dev_info(dev, "registered with %ds timeout, %ds pretimeout\n",
  

•            wdd->timeout, gwdt->pretimeout);
  

•   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);
  

•   struct watchdog_device *wdd = &gwdt->wdd;
  

•   int ret = 0;
  

•   if (!nowayout)
  

•           ret = sbsa_gwdt_stop(wdd);
  

•   watchdog_unregister_device(wdd);
  

•   return ret;
  

+}

+#ifdef CONFIG_PM_SLEEP +/* Disable watchdog if it is active during suspend */ +static int sbsa_gwdt_suspend(struct device *dev) +{

•   struct sbsa_gwdt *gwdt = dev_get_drvdata(dev);
  

•   struct sbsa_gwdt_device_data *device_data = &gwdt->device_data;
  

•   struct watchdog_device *wdd = &gwdt->wdd;
  

•   mutex_lock(&device_data->lock);
  

•   gwdt->pm_status_store = sbsa_gwdt_cf_read(SBSA_GWDT_WCS, wdd);
  

•   mutex_unlock(&device_data->lock);
  

•   if (gwdt->pm_status_store & SBSA_GWDT_WCS_EN)
  

•           return sbsa_gwdt_stop(wdd);
  

•   return 0;
  

+}

+/* Enable watchdog and configure it if necessary */ +static int sbsa_gwdt_resume(struct device *dev) +{

•   struct sbsa_gwdt *gwdt = dev_get_drvdata(dev);
  

•   struct watchdog_device *wdd = &gwdt->wdd;
  

•   /*
  

•    * If watchdog was stopped before suspend be sure it gets disabled
  

•    * again, for the case BIOS has enabled it during resume
  

•    */
  

•   if (gwdt->pm_status_store & SBSA_GWDT_WCS_EN)
  

•           return sbsa_gwdt_start(wdd);
  

•   else
  

•           return sbsa_gwdt_stop(wdd);
  

+} +#endif

+#if IS_BUILTIN(CONFIG_OF) +static const struct of_device_id sbsa_gwdt_of_match[] = {

•   { .compatible = "arm,sbsa-gwdt", },
  

•   {},
  

+}; +MODULE_DEVICE_TABLE(of, sbsa_gwdt_of_match); +#endif

+static struct dev_pm_ops sbsa_gwdt_pm_ops = {

•   SET_SYSTEM_SLEEP_PM_OPS(sbsa_gwdt_suspend, sbsa_gwdt_resume)
  

+};

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

+};

+module_platform_driver(sbsa_gwdt_driver);

+MODULE_DESCRIPTION("SBSA Generic Watchdog Driver"); +MODULE_AUTHOR("Fu Wei fu.wei@linaro.org"); +MODULE_LICENSE("GPL v2"); diff --git a/drivers/watchdog/sbsa_gwdt.h b/drivers/watchdog/sbsa_gwdt.h new file mode 100644 index 0000000..5dc8ace --- /dev/null +++ b/drivers/watchdog/sbsa_gwdt.h @@ -0,0 +1,99 @@ +/*

• • SBSA(Server Base System Architecture) Generic Watchdog driver

definitions

• • Copyright (c) 2015, Linaro Ltd.
• • Author: Fu Wei fu.wei@linaro.org
• • 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.
• */

+#ifndef _SBSA_GENERIC_WATCHDOG_H_ +#define _SBSA_GENERIC_WATCHDOG_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)

All those ( ) are unnecessary.

for this define, I hope we can keep () to prevent someone use these uncarefully in some case, but it is also my habit to make define, will check it later

...

+/* Watchdog Control and Status Register */ +#define SBSA_GWDT_WCS_EN (1 << 0) +#define SBSA_GWDT_WCS_WS0 (1 << 1) +#define SBSA_GWDT_WCS_WS1 (1 << 2)

+/* Watchdog Interface Identification Register */ +#define SBSA_GWDT_W_IIDR_PID(x) ((x >> 20) & 0xfff) +#define SBSA_GWDT_W_IIDR_ARCH_VER(x) ((x >> 16) & 0xf) +#define SBSA_GWDT_W_IIDR_REV(x) ((x >> 12) & 0xf) +#define SBSA_GWDT_W_IIDR_IMPLEMENTER(x) (x & 0xfff) +#define SBSA_GWDT_W_IIDR_VID_BANK(x) ((x >> 8) & 0xf) +#define SBSA_GWDT_W_IIDR_VID(x) (x & 0x7f)

+/* Watchdog Identification Register */ +#define SBSA_GWDT_IDR_W_PIDR2 (0xfe8) +#define SBSA_GWDT_IDR_W_PIDR2_ARCH_VER(x) ((x >> 4) & 0xf)

+/**

• • struct sbsa_gwdt_info - SBSA Generic Watchdog device hardware

information

• • @pid: GWDT ProductID
• • @arch_ver: GWDT architecture version
• • @revision: GWDT revision number
• • @vid_bank: GWDT The JEP106 continuation code of the implementer
• • @vid: GWDT The JEP106 identity code of the implementer
• • @refresh_pa: the watchdog refresh frame(PA)
• • @control_pa: the watchdog control frame(PA)
• */

+struct sbsa_gwdt_info {

•   unsigned int    pid;
  

•   unsigned int    arch_ver;
  

•   unsigned int    revision;
  

•   unsigned int    vid_bank;
  

•   unsigned int    vid;
  

•   void            *refresh_pa;
  

•   void            *control_pa;
  

+};

+/**

• • struct sbsa_gwdt_device_data - Internal representation of the SBSA

GWDT

• • @refresh_base: VA of the watchdog refresh frame
• • @control_base: VA of the watchdog control frame
• • @irq: Watchdog Timer GSIV (WS0)
• • @flags: Watchdog Timer Flags
• • @dev: Pointer to kernel device structure
• • @info: SBSA Generic Watchdog info structure
• • @lock: GWDT mutex
• */

+struct sbsa_gwdt_device_data {

•   void __iomem            *refresh_base;
  

•   void __iomem            *control_base;
  

•   int                     irq;
  

•   u32                     flags;
  

•   struct device           *dev;
  

•   struct sbsa_gwdt_info   info;
  

•   struct mutex            lock;
  

+};

+struct sbsa_gwdt {

•   struct sbsa_gwdt_device_data    device_data;
  

•   struct watchdog_device          wdd;
  

•   unsigned int                    pretimeout; /* seconds */
  

•   unsigned int                    max_pretimeout;
  

•   u64                             wcv_dump;
  

+#ifdef CONFIG_PM_SLEEP

•   u8                              pm_status_store;
  

+#endif +};

+#define to_sbsa_gwdt(e) container_of(e, struct sbsa_gwdt, wdd)

+#endif /* _SBSA_GENERIC_WATCHDOG_H_ */

On Mon, May 04, 2015 at 08:04:48PM +0800, fu.wei@linaro.org wrote:

From: Fu Wei fu.wei@linaro.org

Your subject lines realy need some improvements.

I assume this one as well as the watchdog subject lines are accidential, but some of the others also look pretty long. Please read chapter 14 of Documentation/SubmittingPatches.

Thanks, Guenter

Signed-off-by: Fu Wei fu.wei@linaro.org

drivers/clocksource/arm_arch_timer.c | 16 ---------------- include/linux/acpi.h | 18 ++++++++++++++++++ 2 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c index 0aa135d..9753ffa 100644 --- a/drivers/clocksource/arm_arch_timer.c +++ b/drivers/clocksource/arm_arch_timer.c @@ -817,22 +817,6 @@ CLOCKSOURCE_OF_DECLARE(armv7_arch_timer_mem, "arm,armv7-timer-mem", arch_timer_mem_init); #ifdef CONFIG_ACPI -static int __init map_generic_timer_interrupt(u32 interrupt, u32 flags) -{

• int trigger, polarity;
• if (!interrupt)

• return 0;
  

• trigger = (flags & ACPI_GTDT_INTERRUPT_MODE) ? ACPI_EDGE_SENSITIVE

• 	: ACPI_LEVEL_SENSITIVE;
  

• polarity = (flags & ACPI_GTDT_INTERRUPT_POLARITY) ? ACPI_ACTIVE_LOW

• 	: ACPI_ACTIVE_HIGH;
  

• return acpi_register_gsi(NULL, interrupt, trigger, polarity);

-}

/* Initialize per-processor generic timer */ static int __init arch_timer_acpi_init(struct acpi_table_header *table) { diff --git a/include/linux/acpi.h b/include/linux/acpi.h index 598f0f1..8438b20 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -826,4 +826,22 @@ static inline bool acpi_spcr_console_check(struct acpi_device *adev, } #endif +#if defined(CONFIG_ACPI) +static inline int map_generic_timer_interrupt(u32 interrupt, u32 flags) +{

• int trigger, polarity;
• if (!interrupt)

• return 0;
  

• trigger = (flags & ACPI_GTDT_INTERRUPT_MODE) ? ACPI_EDGE_SENSITIVE

• 	: ACPI_LEVEL_SENSITIVE;
  

• polarity = (flags & ACPI_GTDT_INTERRUPT_POLARITY) ? ACPI_ACTIVE_LOW

• 	: ACPI_ACTIVE_HIGH;
  

• return acpi_register_gsi(NULL, interrupt, trigger, polarity);

+} +#endif

#endif /*_LINUX_ACPI_H*/

1.9.1

Hi Timur,

Thanks for your review, :-) comment inline below :

On 5 May 2015 at 05:25, Timur Tabi timur@codeaurora.org wrote:

On 05/04/2015 07:04 AM, fu.wei@linaro.org wrote:

...

From: Fu Wei fu.wei@linaro.org

Signed-off-by: Fu Wei fu.wei@linaro.org

Like others have said, you need to fix your patch descriptions.

Also, this patch should be 3/6. That is, before patch #4.

yes, will do so

---

...

arch/arm64/kernel/acpi.c | 154 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 154 insertions(+)

diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c index 8b83955..833d21e 100644 --- a/arch/arm64/kernel/acpi.c +++ b/arch/arm64/kernel/acpi.c @@ -23,6 +23,7 @@ #include <linux/irqdomain.h> #include <linux/memblock.h> #include <linux/of_fdt.h> +#include <linux/platform_device.h> #include <linux/smp.h>

#include <asm/cputype.h> @@ -343,3 +344,156 @@ void __init acpi_gic_init(void)

    early_acpi_os_unmap_memory((char *)table, tbl_size);

}

+static void __init *acpi_gtdt_subtable(struct acpi_table_gtdt *gtdt) +{

•   if (!gtdt->platform_timer_count)
  

•           return NULL;
  

•   return ((void *)gtdt + (gtdt->platform_timer_offset));
  

+}

+static int __init acpi_gtdt_set_resource_irq_flags(u32 timer_flags) +{

•   int flags;
  

•   switch (timer_flags &
  

•           (ACPI_GTDT_INTERRUPT_MODE |
  

ACPI_GTDT_INTERRUPT_POLARITY)) {

•   case (ACPI_GTDT_INTERRUPT_MODE | ACPI_GTDT_INTERRUPT_POLARITY):
  

•           flags = IORESOURCE_IRQ_LOWEDGE | IORESOURCE_IRQ;
  

•           pr_debug("GTDT: sbsa-gwdt irq flags is low edge");
  

•           break;
  

•   case ACPI_GTDT_INTERRUPT_MODE:
  

•           flags = IORESOURCE_IRQ_HIGHEDGE | IORESOURCE_IRQ;
  

•           pr_debug("GTDT: sbsa-gwdt irq flags is high edge");
  

•           break;
  

•   case ACPI_GTDT_INTERRUPT_POLARITY:
  

•           flags = IORESOURCE_IRQ_LOWLEVEL | IORESOURCE_IRQ;
  

•           pr_debug("GTDT: sbsa-gwdt irq flags is low level");
  

•           break;
  

•   default:
  

•           flags = IORESOURCE_IRQ_HIGHLEVEL | IORESOURCE_IRQ;
  

•           pr_debug("GTDT: sbsa-gwdt irq flags is high level");
  

•   }
  

•   pr_debug("GTDT: sbsa-gwdt irq flags:%d\n", flags);
  

•   return flags;
  
  

Blank line before the 'return'

yes, will do

+}

...

+static int __init acpi_gtdt_import_sbsa_gwdt(struct acpi_gtdt_header *table,

•                                        int index)
  

+{

•   struct acpi_gtdt_watchdog *watchdog;
  

•   int irq, gsi;
  

•   int err;
  

•   u32 flags;
  

•   void *rf_base_phy, *cf_base_phy;
  

•   struct platform_device *pdev;
  

•   struct resource *res;
  

•   watchdog = container_of(table, struct acpi_gtdt_watchdog, header);
  

•   /* get SBSA Generic Watchdog device information from GTDT */
  

•   rf_base_phy = (void *)watchdog->refresh_frame_address;
  

•   cf_base_phy = (void *)watchdog->control_frame_address;
  

•   irq = watchdog->timer_interrupt;
  

•   flags = watchdog->timer_flags;
  

•   pr_info("GTDT: found a device @ 0x%p/0x%p irq:%d flags:%x\n",
  

•           rf_base_phy, cf_base_phy, irq, flags);
  

•   if (!rf_base_phy || !cf_base_phy || !irq) {
  

•           pr_err("GTDT: fail to get the device info.\n");
  

•           return -EINVAL;
  

•   }
  

•   gsi = map_generic_timer_interrupt(irq, flags);
  
  

How about using acpi_register_gsi()? That way, you won't need to export map_generic_timer_interrupt().

will try to improve this , ;thanks

...

•   pdev = platform_device_alloc("sbsa-gwdt", index);
  

•   if (!pdev)
  

•           return -ENOMEM;
  

•   res = kcalloc(3, sizeof(*res), GFP_KERNEL);
  

•   if (!res)
  

•           goto err_free_device;
  

•   res[0].start = (resource_size_t)rf_base_phy;
  

•   res[0].end = (resource_size_t)(rf_base_phy + SZ_64K - 1);
  

•   res[0].name = "refresh_frame";
  

•   res[0].flags = IORESOURCE_MEM;
  

•   res[1].start = (resource_size_t)cf_base_phy;
  

•   res[1].end = (resource_size_t)(cf_base_phy + SZ_64K - 1);
  

•   res[1].name = "control_frame";
  

•   res[1].flags = IORESOURCE_MEM;
  

•   res[2].start = res[2].end = gsi;
  

•   res[2].name = "ws0";
  

•   res[2].flags = acpi_gtdt_set_resource_irq_flags(flags);
  

•   err = platform_device_add_resources(pdev, res, 3);
  

•   if (err)
  

•           goto err_free_res;
  

•   err = platform_device_add(pdev);
  

•   if (err)
  

•           goto err_free_res;
  

•   return 0;
  

+err_free_res:

•   kfree(res);
  

+err_free_device:

•   platform_device_put(pdev);
  

•   return err;
  

+}

+/* Initialize SBSA generic Watchdog platform device info from GTDT */ +static int __init acpi_gtdt_sbsa_gwdt_init(struct acpi_table_header *table) +{

•   struct acpi_table_gtdt *gtdt;
  

•   int i, timer_count, gwdt_count = 0;
  

•   struct acpi_gtdt_header *header;
  

•   void __iomem *gtdt_subtable;
  

•   if (table->revision < 2) {
  

•           pr_warn("GTDT: Revision:%d doesn't support SBSA GWDT.\n",
  

•                   table->revision);
  

•           return -ENODEV;
  

•   }
  

•   gtdt = container_of(table, struct acpi_table_gtdt, header);
  

•   timer_count = gtdt->platform_timer_count;
  

•   gtdt_subtable = acpi_gtdt_subtable(gtdt);
  
  

Just merge the contents of acpi_gtdt_subtable() into here. No one else will call acpi_gtdt_subtable().

I may do this in my next patchset

•   if (!gtdt_subtable) {
  

...

•           pr_warn("GTDT: No Platform Timer structures!\n");
  

•           return -EINVAL;
  

•   }
  

•   for (i = 0; i < timer_count; i++) {
  

•           header = (struct acpi_gtdt_header *)gtdt_subtable;
  

•           if (header->type == ACPI_GTDT_TYPE_WATCHDOG) {
  

•                   if (acpi_gtdt_import_sbsa_gwdt(header, i))
  

•                           pr_err("GTDT: adding sbsa-gwdt%d "
  

•                                  "platform device fail! subtable:
  

%d\n",

Please no exclamation marks in messages. Try to make your messages like normal English. How about this:

    pr_err("GTDT: failed trying to create platform sbsa-gwdt%d\n",

gwdt_count);