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

Wolfgang Denk
2011-05-16T15:35:51Z

Dear Aneesh V,

In message <4DD11511.1060208@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