Re: [Intel-xe] [PATCH v2 1/2] drm/xe: Add a couple of pcode helpers

["Sundaresan, Sujaritha" <sujaritha.sundaresan@intel.com>]

On 9/26/2023 10:58 PM, Rodrigo Vivi wrote:
> On Tue, Sep 26, 2023 at 08:11:19PM +0530, Sujaritha Sundaresan wrote:
>> Some pcode commands take additional sub-commands and parameters. Add a
>> couple of helpers to help formatting these commands to improve code
>> readability.
>>
>> v2: change function name (Rodrigo)
>>
>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_pcode.c | 28 ++++++++++++++++++++++++++++
>>   drivers/gpu/drm/xe/xe_pcode.h |  3 +++
>>   2 files changed, 31 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_pcode.c b/drivers/gpu/drm/xe/xe_pcode.c
>> index 7f1bf2297f51..eed096ffc23c 100644
>> --- a/drivers/gpu/drm/xe/xe_pcode.c
>> +++ b/drivers/gpu/drm/xe/xe_pcode.c
>> @@ -104,6 +104,34 @@ int xe_pcode_read(struct xe_gt *gt, u32 mbox, u32 *val, u32 *val1)
>>   	return err;
>>   }
>>   
>> +int xe_gt_pcode_read(struct xe_gt *gt, u32 mbcmd, u32 p1, u32 p2, u32 *val)
>> +{
>> +	u32 mbox;
>> +	int err;
>> +
>> +	mbox = REG_FIELD_PREP(PCODE_MB_COMMAND, mbcmd)
>> +		| REG_FIELD_PREP(PCODE_MB_PARAM1, p1)
>> +		| REG_FIELD_PREP(PCODE_MB_PARAM2, p2);
>> +
>> +	err = xe_pcode_read(gt, mbox, val, NULL);
>> +
>> +	return err;
>> +}
>> +
>> +int xe_gt_pcode_write(struct xe_gt *gt, u32 mbcmd, u32 p1, u32 p2, u32 val)
>> +{
>> +	u32 mbox;
>> +	int err;
>> +
>> +	mbox = REG_FIELD_PREP(PCODE_MB_COMMAND, mbcmd)
>> +		| REG_FIELD_PREP(PCODE_MB_PARAM1, p1)
>> +		| REG_FIELD_PREP(PCODE_MB_PARAM2, p2);
>> +
>> +	err = xe_pcode_write(gt, mbox, val);
>> +
>> +	return err;
>> +}
>> +
>>   static int xe_pcode_try_request(struct xe_gt *gt, u32 mbox,
>>   				u32 request, u32 reply_mask, u32 reply,
>>   				u32 *status, bool atomic, int timeout_us)
>> diff --git a/drivers/gpu/drm/xe/xe_pcode.h b/drivers/gpu/drm/xe/xe_pcode.h
>> index 3b4aa8c1a3ba..79ae45e71228 100644
>> --- a/drivers/gpu/drm/xe/xe_pcode.h
>> +++ b/drivers/gpu/drm/xe/xe_pcode.h
>> @@ -19,6 +19,9 @@ int xe_pcode_write_timeout(struct xe_gt *gt, u32 mbox, u32 val,
>>   #define xe_pcode_write(gt, mbox, val) \
>>   	xe_pcode_write_timeout(gt, mbox, val, 1)
>>   
>> +int xe_gt_pcode_read(struct xe_gt *gt, u32 mbcmd, u32 p1, u32 p2, u32 *val);
>> +int xe_gt_pcode_write(struct xe_gt *gt, u32 mbcmd, u32 p1, u32 p2, u32 val);
> A few things that are wrong here:
>
> 1. namespace: xe_pcode is the name of the component. That should be in the prefix.
> Please notice the xe_mmio for instance on the tile option vs gt... 'tile' comes
> after the xe_mmio, not before.
>
> 2. 'gt' is already the main argument in the other xe_pcode functions, why are
> you adding that as prefix?
>
> 3. Please unify the read and write functions, without creating new ones.
> Please take a look to hwmon series there... There's a need for something
> different there, and that solution from Badal is better than adding new
> signatures. But please, let's combine them all.

I think I'll just skip the functions and have the mbox be defined in the 
call itself.

Thanks,

Suja

>
>> +
>>   int xe_pcode_request(struct xe_gt *gt, u32 mbox, u32 request,
>>   		     u32 reply_mask, u32 reply, int timeout_ms);
>>   
>> -- 
>> 2.25.1
>>