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

[Yangyu Chen <cyy@cyyself.name>]

On 2024/3/8 05:03, Yangyu Chen wrote:
> 
> 
>> 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
> 

Hi Damien,

I'm waiting for your decision before sending the next patch revision.
Please reply to this when you are free.

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