Re: [RFC PATCH 1/2] arm64: amlogic: add new ARCH_AMLIPC for IPC SoC

From: Neil Armstrong <neil.armstrong@linaro.org>
To: Dmitry Rokosov <ddrokosov@sberdevices.ru>
Cc: =Xianwei Zhao <xianwei.zhao@amlogic.com>,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, linux-amlogic@lists.infradead.org,
	devicetree@vger.kernel.org,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>,
	Kevin Hilman <khilman@baylibre.com>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
Subject: Re: [RFC PATCH 1/2] arm64: amlogic: add new ARCH_AMLIPC for IPC SoC
Date: Wed, 19 Apr 2023 18:25:07 +0200	[thread overview]
Message-ID: <427e79ef-156d-027e-9296-6f4e6513a04d@linaro.org> (raw)
In-Reply-To: <20230419160405.d7qfir3nv6tlxx2a@CAB-WSD-L081021>

On 19/04/2023 18:04, Dmitry Rokosov wrote:
> On Wed, Apr 19, 2023 at 03:43:12PM +0200, Neil Armstrong wrote:
>> On 19/04/2023 15:14, Dmitry Rokosov wrote:
>>> On Wed, Apr 19, 2023 at 03:38:33PM +0800, =Xianwei Zhao wrote:
>>>> From: Xianwei Zhao <xianwei.zhao@amlogic.com>
>>>>
>>>> The C series SoCs are designed for smart IP camera
>>>> applications, which does not belong to Meson series.
>>>> So, Add ARCH_AMLIPC for the new series.
>>>>
>>>> There are now multiple amlogic SoC seies supported, so group them under
>>>> their own menu. we can easily add new platforms there in the future.
>>>> Introduce ARCH_AMLOGIC to cover all Amlogic SoC series.
>>>>
>>>> No functional changes introduced.
>>>>
>>>> Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com>
>>>> ---
>>>>    arch/arm64/Kconfig.platforms | 12 ++++++++++++
>>>>    arch/arm64/configs/defconfig |  2 ++
>>>>    2 files changed, 14 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
>>>> index 89a0b13b058d..bfbc817eef8f 100644
>>>> --- a/arch/arm64/Kconfig.platforms
>>>> +++ b/arch/arm64/Kconfig.platforms
>>>> @@ -162,12 +162,24 @@ config ARCH_MEDIATEK
>>>>    	  This enables support for MediaTek MT27xx, MT65xx, MT76xx
>>>>    	  & MT81xx ARMv8 SoCs
>>>> +menuconfig ARCH_AMLOGIC
>>>> +	bool "NXP SoC support"
>>>
>>> NXP? Did you mean "Amlogic"?
>>>
>>>> +
>>>> +if ARCH_AMLOGIC
>>>> +
>>>>    config ARCH_MESON
>>>>    	bool "Amlogic Platforms"
>>>>    	help
>>>>    	  This enables support for the arm64 based Amlogic SoCs
>>>>    	  such as the s905, S905X/D, S912, A113X/D or S905X/D2
>>>> +config ARCH_AMLIPC
>>>
>>> Do we really need a different ARCH for Amlogic IPC?
>>> I can imagine that it's not the Meson architecture at all.
>>> But maybe a better solution is just to rename ARCH_MESON to ARCH_AMLOGIC?
>>
>> It should be changed treewide, and is it worth it ?
> 
> As far as I understand, the A1 and S4 families are not fully compatible
> with the Meson architecture, and we haven't provided additional ARCH_*
> for them.

The GXBB, GXL/GXM, G12A, G12B & SM1 are also not fully compatible,
but they lie under the "MESON" umbrella which covers SoC since the
Meson6 architecture. It's a facility to include/exclude Amlogic
drivers/DT, nothing else.

If you compare it to BCM or NXP, it's different situation, the
different ARCH_* actually targets totally different SoCs from
completely different Business Units or from companies acquisitions.

We should have named it ARCH_AMLOGIC since the beginning, but we
can't change history.

> In my opinion, it's a good time to split the Meson architecture into
> proper subsets, or rename it treewide (maybe only config option
> ARCH_MESON => ARCH_AMLOGIC).

MESON is only a codename to differentiate from other SoC vendors
because Amlogic used it as a codename for a long time.
Compare this to Allwinner's "sunxi" or Qualcomm's "msm".

This config has no functional mean, it's only a config namespace.

Renaming it would need renaming it in all subsystems Kconfig/Makefiles
and will certainly break builds with custom kernel configs
in various publicly used builds like Armbian, meta-meson, LibreELEC,
Debian, Suse, ...

So it's pointless to change, and even add a different one since
it's not a family differentiator since the Kernel is modular
and works around DT to determine which drivers to probe.

Neil

> 
>>>
>>>> +	bool "Amlogic IPC Platforms"
>>>> +	help
>>>> +	  This enables support for the arm64 based Amlogic IPC SoCs
>>>> +	  such as the C302X, C308L
>>>> +endif
>>>> +
>>>>    config ARCH_MVEBU
>>>>    	bool "Marvell EBU SoC Family"
>>>>    	select ARMADA_AP806_SYSCON
>>>> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
>>>> index 7790ee42c68a..f231bd1723fd 100644
>>>> --- a/arch/arm64/configs/defconfig
>>>> +++ b/arch/arm64/configs/defconfig
>>>> @@ -46,7 +46,9 @@ CONFIG_ARCH_LG1K=y
>>>>    CONFIG_ARCH_HISI=y
>>>>    CONFIG_ARCH_KEEMBAY=y
>>>>    CONFIG_ARCH_MEDIATEK=y
>>>> +CONFIG_ARCH_AMLOGIC=y
>>>>    CONFIG_ARCH_MESON=y
>>>> +CONFIG_ARCH_AMLIPC=y
>>>>    CONFIG_ARCH_MVEBU=y
>>>>    CONFIG_ARCH_NXP=y
>>>>    CONFIG_ARCH_LAYERSCAPE=y
>>>> -- 
>>>> 2.37.1
>>>>
>>>>
>>>> _______________________________________________
>>>> linux-amlogic mailing list
>>>> linux-amlogic@lists.infradead.org
>>>> http://lists.infradead.org/mailman/listinfo/linux-amlogic
>>>
>>
> 