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

[Peter Tyser]

On Fri, 2011-03-25 at 09:16 -0700, Tom Warren wrote:
> 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.

I understand your perspective, but why not spend the extra 30 minutes
and do it the right way?  Passing the buck to someone else who cares
about maintaining high quality code isn't the right thing to do in my
opinion.  This patch isn't going to make it into the upcoming release,
so it won't gate the other eMMC/SPI/USB drivers you want to add.  The
bar to get code into open source project generally is higher than "it
works" - it has to adhere to the project's design principles and
guidelines.  U-Boot already needs cleanup as is without adding more
cruft.  Solving this small issue now results in cleaner code, less
headache down the road, and shouldn't take long.  As usual, I'm not the
maintainer, so its just my $0.02.

Best,
Peter