[U-Boot] [PATCH v7 00/15] split tegra20 arm7 code into separate SPL

[Simon Glass]

Hi Tom / Allen,

On Thu, Jul 19, 2012 at 4:37 PM, Tom Warren <TWarren at nvidia.com> wrote:
> Allen,
>
>> -----Original Message-----
>> From: Allen Martin [mailto:amartin at nvidia.com]
>> Sent: Wednesday, July 18, 2012 5:02 PM
>> To: Tom Warren
>> Cc: swarren at wwwdotorg.org; sjg at chromium.org; thierry.reding at avionic-
>> design.de; u-boot at lists.denx.de
>> Subject: Re: [PATCH v7 00/15] split tegra20 arm7 code into separate SPL
>>
>> On Tue, Jul 17, 2012 at 12:32:53PM -0700, Tom Warren wrote:
>> > Allen,
>> >
>> > > -----Original Message-----
>> > > From: Allen Martin [mailto:amartin at nvidia.com]
>> > > Sent: Monday, July 16, 2012 4:02 PM
>> > > To: Tom Warren; swarren at wwwdotorg.org; sjg at chromium.org;
>> > > thierry.reding at avionic-design.de
>> > > Cc: u-boot at lists.denx.de; Allen Martin
>> > > Subject: [PATCH v7 00/15] split tegra20 arm7 code into separate SPL
>> > >
>> > > This patch series fixes a long standing problem with the tegra20
>> > > u-boot build.  Tegra20 contains an ARM7TDMI boot processor and a
>> > > Cortex A9 main processor.  Prior to this patch series this was
>> > > accomplished by #ifdefing out any armv7 code from the early boot
>> > > sequence and creating a single binary that runs on both both the
>> > > ARM7TDMI and A9.  This was very fragile as changes to compiler
>> > > options or any additions or rearranging of the early boot code could
>> > > add additional armv7 specific code causing it to fail on the ARM7TDMI.
>> > >
>> > > This patch series pulls all the armv4t code out into a separate SPL
>> > > that does nothing more than initialize the A9 and transfer control
>> > > to it.  The resultint SPL and armv7 u-boot are concatenated together
>> > > into a single image.
>> > >
>> > > This patch series is also available from:
>> > > git://github.com/arm000/u-boot.git
>> > > branch: tegra-spl-v7
>> > >
>> >
>> > Applied to u-boot-tegra/next AOK, tested on my Seaboard AOK, so:
>> > Tested-by: Tom Warren <twarren at nvidia.com>
>> >
>> > Note that I was confused by the final binary name (u-boot-dtb-tegra.bin),
>> since I'm used to flashing u-boot-dtb.bin.
>> >
>> > We need to come to a consensus about the final binary name for Tegra U-
>> Boot (I'd thought we had, and that it would be u-boot-dtb.bin, since that's
>> what most devs are used to looking for in Tegra builds).

I think so.

>> >
>>
>> Yeah, I'd like some stability there too.  The -dtb rule is not tegra
>> specific, which is why I didn't want to modify or remove it.  I think we're
>> the only one that uses it though, so maybe it's not so bad.

Not for long :-)

>>
>> > Also, one nit: I see the 2 sign-on strings (U-Boot SPL 2012.04.xxx, and
>> then U-Boot 2012.04.xxx), separated by 2 lines. I think it'd look better if
>> you had them one right after the other, i.e. eliminate the extra linefeeds.
>> >

>>
>> The extra lines come from display_banner() which is ARM generic from the
>> main u-boot.  I assume they are there to separate the banner from any junk
>> that was on your screen before you rebooted, so it would make sense to move
>> them to the SPL banner instead if you have SPL enabled.
>> I'll make a separate patch for that in a week after I get back from
>> vacation.

I suspect you could remove the extra line by not printing a \n in SPL.
The other one might be a bit tricky as I think it is in U-Boot proper
as you say.

Also do we need the full version tag on the SPL version?

>>
>> -Allen
>
> Cool, thanks. Until then:
>
> Tegra2 SPL patches have been applied to u-boot-tegra/next & pushed to Denx. I'm going to hold off putting it into tegra/master and generating a pull request for awhile to allow all Tegra devs to test it, comment, etc., since it's a major change to the Tegra build.
>
> If possible, please post any new Tegra changes against tegra/next (i.e. using Allen's SPL file locations).

I have a few minor comments on the series now that I have made time to
go through it in final form:

1. In the resulting -tegra.bin image I see this:

00108038 <_fiq>:
  108038:	00108038 	.word	0x00108038
  10803c:	deadbeef 	.word	0xdeadbeef

00108040 <_TEXT_BASE>:
  108040:	0010c000 	.word	0x0010c000

To me it seems odd that SPL shows a TEXT_BASE of 10c000 when we
actually need to load it at 108000. Can we change that? Also due to
the difference between arm720 and armv7 there is no 0x12345678 tag
before the text base. It would be nice if we could have that, as it is
a convenient tag to point to the text base.

In the 'ARM: add tegra20 support' patch:

2. lastdec = 0 seems unnecessary since it should already be 0 at init.

3. Can we init the JTAG earlier (before any serial output, for
example)? It may be useful to be able to set breakpoints in SPL.

In 'tegra20: add u-boot-*-tegra.bin targets':

4. The Makefile stuff could perhaps be split out a bit. You have:

ifeq ($(SOC),tegra20)
ifeq ($(CONFIG_OF_SEPARATE),y)
$(obj)u-boot-dtb-tegra.bin:	$(obj)spl/u-boot-spl.bin $(obj)u-boot.bin
$(obj)u-boot.dtb
		$(OBJCOPY) ${OBJCFLAGS} --pad-to=$(CONFIG_SYS_TEXT_BASE) -O binary
$(obj)spl/u-boot-spl $(obj)spl/u-boot-spl-pad.bin
		cat $(obj)spl/u-boot-spl-pad.bin $(obj)u-boot.bin $(obj)u-boot.dtb > $@
		rm $(obj)spl/u-boot-spl-pad.bin
else
$(obj)u-boot-nodtb-tegra.bin:	$(obj)spl/u-boot-spl.bin $(obj)u-boot.bin
		$(OBJCOPY) ${OBJCFLAGS} --pad-to=$(CONFIG_SYS_TEXT_BASE) -O binary
$(obj)spl/u-boot-spl $(obj)spl/u-boot-spl-pad.bin
		cat $(obj)spl/u-boot-spl-pad.bin $(obj)u-boot.bin > $@
		rm $(obj)spl/u-boot-spl-pad.bin
endif
endif

Leaving aside the nodtb stuff which I think we already discussed, and
breaking the 80col limit, maybe you could do cut it back from:

$(obj)spl/u-boot-spl-pad.bin: $(obj)spl/u-boot-spl.bin
		$(OBJCOPY) ${OBJCFLAGS} --pad-to=$(CONFIG_SYS_TEXT_BASE) -O binary \
                         $(obj)spl/u-boot-spl $(obj)spl/u-boot-spl-pad.bin

$(obj)u-boot-dtb-tegra.bin:	$(obj)spl/u-boot-spl-pad.bin
$(obj)u-boot.bin $(obj)u-boot.dtb
		cat $^ > $@
		rm $(obj)spl/u-boot-spl-pad.bin
else
$(obj)u-boot-nodtb-tegra.bin:$(obj)spl/u-boot-spl-pad.bin $(obj)u-boot.bin
		cat $^ > $@
		rm $(obj)spl/u-boot-spl-pad.bin

to something like:

%-spl-pad.bin: %-spl
		$(OBJCOPY) ${OBJCFLAGS} --pad-to=$(CONFIG_SYS_TEXT_BASE) -O binary \
                         $< $@
$(obj)u-boot-dtb-tegra.bin:	$(obj)spl/u-boot-spl-pad.bin
$(obj)u-boot.bin $(obj)u-boot.dtb
		cat $^ > $@
		rm $<
else
$(obj)u-boot-nodtb-tegra.bin:$(obj)spl/u-boot-spl-pad.bin $(obj)u-boot.bin
		cat $^ > $@
		rm $<

If any of the above merit attention then perhaps you could do a
follow-on patch or two?

Regards,
Simon

>
> Thanks,
>
> Tom
> --
> nvpublic