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

[Tom Warren]

Peter,

On Fri, Mar 25, 2011 at 10:22 AM, Peter Tyser <ptyser at xes-inc.com> wrote:
> 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.
FWIW, I spent _far_ more than 30 minutes on this .. close to a full
day of frustration/banging my head against the wall.  I have other
priorities besides upstreaming Tegra2 U-Boot support, and I can't
justify spending days on this. As I originally stated, I'm no expert
in the intricacies of ARM asm programming - my expertise is in x86
CPUs, and hit a wall with this port to assembly.

I'm not passing the buck, and while I agree that this code is not the
cleanest I've seen, I don't think I'm pushing low-quality code here.
It's 8 lines of embedded assembly, plus a table. Looking at what the C
compiler produces in assembly, it's twice as long, pushes some of the
tabled values on the stack and then pulls them back into registers,
and is, IMHO, harder to understand.  I could just cut-and-paste the
compiler output into lowlevel_init.S, and add some comments, but is
that really any better?  Is the goal to get clean, understandable
code, or messier, harder to parse code in the right files?

As to adhering to U-Boot's design principles and guidelines, could you
point me to the section on embedded assembly in C files? I don't
remember seeing a specific section on that topic, don't see it under
U-Boot Coding Style, nor in the Linux coding guidelines, and I'd like
to reference it for future use.

For now, I'm going to let this percolate, as I have other fish to fry.

Thanks for your input,

Tom
>
> Best,
> Peter
>
>