Re: [PATCH] powercap / RAPL : remove dependency on iosf_mbi

[Pengyu Ma <pengyu.ma@windriver.com>]

On 09/23/2015 01:01 AM, Jacob Pan wrote:
> 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.
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

Pengyu
>
> +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]