Hello.
Kevin Hilman wrote:
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.
That somewhat smells of a wishful thinking. :-) We're still relying of
#ifdef'ery quite a lot.
The real question is whether all three sets go in one dir or each
set gets its own dir.
Well, that's one question, but that's not the real question for me.
The real question for me is which da8xx IP blocks are really that
different from the davinci family to warrant a new file. From what I
have seen so far in the modules I have looked at, the da8xx IP blocks
(at least ones that would handled by mach-* code) are either the same,
or extension of existing davinci HW IP. The only exception being the
new CP_INTC, and maybe the new mux, but I haven't looked at the
details there yet.
PinMux is the part of different module now (not an issue with how the data
is represented via struct mux_config) AND they require unlocking before access
(that's where we had to use #ifdef).
IOW, rather than add new files for da8xx, I would rather see the
existing davinci files refactored (maybe generalized is a better term)
with support for the da8xx added to the same file.
Sigh...
Maybe you guys have already done it this way. I have no idea since I
don't have access to the da8xx kernel code. [hint, hint...]
We have, but we have ditched it since. Well, it's retrievable...
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.
That's trifles -- 'struct mux_config' which will take up really much space
with DA830's 400+ controllable signals.
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.
Yes.
What a pity that ARM is not x86. :-P
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.
Yes, we agree. Code has to be refactored/generalized and in some
cases redesigned to be more generic (in the case of the PLLC/clock
code, it needs a redesign, which I've started.)
I think the disagreement comes in whether or not to refactor into
separate files, or just make the existing file more generic to handle
the differences across devices.
Again, maybe you've already done it this way. I'd love to see the
code.
Yes, we have. There are different ways to do it though...
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.
Yes. This is how I would like to proceed as well.
And to take it a step further, I would like to see the da8xx support
added to existing _files_ by refactoring existing code or generalizing
it where necessary (except, of course, for the new blocks like
CP_INTC.)
So to get concrete, here is how I envision things looking
under mach-davinci.
Device or platform specific files:
<device>.c: device-specific init, platform_data, etc.
where <device> = [dm644x | dm355 | dm646x | da8xx | ...]
This will warrant almost similar files for 3 DaVincis, so I'd reconsider
that...
NOTE: Care needs to be taken so that device-specific stuff and
board-specific stuff stays in the appropriate <device>.c and
board-*.c files. This way, adding support for new boards is
as easy as possible.
Sure, and it may be non-trivial. We haven't completely dealt with that.
Generic code for all devices, with init functions that handle device
pecific parameters, either using cpu_is_* checks, or with options
passed in from <device>.c.
Oh, the second scheme appeals more to me.
clock.c - with clock definitions moved to <device>.c
edma.c
gpio.c
id.c
io.c - could probably be moved into <device>.c as well
mux.c
Where do we put what's now in mux_cfg.c, given it's optional?
psc.c
Er, I'm seeing a tendention of moving this code to devices.c (or is it the
opposite? :-).
serial.c
Platform device and quirks needs to be moved to <device>.c from there...
time.c
and then the non-common blocks, where splitting into a separate file
is much more obvious.
irq.c | cp_intc.c
Not that the latter won'tbe a part of arch/arm/mach-davinci/.
Kevin
WBR, Sergei
_______________________________________________
Davinci-linux-open-source mailing list
[email protected]
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
