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 <[email protected]>
---
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
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 <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
> ---
> arch/arm64/Kconfig.platforms | 2 +-
> drivers/clk/Makefile | 2 +-
For clk part
Acked-by: Stephen Boyd <[email protected]>
On Fri, 10 Apr 2020 at 04:17, Sandeep Patil <[email protected]> wrote:
>
>
>
> On Wed, Apr 8, 2020 at 11:09 PM Chunyan Zhang <[email protected]> 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 <[email protected]> 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 <[email protected]>
>
> - 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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
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 <[email protected]>
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 <[email protected]> wrote:
> On Thu, Apr 9, 2020 at 2:57 AM Arnd Bergmann <[email protected]> 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 <[email protected]>
>
Ok, I applied it on the arm/drivers branch now, thanks for following
up on this!
Arnd