Re: [PATCH-V3 2/4] arm:omap:am33xx: Update common OMAP machine specific sources

[Kevin Hilman <khilman@ti.com>]

"Premi, Sanjeev" <premi@ti.com> writes:

>> -----Original Message-----
>> From: linux-omap-owner@vger.kernel.org 
>> [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Hilman, Kevin
>> Sent: Tuesday, September 27, 2011 12:16 AM
>> To: Hiremath, Vaibhav
>> Cc: linux-omap@vger.kernel.org; paul@pwsan.com; 
>> tony@atomide.com; linux-arm-kernel@lists.infradead.org; 
>> Mohammed, Afzal
>> Subject: Re: [PATCH-V3 2/4] arm:omap:am33xx: Update common 
>> OMAP machine specific sources
>> 
>> <hvaibhav@ti.com> writes:
>> 
>> > From: Afzal Mohammed <afzal@ti.com>
>> >
>> > This patch updates the common machine specific source files for
>> > support for AM33XX/AM335x with cpu type, macros for 
>> identification of
>> > AM33XX/AM335X device.
>> >
>> > Signed-off-by: Afzal Mohammed <afzal@ti.com>
>> > Signed-off-by: Vaibhav Hiremath <hvaibhav@ti.com>
>> 
>> [...]
>> 
>> > @@ -3576,7 +3579,8 @@ int __init omap3xxx_clk_init(void)
>> >  	 * Lock DPLL5 -- here only until other device init code can
>> >  	 * handle this
>> >  	 */
>> > -	if (!cpu_is_ti816x() && (omap_rev() >= OMAP3430_REV_ES2_0))
>> > +	if (!cpu_is_ti816x() && !cpu_is_am33xx() &&
>> > +			(omap_rev() >= OMAP3430_REV_ES2_0))
>> >  		omap3_clk_lock_dpll5();
>> 
>> This is getting ugly.  
>> 
>> Instead of continuing to expand this if-list, I think it's time for a
>> new feature-flag for whether or not an SoC has DPLL5 instead.
>
> I agree that the code is really getting ugly here. But, isn't
> feature-flag going to be over-used with this and similar features?
>
> Just thinking ahead, for these possible cases:
> 1) An soc adds DPLL6.
> 2) An soc uses DPLL5, but mechanism to lock is different.

You're right.

> Wouldn't it be better to have a scheme like this:
> 1) Define a simple structure for DPLLs.
> 2) Initialize the unused DPLLs to be null/ -1 early
>    in arch/soc specific init.
> 3) The DPLL functions check for corresponding flag on
>    entry.

Actually, looking at this closer, I think the infrastructure is already
there to handle this cleanly.

Basically, dpll5 should not even be registered for SoCs where it doesn't
exist.  Then, any attempts to use DPLL5 would know it doesn't exist
because the call to clk_get() in omap3_clk_lock_dpll5() would fail.

I think the clock3xxx_data.c needs a bit more cleanup so that only
clocks that exist for a given SoC are registered.

Paul already did a similar cleanup for the powerdomain data files by
creating separate lists for common ones and unique ones.  Looks like we
need the same for the clock data.

Patches welcome.

Kevin