[PATCH v2] drm/xe/hwmon: Fix xe_hwmon_power_max_write

[Poosa, Karthik]

On 12-06-2025 15:16, Riana Tauro wrote:
>
>
> On 6/9/2025 11:43 AM, Karthik Poosa wrote:
>> Prevent the power limit time interval from being overwritten with 0.
>> This issue was due to a missing read and modify of current power limit,
>> before setting a requested mailbox power limit, which is added in this
>> patch.
>
> Maybe prevent other bits from being overwritten or add a rmw version 
> for pcode mailbox
>
> Got confused because of power limit interval being mentioned

I shall rephrase this.

>
>>
>> v2:
>>   - Improve commit message. (Anshuman)
>>
>> Signed-off-by: Karthik Poosa <karthik.poosa at intel.com>
>> Fixes: 7596d839f6228 ("drm/xe/hwmon: Add support to manage power 
>> limits though mailbox")
>> ---
>>   drivers/gpu/drm/xe/regs/xe_mchbar_regs.h | 1 +
>>   drivers/gpu/drm/xe/xe_hwmon.c            | 7 +++++--
>>   2 files changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h 
>> b/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
>> index 5394a1373a6b..ef2bf984723f 100644
>> --- a/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
>> +++ b/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
>> @@ -40,6 +40,7 @@
>>   #define PCU_CR_PACKAGE_RAPL_LIMIT XE_REG(MCHBAR_MIRROR_BASE_SNB + 
>> 0x59a0)
>>   #define   PWR_LIM_VAL                REG_GENMASK(14, 0)
>>   #define   PWR_LIM_EN                REG_BIT(15)
>> +#define   PWR_LIM                REG_GENMASK(15, 0)
>>   #define   PWR_LIM_TIME                REG_GENMASK(23, 17)
>>   #define   PWR_LIM_TIME_X            REG_GENMASK(23, 22)
>>   #define   PWR_LIM_TIME_Y            REG_GENMASK(21, 17)
>> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c 
>> b/drivers/gpu/drm/xe/xe_hwmon.c
>> index 0d32e977537c..abb5c3693437 100644
>> --- a/drivers/gpu/drm/xe/xe_hwmon.c
>> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
>> @@ -377,11 +377,14 @@ static int xe_hwmon_power_max_write(struct 
>> xe_hwmon *hwmon, u32 attr, int channe
>>         reg_val = PWR_LIM_EN | REG_FIELD_PREP(PWR_LIM_VAL, reg_val);
>>   -    if (hwmon->xe->info.has_mbx_power_limits)
>> +    if (hwmon->xe->info.has_mbx_power_limits) {
>> +        xe_hwmon_pcode_read_power_limit(hwmon, attr, channel, &max);
>> +        reg_val = (max & ~PWR_LIM) | reg_val;
> If you re-use this in the future, maybe add a different helper function.
> Looks good for now
>
> Reviewed-by: Riana Tauro <riana.tauro at intel.com>
>
>
>>           ret = xe_hwmon_pcode_write_power_limit(hwmon, attr, 
>> channel, reg_val);
>> -    else
>> +    } else {
>>           reg_val = xe_mmio_rmw32(mmio, rapl_limit, PWR_LIM_EN | 
>> PWR_LIM_VAL,
>>                       reg_val);
>> +    }
>>   unlock:
>>       mutex_unlock(&hwmon->hwmon_lock);
>>       return ret;
>