From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753664AbbIYAZp (ORCPT ); Thu, 24 Sep 2015 20:25:45 -0400 Received: from mail-pa0-f46.google.com ([209.85.220.46]:34738 "EHLO mail-pa0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752632AbbIYAZn (ORCPT ); Thu, 24 Sep 2015 20:25:43 -0400 Subject: Re: [PATCH v1 1/3] clocksource: rockchip: Make the driver more readability and compatible To: Caesar Wang , =?UTF-8?Q?Heiko_St=c3=bcbner?= References: <1442566271-10695-1-git-send-email-wxt@rock-chips.com> <1442566271-10695-2-git-send-email-wxt@rock-chips.com> <5455510.kk9S3OIaU9@diego> <56016275.4080704@rock-chips.com> Cc: Caesar Wang , devicetree@vger.kernel.org, catalin.marinas@arm.com, will.deacon@arm.com, linux-kernel@vger.kernel.org, linux-rockchip@lists.infradead.org, olof@lixom.net, tglx@linutronix.de, linux-arm-kernel@lists.infradead.org From: Daniel Lezcano Message-ID: <56049476.1040605@linaro.org> Date: Thu, 24 Sep 2015 17:25:26 -0700 User-Agent: Mozilla/5.0 (X11; Linux i686; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 MIME-Version: 1.0 In-Reply-To: <56016275.4080704@rock-chips.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Caesar, so thinking a bit more about this patch. I would like to split it into two. One fixing the NO_IRQ and another fixing the dsb(). IIUC, the ARMv8 support is not yet ready and dsb() is not necessary as a fix for the previous kernel version. However, the timer is used with the ARMv7 boards and the NO_IRQ should be merged into tip-urgent. I already done the fix and I am ready to submit it (for the timer keystone also). So I suggest your resend the dsb() fix only. Regarding the indentation, I prefer you do that in a separate patch by cleaning up the macros (if relevant) or send the patch to trivial@ -- Daniel On 09/22/2015 07:15 AM, Caesar Wang wrote: > Hi Heiko, > > 在 2015年09月22日 22:00, Heiko Stübner 写道: >> Hi Caesar, >> >> Am Freitag, 18. September 2015, 16:51:09 schrieb Caesar Wang: >>> Build the arm64 SoCs (e.g.: RK3368) on Rockchip platform, >>> There are some failure with build up on timer driver for rockchip. >>> >>> logs: >>> ... >>> drivers/clocksource/rockchip_timer.c:156:13: error: 'NO_IRQ' undeclared >>> /tmp/ccdAnNy5.s:47: Error: missing immediate expression at operand 1 -- >>> `dsb` >>> ... >>> >>> The problem was different semantics of dsb on btw arm32 and arm64, >>> Here we can convert the dsb with insteading of dsb(sy). >>> >>> NO_IRQ definition is missing for ARM64, since NO_IRQ being -1 is a >>> legacy thing for ARM - all ARM drivers are supposed to be converted to >>> use <= 0 or == 0 to detect invalid IRQs, and _eventually_ once all users >>> are gone, NO_IRQ deleted. Modern drivers should _all_ be using !irq to >>> detect invalid IRQs, and not using NO_IRQ. >>> >>> Meanwhile, I change a bit to make the code more readability for driver >>> when I check the code style. >>> >>> Signed-off-by: Caesar Wang >>> --- >>> >>> Changes in v1: >>> - As Russell, Thomas, Daniel comments, let's replace NO_IRQ by '!irq'. >>> >>> drivers/clocksource/rockchip_timer.c | 29 >>> +++++++++++++++-------------- >>> 1 file changed, 15 insertions(+), 14 deletions(-) >>> >>> diff --git a/drivers/clocksource/rockchip_timer.c >>> b/drivers/clocksource/rockchip_timer.c index bb2c2b0..e1af449 100644 >>> --- a/drivers/clocksource/rockchip_timer.c >>> +++ b/drivers/clocksource/rockchip_timer.c >>> @@ -17,16 +17,16 @@ >>> >>> #define TIMER_NAME "rk_timer" >>> >>> -#define TIMER_LOAD_COUNT0 0x00 >>> -#define TIMER_LOAD_COUNT1 0x04 >>> -#define TIMER_CONTROL_REG 0x10 >>> -#define TIMER_INT_STATUS 0x18 >>> +#define TIMER_LOAD_COUNT0 0x00 >>> +#define TIMER_LOAD_COUNT1 0x04 >>> +#define TIMER_CONTROL_REG 0x10 >>> +#define TIMER_INT_STATUS 0x18 >>> >>> -#define TIMER_DISABLE 0x0 >>> -#define TIMER_ENABLE 0x1 >>> -#define TIMER_MODE_FREE_RUNNING (0 << 1) >>> -#define TIMER_MODE_USER_DEFINED_COUNT (1 << 1) >>> -#define TIMER_INT_UNMASK (1 << 2) >>> +#define TIMER_DISABLE (0 << 0) >>> +#define TIMER_ENABLE (1 << 0) >>> +#define TIMER_MODE_FREE_RUNNING (0 << 1) >>> +#define TIMER_MODE_USER_DEFINED_COUNT (1 << 1) >>> +#define TIMER_INT_UNMASK (1 << 2) >> not sure how Daniel sees this, but those could count as "unrelated >> change", as >> they have nothing to do with the arm64 build-fixes. > > Yep, it's no related to the arm64 uild fixes. > I only make the code more readability for driver. > >> >>> struct bc_timer { >>> struct clock_event_device ce; >>> @@ -49,14 +49,14 @@ static inline void __iomem *rk_base(struct >>> clock_event_device *ce) static inline void rk_timer_disable(struct >>> clock_event_device *ce) { >>> writel_relaxed(TIMER_DISABLE, rk_base(ce) + TIMER_CONTROL_REG); >>> - dsb(); >>> + dsb(sy); >>> } >>> >>> static inline void rk_timer_enable(struct clock_event_device *ce, u32 >>> flags) { >>> writel_relaxed(TIMER_ENABLE | TIMER_INT_UNMASK | flags, >>> rk_base(ce) + TIMER_CONTROL_REG); >>> - dsb(); >>> + dsb(sy); >>> } >>> >>> static void rk_timer_update_counter(unsigned long cycles, >>> @@ -64,13 +64,13 @@ static void rk_timer_update_counter(unsigned long >>> cycles, { >>> writel_relaxed(cycles, rk_base(ce) + TIMER_LOAD_COUNT0); >>> writel_relaxed(0, rk_base(ce) + TIMER_LOAD_COUNT1); >>> - dsb(); >>> + dsb(sy); >>> } >>> >>> static void rk_timer_interrupt_clear(struct clock_event_device *ce) >>> { >>> writel_relaxed(1, rk_base(ce) + TIMER_INT_STATUS); >>> - dsb(); >>> + dsb(sy); >>> } >>> >>> static inline int rk_timer_set_next_event(unsigned long cycles, >>> @@ -148,7 +148,7 @@ static void __init rk_timer_init(struct >>> device_node *np) >>> bc_timer.freq = clk_get_rate(timer_clk); >>> >>> irq = irq_of_parse_and_map(np, 0); >>> - if (irq == NO_IRQ) { >>> + if (!irq) { >>> pr_err("Failed to map interrupts for '%s'\n", TIMER_NAME); >>> return; >>> } >>> @@ -173,4 +173,5 @@ static void __init rk_timer_init(struct >>> device_node *np) >>> >>> clockevents_config_and_register(ce, bc_timer.freq, 1, UINT_MAX); >>> } >>> + >> unnecessary addition of a blank line (same reasons as above) > > It's the same reason with the above. > > CHECK: Please use a blank line after function/struct/union/enum > declarations > #176: FILE: rockchip_timer.c:176: > +} > +CLOCKSOURCE_OF_DECLARE(rk_timer, "rockchip,rk3288-timer", rk_timer_init); > > I know, we can ignore the above warning. > That's a bit better, I thnik. > > >>> CLOCKSOURCE_OF_DECLARE(rk_timer, "rockchip,rk3288-timer", >>> rk_timer_init); >> >> Heiko >> >> _______________________________________________ >> Linux-rockchip mailing list >> Linux-rockchip@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-rockchip > > -- Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog