Re: [PATCH v5 01/12] MIPS/Oprofile: extract PMU defines/helper functions for sharing

[David Daney <david.s.daney@gmail.com>]

On 05/28/2010 08:06 PM, Deng-Cheng Zhu wrote:
> Hi, David
>
>
> Thanks for your comments! I'm replying to you in the patch order. If my
> comments are wrong or are bad ideas, please point out.
>
>
> 2010/5/28 David Daney<david.s.daney@gmail.com>:
>    
>> Why predicate the entire contents of the file?
>>
>> In any event, if you keep it, it shold probably be something like:
>>
>>     #if defined(CONFIG_CPU_MIPSR1) || defined(CONFIG_CPU_MIPSR2)
>>      
> [DC]:
> 1) This line is not for the "entire" contents of the file, other CPUs
> LOONGSON2 and RM9000 are here.
>    

In any event I think you want defined(CONFIG_CPU_MIPSR1) || 
defined(CONFIG_CPU_MIPSR2) instead of what you had.

> 2) The CONFIG_CPU* came from the Makefile of oprofile. If other CPUs are
> newly supported, we can add into the #if #elif.
>    

TOO_MANY_#IF == BAD.  You are rewriting the code here, you can take the 
opportunity to improve it.

> 3) The perf counter helper functions are special to mipsxx/loongson2/rm9000
> espcially the reset_counter() will be implemented respectively. Although
> they will be moved to perf_event_$cpu.c when Oprofile uses Perf-events as
> backend, they are currently shared by Oprofile and Perf-events to reduce
> code copying.
>
>
>    
>> Some or all of that should probably go in asm/mipsregs.h
>>      
> [DC]: Now that we create pmu.h and these #define's are originally in
> op_model_$cpu.c not in mipsregs.h, how about keeping them here? BTW, after
> making Oprofile use Perf-events as backend (patches 8~12 do this), pmu.h
> will only contain register definitions (no static function definitions),
> then we can consider deleting pmu.h and moving its contents to mipsregs.h,
> is it OK?
>
>    

I don't have super strong preferences for where the bit definitions for 
cop0 registers go.  To date most are in mipsregs.h.  Perhaps Ralf has a 
preference.
>> Are 0 and 1 really the only conceivable values?
>>      
> [DC]: This is also from Oprofile. If we use:
> #define vpe_id()       (cpu_has_mipsmt_pertccounters ? \
>                         0 : cpu_data[smp_processor_id()].vpe_id)
> The possible return value is supposed to be 0 or 1.
>    

I haven't read the architecture specification for the multithread ASE.  
Is it really true that exactly two VPEs is all that is allowed?  If so 
this seems fine, otherwise it should reflect the full range of allowed 
values.

David Daney