Re: [PATCH 1/5] clocksource: Add MStar MSC313e timer support

[Daniel Lezcano <daniel.lezcano@linaro.org>]

On 30/11/2021 15:12, Romain Perier wrote:
> Hi,
> 
> Le lun. 29 nov. 2021 à 18:02, Daniel Lezcano <daniel.lezcano@linaro.org
> <mailto:daniel.lezcano@linaro.org>> a écrit :
> 
>     On 26/11/2021 21:21, Romain Perier wrote:
>     > The MSC313e-compatible SoCs have 3 timer hardware blocks. All of these
>     > are free running 32-bit increasing counters and can generate
>     interrupts.
>     > This commit adds basic support for these timers, the first timer block
>     > being used as clocksource/sched_clock and delay, while the others will
>     > be used as clockevents.
> 
>     Please you elaborate a bit more the internals of this timer as it is a
>     initial submission
> 
> 
> Ok, will try to elaborate.
>  
> 
> 
> 
>     > Signed-off-by: Romain Perier <romain.perier@gmail.com
>     <mailto:romain.perier@gmail.com>>
>     > Co-developed-by: Daniel Palmer <daniel@0x0f.com
>     <mailto:daniel@0x0f.com>>
>     > Signed-off-by: Daniel Palmer <daniel@0x0f.com
>     <mailto:daniel@0x0f.com>>
>     > ---
>     >  MAINTAINERS                         |   1 +
>     >  drivers/clocksource/Kconfig         |  10 ++
>     >  drivers/clocksource/Makefile        |   1 +
>     >  drivers/clocksource/timer-msc313e.c | 228
>     ++++++++++++++++++++++++++++
>     >  4 files changed, 240 insertions(+)
>     >  create mode 100644 drivers/clocksource/timer-msc313e.c
>     >
>     > diff --git a/MAINTAINERS b/MAINTAINERS
>     > index 7a2345ce8521..f39a1617bf50 100644
>     > --- a/MAINTAINERS
>     > +++ b/MAINTAINERS
>     > @@ -2282,6 +2282,7 @@ F:     
>     Documentation/devicetree/bindings/gpio/mstar,msc313-gpio.yaml
>     >  F:   arch/arm/boot/dts/mstar-*
>     >  F:   arch/arm/mach-mstar/
>     >  F:   drivers/clk/mstar/
>     > +F:   drivers/clocksource/timer-msc313e.c
>     >  F:   drivers/gpio/gpio-msc313.c
>     >  F:   drivers/rtc/rtc-msc313.c
>     >  F:   drivers/watchdog/msc313e_wdt.c
>     > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
>     > index f65e31bab9ae..822e711da284 100644
>     > --- a/drivers/clocksource/Kconfig
>     > +++ b/drivers/clocksource/Kconfig
>     > @@ -671,6 +671,16 @@ config MILBEAUT_TIMER
>     >       help
>     >         Enables the support for Milbeaut timer driver.
>     > 
>     > +config MSC313E_TIMER
>     > +     bool "MSC313E timer driver"
> 
>     Silent option please and platform config option enables it.
> 
> 
> What do you mean ? no short description at all ?

We try to let the platform Kconfig option to select silently the timer
in order to prevent selecting it manually.

If the timer is mandatory on your platform it should be a silent option,
except for COMPILE_TEST

That leads to:

	bool "MSC313E timer driver" if COMPILE_TEST

and you should be able to compile it on x86, ...

If the timer is optional because there is another one on the platform,
it could be unselected manually. That is the configuration you've done here.

So if there is no other broadcast timer, this timer should selected for
the platform and the option should be silent (except in case of
COMPILE_TEST).

>     > +     depends on ARCH_MSTARV7 || COMPILE_TEST
>     > +     select TIMER_OF
>     > +     select CLKSRC_MMIO
>     > +     help
>     > +       Enables support for the MStar MSC313E timer driver.
>     > +       This provides access to multiple interrupt generating
>     > +       programmable 32-bit free running incrementing counters.
>     > +
>     >  config INGENIC_TIMER

[ ... ]

>     > +
>     > +struct msc313e_delay {
>     > +     void __iomem *base;
>     > +     struct delay_timer delay;
>     > +};
>     > +
>     > +static void __iomem *msc313e_clksrc;
>     > +static struct msc313e_delay msc313e_delay;
> 
>     I'm not sure that compiles on other platform than mstarv7
> 
> 
> It is armv7-based, and its size is known at build-time, no ?
> Everything builds with WERROR here.

I should have say "arch" instead of "platform".

The COMPILE_TEST option is set above, that means the driver can be
compiled on a x86 (for compilation test coverage, stubs already exists
except for delay AFAIR).

[ ... ]

>     > +     msc313e_delay.base = timer_of_base(&to);
>     > +     msc313e_delay.delay.read_current_timer =
>     msc313e_read_delay_timer_read;
>     > +     msc313e_delay.delay.freq = timer_of_rate(&to);
>     > +
>     > +     msc313e_clksrc = timer_of_base(&to);
>     > +     reg = readw(msc313e_clksrc + MSC313E_REG_CTRL);
>     > +     reg |= MSC313E_REG_CTRL_TIMER_EN;
>     > +     writew(reg, msc313e_clksrc + MSC313E_REG_CTRL);
>     > +
>     > +     register_current_timer_delay(&msc313e_delay.delay);
>     > +
>     > +     sched_clock_register(msc313e_timer_sched_clock_read, 32,
>     timer_of_rate(&to));
>     > +     return clocksource_mmio_init(timer_of_base(&to), TIMER_NAME,
>     timer_of_rate(&to), 300, 32,
>     > +                                  msc313e_timer_clksrc_read);
> 
>     format 80char max please, run checkpatch.pl <http://checkpatch.pl>
>     before submitting
> 
> 
> max_line_lenght is set to "100" in checkpatch.pl <http://checkpatch.pl>
> since a while now :) .
> I have passed it with "--strict" before sending the series, however, if
> you prefer 80 chars
> max just ask, I can limit to 80 chars.

Oh, indeed. Fair enough, limit to 80 chars is now deprecated and
suggested length is 100.

In this case, at your convenience.

Thanks
  -- Daniel


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