Re: [PATCH 1/1] OMAP3: Common mechanism to identify cpu revision

[Kevin Hilman <khilman@deeprootsystems.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 Premi, Sanjeev
>> Sent: Monday, October 05, 2009 7:18 PM
>> To: Kevin Hilman
>> Cc: linux-omap@vger.kernel.org
>> Subject: RE: [PATCH 1/1] OMAP3: Common mechanism to identify 
>> cpu revision
>> 
>> > -----Original Message-----
>> > From: Kevin Hilman [mailto:khilman@deeprootsystems.com] 
>> > Sent: Wednesday, September 30, 2009 7:32 PM
>> > To: Premi, Sanjeev
>> > Cc: linux-omap@vger.kernel.org
>> > Subject: Re: [PATCH 1/1] OMAP3: Common mechanism to identify 
>> > cpu revision
>> > 
>> > Sanjeev Premi <premi@ti.com> writes:
>> > 
>> > > There are multiple mechanisms to identify the cpu revisions.
>> > > Most common is use of omap_rev(). This, however, does a
>> > > absolute comparison of omap_revision - which includes
>> > > CPU id, CPU rev and CPU class. This comparison fails for
>> > > OMAP35x processors.
>> > >
>> > > This patch defines generic functions that use only the
>> > > CPU rev bits in omap_revision to identify the revision
>> > > information.
>> > >
>> > > Usage will change from (for example):
>> > > 	if (omap_rev() > OMAP3430_REV_ES2_0)
>> > > to:
>> > > 	if (cpu_is_omap34xx() && omap_rev_gt_2_0())
>> > >
>> > > Specific check for cpu_is_xxx() will not be needed for
>> > > files specific to silicon e.g. pm34xx.c, clock34xx.c, etc.
>> > >
>> > > Signed-off-by: Sanjeev Premi <premi@ti.com>
>> > 
>> > Looks mostly good, some minor comments/questions below...
>> > 
>> [snip]--[snip]
>> > 
>> > I don't think the cpu_is_... is needed here because of the OMAP3
>> > specific function.
>> 
>> [sp] Yes. This can be removed.
>> 
>> > >  /*
>> > > + * Silicon revisions
>> > > + */
>> > > +#define OMAP_ES_1_0		0x00
>> > > +#define OMAP_ES_2_0		0x10
>> > > +#define OMAP_ES_2_1		0x20
>> > > +#define OMAP_ES_3_0		0x30
>> > > +#define OMAP_ES_3_1		0x40
>> > 
>> > Hmm, are these the same values on OMAP2? and OMAP4?
>> > 
>> [sp] Based on the values defined for OMAP2420 and OMA2430,
>> these definitions are applicable. There aren't as many
>> Revisions though.
>> 
>> Not sure if it will hold good for OMAP4..
>> 
>> Do you think, we make these definitions silicon specific
>> now?

Not necessarily, for OMAP4, we can use OMAP4_ prefix.  But
you should add a comment that these are valid for OMAP2/3.

>> > > +#define OMAP_REV_MASK		0x0000ff00
>> > > +#define OMAP_REV_BITS		((omap_rev() & 
>> > OMAP_REV_MASK) >> 8)
>> > > +
>> > > +#define OMAP_REV_IS(revid)				
>> 	\
>> > > +static inline u8 omap_rev_is_ ##revid (void)		
>> > 	\
>> > 
>> > Minor nit, but these should return bool.
>> 
>> [sp] I will update.
>
> Kevin, Tony,
>
> The original patch was created against the pm branch.
>
> While re-submitting, should I split in two - one against
> master and delta for pm branch?

Yes.  The main part of this should go into l-o master.

Kevin

> Best regards,
> Sanjeev
>
>> 
>> > 
>> > > +{							
>> 	\
>> > > +	return (OMAP_REV_BITS == OMAP_ES_ ##revid) ? 1 : 0;	\
>> > > +}
>> > > +
>> > > +#define OMAP_REV_LT(revid)				
>> 	\
>> > > +static inline u8 omap_rev_lt_ ##revid (void)		
>> > 	\
>> [snip]--[snip]
>> 
>> > 
>> > Kevin
>> > 
>> > --
>> To unsubscribe from this list: send the line "unsubscribe 
>> linux-omap" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> 
>>