[U-Boot] [PATCH v2 02/22] omap4: add OMAP4430 revision check

[Wolfgang Denk]

Dear Aneesh V,

In message <4DD11511.1060208 at ti.com> you wrote:
>
> >> +	const char *omap4_rev = NULL;
> >> +	switch (omap4_revision()) {
> >> +	case OMAP4430_ES1_0:
> >> +		omap4_rev = "OMAP4430 ES1.0";
> >> +		break;
> >> +	case OMAP4430_ES2_0:
> >> +		omap4_rev = "OMAP4430 ES2.0";
> >> +		break;
> >> +	case OMAP4430_ES2_1:
> >> +		omap4_rev = "OMAP4430 ES2.1";
> >> +		break;
> >> +	case OMAP4430_ES2_2:
> >> +		omap4_rev = "OMAP4430 ES2.2";
> >> +		break;
> >
> > Such code does not really scale well.  Can this not be improved?  I
> > think we just had similar discussions for i.MX5x - please check what
> > the result of these was.
> 
> Are you referring to this one?
> http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/98522
> 
> If so, it may not work for us:
> 
> 1. Please note that the above function is just for getting the string
> not for the revision itself. To get the revision we have
> omap4_revision().

Well, when you already have such a funxction, then why cannot it be
made to return useful values that are well-suited for formatting?

Instead of 

	#define OMAP4430_ES1_0 1
	#define OMAP4430_ES2_0 2
	#define OMAP4430_ES2_1 3
	#define OMAP4430_ES2_2 4

you could use

	#define OMAP4430_ES1_0 10
	#define OMAP4430_ES2_0 20
	#define OMAP4430_ES2_1 21
	#define OMAP4430_ES2_2 22

And then use a plain

	sprintf(omap4_rev, "OMAP4430 ES%d.%d", rev/10, rev%10);

or similar.

> 2. In our case we do not have a 1:1 mapping between the
> revisions(monotonically increasing numbers) we need in the U-Boot and
> the value obtained from the ID_CODE register. So, a translation is
> inevitable.

This is not needed. See above.  Any form of table driven approach
would be suitable, too.

> 3. We need increasing numbers for subsequent revisions so that we can
> have something like:

Should be no problem, see above.  Just define your number scheme.
Instead of decimal packing you could also adapt something like the
major/minor numbers for devices, and use the existing macros to
extract the parts.

There are tons of existing solutions for this type of problem, just
pick one that fits.

> >> +	default:
> >> +		omap4_rev = "OMAP4 - Unknown Rev";
> >> +		break;
> >
> > Please also output what the unknown revision was - this saves at least
> > one debug round if you ever run into this case.
> 
> This function should be in sync with omap4_revision() function(unless
> there is a bug). So, the rev will be OMAP4430_SILICON_ID_INVALID in
> that case.

In this case omap4_revision() should print the value it is reading.
Whenever you are reading "unexpected? numbers the first thing you will
do during debugging is to check _what_ exactly you are reawding - so
you can help and safe one step by just printing thei information which
you have ready in your hands.

> I shall print out the ARM revision and OMAP revision registers when I
> get into OMAP4430_SILICON_ID_INVALID situation in omap4_revision()

Thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"We have the right to survive!"
"Not be killing others."
	-- Deela and Kirk, "Wink of An Eye", stardate 5710.5