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?
>
> IOW, this option should be:
>
> config RALINK_TIMER
> bool "Ralink System Tick Counter" if COMPILE_TEST
> select CLKSRC_MMIO
> select TIMER_OF
> help
> Enables support for system tick counter present on
> Ralink SoCs RT3352 and MT7620.
>
> The option COMPILE_TEST is to compile on different platforms, thus
> increasing the compilation test coverage. At the first glance, the
> driver does not seem to pull arch dependent code except definitions
> which look compatible with other archs, so it should be fine.
Yes, there is no arch dependencies since the only include which was
dependent was not really needed and I already got rid of it when I
performed the git mv. I already checked
that the driver is properly compiled for allyesconfig target.
Thanks,
Sergio Paracuellos
>
>
> >>> + select CLKSRC_MMIO
> >>> + select TIMER_OF
> >>> + help
> >>> + Enables support for system tick counter present on
> >>> + Ralink SoCs RT3352 and MT7620.
> >>> +
> >>> endmenu
> >>> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> >>> index 22743785299e..c9214afcb712 100644
> >>> --- a/drivers/clocksource/Makefile
> >>
> >> You should use git mv
> >>
> >> Otherwise the code is like submitting a new driver
> >
> > Ok, i will squash two patches in one performing the git mv then.
> >
> > Thanks,
> > Sergio Paracuellos
> >>
> >>
> >>
> >> --
> >> <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
>
>
> --
> <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
