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

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