Date:	Thu, 24 Sep 2015 11:09:50 -0400
From:	Austin S Hemmelgarn <ahferroin7@...il.com>
To:	Pengyu Ma <pengyu.ma@...driver.com>,
	Jacob Pan <jacob.jun.pan@...ux.intel.com>
Cc:	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	rafael.j.wysocki@...el.com, linux-kernel@...r.kernel.org,
	"Box, David E" <david.e.box@...el.com>,
	"Anvin, H Peter" <h.peter.anvin@...el.com>
Subject: Re: [PATCH] powercap / RAPL : remove dependency on iosf_mbi

On 2015-09-24 06:03, Pengyu Ma wrote:
>
>
> On 09/23/2015 01:01 AM, Jacob Pan wrote:
>> On Tue, 22 Sep 2015 11:11:36 +0800
>> Pengyu Ma <pengyu.ma@...driver.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@...driver.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@...ysocki.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@...ux.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.
> iosf_mbi_write/read will warn itself.
>> 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.
> Actually there is a CONFIG_MATOM already in Kconfig.cpu
That's for code optimization (it changes compiler flags), not 
determining what system we're actually building for, and on top of that 
it's for older atom processors, not the new ones.  It's fully possible 
to build a kernel for an Atom processor without selecting this.
>
> Pengyu
>>
>> +David, HPA
>>
>> Jacob
>>
>>> Pengyu
>>>
>>>>> Pengyu
>>>>>>>> Signed-off-by: Pengyu Ma <pengyu.ma@...driver.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]
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/



Download attachment "smime.p7s" of type "application/pkcs7-signature" (3019 bytes)

Powered by blists - more mailing lists