| 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: <cb6ef0c7-c8ca-73e6-0c15-62fee7fd0aaa@linaro.org>
Date: Tue, 16 Apr 2019 18:05:31 +0200
From: Daniel Lezcano <daniel.lezcano@...aro.org>
To: Bartosz Golaszewski <brgl@...ev.pl>
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
Hi Bartosz,
On 16/04/2019 15:44, Bartosz Golaszewski wrote:
> pon., 15 kwi 2019 o 14:36 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.
>>
>> Can you give a quick description of the timer hardware and how it works?
>>
>
> Will do.
>
>>> 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"
>>
>> 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@...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,
>>> +};
>>
>> 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.
--
<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
Powered by blists - more mailing lists