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@xxxxxxxxx>:
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
