From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752222AbdEPL7X (ORCPT ); Tue, 16 May 2017 07:59:23 -0400 Received: from mail.free-electrons.com ([62.4.15.54]:44571 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750930AbdEPL7S (ORCPT ); Tue, 16 May 2017 07:59:18 -0400 Date: Tue, 16 May 2017 13:59:16 +0200 From: Alexandre Belloni To: Daniel Lezcano Cc: Nicolas Ferre , Boris Brezillon , Jean-Christophe Plagniol-Villard , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Thomas Gleixner Subject: Re: [PATCH 42/48] clocksource/drivers: Add a new driver for the Atmel ARM TC blocks Message-ID: <20170516115916.ip3ea34bofg3jpuo@piout.net> References: <1465596231-21766-1-git-send-email-alexandre.belloni@free-electrons.com> <1465596231-21766-43-git-send-email-alexandre.belloni@free-electrons.com> <576D0645.2070805@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <576D0645.2070805@linaro.org> User-Agent: NeoMutt/20170113 (1.7.2) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Daniel, Almost one year later, I'm back on that topic. On 24/06/2016 at 12:07:01 +0200, Daniel Lezcano wrote: > > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig > > index 47352d25c15e..ff7f4022c749 100644 > > --- a/drivers/clocksource/Kconfig > > +++ b/drivers/clocksource/Kconfig > > @@ -258,6 +258,19 @@ config ATMEL_ST > > select CLKSRC_OF > > select MFD_SYSCON > > > > +config ATMEL_ARM_TCB_CLKSRC > > + bool "TC Block Clocksource" > > The Kconfig options are set now with the COMPILE_TEST option in order to > increase the compilation test coverage. > > Please, add bool "TC Block Clocksource" if COMPILE_TEST, ... > > > + select REGMAP_MMIO > > + depends on GENERIC_CLOCKEVENTS > > + depends on SOC_AT91RM9200 || SOC_AT91SAM9 || SOC_SAMA5 > > + default SOC_AT91RM9200 || SOC_AT91SAM9 || SOC_SAMA5 > > ... remove these dependencies and let the SoC's Kconfig to select the timer > like the other timers are. > The main issue with what you suggest is that it removes the possibility to not compile the driver. This may be interesting for people using the PIT as the clocksource/clockevent and the TCBs for something else. > > +static cycle_t tc_get_cycles(struct clocksource *cs) > > +{ > > + unsigned long flags; > > + u32 lower, upper, tmp; > > + > > + raw_local_irq_save(flags); > > + do { > > + regmap_read(tc.regmap, ATMEL_TC_CV(1), &upper); > > + regmap_read(tc.regmap, ATMEL_TC_CV(0), &lower); > > + regmap_read(tc.regmap, ATMEL_TC_CV(1), &tmp); > > + } while (upper != tmp); > > + > > + raw_local_irq_restore(flags); > > Why is this lock needed ? > I've taken that from the old driver, I'm not sure this is needed. Maybe to lower the chance to have upper != tmp. > > + return (upper << 16) | lower; > > +} > > + > > +static int tcb_clkevt_oneshot(struct clock_event_device *dev) > > +{ > > + int ret; > > + > > + if (tc.irq_requested) > > + return 0; > > + > > + ret = request_irq(tc.irq, tc_clkevt_irq, IRQF_TIMER | IRQF_SHARED, > > + "tcb_clkevt", &tc); > > + if (!ret) > > + tc.irq_requested = true; > > The legacy driver checks clockevent_state_detached() and disables the clock. > This feature is different from the legacy driver. Here, the driver is using a single TCB channel for both clocksource and clockevent while the legacy driver always uses at least two channels. > Why is 'irq_requested' and request_irq/free_irq cleaner ? > > Isn't there a configuration with the TCB register to disable the clockevent > only ? > I'll try something cleaner anyway. -- Alexandre Belloni, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com