On Thu, Sep 13, 2012 at 03:16:21PM -0700, Tom Warren wrote:
> Tom,
>
> On Thu, Sep 13, 2012 at 3:04 PM, Tom Rini <tr...@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 <tr...@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
> >> >>> <swar...@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?
I'd like to see <plat/blah.h> which goes to <plat/tegra/blah.h> or
whatever, which is similar to what the kernel has nowadays. Solving
this for TI parts (OMAP/AM33x and DaVinci share at times) is still on my
TODO list. I could live with <asm/arch-foo/blah.h> or maybe even
<asm/plat/blah.h> (I'm not awesome at naming things). For the C files,
I thought the concensus ended up with arch/arm/cpu/tegra-common/ as the
least-objectionable way to handle this problem in general. Then it's
just a matter of adding a line or 5 to Makefile and spl/Makefile to say
on tegra, also build in arch/arm/cpu/tegra-common. Note that on
OMAP/AM33x we have arch/arm/cpu/armv7/omap-common and
arch/arm/cpu/armv7/{omap3,omap4,omap5,am33xx} for armv7 but cross-SoC
bits go and armv7 but SoC specific bits go. The rule for omap-common is
two or more rather than all of the above.
--
Tom
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
