Re: [PATCH] [RFC] clk: sprd: fix compile-testing

[Chunyan Zhang]

On Fri, 10 Apr 2020 at 04:17, Sandeep Patil <sspatil@xxxxxxxxxxx> wrote:
>
>
>
> On Wed, Apr 8, 2020 at 11:09 PM Chunyan Zhang <zhang.lyra@xxxxxxxxx> wrote:
>>
>> Hi Arnd,
>>
>> Thanks for finding out this and fixing it, but we have a minor concern
>> for changing ARCH_APRD back to bool.
>>
>> On Thu, Apr 9, 2020 at 2:57 AM Arnd Bergmann <arnd@xxxxxxxx> wrote:
>> >
>> > I got a build failure with CONFIG_ARCH_SPRD=m when the
>> > main portion of the clock driver failed to get linked into
>> > the kernel:
>> >
>> > ERROR: modpost: "sprd_pll_sc_gate_ops" [drivers/clk/sprd/sc9863a-clk.ko] undefined!
>> > ERROR: modpost: "sprd_pll_ops" [drivers/clk/sprd/sc9863a-clk.ko] undefined!
>> > ERROR: modpost: "sprd_div_ops" [drivers/clk/sprd/sc9863a-clk.ko] undefined!
>> > ERROR: modpost: "sprd_comp_ops" [drivers/clk/sprd/sc9863a-clk.ko] undefined!
>> > ERROR: modpost: "sprd_mux_ops" [drivers/clk/sprd/sc9863a-clk.ko] undefined!
>> > ERROR: modpost: "sprd_gate_ops" [drivers/clk/sprd/sc9863a-clk.ko] undefined!
>> > ERROR: modpost: "sprd_sc_gate_ops" [drivers/clk/sprd/sc9863a-clk.ko] undefined!
>> > ERROR: modpost: "sprd_clk_probe" [drivers/clk/sprd/sc9863a-clk.ko] undefined!
>> > ERROR: modpost: "sprd_clk_regmap_init" [drivers/clk/sprd/sc9863a-clk.ko] undefined!
>> > ERROR: modpost: "sprd_pll_ops" [drivers/clk/sprd/sc9860-clk.ko] undefined!
>> > ERROR: modpost: "sprd_div_ops" [drivers/clk/sprd/sc9860-clk.ko] undefined!
>> > ERROR: modpost: "sprd_mux_ops" [drivers/clk/sprd/sc9860-clk.ko] undefined!
>> >
>> > This is a combination of two trivial bugs:
>> >
>> > - A platform should not be 'tristate', it should be a 'bool' symbol
>> >   like the other platforms, if only for consistency, and to avoid
>> >   surprises like this one.
>>
>> After a discussion, we decided to change ARCH_SPRD to tristate, the
>> idea was that we hope we can simply switch all sprd drivers' configs
>> (whose default is ARCH_SPRD) to 'm' by setting ARCH_SPRD=m, or switch
>> all them to 'y' by setting ARCH_SPRD=y, instead of changing them one
>> by one. This requirement originally came from that Google GKI project
>> asks all vendor drivers to be built as modules.
>
>
>
> Unfortunately, even if ARCH_SPRD can be tristate, we found out (like Ard did here) that none of the other platform symbols can be tristate :(.
>
> So, we are going to enable all CONFIG_ARCH_XXXX in our defconfig[1]. Chunyan, Please feel free to submit that patch to AOSP for that.
>
> This does present us with a problem. We found that a bunch of drivers are  'default y if ARCH_XXX'. A lot of them have no symbol dependencies on the code that gets compiled with ARCH_XXX. They depend on it only because "the driver is only needed for the XXX SoC or the family".
>
> For example, enabling CONFIG_ARCH_MEDIATEK, will end up building almost all drivers in drivers/pinctrl/mediatek as far as I can see.
>
> This does add up. It increases the size of the kernel considerably. I have plans to send out the comparison in the future (later this year) once we are done collecting all def configs and see how bad that is.
>
> The only sane way I can see that can be resolved (if people agree that's a problem), is to make the ARCH_XXX code tristate-able and make the ARCH_XXX Kconfig select every driver it needs, instead of the other way round.

If we making the ARCH_XXX Kconfig select all drivers it needs, we will
not have chance to custom the kernel Image for debug purpose. For
example we can bringup a minimum system with only serial driver on
sprd platforms.

>
> All that being said, It is obviously not ok to have the allmodconfig broken like this without adding explicit dependencies as suggested above, or revert CONFIG_ARCH_SPRD to be a 'bool'.

We see this broken because I shouldn't leave clk Makefile a tristate
compile [1] after changing ARCH_SPRD to be tristate.

If we will make ARCH_SPRD tristate-able in the future and you all
aggree that, I would like to do it now, and pay more attention to
Makefiles and dependencies.

I can also make a change like below:

diff --git a/drivers/clk/sprd/Kconfig b/drivers/clk/sprd/Kconfig
index e18c80fbe804..9f7d9d8899a5 100644
--- a/drivers/clk/sprd/Kconfig
+++ b/drivers/clk/sprd/Kconfig
@@ -2,6 +2,7 @@
 config SPRD_COMMON_CLK
        tristate "Clock support for Spreadtrum SoCs"
        depends on ARCH_SPRD || COMPILE_TEST
+       depends on m || ARCH_SPRD != m
        default ARCH_SPRD
        select REGMAP_MMIO

Arnd, Stephen, Sandeep, what do you think? Does that make sense?

Thanks,
Chunyan

[1] https://elixir.bootlin.com/linux/v5.6.3/source/drivers/clk/Makefile#L108

>
> So fwiw,
>
> Acked-by: Sandeep Patil <sspatil@xxxxxxxxxxx>
>
> - ssp
>
> 1. https://android.googlesource.com/kernel/common/+/refs/heads/android-5.4/arch/arm64/configs/gki_defconfig#45