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

Yangyu Chen
2024-03-07T21:04:18Z

> On Mar 6, 2024, at 07:58, Damien Le Moal <dlemoal@kernel.org> 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@cyyself.name>
>>>>> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
>>>>> ---
>>>>> 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@qq.com/

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