Received-SPF: pass (google.com: domain of linux-kernel+bounces-96230-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <tencent_FF86EF51905CFBDF1102F721663984B2F105@qq.com>
Content-Type: text/plain;
	charset=us-ascii
Precedence: bulk
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3774.400.31\))
Subject: Re: [PATCH v3 5/7] riscv: Kconfig.socs: Allow SOC_CANAAN with MMU for
 K230
From: Yangyu Chen <cyy@cyyself.name>
In-Reply-To: <311bdf17-c16f-41d8-8366-10f9b00adf27@kernel.org>
Date: Fri, 8 Mar 2024 05:03:34 +0800
Cc: Conor Dooley <conor@kernel.org>,
 linux-riscv@lists.infradead.org,
 Rob Herring <robh+dt@kernel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
 Paul Walmsley <paul.walmsley@sifive.com>,
 Palmer Dabbelt <palmer@dabbelt.com>,
 Albert Ou <aou@eecs.berkeley.edu>,
 Guo Ren <guoren@kernel.org>,
 devicetree@vger.kernel.org,
 linux-kernel@vger.kernel.org,
 Conor Dooley <conor.dooley@microchip.com>
Content-Transfer-Encoding: quoted-printable
References: <tencent_BB2364BBF1812F4E304F7BDDD11E57356605@qq.com>
 <tencent_0432DA968E39B81431F921F38D747C008208@qq.com>
 <ef8df22f-dac8-4652-bf17-d10254e6abfb@kernel.org>
 <tencent_E56A833916E00EC7B4840C34FAF1250ADE0A@qq.com>
 <20240305-fascism-enrich-06483ddeb149@spud>
 <311bdf17-c16f-41d8-8366-10f9b00adf27@kernel.org>
To: Damien Le Moal <dlemoal@kernel.org>


> On Mar 6, 2024, at 07:58, Damien Le Moal <dlemoal@kernel.org> wrote:
>=20
> 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.
>>>>>=20
>>>>> 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(-)
>>>>>=20
>>>>> 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
>>>>=20
>>>> This seems wrong to me. The k210 support does require no-mmu. So =
why remove
>>>> this ?
>>>=20
>>> It just allows SOC_CANAAN to be selected when MMU=3Dy. With this =
patch,
>>> nommu_k210_defconfig still works.
>>=20
>> 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.
>=20
> Yes, this is my concern. Apologies for the lack of clarity.
>=20

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=3Dy 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_6F35FEF31908DE6AEB385AE30AC658=
863C0A@qq.com/

Thanks,
Yangyu Chen

>>=20
>> Maybe you could squash in something like the following?
>>=20
>> 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.
>>=20
>> config ARCH_CANAAN
>> - def_bool SOC_CANAAN
>> + bool "Canaan Kendryte SoCs"
>> + help
>> +   This enables support for Canaan Kendryte SoC platform hardware.
>>=20
>> 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.
>>=20
>> endmenu # "SoC selection"
>>=20
>> (Which reminds me, I really need to go and finish sorting out the =
ARCH_
>> stuff)
>=20
> --=20
> Damien Le Moal
> Western Digital Research