Date:	Mon, 24 May 2010 17:30:58 -0400
From:	Chris Metcalf <cmetcalf@...era.com>
To:	Sam Ravnborg <sam@...nborg.org>
CC:	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	linux-arch@...r.kernel.org
Subject: Re: [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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists