Re: [PATCHv3/RESEND 4/4] clocksource: arch_timer: Add support for memory mapped timers

[Stephen Boyd <sboyd@codeaurora.org>]

On 06/21/13 14:18, Thomas Gleixner wrote:
> On Fri, 21 Jun 2013, Stephen Boyd wrote:
>>  /*
>>   * Architected system timer support.
>> @@ -46,13 +73,69 @@ static bool arch_timer_use_virtual = true;
>>  static inline void arch_timer_reg_write(int access, int reg, u32 val,
>>  					struct clock_event_device *clk)
>>  {
>> -	arch_timer_reg_write_cp15(access, reg, val);
>> +	if (access == ARCH_TIMER_MEM_PHYS_ACCESS) {
>> +		struct arch_timer *timer = to_arch_timer(clk);
>> +		switch (reg) {
>> +		case ARCH_TIMER_REG_CTRL:
>> +			writel_relaxed(val, timer->base + CNTP_CTL);
>> +			break;
>> +		case ARCH_TIMER_REG_TVAL:
>> +			writel_relaxed(val, timer->base + CNTP_TVAL);
>> +			break;
>> +		default:
>> +			BUILD_BUG();
> So you are relying on the compiler cleverness to identify a caller
> which calls that inline with a not supported reg value.
>
> How does that work, when the compiler decides not to inline that?
>
> enum without a default case emits at least a reliable warning.

Right now arm and arm64 don't allow inline to be anything besides
always_inline so the compiler is forced to inline. But point taken, I'll
remove the BUILD_BUG and make the register argument into an enum (which
it isn't right now).

>
>> +		}
>> +	} else if (access == ARCH_TIMER_MEM_VIRT_ACCESS) {
>> +		struct arch_timer *timer = to_arch_timer(clk);
>> +		switch (reg) {
>> +		case ARCH_TIMER_REG_CTRL:
>> +			writel_relaxed(val, timer->base + CNTV_CTL);
>> +			break;
>> +		case ARCH_TIMER_REG_TVAL:
>> +			writel_relaxed(val, timer->base + CNTV_TVAL);
>> +			break;
>> +		default:
>> +			BUILD_BUG();
>> +		}
>> +	} else {
>> +		arch_timer_reg_write_cp15(access, reg, val);
>> +	}
>>  }
> Something in my little brain yells: function pointer
>
> You can't be serious about hacking nested if/else/switch constructs
> into a hot path.
>
> Why not making your cpu data:
>
> struct arch_timer {
>        struct clock_event_device evt;
>        ....
>        void (*write_ctrl)(val, timer);
>        void (*write_tval)(val, timer);
>        ....
> }
>
> and get rid of all that conditionals?

It sounds like that's undesirable according to the comment above
arch_timer_reg_write(). It seems that all this code was written under
the assumption that the compiler is good enough to optimize all the code
paths and only generate the code that is necessary. So far this seems to
be working and the hotpath is optimized for each type of access.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation