| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAMRc=MezSr4YCz_V=KMKiZTZDuT01T+Es4nAHDES=07qcYysrQ@mail.gmail.com>
Date: Mon, 8 Apr 2019 15:49:03 +0200
From: Bartosz Golaszewski <brgl@...ev.pl>
To: Daniel Lezcano <daniel.lezcano@...aro.org>
Cc: Sekhar Nori <nsekhar@...com>, Kevin Hilman <khilman@...nel.org>,
Thomas Gleixner <tglx@...utronix.de>,
David Lechner <david@...hnology.com>,
Linux ARM <linux-arm-kernel@...ts.infradead.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Bartosz Golaszewski <bgolaszewski@...libre.com>
Subject: Re: [PATCH v4 01/11] clocksource: davinci-timer: new driver
wt., 2 kwi 2019 o 11:21 Daniel Lezcano <daniel.lezcano@...aro.org> napisał(a):
>
> On 18/03/2019 13:10, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@...libre.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@...libre.com>
> > Reviewed-by: David Lechner <david@...hnology.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@...libre.com>
> > +// (with some parts adopted from code by Kevin Hilman <khilman@...libre.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
Powered by blists - more mailing lists