Re: [PATCH v4 5/7] Watchdog: introduce ARM SBSA watchdog driver
From: Guenter Roeck
Date: Tue Jun 09 2015 - 00:37:29 EST
On 06/08/2015 08:59 PM, Fu Wei wrote:
Hi Guenter,
On 9 June 2015 at 02:26, Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
On 06/08/2015 09:05 AM, Fu Wei wrote:
Hi Gurnter
On 3 June 2015 at 01:07, Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
On 06/02/2015 09:55 AM, Fu Wei wrote:
Hi Timur,
Thanks , feedback inline
On 2 June 2015 at 23:32, Timur Tabi <timur@xxxxxxxxxxxxxx> wrote:
On 06/01/2015 11:05 PM, fu.wei@xxxxxxxxxx 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 still think you should get rid of these functions and just call
readl_relaxed() and writel_relaxed() every time, but I won't complain
again
if you keep them.
yes, that make sense, and will reduce the size of code, and I think
the code's readability will be OK too.
will try in my next patch,
+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;
+
+ if (wdd->pretimeout)
+ /* The pretimeout is valid, go panic */
+ panic("SBSA Watchdog pre-timeout");
+ else
+ /* We don't use pretimeout, trigger WS1 now*/
+ sbsa_gwdt_set_wcv(wdd, 0);
I don't like this.
If so, what is your idea ,if pretimeout == 0?
the reason of using WCV as (timout - pretimeout): it can provide the
longer timeout period,
(1)If we use WOR, it can only provide 10s @ 400MHz(max).
as Guenter said earlier, the default timer out for most watchdog will
be 30s, so I think 10s limit will be a little short
(2)we can always program WCV just like ping.
(3)if a timeout arrives, WOR will be use, so use it as pretimeout, but
we still can make this pretimeout longer by programming WCV(I don't
think it's necessary)
The triggering of the hardware reset should never depend
on an interrupt being handled properly.
if this fail, system reset in 1S, because WOR == (1s)
So ?
Even the interrupt routine isn't triggered, (WOR + system counter) -->
WCV,
then, sy system reset in 1S.
the hardware reset doesn't depend on an interrupt.
You should always program WCV
correctly in advance. This is especially true since pre-timeout will
probably rarely be used.
always programming WCV is doable. But I absolutely can not agree "
pre-timeout will probably rarely be used"
If so, SBSA watchdog is just a normal watchdog, This use case just
makes this HW useless.
If so, go to use SP805.
you still don't see the importance of this warning and pretimeout to a
real server.
If pretimeout isn't used, why not just set WCV = timeout, WOR = 0 ?
Because if WOR = 0 , according to SBSA, once you want to enable watchdog,
(0 + system counter) --> WCV , then , WS0 and WS1 will be triggered
immediately.
we have not a chance(a time slot) to update WCV.
I would have thought that this is exactly what we want if pretimeout is not
used.
Although pretimeout == 0 is not good for a server, but If
administrator set up pretimeout == 0. *I thinks we should trigger WS1
ASAP* . Because WS1 maybe a interrupter or a reboot signal, that is
why we can not reboot system directly.
This driver is SBSA watchdog driver, that means we need to follow SBSA spec:
(1) SBSA watchdog has two stage timeouts --> timeout and pretimeout is
the best solution in watchdog framework, at least for now.
(2) The watchdog has the following output signals:
Watchdog Signal 0 (WS0)---> "The initial signal is typically wired
to an interrupt and alerts the system."(original word from spec), I
thinks the key work should be "interrupt" and "alerts". So in WS0
interrupt routine, reset is absolutely a wrong operation. Although I
think we should make this "alerts" more useful. But for the first
version of driver, I thinks panic is useful enough.
Watchdog Signal 1 (WS1). ---> "The signal is fed to a higher agent
as an interrupt or reset for it to take executive action." (original
word from spec) . The key work should be "interrupt", "or" and "reset"
. So WS1 maybe a interrupt.
so even in the WS0 interrupt routine, if pretimeout == 0 , we need to
trigger WS1(that is what my patch is doing now, set WCV to 0, so WCV
is always less than SystemCounter, and in this situation(WS0 = TRUE),
WS1 will be trigger immediately), but definitely not a reset too.
But in worst case, if the WS0 is triggered, but the interrupt routine
doesn't work(can not set up WCV), it doesn't matter, we just need to
wait for a WOR(1s in my driver) timeout, then WS1 will be triggered.
That is hardware mechanism, once we config SBSA watchdog correctly,
that should work. If it doesn't, I think the chip design doesn't
follow the SBSA spec.
Make a summary here, for SBSA watchdog driver, it need to support two
stage timeouts and need to trigger WS0/1 when timeouts occur(can not
simply reset system in interrupt routines).
If a driver doesn't do these above, the driver can not be called SBSA
watchdog driver.
But according to SBSA, even pretimeout == 0, we can not setup WOR = 0.
if we make WOR = 0 , once we set up WCS(enabling watchdog), that cause
a explicit watchdog refresh, then WCV = (0 + system counter), so WS0
and WS1 will be triggered serially and immediately(in theory, the
I still don't understand why this would be a problem.
"delay" also depend on implementation). So in my patchset , if
pretimeout == 0, WOR will be 1s at least to make sure we have time to
setup WCV. I have made comments in the patch for explaining this.
Maybe some people want to ask: if we can not set up WOR = 0, but
pretimeout can be 0 and timeout can not, why I still want to use WOR
for pretimeout and using WCV as (timout - pretimeout) ??
For this:
(1)WCV can provide the longer timeout period, If we use WOR, it can
only provide 10s @ 400MHz(max). The default timer out for most
watchdog will be 30s, so I think 10s limit will be a little short.
(2)we can always program(write) WCV just like ping.
(3)if the first timeout occurs, WOR will be loaded to WCV(WOR + system
counter) automatically , so why not just use WOR as pretimeout?
Although we still can make this pretimeout longer by programming WCV,
I don't think it's necessary for now.
Too bad I don't have an arm64 system to test myself. I am not sure
I understand why WOR must be set to > 0 if pretimeout == 0, and even
if it must be set to a value > 0 I don't understand why
setting it to 1 (instead of 1 second) would not be sufficient.
Guenter
--
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/