diff mbox series

[04/15] powercap/intel_rapl: Support per Interface primitive information

Message ID 20230316153841.3666-5-rui.zhang@intel.com (mailing list archive)
State Superseded, archived
Headers show
Series powercap/intel_rapl: Introduce RAPL TPMI support | expand

Commit Message

Zhang, Rui March 16, 2023, 3:38 p.m. UTC 
RAPL primitive information is Interface specific.

Although current MSR and MMIO Interface share the same RAPL primitives,
new Interface like TPMI has its own RAPL primitive information.

Save the primitive information in the Interface private structure.

Plus, using variant name "rp" for struct rapl_primitive_info is
confusing because "rp" is also used for struct rapl_package.
Use "rpi" as the variant name for struct rapl_primitive_info, and rename
the previous rpi[] array to avoid conflict.

No functional change.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/powercap/intel_rapl_common.c | 50 ++++++++++++++++++----------
 include/linux/intel_rapl.h           |  2 ++
 2 files changed, 35 insertions(+), 17 deletions(-)

Comments

Rafael J. Wysocki March 30, 2023, 5:56 p.m. UTC | #1 
On Thu, Mar 16, 2023 at 4:42 PM Zhang Rui <rui.zhang@intel.com> wrote:
>
> RAPL primitive information is Interface specific.
>
> Although current MSR and MMIO Interface share the same RAPL primitives,
> new Interface like TPMI has its own RAPL primitive information.
>
> Save the primitive information in the Interface private structure.
>
> Plus, using variant name "rp" for struct rapl_primitive_info is
> confusing because "rp" is also used for struct rapl_package.
> Use "rpi" as the variant name for struct rapl_primitive_info, and rename
> the previous rpi[] array to avoid conflict.
>
> No functional change.
>
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> ---
>  drivers/powercap/intel_rapl_common.c | 50 ++++++++++++++++++----------
>  include/linux/intel_rapl.h           |  2 ++
>  2 files changed, 35 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/powercap/intel_rapl_common.c b/drivers/powercap/intel_rapl_common.c
> index 56e8af2a1e6f..898238285188 100644
> --- a/drivers/powercap/intel_rapl_common.c
> +++ b/drivers/powercap/intel_rapl_common.c
> @@ -654,7 +654,7 @@ static u64 rapl_unit_xlate(struct rapl_domain *rd, enum unit_type type,
>  }
>
>  /* in the order of enum rapl_primitives */
> -static struct rapl_primitive_info rpi[] = {
> +static struct rapl_primitive_info rpis_default[] = {

What does the 's' in the name stand for?

>         /* name, mask, shift, msr index, unit divisor */
>         PRIMITIVE_INFO_INIT(ENERGY_COUNTER, ENERGY_STATUS_MASK, 0,
>                             RAPL_DOMAIN_REG_STATUS, ENERGY_UNIT, 0),
> @@ -710,9 +710,20 @@ static struct rapl_primitive_info rpi[] = {
>         {NULL, 0, 0, 0},
>  };
>
> +static struct rapl_primitive_info *get_rpi(struct rapl_package *rp, int prim)
> +{
> +       struct rapl_primitive_info *rpi = rp->priv->rpi;
> +
> +       if (prim < 0 || prim > NR_RAPL_PRIMITIVES || !rpi)
> +               return NULL;
> +
> +       return &rpi[prim];
> +}
> +
>  static int rapl_config(struct rapl_package *rp)
>  {
>         rp->priv->rpd = (void *)rapl_defaults;
> +       rp->priv->rpi = (void *)rpis_default;
>         return 0;
>  }
>
> @@ -763,14 +774,14 @@ static int rapl_read_data_raw(struct rapl_domain *rd,
>  {
>         u64 value;
>         enum rapl_primitives prim_fixed = prim_fixups(rd, prim);
> -       struct rapl_primitive_info *rp = &rpi[prim_fixed];
> +       struct rapl_primitive_info *rpi = get_rpi(rd->rp, prim_fixed);
>         struct reg_action ra;
>         int cpu;
>
> -       if (!rp->name || rp->flag & RAPL_PRIMITIVE_DUMMY)
> +       if (!rpi || !rpi->name || rpi->flag & RAPL_PRIMITIVE_DUMMY)
>                 return -EINVAL;
>
> -       ra.reg = rd->regs[rp->id];
> +       ra.reg = rd->regs[rpi->id];
>         if (!ra.reg)
>                 return -EINVAL;
>
> @@ -778,26 +789,26 @@ static int rapl_read_data_raw(struct rapl_domain *rd,
>
>         /* domain with 2 limits has different bit */
>         if (prim == FW_LOCK && rd->rp->priv->limits[rd->id] == 2) {
> -               rp->mask = POWER_HIGH_LOCK;
> -               rp->shift = 63;
> +               rpi->mask = POWER_HIGH_LOCK;
> +               rpi->shift = 63;
>         }
>         /* non-hardware data are collected by the polling thread */
> -       if (rp->flag & RAPL_PRIMITIVE_DERIVED) {
> +       if (rpi->flag & RAPL_PRIMITIVE_DERIVED) {
>                 *data = rd->rdd.primitives[prim];
>                 return 0;
>         }
>
> -       ra.mask = rp->mask;
> +       ra.mask = rpi->mask;
>
>         if (rd->rp->priv->read_raw(cpu, &ra)) {
>                 pr_debug("failed to read reg 0x%llx on cpu %d\n", ra.reg, cpu);
>                 return -EIO;
>         }
>
> -       value = ra.value >> rp->shift;
> +       value = ra.value >> rpi->shift;
>
>         if (xlate)
> -               *data = rapl_unit_xlate(rd, rp->unit, value, 0);
> +               *data = rapl_unit_xlate(rd, rpi->unit, value, 0);
>         else
>                 *data = value;
>
> @@ -810,21 +821,24 @@ static int rapl_write_data_raw(struct rapl_domain *rd,
>                                unsigned long long value)
>  {
>         enum rapl_primitives prim_fixed = prim_fixups(rd, prim);
> -       struct rapl_primitive_info *rp = &rpi[prim_fixed];
> +       struct rapl_primitive_info *rpi = get_rpi(rd->rp, prim_fixed);
>         int cpu;
>         u64 bits;
>         struct reg_action ra;
>         int ret;
>
> +       if (!rpi || !rpi->name || rpi->flag & RAPL_PRIMITIVE_DUMMY)
> +               return -EINVAL;
> +
>         cpu = rd->rp->lead_cpu;
> -       bits = rapl_unit_xlate(rd, rp->unit, value, 1);
> -       bits <<= rp->shift;
> -       bits &= rp->mask;
> +       bits = rapl_unit_xlate(rd, rpi->unit, value, 1);
> +       bits <<= rpi->shift;
> +       bits &= rpi->mask;
>
>         memset(&ra, 0, sizeof(ra));
>
> -       ra.reg = rd->regs[rp->id];
> -       ra.mask = rp->mask;
> +       ra.reg = rd->regs[rpi->id];
> +       ra.mask = rpi->mask;
>         ra.value = bits;
>
>         ret = rd->rp->priv->write_raw(cpu, &ra);
> @@ -1176,8 +1190,10 @@ static void rapl_update_domain_data(struct rapl_package *rp)
>                          rp->domains[dmn].name);
>                 /* exclude non-raw primitives */
>                 for (prim = 0; prim < NR_RAW_PRIMITIVES; prim++) {
> +                       struct rapl_primitive_info *rpi = get_rpi(rp, prim);
> +
>                         if (!rapl_read_data_raw(&rp->domains[dmn], prim,
> -                                               rpi[prim].unit, &val))
> +                                               rpi->unit, &val))
>                                 rp->domains[dmn].rdd.primitives[prim] = val;
>                 }
>         }
> diff --git a/include/linux/intel_rapl.h b/include/linux/intel_rapl.h
> index 76d480733b0f..b935484dde3a 100644
> --- a/include/linux/intel_rapl.h
> +++ b/include/linux/intel_rapl.h
> @@ -122,6 +122,7 @@ struct reg_action {
>   * @write_raw:                 Callback for writing RAPL interface specific
>   *                             registers.
>   * @rpd:                       internal pointer to interface default settings
> + * @rpi:                       internal pointer to interface primitive info
>   */
>  struct rapl_if_priv {
>         struct powercap_control_type *control_type;
> @@ -132,6 +133,7 @@ struct rapl_if_priv {
>         int (*read_raw)(int cpu, struct reg_action *ra);
>         int (*write_raw)(int cpu, struct reg_action *ra);
>         void *rpd;
> +       void *rpi;
>  };
>
>  /* maximum rapl package domain name: package-%d-die-%d */
> --
> 2.25.1
>
Zhang, Rui April 2, 2023, 7:43 a.m. UTC | #2 
On Thu, 2023-03-30 at 19:56 +0200, Rafael J. Wysocki wrote:
> On Thu, Mar 16, 2023 at 4:42 PM Zhang Rui <rui.zhang@intel.com>
> wrote:
> > RAPL primitive information is Interface specific.
> > 
> > Although current MSR and MMIO Interface share the same RAPL
> > primitives,
> > new Interface like TPMI has its own RAPL primitive information.
> > 
> > Save the primitive information in the Interface private structure.
> > 
> > Plus, using variant name "rp" for struct rapl_primitive_info is
> > confusing because "rp" is also used for struct rapl_package.
> > Use "rpi" as the variant name for struct rapl_primitive_info, and
> > rename
> > the previous rpi[] array to avoid conflict.
> > 
> > No functional change.
> > 
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > ---
> >  drivers/powercap/intel_rapl_common.c | 50 ++++++++++++++++++----
> > ------
> >  include/linux/intel_rapl.h           |  2 ++
> >  2 files changed, 35 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/powercap/intel_rapl_common.c
> > b/drivers/powercap/intel_rapl_common.c
> > index 56e8af2a1e6f..898238285188 100644
> > --- a/drivers/powercap/intel_rapl_common.c
> > +++ b/drivers/powercap/intel_rapl_common.c
> > @@ -654,7 +654,7 @@ static u64 rapl_unit_xlate(struct rapl_domain
> > *rd, enum unit_type type,
> >  }
> > 
> >  /* in the order of enum rapl_primitives */
> > -static struct rapl_primitive_info rpi[] = {
> > +static struct rapl_primitive_info rpis_default[] = {
> 
> What does the 's' in the name stand for?

'rpi' stands for rapl_primitive_info, so I use 'rpis' for a series of
rapl_primitive_info.

thanks,
rui
Rafael J. Wysocki April 3, 2023, 5:51 p.m. UTC | #3 
On Sun, Apr 2, 2023 at 9:43 AM Zhang, Rui <rui.zhang@intel.com> wrote:
>
> On Thu, 2023-03-30 at 19:56 +0200, Rafael J. Wysocki wrote:
> > On Thu, Mar 16, 2023 at 4:42 PM Zhang Rui <rui.zhang@intel.com>
> > wrote:
> > > RAPL primitive information is Interface specific.
> > >
> > > Although current MSR and MMIO Interface share the same RAPL
> > > primitives,
> > > new Interface like TPMI has its own RAPL primitive information.
> > >
> > > Save the primitive information in the Interface private structure.
> > >
> > > Plus, using variant name "rp" for struct rapl_primitive_info is
> > > confusing because "rp" is also used for struct rapl_package.
> > > Use "rpi" as the variant name for struct rapl_primitive_info, and
> > > rename
> > > the previous rpi[] array to avoid conflict.
> > >
> > > No functional change.
> > >
> > > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > > ---
> > >  drivers/powercap/intel_rapl_common.c | 50 ++++++++++++++++++----
> > > ------
> > >  include/linux/intel_rapl.h           |  2 ++
> > >  2 files changed, 35 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/drivers/powercap/intel_rapl_common.c
> > > b/drivers/powercap/intel_rapl_common.c
> > > index 56e8af2a1e6f..898238285188 100644
> > > --- a/drivers/powercap/intel_rapl_common.c
> > > +++ b/drivers/powercap/intel_rapl_common.c
> > > @@ -654,7 +654,7 @@ static u64 rapl_unit_xlate(struct rapl_domain
> > > *rd, enum unit_type type,
> > >  }
> > >
> > >  /* in the order of enum rapl_primitives */
> > > -static struct rapl_primitive_info rpi[] = {
> > > +static struct rapl_primitive_info rpis_default[] = {
> >
> > What does the 's' in the name stand for?
>
> 'rpi' stands for rapl_primitive_info, so I use 'rpis' for a series of
> rapl_primitive_info.

I would call it rpi_default[] (without the 's'), because the "array"
part is implied by its definition, so the plural is not really
necessary.
diff mbox series

Patch

diff --git a/drivers/powercap/intel_rapl_common.c b/drivers/powercap/intel_rapl_common.c
index 56e8af2a1e6f..898238285188 100644
--- a/drivers/powercap/intel_rapl_common.c
+++ b/drivers/powercap/intel_rapl_common.c
@@ -654,7 +654,7 @@  static u64 rapl_unit_xlate(struct rapl_domain *rd, enum unit_type type,
 }
 
 /* in the order of enum rapl_primitives */
-static struct rapl_primitive_info rpi[] = {
+static struct rapl_primitive_info rpis_default[] = {
 	/* name, mask, shift, msr index, unit divisor */
 	PRIMITIVE_INFO_INIT(ENERGY_COUNTER, ENERGY_STATUS_MASK, 0,
 			    RAPL_DOMAIN_REG_STATUS, ENERGY_UNIT, 0),
@@ -710,9 +710,20 @@  static struct rapl_primitive_info rpi[] = {
 	{NULL, 0, 0, 0},
 };
 
+static struct rapl_primitive_info *get_rpi(struct rapl_package *rp, int prim)
+{
+	struct rapl_primitive_info *rpi = rp->priv->rpi;
+
+	if (prim < 0 || prim > NR_RAPL_PRIMITIVES || !rpi)
+		return NULL;
+
+	return &rpi[prim];
+}
+
 static int rapl_config(struct rapl_package *rp)
 {
 	rp->priv->rpd = (void *)rapl_defaults;
+	rp->priv->rpi = (void *)rpis_default;
 	return 0;
 }
 
@@ -763,14 +774,14 @@  static int rapl_read_data_raw(struct rapl_domain *rd,
 {
 	u64 value;
 	enum rapl_primitives prim_fixed = prim_fixups(rd, prim);
-	struct rapl_primitive_info *rp = &rpi[prim_fixed];
+	struct rapl_primitive_info *rpi = get_rpi(rd->rp, prim_fixed);
 	struct reg_action ra;
 	int cpu;
 
-	if (!rp->name || rp->flag & RAPL_PRIMITIVE_DUMMY)
+	if (!rpi || !rpi->name || rpi->flag & RAPL_PRIMITIVE_DUMMY)
 		return -EINVAL;
 
-	ra.reg = rd->regs[rp->id];
+	ra.reg = rd->regs[rpi->id];
 	if (!ra.reg)
 		return -EINVAL;
 
@@ -778,26 +789,26 @@  static int rapl_read_data_raw(struct rapl_domain *rd,
 
 	/* domain with 2 limits has different bit */
 	if (prim == FW_LOCK && rd->rp->priv->limits[rd->id] == 2) {
-		rp->mask = POWER_HIGH_LOCK;
-		rp->shift = 63;
+		rpi->mask = POWER_HIGH_LOCK;
+		rpi->shift = 63;
 	}
 	/* non-hardware data are collected by the polling thread */
-	if (rp->flag & RAPL_PRIMITIVE_DERIVED) {
+	if (rpi->flag & RAPL_PRIMITIVE_DERIVED) {
 		*data = rd->rdd.primitives[prim];
 		return 0;
 	}
 
-	ra.mask = rp->mask;
+	ra.mask = rpi->mask;
 
 	if (rd->rp->priv->read_raw(cpu, &ra)) {
 		pr_debug("failed to read reg 0x%llx on cpu %d\n", ra.reg, cpu);
 		return -EIO;
 	}
 
-	value = ra.value >> rp->shift;
+	value = ra.value >> rpi->shift;
 
 	if (xlate)
-		*data = rapl_unit_xlate(rd, rp->unit, value, 0);
+		*data = rapl_unit_xlate(rd, rpi->unit, value, 0);
 	else
 		*data = value;
 
@@ -810,21 +821,24 @@  static int rapl_write_data_raw(struct rapl_domain *rd,
 			       unsigned long long value)
 {
 	enum rapl_primitives prim_fixed = prim_fixups(rd, prim);
-	struct rapl_primitive_info *rp = &rpi[prim_fixed];
+	struct rapl_primitive_info *rpi = get_rpi(rd->rp, prim_fixed);
 	int cpu;
 	u64 bits;
 	struct reg_action ra;
 	int ret;
 
+	if (!rpi || !rpi->name || rpi->flag & RAPL_PRIMITIVE_DUMMY)
+		return -EINVAL;
+
 	cpu = rd->rp->lead_cpu;
-	bits = rapl_unit_xlate(rd, rp->unit, value, 1);
-	bits <<= rp->shift;
-	bits &= rp->mask;
+	bits = rapl_unit_xlate(rd, rpi->unit, value, 1);
+	bits <<= rpi->shift;
+	bits &= rpi->mask;
 
 	memset(&ra, 0, sizeof(ra));
 
-	ra.reg = rd->regs[rp->id];
-	ra.mask = rp->mask;
+	ra.reg = rd->regs[rpi->id];
+	ra.mask = rpi->mask;
 	ra.value = bits;
 
 	ret = rd->rp->priv->write_raw(cpu, &ra);
@@ -1176,8 +1190,10 @@  static void rapl_update_domain_data(struct rapl_package *rp)
 			 rp->domains[dmn].name);
 		/* exclude non-raw primitives */
 		for (prim = 0; prim < NR_RAW_PRIMITIVES; prim++) {
+			struct rapl_primitive_info *rpi = get_rpi(rp, prim);
+
 			if (!rapl_read_data_raw(&rp->domains[dmn], prim,
-						rpi[prim].unit, &val))
+						rpi->unit, &val))
 				rp->domains[dmn].rdd.primitives[prim] = val;
 		}
 	}
diff --git a/include/linux/intel_rapl.h b/include/linux/intel_rapl.h
index 76d480733b0f..b935484dde3a 100644
--- a/include/linux/intel_rapl.h
+++ b/include/linux/intel_rapl.h
@@ -122,6 +122,7 @@  struct reg_action {
  * @write_raw:			Callback for writing RAPL interface specific
  *				registers.
  * @rpd:			internal pointer to interface default settings
+ * @rpi:			internal pointer to interface primitive info
  */
 struct rapl_if_priv {
 	struct powercap_control_type *control_type;
@@ -132,6 +133,7 @@  struct rapl_if_priv {
 	int (*read_raw)(int cpu, struct reg_action *ra);
 	int (*write_raw)(int cpu, struct reg_action *ra);
 	void *rpd;
+	void *rpi;
 };
 
 /* maximum rapl package domain name: package-%d-die-%d */