Re: [PATCH v1 1/3] clocksource: rockchip: Make the driver more readability and compatible

[Caesar Wang <caesar.upstream@gmail.com>]

Daniel,

在 2015年09月25日 08:25, Daniel Lezcano 写道:
>
> 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@
>

I know the indentation is trivial for this driver, but I just send the 
patch v2.

>   -- 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 <wxt@rock-chips.com>
>>>> ---
>>>>
>>>> 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
>>
>>
>
>
>
> -- 
> Thanks,
> Caesar