On Tue, Jan 20, 2009 at 05:26:38PM -0800, Kevin Hilman wrote: > "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,
Hi Kevin. > I was hoping you would be joining this thread... :) > > > 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. Agreed. > 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. Unless something changes, the .config options and macros will still be DA8XX so I think [hope] we're okay. > > 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. Right, and that what's we did and have been calling "splitting"/"refactoring" the code. Basicaly, where #ifdef's would have had to go, the code was refactored and the correct value passed into the routine instead of using the previous macro or whatever. Actually, you're supporting one of my points. That is, no matter whether the code is kept in one dir or spread over three dirs, the code will be split/refactored/<whatever you want to call it>. Its just a technical necessity. We _will_ end up with 3 sets of code, a davinci-specific set, a da8xx-specific set, and a common set. The real question is whether all three sets go in one dir or each set gets its own dir. > > 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. Actually, almost all of that statically initialized data is not __initdata and can't be (e.g., 'stuct clk' data, 'struct resource' data, platform_data). Its not megabytes but it is a waste. > > 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. Well, "HUGE win" for git tree maintenance is a bit of an overstatement, I think. It may make it easier to find all the callers of a function in the common code with a simple grep but, OTOH, I find git-grep and cscope work pretty well for that. ;) I think its more of a gain for product development/deployment. You only have to worry about one kernel for your products. So, how much overlap is there with davinci products and da8xx products (da8xx are for stereos and home theater audio AFAIK)? On the cost side of the equation, things are going to be messier in mach-davinci because there will now be parallel files (e.g., a davinci devices.c file and a da8xx devices.c file, a davinci irq.c file and a da8xx irq.c file). Meanwhile, the other code will still be refactored to deal with different reg base addrs, etc. > 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. Yep. > 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. Maybe I don't know what you mean when you say, "a single tree". There's still only one tree. If you applied one patch before, you apply one patch now. There would be 2 kernel images, though--one for davincis and one for da8xxs. Its only when you changed the common code that you would have to test both. I get your point though, if you changed something in common, you'd rather do 1 build and test than do 2 builds and deal with 2 kernels. > > - 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.) That's good to hear. > Splitting when primarly the base address is different results in > duplicated code which is almost identical. IMHO, this becomes very > messy to maintain. Erm, I think you misunderstand what we mean by "splitting". We mean refactor the code so that it uses the argument passed into the routine instead of a hardcoded value or macro/#define. There is no code duplication. You're doing what we've done and have been arguing _has_ to be done. That is, refactor the code to pass in base addrs or whatever. IOW, you're arguing my point again. :) I think we're in violent agreement on this. > > 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. That's reasonable. > 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. No, the code _has_ to be split/refactored and you've been arguing for it above. Its just a technical reality and I think we both agree on this but we don't realize it. [As I reread your comment...yes, there is potential that code in mach-davinci would evolve differently than code in mach-da8xx.] I'll summarize what I'm saying: a) No matter what, the code needs to be refactored so that the differences between the SoCs are parameterized and one set of common routines use those parameters. b) Due to the differences between the SoC families, members within the SoC families, and platforms with the same SoC, there will be several sets of files (some sets may be empty for now): i) files with common code ii) files with code common for all davincis iii) files with code for specific davinci SoC (e.g., dm355) iv) files with code common for all da8xxs v) files with code for specific da8xx SoC (e.g., da830) vi) files with platform specific code (e.g., dm355 evm, da830 evm). c) The question we arguing about is whether all of these go into one directory or not. > > 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 1 kernel build vs. 2 kernel builds, and only when common code changes. > 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. There is no duplication in what we're suggesting so this point is moot. > 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. We're improving the structure by refactoring the code, getting rid of gratuitous assumption of where blocks of registers are, etc. > 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. Again, you argue in support of my point. > 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, It does. I now realize that we agree on the refactoring. The dispute is solely over keeping the files in one directory or not. Advantages of keeping in one dir are: - 1 kernel build vs. 2 when testing - you only have to grep one directory when a common code interface changes Advantages of separating into 3 dirs are: - less confusing to know what code to look at when adding support for a new board - less file clutter in any one directory I suppose that the pragmatic thing to do is leave the code in one dir until it becomes too cluttered then move to separate dirs. A somewhat related point, use of cpu_is_*. I think it would be smarter to factor out the use of those calls as much as you can. If you had one overall board.c file that did the cpu_is_*'s and hooked up the correct device data, platform_data, etc. (or called a routine in an SoC specific file that did the hooking up). It would make the code more modular, easier to follow, and easier to add support for new variants. It would also allow you to alloc space for and copy the necessary data and make the structs that now can't be __initdata, __initdata. Make sense? Mark -- _______________________________________________ Davinci-linux-open-source mailing list [email protected] http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
