Re: [PATCH v3 5/6] Watchdog: introduce ARM SBSA watchdog driver
Hi Guenter , Timur On 30 May 2015 at 06:10, Guenter Roeck 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 > -- Best regards, Fu Wei Software Engineer Red Hat Software (Beijing) Co.,Ltd.Shanghai Branch Ph: +86 21 61221326(direct) Ph: +86 186 2020 4684 (mobile) Room 1512, Regus One Corporate Avenue,Level 15, One Corporate Avenue,222 Hubin Road,Huangpu District, Shanghai,China 200021 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 5/6] Watchdog: introduce ARM SBSA watchdog driver
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. Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 5/6] Watchdog: introduce ARM SBSA watchdog driver
On 05/29/2015 12:53 PM, Fu Wei wrote: 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. But the interrupt handler is not registered as shared, which means that it cannot be generated by another device. And in a interrupter routine , checking the Interrupter status register is right way to do. If you get an interrupt, but WS0 is not set, then you should return IRQ_NONE instead of IRQ_HANDLED. Also, I don't think IRQF_TIMER is correct. It's not a timer interrupt. Watchdogs are *not* timers. -- Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 5/6] Watchdog: introduce ARM SBSA watchdog driver
Hi Timur On 29 May 2015 at 23:46, 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. > 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. -- Best regards, Fu Wei Software Engineer Red Hat Software (Beijing) Co.,Ltd.Shanghai Branch Ph: +86 21 61221326(direct) Ph: +86 186 2020 4684 (mobile) Room 1512, Regus One Corporate Avenue,Level 15, One Corporate Avenue,222 Hubin Road,Huangpu District, Shanghai,China 200021 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 5/6] Watchdog: introduce ARM SBSA watchdog driver
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. -- Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 5/6] Watchdog: introduce ARM SBSA watchdog driver
Hi Guenter,
On 29 May 2015 at 22:54, Guenter Roeck 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
>
--
Best regards,
Fu Wei
Software Engineer
Red Hat Software (Beijing) Co.,Ltd.Shanghai Branch
Ph: +86 21 61221326(direct)
Ph: +86 186 2020 4684 (mobile)
Room 1512, Regus One Corporate Avenue,Level 15,
One Corporate Avenue,222 Hubin Road,Huangpu District,
Shanghai,China 200021
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 5/6] Watchdog: introduce ARM SBSA watchdog driver
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.
Thanks,
Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 5/6] Watchdog: introduce ARM SBSA watchdog driver
Hi Timur On 29 May 2015 at 21:28, Timur Tabi 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. -- Best regards, Fu Wei Software Engineer Red Hat Software (Beijing) Co.,Ltd.Shanghai Branch Ph: +86 21 61221326(direct) Ph: +86 186 2020 4684 (mobile) Room 1512, Regus One Corporate Avenue,Level 15, One Corporate Avenue,222 Hubin Road,Huangpu District, Shanghai,China 200021 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 5/6] Watchdog: introduce ARM SBSA watchdog driver
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. -- 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 5/6] Watchdog: introduce ARM SBSA watchdog driver
Hi Timur,
On 27 May 2015 at 00:50, Timur Tabi wrote:
> On 05/25/2015 05:03 AM, fu@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
Re: [PATCH v3 5/6] Watchdog: introduce ARM SBSA watchdog driver
Hi Guenter, Great thanks, feedback inline On 26 May 2015 at 03:39, Guenter Roeck wrote: > On 05/25/2015 03:03 AM, fu@linaro.org wrote: >> >> From: Fu Wei >> >> 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 >> Tested-by: Suravee Suthikulpanit >> Signed-off-by: Fu Wei >> --- > > > 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 000..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 >> + * Suravee Suthikulpanit >> + * >> + * 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--WS0T timeout >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +/* 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
Re: [PATCH v3 5/6] Watchdog: introduce ARM SBSA watchdog driver
On 05/25/2015 05:03 AM, fu@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.
+/*
+ * 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.
+
+ 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.
+
+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.
+
+ 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?
+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?
+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().
+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.
--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 5/6] Watchdog: introduce ARM SBSA watchdog driver
On 05/25/2015 03:03 AM, fu@linaro.org wrote: From: Fu Wei 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 Tested-by: Suravee Suthikulpanit Signed-off-by: Fu Wei --- 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 000..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 + * Suravee Suthikulpanit + * + * 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--WS0T timeout + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +/* 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 wat
[PATCH v3 5/6] Watchdog: introduce ARM SBSA watchdog driver
From: Fu Wei
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
Tested-by: Suravee Suthikulpanit
Signed-off-by: Fu Wei
---
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 000..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
+ * Suravee Suthikulpanit
+ *
+ * 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--WS0T timeout
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+/* 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
