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

[Tom Warren]

Peter,

On Thu, Mar 17, 2011 at 7:32 AM, Peter Tyser <ptyser at xes-inc.com> wrote:
> 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.
I've removed both the defines and the use of TRUE/FALSE in ap20.* -
there were only a few instances.

>
> - Consolidating the definition of TRUE/FALSE.
>
> Wolfgang, do you have a preference about how TRUE/FALSE are generally
> used/defined?
>
> Best,
> Peter
Thanks,
Tom
>
>
>
>