[V2,3/9] clk: imx: Support building SCU clock driver as module

Commit Message

Anson Huang June 9, 2020, 7:32 a.m. UTC

There are more and more requirements of building SoC specific drivers
as modules, add support for building SCU clock driver as module to meet
the requirement.

Signed-off-by: Anson Huang <[e-mail]>
---
Changes since V1:
	- add ARCH_MXC to build dependency to avoid build fail on x86 arch;
	- move clk-lpcg-scu.c change in to this patch.
---
 drivers/clk/imx/Kconfig        | 4 ++--
 drivers/clk/imx/Makefile       | 5 ++---
 drivers/clk/imx/clk-lpcg-scu.c | 1 +
 drivers/clk/imx/clk-scu.c      | 5 +++++
 4 files changed, 10 insertions(+), 5 deletions(-)

Comments

Aisheng Dong June 17, 2020, 10:44 a.m. UTC | #1

> From: Anson Huang <[e-mail]>
> Sent: Tuesday, June 9, 2020 3:32 PM
> 
> There are more and more requirements of building SoC specific drivers as
> modules, add support for building SCU clock driver as module to meet the
> requirement.
> 
> Signed-off-by: Anson Huang <[e-mail]>
> ---
> Changes since V1:
> 	- add ARCH_MXC to build dependency to avoid build fail on x86 arch;
> 	- move clk-lpcg-scu.c change in to this patch.
> ---
>  drivers/clk/imx/Kconfig        | 4 ++--
>  drivers/clk/imx/Makefile       | 5 ++---
>  drivers/clk/imx/clk-lpcg-scu.c | 1 +
>  drivers/clk/imx/clk-scu.c      | 5 +++++
>  4 files changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/clk/imx/Kconfig b/drivers/clk/imx/Kconfig index
> db0253f..ded0643 100644
> --- a/drivers/clk/imx/Kconfig
> +++ b/drivers/clk/imx/Kconfig
> @@ -5,8 +5,8 @@ config MXC_CLK
>  	def_bool ARCH_MXC
> 
>  config MXC_CLK_SCU
> -	bool
> -	depends on IMX_SCU

Keep this line as it is

> +	tristate "IMX SCU clock"

i.MX SCU Clock core driver

> +	depends on ARCH_MXC && IMX_SCU
> 
>  config CLK_IMX8MM
>  	bool "IMX8MM CCM Clock Driver"
> diff --git a/drivers/clk/imx/Makefile b/drivers/clk/imx/Makefile index
> 928f874..1af8cff 100644
> --- a/drivers/clk/imx/Makefile
> +++ b/drivers/clk/imx/Makefile
> @@ -21,9 +21,8 @@ obj-$(CONFIG_MXC_CLK) += \
>  	clk-sscg-pll.o \
>  	clk-pll14xx.o
> 
> -obj-$(CONFIG_MXC_CLK_SCU) += \
> -	clk-scu.o \
> -	clk-lpcg-scu.o
> +mxc-clk-scu-objs += clk-scu.o clk-lpcg-scu.o
> +obj-$(CONFIG_MXC_CLK_SCU) += mxc-clk-scu.o

Like i.MX pinctrl, I'm not sure if it's really necessary to build core libraries
as modules. Probably the simplest way is only building platform drivers part
as module. And leave those core libraries built in kernel.
This may make the code a bit cleaner.

Regards
Aisheng

> 
>  obj-$(CONFIG_CLK_IMX8MM) += clk-imx8mm.o
>  obj-$(CONFIG_CLK_IMX8MN) += clk-imx8mn.o diff --git
> a/drivers/clk/imx/clk-lpcg-scu.c b/drivers/clk/imx/clk-lpcg-scu.c index
> a73a799..8177f0e 100644
> --- a/drivers/clk/imx/clk-lpcg-scu.c
> +++ b/drivers/clk/imx/clk-lpcg-scu.c
> @@ -114,3 +114,4 @@ struct clk_hw *imx_clk_lpcg_scu(const char *name,
> const char *parent_name,
> 
>  	return hw;
>  }
> +EXPORT_SYMBOL_GPL(imx_clk_lpcg_scu);
> diff --git a/drivers/clk/imx/clk-scu.c b/drivers/clk/imx/clk-scu.c index
> b8b2072..9688981 100644
> --- a/drivers/clk/imx/clk-scu.c
> +++ b/drivers/clk/imx/clk-scu.c
> @@ -8,6 +8,7 @@
>  #include <linux/arm-smccc.h>
>  #include <linux/clk-provider.h>
>  #include <linux/err.h>
> +#include <linux/module.h>
>  #include <linux/slab.h>
> 
>  #include "clk-scu.h"
> @@ -132,6 +133,7 @@ int imx_clk_scu_init(void)  {
>  	return imx_scu_get_handle(&ccm_ipc_handle);
>  }
> +EXPORT_SYMBOL_GPL(imx_clk_scu_init);
> 
>  /*
>   * clk_scu_recalc_rate - Get clock rate for a SCU clock @@ -387,3 +389,6 @@
> struct clk_hw *__imx_clk_scu(const char *name, const char * const *parents,
> 
>  	return hw;
>  }
> +EXPORT_SYMBOL_GPL(__imx_clk_scu);
> +
> +MODULE_LICENSE("GPL v2");
> --
> 2.7.4

Anson Huang June 17, 2020, 12:26 p.m. UTC | #2

> Subject: RE: [PATCH V2 3/9] clk: imx: Support building SCU clock driver as
> module
> 
> > From: Anson Huang <[e-mail]>
> > Sent: Tuesday, June 9, 2020 3:32 PM
> >
> > There are more and more requirements of building SoC specific drivers
> > as modules, add support for building SCU clock driver as module to
> > meet the requirement.
> >
> > Signed-off-by: Anson Huang <[e-mail]>
> > ---
> > Changes since V1:
> > 	- add ARCH_MXC to build dependency to avoid build fail on x86 arch;
> > 	- move clk-lpcg-scu.c change in to this patch.
> > ---
> >  drivers/clk/imx/Kconfig        | 4 ++--
> >  drivers/clk/imx/Makefile       | 5 ++---
> >  drivers/clk/imx/clk-lpcg-scu.c | 1 +
> >  drivers/clk/imx/clk-scu.c      | 5 +++++
> >  4 files changed, 10 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/clk/imx/Kconfig b/drivers/clk/imx/Kconfig index
> > db0253f..ded0643 100644
> > --- a/drivers/clk/imx/Kconfig
> > +++ b/drivers/clk/imx/Kconfig
> > @@ -5,8 +5,8 @@ config MXC_CLK
> >  	def_bool ARCH_MXC
> >
> >  config MXC_CLK_SCU
> > -	bool
> > -	depends on IMX_SCU
> 
> Keep this line as it is
> 
> > +	tristate "IMX SCU clock"
> 
> i.MX SCU Clock core driver
> 
> > +	depends on ARCH_MXC && IMX_SCU
> >
> >  config CLK_IMX8MM
> >  	bool "IMX8MM CCM Clock Driver"
> > diff --git a/drivers/clk/imx/Makefile b/drivers/clk/imx/Makefile index
> > 928f874..1af8cff 100644
> > --- a/drivers/clk/imx/Makefile
> > +++ b/drivers/clk/imx/Makefile
> > @@ -21,9 +21,8 @@ obj-$(CONFIG_MXC_CLK) += \
> >  	clk-sscg-pll.o \
> >  	clk-pll14xx.o
> >
> > -obj-$(CONFIG_MXC_CLK_SCU) += \
> > -	clk-scu.o \
> > -	clk-lpcg-scu.o
> > +mxc-clk-scu-objs += clk-scu.o clk-lpcg-scu.o
> > +obj-$(CONFIG_MXC_CLK_SCU) += mxc-clk-scu.o
> 
> Like i.MX pinctrl, I'm not sure if it's really necessary to build core libraries as
> modules. Probably the simplest way is only building platform drivers part as
> module. And leave those core libraries built in kernel.
> This may make the code a bit cleaner.
> 

Will discuss this with Linaro guys about it, previous requirement I received is
all SoC specific modules need to be built as module.

Anson

Aisheng Dong June 18, 2020, 1:58 a.m. UTC | #3

> From: Anson Huang <[e-mail]>
> Sent: Wednesday, June 17, 2020 8:27 PM
> 
> 
> > Subject: RE: [PATCH V2 3/9] clk: imx: Support building SCU clock
> > driver as module
> >
> > > From: Anson Huang <[e-mail]>
> > > Sent: Tuesday, June 9, 2020 3:32 PM
> > >
> > > There are more and more requirements of building SoC specific
> > > drivers as modules, add support for building SCU clock driver as
> > > module to meet the requirement.
> > >
> > > Signed-off-by: Anson Huang <[e-mail]>
> > > ---
> > > Changes since V1:
> > > 	- add ARCH_MXC to build dependency to avoid build fail on x86 arch;
> > > 	- move clk-lpcg-scu.c change in to this patch.
> > > ---
> > >  drivers/clk/imx/Kconfig        | 4 ++--
> > >  drivers/clk/imx/Makefile       | 5 ++---
> > >  drivers/clk/imx/clk-lpcg-scu.c | 1 +
> > >  drivers/clk/imx/clk-scu.c      | 5 +++++
> > >  4 files changed, 10 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/clk/imx/Kconfig b/drivers/clk/imx/Kconfig index
> > > db0253f..ded0643 100644
> > > --- a/drivers/clk/imx/Kconfig
> > > +++ b/drivers/clk/imx/Kconfig
> > > @@ -5,8 +5,8 @@ config MXC_CLK
> > >  	def_bool ARCH_MXC
> > >
> > >  config MXC_CLK_SCU
> > > -	bool
> > > -	depends on IMX_SCU
> >
> > Keep this line as it is
> >
> > > +	tristate "IMX SCU clock"
> >
> > i.MX SCU Clock core driver
> >
> > > +	depends on ARCH_MXC && IMX_SCU
> > >
> > >  config CLK_IMX8MM
> > >  	bool "IMX8MM CCM Clock Driver"
> > > diff --git a/drivers/clk/imx/Makefile b/drivers/clk/imx/Makefile
> > > index 928f874..1af8cff 100644
> > > --- a/drivers/clk/imx/Makefile
> > > +++ b/drivers/clk/imx/Makefile
> > > @@ -21,9 +21,8 @@ obj-$(CONFIG_MXC_CLK) += \
> > >  	clk-sscg-pll.o \
> > >  	clk-pll14xx.o
> > >
> > > -obj-$(CONFIG_MXC_CLK_SCU) += \
> > > -	clk-scu.o \
> > > -	clk-lpcg-scu.o
> > > +mxc-clk-scu-objs += clk-scu.o clk-lpcg-scu.o
> > > +obj-$(CONFIG_MXC_CLK_SCU) += mxc-clk-scu.o
> >
> > Like i.MX pinctrl, I'm not sure if it's really necessary to build core
> > libraries as modules. Probably the simplest way is only building
> > platform drivers part as module. And leave those core libraries built in kernel.
> > This may make the code a bit cleaner.
> >
> 
> Will discuss this with Linaro guys about it, previous requirement I received is all
> SoC specific modules need to be built as module.
> 

Okay. AFAIK it's not conflict.
You still make drivers into modules.
Only difference is for those common libraries part, we don't convert them into module
Which is less meaningless.
 
Regards
Aisheng

> Anson

Stephen Boyd June 20, 2020, 3:27 a.m. UTC | #4

Quoting Aisheng Dong (2020-06-17 18:58:51)
> > From: Anson Huang <[e-mail]>
> > > > +obj-$(CONFIG_MXC_CLK_SCU) += mxc-clk-scu.o
> > >
> > > Like i.MX pinctrl, I'm not sure if it's really necessary to build core
> > > libraries as modules. Probably the simplest way is only building
> > > platform drivers part as module. And leave those core libraries built in kernel.
> > > This may make the code a bit cleaner.
> > >
> > 
> > Will discuss this with Linaro guys about it, previous requirement I received is all
> > SoC specific modules need to be built as module.
> > 
> 
> Okay. AFAIK it's not conflict.
> You still make drivers into modules.
> Only difference is for those common libraries part, we don't convert them into module
> Which is less meaningless.
>  

What is the benefit of making the core part of the SoC driver not a
module? From the module perspective it should be perfectly fine to make
it a module as well, and then depmod will sort out loading modules in
the right order.

This is for android right?

Aisheng Dong June 23, 2020, 3:42 a.m. UTC | #5

> From: Stephen Boyd <[e-mail]>
> Sent: Saturday, June 20, 2020 11:28 AM
> Subject: RE: [PATCH V2 3/9] clk: imx: Support building SCU clock driver as
> module
> 
> Quoting Aisheng Dong (2020-06-17 18:58:51)
> > > From: Anson Huang <[e-mail]>
> > > > > +obj-$(CONFIG_MXC_CLK_SCU) += mxc-clk-scu.o
> > > >
> > > > Like i.MX pinctrl, I'm not sure if it's really necessary to build
> > > > core libraries as modules. Probably the simplest way is only
> > > > building platform drivers part as module. And leave those core libraries
> built in kernel.
> > > > This may make the code a bit cleaner.
> > > >
> > >
> > > Will discuss this with Linaro guys about it, previous requirement I
> > > received is all SoC specific modules need to be built as module.
> > >
> >
> > Okay. AFAIK it's not conflict.
> > You still make drivers into modules.
> > Only difference is for those common libraries part, we don't convert
> > them into module Which is less meaningless.
> >
> 
> What is the benefit of making the core part of the SoC driver not a module?

Usually we could try to build it as module if it's not hard.

One question is sometimes those core part are shared with some platforms which can't built as module.
For i.MX case, it's mainly patch 4:
[V2,4/9] clk: imx: Support building i.MX common clock driver as module
https://patchwork.kernel.org/patch/11594801/

Those libraries are also used by i.MX6&7 which can't build as module.
So we need an extra workaround patch to forcely 'select' it under arch/arm/mach-imx/Kconfig
[V2,2/9] ARM: imx: Select MXC_CLK for ARCH_MXC
https://patchwork.kernel.org/patch/11594793/
Then the users can't configure it as module in order to not break build.

If build-in those common libraries, the implementation could be a bit easier and cleaner.
So I'm not sure if we still have to build them as module.
How would you suggest for such case?

> From the module perspective it should be perfectly fine to make it a module as
> well, and then depmod will sort out loading modules in the right order.
> 
> This is for android right?

Yes, for Android GKI support.

Regards
Aisheng

Stephen Boyd June 23, 2020, 8:34 a.m. UTC | #6

Quoting Aisheng Dong (2020-06-22 20:42:19)
> > From: Stephen Boyd <[e-mail]>
> > Sent: Saturday, June 20, 2020 11:28 AM
> > Subject: RE: [PATCH V2 3/9] clk: imx: Support building SCU clock driver as
> > module
> > 
> > Quoting Aisheng Dong (2020-06-17 18:58:51)
> > > > From: Anson Huang <[e-mail]>
> > > > > > +obj-$(CONFIG_MXC_CLK_SCU) += mxc-clk-scu.o
> > > > >
> > > > > Like i.MX pinctrl, I'm not sure if it's really necessary to build
> > > > > core libraries as modules. Probably the simplest way is only
> > > > > building platform drivers part as module. And leave those core libraries
> > built in kernel.
> > > > > This may make the code a bit cleaner.
> > > > >
> > > >
> > > > Will discuss this with Linaro guys about it, previous requirement I
> > > > received is all SoC specific modules need to be built as module.
> > > >
> > >
> > > Okay. AFAIK it's not conflict.
> > > You still make drivers into modules.
> > > Only difference is for those common libraries part, we don't convert
> > > them into module Which is less meaningless.
> > >
> > 
> > What is the benefit of making the core part of the SoC driver not a module?
> 
> Usually we could try to build it as module if it's not hard.
> 
> One question is sometimes those core part are shared with some platforms which can't built as module.
> For i.MX case, it's mainly patch 4:
> [V2,4/9] clk: imx: Support building i.MX common clock driver as module
> https://patchwork.kernel.org/patch/11594801/
> 
> Those libraries are also used by i.MX6&7 which can't build as module.
> So we need an extra workaround patch to forcely 'select' it under arch/arm/mach-imx/Kconfig
> [V2,2/9] ARM: imx: Select MXC_CLK for ARCH_MXC
> https://patchwork.kernel.org/patch/11594793/
> Then the users can't configure it as module in order to not break build.
> 
> If build-in those common libraries, the implementation could be a bit easier and cleaner.
> So I'm not sure if we still have to build them as module.
> How would you suggest for such case?

Stop using 'select MXC_CLK' when requiring the core library code?
Instead, make it a 'depends' and then that will make depending modules
(i.e. the SoC files) that want to be builtin force the core module to be
builtin too. Other modular configs that depend on the core will still be
modular. 

I don't know why an architecture is selecting the clk code at all to be
honest. That can be moved to the defconfig instead of in the
architecture Kconfig and then you don't get a working system unless you
select the MXC_CLK config from the configurator tool (menuconfig,
nconfig, etc.) So ARCH_MXC shouldn't be in this discussion and the core
module should be selectable by the configurator and that should be
tristate and all SoC modules should depend on that core library module
and be selectable too and those Kconfigs can be tristate or bool.

Aisheng Dong June 23, 2020, 9 a.m. UTC | #7

> From: Stephen Boyd <[e-mail]>
> Sent: Tuesday, June 23, 2020 4:34 PM
> Subject: RE: [PATCH V2 3/9] clk: imx: Support building SCU clock driver as
> module
> 
> Quoting Aisheng Dong (2020-06-22 20:42:19)
> > > From: Stephen Boyd <[e-mail]>
> > > Sent: Saturday, June 20, 2020 11:28 AM
> > > Subject: RE: [PATCH V2 3/9] clk: imx: Support building SCU clock
> > > driver as module
> > >
> > > Quoting Aisheng Dong (2020-06-17 18:58:51)
> > > > > From: Anson Huang <[e-mail]>
> > > > > > > +obj-$(CONFIG_MXC_CLK_SCU) += mxc-clk-scu.o
> > > > > >
> > > > > > Like i.MX pinctrl, I'm not sure if it's really necessary to
> > > > > > build core libraries as modules. Probably the simplest way is
> > > > > > only building platform drivers part as module. And leave those
> > > > > > core libraries
> > > built in kernel.
> > > > > > This may make the code a bit cleaner.
> > > > > >
> > > > >
> > > > > Will discuss this with Linaro guys about it, previous
> > > > > requirement I received is all SoC specific modules need to be built as
> module.
> > > > >
> > > >
> > > > Okay. AFAIK it's not conflict.
> > > > You still make drivers into modules.
> > > > Only difference is for those common libraries part, we don't
> > > > convert them into module Which is less meaningless.
> > > >
> > >
> > > What is the benefit of making the core part of the SoC driver not a module?
> >
> > Usually we could try to build it as module if it's not hard.
> >
> > One question is sometimes those core part are shared with some platforms
> which can't built as module.
> > For i.MX case, it's mainly patch 4:
> > [V2,4/9] clk: imx: Support building i.MX common clock driver as module
> >
> >
> > Those libraries are also used by i.MX6&7 which can't build as module.
> > So we need an extra workaround patch to forcely 'select' it under
> > arch/arm/mach-imx/Kconfig [V2,2/9] ARM: imx: Select MXC_CLK for
> > ARCH_MXC
> > Then the users can't configure it as module in order to not break build.
> >
> > If build-in those common libraries, the implementation could be a bit easier
> and cleaner.
> > So I'm not sure if we still have to build them as module.
> > How would you suggest for such case?
> 
> Stop using 'select MXC_CLK' when requiring the core library code?
> Instead, make it a 'depends' and then that will make depending modules (i.e. the
> SoC files) that want to be builtin force the core module to be builtin too. Other
> modular configs that depend on the core will still be modular.
> 

It seems not work.
Patch 4 already changes it to depend on ARCH_MXC which can only be 'Y'.
[V2,4/9] clk: imx: Support building i.MX common clock driver as module
diff --git a/drivers/clk/imx/Kconfig b/drivers/clk/imx/Kconfig
index ded0643..678113b 100644
--- a/drivers/clk/imx/Kconfig
+++ b/drivers/clk/imx/Kconfig
@@ -1,8 +1,8 @@ 
 # SPDX-License-Identifier: GPL-2.0
 # common clock support for NXP i.MX SoC family.
 config MXC_CLK
-	bool
-	def_bool ARCH_MXC
+	tristate "IMX clock"
+	depends on ARCH_MXC

But user can still set MXC_CLK to be m, either via make menuconfig or defconfig.

Looks like only select it in arch Kconfig in Patch 2, there will be no issue.
diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig
index e7d7b90..47b10d2 100644
--- a/arch/arm/mach-imx/Kconfig
+++ b/arch/arm/mach-imx/Kconfig
@@ -4,6 +4,7 @@  menuconfig ARCH_MXC
 	depends on ARCH_MULTI_V4_V5 || ARCH_MULTI_V6_V7 || ARM_SINGLE_ARMV7M
 	select ARCH_SUPPORTS_BIG_ENDIAN
 	select CLKSRC_IMX_GPT
+	select MXC_CLK
 	select GENERIC_IRQ_CHIP
 	select GPIOLIB
 	select PINCTRL

> I don't know why an architecture is selecting the clk code at all to be honest.
> That can be moved to the defconfig instead of in the architecture Kconfig and
> then you don't get a working system unless you select the MXC_CLK config from
> the configurator tool (menuconfig, nconfig, etc.) So ARCH_MXC shouldn't be in
> this discussion and the core module should be selectable by the configurator
> and that should be tristate and all SoC modules should depend on that core
> library module and be selectable too and those Kconfigs can be tristate or bool.

I guess it is because we didn't have requirements to make minimum booting required
drivers to be built as module before.

Regards
Aisheng

Stephen Boyd June 24, 2020, 12:57 a.m. UTC | #8

Quoting Aisheng Dong (2020-06-23 02:00:47)
> > From: Stephen Boyd <[e-mail]>
> > Sent: Tuesday, June 23, 2020 4:34 PM
> > Subject: RE: [PATCH V2 3/9] clk: imx: Support building SCU clock driver as
> > module
> > 
> > Quoting Aisheng Dong (2020-06-22 20:42:19)
> > > > From: Stephen Boyd <[e-mail]>
> > > > Sent: Saturday, June 20, 2020 11:28 AM
> > > > Subject: RE: [PATCH V2 3/9] clk: imx: Support building SCU clock
> > > > driver as module
> > > >
> > > > Quoting Aisheng Dong (2020-06-17 18:58:51)
> > > > > > From: Anson Huang <[e-mail]>
> > > > > > > > +obj-$(CONFIG_MXC_CLK_SCU) += mxc-clk-scu.o
> > > > > > >
> > > > > > > Like i.MX pinctrl, I'm not sure if it's really necessary to
> > > > > > > build core libraries as modules. Probably the simplest way is
> > > > > > > only building platform drivers part as module. And leave those
> > > > > > > core libraries
> > > > built in kernel.
> > > > > > > This may make the code a bit cleaner.
> > > > > > >
> > > > > >
> > > > > > Will discuss this with Linaro guys about it, previous
> > > > > > requirement I received is all SoC specific modules need to be built as
> > module.
> > > > > >
> > > > >
> > > > > Okay. AFAIK it's not conflict.
> > > > > You still make drivers into modules.
> > > > > Only difference is for those common libraries part, we don't
> > > > > convert them into module Which is less meaningless.
> > > > >
> > > >
> > > > What is the benefit of making the core part of the SoC driver not a module?
> > >
> > > Usually we could try to build it as module if it's not hard.
> > >
> > > One question is sometimes those core part are shared with some platforms
> > which can't built as module.
> > > For i.MX case, it's mainly patch 4:
> > > [V2,4/9] clk: imx: Support building i.MX common clock driver as module
> > >
> > >
> > > Those libraries are also used by i.MX6&7 which can't build as module.
> > > So we need an extra workaround patch to forcely 'select' it under
> > > arch/arm/mach-imx/Kconfig [V2,2/9] ARM: imx: Select MXC_CLK for
> > > ARCH_MXC
> > > Then the users can't configure it as module in order to not break build.
> > >
> > > If build-in those common libraries, the implementation could be a bit easier
> > and cleaner.
> > > So I'm not sure if we still have to build them as module.
> > > How would you suggest for such case?
> > 
> > Stop using 'select MXC_CLK' when requiring the core library code?
> > Instead, make it a 'depends' and then that will make depending modules (i.e. the
> > SoC files) that want to be builtin force the core module to be builtin too. Other
> > modular configs that depend on the core will still be modular.
> > 
> 
> It seems not work.
> Patch 4 already changes it to depend on ARCH_MXC which can only be 'Y'.
> [V2,4/9] clk: imx: Support building i.MX common clock driver as module
> diff --git a/drivers/clk/imx/Kconfig b/drivers/clk/imx/Kconfig
> index ded0643..678113b 100644
> --- a/drivers/clk/imx/Kconfig
> +++ b/drivers/clk/imx/Kconfig
> @@ -1,8 +1,8 @@ 
>  # SPDX-License-Identifier: GPL-2.0
>  # common clock support for NXP i.MX SoC family.
>  config MXC_CLK
> -       bool
> -       def_bool ARCH_MXC
> +       tristate "IMX clock"
> +       depends on ARCH_MXC
> 
> But user can still set MXC_CLK to be m, either via make menuconfig or defconfig.

Isn't that what we want? Why does ARCH_MXC being enabled mandate that it
is builtin? Is some architecture level code calling into the clk
driver?

Aisheng Dong June 24, 2020, 2:19 a.m. UTC | #9

> From: Stephen Boyd <[e-mail]>
> Sent: Wednesday, June 24, 2020 8:58 AM
> Subject: RE: [PATCH V2 3/9] clk: imx: Support building SCU clock driver as
> module
> 
> Quoting Aisheng Dong (2020-06-23 02:00:47)
> > > From: Stephen Boyd <[e-mail]>
> > > Sent: Tuesday, June 23, 2020 4:34 PM
> > > Subject: RE: [PATCH V2 3/9] clk: imx: Support building SCU clock
> > > driver as module
> > >
> > > Quoting Aisheng Dong (2020-06-22 20:42:19)
> > > > > From: Stephen Boyd <[e-mail]>
> > > > > Sent: Saturday, June 20, 2020 11:28 AM
> > > > > Subject: RE: [PATCH V2 3/9] clk: imx: Support building SCU clock
> > > > > driver as module
> > > > >
> > > > > Quoting Aisheng Dong (2020-06-17 18:58:51)
> > > > > > > From: Anson Huang <[e-mail]>
> > > > > > > > > +obj-$(CONFIG_MXC_CLK_SCU) += mxc-clk-scu.o
> > > > > > > >
> > > > > > > > Like i.MX pinctrl, I'm not sure if it's really necessary
> > > > > > > > to build core libraries as modules. Probably the simplest
> > > > > > > > way is only building platform drivers part as module. And
> > > > > > > > leave those core libraries
> > > > > built in kernel.
> > > > > > > > This may make the code a bit cleaner.
> > > > > > > >
> > > > > > >
> > > > > > > Will discuss this with Linaro guys about it, previous
> > > > > > > requirement I received is all SoC specific modules need to
> > > > > > > be built as
> > > module.
> > > > > > >
> > > > > >
> > > > > > Okay. AFAIK it's not conflict.
> > > > > > You still make drivers into modules.
> > > > > > Only difference is for those common libraries part, we don't
> > > > > > convert them into module Which is less meaningless.
> > > > > >
> > > > >
> > > > > What is the benefit of making the core part of the SoC driver not a
> module?
> > > >
> > > > Usually we could try to build it as module if it's not hard.
> > > >
> > > > One question is sometimes those core part are shared with some
> > > > platforms
> > > which can't built as module.
> > > > For i.MX case, it's mainly patch 4:
> > > > [V2,4/9] clk: imx: Support building i.MX common clock driver as
> > > > module
> > > >
> > > >
> > > > Those libraries are also used by i.MX6&7 which can't build as module.
> > > > So we need an extra workaround patch to forcely 'select' it under
> > > > arch/arm/mach-imx/Kconfig [V2,2/9] ARM: imx: Select MXC_CLK for
> > > > ARCH_MXC Then the users can't configure it as module in order to
> > > > not break build.
> > > >
> > > > If build-in those common libraries, the implementation could be a
> > > > bit easier
> > > and cleaner.
> > > > So I'm not sure if we still have to build them as module.
> > > > How would you suggest for such case?
> > >
> > > Stop using 'select MXC_CLK' when requiring the core library code?
> > > Instead, make it a 'depends' and then that will make depending
> > > modules (i.e. the SoC files) that want to be builtin force the core
> > > module to be builtin too. Other modular configs that depend on the core will
> still be modular.
> > >
> >
> > It seems not work.
> > Patch 4 already changes it to depend on ARCH_MXC which can only be 'Y'.
> > [V2,4/9] clk: imx: Support building i.MX common clock driver as module
> > diff --git a/drivers/clk/imx/Kconfig b/drivers/clk/imx/Kconfig index
> > ded0643..678113b 100644
> > --- a/drivers/clk/imx/Kconfig
> > +++ b/drivers/clk/imx/Kconfig
> > @@ -1,8 +1,8 @@
> >  # SPDX-License-Identifier: GPL-2.0
> >  # common clock support for NXP i.MX SoC family.
> >  config MXC_CLK
> > -       bool
> > -       def_bool ARCH_MXC
> > +       tristate "IMX clock"
> > +       depends on ARCH_MXC
> >
> > But user can still set MXC_CLK to be m, either via make menuconfig or
> defconfig.
> 
> Isn't that what we want? 

No, if user set MXC_CLK to m, the build will break for i.MX6&7.

> Why does ARCH_MXC being enabled mandate that it is
> builtin? Is some architecture level code calling into the clk driver?


It's mainly because there's no Kconfig options for i.MX6 &7 clock drivers.
It just reuses ARCH config CONFIG_SOC_XXX which can only be y.
e.g.
obj-$(CONFIG_SOC_IMX6Q)  += clk-imx6q.o
obj-$(CONFIG_SOC_IMX6SL) += clk-imx6sl.o
obj-$(CONFIG_SOC_IMX7ULP) += clk-imx7ulp.o
obj-$(CONFIG_SOC_VF610)  += clk-vf610.o
..

If setting MXC_CLK to m, the platform clock drivers will fail to build due to miss
to find symbols defined in the common clock library by CONFIG_MXC_CLK.
So we have to avoid users to be able to config MXC_CLK to m for i.MX6&7.
Only depends on ARCH_MXC mean user still can set it to m.

Regards
Aisheng

Anson Huang June 24, 2020, 2:36 a.m. UTC | #10

> Subject: RE: [PATCH V2 3/9] clk: imx: Support building SCU clock driver as
> module
> 
> > From: Stephen Boyd <[e-mail]>
> > Sent: Wednesday, June 24, 2020 8:58 AM
> > Subject: RE: [PATCH V2 3/9] clk: imx: Support building SCU clock
> > driver as module
> >
> > Quoting Aisheng Dong (2020-06-23 02:00:47)
> > > > From: Stephen Boyd <[e-mail]>
> > > > Sent: Tuesday, June 23, 2020 4:34 PM
> > > > Subject: RE: [PATCH V2 3/9] clk: imx: Support building SCU clock
> > > > driver as module
> > > >
> > > > Quoting Aisheng Dong (2020-06-22 20:42:19)
> > > > > > From: Stephen Boyd <[e-mail]>
> > > > > > Sent: Saturday, June 20, 2020 11:28 AM
> > > > > > Subject: RE: [PATCH V2 3/9] clk: imx: Support building SCU
> > > > > > clock driver as module
> > > > > >
> > > > > > Quoting Aisheng Dong (2020-06-17 18:58:51)
> > > > > > > > From: Anson Huang <[e-mail]>
> > > > > > > > > > +obj-$(CONFIG_MXC_CLK_SCU) += mxc-clk-scu.o
> > > > > > > > >
> > > > > > > > > Like i.MX pinctrl, I'm not sure if it's really necessary
> > > > > > > > > to build core libraries as modules. Probably the
> > > > > > > > > simplest way is only building platform drivers part as
> > > > > > > > > module. And leave those core libraries
> > > > > > built in kernel.
> > > > > > > > > This may make the code a bit cleaner.
> > > > > > > > >
> > > > > > > >
> > > > > > > > Will discuss this with Linaro guys about it, previous
> > > > > > > > requirement I received is all SoC specific modules need to
> > > > > > > > be built as
> > > > module.
> > > > > > > >
> > > > > > >
> > > > > > > Okay. AFAIK it's not conflict.
> > > > > > > You still make drivers into modules.
> > > > > > > Only difference is for those common libraries part, we don't
> > > > > > > convert them into module Which is less meaningless.
> > > > > > >
> > > > > >
> > > > > > What is the benefit of making the core part of the SoC driver
> > > > > > not a
> > module?
> > > > >
> > > > > Usually we could try to build it as module if it's not hard.
> > > > >
> > > > > One question is sometimes those core part are shared with some
> > > > > platforms
> > > > which can't built as module.
> > > > > For i.MX case, it's mainly patch 4:
> > > > > [V2,4/9] clk: imx: Support building i.MX common clock driver as
> > > > > module
> > > > >
> > > > >
> > > > > Those libraries are also used by i.MX6&7 which can't build as module.
> > > > > So we need an extra workaround patch to forcely 'select' it
> > > > > under arch/arm/mach-imx/Kconfig [V2,2/9] ARM: imx: Select
> > > > > MXC_CLK for ARCH_MXC Then the users can't configure it as module
> > > > > in order to not break build.
> > > > >
> > > > > If build-in those common libraries, the implementation could be
> > > > > a bit easier
> > > > and cleaner.
> > > > > So I'm not sure if we still have to build them as module.
> > > > > How would you suggest for such case?
> > > >
> > > > Stop using 'select MXC_CLK' when requiring the core library code?
> > > > Instead, make it a 'depends' and then that will make depending
> > > > modules (i.e. the SoC files) that want to be builtin force the
> > > > core module to be builtin too. Other modular configs that depend
> > > > on the core will
> > still be modular.
> > > >
> > >
> > > It seems not work.
> > > Patch 4 already changes it to depend on ARCH_MXC which can only be 'Y'.
> > > [V2,4/9] clk: imx: Support building i.MX common clock driver as
> > > module diff --git a/drivers/clk/imx/Kconfig
> > > b/drivers/clk/imx/Kconfig index ded0643..678113b 100644
> > > --- a/drivers/clk/imx/Kconfig
> > > +++ b/drivers/clk/imx/Kconfig
> > > @@ -1,8 +1,8 @@
> > >  # SPDX-License-Identifier: GPL-2.0
> > >  # common clock support for NXP i.MX SoC family.
> > >  config MXC_CLK
> > > -       bool
> > > -       def_bool ARCH_MXC
> > > +       tristate "IMX clock"
> > > +       depends on ARCH_MXC
> > >
> > > But user can still set MXC_CLK to be m, either via make menuconfig
> > > or
> > defconfig.
> >
> > Isn't that what we want?
> 
> No, if user set MXC_CLK to m, the build will break for i.MX6&7.
> 
> > Why does ARCH_MXC being enabled mandate that it is builtin? Is some
> > architecture level code calling into the clk driver?
> 
> 
> It's mainly because there's no Kconfig options for i.MX6 &7 clock drivers.
> It just reuses ARCH config CONFIG_SOC_XXX which can only be y.
> e.g.
> obj-$(CONFIG_SOC_IMX6Q)  += clk-imx6q.o
> obj-$(CONFIG_SOC_IMX6SL) += clk-imx6sl.o
> obj-$(CONFIG_SOC_IMX7ULP) += clk-imx7ulp.o
> obj-$(CONFIG_SOC_VF610)  += clk-vf610.o
> ..
> 
> If setting MXC_CLK to m, the platform clock drivers will fail to build due to
> miss to find symbols defined in the common clock library by
> CONFIG_MXC_CLK.
> So we have to avoid users to be able to config MXC_CLK to m for i.MX6&7.
> Only depends on ARCH_MXC mean user still can set it to m.

I think for i.MX6/7, although MXC_CLK is tristate, but it is selected by ARCH_MXC which
is always "y", so MXC_CLK can ONLY be "y" even it is explicitly set to "m" in imx_v6_v7_defconfig
file. So that means MXC_CLK can ONLY support built-in for i.MX6/7 SoCs, and that is what
we want?

Anson

Aisheng Dong June 24, 2020, 2:59 a.m. UTC | #11

> From: Anson Huang <[e-mail]>
> Sent: Wednesday, June 24, 2020 10:36 AM
> 
> > Subject: RE: [PATCH V2 3/9] clk: imx: Support building SCU clock
> > driver as module
> >
> > > From: Stephen Boyd <[e-mail]>
> > > Sent: Wednesday, June 24, 2020 8:58 AM
> > > Subject: RE: [PATCH V2 3/9] clk: imx: Support building SCU clock
> > > driver as module
> > >
> > > Quoting Aisheng Dong (2020-06-23 02:00:47)
> > > > > From: Stephen Boyd <[e-mail]>
> > > > > Sent: Tuesday, June 23, 2020 4:34 PM
> > > > > Subject: RE: [PATCH V2 3/9] clk: imx: Support building SCU clock
> > > > > driver as module
> > > > >
> > > > > Quoting Aisheng Dong (2020-06-22 20:42:19)
> > > > > > > From: Stephen Boyd <[e-mail]>
> > > > > > > Sent: Saturday, June 20, 2020 11:28 AM
> > > > > > > Subject: RE: [PATCH V2 3/9] clk: imx: Support building SCU
> > > > > > > clock driver as module
> > > > > > >
> > > > > > > Quoting Aisheng Dong (2020-06-17 18:58:51)
> > > > > > > > > From: Anson Huang <[e-mail]>
> > > > > > > > > > > +obj-$(CONFIG_MXC_CLK_SCU) += mxc-clk-scu.o
> > > > > > > > > >
> > > > > > > > > > Like i.MX pinctrl, I'm not sure if it's really
> > > > > > > > > > necessary to build core libraries as modules. Probably
> > > > > > > > > > the simplest way is only building platform drivers
> > > > > > > > > > part as module. And leave those core libraries
> > > > > > > built in kernel.
> > > > > > > > > > This may make the code a bit cleaner.
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > Will discuss this with Linaro guys about it, previous
> > > > > > > > > requirement I received is all SoC specific modules need
> > > > > > > > > to be built as
> > > > > module.
> > > > > > > > >
> > > > > > > >
> > > > > > > > Okay. AFAIK it's not conflict.
> > > > > > > > You still make drivers into modules.
> > > > > > > > Only difference is for those common libraries part, we
> > > > > > > > don't convert them into module Which is less meaningless.
> > > > > > > >
> > > > > > >
> > > > > > > What is the benefit of making the core part of the SoC
> > > > > > > driver not a
> > > module?
> > > > > >
> > > > > > Usually we could try to build it as module if it's not hard.
> > > > > >
> > > > > > One question is sometimes those core part are shared with some
> > > > > > platforms
> > > > > which can't built as module.
> > > > > > For i.MX case, it's mainly patch 4:
> > > > > > [V2,4/9] clk: imx: Support building i.MX common clock driver
> > > > > > as module
> > > > > >
> > > > > >
> > > > > > Those libraries are also used by i.MX6&7 which can't build as module.
> > > > > > So we need an extra workaround patch to forcely 'select' it
> > > > > > under arch/arm/mach-imx/Kconfig [V2,2/9] ARM: imx: Select
> > > > > > MXC_CLK for ARCH_MXC Then the users can't configure it as
> > > > > > module in order to not break build.
> > > > > >
> > > > > > If build-in those common libraries, the implementation could
> > > > > > be a bit easier
> > > > > and cleaner.
> > > > > > So I'm not sure if we still have to build them as module.
> > > > > > How would you suggest for such case?
> > > > >
> > > > > Stop using 'select MXC_CLK' when requiring the core library code?
> > > > > Instead, make it a 'depends' and then that will make depending
> > > > > modules (i.e. the SoC files) that want to be builtin force the
> > > > > core module to be builtin too. Other modular configs that depend
> > > > > on the core will
> > > still be modular.
> > > > >
> > > >
> > > > It seems not work.
> > > > Patch 4 already changes it to depend on ARCH_MXC which can only be 'Y'.
> > > > [V2,4/9] clk: imx: Support building i.MX common clock driver as
> > > > module diff --git a/drivers/clk/imx/Kconfig
> > > > b/drivers/clk/imx/Kconfig index ded0643..678113b 100644
> > > > --- a/drivers/clk/imx/Kconfig
> > > > +++ b/drivers/clk/imx/Kconfig
> > > > @@ -1,8 +1,8 @@
> > > >  # SPDX-License-Identifier: GPL-2.0  # common clock support for
> > > > NXP i.MX SoC family.
> > > >  config MXC_CLK
> > > > -       bool
> > > > -       def_bool ARCH_MXC
> > > > +       tristate "IMX clock"
> > > > +       depends on ARCH_MXC
> > > >
> > > > But user can still set MXC_CLK to be m, either via make menuconfig
> > > > or
> > > defconfig.
> > >
> > > Isn't that what we want?
> >
> > No, if user set MXC_CLK to m, the build will break for i.MX6&7.
> >
> > > Why does ARCH_MXC being enabled mandate that it is builtin? Is some
> > > architecture level code calling into the clk driver?
> >
> >
> > It's mainly because there's no Kconfig options for i.MX6 &7 clock drivers.
> > It just reuses ARCH config CONFIG_SOC_XXX which can only be y.
> > e.g.
> > obj-$(CONFIG_SOC_IMX6Q)  += clk-imx6q.o
> > obj-$(CONFIG_SOC_IMX6SL) += clk-imx6sl.o
> > obj-$(CONFIG_SOC_IMX7ULP) += clk-imx7ulp.o
> > obj-$(CONFIG_SOC_VF610)  += clk-vf610.o ..
> >
> > If setting MXC_CLK to m, the platform clock drivers will fail to build
> > due to miss to find symbols defined in the common clock library by
> > CONFIG_MXC_CLK.
> > So we have to avoid users to be able to config MXC_CLK to m for i.MX6&7.
> > Only depends on ARCH_MXC mean user still can set it to m.
> 
> I think for i.MX6/7, although MXC_CLK is tristate, but it is selected by
> ARCH_MXC which is always "y", so MXC_CLK can ONLY be "y" even it is explicitly
> set to "m" in imx_v6_v7_defconfig file. So that means MXC_CLK can ONLY
> support built-in for i.MX6/7 SoCs, and that is what we want?
> 

Yes, I'm trying to explain to Stephen why we have to select MXC_CLK in ARCH_MXC
And what issues we will met if not select it.

Regards
Aisheng

> Anson

Arnd Bergmann June 24, 2020, 7:46 a.m. UTC | #12

On Wed, Jun 24, 2020 at 4:19 AM Aisheng Dong <[e-mail]> wrote:
> > Isn't that what we want?
>
> No, if user set MXC_CLK to m, the build will break for i.MX6&7.
>
> > Why does ARCH_MXC being enabled mandate that it is
> > builtin? Is some architecture level code calling into the clk driver?
>
>
> It's mainly because there's no Kconfig options for i.MX6 &7 clock drivers.
> It just reuses ARCH config CONFIG_SOC_XXX which can only be y.
> e.g.
> obj-$(CONFIG_SOC_IMX6Q)  += clk-imx6q.o
> obj-$(CONFIG_SOC_IMX6SL) += clk-imx6sl.o
> obj-$(CONFIG_SOC_IMX7ULP) += clk-imx7ulp.o
> obj-$(CONFIG_SOC_VF610)  += clk-vf610.o
> ..
>
> If setting MXC_CLK to m, the platform clock drivers will fail to build due to miss
> to find symbols defined in the common clock library by CONFIG_MXC_CLK.
> So we have to avoid users to be able to config MXC_CLK to m for i.MX6&7.
> Only depends on ARCH_MXC mean user still can set it to m.

The link error can be easily avoided by building all the clk support into
a single loadable module like below.

Hower this only works if all drivers that have a runtime dependency
on the clk driver support deferred probing or are built as loadable
modules as well and get loaded after the clk driver.

     Arnd

8<---
diff --git a/drivers/clk/imx/Makefile b/drivers/clk/imx/Makefile
index 928f874c73d2..638bc00f5731 100644
--- a/drivers/clk/imx/Makefile
+++ b/drivers/clk/imx/Makefile
@@ -1,6 +1,8 @@
 # SPDX-License-Identifier: GPL-2.0

-obj-$(CONFIG_MXC_CLK) += \
+obj-$(CONFIG_MXC_CLK) := clk-imx.ko
+
+clk-imx-y += \
        clk.o \
        clk-busy.o \
        clk-composite-8m.o \
@@ -25,24 +27,24 @@ obj-$(CONFIG_MXC_CLK_SCU) += \
        clk-scu.o \
        clk-lpcg-scu.o

-obj-$(CONFIG_CLK_IMX8MM) += clk-imx8mm.o
-obj-$(CONFIG_CLK_IMX8MN) += clk-imx8mn.o
-obj-$(CONFIG_CLK_IMX8MP) += clk-imx8mp.o
-obj-$(CONFIG_CLK_IMX8MQ) += clk-imx8mq.o
-obj-$(CONFIG_CLK_IMX8QXP) += clk-imx8qxp.o clk-imx8qxp-lpcg.o
+clk-imx-$(CONFIG_CLK_IMX8MM) += clk-imx8mm.o
+clk-imx-$(CONFIG_CLK_IMX8MN) += clk-imx8mn.o
+clk-imx-$(CONFIG_CLK_IMX8MP) += clk-imx8mp.o
+clk-imx-$(CONFIG_CLK_IMX8MQ) += clk-imx8mq.o
+clk-imx-$(CONFIG_CLK_IMX8QXP) += clk-imx8qxp.o clk-imx8qxp-lpcg.o

-obj-$(CONFIG_SOC_IMX1)   += clk-imx1.o
-obj-$(CONFIG_SOC_IMX21)  += clk-imx21.o
-obj-$(CONFIG_SOC_IMX25)  += clk-imx25.o
-obj-$(CONFIG_SOC_IMX27)  += clk-imx27.o
-obj-$(CONFIG_SOC_IMX31)  += clk-imx31.o
-obj-$(CONFIG_SOC_IMX35)  += clk-imx35.o
-obj-$(CONFIG_SOC_IMX5)   += clk-imx5.o
-obj-$(CONFIG_SOC_IMX6Q)  += clk-imx6q.o
-obj-$(CONFIG_SOC_IMX6SL) += clk-imx6sl.o
-obj-$(CONFIG_SOC_IMX6SLL) += clk-imx6sll.o
-obj-$(CONFIG_SOC_IMX6SX) += clk-imx6sx.o
-obj-$(CONFIG_SOC_IMX6UL) += clk-imx6ul.o
-obj-$(CONFIG_SOC_IMX7D)  += clk-imx7d.o
-obj-$(CONFIG_SOC_IMX7ULP) += clk-imx7ulp.o
-obj-$(CONFIG_SOC_VF610)  += clk-vf610.o
+clk-imx-$(CONFIG_SOC_IMX1)   += clk-imx1.o
+clk-imx-$(CONFIG_SOC_IMX21)  += clk-imx21.o
+clk-imx-$(CONFIG_SOC_IMX25)  += clk-imx25.o
+clk-imx-$(CONFIG_SOC_IMX27)  += clk-imx27.o
+clk-imx-$(CONFIG_SOC_IMX31)  += clk-imx31.o
+clk-imx-$(CONFIG_SOC_IMX35)  += clk-imx35.o
+clk-imx-$(CONFIG_SOC_IMX5)   += clk-imx5.o
+clk-imx-$(CONFIG_SOC_IMX6Q)  += clk-imx6q.o
+clk-imx-$(CONFIG_SOC_IMX6SL) += clk-imx6sl.o
+clk-imx-$(CONFIG_SOC_IMX6SLL) += clk-imx6sll.o
+clk-imx-$(CONFIG_SOC_IMX6SX) += clk-imx6sx.o
+clk-imx-$(CONFIG_SOC_IMX6UL) += clk-imx6ul.o
+clk-imx-$(CONFIG_SOC_IMX7D)  += clk-imx7d.o
+clk-imx-$(CONFIG_SOC_IMX7ULP) += clk-imx7ulp.o
+clk-imx-$(CONFIG_SOC_VF610)  += clk-vf610.o

Aisheng Dong June 24, 2020, 9:18 a.m. UTC | #13

> From: Arnd Bergmann <[e-mail]>
> Sent: Wednesday, June 24, 2020 3:47 PM
> 
> On Wed, Jun 24, 2020 at 4:19 AM Aisheng Dong <[e-mail]>
> wrote:
> > > Isn't that what we want?
> >
> > No, if user set MXC_CLK to m, the build will break for i.MX6&7.
> >
> > > Why does ARCH_MXC being enabled mandate that it is builtin? Is some
> > > architecture level code calling into the clk driver?
> >
> >
> > It's mainly because there's no Kconfig options for i.MX6 &7 clock drivers.
> > It just reuses ARCH config CONFIG_SOC_XXX which can only be y.
> > e.g.
> > obj-$(CONFIG_SOC_IMX6Q)  += clk-imx6q.o
> > obj-$(CONFIG_SOC_IMX6SL) += clk-imx6sl.o
> > obj-$(CONFIG_SOC_IMX7ULP) += clk-imx7ulp.o
> > obj-$(CONFIG_SOC_VF610)  += clk-vf610.o ..
> >
> > If setting MXC_CLK to m, the platform clock drivers will fail to build
> > due to miss to find symbols defined in the common clock library by
> CONFIG_MXC_CLK.
> > So we have to avoid users to be able to config MXC_CLK to m for i.MX6&7.
> > Only depends on ARCH_MXC mean user still can set it to m.
> 
> The link error can be easily avoided by building all the clk support into a single
> loadable module like below.
> 
> Hower this only works if all drivers that have a runtime dependency on the clk
> driver support deferred probing or are built as loadable modules as well and get
> loaded after the clk driver.
> 

Thanks for the info.
Currently all i.MX6&7/VF clocks driver are not loadable modules (they're using CLK_OF_DECLARE),
so can't be built as m.

If we really want to build i.MX common clock libraries into module, I guess the easiest way
Is as what Patch [2/9] does, select it by ARCH_MXC.
Then users have no chance to build it as module, so no build issues.

Regards
Aisheng

>      Arnd
> 
> 8<---
> diff --git a/drivers/clk/imx/Makefile b/drivers/clk/imx/Makefile index
> 928f874c73d2..638bc00f5731 100644
> --- a/drivers/clk/imx/Makefile
> +++ b/drivers/clk/imx/Makefile
> @@ -1,6 +1,8 @@
>  # SPDX-License-Identifier: GPL-2.0
> 
> -obj-$(CONFIG_MXC_CLK) += \
> +obj-$(CONFIG_MXC_CLK) := clk-imx.ko
> +
> +clk-imx-y += \
>         clk.o \
>         clk-busy.o \
>         clk-composite-8m.o \
> @@ -25,24 +27,24 @@ obj-$(CONFIG_MXC_CLK_SCU) += \
>         clk-scu.o \
>         clk-lpcg-scu.o
> 
> -obj-$(CONFIG_CLK_IMX8MM) += clk-imx8mm.o
> -obj-$(CONFIG_CLK_IMX8MN) += clk-imx8mn.o
> -obj-$(CONFIG_CLK_IMX8MP) += clk-imx8mp.o
> -obj-$(CONFIG_CLK_IMX8MQ) += clk-imx8mq.o
> -obj-$(CONFIG_CLK_IMX8QXP) += clk-imx8qxp.o clk-imx8qxp-lpcg.o
> +clk-imx-$(CONFIG_CLK_IMX8MM) += clk-imx8mm.o
> +clk-imx-$(CONFIG_CLK_IMX8MN) += clk-imx8mn.o
> +clk-imx-$(CONFIG_CLK_IMX8MP) += clk-imx8mp.o
> +clk-imx-$(CONFIG_CLK_IMX8MQ) += clk-imx8mq.o
> +clk-imx-$(CONFIG_CLK_IMX8QXP) += clk-imx8qxp.o clk-imx8qxp-lpcg.o
> 
> -obj-$(CONFIG_SOC_IMX1)   += clk-imx1.o
> -obj-$(CONFIG_SOC_IMX21)  += clk-imx21.o
> -obj-$(CONFIG_SOC_IMX25)  += clk-imx25.o
> -obj-$(CONFIG_SOC_IMX27)  += clk-imx27.o
> -obj-$(CONFIG_SOC_IMX31)  += clk-imx31.o
> -obj-$(CONFIG_SOC_IMX35)  += clk-imx35.o
> -obj-$(CONFIG_SOC_IMX5)   += clk-imx5.o
> -obj-$(CONFIG_SOC_IMX6Q)  += clk-imx6q.o
> -obj-$(CONFIG_SOC_IMX6SL) += clk-imx6sl.o
> -obj-$(CONFIG_SOC_IMX6SLL) += clk-imx6sll.o
> -obj-$(CONFIG_SOC_IMX6SX) += clk-imx6sx.o
> -obj-$(CONFIG_SOC_IMX6UL) += clk-imx6ul.o
> -obj-$(CONFIG_SOC_IMX7D)  += clk-imx7d.o
> -obj-$(CONFIG_SOC_IMX7ULP) += clk-imx7ulp.o
> -obj-$(CONFIG_SOC_VF610)  += clk-vf610.o
> +clk-imx-$(CONFIG_SOC_IMX1)   += clk-imx1.o
> +clk-imx-$(CONFIG_SOC_IMX21)  += clk-imx21.o
> +clk-imx-$(CONFIG_SOC_IMX25)  += clk-imx25.o
> +clk-imx-$(CONFIG_SOC_IMX27)  += clk-imx27.o
> +clk-imx-$(CONFIG_SOC_IMX31)  += clk-imx31.o
> +clk-imx-$(CONFIG_SOC_IMX35)  += clk-imx35.o
> +clk-imx-$(CONFIG_SOC_IMX5)   += clk-imx5.o
> +clk-imx-$(CONFIG_SOC_IMX6Q)  += clk-imx6q.o
> +clk-imx-$(CONFIG_SOC_IMX6SL) += clk-imx6sl.o
> +clk-imx-$(CONFIG_SOC_IMX6SLL) += clk-imx6sll.o
> +clk-imx-$(CONFIG_SOC_IMX6SX) += clk-imx6sx.o
> +clk-imx-$(CONFIG_SOC_IMX6UL) += clk-imx6ul.o
> +clk-imx-$(CONFIG_SOC_IMX7D)  += clk-imx7d.o
> +clk-imx-$(CONFIG_SOC_IMX7ULP) += clk-imx7ulp.o
> +clk-imx-$(CONFIG_SOC_VF610)  += clk-vf610.o

Stephen Boyd June 24, 2020, 10:43 p.m. UTC | #14

Quoting Aisheng Dong (2020-06-23 19:59:09)
> > > > > -       bool
> > > > > -       def_bool ARCH_MXC
> > > > > +       tristate "IMX clock"
> > > > > +       depends on ARCH_MXC
> > > > >
> > > > > But user can still set MXC_CLK to be m, either via make menuconfig
> > > > > or
> > > > defconfig.
> > > >
> > > > Isn't that what we want?
> > >
> > > No, if user set MXC_CLK to m, the build will break for i.MX6&7.
> > >
> > > > Why does ARCH_MXC being enabled mandate that it is builtin? Is some
> > > > architecture level code calling into the clk driver?
> > >
> > >
> > > It's mainly because there's no Kconfig options for i.MX6 &7 clock drivers.
> > > It just reuses ARCH config CONFIG_SOC_XXX which can only be y.
> > > e.g.
> > > obj-$(CONFIG_SOC_IMX6Q)  += clk-imx6q.o
> > > obj-$(CONFIG_SOC_IMX6SL) += clk-imx6sl.o
> > > obj-$(CONFIG_SOC_IMX7ULP) += clk-imx7ulp.o
> > > obj-$(CONFIG_SOC_VF610)  += clk-vf610.o ..
> > >
> > > If setting MXC_CLK to m, the platform clock drivers will fail to build
> > > due to miss to find symbols defined in the common clock library by
> > > CONFIG_MXC_CLK.
> > > So we have to avoid users to be able to config MXC_CLK to m for i.MX6&7.
> > > Only depends on ARCH_MXC mean user still can set it to m.
> > 
> > I think for i.MX6/7, although MXC_CLK is tristate, but it is selected by
> > ARCH_MXC which is always "y", so MXC_CLK can ONLY be "y" even it is explicitly
> > set to "m" in imx_v6_v7_defconfig file. So that means MXC_CLK can ONLY
> > support built-in for i.MX6/7 SoCs, and that is what we want?
> > 
> 
> Yes, I'm trying to explain to Stephen why we have to select MXC_CLK in ARCH_MXC
> And what issues we will met if not select it.
> 

Why aren't there options to enable clk-imx6q and clk-imx6sl in the
clk/imx/Kconfig file? Those can be bool or tristate depending on if the
SoC drivers use CLK_OF_DECLARE or not and depend on the mxc-clk library
and SoC config we have in the makefile today.

Dong Aisheng June 29, 2020, 7:04 a.m. UTC | #15

On Thu, Jun 25, 2020 at 6:43 AM Stephen Boyd <[e-mail]> wrote:
>
> Quoting Aisheng Dong (2020-06-23 19:59:09)
> > > > > > -       bool
> > > > > > -       def_bool ARCH_MXC
> > > > > > +       tristate "IMX clock"
> > > > > > +       depends on ARCH_MXC
> > > > > >
> > > > > > But user can still set MXC_CLK to be m, either via make menuconfig
> > > > > > or
> > > > > defconfig.
> > > > >
> > > > > Isn't that what we want?
> > > >
> > > > No, if user set MXC_CLK to m, the build will break for i.MX6&7.
> > > >
> > > > > Why does ARCH_MXC being enabled mandate that it is builtin? Is some
> > > > > architecture level code calling into the clk driver?
> > > >
> > > >
> > > > It's mainly because there's no Kconfig options for i.MX6 &7 clock drivers.
> > > > It just reuses ARCH config CONFIG_SOC_XXX which can only be y.
> > > > e.g.
> > > > obj-$(CONFIG_SOC_IMX6Q)  += clk-imx6q.o
> > > > obj-$(CONFIG_SOC_IMX6SL) += clk-imx6sl.o
> > > > obj-$(CONFIG_SOC_IMX7ULP) += clk-imx7ulp.o
> > > > obj-$(CONFIG_SOC_VF610)  += clk-vf610.o ..
> > > >
> > > > If setting MXC_CLK to m, the platform clock drivers will fail to build
> > > > due to miss to find symbols defined in the common clock library by
> > > > CONFIG_MXC_CLK.
> > > > So we have to avoid users to be able to config MXC_CLK to m for i.MX6&7.
> > > > Only depends on ARCH_MXC mean user still can set it to m.
> > >
> > > I think for i.MX6/7, although MXC_CLK is tristate, but it is selected by
> > > ARCH_MXC which is always "y", so MXC_CLK can ONLY be "y" even it is explicitly
> > > set to "m" in imx_v6_v7_defconfig file. So that means MXC_CLK can ONLY
> > > support built-in for i.MX6/7 SoCs, and that is what we want?
> > >
> >
> > Yes, I'm trying to explain to Stephen why we have to select MXC_CLK in ARCH_MXC
> > And what issues we will met if not select it.
> >
>
> Why aren't there options to enable clk-imx6q and clk-imx6sl in the
> clk/imx/Kconfig file? Those can be bool or tristate depending on if the
> SoC drivers use CLK_OF_DECLARE or not and depend on the mxc-clk library
> and SoC config we have in the makefile today.

Yes, we can do that in clk/imx/Kconfig as follows theoretically.
config CLK_IMX6Q
        bool
        def_bool ARCH_MXC && ARM
        select MXC_CLK

But we have totally 15 platforms that need to change.
e.g.
drivers/clk/imx/Makefile
obj-$(CONFIG_SOC_IMX1)   += clk-imx1.o
obj-$(CONFIG_SOC_IMX21)  += clk-imx21.o
obj-$(CONFIG_SOC_IMX25)  += clk-imx25.o
obj-$(CONFIG_SOC_IMX27)  += clk-imx27.o
obj-$(CONFIG_SOC_IMX31)  += clk-imx31.o
obj-$(CONFIG_SOC_IMX35)  += clk-imx35.o
obj-$(CONFIG_SOC_IMX5)   += clk-imx5.o
obj-$(CONFIG_SOC_IMX6Q)  += clk-imx6q.o
obj-$(CONFIG_SOC_IMX6SL) += clk-imx6sl.o
obj-$(CONFIG_SOC_IMX6SLL) += clk-imx6sll.o
obj-$(CONFIG_SOC_IMX6SX) += clk-imx6sx.o
obj-$(CONFIG_SOC_IMX6UL) += clk-imx6ul.o
obj-$(CONFIG_SOC_IMX7D)  += clk-imx7d.o
obj-$(CONFIG_SOC_IMX7ULP) += clk-imx7ulp.o
obj-$(CONFIG_SOC_VF610)  += clk-vf610.o

Not sure if it's really worth to do that.
The easiest way to address this issue is just select it in
arch/arm/mach-imx/Kconfig,
then no need to change those 15 clock config options or just builtin
clk libraries.

Stephen,
which one do you prefer?

Regards
Aisheng

Arnd Bergmann June 29, 2020, 8:19 a.m. UTC | #16

On Mon, Jun 29, 2020 at 9:18 AM Dong Aisheng <[e-mail]> wrote:
> On Thu, Jun 25, 2020 at 6:43 AM Stephen Boyd <[e-mail]> wrote:
> > Quoting Aisheng Dong (2020-06-23 19:59:09)
> > Why aren't there options to enable clk-imx6q and clk-imx6sl in the
> > clk/imx/Kconfig file? Those can be bool or tristate depending on if the
> > SoC drivers use CLK_OF_DECLARE or not and depend on the mxc-clk library
> > and SoC config we have in the makefile today.
>
> Yes, we can do that in clk/imx/Kconfig as follows theoretically.
> config CLK_IMX6Q
>         bool
>         def_bool ARCH_MXC && ARM
>         select MXC_CLK
>
> But we have totally 15 platforms that need to change.

I would make that

config CLK_IMX6Q
         bool "Clock driver for NXP i.MX6Q"
         depends on SOC_IMX6Q || COMPILE_TEST
         default SOC_IMX6Q
         select MXC_CLK

> e.g.
> drivers/clk/imx/Makefile
> obj-$(CONFIG_SOC_IMX1)   += clk-imx1.o
> obj-$(CONFIG_SOC_IMX21)  += clk-imx21.o
> obj-$(CONFIG_SOC_IMX25)  += clk-imx25.o
> obj-$(CONFIG_SOC_IMX27)  += clk-imx27.o
> obj-$(CONFIG_SOC_IMX31)  += clk-imx31.o
> obj-$(CONFIG_SOC_IMX35)  += clk-imx35.o
> obj-$(CONFIG_SOC_IMX5)   += clk-imx5.o
> obj-$(CONFIG_SOC_IMX6Q)  += clk-imx6q.o
> obj-$(CONFIG_SOC_IMX6SL) += clk-imx6sl.o
> obj-$(CONFIG_SOC_IMX6SLL) += clk-imx6sll.o
> obj-$(CONFIG_SOC_IMX6SX) += clk-imx6sx.o
> obj-$(CONFIG_SOC_IMX6UL) += clk-imx6ul.o
> obj-$(CONFIG_SOC_IMX7D)  += clk-imx7d.o
> obj-$(CONFIG_SOC_IMX7ULP) += clk-imx7ulp.o
> obj-$(CONFIG_SOC_VF610)  += clk-vf610.o
>
> Not sure if it's really worth to do that.
> The easiest way to address this issue is just select it in
> arch/arm/mach-imx/Kconfig,

Changing them can be a one or two patches, that's totally
worth it IMHO.

I really don't like the 'select' in arch/arm/mach-imx/Kconfig: if
you've done the work to make the imx8 clk driver modular,
I would expect to see the same at least tried for the other
ones.

For the clk drivers that cannot yet be 'tristate' because of the
CLK_OF_DECLARE(), can you include a list of drivers
that depend on the clocks being available during early
boot?

       Arnd

Stephen Boyd June 29, 2020, 9:05 p.m. UTC | #17

Quoting Arnd Bergmann (2020-06-29 01:19:44)
> On Mon, Jun 29, 2020 at 9:18 AM Dong Aisheng <[e-mail]> wrote:
> > On Thu, Jun 25, 2020 at 6:43 AM Stephen Boyd <[e-mail]> wrote:
> > > Quoting Aisheng Dong (2020-06-23 19:59:09)
> > > Why aren't there options to enable clk-imx6q and clk-imx6sl in the
> > > clk/imx/Kconfig file? Those can be bool or tristate depending on if the
> > > SoC drivers use CLK_OF_DECLARE or not and depend on the mxc-clk library
> > > and SoC config we have in the makefile today.
> >
> > Yes, we can do that in clk/imx/Kconfig as follows theoretically.
> > config CLK_IMX6Q
> >         bool
> >         def_bool ARCH_MXC && ARM
> >         select MXC_CLK
> >
> > But we have totally 15 platforms that need to change.
> 
> I would make that
> 
> config CLK_IMX6Q
>          bool "Clock driver for NXP i.MX6Q"
>          depends on SOC_IMX6Q || COMPILE_TEST
>          default SOC_IMX6Q
>          select MXC_CLK

Yes, do this.

> 
> > e.g.
> > drivers/clk/imx/Makefile
> > obj-$(CONFIG_SOC_IMX1)   += clk-imx1.o
> > obj-$(CONFIG_SOC_IMX21)  += clk-imx21.o
> > obj-$(CONFIG_SOC_IMX25)  += clk-imx25.o
> > obj-$(CONFIG_SOC_IMX27)  += clk-imx27.o
> > obj-$(CONFIG_SOC_IMX31)  += clk-imx31.o
> > obj-$(CONFIG_SOC_IMX35)  += clk-imx35.o
> > obj-$(CONFIG_SOC_IMX5)   += clk-imx5.o
> > obj-$(CONFIG_SOC_IMX6Q)  += clk-imx6q.o
> > obj-$(CONFIG_SOC_IMX6SL) += clk-imx6sl.o
> > obj-$(CONFIG_SOC_IMX6SLL) += clk-imx6sll.o
> > obj-$(CONFIG_SOC_IMX6SX) += clk-imx6sx.o
> > obj-$(CONFIG_SOC_IMX6UL) += clk-imx6ul.o
> > obj-$(CONFIG_SOC_IMX7D)  += clk-imx7d.o
> > obj-$(CONFIG_SOC_IMX7ULP) += clk-imx7ulp.o
> > obj-$(CONFIG_SOC_VF610)  += clk-vf610.o
> >
> > Not sure if it's really worth to do that.
> > The easiest way to address this issue is just select it in
> > arch/arm/mach-imx/Kconfig,
> 
> Changing them can be a one or two patches, that's totally
> worth it IMHO.

Agreed.

> 
> I really don't like the 'select' in arch/arm/mach-imx/Kconfig: if
> you've done the work to make the imx8 clk driver modular,
> I would expect to see the same at least tried for the other
> ones.

Agreed.

> 
> For the clk drivers that cannot yet be 'tristate' because of the
> CLK_OF_DECLARE(), can you include a list of drivers
> that depend on the clocks being available during early
> boot?
>

Aisheng Dong June 30, 2020, 3:01 a.m. UTC | #18

> From: Arnd Bergmann <[e-mail]>
> Sent: Monday, June 29, 2020 4:20 PM
> 
> On Mon, Jun 29, 2020 at 9:18 AM Dong Aisheng <[e-mail]>
> wrote:
> > On Thu, Jun 25, 2020 at 6:43 AM Stephen Boyd <[e-mail]> wrote:
> > > Quoting Aisheng Dong (2020-06-23 19:59:09) Why aren't there options
> > > to enable clk-imx6q and clk-imx6sl in the clk/imx/Kconfig file?
> > > Those can be bool or tristate depending on if the SoC drivers use
> > > CLK_OF_DECLARE or not and depend on the mxc-clk library and SoC
> > > config we have in the makefile today.
> >
> > Yes, we can do that in clk/imx/Kconfig as follows theoretically.
> > config CLK_IMX6Q
> >         bool
> >         def_bool ARCH_MXC && ARM
> >         select MXC_CLK
> >
> > But we have totally 15 platforms that need to change.
> 
> I would make that
> 
> config CLK_IMX6Q
>          bool "Clock driver for NXP i.MX6Q"
>          depends on SOC_IMX6Q || COMPILE_TEST
>          default SOC_IMX6Q
>          select MXC_CLK
> 

Yes, this seems better.
Anson, pls follow this way.

> > e.g.
> > drivers/clk/imx/Makefile
> > obj-$(CONFIG_SOC_IMX1)   += clk-imx1.o
> > obj-$(CONFIG_SOC_IMX21)  += clk-imx21.o
> > obj-$(CONFIG_SOC_IMX25)  += clk-imx25.o
> > obj-$(CONFIG_SOC_IMX27)  += clk-imx27.o
> > obj-$(CONFIG_SOC_IMX31)  += clk-imx31.o
> > obj-$(CONFIG_SOC_IMX35)  += clk-imx35.o
> > obj-$(CONFIG_SOC_IMX5)   += clk-imx5.o
> > obj-$(CONFIG_SOC_IMX6Q)  += clk-imx6q.o
> > obj-$(CONFIG_SOC_IMX6SL) += clk-imx6sl.o
> > obj-$(CONFIG_SOC_IMX6SLL) += clk-imx6sll.o
> > obj-$(CONFIG_SOC_IMX6SX) += clk-imx6sx.o
> > obj-$(CONFIG_SOC_IMX6UL) += clk-imx6ul.o
> > obj-$(CONFIG_SOC_IMX7D)  += clk-imx7d.o
> > obj-$(CONFIG_SOC_IMX7ULP) += clk-imx7ulp.o
> > obj-$(CONFIG_SOC_VF610)  += clk-vf610.o
> >
> > Not sure if it's really worth to do that.
> > The easiest way to address this issue is just select it in
> > arch/arm/mach-imx/Kconfig,
> 
> Changing them can be a one or two patches, that's totally worth it IMHO.
> 
> I really don't like the 'select' in arch/arm/mach-imx/Kconfig: if you've done the
> work to make the imx8 clk driver modular, I would expect to see the same at
> least tried for the other ones.
> 

Got it.

> For the clk drivers that cannot yet be 'tristate' because of the
> CLK_OF_DECLARE(), can you include a list of drivers that depend on the clocks
> being available during early boot?
> 

I guess we can find out them one by one for those 15 platforms when converting them
to modules as well. Currently we're converting ARM64 clock drivers first.

Regards
Aisheng

>        Arnd

Anson Huang June 30, 2020, 4:41 a.m. UTC | #19

> Subject: RE: [PATCH V2 3/9] clk: imx: Support building SCU clock driver as
> module
> 
> > From: Arnd Bergmann <[e-mail]>
> > Sent: Monday, June 29, 2020 4:20 PM
> >
> > On Mon, Jun 29, 2020 at 9:18 AM Dong Aisheng <[e-mail]>
> > wrote:
> > > On Thu, Jun 25, 2020 at 6:43 AM Stephen Boyd <[e-mail]>
> wrote:
> > > > Quoting Aisheng Dong (2020-06-23 19:59:09) Why aren't there
> > > > options to enable clk-imx6q and clk-imx6sl in the clk/imx/Kconfig file?
> > > > Those can be bool or tristate depending on if the SoC drivers use
> > > > CLK_OF_DECLARE or not and depend on the mxc-clk library and SoC
> > > > config we have in the makefile today.
> > >
> > > Yes, we can do that in clk/imx/Kconfig as follows theoretically.
> > > config CLK_IMX6Q
> > >         bool
> > >         def_bool ARCH_MXC && ARM
> > >         select MXC_CLK
> > >
> > > But we have totally 15 platforms that need to change.
> >
> > I would make that
> >
> > config CLK_IMX6Q
> >          bool "Clock driver for NXP i.MX6Q"
> >          depends on SOC_IMX6Q || COMPILE_TEST
> >          default SOC_IMX6Q
> >          select MXC_CLK
> >
> 
> Yes, this seems better.
> Anson, pls follow this way.

In V4 patch series, I will add a patch to introduce CLK_IMX6Q and other i.MX6/7
clock config following this way.

Anson

Anson Huang July 2, 2020, 6:15 a.m. UTC | #20

Hi, Arnd/Stephen

> Subject: Re: [PATCH V2 3/9] clk: imx: Support building SCU clock driver as
> module
> 
> Quoting Arnd Bergmann (2020-06-29 01:19:44)
> > On Mon, Jun 29, 2020 at 9:18 AM Dong Aisheng <[e-mail]>
> wrote:
> > > On Thu, Jun 25, 2020 at 6:43 AM Stephen Boyd <[e-mail]>
> wrote:
> > > > Quoting Aisheng Dong (2020-06-23 19:59:09) Why aren't there
> > > > options to enable clk-imx6q and clk-imx6sl in the clk/imx/Kconfig
> > > > file? Those can be bool or tristate depending on if the SoC
> > > > drivers use CLK_OF_DECLARE or not and depend on the mxc-clk
> > > > library and SoC config we have in the makefile today.
> > >
> > > Yes, we can do that in clk/imx/Kconfig as follows theoretically.
> > > config CLK_IMX6Q
> > >         bool
> > >         def_bool ARCH_MXC && ARM
> > >         select MXC_CLK
> > >
> > > But we have totally 15 platforms that need to change.
> >
> > I would make that
> >
> > config CLK_IMX6Q
> >          bool "Clock driver for NXP i.MX6Q"
> >          depends on SOC_IMX6Q || COMPILE_TEST
> >          default SOC_IMX6Q
> >          select MXC_CLK
> 
> Yes, do this.

The COMPILE_TEST existing on the config will introduce some build error
on other ARCH's allyesconfig build on i.MX2/3 platforms, I received some build error report.
So in next patch series, I will drop the COMPILE_TEST for these new added clock configurations,
as I think it is NOT worth to support the COMPILE_TEST for these old legacy platforms which are
NOT supported before.

Thanks,
Anson

Patch

diff --git a/drivers/clk/imx/Kconfig b/drivers/clk/imx/Kconfig
index db0253f..ded0643 100644
--- a/drivers/clk/imx/Kconfig
+++ b/drivers/clk/imx/Kconfig
@@ -5,8 +5,8 @@  config MXC_CLK
 	def_bool ARCH_MXC
 
 config MXC_CLK_SCU
-	bool
-	depends on IMX_SCU
+	tristate "IMX SCU clock"
+	depends on ARCH_MXC && IMX_SCU
 
 config CLK_IMX8MM
 	bool "IMX8MM CCM Clock Driver"
diff --git a/drivers/clk/imx/Makefile b/drivers/clk/imx/Makefile
index 928f874..1af8cff 100644
--- a/drivers/clk/imx/Makefile
+++ b/drivers/clk/imx/Makefile
@@ -21,9 +21,8 @@  obj-$(CONFIG_MXC_CLK) += \
 	clk-sscg-pll.o \
 	clk-pll14xx.o
 
-obj-$(CONFIG_MXC_CLK_SCU) += \
-	clk-scu.o \
-	clk-lpcg-scu.o
+mxc-clk-scu-objs += clk-scu.o clk-lpcg-scu.o
+obj-$(CONFIG_MXC_CLK_SCU) += mxc-clk-scu.o
 
 obj-$(CONFIG_CLK_IMX8MM) += clk-imx8mm.o
 obj-$(CONFIG_CLK_IMX8MN) += clk-imx8mn.o
diff --git a/drivers/clk/imx/clk-lpcg-scu.c b/drivers/clk/imx/clk-lpcg-scu.c
index a73a799..8177f0e 100644
--- a/drivers/clk/imx/clk-lpcg-scu.c
+++ b/drivers/clk/imx/clk-lpcg-scu.c
@@ -114,3 +114,4 @@  struct clk_hw *imx_clk_lpcg_scu(const char *name, const char *parent_name,
 
 	return hw;
 }
+EXPORT_SYMBOL_GPL(imx_clk_lpcg_scu);
diff --git a/drivers/clk/imx/clk-scu.c b/drivers/clk/imx/clk-scu.c
index b8b2072..9688981 100644
--- a/drivers/clk/imx/clk-scu.c
+++ b/drivers/clk/imx/clk-scu.c
@@ -8,6 +8,7 @@ 
 #include <linux/arm-smccc.h>
 #include <linux/clk-provider.h>
 #include <linux/err.h>
+#include <linux/module.h>
 #include <linux/slab.h>
 
 #include "clk-scu.h"
@@ -132,6 +133,7 @@  int imx_clk_scu_init(void)
 {
 	return imx_scu_get_handle(&ccm_ipc_handle);
 }
+EXPORT_SYMBOL_GPL(imx_clk_scu_init);
 
 /*
  * clk_scu_recalc_rate - Get clock rate for a SCU clock
@@ -387,3 +389,6 @@  struct clk_hw *__imx_clk_scu(const char *name, const char * const *parents,
 
 	return hw;
 }
+EXPORT_SYMBOL_GPL(__imx_clk_scu);
+
+MODULE_LICENSE("GPL v2");