Re: [PATCH v3 5/7] riscv: Kconfig.socs: Allow SOC_CANAAN with MMU for K230

[Yangyu Chen]

> On Mar 6, 2024, at 07:58, Damien Le Moal <dlemoal@xxxxxxxxxx> wrote:
> 
> On 3/6/24 02:20, Conor Dooley wrote:
>> On Tue, Mar 05, 2024 at 03:47:15PM +0800, Yangyu Chen wrote:
>>> On 2024/3/5 07:46, Damien Le Moal wrote:
>>>> On 3/5/24 06:05, Yangyu Chen wrote:
>>>>> Since K230 was released, SOC_CANAAN is no longer only referred to the K210.
>>>>> Remove it depends on !MMU will allow building dts for K230 and remove the
>>>>> K210 string from the help message.
>>>>> 
>>>>> Signed-off-by: Yangyu Chen <cyy@xxxxxxxxxxxx>
>>>>> Reviewed-by: Conor Dooley <conor.dooley@xxxxxxxxxxxxx>
>>>>> ---
>>>>>  arch/riscv/Kconfig.socs | 5 ++---
>>>>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>>>> 
>>>>> diff --git a/arch/riscv/Kconfig.socs b/arch/riscv/Kconfig.socs
>>>>> index 623de5f8a208..b4e9b7f75510 100644
>>>>> --- a/arch/riscv/Kconfig.socs
>>>>> +++ b/arch/riscv/Kconfig.socs
>>>>> @@ -75,13 +75,12 @@ config ARCH_CANAAN
>>>>>   def_bool SOC_CANAAN
>>>>>  config SOC_CANAAN
>>>>> - bool "Canaan Kendryte K210 SoC"
>>>>> - depends on !MMU
>>>> 
>>>> This seems wrong to me. The k210 support does require no-mmu. So why remove
>>>> this ?
>>> 
>>> It just allows SOC_CANAAN to be selected when MMU=y. With this patch,
>>> nommu_k210_defconfig still works.
>> 
>> I think the concern here is that this would allow people to build a
>> kernel for the k120 with the MMU enabled, not that the existing nommu
>> build will be affected.
> 
> Yes, this is my concern. Apologies for the lack of clarity.
> 

Hi,

Thanks for the review comments. After thinking about it for a while,
I think we don't need to change it as we have changed the help
message which deleted the "K210". And the dts on k210.dtsi shows
mmu-type is riscv.none, I think if someone noticed this would know
why it fails to boot on the S-Mode MMU Kernel on K210. The only
special thing for ARCH_CANAAN is that a loader.bin will be built
when M-Mode is on arch/riscv/Makefile. However, Canaan has no other
M-Mode chips except for K210. So I think we don't need to change
it.

Another reason is that SOC_CANAAN for K210 is somehow hard to change.
If we continue using SOC_CANAAN for K210 but not for other Canaan
SoCs such as K230, it will cause some confusion to users. If we
rename SOC_CANAAN to SOC_CANAAN_K210, it will change many drivers
in many subsystems like my patch v5 [1]. So I don't think we need
to fix it.


If we don't change it, A concern for this is that some drivers for
K210 will be built when SOC_CANAAN=y and if we add this to defconfig,
all riscv builds will also build some K210 drivers even on MMU. But
I think this will not be a problem just need some memory/storage
for a slightly bigger kernel. Also, we will enable some new configs
in defconfig when a new soc gets supported, it's normal for K210
SoC drivers.

Thus, I think we don't need to change it. If you have some other
opinions, please let me know.

[1] https://lore.kernel.org/linux-riscv/tencent_6F35FEF31908DE6AEB385AE30AC658863C0A@xxxxxx/

Thanks,
Yangyu Chen

>> 
>> Maybe you could squash in something like the following?
>> 
>> diff --git a/arch/riscv/Kconfig.socs b/arch/riscv/Kconfig.socs
>> index b4e9b7f75510..75d55059163f 100644
>> --- a/arch/riscv/Kconfig.socs
>> +++ b/arch/riscv/Kconfig.socs
>> @@ -72,15 +72,19 @@ config SOC_VIRT
>>   This enables support for QEMU Virt Machine.
>> 
>> config ARCH_CANAAN
>> - def_bool SOC_CANAAN
>> + bool "Canaan Kendryte SoCs"
>> + help
>> +   This enables support for Canaan Kendryte SoC platform hardware.
>> 
>> config SOC_CANAAN
>> - bool "Canaan Kendryte SoC"
>> + bool "Canaan Kendryte K210 SoC"
>> + depends on !MMU
>> + depends on ARCH_CANAAN
>> select CLINT_TIMER if RISCV_M_MODE
>> select ARCH_HAS_RESET_CONTROLLER
>> select PINCTRL
>> select COMMON_CLK
>> help
>> -   This enables support for Canaan Kendryte SoC platform hardware.
>> +   This enables support for Canaan Kendryte K210 SoC platform hardware.
>> 
>> endmenu # "SoC selection"
>> 
>> (Which reminds me, I really need to go and finish sorting out the ARCH_
>> stuff)
> 
> -- 
> Damien Le Moal
> Western Digital Research