Message-ID: <a253f338-c162-17b8-f412-95cd63195151@collabora.com>
Date:   Thu, 19 May 2022 10:49:21 +0200
From:   AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>
To:     Chen-Yu Tsai <wenst@...omium.org>
Cc:     Miles Chen <miles.chen@...iatek.com>, bgolaszewski@...libre.com,
        chun-jie.chen@...iatek.com, ck.hu@...iatek.com,
        devicetree@...r.kernel.org, fparent@...libre.com,
        ikjn@...omium.org, jason-jh.lin@...iatek.com, kernel@...labora.com,
        konrad.dybcio@...ainline.org, krzysztof.kozlowski+dt@...aro.org,
        linux-arm-kernel@...ts.infradead.org, linux-clk@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-mediatek@...ts.infradead.org,
        marijn.suijten@...ainline.org, martin.botka@...ainline.org,
        matthias.bgg@...il.com, mturquette@...libre.com,
        p.zabel@...gutronix.de, paul.bouchara@...ainline.org,
        phone-devel@...r.kernel.org, rex-bc.chen@...iatek.com,
        robh+dt@...nel.org, sam.shih@...iatek.com, sboyd@...nel.org,
        tinghan.shen@...iatek.com, weiyi.lu@...iatek.com,
        y.oudjana@...tonmail.com, ~postmarketos/upstreaming@...ts.sr.ht
Subject: Re: [PATCH v2 7/7] clk: mediatek: Add MediaTek Helio X10 MT6795 clock
 drivers

Il 19/05/22 10:37, Chen-Yu Tsai ha scritto:
> On Thu, May 19, 2022 at 4:17 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@...labora.com> wrote:
>>
>> Il 19/05/22 06:53, Miles Chen ha scritto:
>>>
>>> Hi Angelo,
>>>
>>>> Add the clock drivers for the entire clock tree of MediaTek Helio X10
>>>> MT6795, including system clocks (apmixedsys, infracfg, pericfg, topckgen)
>>>> and multimedia clocks (mmsys, mfg, vdecsys, vencsys).
>>>>
>>>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
>>>> ---
>>>> drivers/clk/mediatek/Kconfig                 |  37 ++
>>>> drivers/clk/mediatek/Makefile                |   6 +
>>>> drivers/clk/mediatek/clk-mt6795-apmixedsys.c | 157 +++++
>>>> drivers/clk/mediatek/clk-mt6795-infracfg.c   | 148 +++++
>>>> drivers/clk/mediatek/clk-mt6795-mfg.c        |  50 ++
>>>> drivers/clk/mediatek/clk-mt6795-mm.c         | 106 ++++
>>>> drivers/clk/mediatek/clk-mt6795-pericfg.c    | 160 +++++
>>>> drivers/clk/mediatek/clk-mt6795-topckgen.c   | 611 +++++++++++++++++++
>>>> drivers/clk/mediatek/clk-mt6795-vdecsys.c    |  55 ++
>>>> drivers/clk/mediatek/clk-mt6795-vencsys.c    |  50 ++
>>>> 10 files changed, 1380 insertions(+)
>>>> create mode 100644 drivers/clk/mediatek/clk-mt6795-apmixedsys.c
>>>> create mode 100644 drivers/clk/mediatek/clk-mt6795-infracfg.c
>>>> create mode 100644 drivers/clk/mediatek/clk-mt6795-mfg.c
>>>> create mode 100644 drivers/clk/mediatek/clk-mt6795-mm.c
>>>> create mode 100644 drivers/clk/mediatek/clk-mt6795-pericfg.c
>>>> create mode 100644 drivers/clk/mediatek/clk-mt6795-topckgen.c
>>>> create mode 100644 drivers/clk/mediatek/clk-mt6795-vdecsys.c
>>>> create mode 100644 drivers/clk/mediatek/clk-mt6795-vencsys.c
>>>>
>>>> diff --git a/drivers/clk/mediatek/Kconfig b/drivers/clk/mediatek/Kconfig
>>>> index d5936cfb3bee..da8142dff3c3 100644
>>>> --- a/drivers/clk/mediatek/Kconfig
>>>> +++ b/drivers/clk/mediatek/Kconfig
>>>> @@ -259,6 +259,43 @@ config COMMON_CLK_MT6779_AUDSYS
>>>>       help
>>>>         This driver supports Mediatek MT6779 audsys clocks.
>>>>
>>>> +config COMMON_CLK_MT6795
>>>> +    tristate "Clock driver for MediaTek MT6795"
>>>> +    depends on ARCH_MEDIATEK || COMPILE_TEST
>>>> +    select COMMON_CLK_MEDIATEK
>>>> +    default ARCH_MEDIATEK
>>>> +    help
>>>> +      This driver supports MediaTek MT6795 basic clocks and clocks
>>>> +      required for various peripherals found on MediaTek.
>>>
>>> Thanks for doing this, I was wondering if we can use only COMMON_CLK_MT6795 to build all
>>> clk-mt6795-*? like CONFIG_COMMON_CLK_MT8195 style:
>>>
>>> obj-$(CONFIG_COMMON_CLK_MT8195) += clk-mt8195-apmixedsys.o clk-mt8195-topckgen.o \
>>>                                   clk-mt8195-peri_ao.o clk-mt8195-infra_ao.o \
>>>                                   clk-mt8195-cam.o clk-mt8195-ccu.o clk-mt8195-img.o \
>>>                                   clk-mt8195-ipe.o clk-mt8195-mfg.o clk-mt8195-scp_adsp.o \
>>>                                   clk-mt8195-vdec.o clk-mt8195-vdo0.o clk-mt8195-vdo1.o \
>>>                                   clk-mt8195-venc.o clk-mt8195-vpp0.o clk-mt8195-vpp1.o \
>>>                                   clk-mt8195-wpe.o clk-mt8195-imp_iic_wrap.o \
>>>                                   clk-mt8195-apusys_pll.o
>>>
>>> So we do not have to keep other COMMON_CLK_MT6795_* configs.
>>>
>>
>> I don't think that this would bring any benefit - it's the opposite, if anything!
>>
>> Think about platforms that don't need any ISP functionality, or are headless (hence
>> not requiring anything for dsi/hdmi/dp and display and/or media generally): what
>> I've done is splitting the clock drivers that are critical for any functionality
>> of the SoC to the ones that are enabling "facultative" functionality.
>>
>> Hence, the usecases for this kind of splitting are:
>> 1. Somewhat rare (corner) cases: someone may not want to compile in any of the
>>      mm/venc/vdec/mfg clock drivers because they don't need the functionality at
>>      all (probably, including the other related drivers), or;
>> 2. It would be possible to compile as built-in only the "main" drivers (apmixed,
>>      infra, peri, topck) to achieve a boot (ex.: you need eMMC to boot, at least)
>>      and then compile the mm/venc/vdec/mfg as modules to be loaded after mounting
>>      a rootfs (where you probably also have mediatek-drm, vcodec, etc as modules).
> 
> I assume you mean split them into two groups:
> 
>    - essential for booting to a state capable of loading modules from storage
>      So apmixedsys + topckgen + infra_ao + peri_ao + imp_iic_wrap (maybe?)
>    - everything else
> 
> The whole MT8195 clock driver suite is roughly 70KB, with half of that for
> the four essential clock drivers.
> 
> If the system is really that limited in storage, you probably are already
> running a highly customized config and build system. And kind of doesn't
> make sense that that is a real limitation for the larger chips like the
> MT8195.
> 
> IMO having two Kconfig symbols for one chip is still much better than
> having ten though.
> 

For MT8195... and 92, 83, 73... and others from the same era, being them for
chromebooks, iot, smartphones and whatever else... yeah you're totally right.

The issue starts raising when looking at older SoCs featuring an older bootloader
that does have a kernel size limitation; for example, to make the loader happy on
MT6795, I had to strip the defconfig a lot and keep the Android-style boot.img
smaller than 10MB (that's Image.gz-dtb + ramdisk).

As for everything else, including newer platforms like MT8195...
I'm nitpicking now, but being able to have a smaller kernel image means less time
spent on loading from storage and less decompression time: but that specific one
would be a broad discussion with too many variables to consider, so... again,
read that as something very nitpicking and nothing else :-)

>> For this reason, I would propose to actually split the MT8195 clocks as well
>> and the ones for other models to achieve what I explained so that, in the future,
>> when this entire framework will fully support (read: fully tested) modularity,
>> we will be able to set these to compile as module by default, which would greatly
>> reduce the kernel size.
>> That's important, not only for MediaTek SoCs, but also for others (nxp, qcom, etc)
>> that don't need MediaTek clock drivers at all, since upstream we build one kernel
>> image for all, and not specialized images.
> 
> AFAIK the upstream defconfig was never intended for end users to directly
> consume. It is only there for all the automated testing. End users should
> either generate their own config, or have their distros come up with
> something sane, like building everything as modules. The soc maintainers
> (if not Linus) were quite clear about this.
> 

I appreciate the reminder, but I think that CIs are already able to ship modules
during testing, and for compile-test purposes, it would probably even make more
sense to compile as module (as that's where things sometimes start failing: see
what we're doing here...), but I'm not arguing anyway.

Cheers,
Angelo

Powered by blists - more mailing lists