| Message ID | 1472045342-7434-1-git-send-email-p.zabel@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wednesday, August 24, 2016 3:28:53 PM CEST Philipp Zabel wrote: > if RESET_CONTROLLER > > +config RESET_ATH79 > + bool "AR71xx Reset Driver" if COMPILE_TEST > + default ATH79 > + help > + This enables the ATH79 reset controller driver that supports the > + AR71xx SoC reset controller. > + > Nice series! Just note that there is one possible problem with COMPILE_TEST when the platforms are enabled, as you can then disable a driver that is normally there, and that can in turn cause problems in rare cases, e.g. when the driver has a global function that is called from platform code. I don't know if any of the drivers do that, but if they do, you'd have to use config RESET_ATH79 bool "AR71xx Reset Driver" if COMPILE_TEST && !ATH79 default ATH79 to ensure that it's impossible to disable the driver on platforms that require it. Arnd
2016-08-24 22:28 GMT+09:00 Philipp Zabel <p.zabel@pengutronix.de>: > Visible only if COMPILE_TEST is enabled, this allows to include the > driver in build tests. > > Cc: Alban Bedel <albeu@free.fr> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> Reviewed-by: Masahiro Yamada <yamada.masahiro@socionext.com>
Hi Arnd, 2016-08-25 0:51 GMT+09:00 Arnd Bergmann <arnd@arndb.de>: > On Wednesday, August 24, 2016 3:28:53 PM CEST Philipp Zabel wrote: >> if RESET_CONTROLLER >> >> +config RESET_ATH79 >> + bool "AR71xx Reset Driver" if COMPILE_TEST >> + default ATH79 >> + help >> + This enables the ATH79 reset controller driver that supports the >> + AR71xx SoC reset controller. >> + >> > > Nice series! > > Just note that there is one possible problem with COMPILE_TEST > when the platforms are enabled, as you can then disable a driver > that is normally there, and that can in turn cause problems in > rare cases, e.g. when the driver has a global function that is > called from platform code. I don't know if any of the drivers > do that, but if they do, you'd have to use > > config RESET_ATH79 > bool "AR71xx Reset Driver" if COMPILE_TEST && !ATH79 > default ATH79 > > to ensure that it's impossible to disable the driver on platforms > that require it. Hmm, Can we do this only when we really have to do so? I think we should not care about such a rare case that may not happen. Let's start with only "if COMPILE_TEST", and take a look at it if a build error is detected. Anyway, depending on platform code is a sign of weird implementation. It might be better to find a potential issue rather than hide it.
On Thursday, August 25, 2016 3:18:55 AM CEST Masahiro Yamada wrote: > Hi Arnd, > > > 2016-08-25 0:51 GMT+09:00 Arnd Bergmann <arnd@arndb.de>: > > On Wednesday, August 24, 2016 3:28:53 PM CEST Philipp Zabel wrote: > >> if RESET_CONTROLLER > >> > >> +config RESET_ATH79 > >> + bool "AR71xx Reset Driver" if COMPILE_TEST > >> + default ATH79 > >> + help > >> + This enables the ATH79 reset controller driver that supports the > >> + AR71xx SoC reset controller. > >> + > >> > > > > Nice series! > > > > Just note that there is one possible problem with COMPILE_TEST > > when the platforms are enabled, as you can then disable a driver > > that is normally there, and that can in turn cause problems in > > rare cases, e.g. when the driver has a global function that is > > called from platform code. I don't know if any of the drivers > > do that, but if they do, you'd have to use > > > > config RESET_ATH79 > > bool "AR71xx Reset Driver" if COMPILE_TEST && !ATH79 > > default ATH79 > > > > to ensure that it's impossible to disable the driver on platforms > > that require it. > > Hmm, > Can we do this only when we really have to do so? > I think we should not care about such a rare case that may not happen. > > Let's start with only "if COMPILE_TEST", > and take a look at it if a build error is detected. > > Anyway, depending on platform code is a sign of weird implementation. > > It might be better to find a potential issue rather than hide it. > > > I just checked the object files in an allyesconfig build and found one instance: arch/arm/mach-sunxi/sunxi.c:extern void __init sun6i_reset_init(void); arch/arm/mach-sunxi/sunxi.c: sun6i_reset_init(); drivers/reset/reset-sunxi.c:void __init sun6i_reset_init(void) We should definitely make sure this one is handled right, and maybe check the source code for other instances. Arnd
2016-08-25 5:06 GMT+09:00 Arnd Bergmann <arnd@arndb.de>: > On Thursday, August 25, 2016 3:18:55 AM CEST Masahiro Yamada wrote: >> Hi Arnd, >> >> >> 2016-08-25 0:51 GMT+09:00 Arnd Bergmann <arnd@arndb.de>: >> > On Wednesday, August 24, 2016 3:28:53 PM CEST Philipp Zabel wrote: >> >> if RESET_CONTROLLER >> >> >> >> +config RESET_ATH79 >> >> + bool "AR71xx Reset Driver" if COMPILE_TEST >> >> + default ATH79 >> >> + help >> >> + This enables the ATH79 reset controller driver that supports the >> >> + AR71xx SoC reset controller. >> >> + >> >> >> > >> > Nice series! >> > >> > Just note that there is one possible problem with COMPILE_TEST >> > when the platforms are enabled, as you can then disable a driver >> > that is normally there, and that can in turn cause problems in >> > rare cases, e.g. when the driver has a global function that is >> > called from platform code. I don't know if any of the drivers >> > do that, but if they do, you'd have to use >> > >> > config RESET_ATH79 >> > bool "AR71xx Reset Driver" if COMPILE_TEST && !ATH79 >> > default ATH79 >> > >> > to ensure that it's impossible to disable the driver on platforms >> > that require it. >> >> Hmm, >> Can we do this only when we really have to do so? >> I think we should not care about such a rare case that may not happen. >> >> Let's start with only "if COMPILE_TEST", >> and take a look at it if a build error is detected. >> >> Anyway, depending on platform code is a sign of weird implementation. >> >> It might be better to find a potential issue rather than hide it. >> >> >> > > I just checked the object files in an allyesconfig build and found > one instance: > > arch/arm/mach-sunxi/sunxi.c:extern void __init sun6i_reset_init(void); > arch/arm/mach-sunxi/sunxi.c: sun6i_reset_init(); > drivers/reset/reset-sunxi.c:void __init sun6i_reset_init(void) > > We should definitely make sure this one is handled right, and maybe > check the source code for other instances. Hmm. Is is solved with RESET_OF_DECLARE(), like we have CLK_OF_DECLARE() ? Or, use something like postcore_initcall() to probe it really early? Not sure...
Am Mittwoch, den 24.08.2016, 22:06 +0200 schrieb Arnd Bergmann: > On Thursday, August 25, 2016 3:18:55 AM CEST Masahiro Yamada wrote: > > Hi Arnd, > > > > > > 2016-08-25 0:51 GMT+09:00 Arnd Bergmann <arnd@arndb.de>: > > > On Wednesday, August 24, 2016 3:28:53 PM CEST Philipp Zabel wrote: > > >> if RESET_CONTROLLER > > >> > > >> +config RESET_ATH79 > > >> + bool "AR71xx Reset Driver" if COMPILE_TEST > > >> + default ATH79 > > >> + help > > >> + This enables the ATH79 reset controller driver that supports the > > >> + AR71xx SoC reset controller. > > >> + > > >> > > > > > > Nice series! > > > > > > Just note that there is one possible problem with COMPILE_TEST > > > when the platforms are enabled, as you can then disable a driver > > > that is normally there, and that can in turn cause problems in > > > rare cases, e.g. when the driver has a global function that is > > > called from platform code. I don't know if any of the drivers > > > do that, but if they do, you'd have to use > > > > > > config RESET_ATH79 > > > bool "AR71xx Reset Driver" if COMPILE_TEST && !ATH79 > > > default ATH79 > > > > > > to ensure that it's impossible to disable the driver on platforms > > > that require it. > > > > Hmm, > > Can we do this only when we really have to do so? > > I think we should not care about such a rare case that may not happen. > > > > Let's start with only "if COMPILE_TEST", > > and take a look at it if a build error is detected. > > > > Anyway, depending on platform code is a sign of weird implementation. > > > > It might be better to find a potential issue rather than hide it. > > > > > > > > I just checked the object files in an allyesconfig build and found > one instance: > > arch/arm/mach-sunxi/sunxi.c:extern void __init sun6i_reset_init(void); > arch/arm/mach-sunxi/sunxi.c: sun6i_reset_init(); > drivers/reset/reset-sunxi.c:void __init sun6i_reset_init(void) > > We should definitely make sure this one is handled right, and maybe > check the source code for other instances. I'll add "&& !ARCH_SUNXI" to the RESET_SUNXI symbol The only other drivers that have __init symbols are the STiH4xx reset drivers, which I haven't touched yet: STIH4xx_RESET symbols are already selected by their respective SOC_STIH4xx under ARCH_STI. And the hi6220 driver, which doesn't have a user yet. regards Philipp
2016-08-25 16:22 GMT+09:00 Philipp Zabel <p.zabel@pengutronix.de>: > And the hi6220 > driver, which doesn't have a user yet. It does. See arch/arm64/configs/defconfig
Am Donnerstag, den 25.08.2016, 16:27 +0900 schrieb Masahiro Yamada: > 2016-08-25 16:22 GMT+09:00 Philipp Zabel <p.zabel@pengutronix.de>: > > And the hi6220 > > driver, which doesn't have a user yet. > > It does. > > See arch/arm64/configs/defconfig My mistake, I missed that hi6220_reset_init is a postcore_initcall, so there is no compile time dependency on the reset driver. regards Philipp
Hi Philipp,
[auto build test ERROR on pza/reset/next]
[also build test ERROR on v4.8-rc3 next-20160824]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
[Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on]
[Check https://git-scm.com/docs/git-format-patch for more information]
url: https://github.com/0day-ci/linux/commits/Philipp-Zabel/reset-ath79-add-driver-Kconfig-option/20160824-213846
base: git://git.pengutronix.de/git/pza/linux reset/next
config: blackfin-allyesconfig (attached as .config)
compiler: bfin-uclinux-gcc (GCC) 4.6.3
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=blackfin
All errors (new ones prefixed by >>):
drivers/reset/reset-ath79.c: In function 'ath79_reset_update':
>> drivers/reset/reset-ath79.c:38:2: error: implicit declaration of function 'readl' [-Werror=implicit-function-declaration]
>> drivers/reset/reset-ath79.c:43:2: error: implicit declaration of function 'writel' [-Werror=implicit-function-declaration]
cc1: some warnings being treated as errors
vim +/readl +38 drivers/reset/reset-ath79.c
ff591a91 Alban Bedel 2015-08-03 32 struct ath79_reset *ath79_reset =
ff591a91 Alban Bedel 2015-08-03 33 container_of(rcdev, struct ath79_reset, rcdev);
ff591a91 Alban Bedel 2015-08-03 34 unsigned long flags;
ff591a91 Alban Bedel 2015-08-03 35 u32 val;
ff591a91 Alban Bedel 2015-08-03 36
ff591a91 Alban Bedel 2015-08-03 37 spin_lock_irqsave(&ath79_reset->lock, flags);
ff591a91 Alban Bedel 2015-08-03 @38 val = readl(ath79_reset->base);
ff591a91 Alban Bedel 2015-08-03 39 if (assert)
ff591a91 Alban Bedel 2015-08-03 40 val |= BIT(id);
ff591a91 Alban Bedel 2015-08-03 41 else
ff591a91 Alban Bedel 2015-08-03 42 val &= ~BIT(id);
ff591a91 Alban Bedel 2015-08-03 @43 writel(val, ath79_reset->base);
ff591a91 Alban Bedel 2015-08-03 44 spin_unlock_irqrestore(&ath79_reset->lock, flags);
ff591a91 Alban Bedel 2015-08-03 45
ff591a91 Alban Bedel 2015-08-03 46 return 0;
:::::: The code at line 38 was first introduced by commit
:::::: ff591a91225d3621a503bb18faa0f0d747a06e50 reset: Add a driver for the reset controller on the AR71XX/AR9XXX
:::::: TO: Alban Bedel <albeu@free.fr>
:::::: CC: Philipp Zabel <p.zabel@pengutronix.de>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Thursday, August 25, 2016 6:31:48 PM CEST kbuild test robot wrote: > > drivers/reset/reset-ath79.c: In function 'ath79_reset_update': > >> drivers/reset/reset-ath79.c:38:2: error: implicit declaration of function 'readl' [-Werror=implicit-function-declaration] > >> drivers/reset/reset-ath79.c:43:2: error: implicit declaration of function 'writel' [-Werror=implicit-function-declaration] > cc1: some warnings being treated as errors > > The file lacks "#include <linux/io.h" Arnd
Am Donnerstag, den 25.08.2016, 12:50 +0200 schrieb Arnd Bergmann: > On Thursday, August 25, 2016 6:31:48 PM CEST kbuild test robot wrote: > > > > drivers/reset/reset-ath79.c: In function 'ath79_reset_update': > > >> drivers/reset/reset-ath79.c:38:2: error: implicit declaration of function 'readl' [-Werror=implicit-function-declaration] > > >> drivers/reset/reset-ath79.c:43:2: error: implicit declaration of function 'writel' [-Werror=implicit-function-declaration] > > cc1: some warnings being treated as errors > > > > > > The file lacks "#include <linux/io.h" I already sent the fix separately, should have included it with the series. regards Philipp
On Wed, 24 Aug 2016 15:28:53 +0200 Philipp Zabel <p.zabel@pengutronix.de> wrote: > Visible only if COMPILE_TEST is enabled, this allows to include the > driver in build tests. > > Cc: Alban Bedel <albeu@free.fr> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> Acked-by: Aban Bedel <albeu@free.fr> Alban
diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig index 7dfe8d8..9da0507 100644 --- a/drivers/reset/Kconfig +++ b/drivers/reset/Kconfig @@ -14,6 +14,13 @@ menuconfig RESET_CONTROLLER if RESET_CONTROLLER +config RESET_ATH79 + bool "AR71xx Reset Driver" if COMPILE_TEST + default ATH79 + help + This enables the ATH79 reset controller driver that supports the + AR71xx SoC reset controller. + config RESET_OXNAS bool diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile index 9b45dcf..e3fc337 100644 --- a/drivers/reset/Makefile +++ b/drivers/reset/Makefile @@ -9,7 +9,7 @@ obj-$(CONFIG_ARCH_SUNXI) += reset-sunxi.o obj-$(CONFIG_ARCH_STI) += sti/ obj-$(CONFIG_ARCH_HISI) += hisilicon/ obj-$(CONFIG_ARCH_ZYNQ) += reset-zynq.o -obj-$(CONFIG_ATH79) += reset-ath79.o +obj-$(CONFIG_RESET_ATH79) += reset-ath79.o obj-$(CONFIG_RESET_OXNAS) += reset-oxnas.o obj-$(CONFIG_TI_SYSCON_RESET) += reset-ti-syscon.o obj-$(CONFIG_RESET_UNIPHIER) += reset-uniphier.o
Visible only if COMPILE_TEST is enabled, this allows to include the driver in build tests. Cc: Alban Bedel <albeu@free.fr> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> --- drivers/reset/Kconfig | 7 +++++++ drivers/reset/Makefile | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-)