Re: [PATCH 3/6] OMAP3: Add runtime check for OMAP35x

[Tony Lindgren <tony@atomide.com>]

* Premi, Sanjeev <premi@ti.com> [090806 14:34]:
>  
> > -----Original Message-----
> > From: Tony Lindgren [mailto:tony@atomide.com] 
> > Sent: Thursday, August 06, 2009 4:34 PM
> > To: Premi, Sanjeev
> > Cc: linux-omap@vger.kernel.org
> > Subject: Re: [PATCH 3/6] OMAP3: Add runtime check for OMAP35x
> > 
> > Hi,
> > 
> > * Sanjeev Premi <premi@ti.com> [090806 13:36]:
> > > Added runtime check via omap2_set_globals_35xx().
> > > 
> > > Parts of this patch have been derived from an earlier
> > > earlier patch submitted by Tony Lindgren <tony@atomide.com>
> > > 
> > >  [1] http://marc.info/?l=linux-omap&m=123301852702797&w=2
> > >  [2] http://marc.info/?l=linux-omap&m=123334055822212&w=2
> > > 
> > > Signed-off-by: Sanjeev Premi <premi@ti.com>
> > > ---
> > >  arch/arm/mach-omap2/id.c                 |  115 
> > ++++++++++++++++++++++++------
> > >  arch/arm/plat-omap/common.c              |   18 +++++-
> > >  arch/arm/plat-omap/include/mach/common.h |    1 +
> > >  arch/arm/plat-omap/include/mach/cpu.h    |   64 ++++++++++++++++-
> > >  4 files changed, 173 insertions(+), 25 deletions(-)
> > > 
> > > diff --git a/arch/arm/mach-omap2/id.c b/arch/arm/mach-omap2/id.c
> > > index a98201c..06770aa 100644
> > > --- a/arch/arm/mach-omap2/id.c
> > > +++ b/arch/arm/mach-omap2/id.c
> > > @@ -28,6 +28,14 @@
> > >  static struct omap_chip_id omap_chip;
> > >  static unsigned int omap_revision;
> > >  
> > > +/* The new OMAP35x devices have assymetric names - 
> > OMAP3505 and OMAP3517.
> > > + * It is not possible to define a common macro to identify them.
> > > + *
> > > + * A quick way is to separate them across 'generations' as below.
> > > + */
> > > +#define OMAP35XX_G1	0x1	/* Applies to 3503, 
> > 3515, 3525 and 3530 */
> > > +#define OMAP35XX_G2	0x2	/* Applies to 3505 and 3517 */
> > > +
> > >  
> > >  unsigned int omap_rev(void)
> > >  {
> > > @@ -155,12 +163,71 @@ void __init omap24xx_check_revision(void)
> > >  	pr_info("\n");
> > >  }
> > >  
> > > +static void __init omap34xx_set_revision(u8 rev, char *rev_name)
> > > +{
> > > +	switch (rev) {
> > > +	case 0:
> > > +		omap_revision = OMAP3430_REV_ES2_0;
> > > +		strcat(rev_name, "ES2.0");
> > > +		break;
> > > +	case 2:
> > > +		omap_revision = OMAP3430_REV_ES2_1;
> > > +		strcat(rev_name, "ES2.1");
> > > +		break;
> > > +	case 3:
> > > +		omap_revision = OMAP3430_REV_ES3_0;
> > > +		strcat(rev_name, "ES3.0");
> > > +		break;
> > > +	case 4:
> > > +		omap_revision = OMAP3430_REV_ES3_1;
> > > +		strcat(rev_name, "ES3.1");
> > > +		break;
> > > +	default:
> > > +		/* Use the latest known revision as default */
> > > +		omap_revision = OMAP3430_REV_ES3_1;
> > > +		strcat(rev_name, "Unknown revision");
> > > +	}
> > > +}
> > > +
> > > +static void __init omap35xx_set_revision(u8 rev, u8 gen, 
> > char *rev_name)
> > > +{
> > > +	omap_revision = OMAP35XX_CLASS ;
> > > +
> > > +	if (gen == OMAP35XX_G1) {
> > > +		switch (rev) {
> > > +		case 0:	/* Take care of some older boards */
> > > +		case 1:
> > > +			omap_revision |= OMAP35XX_MASK_ES2_0;
> > > +			strcat(rev_name, "ES2.0");
> > > +			break;
> > > +		case 2:
> > > +			omap_revision |= OMAP35XX_MASK_ES2_1;
> > > +			strcat(rev_name, "ES2.1");
> > > +			break;
> > > +		case 3:
> > > +			omap_revision |= OMAP35XX_MASK_ES3_0;
> > > +			strcat(rev_name, "ES3.0");
> > > +			break;
> > > +		case 4:
> > > +			omap_revision |= OMAP35XX_MASK_ES3_1;
> > > +			strcat(rev_name, "ES3.1");
> > > +			break;
> > > +		default:
> > > +			/* Use the latest known revision as default */
> > > +			omap_revision |= OMAP35XX_MASK_ES3_0;
> > > +			strcat(rev_name, "Unknown revision");
> > > +		}
> > > +	} else {
> > > +		strcat(rev_name, "ES1.0");
> > > +	}
> > > +}
> > > +
> > 
> > To me it looks like you're checking the exact same cores as 
> > we already do
> > for 34xx. That is, (idcode >> 28) & 0xff for both 34xx and 
> > 35xx. So basically
> > they have the same omap cores.
> 
> No, the cores in OMAP3505 and OMAP3517 are very different.
> I have listed major differences in PATCH 2/6.
> 
> These devices differ in following areas:
>  - Power management capabilities
>    (Only 1 power domain, 1 OPP, etc.)
>  - EMIF4 instead of SDRC
>  - Support for DDR2
>  - EMAC
>  - USB
>  - HECC

Sure, but from compiler flags and io point of view they can still
be treated as 34xx.

How about just add the individual type detection for 35xx processors,
and then have something like this:

#define cpu_is_omap35xx()	(cpu_is_omap34xx() && (cpu_is_omap3510() || \
					cpu_is_omap3520() || cpu_is_omap3530())

That should pretty much shrink this patch series down to about 50 lines or
so of code.

> 
> > 
> > Considering this I don't see much sense adding cpu_is_35xx() category
> > because cpu_is_34xx() already covers these processors. Just 
> > like cpu_is_16xx()
> > covers both 1610 and 1710.
> > 
> > Let's just rather add more feature tests for IVA2 etc as needed, then
> > cpu_is_35something() becomse just cpu_is_34xx() && 
> > cpu_has_iva2() or similar.
> 
> I did feel the need for these tests as well, and have an internal patch.
> It was in my queue for submission next.
> 
> 
> > 
> > 
> > >  void __init omap34xx_check_revision(void)
> > >  {
> > >  	u32 cpuid, idcode;
> > >  	u16 hawkeye;
> > >  	u8 rev;
> > > -	char *rev_name = "ES1.0";
> > > +	char rev_name[16] = "";
> > >  
> > >  	/*
> > >  	 * We cannot access revision registers on ES1.0.
> > > @@ -184,28 +251,12 @@ void __init omap34xx_check_revision(void)
> > >  	rev = (idcode >> 28) & 0xff;
> > >  
> > >  	if (hawkeye == 0xb7ae) {
> > > -		switch (rev) {
> > > -		case 0:
> > > -			omap_revision = OMAP3430_REV_ES2_0;
> > > -			rev_name = "ES2.0";
> > > -			break;
> > > -		case 2:
> > > -			omap_revision = OMAP3430_REV_ES2_1;
> > > -			rev_name = "ES2.1";
> > > -			break;
> > > -		case 3:
> > > -			omap_revision = OMAP3430_REV_ES3_0;
> > > -			rev_name = "ES3.0";
> > > -			break;
> > > -		case 4:
> > > -			omap_revision = OMAP3430_REV_ES3_1;
> > > -			rev_name = "ES3.1";
> > > -			break;
> > > -		default:
> > > -			/* Use the latest known revision as default */
> > > -			omap_revision = OMAP3430_REV_ES3_1;
> > > -			rev_name = "Unknown revision\n";
> > > -		}
> > > +		if (cpu_is_omap35xx())
> > > +			omap35xx_set_revision(rev, OMAP35XX_G1, 
> > rev_name);
> > > +		else
> > > +			omap34xx_set_revision(rev, rev_name);
> > > +	} else if (hawkeye == 0xb868) {
> > > +		omap35xx_set_revision(rev, OMAP35XX_G2, rev_name);
> > >  	}
> > 
> > Testing for hawkeye == 0xb868 test should just be added into 
> > the current
> > omap34xx_check_revision().
> > 
> > Regards,
> > 
> > Tony
> > 
> >