[lkml]   [2011]   [May]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [PATCH] arch/c6x: new architecture port for linux
    On Wednesday 11 May 2011 22:13:47 Mark Salter wrote:
    > and the GIT tree holding the attached patches is at:
    > 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

    - 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

    - 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

    - 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.


     \ /
      Last update: 2011-05-21 19:13    [W:4.277 / U:0.052 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site