Messages in this thread Patch in this message | | | Date | Tue, 22 Sep 2015 10:01:49 -0700 | From | Jacob Pan <> | Subject | Re: [PATCH] powercap / RAPL : remove dependency on iosf_mbi |
| |
On Tue, 22 Sep 2015 11:11:36 +0800 Pengyu Ma <pengyu.ma@windriver.com> wrote:
> > > On 09/22/2015 05:36 AM, Jacob Pan wrote: > > On Mon, 21 Sep 2015 11:48:14 +0800 > > Pengyu Ma <pengyu.ma@windriver.com> wrote: > > > >> > >> On 09/18/2015 11:43 PM, Jacob Pan wrote: > >>> On Fri, 18 Sep 2015 02:09:55 +0200 > >>> "Rafael J. Wysocki" <rjw@rjwysocki.net> wrote: > >>> > >>>> On Thursday, September 17, 2015 03:31:41 PM Pengyu Ma wrote: > >>>>> iosf_mbi is supported on Quark, Braswell, Baytrail and some Atom > >>>>> SoC, but RAPL is not limited to these SoC, it supports almost > >>>>> Intel CPUs. Remove this dependece to make RAPL support more > >>>>> Intel CPUs. > >>>>> > >>>>> Please select IOSF_MBI on Atom SoCs. > >>>>> > >>> Unlike Quark, I don't think we want to or do differentiate Atom > >>> from other x86 at compile time. IOSF driver can be compiled as a > >>> module also, therefore RAPL driver needs this explicit dependency > >>> at compile time. > >> As commit had exported iosf_mbi to let user use it. > >> > >> commit aa8e4f22ab7773352ba3895597189b8097f2c307 > >> Author: David E. Box <david.e.box@linux.intel.com> > >> Date: Wed Aug 27 14:40:39 2014 -0700 > >> > >> x86/iosf: Add Kconfig prompt for IOSF_MBI selection > >> > >> > >> While selecting IOSF_MBI is preferred, it does mean carrying extra > >> code on non-SoC architectures. > >> > >> We can NOT force user to build in iosf_mbi if they want use RAPL on > >> haswell/broadwell/skylake. > >> And RAPL can be compiled and worked well on > >> haswell/broadwell/skylake without IOSF_MBI. > >> RAPL is really NOT depended on IOSF_MBI. > >> > > True for haswell/broadwell/skylake platforms. But if we want binary > > compatibility for Atom and Core, I can' see how simply removing the > > dependency would work, unless we have runtime detection of IOSF. > If you want use iosf_mbi on atom, please select it on generic x86 > config. But not force it depend on another feature that not related > on it with other boards. > I don't care how iosf_mbi is added to kernel config, but why should I > be forced to add it if I want use RAPL? > It doesn't make any sense. > I understand your concern about wasting code. But let's look at all the cases of config options here. (without Kconfig dependency as you suggested)
RAPL\IOSF Y M N ___________________________________________________ Y OK DC* Warn on Atom** M OK OK Warn on Atom N OK OK OK ___________________________________________________
Notes: * DC: don't compile ** Warn on Atom is runtime if I add the following code to RAPL driver, but this case is ok.
--- a/drivers/powercap/intel_rapl.c +++ b/drivers/powercap/intel_rapl.c @@ -982,6 +982,11 @@ static void set_floor_freq_atom(struct rapl_domain *rd, bool enable) static u32 power_ctrl_orig_val; u32 mdata; + if (!iosf_mbi_available()) { + pr_warn("No IOSF MBI access to set floor frequency\n"); + return; + } +
So the problematic case is when RAPL=Y IOSF=M Since real IOSF functions are available when #if IS_ENABLED(CONFIG_IOSF_MBI) There will be no dummy functions for RAPL to reference in this case.
Since IOSF is a driver, making it a module is a reasonable requirement. As I mentioned before, I don't think we want to have a CONFIG_ATOM option for X86.
+David, HPA
Jacob > Pengyu > > > > >> Pengyu > >>>>> Signed-off-by: Pengyu Ma <pengyu.ma@windriver.com> > >>>> Jacob? > >>>> > >>>>> --- > >>>>> drivers/powercap/Kconfig | 2 +- > >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>>> > >>>>> diff --git a/drivers/powercap/Kconfig b/drivers/powercap/Kconfig > >>>>> index 85727ef..a7c81b5 100644 > >>>>> --- a/drivers/powercap/Kconfig > >>>>> +++ b/drivers/powercap/Kconfig > >>>>> @@ -17,7 +17,7 @@ if POWERCAP > >>>>> # Client driver configurations go here. > >>>>> config INTEL_RAPL > >>>>> tristate "Intel RAPL Support" > >>>>> - depends on X86 && IOSF_MBI > >>>>> + depends on X86 > >>>>> default n > >>>>> ---help--- > >>>>> This enables support for the Intel Running Average > >>>>> Power Limit (RAPL) > >>>>> > >>> [Jacob Pan] > > [Jacob Pan] >
[Jacob Pan]
| |