From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: MIME-Version: 1.0 In-Reply-To: <20170130165359.GA20288@roeck-us.net> References: <1485312289-18354-1-git-send-email-baoyou.xie@linaro.org> <1485312289-18354-3-git-send-email-baoyou.xie@linaro.org> <20170125162346.GA591@linaro.org> <20170126161729.GA1515@linaro.org> <20170130165359.GA20288@roeck-us.net> From: Baoyou Xie Date: Wed, 1 Feb 2017 11:29:15 +0800 Message-ID: Subject: Re: [PATCH v6 3/3] watchdog: zx2967: add watchdog controller driver for ZTE's zx2967 family Content-Type: multipart/alternative; boundary=001a1144ca223fce0305476fa7f3 To: Guenter Roeck Cc: Mathieu Poirier , Jun Nie , wim@iguana.be, Rob Herring , Mark Rutland , linux-arm Mailing List , linux-watchdog@vger.kernel.org, devicetree , Linux Kernel Mailing List , Shawn Guo , "xie.baoyou" , chen.chaokai@zte.com.cn, wang.qiang01@zte.com.cn List-ID: --001a1144ca223fce0305476fa7f3 Content-Type: text/plain; charset=UTF-8 On 31 January 2017 at 00:53, Guenter Roeck wrote: > On Fri, Jan 27, 2017 at 10:40:09AM +0800, Baoyou Xie wrote: > > On 27 January 2017 at 00:17, Mathieu Poirier > > > 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 > > > > > > --- > [ ... ] > > > > > > + 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 > --001a1144ca223fce0305476fa7f3 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable


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 wr= ote:
> > > > > This patch adds watchdog controller driver for ZTE= 's zx2967 family.
> > > > >
> > > > > Signed-off-by: Baoyou Xie <baoyou.xie@linaro.org>
> > > > > ---
[ ... ]
> > > > > +=C2=A0 =C2=A0 =C2=A0= reset_control_assert(rstc);
> > > > > +=C2=A0 =C2=A0 =C2=A0usleep_range(500, 20000);
> > > >
> > > > Alright, I'm officially confused.
> > > >
> > > > First and foremost you still haven't provided an ex= planation as to why
> > > > this is
> > > > required.=C2=A0 Second, in your previous submission you= had:
> > > >
> > > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0mdelay(10);
> > > >
> > > > That is a busy loop of 10ms.=C2=A0 In this post using u= sleep_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 mergin= g the timer
> > > interrupts. of course, it's happy to replace it with usl= eep_range(500,
> > > 1000).
> >
> > "merging the timer interrupts" - you mean trying to get= the WD tick to be
> > closer
> > to other timers?=C2=A0 If so I really don't see why.=C2=A0 Ti= mers 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 dela= yed
> 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 woul= d
take much longer than necessary, with every process sitting on unnecessary<= br> 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 sto= ry here,
> about before ten years, I pressed a return key in console, you know, i= n
> this case, a process be created and exited, so the cpu core that proce= ss
> run at sent an IPI to other CPU, IPI interrupt resulted in the perform= ance
> 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<= br> 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.

=C2=A0
I have discussed with hardware and reset dri= ver 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.
=C2=A0
Guenter

--001a1144ca223fce0305476fa7f3--