lkml.org 
[lkml]   [2015]   [Jun]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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



    \
     
     \ /
      Last update: 2015-06-08 20:41    [W:3.932 / U:0.000 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site