# Re: \[PATCH v2 04/18\] clocksource: Add ARM System timer driver - List: linux-api - 7 messages, 3 authors - Page: https://ratatoskr.run/linux-api/2015/02/4434835/t ## Re: \[PATCH v2 04/18\] clocksource: Add ARM System timer driver - From: Uwe Kleine-König \ - Date: 2015-02-20 19:55:27 - Message: https://ratatoskr.run/linux-api/2015/02/4434835.md ```text Hello, On Fri, Feb 20, 2015 at 07:01:03PM +0100, Maxime Coquelin wrote: > This patch adds clocksource support for ARMv7-M's System timer, > also known as SysTick. > > Signed-off-by: Maxime Coquelin > --- > drivers/clocksource/Kconfig | 7 ++++ > drivers/clocksource/Makefile | 1 + > drivers/clocksource/armv7m_systick.c | 78 ++++++++++++++++++++++++++++++++++++ > 3 files changed, 86 insertions(+) > create mode 100644 drivers/clocksource/armv7m_systick.c > > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig > index fc01ec2..fb6011e 100644 > --- a/drivers/clocksource/Kconfig > +++ b/drivers/clocksource/Kconfig > @@ -124,6 +124,13 @@ config CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCK > help > Use ARM global timer clock source as sched_clock > > +config ARMV7M_SYSTICK > + bool I assume this symbol is enabled later in the series. Would it make sense to allow enabing the symbol for compile test coverage? > + select CLKSRC_OF if OF What happens if ARMV7M_SYSTICK=y but OF=n? Doesn't the driver fail to compile? > + select CLKSRC_MMIO > + help > + This options enables support for the ARMv7M system timer unit the right spelling is ARMv7-M. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ | ``` ## Re: \[PATCH v2 04/18\] clocksource: Add ARM System timer driver - From: Paul Bolle \ - Date: 2015-02-20 21:48:35 - Message: https://ratatoskr.run/linux-api/2015/02/4434886.md ```text On Fri, 2015-02-20 at 20:54 +0100, Uwe Kleine-König wrote: > On Fri, Feb 20, 2015 at 07:01:03PM +0100, Maxime Coquelin wrote: > > This patch adds clocksource support for ARMv7-M's System timer, > > also known as SysTick. > > > > Signed-off-by: Maxime Coquelin > > --- > > drivers/clocksource/Kconfig | 7 ++++ > > drivers/clocksource/Makefile | 1 + > > drivers/clocksource/armv7m_systick.c | 78 ++++++++++++++++++++++++++++++++++++ > > 3 files changed, 86 insertions(+) > > create mode 100644 drivers/clocksource/armv7m_systick.c > > > > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig > > index fc01ec2..fb6011e 100644 > > --- a/drivers/clocksource/Kconfig > > +++ b/drivers/clocksource/Kconfig > > @@ -124,6 +124,13 @@ config CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCK > > help > > Use ARM global timer clock source as sched_clock > > > > +config ARMV7M_SYSTICK > > + bool > I assume this symbol is enabled later in the series. Yes, I noticed it was selected in 14/18 ("ARM: Add STM32 family machine"). > Would it make sense > to allow enabing the symbol for compile test coverage? > > > + select CLKSRC_OF if OF > What happens if ARMV7M_SYSTICK=y but OF=n? Doesn't the driver fail to > compile? > > > + select CLKSRC_MMIO > > + help > > + This options enables support for the ARMv7M system timer unit > the right spelling is ARMv7-M. This Kconfig entry has no prompt, so no one is going to see this text during make *config. Perhaps this should be made a comment. In that case the right spelling should still be used. Thanks, Paul Bolle ``` ## Re: \[PATCH v2 04/18\] clocksource: Add ARM System timer driver - From: Maxime Coquelin \ - Date: 2015-03-02 16:53:17 - Message: https://ratatoskr.run/linux-api/2015/03/4440547.md ```text Hi Paul, Uwe, 2015-02-20 22:48 GMT+01:00 Paul Bolle [ref]: > On Fri, 2015-02-20 at 20:54 +0100, Uwe Kleine-König wrote: >> On Fri, Feb 20, 2015 at 07:01:03PM +0100, Maxime Coquelin wrote: >> > This patch adds clocksource support for ARMv7-M's System timer, >> > also known as SysTick. >> > >> > Signed-off-by: Maxime Coquelin >> > --- >> > drivers/clocksource/Kconfig | 7 ++++ >> > drivers/clocksource/Makefile | 1 + >> > drivers/clocksource/armv7m_systick.c | 78 ++++++++++++++++++++++++++++++++++++ >> > 3 files changed, 86 insertions(+) >> > create mode 100644 drivers/clocksource/armv7m_systick.c >> > >> > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig >> > index fc01ec2..fb6011e 100644 >> > --- a/drivers/clocksource/Kconfig >> > +++ b/drivers/clocksource/Kconfig >> > @@ -124,6 +124,13 @@ config CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCK >> > help >> > Use ARM global timer clock source as sched_clock >> > >> > +config ARMV7M_SYSTICK >> > + bool >> I assume this symbol is enabled later in the series. > > Yes, I noticed it was selected in 14/18 ("ARM: Add STM32 family > machine"). > >> Would it make sense >> to allow enabing the symbol for compile test coverage? >> >> > + select CLKSRC_OF if OF >> What happens if ARMV7M_SYSTICK=y but OF=n? Doesn't the driver fail to >> compile? >> >> > + select CLKSRC_MMIO >> > + help >> > + This options enables support for the ARMv7M system timer unit >> the right spelling is ARMv7-M. > > This Kconfig entry has no prompt, so no one is going to see this text > during make *config. Perhaps this should be made a comment. In that case > the right spelling should still be used. Yes, you are right. Do you agree if I define it like this: config ARMV7M_SYSTICK bool "Clocksource driver for ARMv7-M System timer" depends on OF && (CPU_V7M || COMPILE_TEST) select CLKSRC_OF select CLKSRC_MMIO help This options enables clocksource support for the ARMv7-M system timer unit. Thanks, Maxime > > Thanks, > > > Paul Bolle > ``` ## Re: \[PATCH v2 04/18\] clocksource: Add ARM System timer driver - From: Paul Bolle \ - Date: 2015-03-03 19:43:16 - Message: https://ratatoskr.run/linux-api/2015/03/4441809.md ```text Maxime Coquelin schreef op ma 02-03-2015 om 17:53 [+0100]: > Do you agree if I define it like this: > > config ARMV7M_SYSTICK > bool "Clocksource driver for ARMv7-M System timer" > depends on OF && (CPU_V7M || COMPILE_TEST) > select CLKSRC_OF > select CLKSRC_MMIO > help > This options enables clocksource support for the ARMv7-M system > timer unit. I don't really have strong feelings on whatever way you choose to fix the, well, minor problem I pointed out. Having said that, if a Kconfig entry without a prompt (and therefor, without help) actually does what you want it to do, why bother adding a prompt and a one line help text? Paul Bolle ``` ## Re: \[PATCH v2 04/18\] clocksource: Add ARM System timer driver - From: Maxime Coquelin \ - Date: 2015-03-04 12:08:12 - Message: https://ratatoskr.run/linux-api/2015/03/4442874.md ```text 2015-03-03 20:43 GMT+01:00 Paul Bolle [ref]: > Maxime Coquelin schreef op ma 02-03-2015 om 17:53 [+0100]: >> Do you agree if I define it like this: >> >> config ARMV7M_SYSTICK >> bool "Clocksource driver for ARMv7-M System timer" >> depends on OF && (CPU_V7M || COMPILE_TEST) >> select CLKSRC_OF >> select CLKSRC_MMIO >> help >> This options enables clocksource support for the ARMv7-M system >> timer unit. > > I don't really have strong feelings on whatever way you choose to fix > the, well, minor problem I pointed out. > > Having said that, if a Kconfig entry without a prompt (and therefor, > without help) actually does what you want it to do, why bother adding a > prompt and a one line help text? This is because I added also support for COMPILE_TEST coverage as per Uwe advice, and thought it was necessary to have an entry for this. Maybe I'm just wrong? Thanks, Maxime > > > Paul Bolle > ``` ## Re: \[PATCH v2 04/18\] clocksource: Add ARM System timer driver - From: Paul Bolle \ - Date: 2015-03-09 21:12:43 - Message: https://ratatoskr.run/linux-api/2015/03/4446995.md ```text On Wed, 2015-03-04 at 13:08 +0100, Maxime Coquelin wrote: > This is because I added also support for COMPILE_TEST coverage as per > Uwe advice, > and thought it was necessary to have an entry for this. > Maybe I'm just wrong? I missed that you added COMPILE_TEST. A quick scan of your idea doesn't show any obvious issues. (Note that I don't really know how people actually use COMPILE_TEST. I guess things like "make allyesconfig" are involved.) Paul Bolle ``` ## Re: \[PATCH v2 04/18\] clocksource: Add ARM System timer driver - From: Uwe Kleine-König \ - Date: 2015-03-09 22:18:55 - Message: https://ratatoskr.run/linux-api/2015/03/4447039.md ```text Hello, On Mon, Mar 09, 2015 at 10:12:32PM +0100, Paul Bolle wrote: > On Wed, 2015-03-04 at 13:08 +0100, Maxime Coquelin wrote: > > This is because I added also support for COMPILE_TEST coverage as per > > Uwe advice, > > and thought it was necessary to have an entry for this. > > Maybe I'm just wrong? > > I missed that you added COMPILE_TEST. > > A quick scan of your idea doesn't show any obvious issues. (Note that I > don't really know how people actually use COMPILE_TEST. I guess things > like "make allyesconfig" are involved.) Maybe this can clearify the purpose of COMPILE_TEST: diff --git a/init/Kconfig b/init/Kconfig --- a/init/Kconfig +++ b/init/Kconfig @@ -67,6 +67,26 @@ config COMPILE_TEST here. If you are a user/distributor, say N here to exclude useless drivers to be distributed. + # If you are a driver author consider to adjust your driver's + # dependencies to make it buildable with minimal preconditions if + # COMPILE_TEST is enabled. This helps contributers and maintainers + # that might not have the necessary toolchain or kernel config handy and + # also increases compile test coverage. It's your advantage if others can + # build your driver more easily! So for a device that is only found on the + # foo cpu use: + # + # depends on CPU_FOO || COMPILE_TEST + # + # . You might have to use + # + # depends on CPU_FOO || (COMPILE_TEST && COOKIE) + # + # or + # + # depends on COOKIE && (CPU_FOO || COMPILE_TEST) + # + # if your driver uses features that are only available if COOKIE is on. + config LOCALVERSION string "Local version - append to kernel release" help -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ | ```