Re: [PATCH] arch/c6x: new architecture port for linux

Arnd Bergmann
2011-05-21T17:10:32Z

On Wednesday 11 May 2011 22:13:47 Mark Salter wrote:
> and the GIT tree holding the attached patches is at:
>
> git@linux-c6x.org:/git/projects/linux-c6x-upstreaming.git
>
> TIA for any feedback and/or guidance on this port.
>

I've taken a very brief look at the repository. These
are the things that stood out at first, in no particular
order:

- You need to reduce the size of the defconfig files using
'make gendefconfig'

- arch/c6x/drivers/ should not exist, just move the drivers
into the respective driver hierarchy (drivers/gpio, ...)

- Do not register static platform devices, they are deprecated.
For devices that are always there, use platform_device_create_simple.
For devices that depend on the specific SoC, use a flattened
device tree to describe the hierarchy of devices.

- Replace your board files with device tree files written
in DTS language as on powerpc, microblaze, etc. Hardcoding
board stuff in the kernel is not the way to go for new
architectures, as we are migrating the existing ones away
from that.

- The CIO console driver should be coverted to use hvc_console.

- Any header file that only includes the asm-generic version
can now use generic infrastructure to generate the header,
so you can remove these files.

- arch/c6x/lib/misc.c appears to be misplaced. Why would you
define abort(), exit() and alloca()? Leftover from the
proprietary compiler?

- Do you really need a.out.h?

- your asm/atomic.h and asm/bitops.h look like they should use
the generic version.

- struct clk support is currently undergoing a rewrite, you should
use the new version once it shows up. If it doesn't work for
you, complain now so we can fix it before it gets merged.

- I would guess that you don't support ISA dma, so don't try to use
asm/dma.h.

- gemac.h and gmdio.h should probably live in the same directory
as the driver using them, not in asm/.

- You are rather inconsistent using MMIO pointer access. You should
always use proper accessors like readl/writel, and use sparse
to check that all mmio pointers are marked as __iomem.
Check all uses of 'volatile' in the code, most of them are wrong
and should be '__iomem' instead. See
Documentation/volatile-considered-harmful.txt

- Fix your readl/writel implementations to do the correct casts
from __iomem pointers.

- Do not use __raw_readl/__raw_writel.

- Remove your IO_ADDRESS(), __REG, VULP, __SYSREG and __SYSREGA.
Replace them with proper use of ioremap().

- Your inb/outb implementations are completely wrong. If you don't
support PCI, best just replace them with BUG(). Set IO_SPACE_LIMIT
to zero to enforce that no driver uses them.

- Go through your machdep.h and make sure that all pointers are
actually used, e.g. mach_floppy_eject /sounds/ like you wouldn't
need it in 2011.

- You have an asm/pci.h but no host implementation. Why is that?

- Why do you have both pll.h and clk.h? Don't they do the same thing?

- Use the common asm-generic/unistd.h instead of your own! A significant
number of the syscalls you define are not needed at all. This will
also simplify merging into glibc.

- Rewrite your ptrace.c to use regsets and the common infrastructure
from kernel/ptrace.c.

- I recommend to split your device drivers into separate git branches
in your repository, to make it easier to see what is coming up,
and to allow merging branches at a time. Not required, but it will
make your life easier.

That's all from me for now. When we have resolved these issues, the
next step would be to create a multi-patch series (e.g. ~20 patches
for arch/c6x/*) split into one patch per topic, and review them
on the linux-arch and linux-kernel mailing lists.

Arnd