Messages in this thread | | | From | Bartosz Golaszewski <> | Date | Mon, 8 Apr 2019 15:49:03 +0200 | Subject | Re: [PATCH v4 01/11] clocksource: davinci-timer: new driver |
| |
wt., 2 kwi 2019 o 11:21 Daniel Lezcano <daniel.lezcano@linaro.org> napisał(a): > > On 18/03/2019 13:10, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > 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. > > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > Reviewed-by: David Lechner <david@lechnology.com> > > --- > > 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" > > + help > > + Enables the support for the TI DaVinci timer driver. > > + > > Please make it a silence option only visible with COMPILE_TEST or > EXPERT, examples here: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clocksource/Kconfig#n45 > > or second format: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clocksource/Kconfig#n459 > > > 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@baylibre.com> > > +// (with some parts adopted from code by Kevin Hilman <khilman@baylibre.com>) > > +#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, > > +}; > > + > > +struct davinci_timer_data; > > + > > +typedef void (*davinci_timer_set_period_func)(struct davinci_timer_data *, > > + unsigned int period); > > + > > +/** > > + * struct davinci_timer_regs - timer-specific register offsets > > + * > > + * @tim_off: timer counter register > > + * @prd_off: timer period register > > + * @enamode_shift: left bit-shift of the enable register associated > > + * with this timer in the TCR register > > + */ > > +struct davinci_timer_regs { > > + unsigned int tim_off; > > + unsigned int prd_off; > > + unsigned int enamode_shift; > > +}; > > + > > +struct davinci_timer_data { > > + void __iomem *base; > > + const struct davinci_timer_regs *regs; > > + unsigned int mode; > > + davinci_timer_set_period_func set_period; > > + unsigned int cmp_off; > > +}; > > + > > +struct davinci_timer_clockevent { > > + struct clock_event_device dev; > > + unsigned int tick_rate; > > + struct davinci_timer_data timer; > > +}; > > The timer-of API provides the functions and the common structures for > the usual operations. Please use them instead of redefining your own > structures. >
After giving it another thought, I don't think we can use timer-of in this driver easily.
It's meant to work both for board files and device-tree. The solution with the least code duplication is to take the device_node on DT systems and put all relevant properties into the custom structure used by board files users. Timer-of maps the memory, enables the clock and requests interrupts directly from the device_node. I'd have to do the same for board files manually anyway. I think it's better to have a single point of entry for the initialization code.
Even if I were just to reuse the timer-of structures without reusing the actual code - they are still missing certain fields. We're using 32-bit "halves" of timers that are actually 64 bits - the 32-bit control register deals with both by using the same layout of bit fields but shifted. This is what the struct davinci_timer_regs is for. Reusing those structures would also be confusing because of the of_ prefix in the naming.
I'd prefer to leave it as it is. Let me know what you think.
Bart
| |