diff mbox series

[1/3] drm/i915/hwmon: Replace hwm_field_scale_and_write with hwm_power_max_write

Message ID 20230213210049.1900681-2-ashutosh.dixit@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915/hwmon: PL1 power limit fixes for ATSM | expand

Commit Message

Dixit, Ashutosh Feb. 13, 2023, 9 p.m. UTC 
hwm_field_scale_and_write has a single caller hwm_power_write and is
specific to hwm_power_write but makes it appear that it is a general
function which can have multiple callers. Replace the function with
hwm_power_max_write which is specific to hwm_power_write and use that in
future patches where the function needs to be extended.

Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
---
 drivers/gpu/drm/i915/i915_hwmon.c | 36 ++++++++++++++-----------------
 1 file changed, 16 insertions(+), 20 deletions(-)

Comments

Rodrigo Vivi Feb. 14, 2023, 2:50 p.m. UTC | #1 
On Mon, Feb 13, 2023 at 01:00:47PM -0800, Ashutosh Dixit wrote:
> hwm_field_scale_and_write has a single caller hwm_power_write and is
> specific to hwm_power_write but makes it appear that it is a general
> function which can have multiple callers. Replace the function with
> hwm_power_max_write which is specific to hwm_power_write and use that in
> future patches where the function needs to be extended.
> 
> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_hwmon.c | 36 ++++++++++++++-----------------
>  1 file changed, 16 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c
> index 1225bc432f0d5..85195d61f89c7 100644
> --- a/drivers/gpu/drm/i915/i915_hwmon.c
> +++ b/drivers/gpu/drm/i915/i915_hwmon.c
> @@ -99,20 +99,6 @@ hwm_field_read_and_scale(struct hwm_drvdata *ddat, i915_reg_t rgadr,
>  	return mul_u64_u32_shr(reg_value, scale_factor, nshift);
>  }
>  
> -static void
> -hwm_field_scale_and_write(struct hwm_drvdata *ddat, i915_reg_t rgadr,
> -			  int nshift, unsigned int scale_factor, long lval)
> -{
> -	u32 nval;
> -
> -	/* Computation in 64-bits to avoid overflow. Round to nearest. */
> -	nval = DIV_ROUND_CLOSEST_ULL((u64)lval << nshift, scale_factor);
> -
> -	hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr,
> -					    PKG_PWR_LIM_1,
> -					    REG_FIELD_PREP(PKG_PWR_LIM_1, nval));
> -}
> -
>  /*
>   * hwm_energy - Obtain energy value
>   *
> @@ -391,6 +377,21 @@ hwm_power_max_read(struct hwm_drvdata *ddat, long *val)
>  	return 0;
>  }
>  
> +static int
> +hwm_power_max_write(struct hwm_drvdata *ddat, long val)
 +{
> +	struct i915_hwmon *hwmon = ddat->hwmon;
> +	u32 nval;
> +
> +	/* Computation in 64-bits to avoid overflow. Round to nearest. */
> +	nval = DIV_ROUND_CLOSEST_ULL((u64)val << hwmon->scl_shift_power, SF_POWER);
> +
> +	hwm_locked_with_pm_intel_uncore_rmw(ddat, hwmon->rg.pkg_rapl_limit,
> +					    PKG_PWR_LIM_1,
> +					    REG_FIELD_PREP(PKG_PWR_LIM_1, nval));
> +	return 0;

Let's keep this function as void and the return 0 in the previous spot.
With that change:

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> +}
> +
>  static int
>  hwm_power_read(struct hwm_drvdata *ddat, u32 attr, int chan, long *val)
>  {
> @@ -425,16 +426,11 @@ hwm_power_read(struct hwm_drvdata *ddat, u32 attr, int chan, long *val)
>  static int
>  hwm_power_write(struct hwm_drvdata *ddat, u32 attr, int chan, long val)
>  {
> -	struct i915_hwmon *hwmon = ddat->hwmon;
>  	u32 uval;
>  
>  	switch (attr) {
>  	case hwmon_power_max:
> -		hwm_field_scale_and_write(ddat,
> -					  hwmon->rg.pkg_rapl_limit,
> -					  hwmon->scl_shift_power,
> -					  SF_POWER, val);
> -		return 0;
> +		return hwm_power_max_write(ddat, val);
>  	case hwmon_power_crit:
>  		uval = DIV_ROUND_CLOSEST_ULL(val << POWER_SETUP_I1_SHIFT, SF_POWER);
>  		return hwm_pcode_write_i1(ddat->uncore->i915, uval);
> -- 
> 2.38.0
>
Dixit, Ashutosh Feb. 14, 2023, 8:20 p.m. UTC | #2 
On Tue, 14 Feb 2023 06:50:44 -0800, Rodrigo Vivi wrote:
>
> > +static int
> > +hwm_power_max_write(struct hwm_drvdata *ddat, long val)
>  +{
> > +	struct i915_hwmon *hwmon = ddat->hwmon;
> > +	u32 nval;
> > +
> > +	/* Computation in 64-bits to avoid overflow. Round to nearest. */
> > +	nval = DIV_ROUND_CLOSEST_ULL((u64)val << hwmon->scl_shift_power, SF_POWER);
> > +
> > +	hwm_locked_with_pm_intel_uncore_rmw(ddat, hwmon->rg.pkg_rapl_limit,
> > +					    PKG_PWR_LIM_1,
> > +					    REG_FIELD_PREP(PKG_PWR_LIM_1, nval));
> > +	return 0;
>
> Let's keep this function as void and the return 0 in the previous spot.

Hmm, see your point. Though there is an identical situation for
hwm_power_max_read read too (in hwm_power_read). Maybe I'll change it there
too in the same patch to keep things symmetrical and retain your R-b?

> With that change:
>
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Thanks.
Rodrigo Vivi Feb. 14, 2023, 8:26 p.m. UTC | #3 
On Tue, Feb 14, 2023 at 12:20:36PM -0800, Dixit, Ashutosh wrote:
> On Tue, 14 Feb 2023 06:50:44 -0800, Rodrigo Vivi wrote:
> >
> > > +static int
> > > +hwm_power_max_write(struct hwm_drvdata *ddat, long val)
> >  +{
> > > +	struct i915_hwmon *hwmon = ddat->hwmon;
> > > +	u32 nval;
> > > +
> > > +	/* Computation in 64-bits to avoid overflow. Round to nearest. */
> > > +	nval = DIV_ROUND_CLOSEST_ULL((u64)val << hwmon->scl_shift_power, SF_POWER);
> > > +
> > > +	hwm_locked_with_pm_intel_uncore_rmw(ddat, hwmon->rg.pkg_rapl_limit,
> > > +					    PKG_PWR_LIM_1,
> > > +					    REG_FIELD_PREP(PKG_PWR_LIM_1, nval));
> > > +	return 0;
> >
> > Let's keep this function as void and the return 0 in the previous spot.
> 
> Hmm, see your point. Though there is an identical situation for
> hwm_power_max_read read too (in hwm_power_read). Maybe I'll change it there
> too in the same patch to keep things symmetrical and retain your R-b?

okay then. let's move this one as is and fix both functions in a follow up.

Thanks,
Rodrigo.

> 
> > With that change:
> >
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> Thanks.
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c
index 1225bc432f0d5..85195d61f89c7 100644
--- a/drivers/gpu/drm/i915/i915_hwmon.c
+++ b/drivers/gpu/drm/i915/i915_hwmon.c
@@ -99,20 +99,6 @@  hwm_field_read_and_scale(struct hwm_drvdata *ddat, i915_reg_t rgadr,
 	return mul_u64_u32_shr(reg_value, scale_factor, nshift);
 }
 
-static void
-hwm_field_scale_and_write(struct hwm_drvdata *ddat, i915_reg_t rgadr,
-			  int nshift, unsigned int scale_factor, long lval)
-{
-	u32 nval;
-
-	/* Computation in 64-bits to avoid overflow. Round to nearest. */
-	nval = DIV_ROUND_CLOSEST_ULL((u64)lval << nshift, scale_factor);
-
-	hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr,
-					    PKG_PWR_LIM_1,
-					    REG_FIELD_PREP(PKG_PWR_LIM_1, nval));
-}
-
 /*
  * hwm_energy - Obtain energy value
  *
@@ -391,6 +377,21 @@  hwm_power_max_read(struct hwm_drvdata *ddat, long *val)
 	return 0;
 }
 
+static int
+hwm_power_max_write(struct hwm_drvdata *ddat, long val)
+{
+	struct i915_hwmon *hwmon = ddat->hwmon;
+	u32 nval;
+
+	/* Computation in 64-bits to avoid overflow. Round to nearest. */
+	nval = DIV_ROUND_CLOSEST_ULL((u64)val << hwmon->scl_shift_power, SF_POWER);
+
+	hwm_locked_with_pm_intel_uncore_rmw(ddat, hwmon->rg.pkg_rapl_limit,
+					    PKG_PWR_LIM_1,
+					    REG_FIELD_PREP(PKG_PWR_LIM_1, nval));
+	return 0;
+}
+
 static int
 hwm_power_read(struct hwm_drvdata *ddat, u32 attr, int chan, long *val)
 {
@@ -425,16 +426,11 @@  hwm_power_read(struct hwm_drvdata *ddat, u32 attr, int chan, long *val)
 static int
 hwm_power_write(struct hwm_drvdata *ddat, u32 attr, int chan, long val)
 {
-	struct i915_hwmon *hwmon = ddat->hwmon;
 	u32 uval;
 
 	switch (attr) {
 	case hwmon_power_max:
-		hwm_field_scale_and_write(ddat,
-					  hwmon->rg.pkg_rapl_limit,
-					  hwmon->scl_shift_power,
-					  SF_POWER, val);
-		return 0;
+		return hwm_power_max_write(ddat, val);
 	case hwmon_power_crit:
 		uval = DIV_ROUND_CLOSEST_ULL(val << POWER_SETUP_I1_SHIFT, SF_POWER);
 		return hwm_pcode_write_i1(ddat->uncore->i915, uval);