[PATCH 03/74] ST SPEAr: Formalized timer support

[Shiraz Hashim]

Dear Jean,

On 9/7/2010 4:25 AM, Jean-Christophe PLAGNIOL-VILLARD wrote:
>>  
>> -#endif
>> +static void __init spear3xx_timer_init(void)
>> +{
>> +	spear_setup_timer();
> why not call it directly?

A single level of abstraction was provided so that if archs (spear3xx/6xx/13xx)
required to do something extra.

>> +}
>> +
>> +struct sys_timer spear3xx_timer = {
>> +	.init = spear3xx_timer_init,
>> +};
>> diff --git a/arch/arm/mach-spear6xx/include/mach/generic.h b/arch/arm/mach-spear6xx/include/mach/generic.h
>> index 16205a5..e5967ed 100644
>> --- a/arch/arm/mach-spear6xx/include/mach/generic.h
>> +++ b/arch/arm/mach-spear6xx/include/mach/generic.h
>> @@ -31,9 +31,10 @@
>>  /* Add spear6xx family device structure declarations here */
>>  extern struct amba_device gpio_device[];
>>  extern struct amba_device uart_device[];
>> -extern struct sys_timer spear_sys_timer;
>> +extern struct sys_timer spear6xx_timer;
>>  
>>  /* Add spear6xx family function declarations here */
>> +void __init spear_setup_timer(void);
>>  void __init spear6xx_map_io(void);
>>  void __init spear6xx_init_irq(void);
>>  void __init spear6xx_init(void);
>> diff --git a/arch/arm/mach-spear6xx/spear600_evb.c b/arch/arm/mach-spear6xx/spear600_evb.c
>> index daff8d0..bdd5b76 100644
>> --- a/arch/arm/mach-spear6xx/spear600_evb.c
>> +++ b/arch/arm/mach-spear6xx/spear600_evb.c
>> @@ -42,10 +42,11 @@ static void __init spear600_evb_init(void)
>>  		amba_device_register(amba_devs[i], &iomem_resource);
>>  }
>>  
>> +
> please remove

OK

>>  MACHINE_START(SPEAR600, "ST-SPEAR600-EVB")
>>  	.boot_params	=	0x00000100,
>>  	.map_io		=	spear6xx_map_io,
>>  	.init_irq	=	spear6xx_init_irq,
>> -	.timer		=	&spear_sys_timer,
>> +	.timer		=	&spear6xx_timer,
>>  	.init_machine	=	spear600_evb_init,
>>  MACHINE_END
>> diff --git a/arch/arm/mach-spear6xx/spear6xx.c b/arch/arm/mach-spear6xx/spear6xx.c
>> index baf6bcc..88a82ca 100644
>> --- a/arch/arm/mach-spear6xx/spear6xx.c
>> +++ b/arch/arm/mach-spear6xx/spear6xx.c
>> @@ -155,3 +155,12 @@ void __init spear6xx_map_io(void)
>>  	/* This will initialize clock framework */
>>  	clk_init();
>>  }
>> +
>> +static void __init spear6xx_timer_init(void)
>> +{
>> +	spear_setup_timer();
>> +}
>> +
>> +struct sys_timer spear6xx_timer = {
>> +	.init = spear6xx_timer_init,
>> +};
>> diff --git a/arch/arm/plat-spear/time.c b/arch/arm/plat-spear/time.c
>> index ab21165..850d0cf 100644
>> --- a/arch/arm/plat-spear/time.c
>> +++ b/arch/arm/plat-spear/time.c
>> @@ -66,6 +66,13 @@
>>  static __iomem void *gpt_base;
>>  static struct clk *gpt_clk;
>>  
>> +/* following defines the parent clock to be used */
>> +#ifdef CONFIG_ARCH_SPEAR13XX
>> +static char pclk_name[] = "osc1_24m_clk";
>> +#else
>> +static char pclk_name[] = "pll3_48m_clk";
>> +#endif
> why 2 name?

spear13xx has different parent clock for gpt than spear3xx/6xx.

> it will be better to use clkdev and create an alias

This is specifically to chose parent clock, which can be different in different
architectures. clkdev corresponding to parent clock is already defined. We use
name here to get the clock.

> btw how about move the timer to drivers/clocksource?

I don't know whether this is the right place. Every body else has placed timer
code in his respective mach/plat directory.

>> +
>>  static void clockevent_set_mode(enum clock_event_mode mode,
>>  				struct clock_event_device *clk_event_dev);
>>  static int clockevent_next_event(unsigned long evt,
>> @@ -215,7 +222,8 @@ static void __init spear_clockevent_init(void)
>>  
>>  void __init spear_setup_timer(void)
>>  {
>> -	struct clk *pll3_clk;
>> +	struct clk *clk;
>> +	int ret;
>>  
>>  	if (!request_mem_region(SPEAR_GPT0_BASE, SZ_1K, "gpt0")) {
>>  		pr_err("%s:cannot get IO addr\n", __func__);
>> @@ -234,26 +242,32 @@ void __init spear_setup_timer(void)
>>  		goto err_iomap;
>>  	}
>>  
>> -	pll3_clk = clk_get(NULL, "pll3_48m_clk");
>> -	if (!pll3_clk) {
>> -		pr_err("%s:couldn't get PLL3 as parent for gpt\n", __func__);
>> +	/* get the parent clock */
>> +	clk = clk_get(NULL, pclk_name);
>> +
>> +	if (!clk) {
>> +		pr_err("%s:couldn't get %s as parent for gpt\n",
>> +				__func__, pclk_name);
>>  		goto err_iomap;
>>  	}
>>  
>> -	clk_set_parent(gpt_clk, pll3_clk);
>> +	clk_set_parent(gpt_clk, clk);
> ??
> why not do this in the clock file?

The functional clock of the timer very much depends on the parent clock it
selects. This is a part of the timer itself, hence it is kept here.

regards
Shiraz