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

[Tom Warren]

Peter,

On Fri, Mar 25, 2011 at 9:02 AM, Peter Tyser <ptyser at xes-inc.com> wrote:
> Hi Tom,
> Things look pretty good.  Minor comments/questions below.
>
> <snip>
>
>> +/*
>> + * TBD: Move cold_boot() to assembly file.
>> + * Values/offsets of the table vars make this difficult.
>> + */
>> +
>> +void cold_boot(void)
>> +{
>> +     asm volatile(
>> +             "msr    cpsr_c, #0xD3   \n"
>> +             /*
>> +             * Check current processor: CPU or AVP?
>> +             * If CPU, go to CPU boot code, else continue on AVP path.
>> +             */
>> +             "mov    r0, %0          \n"
>> +             "ldrb   r2, [r0, %1]    \n"
>> +             /* are we the CPU? */
>> +             "cmp    r2, %2          \n"
>> +             "mov    sp, %3          \n"
>> +             /*  yep, we are the CPU */
>> +             "bxeq   %4              \n"
>> +
>> +             /* AVP initialization follows this path */
>> +             "mov    sp, %5          \n"
>> +             /* Init and start CPU */
>> +             "b      startup_cpu     \n"
>> +             :
>> +             : "i"(NV_PA_PG_UP_BASE),
>> +             "i"(PG_UP_TAG_0),
>> +             "r"(proc_tag),
>> +             "r"(cpu_boot_stack),
>> +             "r"(_armboot_start),
>> +             "r"(avp_boot_stack)
>> +             : "r0", "r2", "cc", "lr"
>> +     );
>> +}
>
> What errors did you encounter when this was in the assembly file?  It'd
> be nice to put it there now.  Likely it will never get fixed if it
> doesn't implemented correctly off the bat.  If you post the errors
> perhaps someone on the list can provide insight.
I didn't capture a log of the errors when I was trying to put the
cold_boot code into lowlevel_init.S. But I saw fixup errors and
undefined constant errors, all related to the #defines (NV_PG_UP_BASE,
avp/cpu_boot_stack, etc.) and how the compiler/assembler references
indirect and relative constants.

Note that this code works perfectly as-is, so there's no pressing need
to move it to assembly now, except for a cosmetic/procedural one. I'd
rather get this accepted into mainline, so I can move on to the
eMMC/SPI/USB drivers so people can use the code to boot an OS on our
(many) Tegra2 boards coming to market RSN.

If some ARM / gcc assembly wizard wants to attempt moving this code to
a .S file, I welcome the help - I may even attack it at a later date,
when I've got more bandwidth. But it isn't a priority for me right
now, unless someone on the list adamantly opposes the code as-s. But
I'd expect anyone with that strong an opinion about to be able to fix
it, or at least attempt it and see why I decided to defer moving it to
assembly for now.

>
> <snip>
>
>> +.globl startup_cpu
>> +startup_cpu:
>> +     @ Initialize the AVP, clocks, and memory controller
>> +     @ SDRAM is guaranteed to be on at this point
>> +
>> +     ldr     r0, =cold_boot                  @ R0 = reset vector for CPU
>> +     bl      start_cpu                       @ start the CPU
>> +
>> +     @ Transfer control to the AVP code */
>> +     bl      halt_avp
>> +
>> +     @ Should never get here
>> +_loop_forever2:
>> +     b       _loop_forever2
>> +
>> +.globl cache_configure
>> +cache_configure:
>> +     stmdb r13!,{r14}
>> +@    /* invalidate instruction cache */
>
> It looks like there's a combination of comment forms @, @ */, and @ /*
> */.  Is there a reason not to use the normal /* */ universally?
No, just dross left over from moving the inline assembly from .c to
.S. The rest of lowlevel_init.S uses @ for comments, so I tried to
stick with that. I'l fix it, thanks.

>
> Best,
> Peter
>
>
Thanks,
Tom