Re: [PATCH 2/5] x86/mwait-idle: add SnowRidge C-state table

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.01.2022 11:17, Roger Pau Monné wrote:
> On Mon, Sep 06, 2021 at 03:00:46PM +0200, Jan Beulich wrote:
>> --- a/xen/arch/x86/cpu/mwait-idle.c
>> +++ b/xen/arch/x86/cpu/mwait-idle.c
>> @@ -742,6 +742,32 @@ static const struct cpuidle_state dnv_cs
>>      {}
>>  };
>>  
>> +/*
>> + * Note, depending on HW and FW revision, SnowRidge SoC may or may not 
>> support
>> + * C6, and this is indicated in the CPUID mwait leaf.
>> + */
>> +static const struct cpuidle_state snr_cstates[] = {
>> +    {
>> +            .name = "C1",
> 
> We usually use names like "C1-SNR" or similar in other cpuidle_state
> structs. Is using plain "C1" intentional here?

I don't know. We're simply following the Linux side change. If we're
unhappy with their naming (it indeed looks inconsistent), then we
ought to make a separate patch on top (and maybe submit that also to
Linux).

>> @@ -954,6 +980,11 @@ static const struct idle_cpu idle_cpu_dn
>>      .disable_promotion_to_c1e = 1,
>>  };
>>  
>> +static const struct idle_cpu idle_cpu_snr = {
>> +    .state_table = snr_cstates,
>> +    .disable_promotion_to_c1e = true,
> 
> This likely wants to be 1 because the type is bool_t.

bool_t is just an alias of bool, so "true" ought to be fine. We may
want to follow Linux in switching to bool altogether - iirc we didn't
have bool yet at the time the driver (or the first commit needing it)
was ported. I guess I'll make a patch ...

Jan