[Linux-c6x-dev] Rough review of the c6x kernel tree in git

[William Mills]

On 09/23/2010 11:30 AM, Arnd Bergmann wrote:
> Hi everyone,
>
> I'm interested in all new architecture ports that want to merge
> their code into the upstream kernel.
> I briefly looked at the c6x code now. Large parts of the the port look
> totally reasonable for inclusion, the most important complaint is
> about code that shouldn't be there in the first place, because an
> architecture indepedent version already exists.
>

Thank you very much Arnd for the time spent on this review.  It looks 
like very good information.

My general comments are that I agree with your review.  We don't really 
think we are ready for inclusion in the kernel (even -next or -staging 
etc). However we wanted to publish the first reasonable version, even if 
it is a bit messy.

This code does have a history to a 2.6.13 based version so some areas 
probably have some _extra_ cruft.

We are targeting December to February to resolve enough issues to be at 
the point where we can start asking for inclusion.

For the items that don't have a response assume "Thanks, I think I agree 
and we should look into that."

> Here is the summary of my initial findings:
>
> * the system call table is insane, it has all sorts of obsolete crap
>    in it. Just use the generic syscall implementation like arch/tile
>    does. It should also shrink the kernel image significantly.
>
> * The majority of your arch/c6x/include/asm/* header files are totally
>    bogus, just remove them and use the generic versions. Any hardware
>    specific header files should just go into the directory where they
>    are used.
>
> * You obviously need to port to current versions. A 2.6.34 port is
>    rather pointless, the only way forward is to stay up-to-date with
>    the latest git release, at least every -rc release. Just port
>    to 2.6.36-rc5 now and keep merging with the latest updates. The total
>    amount of work will be drastically less than what you get if you
>    only update occasionally. Try to get into -next as soon as some
>    public review is done.

As we get closer to being ready to ask for inclusion we will follow the 
latest updates closer.  Until then the choice to move versions is left 
to the developer responsible for the bulk of the port.  He has 
contractual obligations to TI that he must balance.  I know there is an 
update coming.  We won't stick on .34 but we may not follow every rc 
until we are ready to ask for inclusion.

>
> * It would be helpful to change the git history in a way that each
>    major feature gets a separate commit, starting with preparatory
>    work in architecture-independent code, then adding a minimal
>    arch/c6x tree that compiles but has no other features, and finally
>    adding one driver or feature (ptrace, perf, ...) at a time.

Yes, I presume the "port" patch will be re-factored a number of times as 
it is put in shape.

>
> * It would be good to support the flattened device tree as a way
>    to pass information about the hardware to the kernel, as the
>    microblaze architecture does.

I am personally very interested in the flattened device tree support.  I 
would like to see this supported.  It would be a good way to eliminate 
many of the #ifdefs you talk about below.  However, it is not currently 
an assigned work item.

Do you think this is critical to getting into the mainline or is this 
something we could add later?

>
> * You have a lot of places in the code with a hardware-dependent
>    #ifdef. In other architectures we try to avoid this, in order
>    to let the same kernel boot on all possible hardware configurations.
>    You can combine compile-time and run-time conditionals though,
>    to shrink the code if only one option is enabled.

Agreed, needs work.

>
> * The patches for the TI toolchain won't make it into mainline easily,
>    better support only gcc. This one in particular should be a separate
>    commit in git so you can separate it easily from the code you are
>    going to submit.

Yes, believe me we know.  This is the biggest issue that holds us from 
being "ready" for inclusion.

It is a good idea to segregate the compiler related issues to a patch of 
their own.

>
> * You should run 'sparse' to check pointer annotations and such, many
>    __iomem and __user annotations are missing.
>
> * The PCI support stub is bogus, just kill that
>
> * ptrace should use more of the generic code
>
> * move the device drivers into driver directories, out of arch/,
>    submit them separately through subsystem maintainers

Yes, this will happen over time except for the ones that couple tightly 
to the core like interrupt controller, power managment, clock tree, etc.

>
> * the console driver should be a hvc backend, not a serial backend,
>    that would reduce it by 90% in size.

I was not familiar with the hvc system.  I checked it out. Thanks for 
the pointer.  We also plan to add a virtual console for use between 
cores on a multi core SOC.  This would be a good base for that as well.

I also intend to collapse the code structure of the cio stuff down to 
one file and fix the style etc.  It is a carry over from the structure 
in the TI C runtime library but we should treat this as just a fork.

>
> * The machdep stuff would get easier if you defined a single data
>    structure to hold all the pointers instead of having numerous
>    global pointers.

Your PPC roots are showing :)  In general we look at ARM and PPC for how 
to structure stuff.  Other suggestions welcome.  We knew there would be 
lots of opinions here and wanted to see the discussion.

>
> 	Arnd
> _______________________________________________
> Linux-c6x-dev mailing list
> Linux-c6x-dev at linux-c6x.org
> http://linux-c6x.org/cgi-bin/mailman/listinfo/linux-c6x-dev