|To:||Deng-Cheng Zhu <email@example.com>|
|Subject:||Re: [PATCH v5 01/12] MIPS/Oprofile: extract PMU defines/helper functions for sharing|
|From:||David Daney <firstname.lastname@example.org>|
|Date:||Sat, 29 May 2010 11:20:08 -0700|
|Cc:||email@example.com, firstname.lastname@example.org, email@example.com, firstname.lastname@example.org, email@example.com, firstname.lastname@example.org, email@example.com, firstname.lastname@example.org|
|Dkim-signature:||v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:received:received:message-id:date:from :user-agent:mime-version:to:cc:subject:references:in-reply-to :content-type:content-transfer-encoding; bh=9lp72GXXX1zVQg0ZK2JdYeDyP76Che5QWL0OY0YQTec=; b=FCB9VTYL4jWTQ3+sZJzTrPBR5hfzPPUY2O9SsObnVWgJutINRaulxwiYl9UgHa2T4L bW3O3B8bi9akliKwsSCSRw4ZzK5Iwa/+qpKtOORF2qOaCzBhBLM9UxcpNSWNoQTssArO 0IpiE6X1WRONrtQVMkb891R4cHDDlMawggpjA=|
|Domainkey-signature:||a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=message-id:date:from:user-agent:mime-version:to:cc:subject :references:in-reply-to:content-type:content-transfer-encoding; b=tUe3AjrTDSbmPt4wwHtuqyeVS8h6nLt5WH/7unnkSrEAlliGViES2Pmr6Bdo9/a+hW jWPMkUt/vPIf0NU9jB0mQ6B1jjwJ8SgNiMX5r8qdWpnpK+1KcFxbHeRVSLRk0TIp7Xlc Rru0iUYxJiq4ybuActZiG8N7mNMqFTaOlLx+E=|
|References:||<email@example.com> <firstname.lastname@example.org> <4BFEE8B6.email@example.com> <AANLkTilx-yMwFE0cj3jQu1BZEx_XW2_sj-lRAfzNO88o@mail.gmail.com>|
|User-agent:||Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:220.127.116.11) Gecko/20100430 Fedora/3.0.4-2.fc11 Thunderbird/3.0.4|
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<firstname.lastname@example.org>: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.
|<Prev in Thread]||Current Thread||[Next in Thread>|
|Previous by Date:||Re: [PATCH v5 06/12] MIPS: add support for hardware performance events (mipsxx), David Daney|
|Next by Date:||[PATCHv2] mips: drop CLEAN_FILES from arch/mips/Makefile, Sam Ravnborg|
|Previous by Thread:||Re: [PATCH v5 01/12] MIPS/Oprofile: extract PMU defines/helper functions for sharing, Deng-Cheng Zhu|
|Next by Thread:||[PATCH v5 02/12] MIPS: use generic atomic64 in non-64bit kernels, Deng-Cheng Zhu|
|Indexes:||[Date] [Thread] [Top] [All Lists]|