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