Re: [PATCH review for 4.4 12/47] clk: wm831x: fix usleep_range with bad range

[Nicholas Mc Guire]

On Sun, Sep 24, 2017 at 12:18:12AM +0000, Levin, Alexander (Sasha Levin) wrote:
> On Fri, Sep 22, 2017 at 09:46:28AM +0100, Charles Keepax wrote:
> >On Wed, Sep 20, 2017 at 04:45:02AM +0000, Levin, Alexander (Sasha Levin) wrote:
> >> From: Nicholas Mc Guire <hofrat@xxxxxxxxx>
> >>
> >> [ Upstream commit ed784c532a3d0959db488f40a96c5127f63d42dc ]
> >>
> >> The delay here is not in atomic context and does not seem critical with
> >> respect to precision, but usleep_range(min,max) with min==max results in
> >> giving the timer subsystem no room to optimize uncritical delays. Fix
> >> this by setting the range to 2000,3000 us.
> >>
> >> Fixes: commit f05259a6ffa4 ("clk: wm831x: Add initial WM831x clock driver")
> >> Signed-off-by: Nicholas Mc Guire <hofrat@xxxxxxxxx>
> >> Acked-by: Charles Keepax <ckeepax@xxxxxxxxxxxxxxxxxxxxxxxxxxx>
> >> Signed-off-by: Stephen Boyd <sboyd@xxxxxxxxxxxxxx>
> >> Signed-off-by: Sasha Levin <alexander.levin@xxxxxxxxxxx>
> >> ---
> >>  drivers/clk/clk-wm831x.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/clk/clk-wm831x.c b/drivers/clk/clk-wm831x.c
> >> index 763aed2de893..dfedcf5bc429 100644
> >> --- a/drivers/clk/clk-wm831x.c
> >> +++ b/drivers/clk/clk-wm831x.c
> >> @@ -101,7 +101,8 @@ static int wm831x_fll_prepare(struct clk_hw *hw)
> >>  	if (ret != 0)
> >>  		dev_crit(wm831x->dev, "Failed to enable FLL: %d\n", ret);
> >>
> >> -	usleep_range(2000, 2000);
> >> +	/* wait 2-3 ms for new frequency taking effect */
> >> +	usleep_range(2000, 3000);
> >
> >Does this patch really make sense for stable, isn't this really
> >just a small optimisation? The patch is pretty harmless so I
> >can't see applying it causing any problems, just curious what
> >problems not having it is causing.
> 
> Looking back at this, I think I misunderstood a scenario in the scheduler this might be causing. What you say makes sense, I'll drop it.
>

sorry for the delay - was off-line.

The motivation is that if usleep_range is used with min==max
then it allows no consolidation of highresolution timers at all
but as this is not an atomic code-section anyway it is not sensible
to force a precise timer - the pach relaxes the timing so that
the highrestimers load can be reduced. 

Technically this should have no effect at all as the jitter of
the system is probably a lot higher than the range given anyway
but the range allows optimization of highresolution timers.

So basically you are right its an optimization only but it is not
only relevant to keep the highrestimers well optimized it is also
the recommendation in the kernel documentation and since there is
not drawback with this optimization I think it should be considered even
if it is not important.

thx!
hofrat