[PATCH v2 1/1] drm/xe/hwmon: Remove xe_hwmon_process_reg

[Lucas De Marchi]

On Thu, May 23, 2024 at 11:55:40PM GMT, Karthik Poosa wrote:
>Remove xe_hwmon_process_reg as it is a umbrella function which can be
>avoided (Lucas).
>
>v2: Improve commit message (Badal).
>
>Signed-off-by: Karthik Poosa <karthik.poosa at intel.com>
>Suggested-by: Lucas De Marchi <lucas.demarchi at intel.com>
>Cc: Badal Nilawar <badal.nilawar at intel.com>
>---
> drivers/gpu/drm/xe/xe_hwmon.c | 80 ++++++++++++++---------------------
> 1 file changed, 31 insertions(+), 49 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
>index dca275117232..4d8ee9cac068 100644
>--- a/drivers/gpu/drm/xe/xe_hwmon.c
>+++ b/drivers/gpu/drm/xe/xe_hwmon.c
>@@ -121,34 +121,6 @@ static struct xe_reg xe_hwmon_get_reg(struct xe_hwmon *hwmon, enum xe_hwmon_reg
> 	return XE_REG(0);
> }
>
>-static void xe_hwmon_process_reg(struct xe_hwmon *hwmon, enum xe_hwmon_reg hwmon_reg,
>-				 enum xe_hwmon_reg_operation operation, u64 *value,
>-				 u32 clr, u32 set, int channel)
>-{
>-	struct xe_reg reg;
>-
>-	reg = xe_hwmon_get_reg(hwmon, hwmon_reg, channel);
>-
>-	if (!xe_reg_is_valid(reg))
>-		return;
>-
>-	switch (operation) {
>-	case REG_READ32:
>-		*value = xe_mmio_read32(hwmon->gt, reg);
>-		break;
>-	case REG_RMW32:
>-		*value = xe_mmio_rmw32(hwmon->gt, reg, clr, set);
>-		break;
>-	case REG_READ64:
>-		*value = xe_mmio_read64_2x32(hwmon->gt, reg);
>-		break;
>-	default:
>-		drm_warn(&gt_to_xe(hwmon->gt)->drm, "Invalid xe hwmon reg operation: %d\n",
>-			 operation);
>-		break;
>-	}
>-}
>-
> #define PL1_DISABLE 0
>
> /*
>@@ -160,10 +132,21 @@ static void xe_hwmon_process_reg(struct xe_hwmon *hwmon, enum xe_hwmon_reg hwmon
> static void xe_hwmon_power_max_read(struct xe_hwmon *hwmon, int channel, long *value)
> {
> 	u64 reg_val, min, max;
>+	struct xe_device *xe = gt_to_xe(hwmon->gt);
>+	struct xe_reg rapl_limit, pkg_power_sku;
>+
>+	rapl_limit = xe_hwmon_get_reg(hwmon, REG_PKG_RAPL_LIMIT, channel);
>+	pkg_power_sku = xe_hwmon_get_reg(hwmon, REG_PKG_POWER_SKU, channel);
>+
>+	if (!xe_reg_is_valid(pkg_power_sku)) {

shouldn´t you also check validity of REG_PKG_RAPL_LIMIT?

>+		drm_warn(&xe->drm, "pkg_power_sku invalid\n");
>+		*value = 0;
>+		return;
>+	}
>
> 	mutex_lock(&hwmon->hwmon_lock);
>
>-	xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_READ32, &reg_val, 0, 0, channel);
>+	reg_val = xe_mmio_read32(hwmon->gt, rapl_limit);
> 	/* Check if PL1 limit is disabled */
> 	if (!(reg_val & PKG_PWR_LIM_1_EN)) {
> 		*value = PL1_DISABLE;
>@@ -173,7 +156,7 @@ static void xe_hwmon_power_max_read(struct xe_hwmon *hwmon, int channel, long *v
> 	reg_val = REG_FIELD_GET(PKG_PWR_LIM_1, reg_val);
> 	*value = mul_u64_u32_shr(reg_val, SF_POWER, hwmon->scl_shift_power);
>
>-	xe_hwmon_process_reg(hwmon, REG_PKG_POWER_SKU, REG_READ64, &reg_val, 0, 0, channel);
>+	reg_val = xe_mmio_read64_2x32(hwmon->gt, pkg_power_sku);
> 	min = REG_FIELD_GET(PKG_MIN_PWR, reg_val);
> 	min = mul_u64_u32_shr(min, SF_POWER, hwmon->scl_shift_power);
> 	max = REG_FIELD_GET(PKG_MAX_PWR, reg_val);
>@@ -189,16 +172,16 @@ static int xe_hwmon_power_max_write(struct xe_hwmon *hwmon, int channel, long va
> {
> 	int ret = 0;
> 	u64 reg_val;
>+	struct xe_reg rapl_limit;
>+
>+	rapl_limit = xe_hwmon_get_reg(hwmon, REG_PKG_RAPL_LIMIT, channel);
>
> 	mutex_lock(&hwmon->hwmon_lock);
>
> 	/* Disable PL1 limit and verify, as limit cannot be disabled on all platforms */
> 	if (value == PL1_DISABLE) {
>-		xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_RMW32, &reg_val,
>-				     PKG_PWR_LIM_1_EN, 0, channel);
>-		xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_READ32, &reg_val,
>-				     PKG_PWR_LIM_1_EN, 0, channel);
>-
>+		reg_val = xe_mmio_rmw32(hwmon->gt, rapl_limit, PKG_PWR_LIM_1_EN, 0);
>+		reg_val = xe_mmio_read32(hwmon->gt, rapl_limit);
> 		if (reg_val & PKG_PWR_LIM_1_EN) {
> 			ret = -EOPNOTSUPP;
> 			goto unlock;
>@@ -208,9 +191,8 @@ static int xe_hwmon_power_max_write(struct xe_hwmon *hwmon, int channel, long va
> 	/* Computation in 64-bits to avoid overflow. Round to nearest. */
> 	reg_val = DIV_ROUND_CLOSEST_ULL((u64)value << hwmon->scl_shift_power, SF_POWER);
> 	reg_val = PKG_PWR_LIM_1_EN | REG_FIELD_PREP(PKG_PWR_LIM_1, reg_val);
>+	reg_val = xe_mmio_rmw32(hwmon->gt, rapl_limit, PKG_PWR_LIM_1_EN | PKG_PWR_LIM_1, reg_val);
>
>-	xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_RMW32, &reg_val,
>-			     PKG_PWR_LIM_1_EN | PKG_PWR_LIM_1, reg_val, channel);
> unlock:
> 	mutex_unlock(&hwmon->hwmon_lock);
> 	return ret;
>@@ -220,7 +202,8 @@ static void xe_hwmon_power_rated_max_read(struct xe_hwmon *hwmon, int channel, l
> {
> 	u64 reg_val;
>
>-	xe_hwmon_process_reg(hwmon, REG_PKG_POWER_SKU, REG_READ32, &reg_val, 0, 0, channel);
>+	/* REG_PKG_POWER_SKU validity is checked in visible, we need not check again here */
>+	reg_val = xe_mmio_read32(hwmon->gt, xe_hwmon_get_reg(hwmon, REG_PKG_POWER_SKU, channel));

what do you mean by "is checked in visible"? Is this about the visible()
op?  Maybe a better comment would be:

	/*
	 * This sysfs file won't be visible if REG_PKG_POWER_SKU is
	 * invalid so the check can be skipped. See <function name doing
	 * the check>
	 */

in any case, I think it's clearer to move the get_reg() outside and have
something like:

	struct xe_reg reg = xe_hwmon_get_reg(hwmon, REG_PKG_POWER_SKU, channel);
	u64 reg_val;

	reg_val = xe_mmio_read32(hwmon->gt, reg);
	reg_val = REG_FIELD_GET(PKG_TDP, reg_val);
	*value = mul_u64_u32_shr(reg_val, SF_POWER, hwmon->scl_shift_power);

rest looks good and those things above are more nits than anything.
If you are going to change, then respin once more. In any case:


Reviewed-by: Lucas De Marchi <lucas.demarchi at intel.com>


thanks
Lucas De Marchi