linux-clk.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH V3 00/10] Support building i.MX8 SoCs clock driver as module
@ 2020-06-29  5:53 Anson Huang
  2020-06-29  5:53 ` [PATCH V3 01/10] clk: composite: Export clk_hw_register_composite() Anson Huang
                   ` (9 more replies)
  0 siblings, 10 replies; 39+ messages in thread
From: Anson Huang @ 2020-06-29  5:53 UTC (permalink / raw)
  To: linux, shawnguo, s.hauer, kernel, festevam, mturquette, sboyd,
	oleksandr.suvorov, stefan.agner, arnd, peng.fan, abel.vesa,
	aisheng.dong, fugang.duan, daniel.baluta, yuehaibing, sfr, viro,
	linux-arm-kernel, linux-kernel, linux-clk
  Cc: Linux-imx

Nowdays, there are more and more requirements of building SoC specific drivers
as modules, such as Android GKI (generic kernel image), this patch set supports
building i.MX8 SoCs clock drivers as modules, including i.MX8MQ/MM/MN/MP/QXP,
the common clock modules are: mxc-clk.ko for i.MX8MQ/MM/MN/MP, mxc-clk-scu.ko
for i.MX8QXP and later SoCs with SCU inside, normally, each platform can ONLY
insmod 1 common i.MX clock driver and its own SoC clock driver.

Since i.MX common clk driver will support module build and no longer selected
by default, so for i.MX ARMv7 platforms, need to manually select it to make
build pass.

Changes since V2:
	- fix __setup_param() instead of handling module build inside clk driver;
	- improve makefile format to include each file in separated line;
	- add linux/export.h where necessary.

Anson Huang (10):
  clk: composite: Export clk_hw_register_composite()
  init.h: Fix the __setup_param() macro for module build
  ARM: imx: Select MXC_CLK for each SoC
  clk: imx: Support building SCU clock driver as module
  clk: imx: Support building i.MX common clock driver as module
  clk: imx8mm: Support module build
  clk: imx8mn: Support module build
  clk: imx8mp: Support module build
  clk: imx8mq: Support module build
  clk: imx8qxp: Support module build

 arch/arm/mach-imx/Kconfig          | 11 +++++++++
 drivers/clk/clk-composite.c        |  1 +
 drivers/clk/imx/Kconfig            | 22 ++++++++++--------
 drivers/clk/imx/Makefile           | 46 +++++++++++++++++++-------------------
 drivers/clk/imx/clk-composite-8m.c |  2 ++
 drivers/clk/imx/clk-cpu.c          |  2 ++
 drivers/clk/imx/clk-frac-pll.c     |  2 ++
 drivers/clk/imx/clk-gate2.c        |  2 ++
 drivers/clk/imx/clk-imx8mm.c       |  1 +
 drivers/clk/imx/clk-imx8mn.c       |  1 +
 drivers/clk/imx/clk-imx8mp.c       |  1 +
 drivers/clk/imx/clk-imx8mq.c       |  1 +
 drivers/clk/imx/clk-imx8qxp-lpcg.c |  1 +
 drivers/clk/imx/clk-imx8qxp.c      |  1 +
 drivers/clk/imx/clk-lpcg-scu.c     |  2 ++
 drivers/clk/imx/clk-pll14xx.c      |  5 +++++
 drivers/clk/imx/clk-scu.c          |  5 +++++
 drivers/clk/imx/clk-sscg-pll.c     |  2 ++
 drivers/clk/imx/clk.c              | 20 ++++++++++++-----
 include/linux/init.h               |  2 +-
 20 files changed, 91 insertions(+), 39 deletions(-)

-- 
2.7.4


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH V3 01/10] clk: composite: Export clk_hw_register_composite()
  2020-06-29  5:53 [PATCH V3 00/10] Support building i.MX8 SoCs clock driver as module Anson Huang
@ 2020-06-29  5:53 ` Anson Huang
  2020-06-29  5:53 ` [PATCH V3 02/10] init.h: Fix the __setup_param() macro for module build Anson Huang
                   ` (8 subsequent siblings)
  9 siblings, 0 replies; 39+ messages in thread
From: Anson Huang @ 2020-06-29  5:53 UTC (permalink / raw)
  To: linux, shawnguo, s.hauer, kernel, festevam, mturquette, sboyd,
	oleksandr.suvorov, stefan.agner, arnd, peng.fan, abel.vesa,
	aisheng.dong, fugang.duan, daniel.baluta, yuehaibing, sfr, viro,
	linux-arm-kernel, linux-kernel, linux-clk
  Cc: Linux-imx

Export clk_hw_register_composite() to support user built as module.

ERROR: modpost: "clk_hw_register_composite" [drivers/clk/imx/mxc-clk.ko] undefined!

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
Reviewed-by: Stephen Boyd <sboyd@kernel.org>
---
Changes since V2:
	- improve the commit message.
---
 drivers/clk/clk-composite.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c
index 7376f57..2ddb54f 100644
--- a/drivers/clk/clk-composite.c
+++ b/drivers/clk/clk-composite.c
@@ -328,6 +328,7 @@ struct clk_hw *clk_hw_register_composite(struct device *dev, const char *name,
 					   rate_hw, rate_ops, gate_hw,
 					   gate_ops, flags);
 }
+EXPORT_SYMBOL_GPL(clk_hw_register_composite);
 
 struct clk_hw *clk_hw_register_composite_pdata(struct device *dev,
 			const char *name,
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH V3 02/10] init.h: Fix the __setup_param() macro for module build
  2020-06-29  5:53 [PATCH V3 00/10] Support building i.MX8 SoCs clock driver as module Anson Huang
  2020-06-29  5:53 ` [PATCH V3 01/10] clk: composite: Export clk_hw_register_composite() Anson Huang
@ 2020-06-29  5:53 ` Anson Huang
  2020-06-29 11:33   ` Arnd Bergmann
  2020-06-29  5:53 ` [PATCH V3 03/10] ARM: imx: Select MXC_CLK for each SoC Anson Huang
                   ` (7 subsequent siblings)
  9 siblings, 1 reply; 39+ messages in thread
From: Anson Huang @ 2020-06-29  5:53 UTC (permalink / raw)
  To: linux, shawnguo, s.hauer, kernel, festevam, mturquette, sboyd,
	oleksandr.suvorov, stefan.agner, arnd, peng.fan, abel.vesa,
	aisheng.dong, fugang.duan, daniel.baluta, yuehaibing, sfr, viro,
	linux-arm-kernel, linux-kernel, linux-clk
  Cc: Linux-imx

Keep __setup_param() to use same parameters for both built in
and built as module, it can make the drivers which call it easier
when the drivers can be built in or built as module.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
new patch.
---
 include/linux/init.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/init.h b/include/linux/init.h
index 212fc9e..8f27299 100644
--- a/include/linux/init.h
+++ b/include/linux/init.h
@@ -293,7 +293,7 @@ void __init parse_early_options(char *cmdline);
 
 #else /* MODULE */
 
-#define __setup_param(str, unique_id, fn)	/* nothing */
+#define __setup_param(str, unique_id, fn, early)	/* nothing */
 #define __setup(str, func) 			/* nothing */
 #endif
 
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH V3 03/10] ARM: imx: Select MXC_CLK for each SoC
  2020-06-29  5:53 [PATCH V3 00/10] Support building i.MX8 SoCs clock driver as module Anson Huang
  2020-06-29  5:53 ` [PATCH V3 01/10] clk: composite: Export clk_hw_register_composite() Anson Huang
  2020-06-29  5:53 ` [PATCH V3 02/10] init.h: Fix the __setup_param() macro for module build Anson Huang
@ 2020-06-29  5:53 ` Anson Huang
  2020-06-29  7:27   ` Aisheng Dong
  2020-06-29  5:53 ` [PATCH V3 04/10] clk: imx: Support building SCU clock driver as module Anson Huang
                   ` (6 subsequent siblings)
  9 siblings, 1 reply; 39+ messages in thread
From: Anson Huang @ 2020-06-29  5:53 UTC (permalink / raw)
  To: linux, shawnguo, s.hauer, kernel, festevam, mturquette, sboyd,
	oleksandr.suvorov, stefan.agner, arnd, peng.fan, abel.vesa,
	aisheng.dong, fugang.duan, daniel.baluta, yuehaibing, sfr, viro,
	linux-arm-kernel, linux-kernel, linux-clk
  Cc: Linux-imx

i.MX common clock drivers may support module build, so it is NOT
selected by default, for ARCH_MXC ARMv7 platforms, need to select
it manually in each SoC to make build pass.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
Changes since V2:
	- manually select the MXC_CLK in each SoC instead of selecting it
	  for ARCH_MXC.
---
 arch/arm/mach-imx/Kconfig | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig
index e7d7b90..a465c0f 100644
--- a/arch/arm/mach-imx/Kconfig
+++ b/arch/arm/mach-imx/Kconfig
@@ -58,6 +58,7 @@ config SOC_IMX21
 	select CPU_ARM926T
 	select IMX_HAVE_IOMUX_V1
 	select MXC_AVIC
+	select MXC_CLK
 
 config SOC_IMX27
 	bool
@@ -65,17 +66,20 @@ config SOC_IMX27
 	select IMX_HAVE_IOMUX_V1
 	select MXC_AVIC
 	select PINCTRL_IMX27
+	select MXC_CLK
 
 config SOC_IMX31
 	bool
 	select CPU_V6
 	select MXC_AVIC
+	select MXC_CLK
 
 config SOC_IMX35
 	bool
 	select ARCH_MXC_IOMUX_V3
 	select MXC_AVIC
 	select PINCTRL_IMX35
+	select MXC_CLK
 
 if ARCH_MULTI_V5
 
@@ -419,6 +423,7 @@ config SOC_IMX1
 	select CPU_ARM920T
 	select MXC_AVIC
 	select PINCTRL_IMX1
+	select MXC_CLK
 	help
 	  This enables support for Freescale i.MX1 processor
 
@@ -432,6 +437,7 @@ config SOC_IMX25
 	select CPU_ARM926T
 	select MXC_AVIC
 	select PINCTRL_IMX25
+	select MXC_CLK
 	help
 	  This enables support for Freescale i.MX25 processor
 endif
@@ -444,6 +450,7 @@ config SOC_IMX5
 	bool
 	select HAVE_IMX_SRC
 	select MXC_TZIC
+	select MXC_CLK
 
 config	SOC_IMX50
 	bool "i.MX50 support"
@@ -477,6 +484,7 @@ config SOC_IMX6
 	select HAVE_IMX_MMDC
 	select HAVE_IMX_SRC
 	select MFD_SYSCON
+	select MXC_CLK
 	select PL310_ERRATA_769419 if CACHE_L2X0
 
 config SOC_IMX6Q
@@ -561,6 +569,7 @@ config SOC_IMX7D_CM4
 config SOC_IMX7D
 	bool "i.MX7 Dual support"
 	select PINCTRL_IMX7D
+	select MXC_CLK
 	select SOC_IMX7D_CA7 if ARCH_MULTI_V7
 	select SOC_IMX7D_CM4 if ARM_SINGLE_ARMV7M
 	select ARM_ERRATA_814220 if ARCH_MULTI_V7
@@ -571,6 +580,7 @@ config SOC_IMX7ULP
 	bool "i.MX7ULP support"
 	select CLKSRC_IMX_TPM
 	select PINCTRL_IMX7ULP
+	select MXC_CLK
 	select SOC_IMX7D_CA7 if ARCH_MULTI_V7
 	select SOC_IMX7D_CM4 if ARM_SINGLE_ARMV7M
 	help
@@ -580,6 +590,7 @@ config SOC_VF610
 	bool "Vybrid Family VF610 support"
 	select ARM_GIC if ARCH_MULTI_V7
 	select PINCTRL_VF610
+	select MXC_CLK
 
 	help
 	  This enables support for Freescale Vybrid VF610 processor.
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH V3 04/10] clk: imx: Support building SCU clock driver as module
  2020-06-29  5:53 [PATCH V3 00/10] Support building i.MX8 SoCs clock driver as module Anson Huang
                   ` (2 preceding siblings ...)
  2020-06-29  5:53 ` [PATCH V3 03/10] ARM: imx: Select MXC_CLK for each SoC Anson Huang
@ 2020-06-29  5:53 ` Anson Huang
  2020-06-29 11:37   ` Arnd Bergmann
  2020-06-29  5:53 ` [PATCH V3 05/10] clk: imx: Support building i.MX common " Anson Huang
                   ` (5 subsequent siblings)
  9 siblings, 1 reply; 39+ messages in thread
From: Anson Huang @ 2020-06-29  5:53 UTC (permalink / raw)
  To: linux, shawnguo, s.hauer, kernel, festevam, mturquette, sboyd,
	oleksandr.suvorov, stefan.agner, arnd, peng.fan, abel.vesa,
	aisheng.dong, fugang.duan, daniel.baluta, yuehaibing, sfr, viro,
	linux-arm-kernel, linux-kernel, linux-clk
  Cc: Linux-imx

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 <Anson.Huang@nxp.com>
---
Changes since V2:
	- use separated line for each file which is included for build;
	- include linux/export.h where necessary.
---
 drivers/clk/imx/Kconfig        | 4 ++--
 drivers/clk/imx/Makefile       | 6 +++---
 drivers/clk/imx/clk-lpcg-scu.c | 2 ++
 drivers/clk/imx/clk-scu.c      | 5 +++++
 4 files changed, 12 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
+	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..c6574a3 100644
--- a/drivers/clk/imx/Makefile
+++ b/drivers/clk/imx/Makefile
@@ -21,9 +21,9 @@ 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-lpcg-scu.o
+mxc-clk-scu-objs += clk-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..d4e78ca 100644
--- a/drivers/clk/imx/clk-lpcg-scu.c
+++ b/drivers/clk/imx/clk-lpcg-scu.c
@@ -6,6 +6,7 @@
 
 #include <linux/clk-provider.h>
 #include <linux/err.h>
+#include <linux/export.h>
 #include <linux/io.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
@@ -114,3 +115,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


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH V3 05/10] clk: imx: Support building i.MX common clock driver as module
  2020-06-29  5:53 [PATCH V3 00/10] Support building i.MX8 SoCs clock driver as module Anson Huang
                   ` (3 preceding siblings ...)
  2020-06-29  5:53 ` [PATCH V3 04/10] clk: imx: Support building SCU clock driver as module Anson Huang
@ 2020-06-29  5:53 ` Anson Huang
  2020-06-29  5:53 ` [PATCH V3 06/10] clk: imx8mm: Support module build Anson Huang
                   ` (4 subsequent siblings)
  9 siblings, 0 replies; 39+ messages in thread
From: Anson Huang @ 2020-06-29  5:53 UTC (permalink / raw)
  To: linux, shawnguo, s.hauer, kernel, festevam, mturquette, sboyd,
	oleksandr.suvorov, stefan.agner, arnd, peng.fan, abel.vesa,
	aisheng.dong, fugang.duan, daniel.baluta, yuehaibing, sfr, viro,
	linux-arm-kernel, linux-kernel, linux-clk
  Cc: Linux-imx

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

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
Changes since V2:
	- remove __setup_param() change for module build, it is already fix in
	  a new patch;
	- include linux/export.h where necessary;
	- use separate line for each file which is included for build.
---
 drivers/clk/imx/Kconfig            |  8 ++++++--
 drivers/clk/imx/Makefile           | 40 +++++++++++++++++++-------------------
 drivers/clk/imx/clk-composite-8m.c |  2 ++
 drivers/clk/imx/clk-cpu.c          |  2 ++
 drivers/clk/imx/clk-frac-pll.c     |  2 ++
 drivers/clk/imx/clk-gate2.c        |  2 ++
 drivers/clk/imx/clk-pll14xx.c      |  5 +++++
 drivers/clk/imx/clk-sscg-pll.c     |  2 ++
 drivers/clk/imx/clk.c              | 20 +++++++++++++------
 9 files changed, 55 insertions(+), 28 deletions(-)

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
 
 config MXC_CLK_SCU
 	tristate "IMX SCU clock"
@@ -11,24 +11,28 @@ config MXC_CLK_SCU
 config CLK_IMX8MM
 	bool "IMX8MM CCM Clock Driver"
 	depends on ARCH_MXC
+	select MXC_CLK
 	help
 	    Build the driver for i.MX8MM CCM Clock Driver
 
 config CLK_IMX8MN
 	bool "IMX8MN CCM Clock Driver"
 	depends on ARCH_MXC
+	select MXC_CLK
 	help
 	    Build the driver for i.MX8MN CCM Clock Driver
 
 config CLK_IMX8MP
 	bool "IMX8MP CCM Clock Driver"
 	depends on ARCH_MXC
+	select MXC_CLK
 	help
 	    Build the driver for i.MX8MP CCM Clock Driver
 
 config CLK_IMX8MQ
 	bool "IMX8MQ CCM Clock Driver"
 	depends on ARCH_MXC
+	select MXC_CLK
 	help
 	    Build the driver for i.MX8MQ CCM Clock Driver
 
diff --git a/drivers/clk/imx/Makefile b/drivers/clk/imx/Makefile
index c6574a3..38fb9d5 100644
--- a/drivers/clk/imx/Makefile
+++ b/drivers/clk/imx/Makefile
@@ -1,25 +1,25 @@
 # SPDX-License-Identifier: GPL-2.0
 
-obj-$(CONFIG_MXC_CLK) += \
-	clk.o \
-	clk-busy.o \
-	clk-composite-8m.o \
-	clk-cpu.o \
-	clk-composite-7ulp.o \
-	clk-divider-gate.o \
-	clk-fixup-div.o \
-	clk-fixup-mux.o \
-	clk-frac-pll.o \
-	clk-gate-exclusive.o \
-	clk-gate2.o \
-	clk-pfd.o \
-	clk-pfdv2.o \
-	clk-pllv1.o \
-	clk-pllv2.o \
-	clk-pllv3.o \
-	clk-pllv4.o \
-	clk-sscg-pll.o \
-	clk-pll14xx.o
+mxc-clk-objs += clk.o
+mxc-clk-objs += clk-busy.o
+mxc-clk-objs += clk-composite-7ulp.o
+mxc-clk-objs += clk-composite-8m.o
+mxc-clk-objs += clk-cpu.o
+mxc-clk-objs += clk-divider-gate.o
+mxc-clk-objs += clk-fixup-div.o
+mxc-clk-objs += clk-fixup-mux.o
+mxc-clk-objs += clk-frac-pll.o
+mxc-clk-objs += clk-gate2.o
+mxc-clk-objs += clk-gate-exclusive.o
+mxc-clk-objs += clk-pfd.o
+mxc-clk-objs += clk-pfdv2.o
+mxc-clk-objs += clk-pllv1.o
+mxc-clk-objs += clk-pllv2.o
+mxc-clk-objs += clk-pllv3.o
+mxc-clk-objs += clk-pllv4.o
+mxc-clk-objs += clk-pll14xx.o
+mxc-clk-objs += clk-sscg-pll.o
+obj-$(CONFIG_MXC_CLK) += mxc-clk.o
 
 mxc-clk-scu-objs += clk-lpcg-scu.o
 mxc-clk-scu-objs += clk-scu.o
diff --git a/drivers/clk/imx/clk-composite-8m.c b/drivers/clk/imx/clk-composite-8m.c
index d2b5af8..78fb7e5 100644
--- a/drivers/clk/imx/clk-composite-8m.c
+++ b/drivers/clk/imx/clk-composite-8m.c
@@ -5,6 +5,7 @@
 
 #include <linux/clk-provider.h>
 #include <linux/errno.h>
+#include <linux/export.h>
 #include <linux/io.h>
 #include <linux/slab.h>
 
@@ -243,3 +244,4 @@ struct clk_hw *imx8m_clk_hw_composite_flags(const char *name,
 	kfree(mux);
 	return ERR_CAST(hw);
 }
+EXPORT_SYMBOL_GPL(imx8m_clk_hw_composite_flags);
diff --git a/drivers/clk/imx/clk-cpu.c b/drivers/clk/imx/clk-cpu.c
index cb182be..cb6ca4c 100644
--- a/drivers/clk/imx/clk-cpu.c
+++ b/drivers/clk/imx/clk-cpu.c
@@ -5,6 +5,7 @@
 
 #include <linux/clk.h>
 #include <linux/clk-provider.h>
+#include <linux/export.h>
 #include <linux/slab.h>
 #include "clk.h"
 
@@ -104,3 +105,4 @@ struct clk_hw *imx_clk_hw_cpu(const char *name, const char *parent_name,
 
 	return hw;
 }
+EXPORT_SYMBOL_GPL(imx_clk_hw_cpu);
diff --git a/drivers/clk/imx/clk-frac-pll.c b/drivers/clk/imx/clk-frac-pll.c
index 101e0a3..c703056 100644
--- a/drivers/clk/imx/clk-frac-pll.c
+++ b/drivers/clk/imx/clk-frac-pll.c
@@ -10,6 +10,7 @@
 
 #include <linux/clk-provider.h>
 #include <linux/err.h>
+#include <linux/export.h>
 #include <linux/io.h>
 #include <linux/iopoll.h>
 #include <linux/slab.h>
@@ -233,3 +234,4 @@ struct clk_hw *imx_clk_hw_frac_pll(const char *name,
 
 	return hw;
 }
+EXPORT_SYMBOL_GPL(imx_clk_hw_frac_pll);
diff --git a/drivers/clk/imx/clk-gate2.c b/drivers/clk/imx/clk-gate2.c
index b87ab3c..512f675 100644
--- a/drivers/clk/imx/clk-gate2.c
+++ b/drivers/clk/imx/clk-gate2.c
@@ -7,6 +7,7 @@
  */
 
 #include <linux/clk-provider.h>
+#include <linux/export.h>
 #include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/io.h>
@@ -177,3 +178,4 @@ struct clk_hw *clk_hw_register_gate2(struct device *dev, const char *name,
 
 	return hw;
 }
+EXPORT_SYMBOL_GPL(clk_hw_register_gate2);
diff --git a/drivers/clk/imx/clk-pll14xx.c b/drivers/clk/imx/clk-pll14xx.c
index f9eb189..f5c3e7e 100644
--- a/drivers/clk/imx/clk-pll14xx.c
+++ b/drivers/clk/imx/clk-pll14xx.c
@@ -6,6 +6,7 @@
 #include <linux/bitops.h>
 #include <linux/clk-provider.h>
 #include <linux/err.h>
+#include <linux/export.h>
 #include <linux/io.h>
 #include <linux/iopoll.h>
 #include <linux/slab.h>
@@ -68,6 +69,7 @@ struct imx_pll14xx_clk imx_1443x_pll = {
 	.rate_table = imx_pll1443x_tbl,
 	.rate_count = ARRAY_SIZE(imx_pll1443x_tbl),
 };
+EXPORT_SYMBOL_GPL(imx_1443x_pll);
 
 struct imx_pll14xx_clk imx_1443x_dram_pll = {
 	.type = PLL_1443X,
@@ -75,12 +77,14 @@ struct imx_pll14xx_clk imx_1443x_dram_pll = {
 	.rate_count = ARRAY_SIZE(imx_pll1443x_tbl),
 	.flags = CLK_GET_RATE_NOCACHE,
 };
+EXPORT_SYMBOL_GPL(imx_1443x_dram_pll);
 
 struct imx_pll14xx_clk imx_1416x_pll = {
 	.type = PLL_1416X,
 	.rate_table = imx_pll1416x_tbl,
 	.rate_count = ARRAY_SIZE(imx_pll1416x_tbl),
 };
+EXPORT_SYMBOL_GPL(imx_1416x_pll);
 
 static const struct imx_pll14xx_rate_table *imx_get_pll_settings(
 		struct clk_pll14xx *pll, unsigned long rate)
@@ -436,3 +440,4 @@ struct clk_hw *imx_dev_clk_hw_pll14xx(struct device *dev, const char *name,
 
 	return hw;
 }
+EXPORT_SYMBOL_GPL(imx_dev_clk_hw_pll14xx);
diff --git a/drivers/clk/imx/clk-sscg-pll.c b/drivers/clk/imx/clk-sscg-pll.c
index 773d8a5..9d6cdff 100644
--- a/drivers/clk/imx/clk-sscg-pll.c
+++ b/drivers/clk/imx/clk-sscg-pll.c
@@ -10,6 +10,7 @@
 
 #include <linux/clk-provider.h>
 #include <linux/err.h>
+#include <linux/export.h>
 #include <linux/io.h>
 #include <linux/iopoll.h>
 #include <linux/slab.h>
@@ -537,3 +538,4 @@ struct clk_hw *imx_clk_hw_sscg_pll(const char *name,
 
 	return hw;
 }
+EXPORT_SYMBOL_GPL(imx_clk_hw_sscg_pll);
diff --git a/drivers/clk/imx/clk.c b/drivers/clk/imx/clk.c
index 87ab8db..3cd106c 100644
--- a/drivers/clk/imx/clk.c
+++ b/drivers/clk/imx/clk.c
@@ -3,6 +3,7 @@
 #include <linux/clk-provider.h>
 #include <linux/err.h>
 #include <linux/io.h>
+#include <linux/module.h>
 #include <linux/of.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
@@ -13,6 +14,7 @@
 #define CCDR_MMDC_CH1_MASK		BIT(16)
 
 DEFINE_SPINLOCK(imx_ccm_lock);
+EXPORT_SYMBOL_GPL(imx_ccm_lock);
 
 void imx_unregister_clocks(struct clk *clks[], unsigned int count)
 {
@@ -29,8 +31,9 @@ void imx_unregister_hw_clocks(struct clk_hw *hws[], unsigned int count)
 	for (i = 0; i < count; i++)
 		clk_hw_unregister(hws[i]);
 }
+EXPORT_SYMBOL_GPL(imx_unregister_hw_clocks);
 
-void __init imx_mmdc_mask_handshake(void __iomem *ccm_base,
+void imx_mmdc_mask_handshake(void __iomem *ccm_base,
 				    unsigned int chn)
 {
 	unsigned int reg;
@@ -59,8 +62,9 @@ void imx_check_clk_hws(struct clk_hw *clks[], unsigned int count)
 			pr_err("i.MX clk %u: register failed with %ld\n",
 			       i, PTR_ERR(clks[i]));
 }
+EXPORT_SYMBOL_GPL(imx_check_clk_hws);
 
-static struct clk * __init imx_obtain_fixed_clock_from_dt(const char *name)
+static struct clk *imx_obtain_fixed_clock_from_dt(const char *name)
 {
 	struct of_phandle_args phandle;
 	struct clk *clk = ERR_PTR(-ENODEV);
@@ -80,7 +84,7 @@ static struct clk * __init imx_obtain_fixed_clock_from_dt(const char *name)
 	return clk;
 }
 
-struct clk * __init imx_obtain_fixed_clock(
+struct clk *imx_obtain_fixed_clock(
 			const char *name, unsigned long rate)
 {
 	struct clk *clk;
@@ -91,7 +95,7 @@ struct clk * __init imx_obtain_fixed_clock(
 	return clk;
 }
 
-struct clk_hw * __init imx_obtain_fixed_clock_hw(
+struct clk_hw *imx_obtain_fixed_clock_hw(
 			const char *name, unsigned long rate)
 {
 	struct clk *clk;
@@ -113,6 +117,7 @@ struct clk_hw * imx_obtain_fixed_clk_hw(struct device_node *np,
 
 	return __clk_get_hw(clk);
 }
+EXPORT_SYMBOL_GPL(imx_obtain_fixed_clk_hw);
 
 /*
  * This fixups the register CCM_CSCMR1 write value.
@@ -143,7 +148,7 @@ void imx_cscmr1_fixup(u32 *val)
 static int imx_keep_uart_clocks;
 static struct clk ** const *imx_uart_clocks;
 
-static int __init imx_keep_uart_clocks_param(char *str)
+static int __maybe_unused imx_keep_uart_clocks_param(char *str)
 {
 	imx_keep_uart_clocks = 1;
 
@@ -164,8 +169,9 @@ void imx_register_uart_clocks(struct clk ** const clks[])
 			clk_prepare_enable(*imx_uart_clocks[i]);
 	}
 }
+EXPORT_SYMBOL_GPL(imx_register_uart_clocks);
 
-static int __init imx_clk_disable_uart(void)
+static int imx_clk_disable_uart(void)
 {
 	if (imx_keep_uart_clocks && imx_uart_clocks) {
 		int i;
@@ -177,3 +183,5 @@ static int __init imx_clk_disable_uart(void)
 	return 0;
 }
 late_initcall_sync(imx_clk_disable_uart);
+
+MODULE_LICENSE("GPL v2");
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH V3 06/10] clk: imx8mm: Support module build
  2020-06-29  5:53 [PATCH V3 00/10] Support building i.MX8 SoCs clock driver as module Anson Huang
                   ` (4 preceding siblings ...)
  2020-06-29  5:53 ` [PATCH V3 05/10] clk: imx: Support building i.MX common " Anson Huang
@ 2020-06-29  5:53 ` Anson Huang
  2020-06-29  5:53 ` [PATCH V3 07/10] clk: imx8mn: " Anson Huang
                   ` (3 subsequent siblings)
  9 siblings, 0 replies; 39+ messages in thread
From: Anson Huang @ 2020-06-29  5:53 UTC (permalink / raw)
  To: linux, shawnguo, s.hauer, kernel, festevam, mturquette, sboyd,
	oleksandr.suvorov, stefan.agner, arnd, peng.fan, abel.vesa,
	aisheng.dong, fugang.duan, daniel.baluta, yuehaibing, sfr, viro,
	linux-arm-kernel, linux-kernel, linux-clk
  Cc: Linux-imx

Support building i.MX8MM clock driver as module.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
No change.
---
 drivers/clk/imx/Kconfig      | 2 +-
 drivers/clk/imx/clk-imx8mm.c | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/imx/Kconfig b/drivers/clk/imx/Kconfig
index 678113b..97d86a3 100644
--- a/drivers/clk/imx/Kconfig
+++ b/drivers/clk/imx/Kconfig
@@ -9,7 +9,7 @@ config MXC_CLK_SCU
 	depends on ARCH_MXC && IMX_SCU
 
 config CLK_IMX8MM
-	bool "IMX8MM CCM Clock Driver"
+	tristate "IMX8MM CCM Clock Driver"
 	depends on ARCH_MXC
 	select MXC_CLK
 	help
diff --git a/drivers/clk/imx/clk-imx8mm.c b/drivers/clk/imx/clk-imx8mm.c
index b793264..1e8242d 100644
--- a/drivers/clk/imx/clk-imx8mm.c
+++ b/drivers/clk/imx/clk-imx8mm.c
@@ -657,3 +657,4 @@ static struct platform_driver imx8mm_clk_driver = {
 	},
 };
 module_platform_driver(imx8mm_clk_driver);
+MODULE_LICENSE("GPL v2");
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH V3 07/10] clk: imx8mn: Support module build
  2020-06-29  5:53 [PATCH V3 00/10] Support building i.MX8 SoCs clock driver as module Anson Huang
                   ` (5 preceding siblings ...)
  2020-06-29  5:53 ` [PATCH V3 06/10] clk: imx8mm: Support module build Anson Huang
@ 2020-06-29  5:53 ` Anson Huang
  2020-06-29  5:54 ` [PATCH V3 08/10] clk: imx8mp: " Anson Huang
                   ` (2 subsequent siblings)
  9 siblings, 0 replies; 39+ messages in thread
From: Anson Huang @ 2020-06-29  5:53 UTC (permalink / raw)
  To: linux, shawnguo, s.hauer, kernel, festevam, mturquette, sboyd,
	oleksandr.suvorov, stefan.agner, arnd, peng.fan, abel.vesa,
	aisheng.dong, fugang.duan, daniel.baluta, yuehaibing, sfr, viro,
	linux-arm-kernel, linux-kernel, linux-clk
  Cc: Linux-imx

Support building i.MX8MN clock driver as module.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
No change.
---
 drivers/clk/imx/Kconfig      | 2 +-
 drivers/clk/imx/clk-imx8mn.c | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/imx/Kconfig b/drivers/clk/imx/Kconfig
index 97d86a3..5f537c3 100644
--- a/drivers/clk/imx/Kconfig
+++ b/drivers/clk/imx/Kconfig
@@ -16,7 +16,7 @@ config CLK_IMX8MM
 	    Build the driver for i.MX8MM CCM Clock Driver
 
 config CLK_IMX8MN
-	bool "IMX8MN CCM Clock Driver"
+	tristate "IMX8MN CCM Clock Driver"
 	depends on ARCH_MXC
 	select MXC_CLK
 	help
diff --git a/drivers/clk/imx/clk-imx8mn.c b/drivers/clk/imx/clk-imx8mn.c
index 213cc37..ba930a7 100644
--- a/drivers/clk/imx/clk-imx8mn.c
+++ b/drivers/clk/imx/clk-imx8mn.c
@@ -608,3 +608,4 @@ static struct platform_driver imx8mn_clk_driver = {
 	},
 };
 module_platform_driver(imx8mn_clk_driver);
+MODULE_LICENSE("GPL v2");
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH V3 08/10] clk: imx8mp: Support module build
  2020-06-29  5:53 [PATCH V3 00/10] Support building i.MX8 SoCs clock driver as module Anson Huang
                   ` (6 preceding siblings ...)
  2020-06-29  5:53 ` [PATCH V3 07/10] clk: imx8mn: " Anson Huang
@ 2020-06-29  5:54 ` Anson Huang
  2020-06-29  5:54 ` [PATCH V3 09/10] clk: imx8mq: " Anson Huang
  2020-06-29  5:54 ` [PATCH V3 10/10] clk: imx8qxp: " Anson Huang
  9 siblings, 0 replies; 39+ messages in thread
From: Anson Huang @ 2020-06-29  5:54 UTC (permalink / raw)
  To: linux, shawnguo, s.hauer, kernel, festevam, mturquette, sboyd,
	oleksandr.suvorov, stefan.agner, arnd, peng.fan, abel.vesa,
	aisheng.dong, fugang.duan, daniel.baluta, yuehaibing, sfr, viro,
	linux-arm-kernel, linux-kernel, linux-clk
  Cc: Linux-imx

Support building i.MX8MP clock driver as module.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
No change.
---
 drivers/clk/imx/Kconfig      | 2 +-
 drivers/clk/imx/clk-imx8mp.c | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/imx/Kconfig b/drivers/clk/imx/Kconfig
index 5f537c3..0811bed 100644
--- a/drivers/clk/imx/Kconfig
+++ b/drivers/clk/imx/Kconfig
@@ -23,7 +23,7 @@ config CLK_IMX8MN
 	    Build the driver for i.MX8MN CCM Clock Driver
 
 config CLK_IMX8MP
-	bool "IMX8MP CCM Clock Driver"
+	tristate "IMX8MP CCM Clock Driver"
 	depends on ARCH_MXC
 	select MXC_CLK
 	help
diff --git a/drivers/clk/imx/clk-imx8mp.c b/drivers/clk/imx/clk-imx8mp.c
index ca74771..8582cb5 100644
--- a/drivers/clk/imx/clk-imx8mp.c
+++ b/drivers/clk/imx/clk-imx8mp.c
@@ -773,3 +773,4 @@ static struct platform_driver imx8mp_clk_driver = {
 	},
 };
 module_platform_driver(imx8mp_clk_driver);
+MODULE_LICENSE("GPL v2");
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH V3 09/10] clk: imx8mq: Support module build
  2020-06-29  5:53 [PATCH V3 00/10] Support building i.MX8 SoCs clock driver as module Anson Huang
                   ` (7 preceding siblings ...)
  2020-06-29  5:54 ` [PATCH V3 08/10] clk: imx8mp: " Anson Huang
@ 2020-06-29  5:54 ` Anson Huang
  2020-06-29  5:54 ` [PATCH V3 10/10] clk: imx8qxp: " Anson Huang
  9 siblings, 0 replies; 39+ messages in thread
From: Anson Huang @ 2020-06-29  5:54 UTC (permalink / raw)
  To: linux, shawnguo, s.hauer, kernel, festevam, mturquette, sboyd,
	oleksandr.suvorov, stefan.agner, arnd, peng.fan, abel.vesa,
	aisheng.dong, fugang.duan, daniel.baluta, yuehaibing, sfr, viro,
	linux-arm-kernel, linux-kernel, linux-clk
  Cc: Linux-imx

Support building i.MX8MQ clock driver as module.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
No change.
---
 drivers/clk/imx/Kconfig      | 2 +-
 drivers/clk/imx/clk-imx8mq.c | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/imx/Kconfig b/drivers/clk/imx/Kconfig
index 0811bed..060f9c3 100644
--- a/drivers/clk/imx/Kconfig
+++ b/drivers/clk/imx/Kconfig
@@ -30,7 +30,7 @@ config CLK_IMX8MP
 	    Build the driver for i.MX8MP CCM Clock Driver
 
 config CLK_IMX8MQ
-	bool "IMX8MQ CCM Clock Driver"
+	tristate "IMX8MQ CCM Clock Driver"
 	depends on ARCH_MXC
 	select MXC_CLK
 	help
diff --git a/drivers/clk/imx/clk-imx8mq.c b/drivers/clk/imx/clk-imx8mq.c
index a64aace..2861c31 100644
--- a/drivers/clk/imx/clk-imx8mq.c
+++ b/drivers/clk/imx/clk-imx8mq.c
@@ -643,3 +643,4 @@ static struct platform_driver imx8mq_clk_driver = {
 	},
 };
 module_platform_driver(imx8mq_clk_driver);
+MODULE_LICENSE("GPL v2");
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH V3 10/10] clk: imx8qxp: Support module build
  2020-06-29  5:53 [PATCH V3 00/10] Support building i.MX8 SoCs clock driver as module Anson Huang
                   ` (8 preceding siblings ...)
  2020-06-29  5:54 ` [PATCH V3 09/10] clk: imx8mq: " Anson Huang
@ 2020-06-29  5:54 ` Anson Huang
  2020-06-29 11:40   ` Arnd Bergmann
  9 siblings, 1 reply; 39+ messages in thread
From: Anson Huang @ 2020-06-29  5:54 UTC (permalink / raw)
  To: linux, shawnguo, s.hauer, kernel, festevam, mturquette, sboyd,
	oleksandr.suvorov, stefan.agner, arnd, peng.fan, abel.vesa,
	aisheng.dong, fugang.duan, daniel.baluta, yuehaibing, sfr, viro,
	linux-arm-kernel, linux-kernel, linux-clk
  Cc: Linux-imx

Support building i.MX8QXP clock driver as module.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
No change.
---
 drivers/clk/imx/Kconfig            | 2 +-
 drivers/clk/imx/clk-imx8qxp-lpcg.c | 1 +
 drivers/clk/imx/clk-imx8qxp.c      | 1 +
 3 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/imx/Kconfig b/drivers/clk/imx/Kconfig
index 060f9c3..26cedbf 100644
--- a/drivers/clk/imx/Kconfig
+++ b/drivers/clk/imx/Kconfig
@@ -37,7 +37,7 @@ config CLK_IMX8MQ
 	    Build the driver for i.MX8MQ CCM Clock Driver
 
 config CLK_IMX8QXP
-	bool "IMX8QXP SCU Clock"
+	tristate "IMX8QXP SCU Clock"
 	depends on ARCH_MXC && IMX_SCU && ARM64
 	select MXC_CLK_SCU
 	help
diff --git a/drivers/clk/imx/clk-imx8qxp-lpcg.c b/drivers/clk/imx/clk-imx8qxp-lpcg.c
index 04c8ee3..8afaefc 100644
--- a/drivers/clk/imx/clk-imx8qxp-lpcg.c
+++ b/drivers/clk/imx/clk-imx8qxp-lpcg.c
@@ -232,3 +232,4 @@ static struct platform_driver imx8qxp_lpcg_clk_driver = {
 };
 
 builtin_platform_driver(imx8qxp_lpcg_clk_driver);
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/clk/imx/clk-imx8qxp.c b/drivers/clk/imx/clk-imx8qxp.c
index 5e2903e..a34c7c5 100644
--- a/drivers/clk/imx/clk-imx8qxp.c
+++ b/drivers/clk/imx/clk-imx8qxp.c
@@ -152,3 +152,4 @@ static struct platform_driver imx8qxp_clk_driver = {
 	.probe = imx8qxp_clk_probe,
 };
 builtin_platform_driver(imx8qxp_clk_driver);
+MODULE_LICENSE("GPL v2");
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* RE: [PATCH V3 03/10] ARM: imx: Select MXC_CLK for each SoC
  2020-06-29  5:53 ` [PATCH V3 03/10] ARM: imx: Select MXC_CLK for each SoC Anson Huang
@ 2020-06-29  7:27   ` Aisheng Dong
  2020-06-29  8:21     ` Anson Huang
  0 siblings, 1 reply; 39+ messages in thread
From: Aisheng Dong @ 2020-06-29  7:27 UTC (permalink / raw)
  To: Anson Huang, linux, shawnguo, s.hauer, kernel, festevam,
	mturquette, sboyd, oleksandr.suvorov, Stefan Agner, arnd,
	Peng Fan, Abel Vesa, Andy Duan, Daniel Baluta, yuehaibing, sfr,
	viro, linux-arm-kernel, linux-kernel, linux-clk
  Cc: dl-linux-imx

> From: Anson Huang <Anson.Huang@nxp.com>
> Sent: Monday, June 29, 2020 1:54 PM
> 
> i.MX common clock drivers may support module build, so it is NOT selected by
> default, for ARCH_MXC ARMv7 platforms, need to select it manually in each SoC
> to make build pass.
> 
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
> Changes since V2:
> 	- manually select the MXC_CLK in each SoC instead of selecting it
> 	  for ARCH_MXC.

Any reason to do this?

Regards
Aisheng

> ---
>  arch/arm/mach-imx/Kconfig | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig index
> e7d7b90..a465c0f 100644
> --- a/arch/arm/mach-imx/Kconfig
> +++ b/arch/arm/mach-imx/Kconfig
> @@ -58,6 +58,7 @@ config SOC_IMX21
>  	select CPU_ARM926T
>  	select IMX_HAVE_IOMUX_V1
>  	select MXC_AVIC
> +	select MXC_CLK
> 
>  config SOC_IMX27
>  	bool
> @@ -65,17 +66,20 @@ config SOC_IMX27
>  	select IMX_HAVE_IOMUX_V1
>  	select MXC_AVIC
>  	select PINCTRL_IMX27
> +	select MXC_CLK
> 
>  config SOC_IMX31
>  	bool
>  	select CPU_V6
>  	select MXC_AVIC
> +	select MXC_CLK
> 
>  config SOC_IMX35
>  	bool
>  	select ARCH_MXC_IOMUX_V3
>  	select MXC_AVIC
>  	select PINCTRL_IMX35
> +	select MXC_CLK
> 
>  if ARCH_MULTI_V5
> 
> @@ -419,6 +423,7 @@ config SOC_IMX1
>  	select CPU_ARM920T
>  	select MXC_AVIC
>  	select PINCTRL_IMX1
> +	select MXC_CLK
>  	help
>  	  This enables support for Freescale i.MX1 processor
> 
> @@ -432,6 +437,7 @@ config SOC_IMX25
>  	select CPU_ARM926T
>  	select MXC_AVIC
>  	select PINCTRL_IMX25
> +	select MXC_CLK
>  	help
>  	  This enables support for Freescale i.MX25 processor  endif @@ -444,6
> +450,7 @@ config SOC_IMX5
>  	bool
>  	select HAVE_IMX_SRC
>  	select MXC_TZIC
> +	select MXC_CLK
> 
>  config	SOC_IMX50
>  	bool "i.MX50 support"
> @@ -477,6 +484,7 @@ config SOC_IMX6
>  	select HAVE_IMX_MMDC
>  	select HAVE_IMX_SRC
>  	select MFD_SYSCON
> +	select MXC_CLK
>  	select PL310_ERRATA_769419 if CACHE_L2X0
> 
>  config SOC_IMX6Q
> @@ -561,6 +569,7 @@ config SOC_IMX7D_CM4  config SOC_IMX7D
>  	bool "i.MX7 Dual support"
>  	select PINCTRL_IMX7D
> +	select MXC_CLK
>  	select SOC_IMX7D_CA7 if ARCH_MULTI_V7
>  	select SOC_IMX7D_CM4 if ARM_SINGLE_ARMV7M
>  	select ARM_ERRATA_814220 if ARCH_MULTI_V7 @@ -571,6 +580,7 @@
> config SOC_IMX7ULP
>  	bool "i.MX7ULP support"
>  	select CLKSRC_IMX_TPM
>  	select PINCTRL_IMX7ULP
> +	select MXC_CLK
>  	select SOC_IMX7D_CA7 if ARCH_MULTI_V7
>  	select SOC_IMX7D_CM4 if ARM_SINGLE_ARMV7M
>  	help
> @@ -580,6 +590,7 @@ config SOC_VF610
>  	bool "Vybrid Family VF610 support"
>  	select ARM_GIC if ARCH_MULTI_V7
>  	select PINCTRL_VF610
> +	select MXC_CLK
> 
>  	help
>  	  This enables support for Freescale Vybrid VF610 processor.
> --
> 2.7.4


^ permalink raw reply	[flat|nested] 39+ messages in thread

* RE: [PATCH V3 03/10] ARM: imx: Select MXC_CLK for each SoC
  2020-06-29  7:27   ` Aisheng Dong
@ 2020-06-29  8:21     ` Anson Huang
  0 siblings, 0 replies; 39+ messages in thread
From: Anson Huang @ 2020-06-29  8:21 UTC (permalink / raw)
  To: Aisheng Dong, linux, shawnguo, s.hauer, kernel, festevam,
	mturquette, sboyd, oleksandr.suvorov, Stefan Agner, arnd,
	Peng Fan, Abel Vesa, Andy Duan, Daniel Baluta, yuehaibing, sfr,
	viro, linux-arm-kernel, linux-kernel, linux-clk
  Cc: dl-linux-imx



> Subject: RE: [PATCH V3 03/10] ARM: imx: Select MXC_CLK for each SoC
> 
> > From: Anson Huang <Anson.Huang@nxp.com>
> > Sent: Monday, June 29, 2020 1:54 PM
> >
> > i.MX common clock drivers may support module build, so it is NOT
> > selected by default, for ARCH_MXC ARMv7 platforms, need to select it
> > manually in each SoC to make build pass.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
> > Changes since V2:
> > 	- manually select the MXC_CLK in each SoC instead of selecting it
> > 	  for ARCH_MXC.
> 
> Any reason to do this?

Form the discussion, looks like selecting it for each SoC make more sense, as there is
no CLK_IMX* for i.MX6/7, or am I misunderstanding Stephen's comment? Just use
previous implementation of selecting it once in ARCH_MXC?

Anson

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH V3 02/10] init.h: Fix the __setup_param() macro for module build
  2020-06-29  5:53 ` [PATCH V3 02/10] init.h: Fix the __setup_param() macro for module build Anson Huang
@ 2020-06-29 11:33   ` Arnd Bergmann
  2020-06-29 11:40     ` Anson Huang
  0 siblings, 1 reply; 39+ messages in thread
From: Arnd Bergmann @ 2020-06-29 11:33 UTC (permalink / raw)
  To: Anson Huang
  Cc: Russell King - ARM Linux, Shawn Guo, Sascha Hauer, Sascha Hauer,
	Fabio Estevam, Michael Turquette, Stephen Boyd,
	oleksandr.suvorov, Stefan Agner, Peng Fan, Abel Vesa,
	Dong Aisheng, Andy Duan, Daniel Baluta, YueHaibing,
	Stephen Rothwell, Al Viro, Linux ARM, linux-kernel, linux-clk,
	NXP Linux Team

On Mon, Jun 29, 2020 at 8:06 AM Anson Huang <Anson.Huang@nxp.com> wrote:
>
> Keep __setup_param() to use same parameters for both built in
> and built as module, it can make the drivers which call it easier
> when the drivers can be built in or built as module.
>
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>

I wonder if we should instead drop the __setup() and __setup_param()
definitions from the #else block here. This was clearly not used anywhere,
and it sounds like any possible user is broken and should be changed to
not use __setup() anyway.

      Arnd

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH V3 04/10] clk: imx: Support building SCU clock driver as module
  2020-06-29  5:53 ` [PATCH V3 04/10] clk: imx: Support building SCU clock driver as module Anson Huang
@ 2020-06-29 11:37   ` Arnd Bergmann
  2020-06-29 12:53     ` Anson Huang
  0 siblings, 1 reply; 39+ messages in thread
From: Arnd Bergmann @ 2020-06-29 11:37 UTC (permalink / raw)
  To: Anson Huang
  Cc: Russell King - ARM Linux, Shawn Guo, Sascha Hauer, Sascha Hauer,
	Fabio Estevam, Michael Turquette, Stephen Boyd,
	oleksandr.suvorov, Stefan Agner, Peng Fan, Abel Vesa,
	Dong Aisheng, Andy Duan, Daniel Baluta, YueHaibing,
	Stephen Rothwell, Al Viro, Linux ARM, linux-kernel, linux-clk,
	NXP Linux Team

On Mon, Jun 29, 2020 at 8:06 AM Anson Huang <Anson.Huang@nxp.com> wrote:

> --- a/drivers/clk/imx/Makefile
> +++ b/drivers/clk/imx/Makefile
> @@ -21,9 +21,9 @@ 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-lpcg-scu.o
> +mxc-clk-scu-objs += clk-scu.o
> +obj-$(CONFIG_MXC_CLK_SCU) += mxc-clk-scu.o

It looks like the two modules are tightly connected, one is useless without the
other. How about linking them into a combined module and dropping the
export statement?

      Arnd

^ permalink raw reply	[flat|nested] 39+ messages in thread

* RE: [PATCH V3 02/10] init.h: Fix the __setup_param() macro for module build
  2020-06-29 11:33   ` Arnd Bergmann
@ 2020-06-29 11:40     ` Anson Huang
  2020-06-29 11:58       ` Arnd Bergmann
  0 siblings, 1 reply; 39+ messages in thread
From: Anson Huang @ 2020-06-29 11:40 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Russell King - ARM Linux, Shawn Guo, Sascha Hauer, Sascha Hauer,
	Fabio Estevam, Michael Turquette, Stephen Boyd,
	oleksandr.suvorov, Stefan Agner, Peng Fan, Abel Vesa,
	Aisheng Dong, Andy Duan, Daniel Baluta, YueHaibing,
	Stephen Rothwell, Al Viro, Linux ARM, linux-kernel, linux-clk,
	dl-linux-imx

Hi, Arnd


> Subject: Re: [PATCH V3 02/10] init.h: Fix the __setup_param() macro for
> module build
> 
> On Mon, Jun 29, 2020 at 8:06 AM Anson Huang <Anson.Huang@nxp.com>
> wrote:
> >
> > Keep __setup_param() to use same parameters for both built in and
> > built as module, it can make the drivers which call it easier when the
> > drivers can be built in or built as module.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> 
> I wonder if we should instead drop the __setup() and __setup_param()
> definitions from the #else block here. This was clearly not used anywhere, and
> it sounds like any possible user is broken and should be changed to not use
> __setup() anyway.
> 


It makes sense to drop the __setup() and __serup_param() in the #else block,
just use one definition for all cases, if no one objects, I will remove them in next patch series.

Thanks,
Anson

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH V3 10/10] clk: imx8qxp: Support module build
  2020-06-29  5:54 ` [PATCH V3 10/10] clk: imx8qxp: " Anson Huang
@ 2020-06-29 11:40   ` Arnd Bergmann
  2020-06-29 11:43     ` Anson Huang
  0 siblings, 1 reply; 39+ messages in thread
From: Arnd Bergmann @ 2020-06-29 11:40 UTC (permalink / raw)
  To: Anson Huang
  Cc: Russell King - ARM Linux, Shawn Guo, Sascha Hauer, Sascha Hauer,
	Fabio Estevam, Michael Turquette, Stephen Boyd,
	oleksandr.suvorov, Stefan Agner, Peng Fan, Abel Vesa,
	Dong Aisheng, Andy Duan, Daniel Baluta, YueHaibing,
	Stephen Rothwell, Al Viro, Linux ARM, linux-kernel, linux-clk,
	NXP Linux Team

On Mon, Jun 29, 2020 at 8:06 AM Anson Huang <Anson.Huang@nxp.com> wrote:
>
> Support building i.MX8QXP clock driver as module.
>
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>

I would just combine the per-soc patches into one, as they all have the same
changelog text.

> diff --git a/drivers/clk/imx/clk-imx8qxp-lpcg.c b/drivers/clk/imx/clk-imx8qxp-lpcg.c
> index 04c8ee3..8afaefc 100644
> --- a/drivers/clk/imx/clk-imx8qxp-lpcg.c
> +++ b/drivers/clk/imx/clk-imx8qxp-lpcg.c
> @@ -232,3 +232,4 @@ static struct platform_driver imx8qxp_lpcg_clk_driver = {
>  };
>
>  builtin_platform_driver(imx8qxp_lpcg_clk_driver);
> +MODULE_LICENSE("GPL v2");

Same thing here: please try to make it possible to unload these drivers,
and add MODULE_AUTHOR/MODULE_DESCRIPTION tags in addition
to MODULE_LICENSE.

          Arnd

^ permalink raw reply	[flat|nested] 39+ messages in thread

* RE: [PATCH V3 10/10] clk: imx8qxp: Support module build
  2020-06-29 11:40   ` Arnd Bergmann
@ 2020-06-29 11:43     ` Anson Huang
  2020-06-29 11:58       ` Arnd Bergmann
  0 siblings, 1 reply; 39+ messages in thread
From: Anson Huang @ 2020-06-29 11:43 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Russell King - ARM Linux, Shawn Guo, Sascha Hauer, Sascha Hauer,
	Fabio Estevam, Michael Turquette, Stephen Boyd,
	oleksandr.suvorov, Stefan Agner, Peng Fan, Abel Vesa,
	Aisheng Dong, Andy Duan, Daniel Baluta, YueHaibing,
	Stephen Rothwell, Al Viro, Linux ARM, linux-kernel, linux-clk,
	dl-linux-imx

Hi, Arnd


> Subject: Re: [PATCH V3 10/10] clk: imx8qxp: Support module build
> 
> On Mon, Jun 29, 2020 at 8:06 AM Anson Huang <Anson.Huang@nxp.com>
> wrote:
> >
> > Support building i.MX8QXP clock driver as module.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> 
> I would just combine the per-soc patches into one, as they all have the same
> changelog text.

OK, I will merge those patches with same changelog text into one patch.
But for i.MX8QXP clock driver, as I need to add more changes about module author
etc., I will keep it as a separate patch.

> 
> > diff --git a/drivers/clk/imx/clk-imx8qxp-lpcg.c
> > b/drivers/clk/imx/clk-imx8qxp-lpcg.c
> > index 04c8ee3..8afaefc 100644
> > --- a/drivers/clk/imx/clk-imx8qxp-lpcg.c
> > +++ b/drivers/clk/imx/clk-imx8qxp-lpcg.c
> > @@ -232,3 +232,4 @@ static struct platform_driver
> > imx8qxp_lpcg_clk_driver = {  };
> >
> >  builtin_platform_driver(imx8qxp_lpcg_clk_driver);
> > +MODULE_LICENSE("GPL v2");
> 
> Same thing here: please try to make it possible to unload these drivers, and
> add MODULE_AUTHOR/MODULE_DESCRIPTION tags in addition to
> MODULE_LICENSE.
> 

OK, will improve it in next patch series.

Thanks,
Anson

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH V3 02/10] init.h: Fix the __setup_param() macro for module build
  2020-06-29 11:40     ` Anson Huang
@ 2020-06-29 11:58       ` Arnd Bergmann
  2020-07-01  5:14         ` Anson Huang
  0 siblings, 1 reply; 39+ messages in thread
From: Arnd Bergmann @ 2020-06-29 11:58 UTC (permalink / raw)
  To: Anson Huang
  Cc: Russell King - ARM Linux, Shawn Guo, Sascha Hauer, Sascha Hauer,
	Fabio Estevam, Michael Turquette, Stephen Boyd,
	oleksandr.suvorov, Stefan Agner, Peng Fan, Abel Vesa,
	Aisheng Dong, Andy Duan, Daniel Baluta, YueHaibing,
	Stephen Rothwell, Al Viro, Linux ARM, linux-kernel, linux-clk,
	dl-linux-imx

On Mon, Jun 29, 2020 at 1:40 PM Anson Huang <anson.huang@nxp.com> wrote:
> > Subject: Re: [PATCH V3 02/10] init.h: Fix the __setup_param() macro for
> > module build
> >
> > On Mon, Jun 29, 2020 at 8:06 AM Anson Huang <Anson.Huang@nxp.com>
> > wrote:
> > >
> > > Keep __setup_param() to use same parameters for both built in and
> > > built as module, it can make the drivers which call it easier when the
> > > drivers can be built in or built as module.
> > >
> > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> >
> > I wonder if we should instead drop the __setup() and __setup_param()
> > definitions from the #else block here. This was clearly not used anywhere, and
> > it sounds like any possible user is broken and should be changed to not use
> > __setup() anyway.
> >
>
>
> It makes sense to drop the __setup() and __serup_param() in the #else block,
> just use one definition for all cases, if no one objects, I will remove them in next patch series.

Ok, sounds good. Note that there may be users of the plain __setup() that
just get turned into nops right now. Usually those are already enclosed in
"#ifndef MODULE", but if they are not, then removing the definition would cause
a build error.

Have a look if you can find such instances, and either change the patch to
add the missing "#ifndef MODULE" checks, or just drop the __setup_param()
and leave the __setup() if it gets too complicated.

       Arnd

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH V3 10/10] clk: imx8qxp: Support module build
  2020-06-29 11:43     ` Anson Huang
@ 2020-06-29 11:58       ` Arnd Bergmann
  0 siblings, 0 replies; 39+ messages in thread
From: Arnd Bergmann @ 2020-06-29 11:58 UTC (permalink / raw)
  To: Anson Huang
  Cc: Russell King - ARM Linux, Shawn Guo, Sascha Hauer, Sascha Hauer,
	Fabio Estevam, Michael Turquette, Stephen Boyd,
	oleksandr.suvorov, Stefan Agner, Peng Fan, Abel Vesa,
	Aisheng Dong, Andy Duan, Daniel Baluta, YueHaibing,
	Stephen Rothwell, Al Viro, Linux ARM, linux-kernel, linux-clk,
	dl-linux-imx

On Mon, Jun 29, 2020 at 1:43 PM Anson Huang <anson.huang@nxp.com> wrote:
> > Subject: Re: [PATCH V3 10/10] clk: imx8qxp: Support module build
> >
> > On Mon, Jun 29, 2020 at 8:06 AM Anson Huang <Anson.Huang@nxp.com>
> > wrote:
> > >
> > > Support building i.MX8QXP clock driver as module.
> > >
> > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> >
> > I would just combine the per-soc patches into one, as they all have the same
> > changelog text.
>
> OK, I will merge those patches with same changelog text into one patch.
> But for i.MX8QXP clock driver, as I need to add more changes about module author
> etc., I will keep it as a separate patch.

Ok, sounds good.

      Arnd

^ permalink raw reply	[flat|nested] 39+ messages in thread

* RE: [PATCH V3 04/10] clk: imx: Support building SCU clock driver as module
  2020-06-29 11:37   ` Arnd Bergmann
@ 2020-06-29 12:53     ` Anson Huang
  2020-06-29 13:20       ` Arnd Bergmann
  0 siblings, 1 reply; 39+ messages in thread
From: Anson Huang @ 2020-06-29 12:53 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Russell King - ARM Linux, Shawn Guo, Sascha Hauer, Sascha Hauer,
	Fabio Estevam, Michael Turquette, Stephen Boyd,
	oleksandr.suvorov, Stefan Agner, Peng Fan, Abel Vesa,
	Aisheng Dong, Andy Duan, Daniel Baluta, YueHaibing,
	Stephen Rothwell, Al Viro, Linux ARM, linux-kernel, linux-clk,
	dl-linux-imx

Hi, Arnd


> Subject: Re: [PATCH V3 04/10] clk: imx: Support building SCU clock driver as
> module
> 
> On Mon, Jun 29, 2020 at 8:06 AM Anson Huang <Anson.Huang@nxp.com>
> wrote:
> 
> > --- a/drivers/clk/imx/Makefile
> > +++ b/drivers/clk/imx/Makefile
> > @@ -21,9 +21,9 @@ 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-lpcg-scu.o
> > +mxc-clk-scu-objs += clk-scu.o
> > +obj-$(CONFIG_MXC_CLK_SCU) += mxc-clk-scu.o
> 
> It looks like the two modules are tightly connected, one is useless without the
> other. How about linking them into a combined module and dropping the
> export statement?
> 

From HW perspective, the SCU clock driver and LPCG SCU clock driver are different,
SCU clock driver is for those clocks controlled by system controller (M4 which runs a firmware),
while LPCG SCU clock is for those clock gates inside module, which means AP core can
control it directly via register access, no need to via SCU API.

So, I think it is NOT that tightly connected, it is because they are both for i.MX8 SoCs with SCU
inside, so they are put together in the Makefile.

If the export statement is acceptable, I think it is better to just keep it, make sense?

Thanks,
Anson


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH V3 04/10] clk: imx: Support building SCU clock driver as module
  2020-06-29 12:53     ` Anson Huang
@ 2020-06-29 13:20       ` Arnd Bergmann
  2020-06-29 14:52         ` Anson Huang
  2020-06-30  3:36         ` Dong Aisheng
  0 siblings, 2 replies; 39+ messages in thread
From: Arnd Bergmann @ 2020-06-29 13:20 UTC (permalink / raw)
  To: Anson Huang
  Cc: Russell King - ARM Linux, Shawn Guo, Sascha Hauer, Sascha Hauer,
	Fabio Estevam, Michael Turquette, Stephen Boyd,
	oleksandr.suvorov, Stefan Agner, Peng Fan, Abel Vesa,
	Aisheng Dong, Andy Duan, Daniel Baluta, YueHaibing,
	Stephen Rothwell, Al Viro, Linux ARM, linux-kernel, linux-clk,
	dl-linux-imx

On Mon, Jun 29, 2020 at 2:53 PM Anson Huang <anson.huang@nxp.com> wrote:
> > Subject: Re: [PATCH V3 04/10] clk: imx: Support building SCU clock driver as
> > module
> >
> > On Mon, Jun 29, 2020 at 8:06 AM Anson Huang <Anson.Huang@nxp.com>
> > wrote:
> >
> > > --- a/drivers/clk/imx/Makefile
> > > +++ b/drivers/clk/imx/Makefile
> > > @@ -21,9 +21,9 @@ 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-lpcg-scu.o
> > > +mxc-clk-scu-objs += clk-scu.o
> > > +obj-$(CONFIG_MXC_CLK_SCU) += mxc-clk-scu.o
> >
> > It looks like the two modules are tightly connected, one is useless without the
> > other. How about linking them into a combined module and dropping the
> > export statement?
> >
>
> From HW perspective, the SCU clock driver and LPCG SCU clock driver are different,
> SCU clock driver is for those clocks controlled by system controller (M4 which runs a firmware),
> while LPCG SCU clock is for those clock gates inside module, which means AP core can
> control it directly via register access, no need to via SCU API.

Sorry, I misread the patch in multiple ways. First of all, you already put
clk-scu.o and clk-lpcg-scu.o files into a combined loadable module, and
I had only looked at clk-scu.c.

What I actually meant here was to link clk-scu.o together with clk-imx8qxp.o
(and possibly future chip-specific files) into a loadable module and drop
the export.

> So, I think it is NOT that tightly connected, it is because they are both for i.MX8 SoCs with SCU
> inside, so they are put together in the Makefile.
>
> If the export statement is acceptable, I think it is better to just keep it, make sense?

There is nothing wrong with the export as such, this was just an
idea to simplify the logic.

      Arnd

^ permalink raw reply	[flat|nested] 39+ messages in thread

* RE: [PATCH V3 04/10] clk: imx: Support building SCU clock driver as module
  2020-06-29 13:20       ` Arnd Bergmann
@ 2020-06-29 14:52         ` Anson Huang
  2020-06-29 15:08           ` Arnd Bergmann
  2020-06-30  3:36         ` Dong Aisheng
  1 sibling, 1 reply; 39+ messages in thread
From: Anson Huang @ 2020-06-29 14:52 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Russell King - ARM Linux, Shawn Guo, Sascha Hauer, Sascha Hauer,
	Fabio Estevam, Michael Turquette, Stephen Boyd,
	oleksandr.suvorov, Stefan Agner, Peng Fan, Abel Vesa,
	Aisheng Dong, Andy Duan, Daniel Baluta, YueHaibing,
	Stephen Rothwell, Al Viro, Linux ARM, linux-kernel, linux-clk,
	dl-linux-imx

Hi, Arnd

> Subject: Re: [PATCH V3 04/10] clk: imx: Support building SCU clock driver as
> module
> 
> On Mon, Jun 29, 2020 at 2:53 PM Anson Huang <anson.huang@nxp.com>
> wrote:
> > > Subject: Re: [PATCH V3 04/10] clk: imx: Support building SCU clock
> > > driver as module
> > >
> > > On Mon, Jun 29, 2020 at 8:06 AM Anson Huang <Anson.Huang@nxp.com>
> > > wrote:
> > >
> > > > --- a/drivers/clk/imx/Makefile
> > > > +++ b/drivers/clk/imx/Makefile
> > > > @@ -21,9 +21,9 @@ 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-lpcg-scu.o mxc-clk-scu-objs += clk-scu.o
> > > > +obj-$(CONFIG_MXC_CLK_SCU) += mxc-clk-scu.o
> > >
> > > It looks like the two modules are tightly connected, one is useless
> > > without the other. How about linking them into a combined module and
> > > dropping the export statement?
> > >
> >
> > From HW perspective, the SCU clock driver and LPCG SCU clock driver
> > are different, SCU clock driver is for those clocks controlled by
> > system controller (M4 which runs a firmware), while LPCG SCU clock is
> > for those clock gates inside module, which means AP core can control it
> directly via register access, no need to via SCU API.
> 
> Sorry, I misread the patch in multiple ways. First of all, you already put
> clk-scu.o and clk-lpcg-scu.o files into a combined loadable module, and I had
> only looked at clk-scu.c.
> 
> What I actually meant here was to link clk-scu.o together with clk-imx8qxp.o
> (and possibly future chip-specific files) into a loadable module and drop the
> export.

Sorry, could you please advise more details about how to do it in Makefile?
I tried below but it looks like NOT working. multiple definition of module_init() error reported.

obj-$(CONFIG_MXC_CLK_SCU) := clk-imx.o
clk-imx-y += clk-scu.o clk-lpcg-scu.o
clk-imx-$(CONFIG_CLK_IMX8QXP) += clk-imx8qxp.o clk-imx8qxp-lpcg.o

Thanks,
Anson

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH V3 04/10] clk: imx: Support building SCU clock driver as module
  2020-06-29 14:52         ` Anson Huang
@ 2020-06-29 15:08           ` Arnd Bergmann
  2020-06-29 15:19             ` Anson Huang
  0 siblings, 1 reply; 39+ messages in thread
From: Arnd Bergmann @ 2020-06-29 15:08 UTC (permalink / raw)
  To: Anson Huang
  Cc: Russell King - ARM Linux, Shawn Guo, Sascha Hauer, Sascha Hauer,
	Fabio Estevam, Michael Turquette, Stephen Boyd,
	oleksandr.suvorov, Stefan Agner, Peng Fan, Abel Vesa,
	Aisheng Dong, Andy Duan, Daniel Baluta, YueHaibing,
	Stephen Rothwell, Al Viro, Linux ARM, linux-kernel, linux-clk,
	dl-linux-imx

On Mon, Jun 29, 2020 at 4:52 PM Anson Huang <anson.huang@nxp.com> wrote:
> > Subject: Re: [PATCH V3 04/10] clk: imx: Support building SCU clock driver as module
> > On Mon, Jun 29, 2020 at 2:53 PM Anson Huang <anson.huang@nxp.com> wrote:
> >
> > Sorry, I misread the patch in multiple ways. First of all, you already put
> > clk-scu.o and clk-lpcg-scu.o files into a combined loadable module, and I had
> > only looked at clk-scu.c.
> >
> > What I actually meant here was to link clk-scu.o together with clk-imx8qxp.o
> > (and possibly future chip-specific files) into a loadable module and drop the
> > export.
>
> Sorry, could you please advise more details about how to do it in Makefile?
> I tried below but it looks like NOT working. multiple definition of module_init() error reported.
>
> obj-$(CONFIG_MXC_CLK_SCU) := clk-imx.o
> clk-imx-y += clk-scu.o clk-lpcg-scu.o
> clk-imx-$(CONFIG_CLK_IMX8QXP) += clk-imx8qxp.o clk-imx8qxp-lpcg.o

Right, you can't have multiple module_init() in a module, so what I suggested
earlier won't work any more as soon as you add a second chip that uses the
clk-scu driver, and then you have to use separate modules, or some hack
that calls the init functions one at a time, which is probably worse.

If it's only imx8qxp, you can do it like this:

obj-$(CONFIG_MXC_CLK_SCU) := clk-imx-scu.o clk-imx-lpcg.o
clk-imx-scu-y         += clk-scu.o clk-imx8qxp.o
clk-imx-lpcq-scu-y += clk-lpcg-scu.o clk-imx8qxp-lpcg.o

If you already know that the scu driver is going to be used in future
chips, then just stay with what you have now, using a separate module
per file, exporting the symbols as needed.

        Arnd

^ permalink raw reply	[flat|nested] 39+ messages in thread

* RE: [PATCH V3 04/10] clk: imx: Support building SCU clock driver as module
  2020-06-29 15:08           ` Arnd Bergmann
@ 2020-06-29 15:19             ` Anson Huang
  2020-06-30  3:55               ` Dong Aisheng
  0 siblings, 1 reply; 39+ messages in thread
From: Anson Huang @ 2020-06-29 15:19 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Russell King - ARM Linux, Shawn Guo, Sascha Hauer, Sascha Hauer,
	Fabio Estevam, Michael Turquette, Stephen Boyd,
	oleksandr.suvorov, Stefan Agner, Peng Fan, Abel Vesa,
	Aisheng Dong, Andy Duan, Daniel Baluta, YueHaibing,
	Stephen Rothwell, Al Viro, Linux ARM, linux-kernel, linux-clk,
	dl-linux-imx

Hi, Arnd


> Subject: Re: [PATCH V3 04/10] clk: imx: Support building SCU clock driver as
> module
> 
> On Mon, Jun 29, 2020 at 4:52 PM Anson Huang <anson.huang@nxp.com>
> wrote:
> > > Subject: Re: [PATCH V3 04/10] clk: imx: Support building SCU clock
> > > driver as module On Mon, Jun 29, 2020 at 2:53 PM Anson Huang
> <anson.huang@nxp.com> wrote:
> > >
> > > Sorry, I misread the patch in multiple ways. First of all, you
> > > already put clk-scu.o and clk-lpcg-scu.o files into a combined
> > > loadable module, and I had only looked at clk-scu.c.
> > >
> > > What I actually meant here was to link clk-scu.o together with
> > > clk-imx8qxp.o (and possibly future chip-specific files) into a
> > > loadable module and drop the export.
> >
> > Sorry, could you please advise more details about how to do it in Makefile?
> > I tried below but it looks like NOT working. multiple definition of
> module_init() error reported.
> >
> > obj-$(CONFIG_MXC_CLK_SCU) := clk-imx.o clk-imx-y += clk-scu.o
> > clk-lpcg-scu.o
> > clk-imx-$(CONFIG_CLK_IMX8QXP) += clk-imx8qxp.o clk-imx8qxp-lpcg.o
> 
> Right, you can't have multiple module_init() in a module, so what I suggested
> earlier won't work any more as soon as you add a second chip that uses the
> clk-scu driver, and then you have to use separate modules, or some hack that
> calls the init functions one at a time, which is probably worse.
> 
> If it's only imx8qxp, you can do it like this:
> 
> obj-$(CONFIG_MXC_CLK_SCU) := clk-imx-scu.o clk-imx-lpcg.o
> clk-imx-scu-y         += clk-scu.o clk-imx8qxp.o
> clk-imx-lpcq-scu-y += clk-lpcg-scu.o clk-imx8qxp-lpcg.o
> 
> If you already know that the scu driver is going to be used in future chips, then
> just stay with what you have now, using a separate module per file, exporting
> the symbols as needed.
> 

Thanks, and yes, I know that scu clk driver will be used for future i.MX8X chips with
SCU inside, the current i.MX8QXP clock driver can NOT cover all i.MX8X chips, so
I will stay with the exporting symbols.

Thanks,
Anson

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH V3 04/10] clk: imx: Support building SCU clock driver as module
  2020-06-29 13:20       ` Arnd Bergmann
  2020-06-29 14:52         ` Anson Huang
@ 2020-06-30  3:36         ` Dong Aisheng
  1 sibling, 0 replies; 39+ messages in thread
From: Dong Aisheng @ 2020-06-30  3:36 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Anson Huang, Russell King - ARM Linux, Shawn Guo, Sascha Hauer,
	Sascha Hauer, Fabio Estevam, Michael Turquette, Stephen Boyd,
	oleksandr.suvorov, Stefan Agner, Peng Fan, Abel Vesa,
	Aisheng Dong, Andy Duan, Daniel Baluta, YueHaibing,
	Stephen Rothwell, Al Viro, Linux ARM, linux-kernel, linux-clk,
	dl-linux-imx

On Tue, Jun 30, 2020 at 3:36 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Mon, Jun 29, 2020 at 2:53 PM Anson Huang <anson.huang@nxp.com> wrote:
> > > Subject: Re: [PATCH V3 04/10] clk: imx: Support building SCU clock driver as
> > > module
> > >
> > > On Mon, Jun 29, 2020 at 8:06 AM Anson Huang <Anson.Huang@nxp.com>
> > > wrote:
> > >
> > > > --- a/drivers/clk/imx/Makefile
> > > > +++ b/drivers/clk/imx/Makefile
> > > > @@ -21,9 +21,9 @@ 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-lpcg-scu.o
> > > > +mxc-clk-scu-objs += clk-scu.o
> > > > +obj-$(CONFIG_MXC_CLK_SCU) += mxc-clk-scu.o
> > >
> > > It looks like the two modules are tightly connected, one is useless without the
> > > other. How about linking them into a combined module and dropping the
> > > export statement?
> > >
> >
> > From HW perspective, the SCU clock driver and LPCG SCU clock driver are different,
> > SCU clock driver is for those clocks controlled by system controller (M4 which runs a firmware),
> > while LPCG SCU clock is for those clock gates inside module, which means AP core can
> > control it directly via register access, no need to via SCU API.
>
> Sorry, I misread the patch in multiple ways. First of all, you already put
> clk-scu.o and clk-lpcg-scu.o files into a combined loadable module, and
> I had only looked at clk-scu.c.
>
> What I actually meant here was to link clk-scu.o together with clk-imx8qxp.o
> (and possibly future chip-specific files) into a loadable module and drop
> the export.

It sounds like a good idea to me.
Actually I planned to combine them into one driver in the future.

Regards
Aisheng

>
> > So, I think it is NOT that tightly connected, it is because they are both for i.MX8 SoCs with SCU
> > inside, so they are put together in the Makefile.
> >
> > If the export statement is acceptable, I think it is better to just keep it, make sense?
>
> There is nothing wrong with the export as such, this was just an
> idea to simplify the logic.
>
>       Arnd

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH V3 04/10] clk: imx: Support building SCU clock driver as module
  2020-06-29 15:19             ` Anson Huang
@ 2020-06-30  3:55               ` Dong Aisheng
  2020-07-01  7:19                 ` Anson Huang
  0 siblings, 1 reply; 39+ messages in thread
From: Dong Aisheng @ 2020-06-30  3:55 UTC (permalink / raw)
  To: Anson Huang
  Cc: Arnd Bergmann, Russell King - ARM Linux, Shawn Guo, Sascha Hauer,
	Sascha Hauer, Fabio Estevam, Michael Turquette, Stephen Boyd,
	oleksandr.suvorov, Stefan Agner, Peng Fan, Abel Vesa,
	Aisheng Dong, Andy Duan, Daniel Baluta, YueHaibing,
	Stephen Rothwell, Al Viro, Linux ARM, linux-kernel, linux-clk,
	dl-linux-imx

On Tue, Jun 30, 2020 at 5:16 AM Anson Huang <anson.huang@nxp.com> wrote:
>
> Hi, Arnd
>
>
> > Subject: Re: [PATCH V3 04/10] clk: imx: Support building SCU clock driver as
> > module
> >
> > On Mon, Jun 29, 2020 at 4:52 PM Anson Huang <anson.huang@nxp.com>
> > wrote:
> > > > Subject: Re: [PATCH V3 04/10] clk: imx: Support building SCU clock
> > > > driver as module On Mon, Jun 29, 2020 at 2:53 PM Anson Huang
> > <anson.huang@nxp.com> wrote:
> > > >
> > > > Sorry, I misread the patch in multiple ways. First of all, you
> > > > already put clk-scu.o and clk-lpcg-scu.o files into a combined
> > > > loadable module, and I had only looked at clk-scu.c.
> > > >
> > > > What I actually meant here was to link clk-scu.o together with
> > > > clk-imx8qxp.o (and possibly future chip-specific files) into a
> > > > loadable module and drop the export.
> > >
> > > Sorry, could you please advise more details about how to do it in Makefile?
> > > I tried below but it looks like NOT working. multiple definition of
> > module_init() error reported.
> > >
> > > obj-$(CONFIG_MXC_CLK_SCU) := clk-imx.o clk-imx-y += clk-scu.o
> > > clk-lpcg-scu.o
> > > clk-imx-$(CONFIG_CLK_IMX8QXP) += clk-imx8qxp.o clk-imx8qxp-lpcg.o
> >
> > Right, you can't have multiple module_init() in a module, so what I suggested
> > earlier won't work any more as soon as you add a second chip that uses the
> > clk-scu driver, and then you have to use separate modules, or some hack that
> > calls the init functions one at a time, which is probably worse.
> >
> > If it's only imx8qxp, you can do it like this:
> >
> > obj-$(CONFIG_MXC_CLK_SCU) := clk-imx-scu.o clk-imx-lpcg.o
> > clk-imx-scu-y         += clk-scu.o clk-imx8qxp.o
> > clk-imx-lpcq-scu-y += clk-lpcg-scu.o clk-imx8qxp-lpcg.o
> >
> > If you already know that the scu driver is going to be used in future chips, then
> > just stay with what you have now, using a separate module per file, exporting
> > the symbols as needed.
> >
>
> Thanks, and yes, I know that scu clk driver will be used for future i.MX8X chips with
> SCU inside, the current i.MX8QXP clock driver can NOT cover all i.MX8X chips, so
> I will stay with the exporting symbols.
>

SCU clock driver is a common driver for all SCU based platforms.
Current i.MX8QXP SCU clock driver will be extended to support all
future SCU based platforms.
So theoretically clk-scu.o and clk-imx8qxp.o can be combined. LPCG  is similar.
Maybe you can give a try as Arnd suggested.

Regards
Aisheng

> Thanks,
> Anson

^ permalink raw reply	[flat|nested] 39+ messages in thread

* RE: [PATCH V3 02/10] init.h: Fix the __setup_param() macro for module build
  2020-06-29 11:58       ` Arnd Bergmann
@ 2020-07-01  5:14         ` Anson Huang
  2020-07-01  8:37           ` Arnd Bergmann
  0 siblings, 1 reply; 39+ messages in thread
From: Anson Huang @ 2020-07-01  5:14 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Russell King - ARM Linux, Shawn Guo, Sascha Hauer, Sascha Hauer,
	Fabio Estevam, Michael Turquette, Stephen Boyd,
	oleksandr.suvorov, Stefan Agner, Peng Fan, Abel Vesa,
	Aisheng Dong, Andy Duan, Daniel Baluta, YueHaibing,
	Stephen Rothwell, Al Viro, Linux ARM, linux-kernel, linux-clk,
	dl-linux-imx

Hi, Arnd


> Subject: Re: [PATCH V3 02/10] init.h: Fix the __setup_param() macro for
> module build
> 
> On Mon, Jun 29, 2020 at 1:40 PM Anson Huang <anson.huang@nxp.com>
> wrote:
> > > Subject: Re: [PATCH V3 02/10] init.h: Fix the __setup_param() macro
> > > for module build
> > >
> > > On Mon, Jun 29, 2020 at 8:06 AM Anson Huang <Anson.Huang@nxp.com>
> > > wrote:
> > > >
> > > > Keep __setup_param() to use same parameters for both built in and
> > > > built as module, it can make the drivers which call it easier when
> > > > the drivers can be built in or built as module.
> > > >
> > > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > >
> > > I wonder if we should instead drop the __setup() and __setup_param()
> > > definitions from the #else block here. This was clearly not used
> > > anywhere, and it sounds like any possible user is broken and should
> > > be changed to not use
> > > __setup() anyway.
> > >
> >
> >
> > It makes sense to drop the __setup() and __serup_param() in the #else
> > block, just use one definition for all cases, if no one objects, I will remove
> them in next patch series.
> 
> Ok, sounds good. Note that there may be users of the plain __setup() that just
> get turned into nops right now. Usually those are already enclosed in "#ifndef
> MODULE", but if they are not, then removing the definition would cause a
> build error.
> 
> Have a look if you can find such instances, and either change the patch to add
> the missing "#ifndef MODULE" checks, or just drop the __setup_param() and
> leave the __setup() if it gets too complicated.

Looks like the __setup_param() defined in "#ifndef MODULE" can NOT be used for 
MODULE build at all, so sharing same implementation is NOT available, so if it is NOT
that critical, I plan to keep the #else block in this patch, let me know if you have further
concern or any other suggestion, below is the build error reported for module build using
__setup_param() implementation for built in.

thanks,
Anson


In file included from ./arch/arm64/include/asm/alternative.h:12,
                 from ./arch/arm64/include/asm/lse.h:15,
                 from ./arch/arm64/include/asm/cmpxchg.h:14,
                 from ./arch/arm64/include/asm/atomic.h:16,
                 from ./include/linux/atomic.h:7,
                 from ./include/asm-generic/bitops/atomic.h:5,
                 from ./arch/arm64/include/asm/bitops.h:26,
                 from ./include/linux/bitops.h:29,
                 from ./include/linux/kernel.h:12,
                 from ./include/linux/clk.h:13,
                 from drivers/clk/imx/clk.c:2:
./include/linux/init.h:177:16: error: variable ‘__setup_imx_keep_uart_earlycon’ has initializer but incomplete type
  177 |  static struct obs_kernel_param __setup_##unique_id  \
      |                ^~~~~~~~~~~~~~~~
drivers/clk/imx/clk.c:157:1: note: in expansion of macro ‘__setup_param’
  157 | __setup_param("earlycon", imx_keep_uart_earlycon,
      | ^~~~~~~~~~~~~
./include/linux/init.h:180:7: warning: excess elements in struct initializer
  180 |   = { __setup_str_##unique_id, fn, early }
      |       ^~~~~~~~~~~~
drivers/clk/imx/clk.c:157:1: note: in expansion of macro ‘__setup_param’
  157 | __setup_param("earlycon", imx_keep_uart_earlycon,
      | ^~~~~~~~~~~~~
./include/linux/init.h:180:7: note: (near initialization for ‘__setup_imx_keep_uart_earlycon’)
  180 |   = { __setup_str_##unique_id, fn, early }
      |       ^~~~~~~~~~~~
drivers/clk/imx/clk.c:157:1: note: in expansion of macro ‘__setup_param’
  157 | __setup_param("earlycon", imx_keep_uart_earlycon,
      | ^~~~~~~~~~~~~
drivers/clk/imx/clk.c:158:8: warning: excess elements in struct initializer
  158 |        imx_keep_uart_clocks_param, 0);
      |        ^~~~~~~~~~~~~~~~~~~~~~~~~~
./include/linux/init.h:180:32: note: in definition of macro ‘__setup_param’
  180 |   = { __setup_str_##unique_id, fn, early }
      |                                ^~
drivers/clk/imx/clk.c:158:8: note: (near initialization for ‘__setup_imx_keep_uart_earlycon’)
  158 |        imx_keep_uart_clocks_param, 0);
      |        ^~~~~~~~~~~~~~~~~~~~~~~~~~
./include/linux/init.h:180:32: note: in definition of macro ‘__setup_param’
  180 |   = { __setup_str_##unique_id, fn, early }
      |                                ^~
drivers/clk/imx/clk.c:158:36: warning: excess elements in struct initializer
  158 |        imx_keep_uart_clocks_param, 0);
      |                                    ^
./include/linux/init.h:180:36: note: in definition of macro ‘__setup_param’
  180 |   = { __setup_str_##unique_id, fn, early }
      |                                    ^~~~~
drivers/clk/imx/clk.c:158:36: note: (near initialization for ‘__setup_imx_keep_uart_earlycon’)
  158 |        imx_keep_uart_clocks_param, 0);
      |                                    ^
./include/linux/init.h:180:36: note: in definition of macro ‘__setup_param’
  180 |   = { __setup_str_##unique_id, fn, early }
      |                                    ^~~~~
./include/linux/init.h:177:16: error: variable ‘__setup_imx_keep_uart_earlyprintk’ has initializer but incomplete type
  177 |  static struct obs_kernel_param __setup_##unique_id  \
      |                ^~~~~~~~~~~~~~~~
drivers/clk/imx/clk.c:159:1: note: in expansion of macro ‘__setup_param’
  159 | __setup_param("earlyprintk", imx_keep_uart_earlyprintk,
      | ^~~~~~~~~~~~~
./include/linux/init.h:180:7: warning: excess elements in struct initializer
  180 |   = { __setup_str_##unique_id, fn, early }
      |       ^~~~~~~~~~~~
drivers/clk/imx/clk.c:159:1: note: in expansion of macro ‘__setup_param’
  159 | __setup_param("earlyprintk", imx_keep_uart_earlyprintk,
      | ^~~~~~~~~~~~~
./include/linux/init.h:180:7: note: (near initialization for ‘__setup_imx_keep_uart_earlyprintk’)
  180 |   = { __setup_str_##unique_id, fn, early }
      |       ^~~~~~~~~~~~
drivers/clk/imx/clk.c:159:1: note: in expansion of macro ‘__setup_param’
  159 | __setup_param("earlyprintk", imx_keep_uart_earlyprintk,
      | ^~~~~~~~~~~~~
drivers/clk/imx/clk.c:160:8: warning: excess elements in struct initializer
  160 |        imx_keep_uart_clocks_param, 0);
      |        ^~~~~~~~~~~~~~~~~~~~~~~~~~
./include/linux/init.h:180:32: note: in definition of macro ‘__setup_param’
  180 |   = { __setup_str_##unique_id, fn, early }
      |                                ^~
drivers/clk/imx/clk.c:160:8: note: (near initialization for ‘__setup_imx_keep_uart_earlyprintk’)
  160 |        imx_keep_uart_clocks_param, 0);
      |        ^~~~~~~~~~~~~~~~~~~~~~~~~~
./include/linux/init.h:180:32: note: in definition of macro ‘__setup_param’
  180 |   = { __setup_str_##unique_id, fn, early }
      |                                ^~
drivers/clk/imx/clk.c:160:36: warning: excess elements in struct initializer
  160 |        imx_keep_uart_clocks_param, 0);
      |                                    ^
./include/linux/init.h:180:36: note: in definition of macro ‘__setup_param’
  180 |   = { __setup_str_##unique_id, fn, early }
      |                                    ^~~~~
drivers/clk/imx/clk.c:160:36: note: (near initialization for ‘__setup_imx_keep_uart_earlyprintk’)
  160 |        imx_keep_uart_clocks_param, 0);
      |                                    ^
./include/linux/init.h:180:36: note: in definition of macro ‘__setup_param’
  180 |   = { __setup_str_##unique_id, fn, early }
      |                                    ^~~~~
./include/linux/init.h:177:33: error: storage size of ‘__setup_imx_keep_uart_earlycon’ isn’t known
  177 |  static struct obs_kernel_param __setup_##unique_id  \
      |                                 ^~~~~~~~
drivers/clk/imx/clk.c:157:1: note: in expansion of macro ‘__setup_param’
  157 | __setup_param("earlycon", imx_keep_uart_earlycon,
      | ^~~~~~~~~~~~~
./include/linux/init.h:177:33: error: storage size of ‘__setup_imx_keep_uart_earlyprintk’ isn’t known
  177 |  static struct obs_kernel_param __setup_##unique_id  \
      |                                 ^~~~~~~~
drivers/clk/imx/clk.c:159:1: note: in expansion of macro ‘__setup_param’
  159 | __setup_param("earlyprintk", imx_keep_uart_earlyprintk,
      | ^~~~~~~~~~~~~
scripts/Makefile.build:280: recipe for target 'drivers/clk/imx/clk.o' failed


^ permalink raw reply	[flat|nested] 39+ messages in thread

* RE: [PATCH V3 04/10] clk: imx: Support building SCU clock driver as module
  2020-06-30  3:55               ` Dong Aisheng
@ 2020-07-01  7:19                 ` Anson Huang
  2020-07-01  8:46                   ` Arnd Bergmann
  0 siblings, 1 reply; 39+ messages in thread
From: Anson Huang @ 2020-07-01  7:19 UTC (permalink / raw)
  To: Dong Aisheng, Arnd Bergmann
  Cc: Russell King - ARM Linux, Shawn Guo, Sascha Hauer, Sascha Hauer,
	Fabio Estevam, Michael Turquette, Stephen Boyd,
	oleksandr.suvorov, Stefan Agner, Peng Fan, Abel Vesa,
	Aisheng Dong, Andy Duan, Daniel Baluta, YueHaibing,
	Stephen Rothwell, Al Viro, Linux ARM, linux-kernel, linux-clk,
	dl-linux-imx

Hi, Arnd


> Subject: Re: [PATCH V3 04/10] clk: imx: Support building SCU clock driver as
> module
> 
> On Tue, Jun 30, 2020 at 5:16 AM Anson Huang <anson.huang@nxp.com>
> wrote:
> >
> > Hi, Arnd
> >
> >
> > > Subject: Re: [PATCH V3 04/10] clk: imx: Support building SCU clock
> > > driver as module
> > >
> > > On Mon, Jun 29, 2020 at 4:52 PM Anson Huang <anson.huang@nxp.com>
> > > wrote:
> > > > > Subject: Re: [PATCH V3 04/10] clk: imx: Support building SCU
> > > > > clock driver as module On Mon, Jun 29, 2020 at 2:53 PM Anson
> > > > > Huang
> > > <anson.huang@nxp.com> wrote:
> > > > >
> > > > > Sorry, I misread the patch in multiple ways. First of all, you
> > > > > already put clk-scu.o and clk-lpcg-scu.o files into a combined
> > > > > loadable module, and I had only looked at clk-scu.c.
> > > > >
> > > > > What I actually meant here was to link clk-scu.o together with
> > > > > clk-imx8qxp.o (and possibly future chip-specific files) into a
> > > > > loadable module and drop the export.
> > > >
> > > > Sorry, could you please advise more details about how to do it in
> Makefile?
> > > > I tried below but it looks like NOT working. multiple definition
> > > > of
> > > module_init() error reported.
> > > >
> > > > obj-$(CONFIG_MXC_CLK_SCU) := clk-imx.o clk-imx-y += clk-scu.o
> > > > clk-lpcg-scu.o
> > > > clk-imx-$(CONFIG_CLK_IMX8QXP) += clk-imx8qxp.o clk-imx8qxp-lpcg.o
> > >
> > > Right, you can't have multiple module_init() in a module, so what I
> > > suggested earlier won't work any more as soon as you add a second
> > > chip that uses the clk-scu driver, and then you have to use separate
> > > modules, or some hack that calls the init functions one at a time, which is
> probably worse.
> > >
> > > If it's only imx8qxp, you can do it like this:
> > >
> > > obj-$(CONFIG_MXC_CLK_SCU) := clk-imx-scu.o clk-imx-lpcg.o
> > > clk-imx-scu-y         += clk-scu.o clk-imx8qxp.o
> > > clk-imx-lpcq-scu-y += clk-lpcg-scu.o clk-imx8qxp-lpcg.o
> > >
> > > If you already know that the scu driver is going to be used in
> > > future chips, then just stay with what you have now, using a
> > > separate module per file, exporting the symbols as needed.
> > >
> >
> > Thanks, and yes, I know that scu clk driver will be used for future
> > i.MX8X chips with SCU inside, the current i.MX8QXP clock driver can
> > NOT cover all i.MX8X chips, so I will stay with the exporting symbols.
> >
> 
> SCU clock driver is a common driver for all SCU based platforms.
> Current i.MX8QXP SCU clock driver will be extended to support all future SCU
> based platforms.
> So theoretically clk-scu.o and clk-imx8qxp.o can be combined. LPCG  is
> similar.
> Maybe you can give a try as Arnd suggested.
> 

Do we really need to link clk-scu and i.MX8QXP clock driver together just to avoid some export?
I met some build issues if using this method, the i.MX8QXP module build is OK, but other platforms
like i.MX8MM/i.MX8MN/i.MX8MQ/i.MX8MP clock drivers are just skipped for build, even these
i.MX8M clock config are existing in .config, anyone know why? Looks like the change in Makefile for
i.MX8QXP clock driver introduce this issue.

I think doing such big change in order to save some exports looks like NOT that worth, is it acceptable
to just keep the original one in Makefile, just to have mxc-clk.ko, clk-scu.ko and clk-imx8qxp.ko etc..

thanks,
Anson

.config:
CONFIG_MXC_CLK=m
CONFIG_MXC_CLK_SCU=m
CONFIG_CLK_IMX8MM=m
CONFIG_CLK_IMX8MN=m
CONFIG_CLK_IMX8MP=m
CONFIG_CLK_IMX8MQ=m
CONFIG_CLK_IMX8QXP=m

Makefile:
mxc-clk-objs += clk-fixup-mux.o
mxc-clk-objs += clk-frac-pll.o
mxc-clk-objs += clk-gate2.o
mxc-clk-objs += clk-gate-exclusive.o
mxc-clk-objs += clk-pfd.o
mxc-clk-objs += clk-pfdv2.o
mxc-clk-objs += clk-pllv1.o
mxc-clk-objs += clk-pllv2.o
mxc-clk-objs += clk-pllv3.o
mxc-clk-objs += clk-pllv4.o
mxc-clk-objs += clk-pll14xx.o
mxc-clk-objs += clk-sscg-pll.o
obj-$(CONFIG_MXC_CLK) += mxc-clk.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_MXC_CLK_SCU) := clk-imx-scu.o clk-imx-lpcg-scu.o
clk-imx-scu-$(CONFIG_CLK_IMX8QXP) += clk-scu.o clk-imx8qxp.o
clk-imx-lpcg-scu-$(CONFIG_CLK_IMX8QXP) += clk-lpcg-scu.o clk-imx8qxp-lpcg.o


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH V3 02/10] init.h: Fix the __setup_param() macro for module build
  2020-07-01  5:14         ` Anson Huang
@ 2020-07-01  8:37           ` Arnd Bergmann
  2020-07-01  9:27             ` Anson Huang
  0 siblings, 1 reply; 39+ messages in thread
From: Arnd Bergmann @ 2020-07-01  8:37 UTC (permalink / raw)
  To: Anson Huang
  Cc: Russell King - ARM Linux, Shawn Guo, Sascha Hauer, Sascha Hauer,
	Fabio Estevam, Michael Turquette, Stephen Boyd,
	oleksandr.suvorov, Stefan Agner, Peng Fan, Abel Vesa,
	Aisheng Dong, Andy Duan, Daniel Baluta, YueHaibing,
	Stephen Rothwell, Al Viro, Linux ARM, linux-kernel, linux-clk,
	dl-linux-imx

On Wed, Jul 1, 2020 at 7:14 AM Anson Huang <anson.huang@nxp.com> wrote:
> > Subject: Re: [PATCH V3 02/10] init.h: Fix the __setup_param() macro for module build
> > On Mon, Jun 29, 2020 at 1:40 PM Anson Huang <anson.huang@nxp.com>
> > wrote:
> > > It makes sense to drop the __setup() and __serup_param() in the #else
> > > block, just use one definition for all cases, if no one objects, I will remove
> > them in next patch series.
> >
> > Ok, sounds good. Note that there may be users of the plain __setup() that just
> > get turned into nops right now. Usually those are already enclosed in "#ifndef
> > MODULE", but if they are not, then removing the definition would cause a
> > build error.
> >
> > Have a look if you can find such instances, and either change the patch to add
> > the missing "#ifndef MODULE" checks, or just drop the __setup_param() and
> > leave the __setup() if it gets too complicated.
>
> Looks like the __setup_param() defined in "#ifndef MODULE" can NOT be used for
> MODULE build at all, so sharing same implementation is NOT available, so if it is NOT
> that critical, I plan to keep the #else block in this patch, let me know if you have further
> concern or any other suggestion, below is the build error reported for module build using
> __setup_param() implementation for built in.

I don't understand what your plan is here. Do you mean you will leave that
part of the clk driver as built-in?

> In file included from ./arch/arm64/include/asm/alternative.h:12,
>                  from ./arch/arm64/include/asm/lse.h:15,
>                  from ./arch/arm64/include/asm/cmpxchg.h:14,
>                  from ./arch/arm64/include/asm/atomic.h:16,
>                  from ./include/linux/atomic.h:7,
>                  from ./include/asm-generic/bitops/atomic.h:5,
>                  from ./arch/arm64/include/asm/bitops.h:26,
>                  from ./include/linux/bitops.h:29,
>                  from ./include/linux/kernel.h:12,
>                  from ./include/linux/clk.h:13,
>                  from drivers/clk/imx/clk.c:2:
> ./include/linux/init.h:177:16: error: variable ‘__setup_imx_keep_uart_earlycon’ has initializer but incomplete type
>   177 |  static struct obs_kernel_param __setup_##unique_id  \
>       |                ^~~~~~~~~~~~~~~~
> drivers/clk/imx/clk.c:157:1: note: in expansion of macro ‘__setup_param’
>   157 | __setup_param("earlycon", imx_keep_uart_earlycon,
>       | ^~~~~~~~~~~~~
> ./include/linux/init.h:180:7: warning: excess elements in struct initializer
>   180 |   = { __setup_str_##unique_id, fn, early }
>       |       ^~~~~~~~~~~~
> drivers/clk/imx/clk.c:157:1: note: in expansion of macro ‘__setup_param’
>   157 | __setup_param("earlycon", imx_keep_uart_earlycon,
>       | ^~~~~~~~~~~~~
> ./include/linux/init.h:180:7: note: (near initialization for ‘__setup_imx_keep_uart_earlycon’)

This error just means you can't have a __setup_param() call in a
loadable module,
which we already knew. If you need to do something with the clocks early on,
that has to be in built-in code and cannot be in a module. If you
don't need that
code, then you should just remove it from both the modular version and the
built-in version.

What is the purpose of that __setup_param() argument parsing in the
clock driver?

       Arnd

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH V3 04/10] clk: imx: Support building SCU clock driver as module
  2020-07-01  7:19                 ` Anson Huang
@ 2020-07-01  8:46                   ` Arnd Bergmann
  2020-07-01  9:28                     ` Anson Huang
  0 siblings, 1 reply; 39+ messages in thread
From: Arnd Bergmann @ 2020-07-01  8:46 UTC (permalink / raw)
  To: Anson Huang
  Cc: Dong Aisheng, Russell King - ARM Linux, Shawn Guo, Sascha Hauer,
	Sascha Hauer, Fabio Estevam, Michael Turquette, Stephen Boyd,
	oleksandr.suvorov, Stefan Agner, Peng Fan, Abel Vesa,
	Aisheng Dong, Andy Duan, Daniel Baluta, YueHaibing,
	Stephen Rothwell, Al Viro, Linux ARM, linux-kernel, linux-clk,
	dl-linux-imx

On Wed, Jul 1, 2020 at 9:19 AM Anson Huang <anson.huang@nxp.com> wrote:
> > Subject: Re: [PATCH V3 04/10] clk: imx: Support building SCU clock driver as module
> > On Tue, Jun 30, 2020 at 5:16 AM Anson Huang <anson.huang@nxp.com> wrote:
> >
> > SCU clock driver is a common driver for all SCU based platforms.
> > Current i.MX8QXP SCU clock driver will be extended to support all future SCU
> > based platforms.
> > So theoretically clk-scu.o and clk-imx8qxp.o can be combined. LPCG  is
> > similar.
> > Maybe you can give a try as Arnd suggested.
> >
>
> Do we really need to link clk-scu and i.MX8QXP clock driver together just to avoid some export?

It was just meant to be easier than exporting a symbol and dealing with module
dependencies. If it's not easier, then don't.

> I met some build issues if using this method, the i.MX8QXP module build is OK, but other platforms
> like i.MX8MM/i.MX8MN/i.MX8MQ/i.MX8MP clock drivers are just skipped for build, even these
> i.MX8M clock config are existing in .config, anyone know why? Looks like the change in Makefile for
> i.MX8QXP clock driver introduce this issue.

You have a ":=" instead of "+=" typo, so all earlier "+=" are ignored:

> 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_MXC_CLK_SCU) := clk-imx-scu.o clk-imx-lpcg-scu.o

                                   ^^^^^^^^

      Arnd

^ permalink raw reply	[flat|nested] 39+ messages in thread

* RE: [PATCH V3 02/10] init.h: Fix the __setup_param() macro for module build
  2020-07-01  8:37           ` Arnd Bergmann
@ 2020-07-01  9:27             ` Anson Huang
  2020-07-01  9:53               ` Arnd Bergmann
  0 siblings, 1 reply; 39+ messages in thread
From: Anson Huang @ 2020-07-01  9:27 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Russell King - ARM Linux, Shawn Guo, Sascha Hauer, Sascha Hauer,
	Fabio Estevam, Michael Turquette, Stephen Boyd,
	oleksandr.suvorov, Stefan Agner, Peng Fan, Abel Vesa,
	Aisheng Dong, Andy Duan, Daniel Baluta, YueHaibing,
	Stephen Rothwell, Al Viro, Linux ARM, linux-kernel, linux-clk,
	dl-linux-imx

Hi, Arnd


> Subject: Re: [PATCH V3 02/10] init.h: Fix the __setup_param() macro for
> module build
> 
> On Wed, Jul 1, 2020 at 7:14 AM Anson Huang <anson.huang@nxp.com>
> wrote:
> > > Subject: Re: [PATCH V3 02/10] init.h: Fix the __setup_param() macro
> > > for module build On Mon, Jun 29, 2020 at 1:40 PM Anson Huang
> > > <anson.huang@nxp.com>
> > > wrote:
> > > > It makes sense to drop the __setup() and __serup_param() in the
> > > > #else block, just use one definition for all cases, if no one
> > > > objects, I will remove
> > > them in next patch series.
> > >
> > > Ok, sounds good. Note that there may be users of the plain __setup()
> > > that just get turned into nops right now. Usually those are already
> > > enclosed in "#ifndef MODULE", but if they are not, then removing the
> > > definition would cause a build error.
> > >
> > > Have a look if you can find such instances, and either change the
> > > patch to add the missing "#ifndef MODULE" checks, or just drop the
> > > __setup_param() and leave the __setup() if it gets too complicated.
> >
> > Looks like the __setup_param() defined in "#ifndef MODULE" can NOT be
> > used for MODULE build at all, so sharing same implementation is NOT
> > available, so if it is NOT that critical, I plan to keep the #else
> > block in this patch, let me know if you have further concern or any
> > other suggestion, below is the build error reported for module build
> > using
> > __setup_param() implementation for built in.
> 
> I don't understand what your plan is here. Do you mean you will leave that
> part of the clk driver as built-in?

I meant I will leave the #else block of __setup_param() defined as nothing as below to
make module build passed.

#define __setup_param(str, unique_id, fn, early)        /* nothing */

> 
> > In file included from ./arch/arm64/include/asm/alternative.h:12,
> >                  from ./arch/arm64/include/asm/lse.h:15,
> >                  from ./arch/arm64/include/asm/cmpxchg.h:14,
> >                  from ./arch/arm64/include/asm/atomic.h:16,
> >                  from ./include/linux/atomic.h:7,
> >                  from ./include/asm-generic/bitops/atomic.h:5,
> >                  from ./arch/arm64/include/asm/bitops.h:26,
> >                  from ./include/linux/bitops.h:29,
> >                  from ./include/linux/kernel.h:12,
> >                  from ./include/linux/clk.h:13,
> >                  from drivers/clk/imx/clk.c:2:
> > ./include/linux/init.h:177:16: error: variable
> ‘__setup_imx_keep_uart_earlycon’ has initializer but incomplete type
> >   177 |  static struct obs_kernel_param __setup_##unique_id  \
> >       |                ^~~~~~~~~~~~~~~~
> > drivers/clk/imx/clk.c:157:1: note: in expansion of macro ‘__setup_param’
> >   157 | __setup_param("earlycon", imx_keep_uart_earlycon,
> >       | ^~~~~~~~~~~~~
> > ./include/linux/init.h:180:7: warning: excess elements in struct initializer
> >   180 |   = { __setup_str_##unique_id, fn, early }
> >       |       ^~~~~~~~~~~~
> > drivers/clk/imx/clk.c:157:1: note: in expansion of macro ‘__setup_param’
> >   157 | __setup_param("earlycon", imx_keep_uart_earlycon,
> >       | ^~~~~~~~~~~~~
> > ./include/linux/init.h:180:7: note: (near initialization for
> > ‘__setup_imx_keep_uart_earlycon’)
> 
> This error just means you can't have a __setup_param() call in a loadable
> module, which we already knew. If you need to do something with the clocks
> early on, that has to be in built-in code and cannot be in a module. If you don't
> need that code, then you should just remove it from both the modular version
> and the built-in version.
> 
> What is the purpose of that __setup_param() argument parsing in the clock
> driver?
> 

We need the code for proper uart clock management of earlycon, from the code, it
is trying to keep console uart clock enabled during kernel boot up. 

Thanks,
Anson

^ permalink raw reply	[flat|nested] 39+ messages in thread

* RE: [PATCH V3 04/10] clk: imx: Support building SCU clock driver as module
  2020-07-01  8:46                   ` Arnd Bergmann
@ 2020-07-01  9:28                     ` Anson Huang
  2020-07-01  9:40                       ` Anson Huang
  0 siblings, 1 reply; 39+ messages in thread
From: Anson Huang @ 2020-07-01  9:28 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Dong Aisheng, Russell King - ARM Linux, Shawn Guo, Sascha Hauer,
	Sascha Hauer, Fabio Estevam, Michael Turquette, Stephen Boyd,
	oleksandr.suvorov, Stefan Agner, Peng Fan, Abel Vesa,
	Aisheng Dong, Andy Duan, Daniel Baluta, YueHaibing,
	Stephen Rothwell, Al Viro, Linux ARM, linux-kernel, linux-clk,
	dl-linux-imx

Hi, Arnd

> Subject: Re: [PATCH V3 04/10] clk: imx: Support building SCU clock driver as
> module
> 
> On Wed, Jul 1, 2020 at 9:19 AM Anson Huang <anson.huang@nxp.com>
> wrote:
> > > Subject: Re: [PATCH V3 04/10] clk: imx: Support building SCU clock
> > > driver as module On Tue, Jun 30, 2020 at 5:16 AM Anson Huang
> <anson.huang@nxp.com> wrote:
> > >
> > > SCU clock driver is a common driver for all SCU based platforms.
> > > Current i.MX8QXP SCU clock driver will be extended to support all
> > > future SCU based platforms.
> > > So theoretically clk-scu.o and clk-imx8qxp.o can be combined. LPCG
> > > is similar.
> > > Maybe you can give a try as Arnd suggested.
> > >
> >
> > Do we really need to link clk-scu and i.MX8QXP clock driver together just to
> avoid some export?
> 
> It was just meant to be easier than exporting a symbol and dealing with
> module dependencies. If it's not easier, then don't.
> 
> > I met some build issues if using this method, the i.MX8QXP module
> > build is OK, but other platforms like
> i.MX8MM/i.MX8MN/i.MX8MQ/i.MX8MP
> > clock drivers are just skipped for build, even these i.MX8M clock
> > config are existing in .config, anyone know why? Looks like the change in
> Makefile for i.MX8QXP clock driver introduce this issue.
> 
> You have a ":=" instead of "+=" typo, so all earlier "+=" are ignored:
> 
> > 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_MXC_CLK_SCU) := clk-imx-scu.o clk-imx-lpcg-scu.o
> 
>                                    ^^^^^^^^

Thanks, I will give another try, I will make the common clk part all linked into each
platform's clock driver, then many exports can be saved.

Thanks,
Anson

^ permalink raw reply	[flat|nested] 39+ messages in thread

* RE: [PATCH V3 04/10] clk: imx: Support building SCU clock driver as module
  2020-07-01  9:28                     ` Anson Huang
@ 2020-07-01  9:40                       ` Anson Huang
  2020-07-01  9:54                         ` Arnd Bergmann
  0 siblings, 1 reply; 39+ messages in thread
From: Anson Huang @ 2020-07-01  9:40 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Dong Aisheng, Russell King - ARM Linux, Shawn Guo, Sascha Hauer,
	Sascha Hauer, Fabio Estevam, Michael Turquette, Stephen Boyd,
	oleksandr.suvorov, Stefan Agner, Peng Fan, Abel Vesa,
	Aisheng Dong, Andy Duan, Daniel Baluta, YueHaibing,
	Stephen Rothwell, Al Viro, Linux ARM, linux-kernel, linux-clk,
	dl-linux-imx

Hi, Arnd

> Subject: RE: [PATCH V3 04/10] clk: imx: Support building SCU clock driver as
> module
> 
> Hi, Arnd
> 
> > Subject: Re: [PATCH V3 04/10] clk: imx: Support building SCU clock
> > driver as module
> >
> > On Wed, Jul 1, 2020 at 9:19 AM Anson Huang <anson.huang@nxp.com>
> > wrote:
> > > > Subject: Re: [PATCH V3 04/10] clk: imx: Support building SCU clock
> > > > driver as module On Tue, Jun 30, 2020 at 5:16 AM Anson Huang
> > <anson.huang@nxp.com> wrote:
> > > >
> > > > SCU clock driver is a common driver for all SCU based platforms.
> > > > Current i.MX8QXP SCU clock driver will be extended to support all
> > > > future SCU based platforms.
> > > > So theoretically clk-scu.o and clk-imx8qxp.o can be combined. LPCG
> > > > is similar.
> > > > Maybe you can give a try as Arnd suggested.
> > > >
> > >
> > > Do we really need to link clk-scu and i.MX8QXP clock driver together
> > > just to
> > avoid some export?
> >
> > It was just meant to be easier than exporting a symbol and dealing
> > with module dependencies. If it's not easier, then don't.
> >
> > > I met some build issues if using this method, the i.MX8QXP module
> > > build is OK, but other platforms like
> > i.MX8MM/i.MX8MN/i.MX8MQ/i.MX8MP
> > > clock drivers are just skipped for build, even these i.MX8M clock
> > > config are existing in .config, anyone know why? Looks like the
> > > change in
> > Makefile for i.MX8QXP clock driver introduce this issue.
> >
> > You have a ":=" instead of "+=" typo, so all earlier "+=" are ignored:
> >
> > > 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_MXC_CLK_SCU) := clk-imx-scu.o clk-imx-lpcg-scu.o
> >
> >                                    ^^^^^^^^
> 
> Thanks, I will give another try, I will make the common clk part all linked into
> each platform's clock driver, then many exports can be saved.

Just tried, it works for i.MX8QXP, andcorrect one thing, other platforms
need more complicated change if want to support them in the same way, so I
plan to ONLY do this change for i.MX8QXP if it is acceptable.

Thanks,
Anson

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH V3 02/10] init.h: Fix the __setup_param() macro for module build
  2020-07-01  9:27             ` Anson Huang
@ 2020-07-01  9:53               ` Arnd Bergmann
  2020-07-01 10:02                 ` Anson Huang
  0 siblings, 1 reply; 39+ messages in thread
From: Arnd Bergmann @ 2020-07-01  9:53 UTC (permalink / raw)
  To: Anson Huang
  Cc: Russell King - ARM Linux, Shawn Guo, Sascha Hauer, Sascha Hauer,
	Fabio Estevam, Michael Turquette, Stephen Boyd,
	oleksandr.suvorov, Stefan Agner, Peng Fan, Abel Vesa,
	Aisheng Dong, Andy Duan, Daniel Baluta, YueHaibing,
	Stephen Rothwell, Al Viro, Linux ARM, linux-kernel, linux-clk,
	dl-linux-imx

On Wed, Jul 1, 2020 at 11:27 AM Anson Huang <anson.huang@nxp.com> wrote:
> > Subject: Re: [PATCH V3 02/10] init.h: Fix the __setup_param() macro for
> > module build
> >
> > On Wed, Jul 1, 2020 at 7:14 AM Anson Huang <anson.huang@nxp.com>
> > wrote:
> >
> > I don't understand what your plan is here. Do you mean you will leave that
> > part of the clk driver as built-in?
>
> I meant I will leave the #else block of __setup_param() defined as nothing as below to
> make module build passed.
>
> #define __setup_param(str, unique_id, fn, early)        /* nothing */

No, I think that is  mistake. It will mean that other drivers with the same
bug as the imx-clk driver will appear to build fine, but not work correctly.

A build error is better than silently dropping the command line parsing in
my opinion.

> > This error just means you can't have a __setup_param() call in a loadable
> > module, which we already knew. If you need to do something with the clocks
> > early on, that has to be in built-in code and cannot be in a module. If you don't
> > need that code, then you should just remove it from both the modular version
> > and the built-in version.
> >
> > What is the purpose of that __setup_param() argument parsing in the clock
> > driver?
>
> We need the code for proper uart clock management of earlycon, from the code, it
> is trying to keep console uart clock enabled during kernel boot up.

Why not move this all to a separate file then and only build it when
CONFIG_CLK_IMX=y?
It seems that you don't need the imx_keep_uart_clocks_param() if the
clk driver is
loaded as a module, but then you also don't need the imx_clk_disable_uart()
and imx_register_uart_clocks() functions or the associated variables.

     Arnd

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH V3 04/10] clk: imx: Support building SCU clock driver as module
  2020-07-01  9:40                       ` Anson Huang
@ 2020-07-01  9:54                         ` Arnd Bergmann
  0 siblings, 0 replies; 39+ messages in thread
From: Arnd Bergmann @ 2020-07-01  9:54 UTC (permalink / raw)
  To: Anson Huang
  Cc: Dong Aisheng, Russell King - ARM Linux, Shawn Guo, Sascha Hauer,
	Sascha Hauer, Fabio Estevam, Michael Turquette, Stephen Boyd,
	oleksandr.suvorov, Stefan Agner, Peng Fan, Abel Vesa,
	Aisheng Dong, Andy Duan, Daniel Baluta, YueHaibing,
	Stephen Rothwell, Al Viro, Linux ARM, linux-kernel, linux-clk,
	dl-linux-imx

On Wed, Jul 1, 2020 at 11:40 AM Anson Huang <anson.huang@nxp.com> wrote:
> > Subject: RE: [PATCH V3 04/10] clk: imx: Support building SCU clock driver as
> > module
> >
> >
> > Thanks, I will give another try, I will make the common clk part all linked into
> > each platform's clock driver, then many exports can be saved.
>
> Just tried, it works for i.MX8QXP, andcorrect one thing, other platforms
> need more complicated change if want to support them in the same way, so I
> plan to ONLY do this change for i.MX8QXP if it is acceptable.

Yes, that is what I expected anyway.

      Arnd

^ permalink raw reply	[flat|nested] 39+ messages in thread

* RE: [PATCH V3 02/10] init.h: Fix the __setup_param() macro for module build
  2020-07-01  9:53               ` Arnd Bergmann
@ 2020-07-01 10:02                 ` Anson Huang
  2020-07-01 10:13                   ` Arnd Bergmann
  0 siblings, 1 reply; 39+ messages in thread
From: Anson Huang @ 2020-07-01 10:02 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Russell King - ARM Linux, Shawn Guo, Sascha Hauer, Sascha Hauer,
	Fabio Estevam, Michael Turquette, Stephen Boyd,
	oleksandr.suvorov, Stefan Agner, Peng Fan, Abel Vesa,
	Aisheng Dong, Andy Duan, Daniel Baluta, YueHaibing,
	Stephen Rothwell, Al Viro, Linux ARM, linux-kernel, linux-clk,
	dl-linux-imx

Hi, Arnd


> Subject: Re: [PATCH V3 02/10] init.h: Fix the __setup_param() macro for
> module build
> 
> On Wed, Jul 1, 2020 at 11:27 AM Anson Huang <anson.huang@nxp.com>
> wrote:
> > > Subject: Re: [PATCH V3 02/10] init.h: Fix the __setup_param() macro
> > > for module build
> > >
> > > On Wed, Jul 1, 2020 at 7:14 AM Anson Huang <anson.huang@nxp.com>
> > > wrote:
> > >
> > > I don't understand what your plan is here. Do you mean you will
> > > leave that part of the clk driver as built-in?
> >
> > I meant I will leave the #else block of __setup_param() defined as
> > nothing as below to make module build passed.
> >
> > #define __setup_param(str, unique_id, fn, early)        /* nothing */
> 
> No, I think that is  mistake. It will mean that other drivers with the same bug
> as the imx-clk driver will appear to build fine, but not work correctly.
> 
> A build error is better than silently dropping the command line parsing in my
> opinion.
> 
> > > This error just means you can't have a __setup_param() call in a
> > > loadable module, which we already knew. If you need to do something
> > > with the clocks early on, that has to be in built-in code and cannot
> > > be in a module. If you don't need that code, then you should just
> > > remove it from both the modular version and the built-in version.
> > >
> > > What is the purpose of that __setup_param() argument parsing in the
> > > clock driver?
> >
> > We need the code for proper uart clock management of earlycon, from
> > the code, it is trying to keep console uart clock enabled during kernel boot
> up.
> 
> Why not move this all to a separate file then and only build it when
> CONFIG_CLK_IMX=y?
> It seems that you don't need the imx_keep_uart_clocks_param() if the clk
> driver is loaded as a module, but then you also don't need the
> imx_clk_disable_uart() and imx_register_uart_clocks() functions or the
> associated variables.

If so, how about just adding "#ifndef MODULE" check for this part of code? I think
it should be easier/better than adding a file and build it conditionally?

Thanks,
Anson

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH V3 02/10] init.h: Fix the __setup_param() macro for module build
  2020-07-01 10:02                 ` Anson Huang
@ 2020-07-01 10:13                   ` Arnd Bergmann
  2020-07-01 10:20                     ` Anson Huang
  0 siblings, 1 reply; 39+ messages in thread
From: Arnd Bergmann @ 2020-07-01 10:13 UTC (permalink / raw)
  To: Anson Huang
  Cc: Russell King - ARM Linux, Shawn Guo, Sascha Hauer, Sascha Hauer,
	Fabio Estevam, Michael Turquette, Stephen Boyd,
	oleksandr.suvorov, Stefan Agner, Peng Fan, Abel Vesa,
	Aisheng Dong, Andy Duan, Daniel Baluta, YueHaibing,
	Stephen Rothwell, Al Viro, Linux ARM, linux-kernel, linux-clk,
	dl-linux-imx

On Wed, Jul 1, 2020 at 12:02 PM Anson Huang <anson.huang@nxp.com> wrote:
> > Subject: Re: [PATCH V3 02/10] init.h: Fix the __setup_param() macro for
> > module build
> > On Wed, Jul 1, 2020 at 11:27 AM Anson Huang <anson.huang@nxp.com>
> > wrote:
> > > > Subject: Re: [PATCH V3 02/10] init.h: Fix the __setup_param() macro
> > > > for module build
> > Why not move this all to a separate file then and only build it when
> > CONFIG_CLK_IMX=y?
> > It seems that you don't need the imx_keep_uart_clocks_param() if the clk
> > driver is loaded as a module, but then you also don't need the
> > imx_clk_disable_uart() and imx_register_uart_clocks() functions or the
> > associated variables.
>
> If so, how about just adding "#ifndef MODULE" check for this part of code? I think
> it should be easier/better than adding a file and build it conditionally?

The #ifdef is clearly a simpler change,  but I think a separate file is
a cleaner way to do it. Up to you (unless one of the imx or clk maintainers
has a preference -- I'm only helping out here, not making decisions).

      Arnd

^ permalink raw reply	[flat|nested] 39+ messages in thread

* RE: [PATCH V3 02/10] init.h: Fix the __setup_param() macro for module build
  2020-07-01 10:13                   ` Arnd Bergmann
@ 2020-07-01 10:20                     ` Anson Huang
  0 siblings, 0 replies; 39+ messages in thread
From: Anson Huang @ 2020-07-01 10:20 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Russell King - ARM Linux, Shawn Guo, Sascha Hauer, Sascha Hauer,
	Fabio Estevam, Michael Turquette, Stephen Boyd,
	oleksandr.suvorov, Stefan Agner, Peng Fan, Abel Vesa,
	Aisheng Dong, Andy Duan, Daniel Baluta, YueHaibing,
	Stephen Rothwell, Al Viro, Linux ARM, linux-kernel, linux-clk,
	dl-linux-imx

Hi, Arnd


> Subject: Re: [PATCH V3 02/10] init.h: Fix the __setup_param() macro for
> module build
> 
> On Wed, Jul 1, 2020 at 12:02 PM Anson Huang <anson.huang@nxp.com>
> wrote:
> > > Subject: Re: [PATCH V3 02/10] init.h: Fix the __setup_param() macro
> > > for module build On Wed, Jul 1, 2020 at 11:27 AM Anson Huang
> > > <anson.huang@nxp.com>
> > > wrote:
> > > > > Subject: Re: [PATCH V3 02/10] init.h: Fix the __setup_param()
> > > > > macro for module build
> > > Why not move this all to a separate file then and only build it when
> > > CONFIG_CLK_IMX=y?
> > > It seems that you don't need the imx_keep_uart_clocks_param() if the
> > > clk driver is loaded as a module, but then you also don't need the
> > > imx_clk_disable_uart() and imx_register_uart_clocks() functions or
> > > the associated variables.
> >
> > If so, how about just adding "#ifndef MODULE" check for this part of
> > code? I think it should be easier/better than adding a file and build it
> conditionally?
> 
> The #ifdef is clearly a simpler change,  but I think a separate file is a cleaner
> way to do it. Up to you (unless one of the imx or clk maintainers has a
> preference -- I'm only helping out here, not making decisions).
> 

OK, the first version of this patch introduced two __setup_param() implementation
for built-in and loadable module, that leads to all the discussion of fixing __setup_param()
in init.h etc., but you just remind me that __setup_param() is NOT necessary at all when
building loadable module, so I think just add '#ifndef MODULE' should be acceptable, I will
go with this change in V4 patch series, and see if anyone has comment.

Please help review V4 patch series which will be sent out soon.

Thanks,
Anson


^ permalink raw reply	[flat|nested] 39+ messages in thread

end of thread, other threads:[~2020-07-01 10:21 UTC | newest]

Thread overview: 39+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-29  5:53 [PATCH V3 00/10] Support building i.MX8 SoCs clock driver as module Anson Huang
2020-06-29  5:53 ` [PATCH V3 01/10] clk: composite: Export clk_hw_register_composite() Anson Huang
2020-06-29  5:53 ` [PATCH V3 02/10] init.h: Fix the __setup_param() macro for module build Anson Huang
2020-06-29 11:33   ` Arnd Bergmann
2020-06-29 11:40     ` Anson Huang
2020-06-29 11:58       ` Arnd Bergmann
2020-07-01  5:14         ` Anson Huang
2020-07-01  8:37           ` Arnd Bergmann
2020-07-01  9:27             ` Anson Huang
2020-07-01  9:53               ` Arnd Bergmann
2020-07-01 10:02                 ` Anson Huang
2020-07-01 10:13                   ` Arnd Bergmann
2020-07-01 10:20                     ` Anson Huang
2020-06-29  5:53 ` [PATCH V3 03/10] ARM: imx: Select MXC_CLK for each SoC Anson Huang
2020-06-29  7:27   ` Aisheng Dong
2020-06-29  8:21     ` Anson Huang
2020-06-29  5:53 ` [PATCH V3 04/10] clk: imx: Support building SCU clock driver as module Anson Huang
2020-06-29 11:37   ` Arnd Bergmann
2020-06-29 12:53     ` Anson Huang
2020-06-29 13:20       ` Arnd Bergmann
2020-06-29 14:52         ` Anson Huang
2020-06-29 15:08           ` Arnd Bergmann
2020-06-29 15:19             ` Anson Huang
2020-06-30  3:55               ` Dong Aisheng
2020-07-01  7:19                 ` Anson Huang
2020-07-01  8:46                   ` Arnd Bergmann
2020-07-01  9:28                     ` Anson Huang
2020-07-01  9:40                       ` Anson Huang
2020-07-01  9:54                         ` Arnd Bergmann
2020-06-30  3:36         ` Dong Aisheng
2020-06-29  5:53 ` [PATCH V3 05/10] clk: imx: Support building i.MX common " Anson Huang
2020-06-29  5:53 ` [PATCH V3 06/10] clk: imx8mm: Support module build Anson Huang
2020-06-29  5:53 ` [PATCH V3 07/10] clk: imx8mn: " Anson Huang
2020-06-29  5:54 ` [PATCH V3 08/10] clk: imx8mp: " Anson Huang
2020-06-29  5:54 ` [PATCH V3 09/10] clk: imx8mq: " Anson Huang
2020-06-29  5:54 ` [PATCH V3 10/10] clk: imx8qxp: " Anson Huang
2020-06-29 11:40   ` Arnd Bergmann
2020-06-29 11:43     ` Anson Huang
2020-06-29 11:58       ` Arnd Bergmann

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).