The sp804 is currently only user selectable if COMPILE_TEST, this was
done by commit dfc82faad725 ("clocksource/drivers/sp804: Add
COMPILE_TEST to CONFIG_ARM_TIMER_SP804") in order to avoid it being
spuriously offered on platforms that won't have the hardware since it's
generally only seen on Arm based platforms.  This config is overly
restrictive, while platforms that rely on the SP804 do select it in
their Kconfig there are others such as the Arm fast models which have a
SP804 available but currently unused by Linux.  Relax the dependency to
allow it to be user selectable on arm and arm64 to avoid surprises and
in case someone comes up with a use for extra timer hardware.

Fixes: dfc82faad725 ("clocksource/drivers/sp804: Add COMPILE_TEST to CONFIG_ARM_TIMER_SP804")
Reported-by: Ross Burton <ross.burton@arm.com>
Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
Changes in v2:
- Rebase onto v6.10-rc1.
- Link to v1: https://lore.kernel.org/r/20240522-arm64-vexpress-sp804-v1-1-0344cd42eb77@kernel.org
---
 drivers/clocksource/Kconfig | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 34faa0320ece..ca6045f90000 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -390,7 +390,8 @@ config ARM_GT_INITIAL_PRESCALER_VAL
 	  This affects CPU_FREQ max delta from the initial frequency.
 
 config ARM_TIMER_SP804
-	bool "Support for Dual Timer SP804 module" if COMPILE_TEST
+	bool "Support for Dual Timer SP804 module"
+	depends on ARM || ARM64 || COMPILE_TEST
 	depends on GENERIC_SCHED_CLOCK && HAVE_CLK
 	select CLKSRC_MMIO
 	select TIMER_OF if OF

---
base-commit: 1613e604df0cd359cf2a7fbd9be7a0bcfacfabd0
change-id: 20240522-arm64-vexpress-sp804-365d6938b07c

Best regards,
-- 
Mark Brown <broonie@kernel.org>

On Wed, May 29, 2024 at 08:48:14PM +0100, Mark Brown wrote:
> The sp804 is currently only user selectable if COMPILE_TEST, this was
> done by commit dfc82faad725 ("clocksource/drivers/sp804: Add
> COMPILE_TEST to CONFIG_ARM_TIMER_SP804") in order to avoid it being
> spuriously offered on platforms that won't have the hardware since it's
> generally only seen on Arm based platforms.  This config is overly
> restrictive, while platforms that rely on the SP804 do select it in
> their Kconfig there are others such as the Arm fast models which have a
> SP804 available but currently unused by Linux.  Relax the dependency to
> allow it to be user selectable on arm and arm64 to avoid surprises and
> in case someone comes up with a use for extra timer hardware.
> 
> Fixes: dfc82faad725 ("clocksource/drivers/sp804: Add COMPILE_TEST to CONFIG_ARM_TIMER_SP804")
> Reported-by: Ross Burton <ross.burton@arm.com>
> Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
> Signed-off-by: Mark Brown <broonie@kernel.org>

Acked-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> ---
> Changes in v2:
> - Rebase onto v6.10-rc1.
> - Link to v1: https://lore.kernel.org/r/20240522-arm64-vexpress-sp804-v1-1-0344cd42eb77@kernel.org
> ---
>  drivers/clocksource/Kconfig | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index 34faa0320ece..ca6045f90000 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -390,7 +390,8 @@ config ARM_GT_INITIAL_PRESCALER_VAL
>  	  This affects CPU_FREQ max delta from the initial frequency.
>  
>  config ARM_TIMER_SP804
> -	bool "Support for Dual Timer SP804 module" if COMPILE_TEST
> +	bool "Support for Dual Timer SP804 module"
> +	depends on ARM || ARM64 || COMPILE_TEST
>  	depends on GENERIC_SCHED_CLOCK && HAVE_CLK
>  	select CLKSRC_MMIO
>  	select TIMER_OF if OF
> 
> ---
> base-commit: 1613e604df0cd359cf2a7fbd9be7a0bcfacfabd0
> change-id: 20240522-arm64-vexpress-sp804-365d6938b07c
> 
> Best regards,
> -- 
> Mark Brown <broonie@kernel.org>
> 
>

On 29/05/2024 21:48, Mark Brown wrote:
> The sp804 is currently only user selectable if COMPILE_TEST, this was
> done by commit dfc82faad725 ("clocksource/drivers/sp804: Add
> COMPILE_TEST to CONFIG_ARM_TIMER_SP804") in order to avoid it being
> spuriously offered on platforms that won't have the hardware since it's
> generally only seen on Arm based platforms.  This config is overly
> restrictive, while platforms that rely on the SP804 do select it in
> their Kconfig there are others such as the Arm fast models which have a
> SP804 available but currently unused by Linux.  Relax the dependency to
> allow it to be user selectable on arm and arm64 to avoid surprises and
> in case someone comes up with a use for extra timer hardware.

Would it make sense to add the option in the platform so it selects the 
timer ?

-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

On Mon, Jul 08, 2024 at 06:49:38PM +0200, Daniel Lezcano wrote:
> On 29/05/2024 21:48, Mark Brown wrote:

> > The sp804 is currently only user selectable if COMPILE_TEST, this was
> > done by commit dfc82faad725 ("clocksource/drivers/sp804: Add
> > COMPILE_TEST to CONFIG_ARM_TIMER_SP804") in order to avoid it being
> > spuriously offered on platforms that won't have the hardware since it's
> > generally only seen on Arm based platforms.  This config is overly
> > restrictive, while platforms that rely on the SP804 do select it in
> > their Kconfig there are others such as the Arm fast models which have a
> > SP804 available but currently unused by Linux.  Relax the dependency to
> > allow it to be user selectable on arm and arm64 to avoid surprises and
> > in case someone comes up with a use for extra timer hardware.

> Would it make sense to add the option in the platform so it selects the
> timer ?

As the commit log says far as I'm aware all the platforms that rely on
the sp804 timer already select it (they wouldn't otherwise be able to
work unless COMPILE_TEST was enabled).  The Arm models and possibly
other platforms have the sp804 but it will currently be ignored by Linux
and the architected timers used instead so it would be wasteful to force
it on for them.

On 08/07/2024 19:44, Mark Brown wrote:
> On Mon, Jul 08, 2024 at 06:49:38PM +0200, Daniel Lezcano wrote:
>> On 29/05/2024 21:48, Mark Brown wrote:
> 
>>> The sp804 is currently only user selectable if COMPILE_TEST, this was
>>> done by commit dfc82faad725 ("clocksource/drivers/sp804: Add
>>> COMPILE_TEST to CONFIG_ARM_TIMER_SP804") in order to avoid it being
>>> spuriously offered on platforms that won't have the hardware since it's
>>> generally only seen on Arm based platforms.  This config is overly
>>> restrictive, while platforms that rely on the SP804 do select it in
>>> their Kconfig there are others such as the Arm fast models which have a
>>> SP804 available but currently unused by Linux.  Relax the dependency to
>>> allow it to be user selectable on arm and arm64 to avoid surprises and
>>> in case someone comes up with a use for extra timer hardware.
> 
>> Would it make sense to add the option in the platform so it selects the
>> timer ?
> 
> As the commit log says far as I'm aware all the platforms that rely on
> the sp804 timer already select it (they wouldn't otherwise be able to
> work unless COMPILE_TEST was enabled).  The Arm models and possibly
> other platforms have the sp804 but it will currently be ignored by Linux
> and the architected timers used instead so it would be wasteful to force
> it on for them.

The policy of the Kconfig is we should keep the option silent.

My suggestion was to provide the option in the platforms Kconfig and 
[un]select the ARM_TIMER_SP804 from there


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

On Tue, Jul 09, 2024 at 06:15:40PM +0200, Daniel Lezcano wrote:
> On 08/07/2024 19:44, Mark Brown wrote:
> > On Mon, Jul 08, 2024 at 06:49:38PM +0200, Daniel Lezcano wrote:
> > > On 29/05/2024 21:48, Mark Brown wrote:
> > 
> > > > The sp804 is currently only user selectable if COMPILE_TEST, this was
> > > > done by commit dfc82faad725 ("clocksource/drivers/sp804: Add
> > > > COMPILE_TEST to CONFIG_ARM_TIMER_SP804") in order to avoid it being
> > > > spuriously offered on platforms that won't have the hardware since it's
> > > > generally only seen on Arm based platforms.  This config is overly
> > > > restrictive, while platforms that rely on the SP804 do select it in
> > > > their Kconfig there are others such as the Arm fast models which have a
> > > > SP804 available but currently unused by Linux.  Relax the dependency to
> > > > allow it to be user selectable on arm and arm64 to avoid surprises and
> > > > in case someone comes up with a use for extra timer hardware.
> > 
> > > Would it make sense to add the option in the platform so it selects the
> > > timer ?
> > 
> > As the commit log says far as I'm aware all the platforms that rely on
> > the sp804 timer already select it (they wouldn't otherwise be able to
> > work unless COMPILE_TEST was enabled).  The Arm models and possibly
> > other platforms have the sp804 but it will currently be ignored by Linux
> > and the architected timers used instead so it would be wasteful to force
> > it on for them.
> 
> The policy of the Kconfig is we should keep the option silent.

Why?

I can understand hiding an option when it *really* depends on something
that isn't available, but the kernel not having knowledge of a specific
soc/platform isn't the same thing -- it's an entirely synthetic
dependency.

What problem is solved by not allowing the user to select this?

> My suggestion was to provide the option in the platforms Kconfig and
> [un]select the ARM_TIMER_SP804 from there

As above, that means that the kernel needs to have knowledge of the
specific platform, which in general we've been moving *away* form (e.g.
with DT and ACPI tables). For virtial platformas specifically that's a
massive pain because they're *so* configurable -- there's not much the
kernel can rely on being present/absent, and users will want to enable
specific features depending on what they're trying to test, at which
point they'd have to hack Kconfig files...

Mark.

> 
> 
> -- 
> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
> 
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog
> 
> 

On Tue, Jul 09, 2024 at 06:15:40PM +0200, Daniel Lezcano wrote:
> On 08/07/2024 19:44, Mark Brown wrote:
> > On Mon, Jul 08, 2024 at 06:49:38PM +0200, Daniel Lezcano wrote:

> > > Would it make sense to add the option in the platform so it selects the
> > > timer ?

> > As the commit log says far as I'm aware all the platforms that rely on
> > the sp804 timer already select it (they wouldn't otherwise be able to
> > work unless COMPILE_TEST was enabled).  The Arm models and possibly
> > other platforms have the sp804 but it will currently be ignored by Linux
> > and the architected timers used instead so it would be wasteful to force
> > it on for them.

> The policy of the Kconfig is we should keep the option silent.

That's not what the changelog claimed was the reason for adding the
dependency, and we have other number of Kconfig options (eg,
TEGRA186_TIMER or ARM_ARCH_TIMER_EVTSTREAM) which don't.  There does
seem to be a bit of a randomness if the options are only visible with
COMPILE_TEST too, quite a few depend on an arch or COMPILE_TEST.

> My suggestion was to provide the option in the platforms Kconfig and
> [un]select the ARM_TIMER_SP804 from there

I can't parse what that means, sorry.  The goal here is to not have the
FVP platforms depend on the driver since they don't actually depend on
it, it's merely physially present.