Hi Ashutosh,
On 27-09-2023 10:15, Dixit, Ashutosh wrote:
On Mon, 25 Sep 2023 01:18:38 -0700, Badal Nilawar wrote:
Hi Badal,
+static int xe_hwmon_process_reg(struct xe_hwmon *hwmon, enum xe_hwmon_reg hwmon_reg,
Maybe xe_hwmon_read_write_reg? process_reg sounds bad. Basically we don't
process a register, we read or write it.
I don't think it sound that bad. When we say process register apart from
read/write/rmw what else we will be doing. I think lets not rename this
function.
+ enum xe_hwmon_reg_operation operation, u32 *value,
+ u32 clr, u32 set)
+{
+ struct xe_reg reg;
+
+ reg.raw = xe_hwmon_get_reg(hwmon, hwmon_reg);
+
+ if (!reg.raw)
+ return -EOPNOTSUPP;
+
+ switch (operation) {
+ case REG_READ:
+ *value = xe_mmio_read32(hwmon->gt, reg);
+ return 0;
+ case REG_WRITE:
+ xe_mmio_write32(hwmon->gt, reg, *value);
+ return 0;
+ case REG_RMW:
+ *value = xe_mmio_rmw32(hwmon->gt, reg, clr, set);
+ return 0;
+ default:
+ drm_warn(>_to_xe(hwmon->gt)->drm, "Invalid xe hwmon reg operation: %d\n",
+ operation);
+ return -EOPNOTSUPP;
+ }
+}
+
+int xe_hwmon_process_reg_read64(struct xe_hwmon *hwmon, enum xe_hwmon_reg hwmon_reg, u64 *value)
+{
+ struct xe_reg reg;
+
+ reg.raw = xe_hwmon_get_reg(hwmon, hwmon_reg);
+
+ if (!reg.raw)
+ return -EOPNOTSUPP;
+
+ *value = xe_mmio_read64_2x32(hwmon->gt, reg);
+
+ return 0;
We can't make read64 part of enum xe_hwmon_reg_operation?
read64 takes argument "u64 *value" so kept it separate.
+}
+
+#define PL1_DISABLE 0
+
+/*
+ * HW allows arbitrary PL1 limits to be set but silently clamps these values to
+ * "typical but not guaranteed" min/max values in REG_PKG_POWER_SKU. Follow the
+ * same pattern for sysfs, allow arbitrary PL1 limits to be set but display
+ * clamped values when read.
+ */
+static int xe_hwmon_power_max_read(struct xe_hwmon *hwmon, long *value)
+{
+ u32 reg_val;
+ u64 reg_val64, min, max;
+
+ xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_READ, ®_val, 0, 0);
+ /* Check if PL1 limit is disabled */
+ if (!(reg_val & PKG_PWR_LIM_1_EN)) {
+ *value = PL1_DISABLE;
+ return 0;
+ }
+
+ 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_read64(hwmon, REG_PKG_POWER_SKU, ®_val64);
+ min = REG_FIELD_GET(PKG_MIN_PWR, reg_val64);
+ min = mul_u64_u32_shr(min, SF_POWER, hwmon->scl_shift_power);
+ max = REG_FIELD_GET(PKG_MAX_PWR, reg_val64);
+ max = mul_u64_u32_shr(max, SF_POWER, hwmon->scl_shift_power);
+
+ if (min && max)
+ *value = clamp_t(u64, *value, min, max);
Not exactly correct. Should be:
if (min)
clamp at min
if (max)
clamp at max
I was thinking of changing it for i915 but was lazy.
Sure, thanks for pointing this.
+
+ return 0;
+}
+
+static int xe_hwmon_power_max_write(struct xe_hwmon *hwmon, long value)
+{
+ u32 reg_val;
+
+ /* 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_RMW, ®_val,
+ PKG_PWR_LIM_1_EN, 0);
+ xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_READ, ®_val,
If we are not checking for return codes from these functions, why are they
not void?
Top level functions expect return. For function xe_hwmon_power_max_write
returning error if PL1 disable not possible. The functions
xe_hwmon_power_max_read/xe_hwmon_power_rated_max_read can be made void,
then it will look like. What difference its going to make? I feel
existing approach is much readable.
case hwmon_power_max:
xe_hwmon_power_max_read(hwmon, val);
return 0;
case hwmon_power_rated_max:
xe_hwmon_power_rated_max_read(hwmon, val);
return 0;
Also, how about separate read/write/rmw functions as Andi was suggesting?
They would be clearer I think.
Would not prefer to add further abstraction, lets keep as is. Going
further while adding new platforms will think about adding it.
Regards,
Badal
Thanks.
--
Ashutosh
