Re: [PATCH 1/2] LoongArch: Remove clock setting during cpu hotplug stage

[maobibo]

在 2022/7/22 16:10, WANG Xuerui 写道:
> On 2022/7/20 15:21, Bibo Mao wrote:
>> On physical machine we can save power by disabling clock of hot removed cpu.
>> However there will be problem, since different platforms have different clock
>> setting methods, the code is platform relative. Also it can be in firmware/pmu
>> compoments or cpu regulator driver, rather than general loongarch cpu booting
>> flow.
>>
>> Also on qemu virt machine, device clock/freq setting is not
>> emulated, there is no such registers.
>>
>> This patch removes hard-coded register accessing in generic
>> loongarch cpu boot flow.
> 
> Improving a little on the wording (mostly fixing eyesore Chinglish):
> 
> "On physical machine we can save power by disabling clock of hot removed cpu. However as different platforms require different methods to configure clocks, the code is platform-specific, and probably belongs to firmware/pmu or cpu regulator, rather than generic arch/loongarch code.
> 
> Also, there is no such register on QEMU virt machine since the clock/frequency regulation is not emulated.
> 
> This patch removes the hard-coded clock register accesses in generic loongarch cpu hotplug flow."
> 
Xuerui,

Thanks for reviewing my patch, I will modify the issue in next patch.

regards
bibo, mao

>>
>> Signed-off-by: Bibo Mao <maobibo@xxxxxxxxxxx>
>> ---
>>   arch/loongarch/kernel/smp.c | 29 -----------------------------
>>   include/linux/cpuhotplug.h  |  1 -
>>   2 files changed, 30 deletions(-)
>>
>> diff --git a/arch/loongarch/kernel/smp.c b/arch/loongarch/kernel/smp.c
>> index 73cec62504fb..98b3e059d344 100644
>> --- a/arch/loongarch/kernel/smp.c
>> +++ b/arch/loongarch/kernel/smp.c
>> @@ -359,35 +359,6 @@ void play_dead(void)
>>       play_dead_uncached(state_addr);
>>   }
>>   -static int loongson3_enable_clock(unsigned int cpu)
>> -{
>> -    uint64_t core_id = cpu_data[cpu].core;
>> -    uint64_t package_id = cpu_data[cpu].package;
>> -
>> -    LOONGSON_FREQCTRL(package_id) |= 1 << (core_id * 4 + 3);
>> -
>> -    return 0;
>> -}
>> -
>> -static int loongson3_disable_clock(unsigned int cpu)
>> -{
>> -    uint64_t core_id = cpu_data[cpu].core;
>> -    uint64_t package_id = cpu_data[cpu].package;
>> -
>> -    LOONGSON_FREQCTRL(package_id) &= ~(1 << (core_id * 4 + 3));
>> -
>> -    return 0;
>> -}
>> -
>> -static int register_loongson3_notifier(void)
>> -{
>> -    return cpuhp_setup_state_nocalls(CPUHP_LOONGARCH_SOC_PREPARE,
>> -                     "loongarch/loongson:prepare",
>> -                     loongson3_enable_clock,
>> -                     loongson3_disable_clock);
>> -}
>> -early_initcall(register_loongson3_notifier);
>> -
>>   #endif
>>     /*
>> diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
>> index 19f0dbfdd7fe..b66c5f389159 100644
>> --- a/include/linux/cpuhotplug.h
>> +++ b/include/linux/cpuhotplug.h
>> @@ -130,7 +130,6 @@ enum cpuhp_state {
>>       CPUHP_ZCOMP_PREPARE,
>>       CPUHP_TIMERS_PREPARE,
>>       CPUHP_MIPS_SOC_PREPARE,
>> -    CPUHP_LOONGARCH_SOC_PREPARE,
>>       CPUHP_BP_PREPARE_DYN,
>>       CPUHP_BP_PREPARE_DYN_END        = CPUHP_BP_PREPARE_DYN + 20,
>>       CPUHP_BRINGUP_CPU,
> 
> Seems good. I should have noticed earlier the fact the clock control registers are, in every regard, model-specific, thus not appropriate for arch/loongarch. Proper drivers should be added afterwards, though I assume you must internally have something like that already.
> 
> Reviewed-by: WANG Xuerui <git@xxxxxxxxxx>