Re: [PATCH v4 01/11] clocksource: davinci-timer: new driver

[Daniel Lezcano]

Hi Bartosz,


On 16/04/2019 15:44, Bartosz Golaszewski wrote:
> pon., 15 kwi 2019 o 14:36 Daniel Lezcano <daniel.lezcano@xxxxxxxxxx> napisaÅ(a):
>>
>> On 18/03/2019 13:10, Bartosz Golaszewski wrote:
>>> From: Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>
>>>
>>> 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@xxxxxxxxxxxx>
>>> Reviewed-by: David Lechner <david@xxxxxxxxxxxxxx>
>>> ---
>>>  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@xxxxxxxxxxxx>
>>> +// (with some parts adopted from code by Kevin Hilman <khilman@xxxxxxxxxxxx>)
>>> +
>>> +#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