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: Fri, 25 Sep 2015 10:18:08 +0800 Message-ID: <5604AEE0.5000300@gmail.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> <56016275.4080704@rock-chips.com> <56049476.1040605@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: In-Reply-To: <56049476.1040605-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Daniel Lezcano Cc: Caesar Wang , =?UTF-8?B?SGVpa28gU3TDvGJuZQ==?= =?UTF-8?B?cg==?= , devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, catalin.marinas-5wv7dgnIgG8@public.gmane.org, will.deacon-5wv7dgnIgG8@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, olof-nZhT3qVonbNeoWH0uzbU5w@public.gmane.org, tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org List-Id: devicetree@vger.kernel.org Daniel, =E5=9C=A8 2015=E5=B9=B409=E6=9C=8825=E6=97=A5 08:25, Daniel Lezcano =E5= =86=99=E9=81=93: > > Hi Caesar, > > so thinking a bit more about this patch. I would like to split it int= o=20 > two. One fixing the NO_IRQ and another fixing the dsb(). > > IIUC, the ARMv8 support is not yet ready and dsb() is not necessary a= s=20 > a fix for the previous kernel version. However, the timer is used wit= h=20 > 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=20 > keystone also). So I suggest your resend the dsb() fix only. > > Regarding the indentation, I prefer you do that in a separate patch b= y=20 > cleaning up the macros (if relevant) or send the patch to trivial@ > I know the indentation is trivial for this driver, but I just send the=20 patch v2. > -- Daniel > > On 09/22/2015 07:15 AM, Caesar Wang wrote: >> Hi Heiko, >> >> =E5=9C=A8 2015=E5=B9=B409=E6=9C=8822=E6=97=A5 22:00, Heiko St=C3=BCb= ner =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'=20 >>>> undeclared >>>> /tmp/ccdAnNy5.s:47: Error: missing immediate expression at operand= =20 >>>> 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 converte= d to >>>> use <=3D 0 or =3D=3D 0 to detect invalid IRQs, and _eventually_ on= ce all=20 >>>> users >>>> are gone, NO_IRQ deleted. Modern drivers should _all_ be using !ir= q to >>>> detect invalid IRQs, and not using NO_IRQ. >>>> >>>> Meanwhile, I change a bit to make the code more readability for dr= iver >>>> 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 '!i= rq'. >>>> >>>> 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 1006= 44 >>>> --- 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_RE= G); >>>> - dsb(); >>>> + dsb(sy); >>>> } >>>> >>>> static inline void rk_timer_enable(struct clock_event_device *ce= ,=20 >>>> 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 l= ong >>>> 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 =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_node *np) >>>> >>>> clockevents_config_and_register(ce, bc_timer.freq, 1, UINT_M= AX); >>>> } >>>> + >>> 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",=20 >> 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-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org >>> http://lists.infradead.org/mailman/listinfo/linux-rockchip >> >> > > > > --=20 > Thanks, > Caesar -- To unsubscribe from this list: send the line "unsubscribe devicetree" i= n the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html