Re: [PATCH 02/19] omap3630: hwmod: sr: enable for higher ES

[Nishanth Menon <nm@ti.com>]

Vishwanath Sripathy wrote, on 02/19/2011 06:52 PM:
>> -----Original Message-----
>> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
>> owner@vger.kernel.org] On Behalf Of Nishanth Menon
>> Sent: Saturday, February 19, 2011 5:32 PM
>> To: linux-omap
>> Cc: Tony Lindgren; Kevin Hilman; Nishanth Menon
>> Subject: [PATCH 02/19] omap3630: hwmod: sr: enable for higher ES
>>
>> Enable hwmod entries for OMAP3630 for higher ES revisions as
>> well. This is to ensure that SR can be used in all revisions of
>> OMAP3630 as of this posting.
>>
>> Signed-off-by: Nishanth Menon<nm@ti.com>
>> ---
>>   arch/arm/mach-omap2/omap_hwmod_3xxx_data.c |    8 ++++++--
>>   1 files changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
>> b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
>> index ea1f49a..bbcbea6 100644
>> --- a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
>> +++ b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
>> @@ -1318,7 +1318,9 @@ static struct omap_hwmod
>> omap36xx_sr1_hwmod = {
>>   	},
>>   	.slaves		= omap3_sr1_slaves,
>>   	.slaves_cnt	= ARRAY_SIZE(omap3_sr1_slaves),
>> -	.omap_chip	= OMAP_CHIP_INIT(CHIP_IS_OMAP3630ES1),
>> +	.omap_chip	= OMAP_CHIP_INIT(CHIP_IS_OMAP3630ES1	|
>> +					CHIP_IS_OMAP3630ES1_1	|
>> +					CHIP_IS_OMAP3630ES1_2),
> What's the need of this?
> Here is the code snippet from id.c
> -----snip-----
> 	case 0xb891:
> 		/* Handle 36xx devices */
> 		omap_chip.oc |= CHIP_IS_OMAP3630ES1;
>
> 		switch(rev) {
> 		case 0: /* Take care of early samples */
> 			omap_revision = OMAP3630_REV_ES1_0;
> 			break;
> 		case 1:
> 			omap_revision = OMAP3630_REV_ES1_1;
> 			omap_chip.oc |= CHIP_IS_OMAP3630ES1_1;
> 			break;
> 		case 2:
> 		default:
> 			omap_revision =  OMAP3630_REV_ES1_2;
> 			omap_chip.oc |= CHIP_IS_OMAP3630ES1_2;
> 		}
> So it's clear that omap_chip.oc will have CHIP_IS_OMAP3630ES1 anyway on
> all ES versions.

Hmm... thanks for picking this up, but this is inconsistent and 
confusing implementation in id.c. So the way CHIP_IS_OMAP3630ES1 behaves 
is entirely different from CHIP_IS_OMAP3630ES1_1 or 
CHIP_IS_OMAP3630ES1_2 or any of the OMAP3430ES* which will only be set 
for the specific rev of the silicon.

I can post a patch later on to replace CHIP_IS_OMAP3630ES1 with 
CHIP_IS_OMAP3630ES_ALL which allows the code to be readable and make 
CHIP_IS_OMAP3630ES1 mean precisely what it should have meant like the 
rest of the ESs - only for ES1.0.


What do people think of this?

-- 
Regards,
Nishanth Menon