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

[WANG Xuerui <kernel@xen0n.name>]

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."

>
> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
> ---
>   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@xen0n.name>