Message-ID: <CAMhs-H9-QdfiuajhmiAJN_BWi4Hc_9A_cq7Fc8XxZXiDJdaYTA@mail.gmail.com>
Date: Mon, 28 Oct 2024 20:02:08 +0100
From: Sergio Paracuellos <sergio.paracuellos@...il.com>
To: Daniel Lezcano <daniel.lezcano@...aro.org>
Cc: linux-mips@...r.kernel.org, tglx@...utronix.de, tsbogend@...ha.franken.de, 
	john@...ozen.org, linux-kernel@...r.kernel.org, yangshiji66@...look.com
Subject: Re: [PATCH 1/2] clocksource: Add Ralink System Tick Counter driver

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@...aro.org> 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@...aro.org> 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@...il.com>
> >>> ---
> >>>    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

Powered by blists - more mailing lists