[PATCH 1/3] ARM: OMAP2+: 32k-counter: Use hwmod lookup to check presence of 32k timer

[Hiremath, Vaibhav]

On Mon, Mar 12, 2012 at 15:47:58, Balbi, Felipe wrote:
> Hi,
> 
> On Mon, Mar 12, 2012 at 09:48:20AM +0000, Hiremath, Vaibhav wrote:
> > On Mon, Mar 12, 2012 at 15:09:24, Balbi, Felipe wrote:
> > > On Fri, Mar 09, 2012 at 05:58:00PM +0000, Hiremath, Vaibhav wrote:
> > > > On Tue, Mar 06, 2012 at 04:25:30, Tony Lindgren wrote:
> > > > > Hi,
> > > > > 
> > > > > * Vaibhav Hiremath <hvaibhav at ti.com> [120119 06:01]:
> > > > > > OMAP device has 32k-sync timer which is currently used as a
> > > > > > clocksource in the kernel (omap2plus_defconfig).
> > > > > > The current implementation uses compile time selection between
> > > > > > gp-timer and 32k-sync timer, which breaks multi-omap build for
> > > > > > the devices like AM33xx, where 32k-sync timer is not available.
> > > > > > 
> > > > > > So use hwmod database lookup mechanism, through which at run-time
> > > > > > we can identify availability of 32k-sync timer on the device,
> > > > > > else fall back to gp-timer.
> > > > > ...
> > > > > 
> > > > > > --- a/arch/arm/plat-omap/counter_32k.c
> > > > > > +++ b/arch/arm/plat-omap/counter_32k.c
> > > > > > @@ -69,52 +69,55 @@ void read_persistent_clock(struct timespec *ts)
> > > > > >  
> > > > > >  int __init omap_init_clocksource_32k(void)
> > > > > >  {
> > > > > > -	static char err[] __initdata = KERN_ERR
> > > > > > -			"%s: can't register clocksource!\n";
> > > > > > -
> > > > > > -	if (cpu_is_omap16xx() || cpu_class_is_omap2()) {
> > > > > > -		u32 pbase;
> > > > > > -		unsigned long size = SZ_4K;
> > > > > > -		void __iomem *base;
> > > > > > -		struct clk *sync_32k_ick;
> > > > > > -
> > > > > > -		if (cpu_is_omap16xx()) {
> > > > > > -			pbase = OMAP16XX_TIMER_32K_SYNCHRONIZED;
> > > > > > -			size = SZ_1K;
> > > > > > -		} else if (cpu_is_omap2420())
> > > > > > -			pbase = OMAP2420_32KSYNCT_BASE + 0x10;
> > > > > > -		else if (cpu_is_omap2430())
> > > > > > -			pbase = OMAP2430_32KSYNCT_BASE + 0x10;
> > > > > > -		else if (cpu_is_omap34xx())
> > > > > > -			pbase = OMAP3430_32KSYNCT_BASE + 0x10;
> > > > > > -		else if (cpu_is_omap44xx())
> > > > > > -			pbase = OMAP4430_32KSYNCT_BASE + 0x10;
> > > > > > -		else
> > > > > > +	u32 pbase;
> > > > > > +	unsigned long size = SZ_4K;
> > > > > > +	void __iomem *base;
> > > > > > +	struct clk *sync_32k_ick;
> > > > > > +
> > > > > > +	if (cpu_is_omap16xx()) {
> > > > > > +		pbase = OMAP16XX_TIMER_32K_SYNCHRONIZED;
> > > > > > +		size = SZ_1K;
> > > > > > +	} else if (cpu_class_is_omap2()) {
> > > > > > +		struct omap_hwmod *oh;
> > > > > > +		const char *oh_name = "counter_32k";
> > > > > > +
> > > > > > +		oh = omap_hwmod_lookup(oh_name);
> > > > > > +		if (!oh || oh->slaves_cnt == 0) {
> > > > > > +			pr_err("Could not lookup %s hwmod\n", oh_name);
> > > > > >  			return -ENODEV;
> > > > > > +		}
> > > > > > +		pbase = oh->slaves[0]->addr->pa_start + 0x10;
> > > > > > +	} else {
> > > > > > +		return -ENODEV;
> > > > > > +	}
> > > > > 
> > > > > How about have separate omap1 and omap2+ init functions that
> > > > > call a common function and passes the pbase as a parameter?
> > > > > 
> > > > > That way we can get rid of the cpu_is_omapxxxx tests here.
> > > > > 
> > > > 
> > > > Tony,
> > > > 
> > > > In the morning, I replied very soon, without much thinking...
> > > > 
> > > > Just now I started working on the patch, I was just reviewing the code, 
> > > > and I felt that, it is unnecessary to split the code between omap1 and 
> > > > omap2+.
> > > > 
> > > > The reason is,
> > > > 
> > > > Currently Only OMAP16xx base-address is hardcoded with
> > > > cpu_is_omap16xx() macro, For all other omap family of devices the
> > > > complete information is fetched from HWDMO api's/data.
> > > 
> > > In that case, why don't you create the platform_device by hand on
> > > arch/arm/mach-omap1/devices.c and move the omap2+ (which is based on
> > > hwmod) to arch/arm/mach-omap2/devices.c ?
> > > 
> > 
> > Balbi,
> > 
> > 32k_counter code is not a platform_device/driver, we don't build the 
> > any device here. We only need hwmod data to fetch the basic information like, 
> > base-address, size, etc...
> > 
> > And I am note sure whether we really intend to make it 
> > platform_device/driver thing.
> 
> that's kinda weird... 

Balbi,

Its not that weird, I think its because, this code is used as system 
clocksource.

> anyway, you can also pass base as argument to
> omap_init_clocksource_32k()... dunno.
> 

That's the whole point, just for one platform (omap16xx), we are changing the code. Rest all other platforms are using hwmod data, which is I believe 
is clean interface as of now.

To be more specific, we have to choose between something like -

"arch/arm/mach-omap1/timer32k.c"
bool __init omap_32k_timer_init(void) {
	omap_init_clocksource_32k(OMAP16XX_TIMER_32K_SYNCHRONIZED, SZ_1K);
	...
}
"arch/arm/mach-omap2/timer.c"

static void __init omap2_gp_clocksource_init(int gptimer_id,
                                                 const char *fck_source)
{
	/* Parse the hwmod data here*/
	Base = oh->slaves[0]->addr->pa_start + 0x10;
	Size = oh->slaves[0]->addr->pa_end - oh->slaves[0]->addr->pa_start;
	res = omap_init_clocksource_32k(base, size);
	if (!res)
		return;

	/* Fall back to gptimer */
}


OR

My current patch, which actually add cpu_is_16xx() check at one place and
does everything.


Thanks,
Vaibhav
> -- 
> balbi
>