Message-ID: <74973567-dd00-1480-288c-3fa3a567df3f@amlogic.com>
Date:   Thu, 27 Apr 2023 17:20:15 +0800
From:   Kelvin Zhang <kelvin.zhang@...ogic.com>
To:     neil.armstrong@...aro.org,
        Dmitry Rokosov <ddrokosov@...rdevices.ru>
Cc:     =Xianwei Zhao <xianwei.zhao@...ogic.com>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linux-amlogic@...ts.infradead.org, devicetree@...r.kernel.org,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>,
        Kevin Hilman <khilman@...libre.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>
Subject: Re: [RFC PATCH 1/2] arm64: amlogic: add new ARCH_AMLIPC for IPC SoC



On 2023/4/20 20:18, Neil Armstrong wrote:
> [ EXTERNAL EMAIL ]
> 
> Hi Kelvin,
> 
> On 20/04/2023 10:43, Kelvin Zhang wrote:
>> On 2023/4/20 01:00, Dmitry Rokosov wrote:
>>> [ EXTERNAL EMAIL ]
>>>
>>> On Wed, Apr 19, 2023 at 06:25:07PM +0200, Neil Armstrong wrote:
>>>> 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@...ogic.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@...ogic.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.
>> GXBB, GXL/GXM, G12A, G12B , SM1 and S4 belong to media box.
>> So, "MESON" represents the media box series.
>> Up to now, "MESON" works well for all existing chips except A1 and AXG.
>>>> 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.
>> Firstly, the new C series is totally different from previous MESON 
>> series.
>>  From the perspective of application, the new C series is designed for 
>> smart IP camera applications,
>> while MESON series is designed for hybrid OTT/ IP Set Top Box  and 
>> high-end media box applications.
>>  From the perspective of architecture, the new C series integrates the 
>> sensor interface, image signal processing unit, Dewarp, video encoder, 
>> neural networking processing unit,
>> which MESON series does not and will never have.
>> Secondly, there are C1 and C2 besides C3.
>> Moreover, more other series are on the way, such as T series.
>> If we always stick to "MESON", people will get more and more confused.
>> Therefore, I think it is the right time to add ARCH_AMLIPC.
> 
> Thanks for sharing such details, we are all happy to see Amlogic's
> commitment to upstream of these SoC families.
> 
> But as I explained, this ARCH_MESON doesn't define the SoC type
> but badly defines the Amlogic SoCs support.
> 
>>>> We should have named it ARCH_AMLOGIC since the beginning, but we
>>>> can't change history.
>> Shouldn't we deserve a chance to make it right?
> 
> Yes, so the right thing to do is to move to ARCH_AMLOGIC
> 
>>>>> 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, ...
>> Let's get back to ARCH_AMLIPC.
>> We just need to add ARCH_AMLIPC in the necessary subsystems 
>> Kconfig/Makefile.
>> This change will keep the existing MESON related code,  and will 
>> neither involve renaming nor break any builds.
> 
> The goal of mainline Linux is to build as much as possible and
> be modular at runtime, the only supported configuration is 
> arch/arm64/configs/defconfig
> and adding a new config to differentiate an Application type
> doesn't make sense.
> 
>>>> 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.
>> Proper names play an important role in understanding the code, right?
> 
> Yes, but stable config names also play an important role for the
> users for mainline, and there's a big number of users relying
> on the stable naming for all SoCs starting from Meson6.
> 
> So if you really want to get rid of the ARCH_MESON, migrating to
> ARCH_AMLOGIC is the right thing to do, but it involves doing
> a treewide migration and there's no easy way to do this and make
> sure the users still manages to build for other Amlogic platforms.
> 
> So as the Amlogic ARM/ARM64 SoC support maintainer I must make sure
> breakage don't happens, and so far I don't see how achieve that.
> 
> Now, you can post a RFC so we can discuss on something.

OK. Let's continue with ARCH_MESON.

> 
> Neil
> 
>>>> Neil
>>>>
>>> Thank you for the detailed explanation; it makes sense!
>>> Actually, I disagree with creating a separate ARCH without first 
>>> reworking
>>> all of its subsets - that's why I started this discussion.
>>> Now, I see that you share my perspective and believe that C3
>>> should be added to the ARCH_MESON subset, so I have no objections.
>>>
>>> [...]
>>>
>>> -- 
>>> Thank you,
>>> Dmitry
>>>
>>> _______________________________________________
>>> linux-amlogic mailing list
>>> linux-amlogic@...ts.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-amlogic
> 

Powered by blists - more mailing lists