From mboxrd@z Thu Jan 1 00:00:00 1970 From: Caesar Wang Subject: Re: [PATCH v1 1/3] clocksource: rockchip: Make the driver more readability and compatible Date: Tue, 22 Sep 2015 22:15:17 +0800 Message-ID: <56016275.4080704@rock-chips.com> References: <1442566271-10695-1-git-send-email-wxt@rock-chips.com> <1442566271-10695-2-git-send-email-wxt@rock-chips.com> <5455510.kk9S3OIaU9@diego> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: In-Reply-To: <5455510.kk9S3OIaU9@diego> Sender: linux-kernel-owner@vger.kernel.org To: =?UTF-8?B?SGVpa28gU3TDvGJuZXI=?= Cc: Caesar Wang , devicetree@vger.kernel.org, catalin.marinas@arm.com, daniel.lezcano@linaro.org, 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 List-Id: devicetree@vger.kernel.org Hi Heiko, =E5=9C=A8 2015=E5=B9=B409=E6=9C=8822=E6=97=A5 22:00, Heiko St=C3=BCbner= =E5=86=99=E9=81=93: > 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' undecla= red >> /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 <=3D 0 or =3D=3D 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 driv= er >> 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 ch= ange", 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 lon= g >> 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_n= ode *np) >> bc_timer.freq =3D clk_get_rate(timer_clk); >> >> irq =3D irq_of_parse_and_map(np, 0); >> - if (irq =3D=3D 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_n= ode *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 declara= tions #176: FILE: rockchip_timer.c:176: +} +CLOCKSOURCE_OF_DECLARE(rk_timer, "rockchip,rk3288-timer", rk_timer_ini= t); 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 --=20 ***********************************************************************= *************** =E7=8E=8B=E6=99=93=E8=85=BE Caesar Wang Product R&D Dept.III =46uzhou Rockchip Electronics Co.Ltd Addr=EF=BC=9A NO.18 Building, A District, Fuzhou Software Park,Gulou D= istrict,Fuzhou, Fujian,China(Fuzhou Headquarters) 21F,Malata Building,Kejizhongyi Avenue,Nanshan District,= Shenzhen (Shenzhen Office) Tel=EF=BC=9A+86-591-83991906/07 - 8221 Mobile:+86 15059456742 E-mail : wxt@rock-chips.com ***********************************************************************= **************** ***********************************************************************= **************** IMPORTANT NOTICE: This email is from Fuzhou Rockchip Electronics Co., L= td .The contents of this email and any attachments may contain information that is privileged, confidential and/or exempt from= disclosure under applicable law and relevant NDA. If you are not the intended recipient, you are hereby notified that any= disclosure, copying, distribution, or use of the information is STRICTLY PROHIBITED. Please immediately contact the send= er as soon as possible and destroy the material in its entirety in any format. Thank you. ***********************************************************************= ****************