| Message ID | 1593410042-10598-3-git-send-email-Anson.Huang@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support building i.MX8 SoCs clock driver as module | expand |
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
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
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
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
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
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
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
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
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
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
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
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(-)