From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753656AbcIIJ30 (ORCPT ); Fri, 9 Sep 2016 05:29:26 -0400 Received: from bombadil.infradead.org ([198.137.202.9]:34201 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753178AbcIIJ3X (ORCPT ); Fri, 9 Sep 2016 05:29:23 -0400 Date: Fri, 9 Sep 2016 11:29:08 +0200 From: Peter Zijlstra To: Harry Pan Cc: LKML , gs0622@gmail.com, tglx@linutronix.de, mingo@redhat.com, hpa@zytor.com, x86@kernel.org, bp@alien8.de, srinivas.pandruvada@linux.intel.com, ray.huang@amd.com Subject: Re: [PATCH 2/2] perf/x86/rapl: Enable Baytrail/Braswell RAPL support Message-ID: <20160909092908.GG10153@twins.programming.kicks-ass.net> References: <1473325738-730-1-git-send-email-harry.pan@intel.com> <1473325738-730-2-git-send-email-harry.pan@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1473325738-730-2-git-send-email-harry.pan@intel.com> User-Agent: Mutt/1.5.23.1 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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, > }; >