Re: [U-Boot] [PATCH 5/8] Tegra124: Add common CPU (shared) files

On 10/07/2013 04:42 PM, Tom Warren wrote:

These files are used by both SPL and main U-Boot.

diff --git a/arch/arm/cpu/tegra-common/ap.c b/arch/arm/cpu/tegra-common/ap.c

@@ -81,7 +81,14 @@ int tegra_get_chip_sku(void) return TEGRA_SOC_T114; } break;

• case CHIPID_TEGRA124:

• switch (sku_id) {
  

• }
  

• case SKU_ID_T124_ENG:
  

• 	return TEGRA_SOC_T124;
  

• break;
  

  }

Something is wrong with the block layout there. Is the switch's closing } two lines too early?

diff --git a/arch/arm/cpu/tegra-common/cache.c b/arch/arm/cpu/tegra-common/cache.c

@@ -33,16 +24,17 @@ void config_cache(void) "orr r0, r0, #0x41\n" "mcr p15, 0, r0, c1, c0, 1\n");

• /* Currently, only T114 needs this L2 cache change to boot Linux */

• /* Currently, only T1x4 needs this L2 cache change to boot Linux */ reg = (readl(&gp->hidrev) & HIDREV_CHIPID_MASK);

• if (reg != (CHIPID_TEGRA114 << HIDREV_CHIPID_SHIFT))

• return;
  

• /*

• * Systems with an architectural L2 cache must not use the PL310.
  

• * Config L2CTLR here for a data RAM latency of 3 cycles.
  

• */
  

• asm("mrc p15, 1, %0, c9, c0, 2" : : "r" (reg));
• reg &= ~7;
• reg |= 2;
• asm("mcr p15, 1, %0, c9, c0, 2" : : "r" (reg));

• reg = (reg >> HIDREV_CHIPID_SHIFT) & 0xFF;
• if ((reg == CHIPID_TEGRA114) || (reg == CHIPID_TEGRA124)) {

• /*
  

•  * SoCs with an architectural L2 cache must not use the PL310.
  

•  * Config L2CTLR here for a data RAM latency of 3 cycles.
  

•  */
  

• asm("mrc p15, 1, %0, c9, c0, 2" : : "r" (reg));
  

• reg &= ~7;
  

• reg |= 2;
  

• asm("mcr p15, 1, %0, c9, c0, 2" : : "r" (reg));
  

• }

}

Why change the indentation level here? The if (xxx) return could have been enhanced to check for either chip without changing the indentation level of the rest of the function.

Also, we shouldn't be checking for the SoC, but rather for the CPU core type. On A9 we want to return. On A15, we want to continue. If we had written the code to check the correct thing before, we wouldn't have to touch this function at all when porting to Tegra124.