Re: [PATCH 08/16] drm/msm/a6xx: Update HFI definitions

[Akhil P Oommen]

On 3/24/2026 3:30 PM, Konrad Dybcio wrote:
> On 3/23/26 9:12 PM, Akhil P Oommen wrote:
>> Update the HFI definitions to support additional GMU based power
>> features.
>>
>> Signed-off-by: Akhil P Oommen <akhilpo@xxxxxxxxxxxxxxxx>
>> ---
> 
> [...]
> 
>> +#define CLX_DATA(irated, num_phases, clx_path, extd_intf) \
>> +	((extd_intf << 29) |				  \
>> +	 (clx_path << 28) |				  \
>> +	 (num_phases << 22) |				  \
>> +	 (irated << 16))
>> +
>> +struct a6xx_hfi_clx_domain_v2 {
>> +	/**
>> +	 * @data: BITS[0:15]  Migration time
>> +	 *        BITS[16:21] Current rating
>> +	 *        BITS[22:27] Phases for domain
>> +	 *        BITS[28:28] Path notification
>> +	 *        BITS[29:31] Extra features
>> +	 */
> 
> My first thought would be to define these as bitfields, i.e.
> u16 mitigation_time
> u8 current_rating : 6;
> u8 num_phases : 6;
> u8 path_notification : 1;
> u8 extra_features : 3;
> 

I am unsure if the compiler would mess with the ordering. Aside from the
fact that this is an ABI with the firmware, what make this piece of data
very risk is that, these are power related configurations where the
issues due to misconfiguration won't be immediately obvious.

-Akhil.

> (hopefully I counted them right)
> 
> You would then not need custom macros to set/get the data
> 
>> +	u32 data;
>> +	/** @clxt: CLX time in microseconds */
> 
> slash-star-star implies kerneldoc, which this isn't - but it would
> be appreciated, see e.g. struct qcom_icc_provider in
> drivers/interconnect/qcom/icc-rpmh.h
> 
> Konrad