| 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
| ||
|
Date: Mon, 4 Feb 2019 20:13:07 -0600
From: David Lechner <david@...hnology.com>
To: Bartosz Golaszewski <brgl@...ev.pl>, Sekhar Nori <nsekhar@...com>,
Kevin Hilman <khilman@...nel.org>,
Daniel Lezcano <daniel.lezcano@...aro.org>,
Rob Herring <robh+dt@...nel.org>,
Mark Rutland <mark.rutland@....com>,
Thomas Gleixner <tglx@...utronix.de>
Cc: devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org,
Bartosz Golaszewski <bgolaszewski@...libre.com>
Subject: Re: [PATCH v2 02/12] clocksource: davinci-timer: new driver
On 2/4/19 11:17 AM, 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, used 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>
> ---
...
> diff --git a/drivers/clocksource/timer-davinci.c b/drivers/clocksource/timer-davinci.c
> new file mode 100644
> index 000000000000..a6d2d3f6526e
> --- /dev/null
> +++ b/drivers/clocksource/timer-davinci.c
> @@ -0,0 +1,431 @@
> +// SPDX-License-Identifier: GPL-2.0
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>
> +
> +#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)
Are these 3 values essentially an enum of exclusive values?
If so, the the BIT() macro doesn't seem right here. If they
are flags, then BIT() is OK, but DAVINCI_TIMER_ENAMODE_DISABLED
could be omitted.
> +
> +#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 {
> + unsigned int tim_off;> + unsigned int prd_off;
It is not obvious to me what the abbreviations tim and prd
mean. Add comments or change the names?
> + unsigned int enamode_shift;
> +};
> +
...
> +static void davinci_timer_update(struct davinci_timer_data *timer,
> + unsigned int reg, unsigned int mask,
> + unsigned int val)
> +{
> + unsigned int new, orig = davinci_timer_read(timer, reg);
Slightly more readable with function not in variable declaration?
> +
> + new = orig & ~mask;
> + new |= val & mask;
> +
> + davinci_timer_write(timer, reg, new);
> +}
...
> +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("%s: Unable to prepare and enable the timer clock\n",
use pr_fmt marco to simplify? e.g. at the top of the file...
#define pr_fmt(fmt) "%s: " fmt "\n", __func__
Then just
pr_err("Unable to prepare and enable the timer clock");
> + __func__);
> + return rv;
> + }
...
> diff --git a/include/clocksource/timer-davinci.h b/include/clocksource/timer-davinci.h
> new file mode 100644
> index 000000000000..ef1de78a1820
> --- /dev/null
> +++ b/include/clocksource/timer-davinci.h
> @@ -0,0 +1,44 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
Seems odd to have the header GPL-2.0+ when the c file GPL-2.0-only
> +/*
> + * TI DaVinci clocksource driver
> + *
> + * Copyright (C) 2019 Texas Instruments
> + * Author: Bartosz Golaszewski <bgolaszewski@...libre.com>
> + */
> +
> +#ifndef __TIMER_DAVINCI_H__
> +#define __TIMER_DAVINCI_H__
> +
> +#include <linux/clk.h>
> +#include <linux/ioport.h>
> +
> +enum {
> + DAVINCI_TIMER_CLOCKEVENT_IRQ = 0,
Isn't = 0 implied, so can be omitted?
> + 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.
I'm a little confused by this comment. I think I get it, but it took me a while.
If cmp_off is 0, we don't use the compare register and therefore
DAVINCI_TIMER_CLOCKEVENT_IRQ and DAVINCI_TIMER_CLOCKSOURCE_IRQ should
be TINT12 and TINT34 (or vice versa?). If cmp_off is non-zero, then
it should be one of the 8 (more or less depending on the SoC) and
DAVINCI_TIMER_CLOCKEVENT_IRQ should be something like T12CMPINT0.
> + *
> + * 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__ */
>
Powered by blists - more mailing lists