[2/2] powercap/rapl: add support for denverton
diff mbox

Message ID 1464727290-5400-3-git-send-email-jacob.jun.pan@linux.intel.com
State Not Applicable, archived
Headers show

Commit Message

Jacob Pan May 31, 2016, 8:41 p.m. UTC
Denverton micro server is Atom based but RAPL interface
is compatible with Core based CPUs.

Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
---
 drivers/powercap/intel_rapl.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Dave Hansen May 31, 2016, 10:19 p.m. UTC | #1
On 05/31/2016 01:41 PM, Jacob Pan wrote:
> --- a/drivers/powercap/intel_rapl.c
> +++ b/drivers/powercap/intel_rapl.c
> @@ -1137,6 +1137,7 @@ static const struct x86_cpu_id rapl_ids[] __initconst = {
>  	RAPL_CPU(0x57, rapl_defaults_hsw_server),/* Knights Landing */
>  	RAPL_CPU(0x8E, rapl_defaults_core),/* Kabylake */
>  	RAPL_CPU(0x9E, rapl_defaults_core),/* Kabylake */
> +	RAPL_CPU(0x5F, rapl_defaults_core),/* Denverton micro server */
>  	{}
>  };

Not to derail this individual patch... but do we really want to continue
open-coding CPU model/family combos all over arch/x86?

For instance, arch/x86/events/intel/core.c has:

>         case 142: /* 14nm Kabylake Mobile */
>         case 158: /* 14nm Kabylake Desktop */
>         case 78: /* 14nm Skylake Mobile */
>         case 94: /* 14nm Skylake Desktop */
>         case 85: /* 14nm Skylake Server */

Which duplicates the two Kabylake family numbers from the RAPL_CPU()
context above (just in decimal instead of hex).

Should we just start sticking these things in a header like:

#define X86_INTEL_FAMILY_KABYLAKE1 	0x8E
#define X86_INTEL_FAMILY_KABYLAKE2	0x9E
#define X86_INTEL_FAMILY_DENVERTON 	0x5F

So we have this:

	RAPL_CPU(X86_INTEL_FAMILY_DENVERTON, rapl_defaults_core),

instead of having to explain our magic number in a comment.
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thomas Gleixner June 1, 2016, 6:57 a.m. UTC | #2
On Tue, 31 May 2016, Dave Hansen wrote:
> On 05/31/2016 01:41 PM, Jacob Pan wrote:
> > --- a/drivers/powercap/intel_rapl.c
> > +++ b/drivers/powercap/intel_rapl.c
> > @@ -1137,6 +1137,7 @@ static const struct x86_cpu_id rapl_ids[] __initconst = {
> >  	RAPL_CPU(0x57, rapl_defaults_hsw_server),/* Knights Landing */
> >  	RAPL_CPU(0x8E, rapl_defaults_core),/* Kabylake */
> >  	RAPL_CPU(0x9E, rapl_defaults_core),/* Kabylake */
> > +	RAPL_CPU(0x5F, rapl_defaults_core),/* Denverton micro server */
> >  	{}
> >  };
> 
> Not to derail this individual patch... but do we really want to continue
> open-coding CPU model/family combos all over arch/x86?
> 
> For instance, arch/x86/events/intel/core.c has:
> 
> >         case 142: /* 14nm Kabylake Mobile */
> >         case 158: /* 14nm Kabylake Desktop */
> >         case 78: /* 14nm Skylake Mobile */
> >         case 94: /* 14nm Skylake Desktop */
> >         case 85: /* 14nm Skylake Server */
> 
> Which duplicates the two Kabylake family numbers from the RAPL_CPU()
> context above (just in decimal instead of hex).
> 
> Should we just start sticking these things in a header like:
> 
> #define X86_INTEL_FAMILY_KABYLAKE1 	0x8E
> #define X86_INTEL_FAMILY_KABYLAKE2	0x9E
> #define X86_INTEL_FAMILY_DENVERTON 	0x5F
> 
> So we have this:
> 
> 	RAPL_CPU(X86_INTEL_FAMILY_DENVERTON, rapl_defaults_core),
> 
> instead of having to explain our magic number in a comment.

Yes please. 
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jacob Pan June 1, 2016, 3:08 p.m. UTC | #3
On Wed, 1 Jun 2016 08:57:27 +0200 (CEST)
Thomas Gleixner <tglx@linutronix.de> wrote:

> On Tue, 31 May 2016, Dave Hansen wrote:
> > On 05/31/2016 01:41 PM, Jacob Pan wrote:
> > > --- a/drivers/powercap/intel_rapl.c
> > > +++ b/drivers/powercap/intel_rapl.c
> > > @@ -1137,6 +1137,7 @@ static const struct x86_cpu_id rapl_ids[]
> > > __initconst = { RAPL_CPU(0x57, rapl_defaults_hsw_server),/*
> > > Knights Landing */ RAPL_CPU(0x8E, rapl_defaults_core),/* Kabylake
> > > */ RAPL_CPU(0x9E, rapl_defaults_core),/* Kabylake */
> > > +	RAPL_CPU(0x5F, rapl_defaults_core),/* Denverton micro
> > > server */ {}
> > >  };
> > 
> > Not to derail this individual patch... but do we really want to
> > continue open-coding CPU model/family combos all over arch/x86?
> > 
> > For instance, arch/x86/events/intel/core.c has:
> > 
> > >         case 142: /* 14nm Kabylake Mobile */
> > >         case 158: /* 14nm Kabylake Desktop */
> > >         case 78: /* 14nm Skylake Mobile */
> > >         case 94: /* 14nm Skylake Desktop */
> > >         case 85: /* 14nm Skylake Server */
> > 
> > Which duplicates the two Kabylake family numbers from the RAPL_CPU()
> > context above (just in decimal instead of hex).
> > 
> > Should we just start sticking these things in a header like:
> > 
> > #define X86_INTEL_FAMILY_KABYLAKE1 	0x8E
> > #define X86_INTEL_FAMILY_KABYLAKE2	0x9E
> > #define X86_INTEL_FAMILY_DENVERTON 	0x5F
> > 
> > So we have this:
> > 
> > 	RAPL_CPU(X86_INTEL_FAMILY_DENVERTON, rapl_defaults_core),
> > 
> > instead of having to explain our magic number in a comment.
> 
> Yes please. 
This open coding also applies to other x86 vendors. I can make change
for Intel since in some case, there is not even a comment about
what the model is. e.g.
in amd_nb.h
   (boot_cpu_data.x86 == 0x15 && boot_cpu_data.x86_model < 0x10))

Should the model numbers be in
arch/x86/include/asm/processor.h?
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/drivers/powercap/intel_rapl.c b/drivers/powercap/intel_rapl.c
index d51a8d4..f0fc1a3 100644
--- a/drivers/powercap/intel_rapl.c
+++ b/drivers/powercap/intel_rapl.c
@@ -1137,6 +1137,7 @@  static const struct x86_cpu_id rapl_ids[] __initconst = {
 	RAPL_CPU(0x57, rapl_defaults_hsw_server),/* Knights Landing */
 	RAPL_CPU(0x8E, rapl_defaults_core),/* Kabylake */
 	RAPL_CPU(0x9E, rapl_defaults_core),/* Kabylake */
+	RAPL_CPU(0x5F, rapl_defaults_core),/* Denverton micro server */
 	{}
 };
 MODULE_DEVICE_TABLE(x86cpu, rapl_ids);