Re: [PATCH v2 6/7] Watchdog: introduce ARM SBSA watchdog driver

[Fu Wei]

Hi Guenter,

Great thanks :-)

On 21 May 2015 at 23:18, Guenter Roeck <linux@xxxxxxxxxxxx> 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 <linux@xxxxxxxxxxxx> wrote:
>> > On 05/21/2015 01:32 AM, fu.wei@xxxxxxxxxx wrote:
>> >>
>> >> From: Fu Wei <fu.wei@xxxxxxxxxx>
>> >>
>> >> 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 <fu.wei@xxxxxxxxxx>
>> >> ---
>> >>   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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/