Re: [PATCH v4] OMAP3: introduce OMAP3630

[Tony Lindgren <tony@atomide.com>]

* Nishanth Menon <nm@ti.com> [091009 12:07]:
> Tony Lindgren had written, on 10/09/2009 01:53 PM, the following:
>> * Nishanth Menon <nm@ti.com> [091009 11:47]:
>>> Tony Lindgren had written, on 10/09/2009 01:03 PM, the following:
>>>> * Aguirre Rodriguez, Sergio Alberto <saaguirre@ti.com> [091009 08:59]:
>>>>>  
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Menon, Nishanth Sent: Friday, October 09, 2009 10:56 AM
>>>>>> To: linux-omap
>>>>>> Cc: Menon, Nishanth; Chikkature Rajashekar, Madhusudhan; 
>>>>>> Pandita, Vikram; Pais, Allen; Gadiyar, Anand; Cousson, Benoit; 
>>>>>> Felipe Balbi; Kevin Hilman; Premi, Sanjeev; Shilimkar, Santosh; 
>>>>>> Aguirre Rodriguez, Sergio Alberto; Tony Lindgren
>>>>>> Subject: [PATCH v4] OMAP3: introduce OMAP3630
>>>>>>
>>>>>> **--- SNIP ME --
>>>>>> Hi,
>>>>>> A bit of history for this patchset
>>>>>> V4 - Sergio's improvement to the handling of rev decision as we just
>>>>>>      change the define to 3630 and remove crapy override logic in
>>>>>>      omap3_check_revision from v3 patch
>>>>> Now looks much better ;)
>>>>>
>>>>>> V3 - Fixes from Sergio's comments + boot tested on SDP3430+3630.
>>>>>> V2 - fixes of generic comments from Felipe Balbi+minor cleanups
>>>>>> V1 - inital implementation of (a) approach
>>>>>> V0 - original approach introducing a new silicon family
>>>>>> **--- END OF SNIP ME --
>>>>>> Device intro:
>>>>>> OMAP3630 is the latest in the family of OMAP3 devices
>>>>>> and among the changes it introduces are:
>>>>>>
>>>>>> New OPP levels for new voltage and frequency levels. a bunch of
>>>>>> Bug fixes to various modules feature additions, notably with ISP,
>>>>>> sDMA etc.
>>>>>>
>>>>>> Details about the chip is available here:
>>>>>> http://focus.ti.com/general/docs/wtbu/wtbuproductcontent.tsp?t
>>>>>> emplateId=6123&navigationId=12836&contentId=52606
>>>>>>
>>>>>> Strategy used:
>>>>>> Strategy to introduce this device into Linux was discussed here:
>>>>>> Ref: http://marc.info/?t=125343303400003&r=1&w=2
>>>>>>
>>>>>> Two approaches were available:
>>>>>> a) Consider 3630 generation of devices as a new family of silicon
>>>>>> b) Consider 3630 as an offshoot of 3430 family of devices
>>>>>>
>>>>>> As a common consensus, (b) seems to be more valid for 3630 as:
>>>>>> * There are changes which are easily handled by using "FEATURES"
>>>>>>   infrastructure.
>>>>>>   For details how to do this, see thread:
>>>>>>   http://marc.info/?t=125050998500001&r=1&w=2
>>>>>> * Most of existing 34xx infrastructure can be reused(almost 90%+)
>>>>>> 	- so no ugly if (cpu_is_omap34xx() || cpu_is_omap36xx())
>>>>>> 	  all over the place
>>>>>> 	- lesser chance of bugs due to reuse of proven code flow
>>>>>> 	- 36xx specific handling can still be done where required
>>>>>> 	  within the existing infrastructure
>>>>>>
>>>>>> NOTE:
>>>>>> * If additional 34xx series are added, OMAP3430_REV_ESXXXX can be
>>>>>>   added on top of the existing 3630 ones are renumbered
>>>>>>
>>>>>> This patch was tested on SDP3430, boot tested on 3630 platform using
>>>>>> 3430sdp defconfig
>>>>>>
>>>>>> Signed-off-by: Madhusudhan Chikkature Rajashekar <madhu.cr@ti.com>
>>>>>> Signed-off-by: Nishanth Menon <nm@ti.com>
>>>>>> Signed-off-by: Vikram Pandita <vikram.pandita@ti.com>
>>>>>> Cc: Allen Pais <allen.pais@ti.com>
>>>>>> Cc: Anand Gadiyar <gadiyar@ti.com>
>>>>>> Cc: Benoit Cousson <b-cousson@ti.com>
>>>>>> Cc: Felipe Balbi <felipe.balbi@nokia.com>
>>>>>> Cc: Kevin Hilman <khilman@deeprootsystems.com>
>>>>>> Cc: Sanjeev Premi <premi@ti.com>
>>>>>> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
>>>>>> Cc: Sergio Alberto Aguirre Rodriguez <saaguirre@ti.com>
>>>>>> Cc: Tony Lindgren <tony@atomide.com>
>>>>> Acked-by: Sergio Aguirre <saaguirre@ti.com>
>>>>>
>>>>>> ---
>>>>>>  arch/arm/mach-omap2/id.c              |   24 ++++++++++++++++++++++--
>>>>>>  arch/arm/plat-omap/include/mach/cpu.h |    6 ++++++
>>>>>>  2 files changed, 28 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/arm/mach-omap2/id.c b/arch/arm/mach-omap2/id.c
>>>>>> index 03b80f2..ee3bb69 100644
>>>>>> --- a/arch/arm/mach-omap2/id.c
>>>>>> +++ b/arch/arm/mach-omap2/id.c
>>>>>> @@ -210,7 +210,9 @@ void __init omap3_check_revision(void)
>>>>>>  	hawkeye = (idcode >> 12) & 0xffff;
>>>>>>  	rev = (idcode >> 28) & 0xff;
>>>>>>  -	if (hawkeye == 0xb7ae) {
>>>>>> +	switch (hawkeye) {
>>>>>> +	case 0xb7ae:
>>>>>> +		/* Handle 34xx devices */
>>>>>>  		switch (rev) {
>>>>>>  		case 0:
>>>>>>  			omap_revision = OMAP3430_REV_ES2_0;
>>>>>> @@ -231,8 +233,26 @@ void __init omap3_check_revision(void)
>>>>>>  		default:
>>>>>>  			/* Use the latest known revision as default */
>>>>>>  			omap_revision = OMAP3430_REV_ES3_1;
>>>>>> -			rev_name = "Unknown revision\n";
>>>>>> +			rev_name = "Unknown 34xx revision\n";
>>>>>>  		}
>>>>>> +		break;
>>>>>> +	case 0xb891:
>>>>>> +		/* Handle 36xx devices */
>>>>>> +		switch (rev) {
>>>>>> +		case 0:
>>>>>> +			omap_revision = OMAP3630_REV_ES1_0;
>>>>>> +			rev_name = "ES1.0";
>>>>>> +			break;
>>>>>> +		default:
>>>>>> +			/* Use the latest known revision as default */
>>>>>> +			omap_revision = OMAP3630_REV_ES1_0;
>>>>>> +			rev_name = "Unknown 36xx revision\n";
>>>>>> +		}
>>>>>> +		break;
>>>>>> +	default:
>>>>>> +		/* Unknown default to latest rev as default*/
>>>>>> +		omap_revision = OMAP3630_REV_ES1_0;
>>>>>> +		rev_name = "Unknown revision\n";
>>>>>>  	}
>>>>>>   out:
>>>>>> diff --git a/arch/arm/plat-omap/include/mach/cpu.h   
>>>>>> b/arch/arm/plat-omap/include/mach/cpu.h
>>>>>> index 431fec4..65f1882 100644
>>>>>> --- a/arch/arm/plat-omap/include/mach/cpu.h
>>>>>> +++ b/arch/arm/plat-omap/include/mach/cpu.h
>>>>>> @@ -383,6 +383,12 @@ IS_OMAP_TYPE(3430, 0x3430)
>>>>>>  #define OMAP3430_REV_ES2_1	0x34302034
>>>>>>  #define OMAP3430_REV_ES3_0	0x34303034
>>>>>>  #define OMAP3430_REV_ES3_1	0x34304034
>>>>>> +/* NOTE: Add 36xx series below
>>>>>> + * If additional 34xx series are added, OMAP3430_REV_ESXXXX can be
>>>>>> + * added above the 3630 defines and series renumbered to ensure
>>>>>> + * rev() > checks to work
>>>>>> + */
>>>>>> +#define OMAP3630_REV_ES1_0	0x36301034
>>>> Hmm, to me it looks that this breaks cpu_is_omap34xx() for 36xx, right?
>>>>
>>>> How about build a kernel that boots both on 3430sdp and on some
>>>> 36xx board and make sure things get detected properly by adding
>>>> some debug printk statements?
>>> Tested with the following patch:
>>> diff --git a/arch/arm/mach-omap2/board-3430sdp.c   
>>> b/arch/arm/mach-omap2/board-3430sdp.c
>>> index 81aabac..2aac26d 100644
>>> --- a/arch/arm/mach-omap2/board-3430sdp.c
>>> +++ b/arch/arm/mach-omap2/board-3430sdp.c
>>> @@ -498,6 +498,8 @@ static struct ehci_hcd_omap_platform_data 
>>> ehci_pdata  __initconst = {
>>>
>>>  static void __init omap_3430sdp_init(void)
>>>  {
>>> +       if (cpu_is_omap34xx())
>>> +               printk(KERN_ERR "_______________------ 34xx detected\n");
>>>         omap3430_i2c_init();
>>>         platform_add_devices(sdp3430_devices, ARRAY_SIZE(sdp3430_devices));
>>>         if (omap_rev() > OMAP3430_REV_ES1_0)
>>>
>>>
>>> and yes, it prints on both 3430 and 3630 platforms (i am hacking at 
>>> the  moment by using 3430sdpdefconfig for both platforms).
>>>
>>> Here is why it works:
>>> arch/arm/plat-omap/include/mach/cpu.h
>>> cpu_is_omap343x() depends on is_omap34xx() depends on  GET_OMAP_CLASS 
>>>  depends on lower 8 bits of value returned by omap_rev()
>>> omap_rev()(arch/arm/mach-omap2/id.c) returns omap_revision. this 
>>> means  omap_rev() will return 0x36301034 -> so class is 0x34.. 
>>> causing  cpu_is_omap343x to be true on 3630 also.
>>
>> Ah, that's right, sorry for being confused. I totally forgot we're using
>> the lower bits for the class.. I guess I need to start reading the code
>> more :)
>>
>>> we might want to consider a follow on patch replacing 0x34 with 0x30 
>>> and  changing cpu_is_omap343x with cpu_is_omap3xxx -> at a later 
>>> point ofcourse.
>>
>> Yeah if necessary. Let's not do it right now though, we already have
>> quite a pile of clean-up patches coming for the next merge window.
>
> so do I get an ack on v4 of the patch? ;)

Yeah, will merge today.

Tony