From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.1 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5FC20C10F13 for ; Tue, 16 Apr 2019 16:05:39 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 1992E20821 for ; Tue, 16 Apr 2019 16:05:39 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="hqewXpKb" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729831AbfDPQFh (ORCPT ); Tue, 16 Apr 2019 12:05:37 -0400 Received: from mail-wm1-f68.google.com ([209.85.128.68]:36074 "EHLO mail-wm1-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726605AbfDPQFh (ORCPT ); Tue, 16 Apr 2019 12:05:37 -0400 Received: by mail-wm1-f68.google.com with SMTP id h18so26174571wml.1 for ; Tue, 16 Apr 2019 09:05:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=oaBdBQN9VKe3y7T9q4ikjsfPMPRTSeQlMsuSLt0Xe+4=; b=hqewXpKbaqckLYWaFXfQUUvPJN+gxdVG8sdIkpM5oh3v8i1XjJdG/sQ4eA6oQ2tTx/ CBq3rSb87WGCNeVVpLNp6ARA9rhr/BQRqR5Uk8LujF5su83kL0RlzAcw/AmmW4NNx9RI bPyjX8nodsfVo2XKVh5cthqDWKYA6u8QAk/YywiZhuN1x6HkAYa5dei4fDw53F7K7nw9 J9yxm+RJ4ChafMj6mKL94uulOe6MhrhxgBopJrXxSJLxPSowUTe03pdOx8ANKsYB1LOP L4NqtOZKL1nq/dGAPOscnppKDSgGHCrWIt080TxlOGFLVTfXdPdTO7zszP98bDNtN123 xf2A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=oaBdBQN9VKe3y7T9q4ikjsfPMPRTSeQlMsuSLt0Xe+4=; b=ukW1v+YX5qOzk5bPma9mOgnutNltQcJkFtqKZbZntVu3eOKOzIjHzy72LsOeBfULcQ ZvfYthvjeyRKKEUazoSsnNQ0JV4J1Uf+bKHZsPq7bkfGqtsxFh7cK/V0PiygPOV+BnQr WU3U+6R2+7iudY88Hnwyfr/4ZX23l4le/RkJvb/N0ZtAxf3wMYmLFluhuIMuCNuHhZBn 4bvyHPzVSGKrppzC0Z8yUJCdC4zOUe2Lie6Ci95VEpKCtsMhKqcZK+imcYg2MXtu7S/o 4H6uPeo/2naDnBEcZ8GC9NH1hMYHDfHppUCIdp/MNB7rs7ehSQIwsJlwdS/qMcizVxRw 5Ryw== X-Gm-Message-State: APjAAAW2TvistNc5slwA4+S8HVasIzw1Rtr3/BfvpanvyiZM8/JKt+7k MLZOH5KMfoOckZbW/+5JYjdejg== X-Google-Smtp-Source: APXvYqzQVkWMhzlboEWEJNB3dUZ6/ozD6JaIKoI1Gp1j8XNdrWSxkHVgi7Oulto6I6PH8YXBnBCTJw== X-Received: by 2002:a1c:1a46:: with SMTP id a67mr28031453wma.21.1555430733434; Tue, 16 Apr 2019 09:05:33 -0700 (PDT) Received: from [192.168.0.41] (50.251.95.92.rev.sfr.net. [92.95.251.50]) by smtp.googlemail.com with ESMTPSA id q24sm16361688wmj.26.2019.04.16.09.05.31 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 16 Apr 2019 09:05:32 -0700 (PDT) Subject: Re: [PATCH v4 01/11] clocksource: davinci-timer: new driver To: Bartosz Golaszewski Cc: Sekhar Nori , Kevin Hilman , Thomas Gleixner , David Lechner , Linux ARM , Linux Kernel Mailing List , Bartosz Golaszewski References: <20190318121100.28132-1-brgl@bgdev.pl> <20190318121100.28132-2-brgl@bgdev.pl> From: Daniel Lezcano Message-ID: Date: Tue, 16 Apr 2019 18:05:31 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Bartosz, On 16/04/2019 15:44, Bartosz Golaszewski wrote: > pon., 15 kwi 2019 o 14:36 Daniel Lezcano napisał(a): >> >> On 18/03/2019 13:10, Bartosz Golaszewski wrote: >>> From: Bartosz Golaszewski >>> >>> 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 >>> Reviewed-by: David Lechner >>> --- >>> 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 >>> +// (with some parts adopted from code by Kevin Hilman ) >>> + >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> + >>> +#include >>> + >>> +#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. -- Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog