From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752508AbcIIPIt (ORCPT ); Fri, 9 Sep 2016 11:08:49 -0400 Received: from mga09.intel.com ([134.134.136.24]:28713 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752333AbcIIPIr (ORCPT ); Fri, 9 Sep 2016 11:08:47 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.30,305,1470726000"; d="scan'208";a="876872401" From: "Pan, Harry" To: "peterz@infradead.org" CC: "linux-kernel@vger.kernel.org" , "tglx@linutronix.de" , "ray.huang@amd.com" , "x86@kernel.org" , "hpa@zytor.com" , "srinivas.pandruvada@linux.intel.com" , "mingo@redhat.com" , "bp@alien8.de" Subject: Re: [PATCH 2/2] perf/x86/rapl: Enable Baytrail/Braswell RAPL support Thread-Topic: [PATCH 2/2] perf/x86/rapl: Enable Baytrail/Braswell RAPL support Thread-Index: AQHSCbUm4QPl834jhE2RK5j2Ulx1SaBwX7QAgABe3wA= Date: Fri, 9 Sep 2016 15:08:43 +0000 Message-ID: <1473433722.3685.3.camel@intel.com> References: <1473325738-730-1-git-send-email-harry.pan@intel.com> <1473325738-730-2-git-send-email-harry.pan@intel.com> <20160909092908.GG10153@twins.programming.kicks-ass.net> In-Reply-To: <20160909092908.GG10153@twins.programming.kicks-ass.net> Accept-Language: zh-TW, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.252.185.139] Content-Type: text/plain; charset="utf-8" Content-ID: MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id u89F8rP5001991 Hi Peter, Totally agreed and uploaded patchset again. https://lkml.org/lkml/2016/9/9/467 https://lkml.org/lkml/2016/9/9/468 One more thing, I did not refine rapl_advertise() description. Advice is welcome. Sincerely, Harry On Fri, 2016-09-09 at 11:29 +0200, Peter Zijlstra wrote: > On Thu, Sep 08, 2016 at 05:08:58PM +0800, Harry Pan wrote: > > > @@ -177,6 +187,16 @@ static inline u64 rapl_scale(u64 v, int cfg) > > pr_warn("Invalid domain %d, failed to scale data\n", cfg); > > return v; > > } > > + > > + /* > > + * Some Atom series processors (BYT/BSW) use 2^ESU microjoules. > > + * > > + * TODO: this looks hacky, it's better to refactor scale-up mechanism > > + * to compromise the main stream processors and Atom ones. > > + */ > > + if (is_baytrail) > > + return v << rapl_hw_unit[cfg - 1]; > > + > > Can't you simply set rapl_hw_unit[] such that 32 - rapl_hw_unit[] ends > up at the right number? Then you only get to much with values in > rapl_check_hw_unit without runtime overhead later. > > > +static int rapl_check_hw_unit(enum rapl_quirk apply_quirk) > > { > > u64 msr_rapl_power_unit_bits; > > int i; > > @@ -634,10 +674,20 @@ static int rapl_check_hw_unit(bool apply_quirk) > > * "Intel Xeon Processor E5-1600 and E5-2600 v3 Product Families, V2 > > * of 2. Datasheet, September 2014, Reference Number: 330784-001 " > > */ > > - if (apply_quirk) > > + if (apply_quirk == RAPL_HSX_QUIRK) > > rapl_hw_unit[RAPL_IDX_RAM_NRG_STAT] = 16; > > > > /* > > + * Some Atom processors (BYT/BSW) have 2^ESU microjoules increment, > > + * refer to Software Developers' Manual, Vol. 3C, Order No. 325384, > > + * Table 35-8 of MSR_RAPL_POWER_UNIT > > + */ > > + if (apply_quirk == RAPL_BYT_QUIRK) > > + is_baytrail = true; > > + else > > + is_baytrail = false; > > it was already false... > > /* > * comment explaining quirk goes here... > */ > if (apply_quirk = RAPL_BYT_QUIRK) { > for (i = 0; i < NR_RAPL_DOMAINS; i++) > rapl_hw_unit[i] = 32 - rapl_hw_unit[i]; > } > > and then you get to verify what to do with rapl_timer_ms. > > > > > static const struct intel_rapl_init_fun snb_rapl_init __initconst = { > > - .apply_quirk = false, > > + .apply_quirk = 0, > > Either leave it out (unmentioned members get initialized to 0) or add > RAPL_NO_QUIRK or so. > > > .cntr_mask = RAPL_IDX_CLN, > > .attrs = rapl_events_cln_attr, > > }; > >