Re: [PATCH V3 1/4] ACPI / CPPC: Optimize PCC Read Write operations

[Timur Tabi <timur@codeaurora.org>]

Ashwin Chaugule wrote:
> ..Only to be re substituted by the macro all over again. So, there
> really is no value in replacing this with a macro.  Also,
> readx_relaxed_poll_timeout() has a usleep(), which will kill all the
> optimization here. Its also horribly wrong in this context.
>
> The alternative readx_poll_timeout_atomic() has a udelay() but it also
> adds way more conditionals than this code. So, there is no need to
> change things for the sake of cosmetic reasons.

This is not a cosmetic change.  Calling a built-in macro instead of 
re-inventing the wheel is not cosmetic.  That's like saying that people 
shouldn't bother using min() or max() because they can just calculate 
the minimum and maximum themselves.  The macros exist for a reason, and 
people should be using them when applicable.

Also, the number of conditionals is not really much of an issue, I think.

The while loop has two conditionals:

1) The while-expression !ktime_after(ktime_get(), next_deadline)
2) The if-statement readw_relaxed(&generic_comm_base->status) & 
PCC_CMD_COMPLETE

readl_relaxed_poll_timeout_atomic() has four:

1) if (cond) break;
2) if (timeout_us && ktime_compare(ktime_get(), timeout) > 0)
3) if (delay_us) udelay(delay_us);	
4) (cond) ? 0 : -ETIMEDOUT; \

The third one is compiled-out because delay_us is a constant.  The 
fourth test is to handle the race condition between timeout and success. 
  If we time out, but the register value changes at the last 
microsecond, then we report success anyway.

And since this is a macro instead of a function, the code is generally 
optimized better.  The compiler typically can optimize macros better 
than functions, so there's still a chance that the macro version will be 
more optimized than the function anyway.

Finally, there's the convention of using a built-in macro/function over 
hand-coding something, even if it's not quite as optimized.  I don't 
think check_pcc_chan() is that time-critical that a single additional 
conditional is really a problem.  This while-loop really does re-invent 
the wheel.

Anyway, I don't want to belabor this point.  The decision is Rafael's now.

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum, a Linux Foundation collaborative project.