Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <d98399c3-738c-e514-85e1-b278dc813cb9@loongson.cn>
Date:   Mon, 25 Jul 2022 11:16:38 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.0
Subject: Re: [PATCH 1/2] LoongArch: Remove clock setting during cpu hotplug
 stage
Content-Language: en-US
To:     WANG Xuerui <kernel@xen0n.name>,
        Huacai Chen <chenhuacai@kernel.org>
Cc:     Jiaxun Yang <jiaxun.yang@flygoat.com>, loongarch@lists.linux.dev,
        linux-kernel@vger.kernel.org
References: <20220720072152.3894559-1-maobibo@loongson.cn>
 <3324feed-e158-8c51-9915-85b0498f1888@xen0n.name>
From:   maobibo <maobibo@loongson.cn>
In-Reply-To: <3324feed-e158-8c51-9915-85b0498f1888@xen0n.name>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk


在 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@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>