Re: [RFC] Common mechanism to identify Si revision

[Olof Johansson <olof@lixom.net>]

On Thu, Sep 03, 2009 at 04:14:28PM +0530, Premi, Sanjeev wrote:
> Hi,
> 
> Currently there are multiple mechanisms for identifying the si revisions.
> 
> Most places the comparison is against omap_rev() as a whole number. Example:
> 
> arch/arm/mach-omap2/board-3430sdp.c:695:    if (omap_rev() > OMAP3430_REV_ES1_0)
> arch/arm/mach-omap2/board-3430sdp.c:728:    if (omap_rev() > OMAP3430_REV_ES1_0)
> 
> Then, there are custom macros. Example (cpu.h):
> 
> #define CHIP_GE_OMAP3430ES3_1		(CHIP_IS_OMAP3430ES3_1)
> #define CHIP_GE_OMAP3430ES3		(CHIP_IS_OMAP3430ES3_0 | \
> 					 CHIP_GE_OMAP3430ES3_1)
> #define CHIP_GE_OMAP3430ES2		(CHIP_IS_OMAP3430ES2 | \
> 					 CHIP_GE_OMAP3430ES3)
> 
> The problem with comparing against a whole number is that comparison is invalid
> for another processor series. E.g. OMAP3430 and OMAP3517.
> 
> Here, I am proposing a common mechanism to identify the si revision; that focuses
> on the revision bits alone. (See code below)
> 
> The usage would then be (example):
> 
>    if (omap_rev() > OMAP3430_REV_ES1_0)
> 
> To
> 
>    if (cpu_is_omap34xx() && OMAP_REV_GT(OMAP_ES_1_0)

What's the purpose of most of these checks in the first place? I can
see two immediate needs:

1) To check for various errata and do appropriate workarounds

2) To check if the current chip has a certain feature

Both of these could just as well be abstracted away such that you use
tests on the form:

	if (OMAP_HAS_ERRATA_FOO) ...

or:
	if (OMAP_FEATURE_FOO) ...

And then move the actual checking of a feature into the header file
where the errata/feature setups are defined.


There's two major benefits to this:

1) Readability. No need to sit and look up in a manual why there's a
check for version X here.
 (and/or no need to add a specific comment about it).

2) Keeping changes centralized. If there's a new revision or chip,
there's just one header file to update, not 20 different source files.

For example, a bunch of the checks in pm34xx.c would be nicer to have as:

	if (OMAP_HAS_USBHOST()) 

Then the current settings.
-Olof