Re: [PATCH 1/2] clocksource: Add Ralink System Tick Counter driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 28/10/2024 20:02, Sergio Paracuellos wrote:
Hi Daniel,

Thanks a lot for the detailed explanation. It was really helpful.

On Mon, Oct 28, 2024 at 7:44 PM Daniel Lezcano
<daniel.lezcano@xxxxxxxxxx> wrote:

On 28/10/2024 19:04, Sergio Paracuellos wrote:
Hi Daniel,

Thanks for reviewing this.

On Mon, Oct 28, 2024 at 5:29 PM Daniel Lezcano
<daniel.lezcano@xxxxxxxxxx> wrote:

On 20/09/2024 09:53, Sergio Paracuellos wrote:
System Tick Counter is present on Ralink SoCs RT3352 and MT7620. This
driver has been in 'arch/mips/ralink' directory since the beggining of
Ralink architecture support. However, it can be moved into a more proper
place in 'drivers/clocksource'. Hence add it here adding also support for
compile test targets.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@xxxxxxxxx>
---
    drivers/clocksource/Kconfig        |  10 ++
    drivers/clocksource/Makefile       |   1 +
    drivers/clocksource/timer-ralink.c | 150 +++++++++++++++++++++++++++++
    3 files changed, 161 insertions(+)
    create mode 100644 drivers/clocksource/timer-ralink.c

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 95dd4660b5b6..50339f4d3201 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -753,4 +753,14 @@ config EP93XX_TIMER
          Enables support for the Cirrus Logic timer block
          EP93XX.

+config CLKSRC_RALINK

It is a timer

          RALINK_TIMER

Sure, I will change to RALINK_TIMER instead.


+     bool "Ralink System Tick Counter"

Silent option please if possible.

Let the platform Kconfig selects the driver

+     depends on SOC_RT305X || SOC_MT7620 || COMPILE_TEST
+     default y if SOC_RT305X || SOC_MT7620

You should have something similar the RISCV option, no default option

Sorry, I am not the best with Kconfig so I am not sure what you are
exactly expecting here.MT7620
Does the following work for you?

config RALINK_TIMER
     bool "Ralink System Tick Counter" if COMPILE_TEST
     depends on SOC_RT305X || SOC_MT7620
     select CLKSRC_MMIO
     select TIMER_OF
     help
       Enables support for system tick counter present on
       Ralink SoCs RT3352 and MT7620.

Basically the idea is to have the platform's Kconfig selecting the
RALINK_TIMER. If I'm not wrong the Kconfig in arch/riscv/ralink should
select RALINK_TIMER under the "config SOC_RT305X" and "config
SOC_MT7620". The block "config CLKEVT_RT3352" has to be removed.

Then this (clocksource) Kconfig option is a silent option. The user
won't have to figure out which option to enable because that will be
done directly when selecting RT305X or MT7620.

The only reason to not have it silent is if you really want to opt-out
this timer because it is not present on a different version of RT305X or
MT7620.

Ok, then I don't want to silence it since those ralink's platform
SOC_RT305X and SOC_MT7620 includes other SoCs models that do not have
this timer (rt3050, mt7628 for example). Only models
rt3352 and mt7620 include this. So I guess having this is the correct
thing to do:

config RALINK_TIMER
     bool "Ralink System Tick Counter" if COMPILE_TEST
     depends on SOC_RT305X || SOC_MT7620
     select CLKSRC_MMIO
     select TIMER_OF
     help
        Enables support for system tick counter present on
        Ralink SoCs RT3352 and MT7620.

Are you ok with this?


No because it is a compile test option only. You may want to do the same as:

https://lore.kernel.org/all/20241001-arm64-vexpress-sp804-v3-1-0a2d3f7883e4@xxxxxxxxxx/

It should be:

config RALINK_TIMER
	bool "Ralink System Tick Counter"
	depends on SOC_RT305X || SOC_MT7620 || COMPILE_TEST
	select CLKSRC_MMIO
	select TIMER_OF
	help
	  Enables support for system tick counter present on
	  Ralink SoCs RT3352 and MT7620.

so very similar to the initial patch except no default is needed as it should be set from SOC_RT305X or SOC_MT7620


--
<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




[Index of Archives]     [LKML Archive]     [Linux ARM Kernel]     [Linux ARM]     [Git]     [Yosemite News]     [Linux SCSI]     [Linux Hams]

  Powered by Linux