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

[Peter Tyser]

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

Understood.  When situations like that happen its good to ask others for
input rather than using a non-optimal solution.  Then either
- Someone suggests how to fix the code, and the code improves.
- No one helps, which implies your method isn't easy to improve upon,
you gave it a good effort, and so it gets included.

As is, the current patch looks like a non-optimal solution that someone
familiar with ARM asm could have quickly helped with.

> 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.

Based on the fact that you tried to fix it for a day, it sounds like you
are aware its less than ideal implementation, and the "TBD" comment for
the function further implies that this function doesn't belong here, and
someone should fix it down the road.  My guess is that this will likely
never happen unless it gets fixed now.

> 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.

There isn't a specific section about inline assembly, similar to how
there isn't documentation other general C rules like what goes in header
files, when to split up a file into 2 files, why global variables should
be avoided, etc.  The general rule is that inline assembly should only
be used in when necessary in rare cases like low-level control/access of
the CPU, special optimizations in critical paths, etc.

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

Sounds good.  I understand the schedule vs perfection issue - I'm just
voicing my opinions based on how I'd write the code with my infinite
free time, which ultimately doesn't carry a whole lot of weight:)
Regardless of how it works out its great seeing Nvidia push their code
upstream.

Best,
Peter