lkml.org 
[lkml]   [2010]   [May]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] arch/tile: new multi-core architecture for Linux
On 5/24/2010 4:22 PM, Sam Ravnborg wrote:
> Kernle code looked good from a quick browsing.
>

Glad to hear it, and thanks for taking the time to look it over.

> Please explain the need for all the different directories within include/
> {arch, hv, netio}
>

Those three directories are shared with other components of our system.
The "arch" headers are "core architecture" headers which can be used in
any build environment (Linux, hypervisor, user-code, booter, other
"supervisors" like VxWorks, etc.); they are partly small inline hacks to
use the hardware more easily, and partly just lists of name-to-number
mappings for special registers, etc. The "hv" headers are imported from
the hypervisor code; these headers are "owned" by our hypervisor, and
the ones shipped with Linux are the ones that have to do with how to run
a supervisor under our hypervisor. The "netio" headers are another type
of hypervisor header that have to do with interacting with the network
I/O silicon on the chip (the 10 Gbe and 10/100/100 Mb Ethernet).

> There is also several TILE specific options missing the TILE_ prefix.
> Like:
> config XGBE_MAIN
> tristate "Tilera GBE/XGBE character device support"
>
> Drop this:
> config XGBE_MAIN
> tristate "Tilera GBE/XGBE character device support"
>
> It is better to test for the gcc version and disable the option
> only in the cases where it is known to fail.
>

Is the "Drop this" comment a cut and paste bug? I'm guessing you were
referring to CONFIG_WERROR, which enables -Werror support. The problem
is that whether or not you can use -Werror really depends on not just
the kernel version and the gcc version, but very likely also what
drivers you have enabled. We always use it internally. I could also
just pull this out completely (and just force it into "make" externally
within our external build process), or move it to a "generic" configure
option. In any case we can't just automate it, unfortunately.

> Do not mess with CC like this:
> CC = $(CROSS_COMPILE)gcc
>
> I guess you had to do this to support:
> LIBGCC_PATH := `$(CC) -print-libgcc-file-name`
>
> If you follow other archs you could do like this:
> LIBGCC_PATH := `$(CC) -print-libgcc-file-name`
>

I'm guessing you meant like what h8300 does, "$(shell
$(CROSS-COMPILE)$(CC) $(KBUILD_CFLAGS) -print-libgcc-file-name)". That
seems reasonable.

> arch/tile/kernel/Makefile
> I has expected that compiling vmlinux.lds required knowledge on $(BITS)
> like this:
> CPPFLAGS_vmlinux.lds := -m$(BITS)
>

Our 32-bit chips only do 32-bit. In the 64-bit mode we always build the
kernel implicitly -m64, which is the compiler default.

> arch/tile/kernel/vmlinux.lds.S
> A lot of effort has been put into unifying the different
> variants of vmlinux.lds.
> Please see the skeleton outlined in include/asm-generic/vmlinux.lds.h
>

Yes, I've tried to track this somewhat over kernel releases, but I'll go
back and re-examine it with fresh eyes.

> You include hvglue.ld.
> We use *.lds for linker script file - please rename.
> The file looks generated?? How and when?
>

It's sort of a semi-generated file. We have a test in our regressions
that just tests that this file matches the API for our hypervisor, which
is just calls to physical address =32KB plus 64 bytes per syscall
number. These defined addresses are then used for calls to e.g.
hv_flush_asid() or whatever. The hypervisor API changes occasionally,
at which point we update this file. You don't see it used in
vmlinux.lds since it's just used as plain C calls through the arch/tile/
code.

> arch/tile/initramfs:
> Does not look like it belongs in the kernel?
>

Fair enough. We ship it with the kernel to make it easy for our users
to bootstrap up into a plausible initramfs filesystem, but it's strictly
speaking not part of the kernel, so I'll remove it.

> arch/tile/include/asm/spinlock.h
> Please make this a one-liner when you uses the asm-generic version only.
> Same goes for byteorder (which includes linux/byteorder/little_endian.h)
>

I'm not sure what you mean when you say to use the asm-generic version
of spinlock.h, since it's not SMP-ready. Also, I don't see an
asm-generic/byteorder.h, so I'm puzzled there too.

> In your mail you did not say anything about the checkpatch status.
> It is better that you make your code reasonable checkpatch clean _before_
> merging. Then you will not be hit by a lot of janitorial patches doing so.
>

I ran checkpatch over everything I submitted. There are many
complaints, to be sure, but I did a first pass cleaning up everything
that was plausible, so for example all the style issues were fixed, but
things like some uses of volatile, some uses of init_MUTEX, etc., were
not modified. However, I think it's in decent shape from a checkpatch
point of view.

> Likewise please state sparse status. We do not expect it to be sparse clean.
> But getting rid of the obvious issues is good too.
>

I have not run sparse over it. I will do so.

Thanks for your review! Getting this much feedback from LKML is great
-- I really appreciate it.

--
Chris Metcalf, Tilera Corp.
http://www.tilera.com




\
 
 \ /
  Last update: 2010-05-24 23:33    [from the cache]
©2003-2011 Jasper Spaans