Re: [PATCH v4 01/11] clocksource: davinci-timer: new driver

[Bartosz Golaszewski]

wt., 16 kwi 2019 o 15:44 Bartosz Golaszewski <brgl@xxxxxxxx> napisaÅ(a):
>
> pon., 15 kwi 2019 o 14:36 Daniel Lezcano <daniel.lezcano@xxxxxxxxxx> napisaÅ(a):
> >
> > On 18/03/2019 13:10, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>
> > >
> > > Currently the clocksource and clockevent support for davinci platforms
> > > lives in mach-davinci. It hard-codes many things, uses global variables,
> > > implements functionalities unused by any platform and has code fragments
> > > scattered across many (often unrelated) files.
> > >
> > > Implement a new, modern and simplified timer driver and put it into
> > > drivers/clocksource. We still need to support legacy board files so
> > > export a config structure and a function that allows machine code to
> > > register the timer.
> > >
> > > We don't bother freeing resources on errors in davinci_timer_register()
> > > as the system won't boot without a timer anyway.
> >
> > Can you give a quick description of the timer hardware and how it works?
> >
>
> Will do.
>
> > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>
> > > Reviewed-by: David Lechner <david@xxxxxxxxxxxxxx>
> > > ---
> > >  drivers/clocksource/Kconfig         |   5 +
> > >  drivers/clocksource/Makefile        |   1 +
> > >  drivers/clocksource/timer-davinci.c | 438 ++++++++++++++++++++++++++++
> > >  include/clocksource/timer-davinci.h |  44 +++
> > >  4 files changed, 488 insertions(+)
> > >  create mode 100644 drivers/clocksource/timer-davinci.c
> > >  create mode 100644 include/clocksource/timer-davinci.h
> > >
> > > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> > > index 171502a356aa..08b1f539cfc4 100644
> > > --- a/drivers/clocksource/Kconfig
> > > +++ b/drivers/clocksource/Kconfig
> > > @@ -42,6 +42,11 @@ config BCM_KONA_TIMER
> > >       help
> > >         Enables the support for the BCM Kona mobile timer driver.
> > >
> > > +config DAVINCI_TIMER
> > > +     bool "Texas Instruments DaVinci timer driver"
> >
> > Make the option silent (eg. if COMPILE_TEST)
> >
>
> Sure, I already did it locally after your last review.
>
> > > +     help
> > > +       Enables the support for the TI DaVinci timer driver.
> > > +
> > >  config DIGICOLOR_TIMER
> > >       bool "Digicolor timer driver" if COMPILE_TEST
> > >       select CLKSRC_MMIO
> > > diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> > > index be6e0fbc7489..3c73d0e58b45 100644
> > > --- a/drivers/clocksource/Makefile
> > > +++ b/drivers/clocksource/Makefile
> > > @@ -15,6 +15,7 @@ obj-$(CONFIG_SH_TIMER_TMU)  += sh_tmu.o
> > >  obj-$(CONFIG_EM_TIMER_STI)   += em_sti.o
> > >  obj-$(CONFIG_CLKBLD_I8253)   += i8253.o
> > >  obj-$(CONFIG_CLKSRC_MMIO)    += mmio.o
> > > +obj-$(CONFIG_DAVINCI_TIMER)  += timer-davinci.o
> > >  obj-$(CONFIG_DIGICOLOR_TIMER)        += timer-digicolor.o
> > >  obj-$(CONFIG_OMAP_DM_TIMER)  += timer-ti-dm.o
> > >  obj-$(CONFIG_DW_APB_TIMER)   += dw_apb_timer.o
> > > diff --git a/drivers/clocksource/timer-davinci.c b/drivers/clocksource/timer-davinci.c
> > > new file mode 100644
> > > index 000000000000..46dfc4d457fc
> > > --- /dev/null
> > > +++ b/drivers/clocksource/timer-davinci.c
> > > @@ -0,0 +1,438 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +//
> > > +// TI DaVinci clocksource driver
> > > +//
> > > +// Copyright (C) 2019 Texas Instruments
> > > +// Author: Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>
> > > +// (with some parts adopted from code by Kevin Hilman <khilman@xxxxxxxxxxxx>)
> > > +
> > > +#include <linux/clk.h>
> > > +#include <linux/clockchips.h>
> > > +#include <linux/clocksource.h>
> > > +#include <linux/err.h>
> > > +#include <linux/interrupt.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/of_address.h>
> > > +#include <linux/of_irq.h>
> > > +#include <linux/sched_clock.h>
> > > +
> > > +#include <clocksource/timer-davinci.h>
> > > +
> > > +#undef pr_fmt
> > > +#define pr_fmt(fmt) "%s: " fmt "\n", __func__
> > > +
> > > +#define DAVINCI_TIMER_REG_TIM12                      0x10
> > > +#define DAVINCI_TIMER_REG_TIM34                      0x14
> > > +#define DAVINCI_TIMER_REG_PRD12                      0x18
> > > +#define DAVINCI_TIMER_REG_PRD34                      0x1c
> > > +#define DAVINCI_TIMER_REG_TCR                        0x20
> > > +#define DAVINCI_TIMER_REG_TGCR                       0x24
> > > +
> > > +#define DAVINCI_TIMER_TIMMODE_MASK           GENMASK(3, 2)
> > > +#define DAVINCI_TIMER_RESET_MASK             GENMASK(1, 0)
> > > +#define DAVINCI_TIMER_TIMMODE_32BIT_UNCHAINED        BIT(2)
> > > +#define DAVINCI_TIMER_UNRESET                        GENMASK(1, 0)
> > > +
> > > +/* Shift depends on timer. */
> > > +#define DAVINCI_TIMER_ENAMODE_MASK           GENMASK(1, 0)
> > > +#define DAVINCI_TIMER_ENAMODE_DISABLED               0x00
> > > +#define DAVINCI_TIMER_ENAMODE_ONESHOT                BIT(0)
> > > +#define DAVINCI_TIMER_ENAMODE_PERIODIC               BIT(1)
> > > +
> > > +#define DAVINCI_TIMER_ENAMODE_SHIFT_TIM12    6
> > > +#define DAVINCI_TIMER_ENAMODE_SHIFT_TIM34    22
> > > +
> > > +#define DAVINCI_TIMER_MIN_DELTA                      0x01
> > > +#define DAVINCI_TIMER_MAX_DELTA                      0xfffffffe
> > > +
> > > +#define DAVINCI_TIMER_CLKSRC_BITS            32
> > > +
> > > +#define DAVINCI_TIMER_TGCR_DEFAULT \
> > > +             (DAVINCI_TIMER_TIMMODE_32BIT_UNCHAINED | DAVINCI_TIMER_UNRESET)
> > > +
> > > +enum {
> > > +     DAVINCI_TIMER_MODE_DISABLED = 0,
> > > +     DAVINCI_TIMER_MODE_ONESHOT,
> > > +     DAVINCI_TIMER_MODE_PERIODIC,
> > > +};
> >
> > This is replicating what is already available. Right after set_periodic
> > or set_oneshot are called, the timer state is changed to respectively
> > CLOCK_EVT_STATE_PERIODIC and CLOCK_EVT_STATE_ONESHOT. So it is useless
> > to define those enum again as what you want is to check the timer mode.
> >
>
> I did it like this because I'm reusing the code in
> davinci_timer_set_period_std() for both clocksource and clockevent
> timers. If you prefer it be split to reuse the clockevent accessors, I
> can do this (see below).
>
> >
> > [ ... ]
> >
> > > +
> > > +     clocksource = kzalloc(sizeof(*clocksource), GFP_KERNEL);
> > > +     if (!clocksource) {
> > > +             pr_err("Error allocating memory for clocksource data");
> > > +             return -ENOMEM;
> > > +     }
> > > +
> > > +     clocksource->dev.rating = 300;
> > > +     clocksource->dev.read = davinci_timer_clksrc_read;
> > > +     clocksource->dev.mask = CLOCKSOURCE_MASK(DAVINCI_TIMER_CLKSRC_BITS);
> > > +     clocksource->dev.flags = CLOCK_SOURCE_IS_CONTINUOUS;
> >
> > >>>
> >
> > > +     clocksource->timer.set_period = davinci_timer_set_period_std;
> > > +     clocksource->timer.mode = DAVINCI_TIMER_MODE_PERIODIC;
> > > +     clocksource->timer.base = base;
> >
> > What for?
> >
>
> What am I assigning the timer for? In order to call
> davinci_timer_set_period() on it at the bottom of the init function.
> I'm not sure if it is a problem you're pointing out, but as I said
> above - I can configure the clocksource timer by hand in the init
> function, drop the davinci_timer_clocksource structure and use the
> clockevent accessors for checking the clock mode if you prefer it that
> way. Would that be fine?
>
> > <<<
> >
> > > +     if (timer_cfg->cmp_off) {
> > > +             clocksource->timer.regs = &davinci_timer_tim12_regs;
> > > +             clocksource->dev.name = "tim12";
> > > +     } else {
> > > +             clocksource->timer.regs = &davinci_timer_tim34_regs;
> > > +             clocksource->dev.name = "tim34";
> > > +     }
> > > +

Just to clarify for the above: I'll still need to account for these
lines here if I go that way. This again leads to code duplication IMO.

Bart

> > > +     rv = request_irq(timer_cfg->irq[DAVINCI_TIMER_CLOCKSOURCE_IRQ].start,
> > > +                      davinci_timer_irq_freerun, IRQF_TIMER,
> > > +                      "free-run counter", clocksource);
> > > +     if (rv) {
> > > +             pr_err("Unable to request the clocksource interrupt");
> > > +             return rv;
> > > +     }
> >
> > Why do you have to request an interrupt to do nothing? Isn't possible to
> > let the timer run and wrap without generating interrupts?
> >
>
> Yes it is, but the already existing DT bindings define two interrupts.
> It's true that nobody uses it, but I thought I'd stick with what was
> done before. If you prefer that, I can use a single interrupt - and
> just ignore the second one defined in DT (and also remove it from the
> config structure for board files).
>
> Thanks,
> Bartosz
>
> > [ ... ]
> >
> >
> >
> >
> > --
> >  <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
> >