"Mark A. Greer" <[email protected]> writes:
> On Tuesday 20 January 2009, David Brownell wrote:
>
> Hi David & Kevin.
>
> [My apologies for being out of the loop on this. I just subscribed a
> few mins ago and still catching up on the emails.]
Hi Mark,
I was hoping you would be joining this thread... :)
>> On Tuesday 20 January 2009, Sergei Shtylyov wrote:
>> > > Either way, the lack of a complete proposal (not necessarily
>> > > in the form of patches) makes it hard to get anywhere with
>> > > such OMAP-L1xx discussions...
>> >
>> > I think I've expressed it clear enough: common shared code is
>> > to be moved to plat-davinci/ and OMAP-L1x support is to be
>> > added into new mach- directory.
>>
>> And yet Kevin felt that was missing details (not complete)...
>>
>> While that sounds plausible to me at this point, it's also
>> clear that the missing details could make a big difference.
>>
>> I suspect that until patches appear, discussion can get no
>> further. Plus, if it's going to be "mach-omap-L1" it'd
>> be good to have enough detail that the OMAP team (and RMK)
>> can see why it should pair with "plat-davinci" instead of
>> the more obvious "plat-omap".
>
> Regarding the name
> ------------------
>
> I really don't like the omap creep that's happening. The da830 is
> based on davinci and not at all on an omap so I'd like to keep it
> contained as possible. TI has expressed a strong desire to have
> both so our current plan is the rename the defconfig
> to "da830_omapl137_defconfig" and menuconfig text options to say
> "DA830/OMAP-L137...". I don't know if this will be acceptable to the
> community.
I don't care much about the defconfig names or the Kconfig option
names. The bigger question is what to to use as function/#define
prefixes (current code uses DM646X_* DM355_*, DM644X_* etc.. I think
using both will result in long names for not very much gain.
Personally, if TI is OK with it, for kernel-internal names, I would
lean towards the da8xx prefix rather thant the omap prefix since it
would result in less confusion with the omap names.
> Regarding the code split
> ------------------------
>
> I agree, I don't think there has been a convincing argument yet.
> I'll try to make one...
>
> First some background. We originally put the da8xx code (what we've
> called it but da830 may be a better name) in mach-davinci. We fought
> with that for quite some time but all the #ifdef's and other ugly
> things continued to add up as we learned more nits about the hardware
> that made it more & more different than a true davinci. So, we ended
> up splitting the code.
>
> The biggest drivers for splitting the code (that I remember ATM) were:
>
> 1) Reduce the amount of #ifdef-ery and other ugliness; however we didn't
> split include/asm-arm/arch-davinci (but neither did omap).
If done right, support for multiple devices should need very minimal
#ifdefs, if any. As patches have come to me with device or board
support added with ifdefs they are always NAK'd. The right way is to
use cpu_is_* or machine_is_* macros along with __init data and
functions.
> 2) Remove unnecessary bloat from davinci code & data laying around
> but entirely useless to us.
Again, when done right the bloat can be mostly handled with __init
data and functions. And for production, simply compile only with the
device/board you care about.
More on bloat below...
> 3) There was a precedent set by the omap code so we followed the basics
> of how that split was made.
Yes, omap1 and omap2/3 code are split, but omap2 and omap3 both live
in mach-omap2 (yes, it's confusing) and within omap2 and 3 most of the
code is shared between the two and uses cpu_is_* and machine_is_* to
handle any differences.
> Some more notes:
>
> - We (or at least *I*) had no desire to have the same kernel binary
> run on both a da8xx and a davinci. So, cutting out the davinci
> runtime code & data that was wasting memory was "A Good Thing (tm)".
The single kernel image is a HUGE win for the maintenance of the git
tree where a single kernel tree will have support for multiple devices
with potentially multiple boards per device. Yes, it leads to larger
kernels and potentially slower boot-time. However the goal of the
single-image is for development and maintenance, not necessarily for
products. That being said, when the multiple-device support is done
correctly, the size and performance problems disappear when you simply
build a kernel for a single device and board. Even in multiple device
kernels, __init data and functions allows you to recover any wasted
memory, and conditional cpu_is_* code can be optimized away:
if (cpu_is_davinci_dm355())
foo()
becomes
if (0)
foo
if dm355 support is not built in, allowing the compiler to simply
optimize this code away.
The current DaVinci git can build a single kernel image for dm6446,
dm355 and dm6467. When reviewing, testing and merging patches for
DaVinci git is a very significant savings for me to apply the patch to
a single tree, build a single image and boot it and test it on
multiple boards. It would be a major slowdown to have to do a kernel
rebuild for every patch for every device.
> - We put the common code in plat-davinci and took the da8xx code
> out of mach-davinci and put into a new dir, mach-da8xx. Basically
> following the model of the omap code.
>
> - Split clock.c. The main reason was different base addrs for the
> psc & pll cntrl base.
This type of thing is handled in various parts of current davinci git
(and OMAP git) by using cpu_is_* macros at init time to set the
correct base address. Then, the base address is just passed around to
the various functions.
Re: PLL, I recently posted some RFC patches to the list which rework
the current PLL code with this in mind and with the eventual goal of
DVFS support. It models the basic PLLC that is shared across all the
devices that I've seen so far (including da8xx follow-ons) and uses
cpu_is* to set flags for various device specific features (like
pre-dividers, post-dividers, # of PLLDIV regs, etc.)
Splitting when primarly the base address is different results in
duplicated code which is almost identical. IMHO, this becomes very
messy to maintain.
> - devices.c contents has virtually no common code between the two.
I'm OK with a new da8xx.c. For that matter, it might make sense to
have dm644x.c, dm355.c and dm646x.c as well. And keep devices.c only
for things that are common to all (if there are any left.)
> - 'struct map_desc' data in io.c is different. da8xx has 2 entries
> and different addresses than the davincis. Could probably solve
> at runtime instead, if that was necessary.
In OMAP git, we use chip-specific tables that are selected at runtime.
> - Need different Makefile.boot files because of different phys addrs
> for system memory.
I addressed this in a reply to Steve's question. Basically, this only
affects the [zu]Image, not the kernel itself which is linked at a
virtual address.
> - Split time.c. Again different base addresses was main reason for the
> split.
Use cpu_is_* at init time.
> - The SPI setup code is quite different.
The SPI driver (not yet sumibitted to DaVincit git, or upstream AFAIK)
probably needs a rework to separate out what is platform specific from
generic. In OMAP git there is quite a wide range of ways to configure
the SPI driver, but it all involves passing in platforms specifics via
platform_data.
> - Entirely different interrupt controller.
Easy to add a new cp_intc.c, who's init is called at runtime.
> - Entirely different dma controller.
Hmm, doesn't it have EDMA3, which AFAICT is basically the same block
that is on dm355.
> - There are probably others that I've forgotten.
>
> None of these by themselves are all that convincing but they add up to
> a mess if they aren't split. Either way, we need some sort of factoring
> out of common code so the question is, do we keep the da8xx and common
> code in the same directory or not?
I will continue to argue for the same directory until pursuaded
otherwise.
Personally, I think splitting them results in a bigger mess as it
results in more code that is mostly the same but will evolve
differently.
> IMHO, there's enough complete different stuff on the da8xx that it
> warrants its own directory (and therefore common code should go in
> a plat-<xx> directory). Otherwise, we're just putting two different
> things in the same bucket for no valid reason. This is debatable,
> of course. MHO would change if there really was a strong desire to
> have the same kernel binary run on both types of SoC's. Is there?
Absolutely.
As the person who has to maintain a single code base that will run on
all these platforms, I have a *very* strong desire to have a single
binary run across multiple SoCs. The savings in my
review/build/test/merge cycle as well as the savings by fixing a bug
in a single place and getting it "for free" on the rest of the
platforms is significant. Not to mention that enforcing this results
in code that has to be structured better, and as a result is more
maintainable over the long term.
In summary, I think some of the current splitting decisions made sense
at the time when there was a concern for minimally changing existing,
working code. However, for getting this code upstream, there is a
much stronger concern for good structure, code recycling and long term
maintenace of a single code base to support multiple devices and
boards. Unfortunately, it does indeed require more work up front, but
in my view of things, it is very much worth it.
I hope that helps clarify my thoughts,
Kevin
_______________________________________________
Davinci-linux-open-source mailing list
[email protected]
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
