Date:	Wed, 3 Jun 2015 00:55:11 +0800
From:	Fu Wei <fu.wei@...aro.org>
To:	Timur Tabi <timur@...eaurora.org>
Cc:	Suravee Suthikulpanit <Suravee.Suthikulpanit@....com>,
	Linaro ACPI Mailman List <linaro-acpi@...ts.linaro.org>,
	linux-watchdog@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org,
	Wei Fu <tekkamanninja@...il.com>,
	G Gregory <graeme.gregory@...aro.org>,
	Al Stone <al.stone@...aro.org>,
	Hanjun Guo <hanjun.guo@...aro.org>,
	Ashwin Chaugule <ashwin.chaugule@...aro.org>,
	Arnd Bergmann <arnd@...db.de>,
	Guenter Roeck <linux@...ck-us.net>, vgandhi@...eaurora.org,
	wim@...ana.be, Jon Masters <jcm@...hat.com>,
	Leo Duran <leo.duran@....com>, Jon Corbet <corbet@....net>,
	Mark Rutland <mark.rutland@....com>,
	Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will.deacon@....com>, rjw@...ysocki.net
Subject: Re: [PATCH v4 5/7] Watchdog: introduce ARM SBSA watchdog driver

Hi Timur,

Thanks , feedback inline

On 2 June 2015 at 23:32, Timur Tabi <timur@...eaurora.org> wrote:
> On 06/01/2015 11:05 PM, fu.wei@...aro.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)

> 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 the software of a real server goes wrong, then you just directly restart it ,
never try to sync/protect the current data, never try to figure out
what is wrong with it.
I don't think that is a good server software.

At least, I don't thinks " pre-timeout will probably rarely be used"
is a good idea for a server.
in another word, in a server ,pre-timeout should always be used.

> So what happens if the CPU is totally hung and

Again, system reset in 1S, because WOR == (1s).

> this interrupt handler is never called?  When will the timeout occur?

if this interrupt hardler is never called,  what I can see is "some
one is feeding the dog".
OK, in case, WS0 is triggered, but  this interrupt hardler isn't
called, then software really goes wrong.  Then , Again, Again system
reset in 1S, because WOR == (1s).

>
>> +static int sbsa_gwdt_probe(struct platform_device *pdev)
>> +{
>> +       u64 first_period_max = U64_MAX;
>> +       struct device *dev = &pdev->dev;
>> +       struct watchdog_device *wdd;
>> +       struct sbsa_gwdt *gwdt;
>> +       struct resource *res;
>> +       void *rf_base, *cf_base;
>> +       int ret, irq;
>> +       u32 status;
>> +
>> +       gwdt = devm_kzalloc(dev, sizeof(*gwdt), GFP_KERNEL);
>> +       if (!gwdt)
>> +               return -ENOMEM;
>> +       platform_set_drvdata(pdev, gwdt);
>
>
> You should probably do this *after* calling platform_get_irq_byname().

 it just dose (pdev->) dev->driver_data = gwdt
If we got gwdt, we can do that.

But maybe I miss something(or a rule of usage),  so please let know
why this has to be called  *after* calling platform_get_irq_byname().

>
>> +
>> +       res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>> "refresh");
>> +       rf_base = devm_ioremap_resource(dev, res);
>> +       if (IS_ERR(rf_base))
>> +               return PTR_ERR(rf_base);
>> +
>> +       res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>> "control");
>> +       cf_base = devm_ioremap_resource(dev, res);
>> +       if (IS_ERR(cf_base))
>> +               return PTR_ERR(cf_base);
>> +
>> +       irq = platform_get_irq_byname(pdev, "ws0");
>> +       if (irq < 0) {
>> +               dev_err(dev, "unable to get ws0 interrupt.\n");
>> +               return irq;
>> +       }
>> +
>> +       /*
>> +        * Get the frequency of system counter from the cp15 interface of
>> ARM
>> +        * Generic timer. We don't need to check it, because if it returns
>> "0",
>> +        * system would panic in very early stage.
>> +        */
>> +       gwdt->clk = arch_timer_get_cntfrq();
>> +       gwdt->refresh_base = rf_base;
>> +       gwdt->control_base = cf_base;
>> +
>> +       wdd = &gwdt->wdd;
>> +       wdd->parent = dev;
>> +       wdd->info = &sbsa_gwdt_info;
>> +       wdd->ops = &sbsa_gwdt_ops;
>> +       watchdog_set_drvdata(wdd, gwdt);
>> +       watchdog_set_nowayout(wdd, nowayout);
>> +
>> +       wdd->min_pretimeout = 0;
>> +       wdd->max_pretimeout = U32_MAX / gwdt->clk;
>> +       wdd->min_timeout = 1;
>> +       do_div(first_period_max, gwdt->clk);
>> +       wdd->max_timeout = first_period_max;
>> +
>> +       wdd->pretimeout = DEFAULT_PRETIMEOUT;
>> +       wdd->timeout = DEFAULT_TIMEOUT;
>> +       watchdog_init_timeouts(wdd, pretimeout, timeout, dev);
>> +
>> +       status = sbsa_gwdt_cf_read(SBSA_GWDT_WCS, wdd);
>> +       if (status & SBSA_GWDT_WCS_WS1) {
>> +               dev_warn(dev, "System reset by WDT(WCV: %llx)\n",
>> +                        sbsa_gwdt_get_wcv(wdd));
>
>
> "System was previously reset via watchdog" is much clearer.

OK

>
>> +               wdd->bootstatus |= WDIOF_CARDRESET;
>> +       }
>> +       /* Check if watchdog is already enabled */
>> +       if (status & SBSA_GWDT_WCS_EN) {
>> +               dev_warn(dev, "already enabled!\n");
>
>
> "watchdog is already enabled".

I think I don't need to print "watchdog",   dev_warn(dev,  has help us on this.
If you do so , the message will be "watchdog: watchdog0: watchdog is
already enabled"

 > Never use exclamation marks in kernel
> messages.

that make sense , will delete it.

>
>> +               sbsa_gwdt_keepalive(wdd);
>> +       }
>> +
>> +       /* update pretimeout to WOR */
>> +       sbsa_gwdt_set_pretimeout(wdd, wdd->pretimeout);
>> +
>> +       ret = devm_request_irq(dev, irq, sbsa_gwdt_interrupt, 0,
>> +                              pdev->name, gwdt);
>> +       if (ret) {
>> +               dev_err(dev, "unable to request IRQ %d\n", irq);
>> +               return ret;
>> +       }
>> +
>> +       ret = watchdog_register_device(wdd);
>> +       if (ret)
>> +               return ret;
>> +
>> +       dev_info(dev, "Initialized with %ds timeout, %ds pretimeout @ %u
>> Hz\n",
>> +                wdd->timeout, wdd->pretimeout, gwdt->clk);
>
>
> if (wdd->pretimeout)
>         "watchdog initialized to %us timeout and %us pre-timeout at %u
> Hz\n", wdd->timeout, wdd->pretimeout, gwdt->clk
> else
>         "watchdog initialized to %us timeout at %u Hz\n", wdd->timeout,
> gwdt->clk
>
> I think it's unlikely that users will use pre-timeout, so your code should
> treat pre-timeout as a special case, not the normal usage.

I don't think so, that why you didn't use pretimeout in your driver.
Because you don't see the meaning of "pretimeout" to a  server.

>
> --
> Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the
> Code Aurora Forum, a Linux Foundation Collaborative Project.



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

Powered by blists - more mailing lists