| Message ID | 20190318121100.28132-2-brgl@bgdev.pl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ARM: davinci: modernize the timer support | expand |
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.
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. > Hi Daniel, do you have any suggestion about where to put the information about register offsets I'm now storing in struct davinci_timer_regs? I thought about having specialized wrappers around all relevant functions that access the registers, but that seems like an overkill. For instance we'd have: davinci_timer_set_period_std() and two specialized variants: davinci_timer_set_period_std_tim12() and davinci_timer_set_period_std_tim34() The former would take struct davinci_timer_regs as argument while two latter routines would fit the callback format. We'd then need the same for three more routines. Alternatively struct timer_of could be packed together with struct davinci_timer_regs in another structure and accessed in callbacks via container_of. Do you have a preference or a different idea? 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 >
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
On 08/04/2019 15:49, Bartosz Golaszewski wrote: > 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. Sorry for not being responsive, I've been OoO last week. Give me some days to digest the emails stack I have in my mailbox and I'll tell you after reviewing how works this timer.
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. Can you give a quick description of the timer hardware and how it works? > 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" Make the option silent (eg. if COMPILE_TEST) > + 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@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, > +}; 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. [ ... ] > + > + 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? <<< > + 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"; > + } > + > + 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? [ ... ]
pon., 15 kwi 2019 o 14:36 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. > > Can you give a quick description of the timer hardware and how it works? > Will do. > > 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" > > 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@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, > > +}; > > 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"; > > + } > > + > > + 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 >
wt., 16 kwi 2019 o 15:44 Bartosz Golaszewski <brgl@bgdev.pl> napisał(a): > > pon., 15 kwi 2019 o 14:36 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. > > > > Can you give a quick description of the timer hardware and how it works? > > > > Will do. > > > > 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" > > > > 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@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, > > > +}; > > > > 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 > >
Hi Bartosz, On 16/04/2019 15:44, Bartosz Golaszewski wrote: > pon., 15 kwi 2019 o 14:36 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. >> >> Can you give a quick description of the timer hardware and how it works? >> > > Will do. > >>> 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" >> >> 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@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, >>> +}; >> >> 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). I see. It is too much complicated for the purpose. The values are computed around every time with this flag. At the first glance, the driver can be simpler. Please do the following rework: 1. One patch providing the clockevents code only 2. One patch providing the clocksource code only and taking account the comments below. In order to not review the entire series every time, just send those two patches until we agree on it and then the rest of the series. The computation in the function davinci_timer_set_period_std() can be replaced with constant put in the field instead of bit shifting everytime. >> [ ... ] >> >>> + >>> + 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? Yes but without checking the clock mode. >>> + 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"; >>> + } >>> + >>> + 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). Yes please, remove the interrupt handling for the clocksource but keep in the config structure the interrupt in case we want in the future swap the clocksource and the clockevent.
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. + 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; +}; + +struct davinci_timer_clocksource { + struct clocksource dev; + struct davinci_timer_data timer; +}; + +static const struct davinci_timer_regs davinci_timer_tim12_regs = { + .tim_off = DAVINCI_TIMER_REG_TIM12, + .prd_off = DAVINCI_TIMER_REG_PRD12, + .enamode_shift = DAVINCI_TIMER_ENAMODE_SHIFT_TIM12, +}; + +static const struct davinci_timer_regs davinci_timer_tim34_regs = { + .tim_off = DAVINCI_TIMER_REG_TIM34, + .prd_off = DAVINCI_TIMER_REG_PRD34, + .enamode_shift = DAVINCI_TIMER_ENAMODE_SHIFT_TIM34, +}; + +/* Must be global for davinci_timer_read_sched_clock(). */ +static struct davinci_timer_data *davinci_timer_clksrc_timer; + +static struct davinci_timer_clockevent * +to_davinci_timer_clockevent(struct clock_event_device *clockevent) +{ + return container_of(clockevent, struct davinci_timer_clockevent, dev); +} + +static struct davinci_timer_clocksource * +to_davinci_timer_clocksource(struct clocksource *clocksource) +{ + return container_of(clocksource, struct davinci_timer_clocksource, dev); +} + +static unsigned int davinci_timer_read(struct davinci_timer_data *timer, + unsigned int reg) +{ + return readl_relaxed(timer->base + reg); +} + +static void davinci_timer_write(struct davinci_timer_data *timer, + unsigned int reg, unsigned int val) +{ + writel_relaxed(val, timer->base + reg); +} + +static void davinci_timer_update(struct davinci_timer_data *timer, + unsigned int reg, unsigned int mask, + unsigned int val) +{ + unsigned int new, orig; + + orig = davinci_timer_read(timer, reg); + new = orig & ~mask; + new |= val & mask; + + davinci_timer_write(timer, reg, new); +} + +static void davinci_timer_set_period(struct davinci_timer_data *timer, + unsigned int period) +{ + timer->set_period(timer, period); +} + +static void davinci_timer_set_period_std(struct davinci_timer_data *timer, + unsigned int period) +{ + const struct davinci_timer_regs *regs = timer->regs; + unsigned int enamode; + + enamode = davinci_timer_read(timer, DAVINCI_TIMER_REG_TCR); + + davinci_timer_update(timer, DAVINCI_TIMER_REG_TCR, + DAVINCI_TIMER_ENAMODE_MASK << regs->enamode_shift, + DAVINCI_TIMER_ENAMODE_DISABLED << regs->enamode_shift); + + davinci_timer_write(timer, regs->tim_off, 0x0); + davinci_timer_write(timer, regs->prd_off, period); + + if (timer->mode == DAVINCI_TIMER_MODE_ONESHOT) + enamode = DAVINCI_TIMER_ENAMODE_ONESHOT; + else if (timer->mode == DAVINCI_TIMER_MODE_PERIODIC) + enamode = DAVINCI_TIMER_ENAMODE_PERIODIC; + + davinci_timer_update(timer, DAVINCI_TIMER_REG_TCR, + DAVINCI_TIMER_ENAMODE_MASK << regs->enamode_shift, + enamode << regs->enamode_shift); +} + +static void davinci_timer_set_period_cmp(struct davinci_timer_data *timer, + unsigned int period) +{ + const struct davinci_timer_regs *regs = timer->regs; + unsigned int curr_time; + + curr_time = davinci_timer_read(timer, regs->tim_off); + davinci_timer_write(timer, timer->cmp_off, curr_time + period); +} + +static irqreturn_t davinci_timer_irq_timer(int irq, void *data) +{ + struct davinci_timer_clockevent *clockevent = data; + + clockevent->dev.event_handler(&clockevent->dev); + + return IRQ_HANDLED; +} + +static irqreturn_t davinci_timer_irq_freerun(int irq, void *data) +{ + return IRQ_HANDLED; +} + +static u64 notrace davinci_timer_read_sched_clock(void) +{ + struct davinci_timer_data *timer; + unsigned int val; + + timer = davinci_timer_clksrc_timer; + val = davinci_timer_read(timer, timer->regs->tim_off); + + return val; +} + +static u64 davinci_timer_clksrc_read(struct clocksource *dev) +{ + struct davinci_timer_clocksource *clocksource; + const struct davinci_timer_regs *regs; + unsigned int val; + + clocksource = to_davinci_timer_clocksource(dev); + regs = clocksource->timer.regs; + + val = davinci_timer_read(&clocksource->timer, regs->tim_off); + + return val; +} + +static int davinci_timer_set_next_event(unsigned long cycles, + struct clock_event_device *dev) +{ + struct davinci_timer_clockevent *clockevent; + + clockevent = to_davinci_timer_clockevent(dev); + davinci_timer_set_period(&clockevent->timer, cycles); + + return 0; +} + +static int davinci_timer_set_state_shutdown(struct clock_event_device *dev) +{ + struct davinci_timer_clockevent *clockevent; + + clockevent = to_davinci_timer_clockevent(dev); + clockevent->timer.mode = DAVINCI_TIMER_MODE_DISABLED; + + return 0; +} + +static int davinci_timer_set_state_periodic(struct clock_event_device *dev) +{ + struct davinci_timer_clockevent *clockevent; + unsigned int period; + + clockevent = to_davinci_timer_clockevent(dev); + period = clockevent->tick_rate / HZ; + + clockevent->timer.mode = DAVINCI_TIMER_MODE_PERIODIC; + davinci_timer_set_period(&clockevent->timer, period); + + return 0; +} + +static int davinci_timer_set_state_oneshot(struct clock_event_device *dev) +{ + struct davinci_timer_clockevent *clockevent; + + clockevent = to_davinci_timer_clockevent(dev); + clockevent->timer.mode = DAVINCI_TIMER_MODE_ONESHOT; + + return 0; +} + +static void davinci_timer_init(void __iomem *base) +{ + /* Set clock to internal mode and disable it. */ + writel_relaxed(0x0, base + DAVINCI_TIMER_REG_TCR); + /* + * Reset both 32-bit timers, set no prescaler for timer 34, set the + * timer to dual 32-bit unchained mode, unreset both 32-bit timers. + */ + writel_relaxed(DAVINCI_TIMER_TGCR_DEFAULT, + base + DAVINCI_TIMER_REG_TGCR); + /* Init both counters to zero. */ + writel_relaxed(0x0, base + DAVINCI_TIMER_REG_TIM12); + writel_relaxed(0x0, base + DAVINCI_TIMER_REG_TIM34); +} + +int __init davinci_timer_register(struct clk *clk, + const struct davinci_timer_cfg *timer_cfg) +{ + struct davinci_timer_clocksource *clocksource; + struct davinci_timer_clockevent *clockevent; + void __iomem *base; + int rv; + + rv = clk_prepare_enable(clk); + if (rv) { + pr_err("Unable to prepare and enable the timer clock"); + return rv; + } + + base = request_mem_region(timer_cfg->reg.start, + resource_size(&timer_cfg->reg), + "davinci-timer"); + if (!base) { + pr_err("Unable to request memory region"); + return -EBUSY; + } + + base = ioremap(timer_cfg->reg.start, resource_size(&timer_cfg->reg)); + if (!base) { + pr_err("Unable to map the register range"); + return -ENOMEM; + } + + davinci_timer_init(base); + + clockevent = kzalloc(sizeof(*clockevent), GFP_KERNEL); + if (!clockevent) { + pr_err("Error allocating memory for clockevent data"); + return -ENOMEM; + } + + clockevent->dev.name = "tim12"; + clockevent->dev.features = CLOCK_EVT_FEAT_ONESHOT; + clockevent->dev.set_next_event = davinci_timer_set_next_event; + clockevent->dev.set_state_shutdown = davinci_timer_set_state_shutdown; + clockevent->dev.set_state_periodic = davinci_timer_set_state_periodic; + clockevent->dev.set_state_oneshot = davinci_timer_set_state_oneshot; + clockevent->dev.cpumask = cpumask_of(0); + clockevent->tick_rate = clk_get_rate(clk); + clockevent->timer.mode = DAVINCI_TIMER_MODE_DISABLED; + clockevent->timer.base = base; + clockevent->timer.regs = &davinci_timer_tim12_regs; + + if (timer_cfg->cmp_off) { + clockevent->timer.cmp_off = timer_cfg->cmp_off; + clockevent->timer.set_period = davinci_timer_set_period_cmp; + } else { + clockevent->dev.features |= CLOCK_EVT_FEAT_PERIODIC; + clockevent->timer.set_period = davinci_timer_set_period_std; + } + + rv = request_irq(timer_cfg->irq[DAVINCI_TIMER_CLOCKEVENT_IRQ].start, + davinci_timer_irq_timer, IRQF_TIMER, + "clockevent", clockevent); + if (rv) { + pr_err("Unable to request the clockevent interrupt"); + return rv; + } + + clockevents_config_and_register(&clockevent->dev, + clockevent->tick_rate, + DAVINCI_TIMER_MIN_DELTA, + DAVINCI_TIMER_MAX_DELTA); + + 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; + + 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"; + } + + 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; + } + + rv = clocksource_register_hz(&clocksource->dev, clockevent->tick_rate); + if (rv) { + pr_err("Unable to register clocksource"); + return rv; + } + + davinci_timer_clksrc_timer = &clocksource->timer; + + sched_clock_register(davinci_timer_read_sched_clock, + DAVINCI_TIMER_CLKSRC_BITS, + clockevent->tick_rate); + + davinci_timer_set_period(&clockevent->timer, + clockevent->tick_rate / HZ); + davinci_timer_set_period(&clocksource->timer, UINT_MAX); + + return 0; +} + +static int __init of_davinci_timer_register(struct device_node *np) +{ + struct davinci_timer_cfg timer_cfg = { }; + struct clk *clk; + int rv; + + rv = of_address_to_resource(np, 0, &timer_cfg.reg); + if (rv) { + pr_err("Unable to get the register range for timer"); + return rv; + } + + rv = of_irq_to_resource_table(np, timer_cfg.irq, + DAVINCI_TIMER_NUM_IRQS); + if (rv != DAVINCI_TIMER_NUM_IRQS) { + pr_err("Unable to get the interrupts for timer"); + return rv; + } + + clk = of_clk_get(np, 0); + if (IS_ERR(clk)) { + pr_err("Unable to get the timer clock"); + return PTR_ERR(clk); + } + + rv = davinci_timer_register(clk, &timer_cfg); + if (rv) + clk_put(clk); + + return rv; +} +TIMER_OF_DECLARE(davinci_timer, "ti,da830-timer", of_davinci_timer_register); diff --git a/include/clocksource/timer-davinci.h b/include/clocksource/timer-davinci.h new file mode 100644 index 000000000000..1dcc1333fbc8 --- /dev/null +++ b/include/clocksource/timer-davinci.h @@ -0,0 +1,44 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * TI DaVinci clocksource driver + * + * Copyright (C) 2019 Texas Instruments + * Author: Bartosz Golaszewski <bgolaszewski@baylibre.com> + */ + +#ifndef __TIMER_DAVINCI_H__ +#define __TIMER_DAVINCI_H__ + +#include <linux/clk.h> +#include <linux/ioport.h> + +enum { + DAVINCI_TIMER_CLOCKEVENT_IRQ, + DAVINCI_TIMER_CLOCKSOURCE_IRQ, + DAVINCI_TIMER_NUM_IRQS, +}; + +/** + * struct davinci_timer_cfg - davinci clocksource driver configuration struct + * @reg: register range resource + * @irq: clockevent and clocksource interrupt resources + * @cmp_off: if set - it specifies the compare register used for clockevent + * + * Note: if the compare register is specified, the driver will use the bottom + * clock half for both clocksource and clockevent and the compare register + * to generate event irqs. The user must supply the correct compare register + * interrupt number. + * + * This is only used by da830 the DSP of which uses the top half. The timer + * driver still configures the top half to run in free-run mode. + */ +struct davinci_timer_cfg { + struct resource reg; + struct resource irq[DAVINCI_TIMER_NUM_IRQS]; + unsigned int cmp_off; +}; + +int __init davinci_timer_register(struct clk *clk, + const struct davinci_timer_cfg *data); + +#endif /* __TIMER_DAVINCI_H__ */