[U-Boot] [PATCH 2/7] Tegra114: Add AVP (arm720t) files

[Tom Warren]

Stephen,

On Wed, Jan 16, 2013 at 3:42 PM, Stephen Warren <swarren at wwwdotorg.org> wrote:
> On 01/16/2013 02:14 PM, Tom Warren wrote:
>> This provides SPL support for T114 boards - AVP early init, plus
>> CPU (A15) init/jump to main U-Boot.
>
>> diff --git a/arch/arm/cpu/arm720t/tegra-common/cpu.c b/arch/arm/cpu/arm720t/tegra-common/cpu.c
>
>> -     if (chip_id == 0x30)
>> +     if (chip_id >= 0x30)
>>               return TEGRA_FAMILY_T3x;
>>       else
>>               return TEGRA_FAMILY_T2x;
>
> Shouldn't that comparison use CHIPID_TEGRA30?
>
> Shouldn't there be a TEGRA_FAMILY_T11x for Tegra114; anything that cares
> about >=Tegra30 can compare the family with >= not ==.
>
> Hmm. It seems the only use of the FAMILY value is get_num_cpus() right
> below; why not have that just switch on the same chip_id variable that
> everything else uses?

Sure, I can refine this code.  I didn't want to spend too much time
obsessively polishing my 'precious' and miss the upstreaming dates I'd
committed to.

But it's worth a look for V2.

>
>> diff --git a/arch/arm/cpu/arm720t/tegra-common/cpu.h b/arch/arm/cpu/arm720t/tegra-common/cpu.h
>
>> +#ifndef TRUE
>> +#define TRUE 1
>> +#endif
>> +#ifndef FALSE
>> +#define FALSE        0
>> +#endif
>
> Surely those are in a standard header somewhere; we shouldn't create yet
> another duplicate of them.

Couldn't find 'em on a quick search (grep define.TRUE) except in
places like scsi.h and ext4_common.h and fpga.h. If you have a
standard header that you know of, let me know.

>
>> diff --git a/arch/arm/cpu/arm720t/tegra114/cpu.c b/arch/arm/cpu/arm720t/tegra114/cpu.c
>
>> +static int is_partition_powered(u32 mask)
>
>> +     reg = readl(&pmc->pmc_pwrgate_status);
>> +     if ((reg & mask) == mask)
>> +             return TRUE;
>> +
>> +     return FALSE;
>
> The last 4 lines are just "return reg & mask;" or "return (reg & mask)
> == mask;". Same in the next function.

I'm porting internal bootloader bringup code here, and trying to avoid
unnecessary changes, but I can modify these to remove the TRUE/FALSE
in V2.

>
>> +void powerup_cpus(void)
>> +{
>> +     debug("powerup_cpus entry\n");
>> +
>> +     /* Are we booting to the fast cluster? */
>> +     if (get_cluster_id() == 0) {
>
> Why would we ever boot on anything other than the fast cluster? I would
> assume that the kernel assume it will always get booted on the fast
> cluster, which would then imply that U-Boot had to boot on or switch to
> the fast cluster.
>

Again, this is from internal NV bootloader code that I know works. I
don't know the circumstances where we might be booted on the LP
cluster, but I figured if the internal bootloader code thought it
worth checking, so should I.  If you have unimpeachable evidence to
the contrary, I can optimize this out.

Tom