Messages in this thread | | | Date | Mon, 08 Jun 2015 21:37:08 -0700 | From | Guenter Roeck <> | Subject | Re: [PATCH v4 5/7] Watchdog: introduce ARM SBSA watchdog driver |
| |
On 06/08/2015 08:59 PM, Fu Wei wrote: > Hi Guenter, > > > On 9 June 2015 at 02:26, Guenter Roeck <linux@roeck-us.net> wrote: >> On 06/08/2015 09:05 AM, Fu Wei wrote: >>> >>> Hi Gurnter >>> >>> On 3 June 2015 at 01:07, Guenter Roeck <linux@roeck-us.net> 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@codeaurora.org> wrote: >>>>>> >>>>>> >>>>>> On 06/01/2015 11:05 PM, fu.wei@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 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
| |