| Message ID | 20200408160044.2550437-1-arnd@arndb.de (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | b5f73d47f34b238221ac771b5fe4907df621d7cb |
| Headers | show |
| Series | [RFC] clk: sprd: fix compile-testing | expand |
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@arndb.de> 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. Thanks, Chunyan > > - The clk Makefile does not traverse into the sprd subdirectory > if the platform is disabled but the drivers are enabled for > compile-testing. > > Fixing either of the two would be sufficient to address the link failure, > but for correctness, both need to be changed. > > Fixes: 2b1b799d7630 ("arm64: change ARCH_SPRD Kconfig to tristate") > Fixes: d41f59fd92f2 ("clk: sprd: Add common infrastructure") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > arch/arm64/Kconfig.platforms | 2 +- > drivers/clk/Makefile | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms > index 55d70cfe0f9e..3c7e310fd8bf 100644 > --- a/arch/arm64/Kconfig.platforms > +++ b/arch/arm64/Kconfig.platforms > @@ -248,7 +248,7 @@ config ARCH_TEGRA > This enables support for the NVIDIA Tegra SoC family. > > config ARCH_SPRD > - tristate "Spreadtrum SoC platform" > + bool "Spreadtrum SoC platform" > help > Support for Spreadtrum ARM based SoCs > > diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile > index f4169cc2fd31..60e811d3f226 100644 > --- a/drivers/clk/Makefile > +++ b/drivers/clk/Makefile > @@ -105,7 +105,7 @@ obj-$(CONFIG_CLK_SIFIVE) += sifive/ > obj-$(CONFIG_ARCH_SIRF) += sirf/ > obj-$(CONFIG_ARCH_SOCFPGA) += socfpga/ > obj-$(CONFIG_PLAT_SPEAR) += spear/ > -obj-$(CONFIG_ARCH_SPRD) += sprd/ > +obj-y += sprd/ > obj-$(CONFIG_ARCH_STI) += st/ > obj-$(CONFIG_ARCH_STRATIX10) += socfpga/ > obj-$(CONFIG_ARCH_SUNXI) += sunxi/ > -- > 2.26.0 >
Quoting Arnd Bergmann (2020-04-08 09:00:44) > 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. > > - The clk Makefile does not traverse into the sprd subdirectory > if the platform is disabled but the drivers are enabled for > compile-testing. > > Fixing either of the two would be sufficient to address the link failure, > but for correctness, both need to be changed. > > Fixes: 2b1b799d7630 ("arm64: change ARCH_SPRD Kconfig to tristate") > Fixes: d41f59fd92f2 ("clk: sprd: Add common infrastructure") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > arch/arm64/Kconfig.platforms | 2 +- > drivers/clk/Makefile | 2 +- For clk part Acked-by: Stephen Boyd <sboyd@kernel.org>
On Fri, 10 Apr 2020 at 04:17, Sandeep Patil <sspatil@android.com> wrote: > > > > On Wed, Apr 8, 2020 at 11:09 PM Chunyan Zhang <zhang.lyra@gmail.com> 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@arndb.de> 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@android.com> > > - ssp > > 1. https://android.googlesource.com/kernel/common/+/refs/heads/android-5.4/arch/arm64/configs/gki_defconfig#45
Quoting Chunyan Zhang (2020-04-09 20:45:16) > 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? Sorry, doesn't make any sense to me. The ARCH_FOO configs for various platforms are intended to be used to limit the configuration space of various other Kconfig symbols for the code that only matters to those platforms. The usage of depends and default is correct here already. The ARCH_FOO configs should always be bool. Any code bloat problems seen by config symbols enabling because they're 'default ARCH_FOO' can be resolved by explicitly disabling those configs.
On Sat, 11 Apr 2020 at 10:27, Stephen Boyd <sboyd@kernel.org> wrote: > > Quoting Chunyan Zhang (2020-04-09 20:45:16) > > 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? > > Sorry, doesn't make any sense to me. The ARCH_FOO configs for various > platforms are intended to be used to limit the configuration space of > various other Kconfig symbols for the code that only matters to those > platforms. The usage of depends and default is correct here already. The > ARCH_FOO configs should always be bool. Any code bloat problems seen by > config symbols enabling because they're 'default ARCH_FOO' can be > resolved by explicitly disabling those configs. Ok - alright, please feel free to merge Arnd's patch then. Thanks, Chunyan
On Thu, Apr 9, 2020 at 2:57 AM Arnd Bergmann <arnd@arndb.de> 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. > > - The clk Makefile does not traverse into the sprd subdirectory > if the platform is disabled but the drivers are enabled for > compile-testing. > > Fixing either of the two would be sufficient to address the link failure, > but for correctness, both need to be changed. > > Fixes: 2b1b799d7630 ("arm64: change ARCH_SPRD Kconfig to tristate") > Fixes: d41f59fd92f2 ("clk: sprd: Add common infrastructure") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> Hi, This patch seems not been applied to next branch? I haven't seen it on linux-next. Arnd, can you please pick it to your tree. In case you need my ack: Acked-by: Chunyan Zhang <chunyan.zhang@unisoc.com> Thanks, Chunyan > --- > arch/arm64/Kconfig.platforms | 2 +- > drivers/clk/Makefile | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms > index 55d70cfe0f9e..3c7e310fd8bf 100644 > --- a/arch/arm64/Kconfig.platforms > +++ b/arch/arm64/Kconfig.platforms > @@ -248,7 +248,7 @@ config ARCH_TEGRA > This enables support for the NVIDIA Tegra SoC family. > > config ARCH_SPRD > - tristate "Spreadtrum SoC platform" > + bool "Spreadtrum SoC platform" > help > Support for Spreadtrum ARM based SoCs > > diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile > index f4169cc2fd31..60e811d3f226 100644 > --- a/drivers/clk/Makefile > +++ b/drivers/clk/Makefile > @@ -105,7 +105,7 @@ obj-$(CONFIG_CLK_SIFIVE) += sifive/ > obj-$(CONFIG_ARCH_SIRF) += sirf/ > obj-$(CONFIG_ARCH_SOCFPGA) += socfpga/ > obj-$(CONFIG_PLAT_SPEAR) += spear/ > -obj-$(CONFIG_ARCH_SPRD) += sprd/ > +obj-y += sprd/ > obj-$(CONFIG_ARCH_STI) += st/ > obj-$(CONFIG_ARCH_STRATIX10) += socfpga/ > obj-$(CONFIG_ARCH_SUNXI) += sunxi/ > -- > 2.26.0 >
On Wed, Jun 3, 2020 at 11:17 AM Chunyan Zhang <zhang.lyra@gmail.com> wrote: > On Thu, Apr 9, 2020 at 2:57 AM Arnd Bergmann <arnd@arndb.de> wrote: > > This patch seems not been applied to next branch? I haven't seen it on > linux-next. > Arnd, can you please pick it to your tree. > In case you need my ack: > Acked-by: Chunyan Zhang <chunyan.zhang@unisoc.com> > Ok, I applied it on the arm/drivers branch now, thanks for following up on this! Arnd
diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms index 55d70cfe0f9e..3c7e310fd8bf 100644 --- a/arch/arm64/Kconfig.platforms +++ b/arch/arm64/Kconfig.platforms @@ -248,7 +248,7 @@ config ARCH_TEGRA This enables support for the NVIDIA Tegra SoC family. config ARCH_SPRD - tristate "Spreadtrum SoC platform" + bool "Spreadtrum SoC platform" help Support for Spreadtrum ARM based SoCs diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile index f4169cc2fd31..60e811d3f226 100644 --- a/drivers/clk/Makefile +++ b/drivers/clk/Makefile @@ -105,7 +105,7 @@ obj-$(CONFIG_CLK_SIFIVE) += sifive/ obj-$(CONFIG_ARCH_SIRF) += sirf/ obj-$(CONFIG_ARCH_SOCFPGA) += socfpga/ obj-$(CONFIG_PLAT_SPEAR) += spear/ -obj-$(CONFIG_ARCH_SPRD) += sprd/ +obj-y += sprd/ obj-$(CONFIG_ARCH_STI) += st/ obj-$(CONFIG_ARCH_STRATIX10) += socfpga/ obj-$(CONFIG_ARCH_SUNXI) += sunxi/
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. - The clk Makefile does not traverse into the sprd subdirectory if the platform is disabled but the drivers are enabled for compile-testing. Fixing either of the two would be sufficient to address the link failure, but for correctness, both need to be changed. Fixes: 2b1b799d7630 ("arm64: change ARCH_SPRD Kconfig to tristate") Fixes: d41f59fd92f2 ("clk: sprd: Add common infrastructure") Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- arch/arm64/Kconfig.platforms | 2 +- drivers/clk/Makefile | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)