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

[Konrad Dybcio]

On 3/26/26 6:47 PM, Akhil P Oommen wrote:
> 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.

These are two fully equivalent ways of representing the same data.
The compiler won't reorder them, since data in structs is.. stuctured..

In any case, either of them is fine. The struct definition would be more
convenient to work with if the data was provided in C code, whereas I can
see the simple-u32 being more convenient if it's going to be pulled from
the DTS

Konrad