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
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 <[email protected]>
---
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
Support building i.MX8MM clock driver as module.
Signed-off-by: Anson Huang <[email protected]>
---
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
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 <[email protected]>
---
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
Support building i.MX8QXP clock driver as module.
Signed-off-by: Anson Huang <[email protected]>
---
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
Support building i.MX8MP clock driver as module.
Signed-off-by: Anson Huang <[email protected]>
---
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
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 <[email protected]>
---
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
Support building i.MX8MN clock driver as module.
Signed-off-by: Anson Huang <[email protected]>
---
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
On Mon, Jun 29, 2020 at 4:52 PM Anson Huang <[email protected]> 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 <[email protected]> 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
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 <[email protected]>
> 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
On Mon, Jun 29, 2020 at 2:53 PM Anson Huang <[email protected]> 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 <[email protected]>
> > 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
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 <[email protected]>
> 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 <[email protected]>
> > > 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
Hi, Arnd
> Subject: Re: [PATCH V3 10/10] clk: imx8qxp: Support module build
>
> On Mon, Jun 29, 2020 at 8:06 AM Anson Huang <[email protected]>
> wrote:
> >
> > Support building i.MX8QXP clock driver as module.
> >
> > Signed-off-by: Anson Huang <[email protected]>
>
> 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
On Mon, Jun 29, 2020 at 8:06 AM Anson Huang <[email protected]> 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 <[email protected]>
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
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 <[email protected]>
> 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 <[email protected]>
>
> 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
On Mon, Jun 29, 2020 at 8:06 AM Anson Huang <[email protected]> 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
On Mon, Jun 29, 2020 at 8:06 AM Anson Huang <[email protected]> wrote:
>
> Support building i.MX8QXP clock driver as module.
>
> Signed-off-by: Anson Huang <[email protected]>
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
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 <[email protected]>
> 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
> <[email protected]> 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
On Mon, Jun 29, 2020 at 1:40 PM Anson Huang <[email protected]> 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 <[email protected]>
> > 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 <[email protected]>
> >
> > 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
On Mon, Jun 29, 2020 at 1:43 PM Anson Huang <[email protected]> wrote:
> > Subject: Re: [PATCH V3 10/10] clk: imx8qxp: Support module build
> >
> > On Mon, Jun 29, 2020 at 8:06 AM Anson Huang <[email protected]>
> > wrote:
> > >
> > > Support building i.MX8QXP clock driver as module.
> > >
> > > Signed-off-by: Anson Huang <[email protected]>
> >
> > 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
On Tue, Jun 30, 2020 at 3:36 AM Arnd Bergmann <[email protected]> wrote:
>
> On Mon, Jun 29, 2020 at 2:53 PM Anson Huang <[email protected]> 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 <[email protected]>
> > > 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
On Tue, Jun 30, 2020 at 5:16 AM Anson Huang <[email protected]> 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 <[email protected]>
> > 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
> > <[email protected]> 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
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 <[email protected]>
> 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 <[email protected]>
> > > 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 <[email protected]>
> > >
> > > 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
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 <[email protected]>
> 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 <[email protected]>
> > > 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
> > > <[email protected]> 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
On Wed, Jul 1, 2020 at 7:14 AM Anson Huang <[email protected]> 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 <[email protected]>
> > 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
On Wed, Jul 1, 2020 at 9:19 AM Anson Huang <[email protected]> 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 <[email protected]> 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
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 <[email protected]>
> 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
> <[email protected]> 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
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 <[email protected]>
> 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
> > > <[email protected]>
> > > 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
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 <[email protected]>
> > 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
> > <[email protected]> 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
On Wed, Jul 1, 2020 at 11:27 AM Anson Huang <[email protected]> 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 <[email protected]>
> > 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
On Wed, Jul 1, 2020 at 11:40 AM Anson Huang <[email protected]> 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
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 <[email protected]>
> 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 <[email protected]>
> > > 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
On Wed, Jul 1, 2020 at 12:02 PM Anson Huang <[email protected]> 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 <[email protected]>
> > 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
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 <[email protected]>
> 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
> > > <[email protected]>
> > > 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