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

[Pengyu Ma]

On 09/23/2015 01:01 AM, Jacob Pan wrote:
> On Tue, 22 Sep 2015 11:11:36 +0800
> Pengyu Ma <pengyu.ma@xxxxxxxxxxxxx> wrote:
>
> > On 09/22/2015 05:36 AM, Jacob Pan wrote:
> > > On Mon, 21 Sep 2015 11:48:14 +0800
> > > Pengyu Ma <pengyu.ma@xxxxxxxxxxxxx> wrote:
> > >
> > > > On 09/18/2015 11:43 PM, Jacob Pan wrote:
> > > > > On Fri, 18 Sep 2015 02:09:55 +0200
> > > > > "Rafael J. Wysocki" <rjw@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> > > > 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@xxxxxxxxxxxxx>
> > > > > > 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/