[U-Boot] [PATCH] arm: Tegra2: add support for A9 CPU init

[Peter Tyser]

Hi Tom,

> >> >
> >> >> +     /* Is PLL-X already running? */
> >> >> +     reg = readl(&clkrst->crc_pllx_base);
> >> >> +     if (reg & PLL_ENABLE)
> >> >> +             return;
> >> >> +
> >> >> +     /* Do PLLX init if it isn't running, but BootROM sets it, so TBD */
> >> >> +}
> >> >
> >> > The above function looks incorrect.
> >> What looks incorrect? It checks to see if the PLL is already
> >> running/enabled, and returns if it is.
> >> Tegra2 mask ROM (BootROM) currently always sets PLLX, so this will
> >> always return, but the comment is there for future chips that may not
> >> set up PLLX.
> >
> > It looks like a function that does a read of a value it doesn't care
> > about, does an unnecessary comparison, and then returns either way,
> > without doing anything:)  If/when you want to do future stuff with the
> > PLL-X, code should be added then - as is it doesn't do anything and is
> > confusing.  The general policy of U-Boot is not to add dead code.
> OK, so not really incorrect, just unnecessary. Agreed - again a
> vestigial leftover from our in-house code. I'll remove it.

Unnecessary/misleading/dead code is inherently not correct:)

<snip>

> >> >> +#include <asm/types.h>
> >> >> +
> >> >> +#ifndef      FALSE
> >> >> +#define FALSE        0
> >> >> +#endif
> >> >> +#ifndef TRUE
> >> >> +#define TRUE 1
> >> >> +#endif
> >> >
> >> > These shouldn't be added here.  They should be somewhere common, or
> >> > shouldn't be used (my preference).
> >> I would think they'd be in the ARM tree somewhere, but I couldn't find
> >> them so I added 'em here.
> >> My preference is to use TRUE/FALSE - it carries more context than 1/0 to me.
> >
> > If you prefer to use TRUE/FALSE, they should be moved into a common
> > location so everywhere uses the same, once-defined definition.  Their
> > definitions are already littered over a few files, which would ideally
> > be cleaned up.  Perhaps moving all definitions into include/common.h, or
> > somewhere similar would work.  Others may have opinions about TRUE/FALSE
> > vs 1/0 - it seems like TRUE/FALSE aren't generally used.
> I don't want to pollute all builds by adding to include/common.h.
> I'll try to find a more central header in my own tree.

My point is that there are already 32 definitions of TRUE/FALSE - adding
a 33rd doesn't seem like a good thing to do.  I view a 33rd identical
definition as polluting the code more than 1 common definition that most
people won't generally use.

Its not my decision, but I assume the powers that be would recommend one
of:
- Not using TRUE/FALSE since using non-zero values and 0 are widely
accepted as TRUE/FALSE.  I think using TRUE/FALSE makes the code harder
to understand and more open to bugs.  Eg for other code that interacts
with your code, or someone reviewing your code, they either have to
either assume you defined TRUE as 1, FALSE as 0, or import your
definitions.  Anyway, I view their use as another layer of unnecessary
abstraction with very little benefit.

- Consolidating the definition of TRUE/FALSE.

Wolfgang, do you have a preference about how TRUE/FALSE are generally
used/defined?

Best,
Peter