Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1160705 > unrolled thread

Re: [PATCH v4 5/7] Watchdog: introduce ARM SBSA watchdog driver

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v4 5/7] Watchdog: introduce ARM SBSA watchdog driver Guenter Roeck <linux@roeck-us.net> - 2015-06-08 20:30 +0200
    Re: [PATCH v4 5/7] Watchdog: introduce ARM SBSA watchdog driver Fu Wei <fu.wei@linaro.org> - 2015-06-09 06:00 +0200
      Re: [PATCH v4 5/7] Watchdog: introduce ARM SBSA watchdog driver Guenter Roeck <linux@roeck-us.net> - 2015-06-09 06:40 +0200
        Re: [PATCH v4 5/7] Watchdog: introduce ARM SBSA watchdog driver Guenter Roeck <linux@roeck-us.net> - 2015-06-09 10:10 +0200
          Re: [PATCH v4 5/7] Watchdog: introduce ARM SBSA watchdog driver Timur Tabi <timur@codeaurora.org> - 2015-06-11 02:30 +0200
            Re: [PATCH v4 5/7] Watchdog: introduce ARM SBSA watchdog driver Fu Wei <fu.wei@linaro.org> - 2015-06-11 05:10 +0200

#1160705 — Re: [PATCH v4 5/7] Watchdog: introduce ARM SBSA watchdog driver

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.
What am I missing here ?

Guenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1160961

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
"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.


> What am I missing here ?
>
> 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1160970

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1161110

On 06/08/2015 11:37 PM, Fu Wei wrote:

>
> I would like to stress that  pretimeout == 0 should not happen in a
> real server system,  that is why we defined  a SBSA watchdog, but not
> a normal one

Clarification - In _your opinion_, a server should always use pretimeout.

> But we still need to thinking about the situation that administrator
> want to do this on a very special purpose.
>
I could as well argue that setting pretimeout is the special situation.
Some administrators may not want to bother but just want the system to reset
if  a watchdog timeout occurs. _Maybe_ if it happens multiple times,
they might want to set up pretimeout to figure out why.

Declaring that one _has_ to configure pretimeout is just a personal
opinion, nothing else. We don't know what server administrators want,
and we should not dictate anything unless technically necessary.

> maybe we can set min_pretimeout = 1 for this driver. that is just a suggestion.
>
No. It is not technically necessary.

>
>> 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.
>
> it don't have to be 1 second , it can be 0.1, 0.5 or 0.01 second. like
> I said before, we just need a time slot to setup WCV in
> sbsa_gwdt_start. I have commented this in sbsa_gwdt_set_pretimeout in
> this patchset.

I think what you really want to say is that you want to have time to
handle the interrupt. But handling the interrupt is not asked for if
pretimeout == 0.

> But I think the minimum time slot depend on implementation, the spec
> doesn't mention about this.

Yes, because a minimum value does not exist.

> If pretimeout == 0, and we set up WOR a little bigger,  it *ONLY*
> affect "the worst case" I mention above. in this case, a administrator
> set up pretimeout == 0 which should not happen in a real server, then
> the system goes very wrong. I don't think it is important that this
> server system reset in 1 second or 0.0001 second, at this time, this
> server can not provide any useful info anyway(because we don't use
> pretimeout).
> If system may go wrong,  the administrator should set up pretimeout >
> 0 to figure out what is wrong with it just like he always should do.
>

Yes, exactly. But otherwise, if pretimeout is set to 0, we want
to reset immediately as directed.

As I interpret the specification, WOR=0 forces WS1 immediately after WS0.

1) if TimeoutRefresh = True:
	CompareValue := SystemCounter + WOR (= SystemCounter
	WS0 := True
2) TimeoutRefresh is True again, WS0 == True:
	WS1 = True

This is exactly the behavior we want if pretimeout == 0. In this situation,
we don't want to handle the interrupt, we just want to reset the system as
fast as possible.

Having said that, have you tested what happens in your system if you
set WOR=0 ?

Thanks,
Guenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1162949

Fu Wei wrote:
> If we make the first stage timeout is timeout/2,  this violates the
> definition of timeout.

The documentation says that the hardware needs to reset after the 
timeout expires.  If you program the hardware to timeout/2, the driver 
can ignore WS0 and allow WS1 to reset the hardware.  That conforms to 
the documentation.

-- 
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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1162971

On 11 June 2015 at 08:22, Timur Tabi <timur@codeaurora.org> wrote:
> Fu Wei wrote:
>>
>> If we make the first stage timeout is timeout/2,  this violates the
>> definition of timeout.
>
>
> The documentation says that the hardware needs to reset after the timeout
> expires.

yes , you are absolutely on this. Great thanks for point out this.
my patch is doing this way: trigger WS1 at the timeout expires
Before that, using pretimeout for WS0 as an warning.


> If you program the hardware to timeout/2, the driver can ignore
> WS0 and allow WS1 to reset the hardware.  That conforms to the
> documentation.

yes, technically we can do nothing in WS0, and just wait for WS1.
So SBSA watchdog become a one stage timeout watchdog, right?
So if user want the WS0 warning, How to make driver know that, and do
something in WS0 routine?
I think by this way,  two stage timeouts, WS0 "alert" will be meaningless.

Could you suggest a good way to use WS0, so we can follow SBSA spec?

>
> --
> 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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web