[RFC] Common mechanism to identify Si revision

[RFC] Common mechanism to identify Si revision

[Premi, Sanjeev]

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)


Though I have not looked at the OMAP1 directory, I am assuming similar case there.
Since there aren't any new devices in the OMAP1 category, this may not appear as
a need. But consistency could help.

Few items that I feel need to be looked at:
- Possible differences in the MASK defined for Si revision.
  Irrespective of (possible) difference in the HW, the bits in 'omap_revision' can
  be same.

- How to phase in the change - one shot OR in steps.

I am not sending this as patch, for now; just to get the discussion started.

Best regards,
Sanjeev


(cpu.h)

/*
 * Identify silicon revision.
 */
#define OMAP_REV_MASK		0x0000ff00

#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

#define OMAP_REV_IS(revid)						\
static inline u8 omap_rev_is_ ##revid (void)				\
{									\
	(((omap_rev() & OMAP_REV_MASK) >> 8) == revid) ? 1 : 0 ;	\
}

#define OMAP_REV_LT(revid)						\
static inline u8 omap_rev_lt_ ##revid (void)				\
{									\
	(((omap_rev() & OMAP_REV_MASK) >> 8) < revid) ? 1 : 0 ;	\
}

#define OMAP_REV_LE(revid)						\
static inline u8 omap_rev_le_ ##revid (void)				\
{									\
	(((omap_rev() & OMAP_REV_MASK) >> 8) <= revid) ? 1 : 0 ;	\
}

#define OMAP_REV_GE(revid)						\
static inline u8 omap_rev_ge_ ##revid (void)				\
{									\
	(((omap_rev() & OMAP_REV_MASK) >> 8) >= revid) ? 1 : 0 ;	\
}

#define OMAP_REV_GT(revid)						\
static inline u8 omap_rev_gt_ ##revid (void)				\
{									\
	(((omap_rev() & OMAP_REV_MASK) >> 8) > revid) ? 1 : 0 ;	\
}

OMAP_REV_IS(OMAP_ES_1_0)
OMAP_REV_LT(OMAP_ES_1_0)
OMAP_REV_LE(OMAP_ES_1_0)
OMAP_REV_GT(OMAP_ES_1_0)
OMAP_REV_GE(OMAP_ES_1_0)

OMAP_REV_IS(OMAP_ES_2_0)
OMAP_REV_LT(OMAP_ES_2_0)
OMAP_REV_LE(OMAP_ES_2_0)
OMAP_REV_GT(OMAP_ES_2_0)
OMAP_REV_GE(OMAP_ES_2_0)

OMAP_REV_IS(OMAP_ES_2_1)
OMAP_REV_LT(OMAP_ES_2_1)
OMAP_REV_LE(OMAP_ES_2_1)
OMAP_REV_GT(OMAP_ES_2_1)
OMAP_REV_GE(OMAP_ES_2_1)

OMAP_REV_IS(OMAP_ES_3_0)
OMAP_REV_LT(OMAP_ES_3_0)
OMAP_REV_LE(OMAP_ES_3_0)
OMAP_REV_GT(OMAP_ES_3_0)
OMAP_REV_GE(OMAP_ES_3_0)

OMAP_REV_IS(OMAP_ES_3_1)
OMAP_REV_LT(OMAP_ES_3_1)
OMAP_REV_LE(OMAP_ES_3_1)
OMAP_REV_GT(OMAP_ES_3_1)
OMAP_REV_GE(OMAP_ES_3_1)

RE: [RFC] Common mechanism to identify Si revision

[Premi, Sanjeev]

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org 
> [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Premi, Sanjeev
> Sent: Thursday, September 03, 2009 4:14 PM
> To: linux-omap@vger.kernel.org
> Subject: [RFC] Common mechanism to identify Si revision
> 
> 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)
> 
> 
> Though I have not looked at the OMAP1 directory, I am 
> assuming similar case there.
> Since there aren't any new devices in the OMAP1 category, 
> this may not appear as
> a need. But consistency could help.
> 
> Few items that I feel need to be looked at:
> - Possible differences in the MASK defined for Si revision.
>   Irrespective of (possible) difference in the HW, the bits 
> in 'omap_revision' can
>   be same.
> 
> - How to phase in the change - one shot OR in steps.
> 
> I am not sending this as patch, for now; just to get the 
> discussion started.
> 
> Best regards,
> Sanjeev
> 

[sp] sorry, missed the return statement in the inline functions below:
     ... perils of writing code in mail :)

~sanjeev

> 
> (cpu.h)
> 
> /*
>  * Identify silicon revision.
>  */
> #define OMAP_REV_MASK		0x0000ff00
> 
> #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
> 
> #define OMAP_REV_IS(revid)					
> 	\
> static inline u8 omap_rev_is_ ##revid (void)			
> 	\
> {								
> 	\
> 	(((omap_rev() & OMAP_REV_MASK) >> 8) == revid) ? 1 : 0 
> ;	\
> }
> 
> #define OMAP_REV_LT(revid)					
> 	\
> static inline u8 omap_rev_lt_ ##revid (void)			
> 	\
> {								
> 	\
> 	(((omap_rev() & OMAP_REV_MASK) >> 8) < revid) ? 1 : 0 ;	\
> }
> 
> #define OMAP_REV_LE(revid)					
> 	\
> static inline u8 omap_rev_le_ ##revid (void)			
> 	\
> {								
> 	\
> 	(((omap_rev() & OMAP_REV_MASK) >> 8) <= revid) ? 1 : 0 
> ;	\
> }
> 
> #define OMAP_REV_GE(revid)					
> 	\
> static inline u8 omap_rev_ge_ ##revid (void)			
> 	\
> {								
> 	\
> 	(((omap_rev() & OMAP_REV_MASK) >> 8) >= revid) ? 1 : 0 
> ;	\
> }
> 
> #define OMAP_REV_GT(revid)					
> 	\
> static inline u8 omap_rev_gt_ ##revid (void)			
> 	\
> {								
> 	\
> 	(((omap_rev() & OMAP_REV_MASK) >> 8) > revid) ? 1 : 0 ;	\
> }
> 
> OMAP_REV_IS(OMAP_ES_1_0)
> OMAP_REV_LT(OMAP_ES_1_0)
> OMAP_REV_LE(OMAP_ES_1_0)
> OMAP_REV_GT(OMAP_ES_1_0)
> OMAP_REV_GE(OMAP_ES_1_0)
> 
> OMAP_REV_IS(OMAP_ES_2_0)
> OMAP_REV_LT(OMAP_ES_2_0)
> OMAP_REV_LE(OMAP_ES_2_0)
> OMAP_REV_GT(OMAP_ES_2_0)
> OMAP_REV_GE(OMAP_ES_2_0)
> 
> OMAP_REV_IS(OMAP_ES_2_1)
> OMAP_REV_LT(OMAP_ES_2_1)
> OMAP_REV_LE(OMAP_ES_2_1)
> OMAP_REV_GT(OMAP_ES_2_1)
> OMAP_REV_GE(OMAP_ES_2_1)
> 
> OMAP_REV_IS(OMAP_ES_3_0)
> OMAP_REV_LT(OMAP_ES_3_0)
> OMAP_REV_LE(OMAP_ES_3_0)
> OMAP_REV_GT(OMAP_ES_3_0)
> OMAP_REV_GE(OMAP_ES_3_0)
> 
> OMAP_REV_IS(OMAP_ES_3_1)
> OMAP_REV_LT(OMAP_ES_3_1)
> OMAP_REV_LE(OMAP_ES_3_1)
> OMAP_REV_GT(OMAP_ES_3_1)
> OMAP_REV_GE(OMAP_ES_3_1)
> --
> 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
> 
> 

Re: [RFC] Common mechanism to identify Si revision

[Olof Johansson]

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

RE: [RFC] Common mechanism to identify Si revision

[Premi, Sanjeev]

> -----Original Message-----
> From: Olof Johansson [mailto:olof@lixom.net] 
> Sent: Sunday, September 06, 2009 6:17 PM
> To: Premi, Sanjeev
> Cc: linux-omap@vger.kernel.org
> Subject: Re: [RFC] Common mechanism to identify Si revision
> 
> 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

[sp] I believe the only need would be to make easy check if the version
     has multiple errata fixes and/or enhancements. And, of course, a
     verbose information to user who may not have (and may not need)
     information of all the errata/enhancements.

     Today, there are multiple ways of doing the same thing... Each way
     builds upon minor issues in the existing one; but adds its own.

> 
> 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.

[sp I have submitted a patch that takes the first step toward this:
    http://marc.info/?l=linux-omap&m=125050987112798&w=2
    ...still waiting to hear from Tony on this.

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

[sp] Can you look and comment on this discussion as well:
     http://marc.info/?l=linux-omap&m=125017671720718&w=2

Best regards,
Sanjeev
> 
> Then the current settings.
> -Olof
> 
> 

Re: [RFC] Common mechanism to identify Si revision

[Olof Johansson]

Hi,

On Mon, Sep 07, 2009 at 11:33:32AM +0530, Premi, Sanjeev wrote:

> > 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
> 
> [sp] I believe the only need would be to make easy check if the version
>      has multiple errata fixes and/or enhancements. And, of course, a
>      verbose information to user who may not have (and may not need)
>      information of all the errata/enhancements.
> 
>      Today, there are multiple ways of doing the same thing... Each way
>      builds upon minor issues in the existing one; but adds its own.

Yeah, ok. I was just fearing that this added ES-level checking would be
used to clutter up the code with extra testing instead of hiding it away in
header files. But it seems like most of it is covered already.


> > 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.
> 
> [sp I have submitted a patch that takes the first step toward this:
>     http://marc.info/?l=linux-omap&m=125050987112798&w=2
>     ...still waiting to hear from Tony on this.

Looks good. I didn't subscribe to linux-omap until recently so I don't
have the original emails (and thus can't comment on them), but my only
feedback is that it really doesn't belong in /proc/cpuinfo whether a
SoC has SGX on-chip or not. 

It'd be better to move that information somewhere else. Keep in mind
that adding information to a /proc file means that you are changing an
API that we would need to live with forever.

> > 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()) 
> 
> [sp] Can you look and comment on this discussion as well:
>      http://marc.info/?l=linux-omap&m=125017671720718&w=2

I think it is a great step in the right direction. My only concern is
that the usage conventions are confusing:

OMAP3_HAS_FEATURE(neon, NEON)

Just seeing that, what is the difference between neon and NEON? Also,
it always requires two consecutive calls to check for a feature (get
the flags, then check the mask).

I would personally prefer to keep a global that is initialized early
in boot (called __omap_features or so -- the __-prefix would indicate
that it is not for direct use in code), then have OMAP3_HAS_FEATURE()
just look at that value instead of having it passed in. It's not like
it changes during runtime anyway.

It can be debated if the format should then be OMAP3_HAS_FEATURE_SGX or
OMAP3_HAS_FEATURE(SGX), but either is fine with me.


-Olof

RE: [RFC] Common mechanism to identify Si revision

[Premi, Sanjeev]

> -----Original Message-----
> From: Olof Johansson [mailto:olof@lixom.net] 
> Sent: Monday, September 07, 2009 6:04 PM
> To: Premi, Sanjeev
> Cc: linux-omap@vger.kernel.org
> Subject: Re: [RFC] Common mechanism to identify Si revision
> 
> Hi,
> 
> On Mon, Sep 07, 2009 at 11:33:32AM +0530, Premi, Sanjeev wrote:
> 
> > > 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
> > 
> > [sp] I believe the only need would be to make easy check if 
> the version
> >      has multiple errata fixes and/or enhancements. And, of 
> course, a
> >      verbose information to user who may not have (and may not need)
> >      information of all the errata/enhancements.
> > 
> >      Today, there are multiple ways of doing the same 
> thing... Each way
> >      builds upon minor issues in the existing one; but adds its own.
> 
> Yeah, ok. I was just fearing that this added ES-level 
> checking would be
> used to clutter up the code with extra testing instead of 
> hiding it away in
> header files. But it seems like most of it is covered already.
> 
> 
> > > 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.
> > 
> > [sp I have submitted a patch that takes the first step toward this:
> >     http://marc.info/?l=linux-omap&m=125050987112798&w=2
> >     ...still waiting to hear from Tony on this.
> 
> Looks good. I didn't subscribe to linux-omap until recently so I don't
> have the original emails (and thus can't comment on them), but my only
> feedback is that it really doesn't belong in /proc/cpuinfo whether a
> SoC has SGX on-chip or not. 
> 
> It'd be better to move that information somewhere else. Keep in mind
> that adding information to a /proc file means that you are changing an
> API that we would need to live with forever.
> 
> > > 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()) 
> > 
> > [sp] Can you look and comment on this discussion as well:
> >      http://marc.info/?l=linux-omap&m=125017671720718&w=2
> 
> I think it is a great step in the right direction. My only concern is
> that the usage conventions are confusing:
> 
> OMAP3_HAS_FEATURE(neon, NEON)

[sp] This is only used to declare a function that would translate to:
     unsigned int omap3_has_neon(void)
     {
	     return (omap_features & OMAP_HAS_NEON);
     }
     EXPORT_SYMBOL(omap3_has_neon);

     A user would always do something like:
     if (omap_has_neon())
         do_neon_specific_stuff();

Best regards,
Sanjeev
> 
> Just seeing that, what is the difference between neon and NEON? Also,
> it always requires two consecutive calls to check for a feature (get
> the flags, then check the mask).
> 
> I would personally prefer to keep a global that is initialized early
> in boot (called __omap_features or so -- the __-prefix would indicate
> that it is not for direct use in code), then have OMAP3_HAS_FEATURE()
> just look at that value instead of having it passed in. It's not like
> it changes during runtime anyway.
> 
> It can be debated if the format should then be 
> OMAP3_HAS_FEATURE_SGX or
> OMAP3_HAS_FEATURE(SGX), but either is fine with me.
> 
> 
> -Olof
> 
> 

Re: [RFC] Common mechanism to identify Si revision

[Olof Johansson]

On Sep 7, 2009, at 8:02 AM, Premi, Sanjeev wrote:


>>
>> I think it is a great step in the right direction. My only concern is
>> that the usage conventions are confusing:
>>
>> OMAP3_HAS_FEATURE(neon, NEON)
>
> [sp] This is only used to declare a function that would translate to:
>     unsigned int omap3_has_neon(void)
>     {
> 	     return (omap_features & OMAP_HAS_NEON);
>     }
>     EXPORT_SYMBOL(omap3_has_neon);
>
>     A user would always do something like:
>     if (omap_has_neon())
>         do_neon_specific_stuff();


Ok, all is well then. :)


-Olof

Re: [RFC] Common mechanism to identify Si revision

[Kevin Hilman]

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

RE: [RFC] Common mechanism to identify Si revision

[Cousson, Benoit]

Hi Kevin,

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of Kevin Hilman
> Sent: Thursday, September 10, 2009 7:55 PM
> To: Olof Johansson
> Cc: Premi, Sanjeev; linux-omap@vger.kernel.org
> Subject: Re: [RFC] Common mechanism to identify Si revision
> 
> 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.

I agree as well, most of the differences between several ES and OMAP variant are related to changes in IPs and not really to the chip version.

> 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.

In that case taking into account the feature version might be useful in order to get rid of most of the omap_rev(). Most of the time new ES are due to new feature or bug fix in an IP that will increase the IP version. 

Regards,
Benoit

RE: [RFC] Common mechanism to identify Si revision

[Premi, Sanjeev]

> -----Original Message-----
> From: Kevin Hilman [mailto:khilman@deeprootsystems.com] 
> Sent: Thursday, September 10, 2009 11:25 PM
> To: Olof Johansson
> Cc: Premi, Sanjeev; linux-omap@vger.kernel.org
> Subject: Re: [RFC] Common mechanism to identify Si revision
> 

 <snip>--<snip>

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

Tony,

I am planning to submit another patch for "speed - enhanced" part for
OMAP3530.

I see enhanced speed as a feature; and would like to build upon the
last submission. BTW, there is no change in ES revision for new part.
Just additional set of control bits in TAP area.

Do you see any issues with the patch:
  [1] http://marc.info/?l=linux-omap&m=125050987112798&w=2

Best regards,
Sanjeev

Re: [RFC] Common mechanism to identify Si revision

[Paul Walmsley]

Hello Sanjeev,

On Thu, 3 Sep 2009, Premi, Sanjeev wrote:

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

For use in structures like clock34xx.h and powerdomains34xx.h, would you 
propose that we use two different fields then - one for OMAP type, the 
other for revision restrictions?  Or a different scheme?


- Paul

RE: [RFC] Common mechanism to identify Si revision

[Premi, Sanjeev]

> -----Original Message-----
> From: Paul Walmsley [mailto:paul@pwsan.com] 
> Sent: Thursday, September 10, 2009 2:59 PM
> To: Premi, Sanjeev
> Cc: linux-omap@vger.kernel.org
> Subject: Re: [RFC] Common mechanism to identify Si revision
> 
> Hello Sanjeev,
> 
> On Thu, 3 Sep 2009, Premi, Sanjeev wrote:
> 
> > 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)
> 
> For use in structures like clock34xx.h and 
> powerdomains34xx.h, would you 
> propose that we use two different fields then - one for OMAP 
> type, the 
> other for revision restrictions?  Or a different scheme?
> 
> - Paul
> 

Paul,

First answer:
We could use two fields e.g. omap_type and omap_rev.
For example:

	.omap_type	  = CHIP_IS_OMAP3430,
      .omap_rev     = OMAP_ES_2_1,

Instead of :

	.omap_chip	  = OMAP_CHIP_INIT(CHIP_IS_OMAP3430),

	.omap_chip	  = OMAP_CHIP_INIT(CHIP_IS_OMAP3430ES1 |
					   CHIP_IS_OMAP3430ES2 |
					   CHIP_IS_OMAP3430ES3_0),

	.omap_chip	  = OMAP_CHIP_INIT(CHIP_GE_OMAP3430ES3_1),

Second answer:
>From the snippets above, it appears that we need to:
1) identify which chip is in use
2) which si rev (if any) does the clock/powerdomain definition
   applies to. Actually, we are checking for:
   a) Does definition depend upon a si rev
   b) if so, which rev? - usually LE and GE should be sufficient.

Which chip in use? - can either be a run-time check OR mapped
to an boot-time setting of "valid" field for the si being used.
e.g. (translating the snippet above):

	.valid	  = (cpu_is_omap34xx())

	.valid	  = (cpu_is_omap34xx() &&
				(OMAP_REV_EQ(OMAP_ES_1_0) ||
				 OMAP_REV_EQ(OMAP_ES_2_1) ||
				 OMAP_REV_EQ(OMAP_ES_3_0)))

	.valid	  = (cpu_is_omap34xx() &&
				(OMAP_REV_GE(OMAP_ES_3_1))

Now decision at runtime can be made based on the just this flag.

I haven't really done any prototyping on this, so there may be
holes in explanation above; but I hope general idea is coming
clear.

Best regards,
Sanjeev

RE: [RFC] Common mechanism to identify Si revision

[Paul Walmsley]

Hello Sanjeev,

another question.  I have heard that on OMAP4, some chip versions will not 
be identified with an ES revision number.  How to handle those?


- Paul

RE: [RFC] Common mechanism to identify Si revision

[Premi, Sanjeev]

> -----Original Message-----
> From: Paul Walmsley [mailto:paul@pwsan.com] 
> Sent: Saturday, September 19, 2009 8:12 PM
> To: Premi, Sanjeev
> Cc: linux-omap@vger.kernel.org
> Subject: RE: [RFC] Common mechanism to identify Si revision
> 
> 
> Hello Sanjeev,
> 
> another question.  I have heard that on OMAP4, some chip 
> versions will not 
> be identified with an ES revision number.  How to handle those?

I am not aware of OMAP4 plans. I believe there *has* to be a mechanism - though possibly not as simple as reading specific bit pattern to makeout differences. This can be used to update a "soft ES" version in the omap_revision.

~sanjeev

> 
> 
> - Paul
> 
>