Re: [PATCH v6 3/3] watchdog: zx2967: add watchdog controller driver for ZTE's zx2967 family

[Baoyou Xie <baoyou.xie@linaro.org>]

[-- Attachment #1: Type: text/plain, Size: 3522 bytes --]

On 31 January 2017 at 00:53, Guenter Roeck <linux@roeck-us.net> wrote:

> On Fri, Jan 27, 2017 at 10:40:09AM +0800, Baoyou Xie wrote:
> > On 27 January 2017 at 00:17, Mathieu Poirier <mathieu.poirier@linaro.org
> >
> > wrote:
> >
> > > On Thu, Jan 26, 2017 at 09:32:56AM +0800, Baoyou Xie wrote:
> > > > On 26 January 2017 at 00:23, Mathieu Poirier <
> mathieu.poirier@linaro.org
> > > >
> > > > wrote:
> > > >
> > > > > On Wed, Jan 25, 2017 at 10:44:49AM +0800, Baoyou Xie wrote:
> > > > > > This patch adds watchdog controller driver for ZTE's zx2967
> family.
> > > > > >
> > > > > > Signed-off-by: Baoyou Xie <baoyou.xie@linaro.org>
> > > > > > ---
> [ ... ]
> > > > > > +     reset_control_assert(rstc);
> > > > > > +     usleep_range(500, 20000);
> > > > >
> > > > > Alright, I'm officially confused.
> > > > >
> > > > > First and foremost you still haven't provided an explanation as to
> why
> > > > > this is
> > > > > required.  Second, in your previous submission you had:
> > > > >
> > > > >         mdelay(10);
> > > > >
> > > > > That is a busy loop of 10ms.  In this post using usleep_range() is
> a
> > > step
> > > > > in the
> > > > > right direction but the min and max values to wait for don't make
> > > sense.
> > > > > Here
> > > > > you have 500 and 20000, which is 0.5ms and 20ms.
> > > > >
> > > > > In fact, sleeping for 0.5ms is enough.
> > > > we extended the sleeping time to 20ms, the purpose is merging the
> timer
> > > > interrupts. of course, it's happy to replace it with
> usleep_range(500,
> > > > 1000).
> > >
> > > "merging the timer interrupts" - you mean trying to get the WD tick to
> be
> > > closer
> > > to other timers?  If so I really don't see why.  Timers are
> asynchronous by
> > > nature and there can be dozens of them enabled at any given time.
> > >
> > > Really?
> > Even if the system runs asynchronously, early process may trigger delayed
> > process, for example delayed work queue or timers, we can merge our
> > watchdog timer and those delayed work's timers.
> >
> In the probe function ?
>
> > Furthermore, what happen if we build this driver as module?
> >
> If every driver writer would use that line of argument, booting would
> take much longer than necessary, with every process sitting on unnecessary
> wait or sleep calls for perceived "optimization" purposes. Probe functions
> run exactly once, and should be time optimized. You should have an idea
> what the minimum reset hold time is, and follow it.
>
> > But, as i said early, it's a trial optimization but can be instead with
> > usleep_range(500, 1000).
> >
> > In some case, such optimization is helpful. I'd like to talk a story
> here,
> > about before ten years, I pressed a return key in console, you know, in
> > this case, a process be created and exited, so the cpu core that process
> > run at sent an IPI to other CPU, IPI interrupt resulted in the
> performance
> > decreased by 20%, so sad:)
> >
> It appears to be somewhat unlikely that each keypress resulted in a driver
> being instantiated. If it did, a sleep in its probe function was the least
> of your problems.
>
> > Unless there is a H/W constraint requiring a delay between the
> > > assert/deassert
> > > of the WD, I don't think adding a wait operation (of any kind) make
> sense.
>
> Correct.
>
>
I have discussed with hardware and reset driver engineers, It's better to
place usleep_range call into reset driver, so we're happy to remove
usleep_range
here.

So, I will send a new patch that remove it.


> Guenter
>

[-- Attachment #2: Type: text/html, Size: 5287 bytes --]