Re: [PATCH 1/2] x86/hpet: Factor hpet_enable_legacy_replacement_mode() out of hpet_setup()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 26/03/2021 09:59, Jan Beulich wrote:
> On 25.03.2021 17:52, Andrew Cooper wrote:
>> ... in preparation to introduce a second caller.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Generally
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Thanks,

> but I think there's one small code change needed plus I have two
> nits (and I expect that this change wouldn't be committed without
> patch 2, as making the function non-static isn't warranted
> otherwise):

Yeah - I intend these to go in together.

>
>> --- a/xen/arch/x86/hpet.c
>> +++ b/xen/arch/x86/hpet.c
>> @@ -754,11 +754,70 @@ int hpet_legacy_irq_tick(void)
>>  }
>>  
>>  static u32 *hpet_boot_cfg;
>> +static u64 __initdata hpet_rate;
> Use uint64_t as you move this here?

Ok.

>
>> +bool __init hpet_enable_legacy_replacement_mode(void)
>> +{
>> +    unsigned int id, cfg, c0_cfg, ticks, count;
>> +
>> +    if ( !hpet_rate ||
> I think you need to also honor opt_hpet here.

Can't (order of patches), and also no need.

When opt_hpet is introduced, hpet_rate can't become nonzero without
opt_hpet being set.

>
>> +         !((id = hpet_read32(HPET_ID)) & HPET_ID_LEGSUP) ||
> I don't think I see a need for the assignment and hence the local
> variable. Dropping it would make the code easier to read imo.

Ok.

~Andrew