lkml.org 
[lkml]   [2015]   [Jan]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] clockevents: rockchip: Add rockchip timer for rk3288
On 01/13/2015 12:20 AM, Heiko Stübner wrote:
> Hi Daniel,
>
> sorry it took a bit longer to reply.

No problem :)

> Generally it looks good to me, just some minor issues inline below.

Thanks for the review.

> It would also be nice to include the rockchip list (linux-
> rockchip@lists.infradead.org) for future patches.

Yes, sure. I will do that in the future.

[...]

>> +- clock-frequency: the frequency the timer is running
>> +
>> +Example:
>> + timer: timer@ff810000 {
>> + compatible = "rockchip,rk3288-timer";
>> + reg = <0xff810000 0x20>;
>> + interrupts = <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>;
>> + clock-frequency = <24000000>;
>
> wouldn't it make sense to use the actual supplying clock?
>
> For the timer you want to use it is just the non-gateable &xin24m, but the
> timers in the other block (timer0-5) actually do have gateable clocks.
>
> Similarly there is a pclk_timer supplying at least one of the two actual
> blocks. I'll try to inquire how the blocks are actually supplied.

Ok, are you suggesting I should use another timer so we can gate it for
power efficiency ?

>> + };
>> diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
>> index 2a878a3..7a7db48 100644
>> --- a/arch/arm/boot/dts/rk3288.dtsi
>> +++ b/arch/arm/boot/dts/rk3288.dtsi
>> @@ -149,6 +149,13 @@
>> clock-frequency = <24000000>;
>> };
>>
>> + timer: timer@ff810000 {
>> + compatible = "rockchip,rk3288-timer";
>> + reg = <0xff810000 0x20>;
>> + interrupts = <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>;
>> + clock-frequency = <24000000>;
>> + };
>> +
>> sdmmc: dwmmc@ff0c0000 {
>> compatible = "rockchip,rk3288-dw-mshc";
>> clock-freq-min-max = <400000 150000000>;
>> diff --git a/arch/arm/mach-rockchip/Kconfig b/arch/arm/mach-rockchip/Kconfig
>> index ac5803c..4c3fa7e 100644
>> --- a/arch/arm/mach-rockchip/Kconfig
>> +++ b/arch/arm/mach-rockchip/Kconfig
>> @@ -11,6 +11,7 @@ config ARCH_ROCKCHIP
>> select HAVE_ARM_SCU if SMP
>> select HAVE_ARM_TWD if SMP
>> select DW_APB_TIMER_OF
>> + select RK3288_TIMER
>> select ARM_GLOBAL_TIMER
>> select CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCK
>> help
>> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
>> index fc01ec2..6ed97a6 100644
>> --- a/drivers/clocksource/Kconfig
>> +++ b/drivers/clocksource/Kconfig
>> @@ -26,6 +26,10 @@ config DW_APB_TIMER_OF
>> select DW_APB_TIMER
>> select CLKSRC_OF
>>
>> +config RK3288_TIMER
>> + bool
>> + select CLKSRC_OF
>> +
>
> the config symbol to compile rockchip_timer.c is RK3288_TIMER?
> I'd think it could match a bit more ...
> ROCKCHIP_TIMER -> rockchip_timer.c
> or
> RK3288_TIMER -> rk3288_timer.c
>
> This timer-block is actually also present on the rk3188, but not on the rk3066
> which uses an unmodified dw_apb_timer.

Ok, then rockchip_timer.c fits better. I will do the change.

>> config ARMADA_370_XP_TIMER
>> bool
>> select CLKSRC_OF
>> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
>> +
>> + if (of_property_read_u32(np, "clock-frequency", &bc_timer.freq)) {
>
> formatting issue with spaces instead of tabs in the 5 lines above

Copy that.

>> + pr_err("Failed to read the frequency for '%s'\n", TIMER_NAME);
>> + return;
>> + }

Thanks!
-- Daniel

--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog



\
 
 \ /
  Last update: 2015-01-14 17:41    [W:0.061 / U:0.400 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site