Re: [RFC] Common mechanism to identify Si revision

[Kevin Hilman <khilman@deeprootsystems.com>]

Olof Johansson <olof@lixom.net> writes:

> 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()) 
>

I tend to agree with Olaf here and am in favor of the new 'features'
patch that Sanjeev has already proposed.

That doesn't mean that I'm opposed to this change in principle, but
would rather see most of the omap_rev() and cpu_is_* checks disappear
in favor of more generic omap_has_feature() checks.

Kevin