From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755853AbbEUPq5 (ORCPT ); Thu, 21 May 2015 11:46:57 -0400 Received: from mail-oi0-f53.google.com ([209.85.218.53]:33198 "EHLO mail-oi0-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755093AbbEUPqy (ORCPT ); Thu, 21 May 2015 11:46:54 -0400 MIME-Version: 1.0 In-Reply-To: <20150521151847.GA16668@roeck-us.net> References: <=fu.wei@linaro.org> <1432197156-16947-1-git-send-email-fu.wei@linaro.org> <1432197156-16947-7-git-send-email-fu.wei@linaro.org> <555DB4C4.5090606@roeck-us.net> <20150521151847.GA16668@roeck-us.net> Date: Thu, 21 May 2015 23:46:53 +0800 Message-ID: Subject: Re: [PATCH v2 6/7] Watchdog: introduce ARM SBSA watchdog driver From: Fu Wei To: Guenter Roeck Cc: Suravee Suthikulpanit , Linaro ACPI Mailman List , linux-watchdog@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org, Wei Fu , G Gregory , Al Stone , Hanjun Guo , Timur Tabi , Ashwin Chaugule , Arnd Bergmann , vgandhi@codeaurora.org, wim@iguana.be, Jon Masters , Leo Duran , Jon Corbet , Mark Rutland Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Guenter, Great thanks :-) On 21 May 2015 at 23:18, Guenter Roeck wrote: > On Thu, May 21, 2015 at 07:08:34PM +0800, Fu Wei wrote: >> Hi Guenter, >> >> Thanks for review :-) >> >> On 21 May 2015 at 18:34, Guenter Roeck wrote: >> > On 05/21/2015 01:32 AM, fu.wei@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(WS0), the interrupt routine run panic to save >> >> system context. >> >> >> >> Signed-off-by: Fu Wei >> >> --- >> >> drivers/watchdog/Kconfig | 12 ++ >> >> drivers/watchdog/Makefile | 1 + >> >> drivers/watchdog/sbsa_gwdt.c | 476 >> >> +++++++++++++++++++++++++++++++++++++++++++ >> >> 3 files changed, 489 insertions(+) >> >> create mode 100644 drivers/watchdog/sbsa_gwdt.c >> >> >> >> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig >> >> index e5e7c55..25a0df1 100644 >> >> --- a/drivers/watchdog/Kconfig >> >> +++ b/drivers/watchdog/Kconfig >> >> @@ -152,6 +152,18 @@ 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 ARM || ARM64 || COMPILE_TEST >> >> + depends on ARM_ARCH_TIMER >> >> + select WATCHDOG_CORE >> >> + help >> >> + ARM SBSA Generic Watchdog timer. This has two Watchdog Signals >> >> + (WS0/WS1), will trigger a warning interrupt(do panic) at the >> >> first >> >> + timeout(WS0); will reboot your system when the second >> >> timeout(WS1) >> >> + is reached. >> > >> > >> > I think it would be better to keep the WS0/WS1 out of the help text. >> > It is an implementation detail, and no user will be able to make sense of >> > it. >> >> I don't mind to delete it , but those are in SBSA spec, is that an >> implementation detail ? >> > I think so. > > Ask yourself - assuming you are not involved in the development of this driver, > and you read this help text. Is the help text going to help you to know about > WS0 and WS1 ? Why not just something like the following, if you want to be > verbose ? > > 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. Great thanks, that is better, yes, If I never read SBSA spec, I don't know WS is watchdog signal :-) > > Anyway, not worth arguing about. yes, agree , Thanks for the example "help" :-) > >> >> + >> >> + /* >> >> + * Try to determine the frequency from the arch_timer interface >> >> + */ >> >> + clk = arch_timer_get_rate(); >> > >> > >> > arch_timer_get_rate() does not seem to be exported. Did you try to build >> > the driver as module ? >> >> yes, I have built it as a ko module, that is why I made a patch to >> export this interface in the first patch of this patchset >> >> but I will confirm it again :-) >> > Guess I'll give it a try myself. I don't really understand how this > can work unless arch_timer_get_rate() is exported in your tree. yes, I have exported it , I think it make sense to export it. Because other driver maybe need to get system counter info Do you agree ? :-) > > 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