[U-Boot] [PATCH 3/9] Tegra: T30: Add CPU (armv7) files

[Tom Warren]

Tom,

On Thu, Sep 13, 2012 at 3:04 PM, Tom Rini <trini at ti.com> wrote:
> On Thu, Sep 13, 2012 at 02:21:54PM -0700, Tom Warren wrote:
>> Tom,
>>
>> On Thu, Sep 13, 2012 at 1:33 PM, Tom Rini <trini at ti.com> wrote:
>> > On 09/13/2012 01:30 PM, Stephen Warren wrote:
>> >> On 09/13/2012 02:16 PM, Tom Warren wrote:
>> >>> Stephen,
>> >>>
>> >>> On Thu, Sep 13, 2012 at 1:03 PM, Stephen Warren <swarren at wwwdotorg.org> wrote:
>> >>>> On 09/12/2012 04:10 PM, Tom Warren wrote:
>> >>>>
>> >>>>> diff --git a/arch/arm/cpu/armv7/tegra30/cmd_enterrcm.c b/arch/arm/cpu/armv7/tegra30/cmd_enterrcm.c
>> >>>>
>> >>>> This whole file is definitely common with Tegra20.
>> >>>
>> >>> I'm going through your previous comments, but I'll just reply quickly
>> >>> to this one since it needs clearing up.
>> >>>
>> >>> The intent of this first series of patches for Tegra30 was just to get
>> >>> to the command prompt on T30 in the quickest way, while impacting
>> >>> Tegra20 code as little as possible. Hence, I used Tegra20 files to
>> >>> create a Tegra30 build, and as I ported it to T30 HW, I tried to
>> >>> eliminate what I could that I knew for sure was T20-specific and not
>> >>> useful. But I've made no effort to combine common files/code in this
>> >>> initial pass. I think it's much easier to understand and review these
>> >>> files as a separate SoC build, rather than having to parse
>> >>> common/combined files and code. I intend to do the
>> >>> combination/common-izing of the TegraXX builds once I have a
>> >>> reasonable T30 build in u-boot-tegra, perhaps even before I start
>> >>> porting the drivers.  But this is the initial approach I took.
>> >>> Hopefully it'll be an acceptable course - I'd hate to have to
>> >>> back-track.
>> >>
>> >> To be honest, it seems like the patch to add the Tegra30 deltas to the
>> >> existing Tegra20 code would be massively smaller than duplicating all of
>> >> Tegra20 as Tegra30 and applying those same changes. In the kernel, we
>> >> have both Tegra20 and Tegra30 support with run-time differentiation, and
>> >> the number of places where we have to do something different is not that
>> >> large at all. With the current patch series, there's a huge amount of
>> >> code to wade through, so spotting any places that haven't been updated
>> >> for Tegra30, or weren't intended to be updated yet, is somewhat painful.
>> >
>> > Since we know that the delta can be small, yes, let's just do this right
>> > the first time (or so).  incremental moves, additions and we can work
>> > out run-vs-build time a little further down the road.
>>
>> Sorry, Tom. I'm not clear on exactly which way you'd like to see this go.
>>
>> Are you advising that I re-cast this patchset as a set of common Tegra
>> files/code, with deltas/diffs for the Tegra30 changes? That implies, I
>> think, that I first have to do a patchset that re-orgs Tegra20 code
>> into common code, and then submit a smaller version of this patchset
>> that is just deltas for Tegra30.  That means that I'll be touching
>> everyone's Tegra20 code, and will need Ack's from all the T20 vendors
>> before I can move forward w/T30 code.
>
> As far as I'm conerend to do a:
> git mv arch/arm/cpu/armv7/tegra20/cmd_enterrcm.c
> arch/arm/cpu/armv7/tegra-common/cmd_enterrcm.c (just looking at top of
> tree mainline) needs just the overall Tegra maintainer to Ack.  The
> Custodians page says that's you, and so long as you MAKEALL -s tegra20
> before and after, that's good to go.  By that same token, breaking out a
> hypothetical set of common functions from tegra20/usb.c into
> tegra-common/usb.c and leaving the T20 specific bits in tegra20/usb.c
> and adding tegra30/usb.c later just needs you and MAKEALL happy.
>
>> The other approach, which is still a 2-(or more)-patchset process, is
>> to continue with this patchset for T30, with corrections as per
>> review, and then immediately work on a 'merge-to-common-code' set of
>> patches to common-ize Tegra20/30.  That way Tegra20 is unaffected, I
>> can keep moving forward, and I think the end result will be the same
>> as the approach above.
>
> As has been noted, when you copy files you get a lot of re-review and
> it's hard to tell what's new and what's not.  It IS good to post what
> you have posted and say "please check areas X/Y/Z, it's new code".  But
> reviewing code that will be dropped as soon as you switch to the
> 'merge-to-common-code' branch being the reviewed one is hard, especially
> when folks with less Tegra background try and read the patches for
> general issues.
>
>> I can see value in both approaches, and it shouldn't surprise you that
>> I'd favor the 2nd approach, since it's less chaotic for me. Let me
>> know what you think,
>
> Well, I'd argue that since you're going to need to do the
> 'merge-to-common-code' path at some point, it's going to save you work
> to do that now rather than fixup issues in two places.  And again, if
> you don't change the code, just where the code is, MAKEALL will catch
> your problems for you.

Moving the code via git as you pointed out above is fine, but how do I
hook 'tegra-common' into the boards.cfg/Makefile/MAKEALL process? I'd
need a 'tegra-common'  subdir for arm720t, armv7 and an 'arch-tegra'
subdir for arch/arm/include/asm/.  The arch-tegra common subdir is
easy, as I can just have the <asm/arch/blah.h> file do an 'include
<asm/arch-tegra/blah.h> when the include file is 100% common and flesh
it out when it's SoC-specific. But I don't see how I can build the SPL
and 'normal' U-Boot sections via boards.cfg w/common code, without
having a SPL.C that says 'include ../tegra-common/spl.c', which seems
messy. Not to mention that we have arch/arm/cpu/tegra20-common and
../tegra30-common, too. How do you do it on OMAP?

Tom
>
> --
> Tom