LinuxLists.cc - [PATCH 1/4] Blackfin: arch patch for 2.6.18

[permalink] [raw]

Subject: Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18

Am Thursday 21 September 2006 05:32 schrieb Luke Yang:
> ? This is the blackfin architecture for 2.6.18, again. As we promised,
> we fixed some issues in our old patches as following.
>
> - use serial core in that driver
>
> - Fix up that ioctl so it a) doesn't sleep in spinlock and b) compiles
>
> - Use generic IRQ framework
>
> - Review all the volatiles, consolidate them in some helper-in-header-file.
>
> ? And we also fixed a lot of other issues and ported it to 2.6.18 now.
> As usual, this architecture patch is too big so I just give a link
> here. Please review it and give you comments, we really appreciate.

I've done a brief review. Overall, it looks fine, but you have lots of
code duplication between your specific machines. It would be good
to generalize that further.

There are a few device drivers in there that should probably be
separate patches.

Most of my review is about your syscall interface, which looks like
you blindly copied many of the warts of other platforms, including
the comments telling you not to do it that way.

It would be good if you can get your code to build cleanly when using
C=1 with sparse, right now that clearly won't work.

In a lot of places your whitespace seems broken. Make sure you don't have
whitespace at the end of a line or spaces instead of tabs for indentation.

Detailed comments follow

> +/*#define IRQ_DEBUG*/
> +#undef IRQ_DEBUG
> +
> +#ifdef IRQ_DEBUG
> +#define IRQ_DPRINTK(x...) printk(x)
> +#else
> +#define IRQ_DPRINTK(x...) do { } while (0)
> +#endif

Use pr_debug here instead of making up your own macros

> +
> +static volatile unsigned long irq_err_count;

volatile doesn't help here. either make it atomic_t if you want it
to be safe or don't bother

> +
> +#define DEBUGP(fmt...)
> +

pr_debug

> +static uint32_t reloc_stack_operate(unsigned int oper, struct module *mod)
> +{
> + uint32_t value;
> + switch (oper) {
> + case R_add:
> + {
> + value =
> + reloc_stack[reloc_stack_tos - 2] +
> + reloc_stack[reloc_stack_tos - 1];
> + reloc_stack_tos -= 2;
> + break;
> + }

no need for the curly braces here and below

> +inline void static leds_switch(int flag);

static inline void

Note that marking the function inline doesn't help if the definition comes
after the first use.

> +inline static void default_idle(void)

static inline void default_idle(void)

> +{
> + while (!need_resched()) {
> + leds_switch(LED_OFF);
> + __asm__("nop;\n\t \
> + nop;\n\t \
> + nop;\n\t \
> + idle;\n\t": : :"cc");
> + leds_switch(LED_ON);
> + }
> +}
> +

This looks racy. What if you get an interrupt after testing need_resched()
but before the idle instruction?

Normally, this should look like

while(!need_resched()) {
local_irq_disable();
if (!need_resched())
asm volatile("idle");
local_irq_enable();
}

Of course that only works if your idle instruction wakes up on pending
interrupts.

> +/*
> + * sys_execve() executes a new program.
> + */
> +
> +asmlinkage int sys_execve(char *name, char **argv, char **envp)
> +{
> + int error;
> + char *filename;
> + struct pt_regs *regs = (struct pt_regs *)((&name) + 6);
> +
> + lock_kernel();
> + filename = getname(name);
> + error = PTR_ERR(filename);
> + if (IS_ERR(filename))
> + goto out;
> + error = do_execve(filename, argv, envp, regs);
> + putname(filename);
> + out:
> + unlock_kernel();
> + return error;
> +}

You also need a kernel_execve() function because of changes in -mm.

> +
> + switch (request) {
> + /* when I and D space are separate, these will need to be fixed. */
> + case PTRACE_PEEKDATA:
> +#ifdef DEBUG
> + printk("PTRACE_PEEKDATA\n");
> +#endif

pr_debug again, here and elsewhere in ptrace.c

> + if (is_user_addr_valid(child, addr + add, sizeof(tmp)) <
> + 0)
> + break;
> +
> + copied =
> + access_process_vm(child, addr + add, &tmp,
> + sizeof(tmp), 0);

interesting indentation ;-). Maybe you should move some of these into separate
functions if you run out of horizontal space.

+
> +/*
> + * Atomically swap in the new signal mask, and wait for a signal.
> + *
> + */
> +asmlinkage int sys_sigsuspend(old_sigset_t mask)
> +{

> +
> +asmlinkage int
> +sys_sigaction(int sig, const struct old_sigaction *act,
> + struct old_sigaction *oact)
> +{

> +
> +asmlinkage int sys_sigaltstack(const stack_t * uss, stack_t * uoss)
> +{

> +static inline int
> +restore_sigcontext(struct pt_regs *regs, struct sigcontext *usc, int *pr0)
> +{

Do yourself a favour and kill the old-style signal infrastructure. You only
need
the sys_rt_* variant of these, and they are provided in common code.

> +asmlinkage long sys_mmap2(unsigned long addr, unsigned long len,
> + unsigned long prot, unsigned long flags,
> + unsigned long fd, unsigned long pgoff)
> +{
> + return do_mmap2(addr, len, prot, flags, fd, pgoff);
> +}
> +
> +asmlinkage int sys_mmap(unsigned long addr, unsigned long len,
> + unsigned long prot, unsigned long flags,
> + unsigned long fd, unsigned long pgoff)
> +{

No need for sys_mmap _and sys_mmap2, better handle this in libc.

> + * Perform the select(nd, in, out, ex, tv) and mmap() system
> + * calls. Linux/bfin cloned Linux/i386, which didn't use to be able to
> + * handle more than 4 system call parameters, so these system calls
> + * used a memory block for parameter passing..
> + */
> +
> +struct sel_arg_struct {
> + unsigned long n;
> + fd_set *inp, *outp, *exp;
> + struct timeval *tvp;
> +};
> +
> +asmlinkage int old_select(struct sel_arg_struct *arg)
> +{

This sounds like a weird argumentation. Since you're defining a syscall
ABI, just make it use six argument registers instead of copying a
hack that was a bad idea to start with.

> + *
> + * This is really horribly ugly.
> + */
> +asmlinkage int
> +sys_ipc(uint call, int first, int second, int third, void *ptr, long fifth)

If it's so ugly then don't copy it!

sys_ipc is not needed at all, just add sys_semop/sys_semget/... to your
sys_call_table directly.

> +{
> + int version, ret;
> +
> + version = call >> 16; /* hack for backward compatibility */
> + call &= 0xffff;

Especially since I hope you don't need compatibility to XENIX binaries ;-)

+
> +#ifndef TRAPS_DEBUG
> +# define TRAPS_DEBUG 0
> +#endif
> +
> +#if (TRAPS_DEBUG > 2 )
> +# define DPRINTK3(args...) printk(KERN_DEBUG args)
> +#else
> +# define DPRINTK3(args...)
> +#endif
> +
> +#if (TRAPS_DEBUG > 1)
> +# define DPRINTK2(args...) printk(KERN_DEBUG args)
> +#else
> +# define DPRINTK2(args...)
> +#endif
> +
> +#ifdef TRAPS_DEBUG
> +# define DPRINTK1(args...) printk(KERN_DEBUG args)
> +#else
> +# define DPRINTK1(args...)
> +#endif
> +

And yet another set of macros you don't need.

> +#if defined(CONFIG_SPI_BFIN) || defined(CONFIG_SPI_BFIN_MODULE)
> +/* all SPI perpherals info goes here */
> +
> +static struct mtd_partition bfin_spi_flash_partitions[] = {
> + {
> + name:"bootloader",
> + size:0x00040000,
> + offset:0,
> + mask_flags:MTD_CAP_ROM}, {
> + name: "kernel",
> + size: 0xc0000,
> + offset: 0x40000}, {
> + name: "file system",
> + size: 0x300000,
> + offset: 0x00100000,
> + }
> +};

It would be nice if you could use a generic way to pass this partition data
to the kernel from the mtd medium, instead of hardcoding it here.

> +/*
> + * When we call pm_suspend, that code enters into idle state.
> + * When there is any interrupt,the core will resume
> + */
> +void bf533_pm_suspend(void)
> +{
> + unsigned long flags;
> +
> + /*FIXME: Add a useful Power Saving Mode Here ... */
> +
> + local_irq_save(flags);
> + bfin_write_SIC_IWR(IWR_ENABLE_ALL);
> + __builtin_bfin_ssync();
> + asm("IDLE;");
> + local_irq_restore(flags);
> +
> +}

I guess you should check for need_resched() here, before calling IDLE.

> + enable_dma(CH_MEM_STREAM2_SRC);
> + enable_dma(CH_MEM_STREAM2_DEST);
> +
> + interruptible_sleep_on(&coreb_dma_wait);
> +
> + disable_dma(CH_MEM_STREAM2_SRC);
> + disable_dma(CH_MEM_STREAM2_DEST);

never ever use sleep_on() in new code. wait_event_interruptible is what you
want instead.

> +
> +static struct file_operations coreb_fops = {
> + owner:THIS_MODULE,
> + llseek:coreb_lseek,
> + read:coreb_read,
> + write:coreb_write,
> + ioctl:coreb_ioctl,
> + open:coreb_open,
> + release:coreb_release
> +};

Use C99 initializers here, like

.owner = THIS_MODULE,

Actually, since you did not fix this already, I assume you haven't run all of
your code through sparse yet. It's a good idea to run sparse on anthing you
intend for inclusion and then fix the warnings you get.

> + printk(KERN_INFO "Core B: Initializing /proc\n");
> + coreb_proc_entry = create_proc_entry("coreb", 0, NULL);
> + if (coreb_proc_entry) {
> + coreb_proc_entry->owner = THIS_MODULE;
> + coreb_proc_entry->read_proc = coreb_read_status;
> + } else {
> + printk(KERN_ERR "Core B: Unable to register /proc/coreb\n");
> + goto release_dma_src;
> + }

Errm, no.

You should not add random files to procfs. Since you already register a misc
device for this, you have a node in /sys where you can add attributes for
the various data you want to export

> +static char *cplb_print_entry(char *buf, int type)
> +{
> + unsigned long *p_addr = dpdt_table;
> + unsigned long *p_data = dpdt_table + 1;
> + unsigned long *p_icount = dpdt_swapcount_table;
> + unsigned long *p_ocount = dpdt_swapcount_table + 1;
> + unsigned long *cplb_addr = (unsigned long *)DCPLB_ADDR0;
> + unsigned long *cplb_data = (unsigned long *)DCPLB_DATA0;
> + int entry, used_cplb = 0;
> +
> + if (type == CPLB_I) {
> + buf += sprintf(buf, "Instrction CPLB entry:\n");
> + p_addr = ipdt_table;
> + p_data = ipdt_table + 1;
> + p_icount = ipdt_swapcount_table;
> + p_ocount = ipdt_swapcount_table + 1;
> + cplb_addr = (unsigned long *)ICPLB_ADDR0;
> + cplb_data = (unsigned long *)ICPLB_DATA0;
> + } else
> + buf += sprintf(buf, "Data CPLB entry:\n");
> +
> + buf += sprintf(buf, "Address\t\tData\tSize\tValid\tLocked\tSwapin\
> +\tiCount\toCount\n");
> +
> + while (*p_addr != 0xffffffff) {
> + entry = cplb_find_entry(cplb_addr, cplb_data, *p_addr);
> + if (entry >= 0 && *p_data == cplb_data[entry])
> + used_cplb |= 1 << entry;
> +
> + buf +=
> + sprintf(buf,
> + "0x%08lx\t0x%05lx\t%s\t%c\t%c\t%2d\t%ld\t%ld\n",
> + *p_addr, *p_data,
> + page_size_string_table[(*p_data & 0x30000) >> 16],
> + (*p_data & CPLB_VALID) ? 'Y' : 'N',
> + (*p_data & CPLB_LOCK) ? 'Y' : 'N', entry, *p_icount,
> + *p_ocount);
> +
> + p_addr += 2;
> + p_data += 2;
> + p_icount += 2;
> + p_ocount += 2;
> + }
> +
> + if (used_cplb != 0xffff) {
> + buf += sprintf(buf, "Unused/mismatched CPLBs:\n");
> +
> + for (entry = 0; entry < 16; entry++)
> + if (0 == ((1 << entry) & used_cplb)) {
> + int flags = cplb_data[entry];
> + buf +=
> + sprintf(buf,
> + "%2d: 0x%08lx\t0x%05x\t%s\t%c\t%c\n",
> + entry, cplb_addr[entry], flags,
> + page_size_string_table[(flags &
> + 0x30000) >>
> + 16],
> + (flags & CPLB_VALID) ? 'Y' : 'N',
> + (flags & CPLB_LOCK) ? 'Y' : 'N');
> + }
> + }
> +
> + buf += sprintf(buf, "\n");
> +
> + return buf;
> +}

Another one of those files that just don't belong in procfs. Find some other
interface for this.

> +
> +ALIGN
> +ENTRY(_sys_call_table)
> + .long _sys_ni_syscall /* 0 - old "setup()" system call*/
> + .long _sys_exit

There is not much point in trying to use the same numbers as an existing
architecture if that means that you have to leave holes like setup().
I don't know if you still have the choice of completely changing the
syscall numbers, but it would make it nicer in the future.

> + .long _sys_waitpid

kill, just use wait4

> + .long _sys_chown16

kill all 16 bit uid syscalls

> + .long _sys_stat

use stat64 instead

> + .long _sys_lseek

use llseek instead

> + .long _sys_mount
> + .long _sys_oldumount

sys_mount is enough

> + .long _sys_setuid16
> + .long _sys_getuid16

no uid16 syscalls please, uid_t is __u32

> + .long _sys_utime /* 30 */

sys_utimes

> + .long _sys_setgid16
> + .long _sys_getgid16
> + .long _sys_geteuid16
> + .long _sys_getegid16 /* 50 */

uit32

> + .long _sys_sigaction

use rt signals

> + .long _sys_setreuid16 /* 70 */
> + .long _sys_setregid16

uid32

> + .long _sys_sigsuspend
> + .long _sys_sigpending

rt signals

> + .long _sys_setrlimit /* 75 */
> + .long _sys_old_getrlimit

sys_getrlimit

> + .long _sys_getgroups16 /* 80 */
> + .long _sys_setgroups16

uid32

> + .long _old_select

kill

> + .long _sys_lstat

lstat64

> + .long _sys_uselib

kill
> + .long _old_readdir

kill

> + .long _sys_mmap /* 90 */

mmap2

> + .long _sys_truncate
> + .long _sys_ftruncate

{f,}truncate64

> + .long _sys_fchown16 /* 95 */

uid32

> + .long _sys_socketcall

Replace this with the actual socket syscalls. sys_socketcall must
go the way of sys_ipc

> + .long _sys_newstat
> + .long _sys_newlstat
> + .long _sys_newfstat

see above. No need for three versions of stat.

> + .long _sys_ipc

as mentioned

> + .long _sys_sigreturn

rt signals

> + .long _sys_sysfs /* 135 */

just kill this

> + .long _sys_setfsuid16
> + .long _sys_setfsgid16

uid32

> + .long _sys_setresuid16
> + .long _sys_getresuid16 /* 165 */

uid32

> + .long _sys_poll

I guess we can now replace select and poll with pselect and ppoll
in new architectures.

> + .long _sys_setresgid16 /* 170 */
> + .long _sys_getresgid16

uid32

> +static __inline__ void atomic_add(int i, atomic_t * v)
> +{
> + int __temp = 0;
> + __asm__ __volatile__(
> + "cli R3;\n\t"
> + "%0 = %1;\n\t"
> + "%0 = %0 + %2;\n\t"
> + "%1 = %0;\n\t"
> + "sti R3;\n\t"
> + : "=d" (__temp), "=m" (v->counter)
> + :"d"(i), "m"(v->counter), "0"(__temp)
> + :"R3");
> +}

Can't you let the compile choose a register instead of forcing it to
use R3?

>From what I can tell, doing this in C as

long flags;
local_irq_save(flags);
v->val += i;
local_irq_restore(flags);

should result in better code than this

> diff -urN linux-2.6.18/include/asm-blackfin/blackfin.h
linux-2.6.18.patch1/include/asm-blackfin/blackfin.h
> --- linux-2.6.18/include/asm-blackfin/blackfin.h 1970-01-01
08:00:00.000000000 +0800
> +++ linux-2.6.18.patch1/include/asm-blackfin/blackfin.h 2006-09-21
09:29:49.000000000 +0800
> @@ -0,0 +1,13 @@
> +/*
> + * Common header file for blackfin family of processors.
> + *
> + */
> +
> +#ifndef _BLACKFIN_H_
> +#define _BLACKFIN_H_
> +
> +#include <asm/macros.h>
> +#include <asm/mach/blackfin.h>
> +#include <asm/bfin-global.h>
> +
> +#endif /* _BLACKFIN_H_ */
> diff -urN linux-2.6.18/include/asm-blackfin/board/eagle.h
linux-2.6.18.patch1/include/asm-blackfin/board/eagle.h
> --- linux-2.6.18/include/asm-blackfin/board/eagle.h 1970-01-01
08:00:00.000000000 +0800
> +++ linux-2.6.18.patch1/include/asm-blackfin/board/eagle.h 2006-09-21
09:29:49.000000000 +0800
> @@ -0,0 +1,4 @@
> +#ifndef _EAGLE_H_
> +#define _EAGLE_H_
> +
> +#endif /* _EAGLE_H_ */
> diff -urN linux-2.6.18/include/asm-blackfin/board/ezkit.h
linux-2.6.18.patch1/include/asm-blackfin/board/ezkit.h
> --- linux-2.6.18/include/asm-blackfin/board/ezkit.h 1970-01-01
08:00:00.000000000 +0800
> +++ linux-2.6.18.patch1/include/asm-blackfin/board/ezkit.h 2006-09-21
09:29:49.000000000 +0800
> @@ -0,0 +1,4 @@
> +#ifndef _EZKIT_H_
> +#define _EZKIT_H_
> +
> +#endif /* _EZKIT_H_ */
> diff -urN linux-2.6.18/include/asm-blackfin/board/hawk.h
linux-2.6.18.patch1/include/asm-blackfin/board/hawk.h
> --- linux-2.6.18/include/asm-blackfin/board/hawk.h 1970-01-01
08:00:00.000000000 +0800
> +++ linux-2.6.18.patch1/include/asm-blackfin/board/hawk.h 2006-09-21
09:29:49.000000000 +0800
> @@ -0,0 +1,4 @@
> +#ifndef _HAWK_H_
> +#define _HAWK_H_
> +
> +#endif /* _HAWK_H_ */

What's th point of these files, can't you just remove them all?

> diff -urN linux-2.6.18/include/asm-blackfin/bug.h
linux-2.6.18.patch1/include/asm-blackfin/bug.h
> --- linux-2.6.18/include/asm-blackfin/bug.h 1970-01-01 08:00:00.000000000
+0800
> +++ linux-2.6.18.patch1/include/asm-blackfin/bug.h 2006-09-21
09:29:49.000000000 +0800
> @@ -0,0 +1,14 @@
> +#ifndef _BLACKFIN_BUG_H
> +#define _BLACKFIN_BUG_H
> +
> +#ifdef CONFIG_BUG
> +#define HAVE_ARCH_BUG
> +#define BUG() do { \
> + dump_stack(); \
> + printk("\nkernel BUG at %s:%d!\n", __FILE__, __LINE__); \
> + panic("BUG!"); \
> +} while (0)
> +#endif
> +

This is probably better done as an external function:

extern void __BUG(const char *file, int line) __attribute__((noreturn));
#define BUG() __BUG(__FILE__, __LINE__)

> diff -urN linux-2.6.18/include/asm-blackfin/cplbtab.h
linux-2.6.18.patch1/include/asm-blackfin/cplbtab.h
> --- linux-2.6.18/include/asm-blackfin/cplbtab.h 1970-01-01
08:00:00.000000000 +0800
> +++ linux-2.6.18.patch1/include/asm-blackfin/cplbtab.h 2006-09-21
09:29:49.000000000 +0800
> @@ -0,0 +1,572 @@
> +/*This file is subject to the terms and conditions of the GNU General
Public
> + * License.
> + *
> + * Blackfin BF533/2.6 support : LG Soft India
> + * Updated : Ashutosh Singh / Jahid Khan : Rrap Software Pvt Ltd
> + * Updated : 1. SDRAM_KERNEL, SDRAM_DKENEL are added as initial cplb's
> + * shouldn't be victimized. cplbmgr.S search logic is corrected
> + * to findout the appropriate victim.
> + * 2. SDRAM_IGENERIC in dpdt_table is replaced with SDRAM_DGENERIC
> + * : LG Soft India
> + */
> +
> +#ifndef __ARCH_BLACKFIN_CPLBTAB_H
> +#define __ARCH_BLACKFIN_CPLBTAB_H
> +
> +/*************************************************************************
> + * ICPLB TABLE
> + *************************************************************************/
> +
> +.data
> +/* This table is configurable */
> + .align 4;
> +
> +.global _icplb_table
> +_icplb_table:
> +.byte4 0x00000000;
> +.byte4 0x00000000;
> +.byte4 0x00000000;
> +.byte4 0x00000000;

This does not look like it belongs into a header file.

> diff -urN linux-2.6.18/include/asm-blackfin/entry.h
linux-2.6.18.patch1/include/asm-blackfin/entry.h
> --- linux-2.6.18/include/asm-blackfin/entry.h 1970-01-01 08:00:00.000000000
+0800
> +++ linux-2.6.18.patch1/include/asm-blackfin/entry.h 2006-09-21
09:29:49.000000000 +0800
> @@ -0,0 +1,367 @@
> +#ifndef __BFIN_ENTRY_H
> +#define __BFIN_ENTRY_H
> +
> +#include <asm/setup.h>
> +#include <asm/page.h>
> +
> +#ifdef __ASSEMBLY__
> +
> +#define LFLUSH_I_AND_D 0x00000808
> +#define LSIGTRAP 5
> +
> +/* process bits for task_struct.flags */
> +#define PF_TRACESYS_OFF 3
> +#define PF_TRACESYS_BIT 5
> +#define PF_PTRACED_OFF 3
> +#define PF_PTRACED_BIT 4
> +#define PF_DTRACE_OFF 1
> +#define PF_DTRACE_BIT 5
> +
> +/* This one is used for exceptions, emulation, and NMI. It doesn't push
> + RETI and doesn't do cli. */
> +#define SAVE_ALL_SYS save_context_no_interrupts
> +/* This is used for all normal interrupts. It saves a minimum of registers
> + to the stack, loads the IRQ number, and jumps to common code. */
> +#define INTERRUPT_ENTRY(N) \
> + [--sp] = SYSCFG; \
> + \
> + [--sp] = P0; /*orig_p0*/ \
> + [--sp] = R0; /*orig_r0*/ \
> + [--sp] = (R7:0,P5:0); \
> + R0 = (N); \
> + jump __common_int_entry;
> +
> +/* For timer interrupts, we need to save IPEND, since the user_mode
> + macro accesses it to determine where to account time. */
> +#define TIMER_INTERRUPT_ENTRY(N) \
> + [--sp] = SYSCFG; \
> + \
> + [--sp] = P0; /*orig_p0*/ \
> + [--sp] = R0; /*orig_r0*/ \
> + [--sp] = (R7:0,P5:0); \
> + p0.l = lo(IPEND); \
> + p0.h = hi(IPEND); \
> + r1 = [p0]; \
> + R0 = (N); \
> + jump __common_int_entry;
> +
> +/* This one pushes RETI without using CLI. Interrupts are enabled. */
> +#define SAVE_CONTEXT_SYSCALL save_context_syscall
> +#define SAVE_CONTEXT save_context_with_interrupts
> +
> +#define RESTORE_ALL_SYS restore_context_no_interrupts
> +#define RESTORE_CONTEXT restore_context_with_interrupts
> +
> +/*
> + * Code to save processor context.
> + * We even save the register which are preserved by a function call
> + * - r4, r5, r6, r7, p3, p4, p5
> + */
> +.macro save_context_with_interrupts
> + [--sp] = SYSCFG;
> +
> + [--sp] = P0; /*orig_p0*/
> + [--sp] = R0; /*orig_r0*/

same here

> diff -urN linux-2.6.18/include/asm-blackfin/fcntl.h
linux-2.6.18.patch1/include/asm-blackfin/fcntl.h
> --- linux-2.6.18/include/asm-blackfin/fcntl.h 1970-01-01 08:00:00.000000000
+0800
> +++ linux-2.6.18.patch1/include/asm-blackfin/fcntl.h 2006-09-21
09:29:49.000000000 +0800
> @@ -0,0 +1,87 @@
> +#ifndef _BFIN_FCNTL_H
> +#define _BFIN_FCNTL_H
> +
> +/* open/fcntl - O_SYNC is only implemented on blocks devices and on files
> + located on an ext2 file system */
> +#define O_ACCMODE 0003
> +#define O_RDONLY 00
> +#define O_WRONLY 01

Please use asm-generic/fcntl.h

> diff -urN linux-2.6.18/include/asm-blackfin/ioctls.h
linux-2.6.18.patch1/include/asm-blackfin/ioctls.h
> --- linux-2.6.18/include/asm-blackfin/ioctls.h 1970-01-01 08:00:00.000000000
+0800
> +++ linux-2.6.18.patch1/include/asm-blackfin/ioctls.h 2006-09-21
09:29:49.000000000 +0800
> @@ -0,0 +1,82 @@
> +#ifndef __ARCH_BFIN_IOCTLS_H__
> +#define __ARCH_BFIN_IOCTLS_H__
> +
> +#include <asm/ioctl.h>
> +
> +/* 0x54 is just a magic number to make these relatively unique ('T') */
> +
> +#define TCGETS 0x5401
> +#define TCSETS 0x5402
> +#define TCSETSW 0x5403
> +#define TCSETSF 0x5404
> +#define TCGETA 0x5405
> +#define TCSETA 0x5406
> +#define TCSETAW 0x5407
> +#define TCSETAF 0x5408
> +#define TCSBRK 0x5409
> +#define TCXONC 0x540A
> +#define TCFLSH 0x540B
> +#define TIOCEXCL 0x540C
> +#define TIOCNXCL 0x540D
> +#define TIOCSCTTY 0x540E
> +#define TIOCGPGRP 0x540F
> +#define TIOCSPGRP 0x5410
> +#define TIOCOUTQ 0x5411
> +#define TIOCSTI 0x5412
> +#define TIOCGWINSZ 0x5413
> +#define TIOCSWINSZ 0x5414
> +#define TIOCMGET 0x5415
> +#define TIOCMBIS 0x5416
> +#define TIOCMBIC 0x5417
> +#define TIOCMSET 0x5418
> +#define TIOCGSOFTCAR 0x5419
> +#define TIOCSSOFTCAR 0x541A
> +#define FIONREAD 0x541B
> +#define TIOCINQ FIONREAD
> +#define TIOCLINUX 0x541C
> +#define TIOCCONS 0x541D
> +#define TIOCGSERIAL 0x541E
> +#define TIOCSSERIAL 0x541F
> +#define TIOCPKT 0x5420
> +#define FIONBIO 0x5421
> +#define TIOCNOTTY 0x5422
> +#define TIOCSETD 0x5423
> +#define TIOCGETD 0x5424
> +#define TCSBRKP 0x5425 /* Needed for POSIX tcsendbreak() */
> +#define TIOCTTYGSTRUCT 0x5426 /* For debugging only */
> +#define TIOCSBRK 0x5427 /* BSD compatibility */
> +#define TIOCCBRK 0x5428 /* BSD compatibility */
> +#define TIOCGSID 0x5429 /* Return the session ID of FD */
> +#define TIOCGPTN _IOR('T',0x30, unsigned int) /* Get Pty Number (of pty-mux
device) */
> +#define TIOCSPTLCK _IOW('T',0x31, int) /* Lock/unlock Pty */
> +
> +#define FIONCLEX 0x5450 /* these numbers need to be adjusted. */
> +#define FIOCLEX 0x5451
> +#define FIOASYNC 0x5452
> +#define TIOCSERCONFIG 0x5453
> +#define TIOCSERGWILD 0x5454
> +#define TIOCSERSWILD 0x5455
> +#define TIOCGLCKTRMIOS 0x5456
> +#define TIOCSLCKTRMIOS 0x5457
> +#define TIOCSERGSTRUCT 0x5458 /* For debugging only */
> +#define TIOCSERGETLSR 0x5459 /* Get line status register */
> +#define TIOCSERGETMULTI 0x545A /* Get multiport config */
> +#define TIOCSERSETMULTI 0x545B /* Set multiport config */
> +
> +#define TIOCMIWAIT 0x545C /* wait for a change on serial input line(s) */
> +#define TIOCGICOUNT 0x545D /* read serial port inline interrupt counts */
> +
> +#define FIOQSIZE 0x545E
> +
> +/* Used for packet mode */
> +#define TIOCPKT_DATA 0
> +#define TIOCPKT_FLUSHREAD 1
> +#define TIOCPKT_FLUSHWRITE 2
> +#define TIOCPKT_STOP 4
> +#define TIOCPKT_START 8
> +#define TIOCPKT_NOSTOP 16
> +#define TIOCPKT_DOSTOP 32
> +
> +#define TIOCSER_TEMT 0x01 /* Transmitter physically empty */

Can't we ever get a proper generic file for these?

> + * These are used to wrap system calls on bfin.
> + *
> + * See arch/blackfin/kernel/sys_bfin.c for ugly details..
> + */
> +struct ipc_kludge {
> + struct msgbuf *msgp;
> + long msgtyp;
> +};
> +
> +#define SEMOP 1
> +#define SEMGET 2
> +#define SEMCTL 3
> +#define MSGSND 11
> +#define MSGRCV 12
> +#define MSGGET 13
> +#define MSGCTL 14
> +#define SHMAT 21
> +#define SHMDT 22
> +#define SHMGET 23
> +#define SHMCTL 24
> +
> +/* Used by the DIPC package, try and avoid reusing it */
> +#define DIPC 25
> +
> +#define IPCCALL(version,op) ((version)<<16 | (op))
> +
> +#endif

As mentioned, kill these comletely

> diff -urN linux-2.6.18/include/asm-blackfin/mach-bf533/anomaly.h
linux-2.6.18.patch1/include/asm-blackfin/mach-bf533/anomaly.h
> --- linux-2.6.18/include/asm-blackfin/mach-bf533/anomaly.h 1970-01-01
08:00:00.000000000 +0800
> +++ linux-2.6.18.patch1/include/asm-blackfin/mach-bf533/anomaly.h 2006-09-21
09:29:49.000000000 +0800
> @@ -0,0 +1,172 @@
> +/*
> + * File: include/asm-blackfin/mach-bf533/anomaly.h

You seem to have lots of these machine specfic header files in include/asm.
Please move them to the respective machine implementation directory
if they are only used from there

> +/* Clock and System Control (0xFFC0 0400-0xFFC0 07FF) */
> +#define bfin_read_PLL_CTL() bfin_read16(PLL_CTL)
> +#define bfin_write_PLL_CTL(val) bfin_write16(PLL_CTL,val)
> +#define bfin_read_PLL_STAT() bfin_read16(PLL_STAT)
(and 700 more of these)

What's the point, are you getting paid by lines of code? Just use
the registers directly!

> +/* Clock and System Control (0xFFC00000 - 0xFFC000FF) */
> +#define bfin_read_PLL_CTL() bfin_read16(PLL_CTL)
> +#define bfin_write_PLL_CTL(val) bfin_write16(PLL_CTL,val)
> +#define bfin_read_PLL_DIV() bfin_read16(PLL_DIV)
> +#define bfin_write_PLL_DIV(val) bfin_write16(PLL_DIV,val)

Here we go again. You have a serious copy-paste problem. If the machines
need the same code, just make that common to them

> +typedef unsigned short __kernel_ipc_pid_t;
> +typedef unsigned short __kernel_uid_t;
> +typedef unsigned short __kernel_gid_t;

Why that? Please make these int

> +typedef unsigned short __kernel_uid16_t;
> +typedef unsigned short __kernel_gid16_t;

and kill these

> diff -urN linux-2.6.18/include/asm-blackfin/stat.h
linux-2.6.18.patch1/include/asm-blackfin/stat.h
> --- linux-2.6.18/include/asm-blackfin/stat.h 1970-01-01 08:00:00.000000000
+0800
> +++ linux-2.6.18.patch1/include/asm-blackfin/stat.h 2006-09-21
09:29:49.000000000 +0800
> @@ -0,0 +1,77 @@
> +#ifndef _BFIN_STAT_H
> +#define _BFIN_STAT_H
> +
> +struct __old_kernel_stat {
> + unsigned short st_dev;
> + unsigned short st_ino;
> + unsigned short st_mode;
> + unsigned short st_nlink;
> + unsigned short st_uid;
> + unsigned short st_gid;
> + unsigned short st_rdev;
> + unsigned long st_size;
> + unsigned long st_atime;
> + unsigned long st_mtime;
> + unsigned long st_ctime;
> +};
> +
> +struct stat {
> + unsigned short st_dev;
> + unsigned short __pad1;
> + unsigned long st_ino;
> + unsigned short st_mode;
> + unsigned short st_nlink;
> + unsigned short st_uid;
> + unsigned short st_gid;
> + unsigned short st_rdev;
> + unsigned short __pad2;
> + unsigned long st_size;
> + unsigned long st_blksize;
> + unsigned long st_blocks;
> + unsigned long st_atime;
> + unsigned long __unused1;
> + unsigned long st_mtime;
> + unsigned long __unused2;
> + unsigned long st_ctime;
> + unsigned long __unused3;
> + unsigned long __unused4;
> + unsigned long __unused5;
> +};
> +
> +/* This matches struct stat64 in glibc2.1, hence the absolutely
> + * insane amounts of padding around dev_t's.
> + */
> +struct stat64 {
> + unsigned long long st_dev;
> + unsigned char __pad1[4];

One of these three should really be enough for a new architecture, as
mentioned
before.

> +
> +#define STAT64_HAS_BROKEN_ST_INO 1
> + unsigned long __st_ino;

Then why didn't you fix it?

> +/* Dma addresses are 32-bits wide. */
> +
> +typedef u32 dma_addr_t;
> +typedef u32 dma64_addr_t;

Shouldn't the dma64_addr_t be u64?

+
> +#define __syscall_return(type, res) \
> +do { \
> + if ((unsigned long)(res) >= (unsigned long)(-125)) \
> + { errno = -(res); \
> + res = -1; \
> + } \
> + return (type) (res); \
> +} while (0)
> +
> +#define _syscall0(type,name) \
> +type name(void) { \
> + long __res; \
> + __asm__ __volatile__ ( \
> + "p0 = %1;\n\t" \
> + "excpt 0;\n\t" \
> + "%0=r0;\n\t" \
> + : "=da" (__res) \
> + : "i" (__NR_##name) \
> + : "memory","CC","R0","P0"); \
> + __syscall_return(type,__res); \
> +}

You can prepare to kill these macros again in 2.6.19

> +
> +#ifdef __KERNEL__
> +#define __ARCH_WANT_IPC_PARSE_VERSION
> +#define __ARCH_WANT_OLD_READDIR
> +#define __ARCH_WANT_OLD_STAT
> +#define __ARCH_WANT_STAT64
> +#define __ARCH_WANT_SYS_ALARM
> +#define __ARCH_WANT_SYS_GETHOSTNAME
> +#define __ARCH_WANT_SYS_PAUSE
> +#define __ARCH_WANT_SYS_SGETMASK
> +#define __ARCH_WANT_SYS_SIGNAL
> +#define __ARCH_WANT_SYS_TIME
> +#define __ARCH_WANT_SYS_UTIME
> +#define __ARCH_WANT_SYS_WAITPID
> +#define __ARCH_WANT_SYS_SOCKETCALL
> +#define __ARCH_WANT_SYS_FADVISE64
> +#define __ARCH_WANT_SYS_GETPGRP
> +#define __ARCH_WANT_SYS_LLSEEK
> +#define __ARCH_WANT_SYS_NICE
> +#define __ARCH_WANT_SYS_OLD_GETRLIMIT
> +#define __ARCH_WANT_SYS_OLDUMOUNT
> +#define __ARCH_WANT_SYS_SIGPENDING
> +#define __ARCH_WANT_SYS_SIGPROCMASK
> +#define __ARCH_WANT_SYS_RT_SIGACTION
> +#define __ARCH_WANT_SYS_RT_SIGSUSPEND
> +#endif

You should not need to define most of these

> +
> +#ifdef __KERNEL_SYSCALLS__
> +
> +#include <linux/interrupt.h>
> +
> +#define __NR__exit __NR_exit
> +#if 0
> +static inline _syscall0(int, pause)
> +static inline _syscall0(int, sync)
> +static inline _syscall0(pid_t, setsid)
> +static inline _syscall3(int, write, int, fd, const char *, buf, off_t,
count)
> +static inline _syscall3(int, read, int, fd, char *, buf, off_t, count)
> +static inline _syscall3(off_t, lseek, int, fd, off_t, offset, int, count)
> +static inline _syscall1(int, dup, int, fd)
> +//static inline _syscall3(int,execve,const char *,file,char **,argv,char
**,envp)
> +static inline _syscall3(int, open, const char *, file, int, flag, int,
mode)
> +static inline _syscall1(int, close, int, fd)
> +static inline _syscall1(int, _exit, int, exitcode)
> +static inline _syscall3(pid_t, waitpid, pid_t, pid, int *, wait_stat, int,
> + options)
> +static inline _syscall1(int, delete_module, const char *, name)
> +
> +static inline pid_t wait(int *wait_stat)
> +{
> + return waitpid(-1, wait_stat, 0);
> +}
> +#endif
> +
> +asmlinkage long execve(char *, char **, char **);
> +
> +asmlinkage long sys_mmap2(unsigned long addr, unsigned long len,
> + unsigned long prot, unsigned long flags,
> + unsigned long fd, unsigned long pgoff);
> +asmlinkage int sys_execve(char *name, char **argv, char **envp);
> +asmlinkage int sys_pipe(unsigned long *fildes);
> +struct pt_regs;
> +int sys_request_irq(unsigned int,
> + irqreturn_t(*)(int, void *, struct pt_regs *),
> + unsigned long, const char *, void *);
> +void sys_free_irq(unsigned int, void *);
> +struct sigaction;
> +asmlinkage long sys_rt_sigaction(int sig,
> + const struct sigaction __user * act,
> + struct sigaction __user * oact,
> + size_t sigsetsize);
> +
> +asmlinkage void *sys_sram_alloc(size_t size, unsigned long flags);
> +asmlinkage int sys_sram_free(const void *addr);
> +asmlinkage void *sys_dma_memcpy(void *dest, const void *src, size_t len);
> +
> +#endif /* __KERNEL_SYSCALLS__ */

And these should definitely go away. execve is the only one that has been
required for ages and that gets removed now.

One more thing about the headers: please add a Kbuild file in there so
'make headers_install' works

> diff -urN linux-2.6.18/init/Kconfig linux-2.6.18.patch1/init/Kconfig
> --- linux-2.6.18/init/Kconfig 2006-09-20 11:42:06.000000000 +0800
> +++ linux-2.6.18.patch1/init/Kconfig 2006-09-21 09:31:35.000000000 +0800
> @@ -267,7 +267,7 @@
>
> config UID16
> bool "Enable 16-bit UID system calls" if EMBEDDED
> - depends on ARM || CRIS || FRV || H8300 || X86_32 || M68K || (S390
&& !64BIT) || SUPERH || SPARC32 || (SPARC64 && SPARC32_COMPAT) || UML ||
(X86_64 && IA32_EMULATION)
> + depends on ARM || CRIS || FRV || H8300 || X86_32 || M68K || (S390
&& !64BIT) || SUPERH || SPARC32 || (SPARC64 && SPARC32_COMPAT) || UML ||
(X86_64 && IA32_EMULATION) || BFIN
> default y
> help
> This enables the legacy 16-bit UID syscall wrappers.

No!

> @@ -375,6 +375,7 @@
> config EPOLL
> bool "Enable eventpoll support" if EMBEDDED
> default y
> + depends on MMU
> help
> Disabling this option will cause the kernel to be built without
> support for epoll family of system calls.

Why that? If you have a good reason for it, please submit this as a
separate patch, as this has nothing to do with blackfin.

> diff -urN linux-2.6.18/init/Kconfig.orig
linux-2.6.18.patch1/init/Kconfig.orig
> --- linux-2.6.18/init/Kconfig.orig 1970-01-01 08:00:00.000000000 +0800
> +++ linux-2.6.18.patch1/init/Kconfig.orig 2006-09-21 09:14:55.000000000
+0800

This should not be in your patch

Arnd <><

2006-09-23 01:17:19

[permalink] [raw]

Subject: Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18

On Sat, 23 Sep 2006 02:18:36 +0200 Arnd Bergmann wrote:

> I've done a brief review. Overall, it looks fine, but you have lots of
> code duplication between your specific machines. It would be good
> to generalize that further.

Yes, I noticed that too.

> It would be good if you can get your code to build cleanly when using
> C=1 with sparse, right now that clearly won't work.

See Documentation/SubmitChecklist.

> Detailed comments follow
>
> > +/*#define IRQ_DEBUG*/
> > +#undef IRQ_DEBUG
> > +
> > +#ifdef IRQ_DEBUG
> > +#define IRQ_DPRINTK(x...) printk(x)
> > +#else
> > +#define IRQ_DPRINTK(x...) do { } while (0)
> > +#endif
>
> Use pr_debug here instead of making up your own macros

I saw 3 versions/variants of debug printk macros. :(
(maybe I missed a few :)

[lots of other good comments snipped :]

> > +static char *cplb_print_entry(char *buf, int type)
> > +{
> > + unsigned long *p_addr = dpdt_table;
> > + unsigned long *p_data = dpdt_table + 1;
> > + unsigned long *p_icount = dpdt_swapcount_table;
> > + unsigned long *p_ocount = dpdt_swapcount_table + 1;
> > + unsigned long *cplb_addr = (unsigned long *)DCPLB_ADDR0;
> > + unsigned long *cplb_data = (unsigned long *)DCPLB_DATA0;
> > + int entry, used_cplb = 0;
> > +
> > + if (type == CPLB_I) {
> > + buf += sprintf(buf, "Instrction CPLB entry:\n");
> > + p_addr = ipdt_table;
> > + p_data = ipdt_table + 1;
> > + p_icount = ipdt_swapcount_table;
> > + p_ocount = ipdt_swapcount_table + 1;
> > + cplb_addr = (unsigned long *)ICPLB_ADDR0;
> > + cplb_data = (unsigned long *)ICPLB_DATA0;
> > + } else
> > + buf += sprintf(buf, "Data CPLB entry:\n");
> > +
> > + buf += sprintf(buf, "Address\t\tData\tSize\tValid\tLocked\tSwapin\
> > +\tiCount\toCount\n");
> > +
> > + while (*p_addr != 0xffffffff) {
> > + entry = cplb_find_entry(cplb_addr, cplb_data, *p_addr);
> > + if (entry >= 0 && *p_data == cplb_data[entry])
> > + used_cplb |= 1 << entry;
> > +
> > + buf +=
> > + sprintf(buf,
> > + "0x%08lx\t0x%05lx\t%s\t%c\t%c\t%2d\t%ld\t%ld\n",
> > + *p_addr, *p_data,
> > + page_size_string_table[(*p_data & 0x30000) >> 16],
> > + (*p_data & CPLB_VALID) ? 'Y' : 'N',
> > + (*p_data & CPLB_LOCK) ? 'Y' : 'N', entry, *p_icount,
> > + *p_ocount);
> > +
> > + p_addr += 2;
> > + p_data += 2;
> > + p_icount += 2;
> > + p_ocount += 2;
> > + }
> > +
> > + if (used_cplb != 0xffff) {
> > + buf += sprintf(buf, "Unused/mismatched CPLBs:\n");
> > +
> > + for (entry = 0; entry < 16; entry++)
> > + if (0 == ((1 << entry) & used_cplb)) {
> > + int flags = cplb_data[entry];
> > + buf +=
> > + sprintf(buf,
> > + "%2d: 0x%08lx\t0x%05x\t%s\t%c\t%c\n",
> > + entry, cplb_addr[entry], flags,
> > + page_size_string_table[(flags &
> > + 0x30000) >>
> > + 16],
> > + (flags & CPLB_VALID) ? 'Y' : 'N',
> > + (flags & CPLB_LOCK) ? 'Y' : 'N');
> > + }
> > + }
> > +
> > + buf += sprintf(buf, "\n");
> > +
> > + return buf;
> > +}
>
> Another one of those files that just don't belong in procfs. Find some other
> interface for this.

examples of acceptable interfaces?

---
~Randy

2006-09-23 01:24:09

by Roland Dreier

[permalink] [raw]

Subject: Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18

> > > +static char *cplb_print_entry(char *buf, int type)

> examples of acceptable interfaces?

I don't know exactly what a CPLB is, but it looks like this file is
something debugging-related. So stick it in debugfs?

- R.

2006-09-23 06:50:04

[permalink] [raw]

Subject: Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18

On 9/22/06, Arnd Bergmann <[email protected]> wrote:
> Use pr_debug here instead of making up your own macros

done

> static inline void

done

> It would be nice if you could use a generic way to pass this partition data
> to the kernel from the mtd medium, instead of hardcoding it here.

i often wish for such things myself :)

unfortunately, the boot loader we utilize (u-boot) isnt exactly
friendly to the idea of managing flash partitions like redboot, and
what we have here is the current standard method for defining flash
partitions with mtd

> Use C99 initializers here, like

done

> There is not much point in trying to use the same numbers as an existing
> architecture if that means that you have to leave holes like setup().
> I don't know if you still have the choice of completely changing the
> syscall numbers, but it would make it nicer in the future.

we do, fortunately, have this luxury ... so we can look forward to a
nice cleaning of our syscall interface

> What's th point of these files, can't you just remove them all?

punted

> > +#define BUG() do { \
> > + dump_stack(); \
> > + printk("\nkernel BUG at %s:%d!\n", __FILE__, __LINE__); \
> > + panic("BUG!"); \
> > +} while (0)
>
> This is probably better done as an external function:

*shrug* i just did it the same way everyone else does ... i'm a sheep like that

> > diff -urN linux-2.6.18/include/asm-blackfin/mach-bf533/anomaly.h
> linux-2.6.18.patch1/include/asm-blackfin/mach-bf533/anomaly.h
> > --- linux-2.6.18/include/asm-blackfin/mach-bf533/anomaly.h 1970-01-01
> 08:00:00.000000000 +0800
> > +++ linux-2.6.18.patch1/include/asm-blackfin/mach-bf533/anomaly.h 2006-09-21
> 09:29:49.000000000 +0800
> > @@ -0,0 +1,172 @@
> > +/*
> > + * File: include/asm-blackfin/mach-bf533/anomaly.h
>
> You seem to have lots of these machine specfic header files in include/asm.
> Please move them to the respective machine implementation directory
> if they are only used from there

these are arch specific, not machine (aka board)

this is what the blackfin family looks like (first entry is the architecture):
- BF535
- BF533 / BF532 / BF531
- BF537 / BF536 / BF534
- BF561

which is why you find the anomaly definitions in the architecture
specific header dir

> > +/* Clock and System Control (0xFFC0 0400-0xFFC0 07FF) */
> > +#define bfin_read_PLL_CTL() bfin_read16(PLL_CTL)
> > +#define bfin_write_PLL_CTL(val) bfin_write16(PLL_CTL,val)
> > +#define bfin_read_PLL_STAT() bfin_read16(PLL_STAT)
> (and 700 more of these)
>
> What's the point, are you getting paid by lines of code? Just use
> the registers directly!

in our last submission we were doing exactly that ... and we were told
to switch to a function style method of reading/writing memory mapped
registers

> One more thing about the headers: please add a Kbuild file in there so
> 'make headers_install' works

will do

thanks for reading through that monster :)
-mike

2006-09-23 11:03:42

[permalink] [raw]

Subject: Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18

On Saturday 23 September 2006 08:50, Mike Frysinger wrote:
> On 9/22/06, Arnd Bergmann <[email protected]> wrote:
>
> > It would be nice if you could use a generic way to pass this partition data
> > to the kernel from the mtd medium, instead of hardcoding it here.
>
> i often wish for such things myself :)
>
> unfortunately, the boot loader we utilize (u-boot) isnt exactly
> friendly to the idea of managing flash partitions like redboot, and
> what we have here is the current standard method for defining flash
> partitions with mtd

For powerpc, we have started using a flattened device tree derived
from the Open Firmware standard for passing data to the kernel.
AFAIK, u-boot has some support for this, or at least you can build
a generic kernel and stick it into uboot together with a device
tree file.

There are different views on whether it's a good idea to have
the same format on platforms that don't have an Open Firmware
implementation, but even if you use a different format, it helps
to have a single blob describing all of your system, including
stuff like mmio register areas, cpu types, flash layout,
attached buses, etc.

> > There is not much point in trying to use the same numbers as an existing
> > architecture if that means that you have to leave holes like setup().
> > I don't know if you still have the choice of completely changing the
> > syscall numbers, but it would make it nicer in the future.
>
> we do, fortunately, have this luxury ... so we can look forward to a
> nice cleaning of our syscall interface

Ah, very good.

> > > +#define BUG() do { \
> > > + dump_stack(); \
> > > + printk("\nkernel BUG at %s:%d!\n", __FILE__, __LINE__); \
> > > + panic("BUG!"); \
> > > +} while (0)
> >
> > This is probably better done as an external function:
>
> *shrug* i just did it the same way everyone else does ... i'm a sheep like that

Ah, then just stay with your code. Maybe someone will want to change it
across architectures some day and it would help to have it common then.

Actually, the preferred way of implementing BUG and BUG_ON is to have
a single inline assembly opcode that will do an unconditional or a
condition trap (e.g. invalid opcode) so you can use your regular
fault handler to print the backtrace and such.

> > > diff -urN linux-2.6.18/include/asm-blackfin/mach-bf533/anomaly.h
> > linux-2.6.18.patch1/include/asm-blackfin/mach-bf533/anomaly.h
> > > --- linux-2.6.18/include/asm-blackfin/mach-bf533/anomaly.h 1970-01-01
> > 08:00:00.000000000 +0800
> > > +++ linux-2.6.18.patch1/include/asm-blackfin/mach-bf533/anomaly.h 2006-09-21
> > 09:29:49.000000000 +0800
> > > @@ -0,0 +1,172 @@
> > > +/*
> > > + * File: include/asm-blackfin/mach-bf533/anomaly.h
> >
> > You seem to have lots of these machine specfic header files in include/asm.
> > Please move them to the respective machine implementation directory
> > if they are only used from there
>
> these are arch specific, not machine (aka board)
>
> this is what the blackfin family looks like (first entry is the architecture):
> - BF535
> - BF533 / BF532 / BF531
> - BF537 / BF536 / BF534
> - BF561
>
> which is why you find the anomaly definitions in the architecture
> specific header dir

Ok, we need to align on the terminology then. The 'architecture'
is a fixed term for the top level of the hierarchy (i386, powerpc,
blackfin, ...), so you should try to avoid using that to refer
to things inside of blackfin.

We have at least three established terms for more specific things:
'subarchitecture', 'platform' and 'machine'.

Calling the most specific one 'machine' is very common, so that's
fine.

What you call 'architecture' (bf535, bf533, bf537, bf561) should
probably be either 'subarchitecture' or 'platform' in your code.

Now to my point: If all the files that use the platform specific
headers are in the same source directory, then these headers should
also be in that platform directory. To compare it with powerpc,
where we have discussed a long time about the ideal file layout,
that would mean you get:

arch/blackfin/platforms/bf535/anomaly.h
setup.c
arch/blackfin/platforms/bf533/anomaly.h
setup-531.c
setup-532.c
setup-533.c

> > > +/* Clock and System Control (0xFFC0 0400-0xFFC0 07FF) */
> > > +#define bfin_read_PLL_CTL() bfin_read16(PLL_CTL)
> > > +#define bfin_write_PLL_CTL(val) bfin_write16(PLL_CTL,val)
> > > +#define bfin_read_PLL_STAT() bfin_read16(PLL_STAT)
> > (and 700 more of these)
> >
> > What's the point, are you getting paid by lines of code? Just use
> > the registers directly!
>
> in our last submission we were doing exactly that ... and we were told
> to switch to a function style method of reading/writing memory mapped
> registers

It's hard to imagine that what you have here was intended by the comment
then. Do you have an archive link about that discussion?

Arnd <><

2006-09-23 11:15:19

[permalink] [raw]

Subject: Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18

On 9/23/06, Arnd Bergmann <[email protected]> wrote:
> On Saturday 23 September 2006 08:50, Mike Frysinger wrote:
> > On 9/22/06, Arnd Bergmann <[email protected]> wrote:
> > > > + * File: include/asm-blackfin/mach-bf533/anomaly.h
> > >
> > > You seem to have lots of these machine specfic header files in include/asm.
> > > Please move them to the respective machine implementation directory
> > > if they are only used from there
> >
> > these are sub-arch specific, not machine (aka board)
>
> Now to my point: If all the files that use the platform specific
> headers are in the same source directory, then these headers should
> also be in that platform directory. To compare it with powerpc,
> where we have discussed a long time about the ideal file layout,
> that would mean you get:

then that would not be just anomaly.h, that would be the entire mach
header subdirs:
include/asm-blackfin/mach-bf533/
include/asm-blackfin/mach-bf535/
include/asm-blackfin/mach-bf537/
include/asm-blackfin/mach-bf561/

relocated to the dirs:
arch/blackfin/mach-bf533/
arch/blackfin/mach-bf535/
arch/blackfin/mach-bf537/
arch/blackfin/mach-bf561/

> > > What's the point, are you getting paid by lines of code? Just use
> > > the registers directly!
> >
> > in our last submission we were doing exactly that ... and we were told
> > to switch to a function style method of reading/writing memory mapped
> > registers
>
> It's hard to imagine that what you have here was intended by the comment
> then. Do you have an archive link about that discussion?

no as i was not around for said discussion. but it should be in the
threads covering the submission of blackfin for 2.6.13 ...
-mike

2006-09-23 11:29:36

[permalink] [raw]

Subject: Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18

On Saturday 23 September 2006 13:15, Mike Frysinger wrote:
> then that would not be just anomaly.h, that would be the entire mach
> header subdirs:
> include/asm-blackfin/mach-bf533/
> include/asm-blackfin/mach-bf535/
> include/asm-blackfin/mach-bf537/
> include/asm-blackfin/mach-bf561/
>
> relocated to the dirs:
> arch/blackfin/mach-bf533/
> arch/blackfin/mach-bf535/
> arch/blackfin/mach-bf537/
> arch/blackfin/mach-bf561/

Right, that sounds good. Of course it doesn't make sense for files that
are used outside of arch/blackfin/mach-bfXXX/, but if your split between
platform specific and generic files is good, you don't have that problem.

Arnd <><

2006-09-23 11:28:33

by matthieu castet

[permalink] [raw]

Subject: Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18

Hi,

On Sat, 23 Sep 2006 02:50:02 -0400, Mike Frysinger wrote:

>> It would be nice if you could use a generic way to pass this partition data
>> to the kernel from the mtd medium, instead of hardcoding it here.
>
> i often wish for such things myself :)
>
> unfortunately, the boot loader we utilize (u-boot) isnt exactly
> friendly to the idea of managing flash partitions like redboot, and
> what we have here is the current standard method for defining flash
> partitions with mtd
>

humm you could use cmdlinepart [1] and pass the partition as a kernel
string with uboot.

That's what we are doing and it works perfectly.

>> > +/* Clock and System Control (0xFFC0 0400-0xFFC0 07FF) */
>> > +#define bfin_read_PLL_CTL() bfin_read16(PLL_CTL)
>> > +#define bfin_write_PLL_CTL(val) bfin_write16(PLL_CTL,val)
>> > +#define bfin_read_PLL_STAT() bfin_read16(PLL_STAT)
>> (and 700 more of these)
>>
>> What's the point, are you getting paid by lines of code? Just use
>> the registers directly!
>
> in our last submission we were doing exactly that ... and we were told
> to switch to a function style method of reading/writing memory mapped
> registers
hum, IRRC in your last submission you used volatile to read/write register.
Some people told you that volatile are evil and you should use a function
to read them.

But there no need to these defines. Just use bfin_read16(register_name) in
your code.

For an example look at arch/mips/au1000/common/usbdev.c for a driver using
memory mapped register.
Also you could use standard function like readl/writel if there no special
constraint (on the example a special function is needed because
readl/writel do some byteswapping on that platform (need for external
device), that is not need for memory mapped register).

Hope it will help you to improve your code.

Matthieu

[1]
see drivers/mtd/cmdlinepart.c for more information.
For example
1 NOR Flash with 2 partitions, 1 NAND with one
edb7312-nor:256k(ARMboot)ro,-(root);edb7312-nand:-(home)

2006-09-23 11:35:39

[permalink] [raw]

Subject: Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18

On 9/23/06, Matthieu CASTET <[email protected]> wrote:
> On Sat, 23 Sep 2006 02:50:02 -0400, Mike Frysinger wrote:
> >> It would be nice if you could use a generic way to pass this partition data
> >> to the kernel from the mtd medium, instead of hardcoding it here.
> >
> > i often wish for such things myself :)
> >
> > unfortunately, the boot loader we utilize (u-boot) isnt exactly
> > friendly to the idea of managing flash partitions like redboot, and
> > what we have here is the current standard method for defining flash
> > partitions with mtd
>
> humm you could use cmdlinepart [1] and pass the partition as a kernel
> string with uboot.
>
> That's what we are doing and it works perfectly.

yes, that is an option ... we find that utilizing the board files "better" as:
- the boot syntax tends to be a little unwieldy
- embedded boards dont change, so if you're defining so much
board-specific information in the boards file, the partition map makes
sense there as well
- the dynamic partitioning aspect of declaring the map at runtime is
unnecessary due to the previous comment

but each to their own i guess
-mike

2006-09-23 13:08:01

[permalink] [raw]

Subject: Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18

On Saturday 23 September 2006 13:15, Mike Frysinger wrote:
> On 9/23/06, Arnd Bergmann <[email protected]> wrote:
> > On Saturday 23 September 2006 08:50, Mike Frysinger wrote:
> > > On 9/22/06, Arnd Bergmann <[email protected]> wrote:

> > > > What's the point, are you getting paid by lines of code? Just use
> > > > the registers directly!
> > >
> > > in our last submission we were doing exactly that ... and we were told
> > > to switch to a function style method of reading/writing memory mapped
> > > registers
> >
> > It's hard to imagine that what you have here was intended by the comment
> > then. Do you have an archive link about that discussion?
>
> no as i was not around for said discussion. but it should be in the
> threads covering the submission of blackfin for 2.6.13 ...

Ok, I found http://marc.theaimsgroup.com/?l=linux-kernel&m=114298753207549&w=2
from akpm, and I fear you heavily misunderstood him.

Your original patch had code like

/* System Reset and Interrupt Controller registers for core A (0xFFC0 0100-0xFFC0 01FF) */
#define SICA_SWRST 0xFFC00100??????/* Software Reset register */
#define SICA_SYSCR 0xFFC00104??????/* System Reset Configuration register */
#define SICA_RVECT 0xFFC00108??????/* SIC Reset Vector Address Register */
#define SICA_IMASK 0xFFC0010C??????/* SIC Interrupt Mask register 0 - hack to fix old tests */
#define SICA_IMASK0 0xFFC0010C??????/* SIC Interrupt Mask register 0 */
#define SICA_IMASK1 0xFFC00110??????/* SIC Interrupt Mask register 1 */
#define SICA_IAR0 0xFFC00124??????/* SIC Interrupt Assignment Register 0 */
#define SICA_IAR1 0xFFC00128??????/* SIC Interrupt Assignment Register 1 */
#define SICA_IAR2 0xFFC0012C??????/* SIC Interrupt Assignment Register 2 */
#define SICA_IAR3 0xFFC00130??????/* SIC Interrupt Assignment Register 3 */
#define SICA_IAR4 0xFFC00134??????/* SIC Interrupt Assignment Register 4 */
#define SICA_IAR5 0xFFC00138??????/* SIC Interrupt Assignment Register 5 */
#define SICA_IAR6 0xFFC0013C??????/* SIC Interrupt Assignment Register 6 */
#define SICA_IAR7 0xFFC00140??????/* SIC Interrupt Assignment Register 7 */
#define SICA_ISR0 0xFFC00114??????/* SIC Interrupt Status register 0 */
#define SICA_ISR1 0xFFC00118??????/* SIC Interrupt Status register 1 */
#define SICA_IWR0 0xFFC0011C??????/* SIC Interrupt Wakeup-Enable register 0 */
#define SICA_IWR1 0xFFC00120??????/* SIC Interrupt Wakeup-Enable register 1 */

...

#define pSICA_SWRST (volatile unsigned short *)SICA_SWRST
#define pSICA_SYSCR (volatile unsigned short *)SICA_SYSCR
#define pSICA_RVECT (volatile unsigned short *)SICA_RVECT
#define pSICA_IMASK (volatile unsigned long *)SICA_IMASK
#define pSICA_IMASK0 (volatile unsigned long *)SICA_IMASK0
#define pSICA_IMASK1 (volatile unsigned long *)SICA_IMASK1
#define pSICA_IAR0 ((volatile unsigned long *)SICA_IAR0)
#define pSICA_IAR1 (volatile unsigned long *)SICA_IAR1
#define pSICA_IAR2 (volatile unsigned long *)SICA_IAR2
#define pSICA_IAR3 (volatile unsigned long *)SICA_IAR3
#define pSICA_IAR4 (volatile unsigned long *)SICA_IAR4
#define pSICA_IAR5 (volatile unsigned long *)SICA_IAR5
#define pSICA_IAR6 (volatile unsigned long *)SICA_IAR6
#define pSICA_IAR7 (volatile unsigned long *)SICA_IAR7
#define pSICA_ISR0 (volatile unsigned long *)SICA_ISR0
#define pSICA_ISR1 (volatile unsigned long *)SICA_ISR1
#define pSICA_IWR0 (volatile unsigned long *)SICA_IWR0
#define pSICA_IWR1 (volatile unsigned long *)SICA_IWR1

static void bf561_internal_mask_irq(unsigned int irq)
{
unsigned long irq_mask;
if ((irq - (IRQ_CORETMR + 1)) < 32) {
irq_mask = (1 << (irq - (IRQ_CORETMR + 1)));
*pSICA_IMASK0 &= ~irq_mask;
} else {
irq_mask = (1 << (irq - (IRQ_CORETMR + 1) - 32));
*pSICA_IMASK1 &= ~irq_mask;
}
}

which Andrew objected to, on multiple grounds. You now converted this to

/* System Reset and Interrupt Controller registers for core A (0xFFC0 0100-0xFFC0 01FF) */
#define SICA_SWRST 0xFFC00100??????/* Software Reset register */
#define SICA_SYSCR 0xFFC00104??????/* System Reset Configuration register */
#define SICA_RVECT 0xFFC00108??????/* SIC Reset Vector Address Register */
#define SICA_IMASK 0xFFC0010C??????/* SIC Interrupt Mask register 0 - hack to fix old tests */
#define SICA_IMASK0 0xFFC0010C??????/* SIC Interrupt Mask register 0 */
#define SICA_IMASK1 0xFFC00110??????/* SIC Interrupt Mask register 1 */
#define SICA_IAR0 0xFFC00124??????/* SIC Interrupt Assignment Register 0 */
#define SICA_IAR1 0xFFC00128??????/* SIC Interrupt Assignment Register 1 */
#define SICA_IAR2 0xFFC0012C??????/* SIC Interrupt Assignment Register 2 */
#define SICA_IAR3 0xFFC00130??????/* SIC Interrupt Assignment Register 3 */
#define SICA_IAR4 0xFFC00134??????/* SIC Interrupt Assignment Register 4 */
#define SICA_IAR5 0xFFC00138??????/* SIC Interrupt Assignment Register 5 */
#define SICA_IAR6 0xFFC0013C??????/* SIC Interrupt Assignment Register 6 */
#define SICA_IAR7 0xFFC00140??????/* SIC Interrupt Assignment Register 7 */
#define SICA_ISR0 0xFFC00114??????/* SIC Interrupt Status register 0 */
#define SICA_ISR1 0xFFC00118??????/* SIC Interrupt Status register 1 */
#define SICA_IWR0 0xFFC0011C??????/* SIC Interrupt Wakeup-Enable register 0 */
#define SICA_IWR1 0xFFC00120??????/* SIC Interrupt Wakeup-Enable register 1 */

/* System Reset and Interrupt Controller registers for core A (0xFFC0 0100-0xFFC0 01FF) */
#define bfin_read_SICA_SWRST() bfin_read16(SICA_SWRST)
#define bfin_write_SICA_SWRST(val) bfin_write16(SICA_SWRST,val)
#define bfin_read_SICA_SYSCR() bfin_read16(SICA_SYSCR)
#define bfin_write_SICA_SYSCR(val) bfin_write16(SICA_SYSCR,val)
#define bfin_read_SICA_RVECT() bfin_read16(SICA_RVECT)
#define bfin_write_SICA_RVECT(val) bfin_write16(SICA_RVECT,val)
#define bfin_read_SICA_IMASK() bfin_read32(SICA_IMASK)
#define bfin_write_SICA_IMASK(val) bfin_write32(SICA_IMASK,val)
#define bfin_read_SICA_IMASK0() bfin_read32(SICA_IMASK0)
#define bfin_write_SICA_IMASK0(val) bfin_write32(SICA_IMASK0,val)
#define bfin_read_SICA_IMASK1() bfin_read32(SICA_IMASK1)
#define bfin_write_SICA_IMASK1(val) bfin_write32(SICA_IMASK1,val)
#define bfin_read_SICA_IAR0() bfin_read32(SICA_IAR0)
#define bfin_write_SICA_IAR0(val) bfin_write32(SICA_IAR0,val)
#define bfin_read_SICA_IAR1() bfin_read32(SICA_IAR1)
#define bfin_write_SICA_IAR1(val) bfin_write32(SICA_IAR1,val)
#define bfin_read_SICA_IAR2() bfin_read32(SICA_IAR2)
#define bfin_write_SICA_IAR2(val) bfin_write32(SICA_IAR2,val)
#define bfin_read_SICA_IAR3() bfin_read32(SICA_IAR3)
#define bfin_write_SICA_IAR3(val) bfin_write32(SICA_IAR3,val)
#define bfin_read_SICA_IAR4() bfin_read32(SICA_IAR4)
#define bfin_write_SICA_IAR4(val) bfin_write32(SICA_IAR4,val)
#define bfin_read_SICA_IAR5() bfin_read32(SICA_IAR5)
#define bfin_write_SICA_IAR5(val) bfin_write32(SICA_IAR5,val)
#define bfin_read_SICA_IAR6() bfin_read32(SICA_IAR6)
#define bfin_write_SICA_IAR6(val) bfin_write32(SICA_IAR6,val)
#define bfin_read_SICA_IAR7() bfin_read32(SICA_IAR7)
#define bfin_write_SICA_IAR7(val) bfin_write32(SICA_IAR7,val)
#define bfin_read_SICA_ISR0() bfin_read32(SICA_ISR0)
#define bfin_write_SICA_ISR0(val) bfin_write32(SICA_ISR0,val)
#define bfin_read_SICA_ISR1() bfin_read32(SICA_ISR1)
#define bfin_write_SICA_ISR1(val) bfin_write32(SICA_ISR1,val)
#define bfin_read_SICA_IWR0() bfin_read32(SICA_IWR0)
#define bfin_write_SICA_IWR0(val) bfin_write32(SICA_IWR0,val)
#define bfin_read_SICA_IWR1() bfin_read32(SICA_IWR1)
#define bfin_write_SICA_IWR1(val) bfin_write32(SICA_IWR1,val)

...
static void bf561_internal_mask_irq(unsigned int irq)
{
unsigned long irq_mask;
if ((irq - (IRQ_CORETMR + 1)) < 32) {
irq_mask = (1 << (irq - (IRQ_CORETMR + 1)));
bfin_write_SICA_IMASK0(bfin_read_SICA_IMASK0() & ~irq_mask);
} else {
irq_mask = (1 << (irq - (IRQ_CORETMR + 1) - 32));
bfin_write_SICA_IMASK1(bfin_read_SICA_IMASK1() & ~irq_mask);
}
}

which is about just as bad, but for different reasons. Now, there are
at least two common methods for how to do this better, both involving
the readl/writel low-level accessors (or something similar).

The first one uses enumerated register numers:

/* System Reset and Interrupt Controller registers for core A (0xFFC0 0100-0xFFC0 01FF) */

enum bfin_sica_regs {
SICA_SWRST = 100,??????/* Software Reset register */
SICA_SYSCR = 104,??????/* System Reset Configuration register */
SICA_RVECT = 108,??????/* SIC Reset Vector Address Register */
SICA_IMASK = 10C,??????/* SIC Interrupt Mask register 0 - hack to fix old tests */
SICA_IMASK0 = 10C,??????/* SIC Interrupt Mask register 0 */
SICA_IMASK1 = 110,??????/* SIC Interrupt Mask register 1 */
SICA_IAR0 = 124,??????/* SIC Interrupt Assignment Register 0 */
SICA_IAR1 = 128,??????/* SIC Interrupt Assignment Register 1 */
SICA_IAR2 = 12C,??????/* SIC Interrupt Assignment Register 2 */
SICA_IAR3 = 130,??????/* SIC Interrupt Assignment Register 3 */
SICA_IAR4 = 134,??????/* SIC Interrupt Assignment Register 4 */
SICA_IAR5 = 138,??????/* SIC Interrupt Assignment Register 5 */
SICA_IAR6 = 13C,??????/* SIC Interrupt Assignment Register 6 */
SICA_IAR7 = 140,??????/* SIC Interrupt Assignment Register 7 */
SICA_ISR0 = 114,??????/* SIC Interrupt Status register 0 */
SICA_ISR1 = 118,??????/* SIC Interrupt Status register 1 */
SICA_IWR0 = 11C,??????/* SIC Interrupt Wakeup-Enable register 0 */
SICA_IWR1 = 120,??????/* SIC Interrupt Wakeup-Enable register 1 */
};

...

static void __iomem *bfin_sica = (void __iomem *)0xffc00100ul;
static inline __le32 bfin_sica_read(int reg)
{
return (__le32)readl(bfin_sica + reg);
}

static inline void bfin_sica_write(int reg, __le32 val)
{
writel((uint32_t)val, bfin_sica + reg);
}

static void bf561_internal_mask_irq(unsigned int irq)
{
unsigned long irq_mask;
if ((irq - (IRQ_CORETMR + 1)) < 32) {
irq_mask = (1 << (irq - (IRQ_CORETMR + 1)));
bfin_sica_write(SICA_IMASK0,
bfin_sica_read(SICA_IMASK0) & ~irq_mask);
} else {
irq_mask = (1 << (irq - (IRQ_CORETMR + 1) - 32));
bfin_sica_write(SICA_IMASK0,
bfin_sica_read(SICA_IMASK0) & ~irq_mask);
}
}

The alternative approach uses a structure:

/* System Reset and Interrupt Controller registers for core A (0xFFC0 0100-0xFFC0 01FF) */

struct bfin_sica_regs {
__le32 swrst; /* Software Reset register */
__le32 syscr; /* System Reset Configuration register */
__le32 rvect; /* SIC Reset Vector Address Register */
__le32 imask[2]; /* SIC Interrupt Mask register 0-1 */
__le32 iar[8]; /* SIC Interrupt Assignment Register 0-7 */
__le32 isr[2]; /* SIC Interrupt Status register 0-1 */
__le32 iwr[2]; /* SIC Interrupt Wakeup-Enable register 0-2 */
};

...

static struct bfin_sica_regs __iomem *bfin_sica = (void __iomem *)0xffc00100ul;

static void bf561_internal_mask_irq(unsigned int irq)
{
int irqnr = irq - (IRQ_CORETMR + 1);
__le32 __iomem *reg = &bfin_sica->imask[irqnr >> 5];
unsigned long irq_mask = 1 << (irqnr & 0x1f);

writel(reg, readl(reg) & ~irq_mask);
}

I'd personally prefer the last approach because of its compactness.

Arnd <><

2006-09-23 13:13:05

[permalink] [raw]

Subject: Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18

On 9/22/06, Roland Dreier <[email protected]> wrote:
> > > > +static char *cplb_print_entry(char *buf, int type)
>
> > examples of acceptable interfaces?
>
> I don't know exactly what a CPLB is, but it looks like this file is
> something debugging-related. So stick it in debugfs?

a CPLB is a blackfinisim for handling cache ... it stands for Cache
Protection Lookaside Buffer

and yes, our proc file for displaying the current software copies of
the data cplb and instruction cplb tables would probably be better
implemented via debugfs as there is no real upper limit on how big the
table can grow ...
-mike

2006-09-23 16:29:00

[permalink] [raw]

Subject: Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18

Arnd provide some feedback:
>On Saturday 23 September 2006 13:15, Mike Frysinger wrote:
> > On 9/23/06, Arnd Bergmann <[email protected]> wrote:
> > > On Saturday 23 September 2006 08:50, Mike Frysinger wrote:
> > > > On 9/22/06, Arnd Bergmann <[email protected]> wrote:
>
> > > > > What's the point, are you getting paid by lines of code? Just use
> > > > > the registers directly!
> > > >
> > > > in our last submission we were doing exactly that ... and we were told
> > > > to switch to a function style method of reading/writing memory mapped
> > > > registers
> > >
> > > It's hard to imagine that what you have here was intended by the comment
> > > then. Do you have an archive link about that discussion?
> >
> > no as i was not around for said discussion. but it should be in the
> > threads covering the submission of blackfin for 2.6.13 ...
>
>Ok, I found http://marc.theaimsgroup.com/?l=linux-kernel&m=114298753207549&w=2
>from akpm, and I fear you heavily misunderstood him.
>
>Your original patch had code like
>
>/* System Reset and Interrupt Controller registers for core A (0xFFC0
>0100-0xFFC0 01FF) */
>#define SICA_SWRST 0xFFC00100?????/* Software Reset register */
>[snip]
>...
>
>#define pSICA_SWRST (volatile unsigned short *)SICA_SWRST
>[snip]
>
>static void bf561_internal_mask_irq(unsigned int irq)
>{
> unsigned long irq_mask;
> if ((irq - (IRQ_CORETMR + 1)) < 32) {
> irq_mask = (1 << (irq - (IRQ_CORETMR + 1)));
> pSICA_IMASK0 &= ~irq_mask;
> } else {
> irq_mask = (1 << (irq - (IRQ_CORETMR + 1) - 32));
> pSICA_IMASK1 &= ~irq_mask;
> }
>}
>
>which Andrew objected to, on multiple grounds. You now converted this to
>
>/* System Reset and Interrupt Controller registers for core A (0xFFC0
>0100-0xFFC0 01FF) */
>#define SICA_SWRST 0xFFC00100?????/* Software Reset register */
>[snip]
>/* System Reset and Interrupt Controller registers for core A (0xFFC0
>0100-0xFFC0 01FF) */
>#define bfin_read_SICA_SWRST() bfin_read16(SICA_SWRST)
>[snip]
>
>...
>static void bf561_internal_mask_irq(unsigned int irq)
>{
> unsigned long irq_mask;
> if ((irq - (IRQ_CORETMR + 1)) < 32) {
> irq_mask = (1 << (irq - (IRQ_CORETMR + 1)));
> bfin_write_SICA_IMASK0(bfin_read_SICA_IMASK0() & ~irq_mask);
> } else {
> irq_mask = (1 << (irq - (IRQ_CORETMR + 1) - 32));
> bfin_write_SICA_IMASK1(bfin_read_SICA_IMASK1() & ~irq_mask);
> }
>}
>
>which is about just as bad, but for different reasons.

I would be interested in the reasons why this is bad. We thought is solved
the problem, and our driver developers are Ok using it, and it is catching
more problems at compile time than we use to (which is the main reason I
thought to remove all the volatile casting.

>Now, there are
>at least two common methods for how to do this better, both involving
>the readl/writel low-level accessors (or something similar).

The thing to understand about the Blackfin architecture - is not all system
register, or peripheral registers are 32 bits. Some are 16 bits, and some
are 32. The Hardware will error if you access a 32 bit instruction, with a
16 bit access, or a 16 bit access on a 32 bit instruction.

This is why we added:
bfin_write16(); bfin_read16(); bfin_write32(); bfin_read32();

We can't use a common function, like:

bfin_sica_read(int reg)

or use the read16/read32 directly - it would be to hard for the driver
developers to keep the manual with them all the time to find out if a
register was 16 or 32 bits. It would move what we have today (compiler
errors), to run time errors if someone used the wrong function.

>The first one uses enumerated register numers:
>
>/* System Reset and Interrupt Controller registers for core A (0xFFC0
>0100-0xFFC0 01FF) */
>
>enum bfin_sica_regs {
> SICA_SWRST = 100,?????/* Software Reset register */
> SICA_SYSCR = 104,?????/* System Reset Configuration register */
> SICA_RVECT = 108,?????/* SIC Reset Vector Address Register */
> SICA_IMASK = 10C,?????/* SIC Interrupt Mask register 0 - hack
> to fix old tests */
> SICA_IMASK0 = 10C,?????/* SIC Interrupt Mask register 0 */
> SICA_IMASK1 = 110,?????/* SIC Interrupt Mask register 1 */
> SICA_IAR0 = 124,?????/* SIC Interrupt Assignment Register 0 */
> SICA_IAR1 = 128,?????/* SIC Interrupt Assignment Register 1 */
> SICA_IAR2 = 12C,?????/* SIC Interrupt Assignment Register 2 */
> SICA_IAR3 = 130,?????/* SIC Interrupt Assignment Register 3 */
> SICA_IAR4 = 134,?????/* SIC Interrupt Assignment Register 4 */
> SICA_IAR5 = 138,?????/* SIC Interrupt Assignment Register 5 */
> SICA_IAR6 = 13C,?????/* SIC Interrupt Assignment Register 6 */
> SICA_IAR7 = 140,?????/* SIC Interrupt Assignment Register 7 */
> SICA_ISR0 = 114,?????/* SIC Interrupt Status register 0 */
> SICA_ISR1 = 118,?????/* SIC Interrupt Status register 1 */
> SICA_IWR0 = 11C,?????/* SIC Interrupt Wakeup-Enable register 0 */
> SICA_IWR1 = 120,?????/* SIC Interrupt Wakeup-Enable register 1 */
>};
>
>...
>
>static void __iomem *bfin_sica = (void __iomem *)0xffc00100ul;
>static inline __le32 bfin_sica_read(int reg)
>{
> return (__le32)readl(bfin_sica + reg);
>}
>
>static inline void bfin_sica_write(int reg, __le32 val)
>{
> writel((uint32_t)val, bfin_sica + reg);
>}
>
>static void bf561_internal_mask_irq(unsigned int irq)
>{
> unsigned long irq_mask;
> if ((irq - (IRQ_CORETMR + 1)) < 32) {
> irq_mask = (1 << (irq - (IRQ_CORETMR + 1)));
> bfin_sica_write(SICA_IMASK0,
> bfin_sica_read(SICA_IMASK0) & ~irq_mask);
> } else {
> irq_mask = (1 << (irq - (IRQ_CORETMR + 1) - 32));
> bfin_sica_write(SICA_IMASK0,
> bfin_sica_read(SICA_IMASK0) & ~irq_mask);
> }
>}
>
>The alternative approach uses a structure:

We could use this approach, filling it up with 16 bit padding all over the
place (which is easy to do), but it is also difficult for the same reason.
(Although I really like this, and can see lots of value from it - I am not
sure we can use it).

You are still using writel(reg, value) and readl(reg) - which is hard to
do, without a hardware reference beside you all the time - to determine
which time you should use a readl or reads.

Unless I am completely missing something (which might be true).

>/* System Reset and Interrupt Controller registers for core A (0xFFC0
>0100-0xFFC0 01FF) */
>
>struct bfin_sica_regs {
> __le32 swrst; /* Software Reset register */
> __le32 syscr; /* System Reset Configuration register */
> __le32 rvect; /* SIC Reset Vector Address Register */
> __le32 imask[2]; /* SIC Interrupt Mask register 0-1 */
> __le32 iar[8]; /* SIC Interrupt Assignment Register 0-7 */
> __le32 isr[2]; /* SIC Interrupt Status register 0-1 */
> __le32 iwr[2]; /* SIC Interrupt Wakeup-Enable register 0-2 */
>};
>
>...
>
>static struct bfin_sica_regs __iomem *bfin_sica = (void __iomem
>*)0xffc00100ul;
>
>static void bf561_internal_mask_irq(unsigned int irq)
>{
> int irqnr = irq - (IRQ_CORETMR + 1);
> __le32 __iomem *reg = &bfin_sica->imask[irqnr >> 5];
> unsigned long irq_mask = 1 << (irqnr & 0x1f);
>
> writel(reg, readl(reg) & ~irq_mask);
>}
>
>I'd personally prefer the last approach because of its compactness.
>
> Arnd <><

Although I think the code is smaller, and nicer - it involves more run time
complexity, and will consume more processor cycles - the old example:

static void bf561_internal_mask_irq(unsigned int irq)
{
unsigned long irq_mask;
if ((irq - (IRQ_CORETMR + 1)) < 32) {
irq_mask = (1 << (irq - (IRQ_CORETMR + 1)));
bfin_write_SICA_IMASK0(bfin_read_SICA_IMASK0() & ~irq_mask);
} else {
irq_mask = (1 << (irq - (IRQ_CORETMR + 1) - 32));
bfin_write_SICA_IMASK1(bfin_read_SICA_IMASK1() & ~irq_mask);
}
}

resolves all addresses at compile time, not run time. So, wouldn't your
example slow things down?

-Robin

2006-09-23 17:57:34

[permalink] [raw]

Subject: Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18

Matthieu wrote:
>On Sat, 23 Sep 2006 02:50:02 -0400, Mike Frysinger wrote:
> >> It would be nice if you could use a generic way to pass this
> >> partition data to the kernel from the mtd medium, instead of
> hardcoding it here.
> >
> > i often wish for such things myself :)
> >
> > unfortunately, the boot loader we utilize (u-boot) isnt exactly
> > friendly to the idea of managing flash partitions like redboot, and
> > what we have here is the current standard method for defining flash
> > partitions with mtd
> >
>
>humm you could use cmdlinepart [1] and pass the partition as a kernel
>string with uboot.
>
>That's what we are doing and it works perfectly.

Thanks for the pointer - we will have a look.

> >> > +/* Clock and System Control (0xFFC0 0400-0xFFC0 07FF) */
> >> > +#define bfin_read_PLL_CTL() bfin_read16(PLL_CTL)
> >> > +#define bfin_write_PLL_CTL(val) bfin_write16(PLL_CTL,val)
> >> > +#define bfin_read_PLL_STAT() bfin_read16(PLL_STAT)
> >> (and 700 more of these)
> >>
> >> What's the point, are you getting paid by lines of code? Just use the
> >> registers directly!
> >
> > in our last submission we were doing exactly that ... and we were told
> > to switch to a function style method of reading/writing memory mapped
> > registers
>hum, IRRC in your last submission you used volatile to read/write register.
>Some people told you that volatile are evil and you should use a function
>to read them.
>
>But there no need to these defines. Just use bfin_read16(register_name) in
>your code.

But then all the driver developers need to remember if a register is 16 or
32 bits. Blackfin peripherals mix and match, depending on what the silicon
designer decided what good at the time. (which means if it fits in 16 bits,
it is a 16-bit register).

I guess we are all lazy, and don't want to have to go through the
complexity of looking up each register when we use it in a driver.

calling bfin_read_PLL_CTL() which is defined and typecast properly as short
bfin_read16(PLL_CTL), ensure we resolve issues like this at compile time,
without having to keep the manual in front of us.

-Robin

2006-09-23 18:10:21

[permalink] [raw]

Subject: Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18

On Saturday 23 September 2006 18:29, Robin Getz wrote:
> I would be interested in the reasons why this is bad. We thought is solved
> the problem, and our driver developers are Ok using it, and it is catching
> more problems at compile time than we use to (which is the main reason I
> thought to remove all the volatile casting.

It adds a lot of unnecessary defines. The problem is mostly that
for each register you add three macros.

The simplest way to avoid this would be using your bfin_write16()
and related functions directly instead of wrapping each register
in a separate macro.

> >Now, there are
> >at least two common methods for how to do this better, both involving
> >the readl/writel low-level accessors (or something similar).
>
> The thing to understand about the Blackfin architecture - is not all system
> register, or peripheral registers are 32 bits. Some are 16 bits, and some
> are 32. The Hardware will error if you access a 32 bit instruction, with a
> 16 bit access, or a 16 bit access on a 32 bit instruction.
>
> This is why we added:
> bfin_write16(); ?bfin_read16(); bfin_write32(); ?bfin_read32();

Sure, that's not unusual at all. Instead of readl(), you can use
readw() for 16 bit accesses. Your bfin_{read,write}{16,32} functions
are fine as well, but you should make the implementation more robust
and change

#define bfin_read16(addr) ({
unsigned __v; \
__asm__ __volatile__ ("NOP;\n\t %0 = w[%1] (z);\n\t" \
: "=d"(__v) : "a"(addr)); \
(unsigned short)__v; \
})

to

static inline __le16 bfin_read16(const __be16 __iomem *addr)
{
__be16 val;
asm volatile("nop; \n\t %0 = w[%1] (z)" : "=d" (val) : "a"(addr);
return val;
}

This adds strict type checking (size, endianess, io address space).
You may prefer to use u16 instead of __be16 here, depending on your needs.

> We can't use a common function, like:
>
> bfin_sica_read(int reg)
>
> or use the read16/read32 directly - it would be to hard for the driver
> developers to keep the manual with them all the time to find out if a
> register was 16 or 32 bits. It would move what we have today (compiler
> errors), to run time errors if someone used the wrong function.

Ok, in the example I picked they were all the same size, so I assumed
that would be the case for most of your register areas.

> >The first one uses enumerated register numers:
> >
> >/* System Reset and Interrupt Controller registers for core A (0xFFC0
> >0100-0xFFC0 01FF) */
> >
> >enum bfin_sica_regs {
> > ? ? ? ? SICA_SWRST ? = 100,?????/* Software Reset register */
> > ? ? ? ? SICA_SYSCR ? = 104,?????/* System Reset Configuration register */
> > ? ? ? ? SICA_RVECT ? = 108,?????/* SIC Reset Vector Address Register */
> > ? ? ? ? SICA_IMASK ? = 10C,?????/* SIC Interrupt Mask register 0 - hack
> > to fix old tests */
> > ? ? ? ? SICA_IMASK0 ?= 10C,?????/* SIC Interrupt Mask register 0 */
> > ? ? ? ? SICA_IMASK1 ?= 110,?????/* SIC Interrupt Mask register 1 */
> > ? ? ? ? SICA_IAR0 ? ?= 124,?????/* SIC Interrupt Assignment Register 0 */
> > ? ? ? ? SICA_IAR1 ? ?= 128,?????/* SIC Interrupt Assignment Register 1 */
> > ? ? ? ? SICA_IAR2 ? ?= 12C,?????/* SIC Interrupt Assignment Register 2 */
> > ? ? ? ? SICA_IAR3 ? ?= 130,?????/* SIC Interrupt Assignment Register 3 */
> > ? ? ? ? SICA_IAR4 ? ?= 134,?????/* SIC Interrupt Assignment Register 4 */
> > ? ? ? ? SICA_IAR5 ? ?= 138,?????/* SIC Interrupt Assignment Register 5 */
> > ? ? ? ? SICA_IAR6 ? ?= 13C,?????/* SIC Interrupt Assignment Register 6 */
> > ? ? ? ? SICA_IAR7 ? ?= 140,?????/* SIC Interrupt Assignment Register 7 */
> > ? ? ? ? SICA_ISR0 ? ?= 114,?????/* SIC Interrupt Status register 0 */
> > ? ? ? ? SICA_ISR1 ? ?= 118,?????/* SIC Interrupt Status register 1 */
> > ? ? ? ? SICA_IWR0 ? ?= 11C,?????/* SIC Interrupt Wakeup-Enable register 0 */
> > ? ? ? ? SICA_IWR1 ? ?= 120,?????/* SIC Interrupt Wakeup-Enable register 1 */
> >};
> >
> >...
> >
> >static void __iomem *bfin_sica = (void __iomem *)0xffc00100ul;
> >static inline __le32 bfin_sica_read(int reg)
> >{
> > ? ? ? ? return (__le32)readl(bfin_sica + reg);
> >}
> >
> >static inline void bfin_sica_write(int reg, __le32 val)
> >{
> > ? ? ? ? writel((uint32_t)val, bfin_sica + reg);
> >}
> >
> >static void bf561_internal_mask_irq(unsigned int irq)
> >{
> > ? ? ? ? unsigned long irq_mask;
> > ? ? ? ? if ((irq - (IRQ_CORETMR + 1)) < 32) {
> > ? ? ? ? ? ? ? ? irq_mask = (1 << (irq - (IRQ_CORETMR + 1)));
> > ? ? ? ? ? ? ? ? bfin_sica_write(SICA_IMASK0,
> > ? ? ? ? ? ? ? ? ? ? ? ? bfin_sica_read(SICA_IMASK0) & ~irq_mask);
> > ? ? ? ? } else {
> > ? ? ? ? ? ? ? ? irq_mask = (1 << (irq - (IRQ_CORETMR + 1) - 32));
> > ? ? ? ? ? ? ? ? bfin_sica_write(SICA_IMASK0,
> > ? ? ? ? ? ? ? ? ? ? ? ? bfin_sica_read(SICA_IMASK0) & ~irq_mask);
> > ? ? ? ? }
> >}
> >
> >The alternative approach uses a structure:
>
> We could use this approach, filling it up with 16 bit padding all over the
> place (which is easy to do), but it is also difficult for the same reason.
> (Although I really like this, and can see lots of value from it - I am not
> sure we can use it).
>
> You are still using writel(reg, value) and readl(reg) - which is hard to
> do, without a hardware reference beside you all the time - to determine
> which time you should use a readl or reads.
>
> Unless I am completely missing something (which might be true).

Ok, I see your point. If you have different size registers in
the area for a single device, then it is better to use a structure
as I showed below.

> >static struct bfin_sica_regs __iomem *bfin_sica = (void __iomem
> >*)0xffc00100ul;
> >

> Although I think the code is smaller, and nicer - it involves more run time
> complexity, and will consume more processor cycles - the old example:
>
> static void bf561_internal_mask_irq(unsigned int irq)
> {
> ? ? ? ? ?unsigned long irq_mask;
> ? ? ? ? ?if ((irq - (IRQ_CORETMR + 1)) < 32) {
> ? ? ? ? ? ? ? ? ?irq_mask = (1 << (irq - (IRQ_CORETMR + 1)));
> ? ? ? ? ? ? ? ? ?bfin_write_SICA_IMASK0(bfin_read_SICA_IMASK0() & ~irq_mask);
> ? ? ? ? ?} else {
> ? ? ? ? ? ? ? ? ?irq_mask = (1 << (irq - (IRQ_CORETMR + 1) - 32));
> ? ? ? ? ? ? ? ? ?bfin_write_SICA_IMASK1(bfin_read_SICA_IMASK1() & ~irq_mask);
> ? ? ? ? ?}
> }
>
> resolves all addresses at compile time, not run time. So, wouldn't your
> example slow things down?

The run time overhead of this is very small compared to an actual mmio
register access, so you should not notice this at all.

You can still do the same with a compile time constant by replacing

static struct bfin_sica_regs __iomem *bfin_sica = (void __iomem *)0xffc00100ul;

from my example with

#define bfin_sica_regs ((struct bfin_sica_regs __iomem*)0xffc00100ul)

That should result in the same object code you had before.

However, I'm used to having a single kernel binary that can run on
multiple hardware versions. Normally this means that you have the same
register layout on every CPU, but on different physical addresses that
are detected at boot time, so I like to avoid hardcoding absolute
addresses in the object code.

Moreover, if you ever want to run with an MMU, the virtual address of
that device is computed by the ioremap function, like

static struct bfin_sica_regs __iomem *bfin_sica;

void __init bfin_init_sica(void)
{
bfin_sica = ioremap(0xffc00100ul);
}

Arnd <><

2006-09-23 19:44:09

[permalink] [raw]

Subject: Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18

On Saturday 23 September 2006 08:50, Mike Frysinger wrote:
> > There is not much point in trying to use the same numbers as an existing
> > architecture if that means that you have to leave holes like setup().
> > I don't know if you still have the choice of completely changing the
> > syscall numbers, but it would make it nicer in the future.
>
> we do, fortunately, have this luxury ... so we can look forward to a
> nice cleaning of our syscall interface

Actually, I have one more general comment here. It would be really nice
if you could add those files that have nothing specific to blackfin moved
to include/asm-generic. That would probably include bug.h, current.h,
flat.h, hardirq.h, ioctls.h, {ipc,msg,sem,msg}buf.h, kmap_types.h, mman.h,
param.h, pci.h, poll.h, posix_types.h, scatterlist.h, semaphore.h,
socket.h, sockios.h, stat.h, termbits.h, termios.h, types.h, and unistd.h.

It doesn't really matter if you're the only user of the new files,
as long as they are generic enough to be used by every future port.
If the files are specific to nommu, 32bit or little-endian, then
they should probably have the respective name so the next person can
do it differently.

For unistd.h, it may be a good idea to leave space for a few syscall
numbers specific to architectures, so you could start the generic numbers
at 32 or so.

Of course nobody is forcing you do do that work, but the next person
trying to do will be really thankful.

Arnd <><

2006-09-23 21:28:22

by David Woodhouse

[permalink] [raw]

Subject: Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18

On Thu, 2006-09-21 at 11:32 +0800, Luke Yang wrote:
> This is the blackfin architecture for 2.6.18, again.

Please run 'make headers_check' for blackfin and then verify that you
can build libc against the resulting headers.

--
dwmw2

2006-09-23 23:25:08

[permalink] [raw]

Subject: Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18

>On Thu, 2006-09-21 at 11:32 +0800, Luke Yang wrote:
> > This is the blackfin architecture for 2.6.18, again.
>
>Please run 'make headers_check' for blackfin and then verify that you can
>build libc against the resulting headers.

We can't build libc, but we can build uClibc ;)

This is how we build our toolchain today (mostly). - now we do a make
prepare, but we will update it.

-Robin

2006-09-24 03:35:33

[permalink] [raw]

Subject: Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18

On 9/23/06, Arnd Bergmann <[email protected]> wrote:
> > +static uint32_t reloc_stack_operate(unsigned int oper, struct module *mod)
> > +{
> > + uint32_t value;
> > + switch (oper) {
> > + case R_add:
> > + {
> > + value =
> > + reloc_stack[reloc_stack_tos - 2] +
> > + reloc_stack[reloc_stack_tos - 1];
> > + reloc_stack_tos -= 2;
> > + break;
> > + }
>
> no need for the curly braces here and below

Hmm, but we need one line < 80 columns, don't we?

>
> static inline void default_idle(void)
>
> > +{
> > + while (!need_resched()) {
> > + leds_switch(LED_OFF);
> > + __asm__("nop;\n\t \
> > + nop;\n\t \
> > + nop;\n\t \
> > + idle;\n\t": : :"cc");
> > + leds_switch(LED_ON);
> > + }
> > +}
> > +
>
> This looks racy. What if you get an interrupt after testing need_resched()
> but before the idle instruction?
>
> Normally, this should look like
>
> while(!need_resched()) {
> local_irq_disable();
> if (!need_resched())
> asm volatile("idle");
> local_irq_enable();
> }
>
> Of course that only works if your idle instruction wakes up on pending
> interrupts.

No, that doesn't work on blackfin. Blackfin need interrupt(here is
timer interrupt) to wake up the core from IDLE mode. Once you disable
the interrupt, the core will sleep forever.

-Aubrey

2006-09-24 03:49:19

by Luke Yang

[permalink] [raw]

Subject: Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18

On 9/24/06, Arnd Bergmann <[email protected]> wrote:
> On Saturday 23 September 2006 08:50, Mike Frysinger wrote:
> > > There is not much point in trying to use the same numbers as an existing
> > > architecture if that means that you have to leave holes like setup().
> > > I don't know if you still have the choice of completely changing the
> > > syscall numbers, but it would make it nicer in the future.
> >
> > we do, fortunately, have this luxury ... so we can look forward to a
> > nice cleaning of our syscall interface
>
> Actually, I have one more general comment here. It would be really nice
> if you could add those files that have nothing specific to blackfin moved
> to include/asm-generic. That would probably include bug.h, current.h,
> flat.h, hardirq.h, ioctls.h, {ipc,msg,sem,msg}buf.h, kmap_types.h, mman.h,
> param.h, pci.h, poll.h, posix_types.h, scatterlist.h, semaphore.h,
> socket.h, sockios.h, stat.h, termbits.h, termios.h, types.h, and unistd.h.
>
> It doesn't really matter if you're the only user of the new files,
> as long as they are generic enough to be used by every future port.
> If the files are specific to nommu, 32bit or little-endian, then
> they should probably have the respective name so the next person can
> do it differently.
>
> For unistd.h, it may be a good idea to leave space for a few syscall
> numbers specific to architectures, so you could start the generic numbers
> at 32 or so.
>
> Of course nobody is forcing you do do that work, but the next person
> trying to do will be really thankful.
Yes I agree that there are many arch header files can be put into the
generic folder. We just simplely followed other architectures. But I
will be glad to help doing this work.
>
> Arnd <><
>

--
Best regards,
Luke Yang
[email protected]

2006-09-24 03:48:59

[permalink] [raw]

Subject: Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18

On Sun, 24 Sep 2006 11:35:31 +0800 Aubrey wrote:

> On 9/23/06, Arnd Bergmann <[email protected]> wrote:
> > > +static uint32_t reloc_stack_operate(unsigned int oper, struct module *mod)
> > > +{
> > > + uint32_t value;
> > > + switch (oper) {
> > > + case R_add:
> > > + {
> > > + value =
> > > + reloc_stack[reloc_stack_tos - 2] +
> > > + reloc_stack[reloc_stack_tos - 1];
> > > + reloc_stack_tos -= 2;
> > > + break;
> > > + }
> >
> > no need for the curly braces here and below
>
> Hmm, but we need one line < 80 columns, don't we?

Yes (preferably), but:
The braces for case R_add above simply aren't needed at all.
And after they are removed, you can indent the remaining code
one less tab stop.

---
~Randy

2006-09-24 04:28:55

[permalink] [raw]

Subject: Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18

On 9/24/06, Randy Dunlap <[email protected]> wrote:
> On Sun, 24 Sep 2006 11:35:31 +0800 Aubrey wrote:
>
> > On 9/23/06, Arnd Bergmann <[email protected]> wrote:
> > > > +static uint32_t reloc_stack_operate(unsigned int oper, struct module *mod)
> > > > +{
> > > > + uint32_t value;
> > > > + switch (oper) {
> > > > + case R_add:
> > > > + {
> > > > + value =
> > > > + reloc_stack[reloc_stack_tos - 2] +
> > > > + reloc_stack[reloc_stack_tos - 1];
> > > > + reloc_stack_tos -= 2;
> > > > + break;
> > > > + }
> > >
> > > no need for the curly braces here and below
> >
> > Hmm, but we need one line < 80 columns, don't we?
>
> Yes (preferably), but:
> The braces for case R_add above simply aren't needed at all.
> And after they are removed, you can indent the remaining code
> one less tab stop.
>
Yeah, the braces for case should be removed. Done.

2006-09-24 07:29:49

by David Woodhouse

[permalink] [raw]

Subject: Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18

On Sat, 2006-09-23 at 19:25 -0400, Robin Getz wrote:
> >On Thu, 2006-09-21 at 11:32 +0800, Luke Yang wrote:
> > > This is the blackfin architecture for 2.6.18, again.
> >
> >Please run 'make headers_check' for blackfin and then verify that you can
> >build libc against the resulting headers.
>
> We can't build libc, but we can build uClibc ;)

That's why I said 'libc' and not 'glibc'.

> This is how we build our toolchain today (mostly). - now we do a make
> prepare, but we will update it.

I don't believe you provided a Kbuild file for your architecture --
without which 'make headers_install' and 'make headers_check' aren't
going to work.

--
dwmw2

2006-09-25 06:54:13

[permalink] [raw]

Subject: Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18

On Sunday 24 September 2006 05:35, Aubrey wrote:
> > This looks racy. What if you get an interrupt after testing need_resched()
> > but before the idle instruction?
> >
> > Normally, this should look like
> >
> > ? ? ? ? while(!need_resched()) {
> > ? ? ? ? ? ? ? ? local_irq_disable();
> > ? ? ? ? ? ? ? ? if (!need_resched())
> > ? ? ? ? ? ? ? ? ? ? ? ? asm volatile("idle");
> > ? ? ? ? ? ? ? ? local_irq_enable();
> > ? ? ? ? }
> >
> > Of course that only works if your idle instruction wakes up on pending
> > interrupts.
>
>
> No, that doesn't work on blackfin. Blackfin need interrupt(here is
> timer interrupt) to wake up the core from IDLE mode. Once you disable
> the interrupt, the core will sleep forever.

Ok, then this is something you should take back as feedback to your
CPU designers. With the behavior you describe, the interrupt latency
(until the point where an application runs) can be up to one jiffy
longer than it should be, which is unacceptable for many real-time
users.

Worse, it means that you can not use the CONFIG_NO_IDLE_HZ option in the
future, because you have no way to guarantee that you ever wake up from
idle if you hit the race.

Arnd <><

2006-09-25 07:49:59

[permalink] [raw]

Subject: Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18

On 9/25/06, Arnd Bergmann <[email protected]> wrote:
> On Sunday 24 September 2006 05:35, Aubrey wrote:
> > > This looks racy. What if you get an interrupt after testing need_resched()
> > > but before the idle instruction?
> > >
> > > Normally, this should look like
> > >
> > > while(!need_resched()) {
> > > local_irq_disable();
> > > if (!need_resched())
> > > asm volatile("idle");
> > > local_irq_enable();
> > > }
> > >
> > > Of course that only works if your idle instruction wakes up on pending
> > > interrupts.
> >
> >
> > No, that doesn't work on blackfin. Blackfin need interrupt(here is
> > timer interrupt) to wake up the core from IDLE mode. Once you disable
> > the interrupt, the core will sleep forever.
>
> Ok, then this is something you should take back as feedback to your
> CPU designers. With the behavior you describe, the interrupt latency
> (until the point where an application runs) can be up to one jiffy
> longer than it should be, which is unacceptable for many real-time
> users.
>
> Worse, it means that you can not use the CONFIG_NO_IDLE_HZ option in the
> future, because you have no way to guarantee that you ever wake up from
> idle if you hit the race.
>

Oh, sorry for my unclear description, any of the peripherals can be configured
to wake up the core from its idled state to process the interrupt on
Blackfin. I should say __if no other interrupts__ here is the timer
interrupt waking up the processor every one jiffy.

I don't understand if interrupt occurs between need_resched() and the
idle instruction, what will become the racy object? Can you comment it
detailed? thanks.

-Aubrey

2006-09-25 09:26:26

[permalink] [raw]

Subject: Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18

On Monday 25 September 2006 09:49, Aubrey wrote:
> Oh, sorry for my unclear description, any of the peripherals can be configured
> to wake up the core from its idled state to process the interrupt on
> Blackfin. I should say __if no other interrupts__ here is the timer
> interrupt waking up the processor every one jiffy.

And that works if interrupts are disabled as it should?

> I don't understand if interrupt occurs between need_resched() and the
> idle instruction, what will become the racy object? Can you comment it
> detailed? thanks.

It's the same problem as why sleep_on() is wrong and wait_event() is
right, you can find lots of documentation about that.

Think about a process calling nanosleep() to wait for a timeout.
It eventually ends up going to sleep and the kernel schedules
the idle task.

The idle task checks need_resched(), which returns false, so it
will call the idle instruction. Before it gets there, the timer
tick happens, which calls the timer softirq. That softirq notices
that the user process should continue running and calls wake_up(),
which makes the condition for need_resched() true.

Since we're handling that softirq that interrupted the idle task,
that task continues what it was last doing and calls the idle instruction.
This is the point where it goes wrong, because your user task should
run, but the CPU is sleeping until the next interrupt happens.

Note that you should in the future disable timer ticks during the
idle function (CONFIG_NO_IDLE_HZ or similar) to save more power, but
in that case the CPU may be in idle indefinitely after missing the
one interrupt that should have woken it up.

Arnd <><

2006-09-25 09:39:45

by Luke Yang

[permalink] [raw]

Subject: Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18

On 9/25/06, Arnd Bergmann <[email protected]> wrote:
> On Monday 25 September 2006 09:49, Aubrey wrote:
> > Oh, sorry for my unclear description, any of the peripherals can be configured
> > to wake up the core from its idled state to process the interrupt on
> > Blackfin. I should say __if no other interrupts__ here is the timer
> > interrupt waking up the processor every one jiffy.
>
> And that works if interrupts are disabled as it should?
>
> > I don't understand if interrupt occurs between need_resched() and the
> > idle instruction, what will become the racy object? Can you comment it
> > detailed? thanks.
>
> It's the same problem as why sleep_on() is wrong and wait_event() is
> right, you can find lots of documentation about that.
>
> Think about a process calling nanosleep() to wait for a timeout.
> It eventually ends up going to sleep and the kernel schedules
> the idle task.
>
> The idle task checks need_resched(), which returns false, so it
> will call the idle instruction. Before it gets there, the timer
> tick happens, which calls the timer softirq. That softirq notices
> that the user process should continue running and calls wake_up(),
> which makes the condition for need_resched() true.
>
> Since we're handling that softirq that interrupted the idle task,
> that task continues what it was last doing and calls the idle instruction.
> This is the point where it goes wrong, because your user task should
> run, but the CPU is sleeping until the next interrupt happens.
Got it. You are right. We'll check to see what we can do on Blackfin.
Thank you!

>
> Note that you should in the future disable timer ticks during the
> idle function (CONFIG_NO_IDLE_HZ or similar) to save more power, but
> in that case the CPU may be in idle indefinitely after missing the
> one interrupt that should have woken it up.
>
> Arnd <><
>

--
Best regards,
Luke Yang
[email protected]

2006-09-25 09:45:22

[permalink] [raw]

Subject: Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18

On 9/25/06, Arnd Bergmann <[email protected]> wrote:
> On Monday 25 September 2006 09:49, Aubrey wrote:
> > Oh, sorry for my unclear description, any of the peripherals can be configured
> > to wake up the core from its idled state to process the interrupt on
> > Blackfin. I should say __if no other interrupts__ here is the timer
> > interrupt waking up the processor every one jiffy.
>
> And that works if interrupts are disabled as it should?

No, We are trying to figure out what's going on.

>
> > I don't understand if interrupt occurs between need_resched() and the
> > idle instruction, what will become the racy object? Can you comment it
> > detailed? thanks.
>
> It's the same problem as why sleep_on() is wrong and wait_event() is
> right, you can find lots of documentation about that.
>
> Think about a process calling nanosleep() to wait for a timeout.
> It eventually ends up going to sleep and the kernel schedules
> the idle task.
>
> The idle task checks need_resched(), which returns false, so it
> will call the idle instruction. Before it gets there, the timer
> tick happens, which calls the timer softirq. That softirq notices
> that the user process should continue running and calls wake_up(),
> which makes the condition for need_resched() true.
>
> Since we're handling that softirq that interrupted the idle task,
> that task continues what it was last doing and calls the idle instruction.
> This is the point where it goes wrong, because your user task should
> run, but the CPU is sleeping until the next interrupt happens.
>
> Note that you should in the future disable timer ticks during the
> idle function (CONFIG_NO_IDLE_HZ or similar) to save more power, but
> in that case the CPU may be in idle indefinitely after missing the
> one interrupt that should have woken it up.
>
Very clear, thanks a lot.
-Aubrey

2006-09-25 15:39:33

[permalink] [raw]

Subject: Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18

On 9/25/06, Arnd Bergmann <[email protected]> wrote:
> It's the same problem as why sleep_on() is wrong and wait_event() is
> right, you can find lots of documentation about that.
>
> Think about a process calling nanosleep() to wait for a timeout.
> It eventually ends up going to sleep and the kernel schedules
> the idle task.
>
> The idle task checks need_resched(), which returns false, so it
> will call the idle instruction. Before it gets there, the timer
> tick happens, which calls the timer softirq. That softirq notices
> that the user process should continue running and calls wake_up(),
> which makes the condition for need_resched() true.
>
> Since we're handling that softirq that interrupted the idle task,
> that task continues what it was last doing and calls the idle instruction.
> This is the point where it goes wrong, because your user task should
> run, but the CPU is sleeping until the next interrupt happens.
>
> Note that you should in the future disable timer ticks during the
> idle function (CONFIG_NO_IDLE_HZ or similar) to save more power, but
> in that case the CPU may be in idle indefinitely after missing the
> one interrupt that should have woken it up.
>

I digged into the code and got something different.
Between need_resched() and IDLE instruction, a timer interrupt occurs.
Yes, softirq may not schedule out to run the user task, but the
interrupt handler will.
You can find in our patch, I believe the same behavior is on the ARM/M68K.

1) Timer interrupt will call do_irq(), then return_from_int().

2) return_from_int() will check if there is interrupt pending or
signal pending, if so, it will call schedule_and_signal_from_int().

3) schedule_and_signal_from_int() will jump to resume_userspace()

4) resume_userspace() will call _schedule to run the user task.

So, here is no interrupt latency. The user task will run even between
need_resched() and IDLE instruction.

-Aubrey

2006-09-25 16:51:22

[permalink] [raw]

Subject: Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18

On Thu, 21 Sep 2006 11:32:57 +0800 Luke Yang wrote:

> Hi everyone,
>
> This is the blackfin architecture for 2.6.18, again. As we promised,
> we fixed some issues in our old patches as following.
>
> - use serial core in that driver
>
> - Fix up that ioctl so it a) doesn't sleep in spinlock and b) compiles
>
> - Use generic IRQ framework
>
> - Review all the volatiles, consolidate them in some helper-in-header-file.
>
> And we also fixed a lot of other issues and ported it to 2.6.18 now.
> As usual, this architecture patch is too big so I just give a link
> here. Please review it and give you comments, we really appreciate.
>
> http://blackfin.uclinux.org/frs/download.php/1010/blackfin_arch_2.6.18.patch

Hi,

Here are the rest of my comments (on .c files now).
(Did you also look at my comments inside this file?
http://www.xenotime.net/linux/doc/blackfin-arch-meta.patch )

1. As Arnd commented, eliminate the multiple versions of
DPRINTK() and friends macros. Just use pr_debug() as much as possible.

2. Re-read Documentation/CodingStyle and frame it.

3. There are lots of function descriptions that need only small
changes to be in kernel-doc format
(see Documentation/kernel-doc-nano-HOWTO.txt).

4. Try to keep source lines <= 80 columns.

5. Lots of printk() calls could use some KERN_* levels in them.

Details below.
---
~Randy

diff -urN linux-2.6.18/arch/blackfin/kernel/bfin_dma_5xx.c linux-2.6.18.patch1/arch/blackfin/kernel/bfin_dma_5xx.c
--- linux-2.6.18/arch/blackfin/kernel/bfin_dma_5xx.c 1970-01-01 08:00:00.000000000 +0800
+++ linux-2.6.18.patch1/arch/blackfin/kernel/bfin_dma_5xx.c 2006-09-21 09:29:49.000000000 +0800
@@ -0,0 +1,749 @@
+
+int dma_channel_active(unsigned int channel)
+{
+ if (dma_ch[channel].chan_status == DMA_CHANNEL_FREE) {
+ return 0;
+ } else {
+ return 1;
+ }

Drop all of those braces. Not needed nor wanted (see Doc/CodingStyle).

+}
+
+/*------------------------------------------------------------------------------
+* Set the Start Address register for the specific DMA channel
+* This function can be used for register based DMA,
+* to setup the start address
+* addr: Starting address of the DMA Data to be transferred.
+*-----------------------------------------------------------------------------*/

Please convert that to kernel-doc format.

+void set_dma_start_addr(unsigned int channel, unsigned long addr)
+{
+ DMA_DBG("set_dma_start_addr() : BEGIN \n");
+
+ assert(dma_ch[channel].chan_status != DMA_CHANNEL_FREE
+ && channel < MAX_BLACKFIN_DMA_CHANNEL);
+
+ dma_ch[channel].regs->start_addr = addr;
+ SSYNC;
+ DMA_DBG("set_dma_start_addr() : END\n");
+}

Ideally, non-static functions (and static ones that have pointer/method
access) would be documented via kernel-doc....

+void set_dma_next_desc_addr(unsigned int channel, unsigned long addr)
+{
+ DMA_DBG("set_dma_next_desc_addr() : BEGIN \n");
+
+ assert(dma_ch[channel].chan_status != DMA_CHANNEL_FREE
+ && channel < MAX_BLACKFIN_DMA_CHANNEL);
+
+ dma_ch[channel].regs->next_desc_ptr = addr;
+ SSYNC;
+ DMA_DBG("set_dma_start_addr() : END\n");
+}
diff -urN linux-2.6.18/arch/blackfin/kernel/bfin_ksyms.c linux-2.6.18.patch1/arch/blackfin/kernel/bfin_ksyms.c
--- linux-2.6.18/arch/blackfin/kernel/bfin_ksyms.c 1970-01-01 08:00:00.000000000 +0800
+++ linux-2.6.18.patch1/arch/blackfin/kernel/bfin_ksyms.c 2006-09-21 09:29:49.000000000 +0800
@@ -0,0 +1,114 @@

Current practice is to put EXPORTs in the source file where they are
defined, preferably immediately after each function (or data), not
in a group at one place in the source file.

+EXPORT_SYMBOL(__ioremap);
+EXPORT_SYMBOL(strcmp);
+EXPORT_SYMBOL(strncmp);
+EXPORT_SYMBOL(dump_thread);
+
+EXPORT_SYMBOL(ip_fast_csum);
+
+EXPORT_SYMBOL(kernel_thread);
+
+EXPORT_SYMBOL(__up);
+EXPORT_SYMBOL(__down);
+EXPORT_SYMBOL(__down_trylock);
+EXPORT_SYMBOL(__down_interruptible);
+
+EXPORT_SYMBOL(is_in_rom);
+
+/* Networking helper routines. */
+EXPORT_SYMBOL(csum_partial_copy);
+
+/* The following are special because they're not called
+ explicitly (the C compiler generates them). Fortunately,
+ their interface isn't gonna change any time soon now, so
+ it's OK to leave it out of version control. */
+EXPORT_SYMBOL(memcpy);
+EXPORT_SYMBOL(memset);
+EXPORT_SYMBOL(memcmp);
+EXPORT_SYMBOL(memmove);
+EXPORT_SYMBOL(memchr);
+EXPORT_SYMBOL(get_wchan);
diff -urN linux-2.6.18/arch/blackfin/kernel/init_task.c linux-2.6.18.patch1/arch/blackfin/kernel/init_task.c
--- linux-2.6.18/arch/blackfin/kernel/init_task.c 1970-01-01 08:00:00.000000000 +0800
+++ linux-2.6.18.patch1/arch/blackfin/kernel/init_task.c 2006-09-21 09:29:49.000000000 +0800
@@ -0,0 +1,63 @@
+
+union thread_union init_thread_union
+ __attribute__ ((__section__(".data.init_task"))) = {
+INIT_THREAD_INFO(init_task)};

last line needs to be indented.

diff -urN linux-2.6.18/arch/blackfin/kernel/irqchip.c linux-2.6.18.patch1/arch/blackfin/kernel/irqchip.c
--- linux-2.6.18/arch/blackfin/kernel/irqchip.c 1970-01-01 08:00:00.000000000 +0800
+++ linux-2.6.18.patch1/arch/blackfin/kernel/irqchip.c 2006-09-21 09:29:49.000000000 +0800
@@ -0,0 +1,150 @@
+
+void ack_bad_irq(unsigned int irq)
+{
+ irq_err_count += 1;
+ printk(KERN_ERR "IRQ: spurious interrupt %d\n", irq);

If these happen at all, you may need to rate-limit the printk() call by
using printk_ratelimit() instead.

+}
+EXPORT_SYMBOL(ack_bad_irq);
+
diff -urN linux-2.6.18/arch/blackfin/kernel/module.c linux-2.6.18.patch1/arch/blackfin/kernel/module.c
--- linux-2.6.18/arch/blackfin/kernel/module.c 1970-01-01 08:00:00.000000000 +0800
+++ linux-2.6.18.patch1/arch/blackfin/kernel/module.c 2006-09-21 09:29:49.000000000 +0800
@@ -0,0 +1,469 @@
+
+static uint32_t reloc_stack_operate(unsigned int oper, struct module *mod)
+{
+ uint32_t value;
+ switch (oper) {
+ case R_add:

Save one indentation level for the entire block (here & below) by putting the
braces at the same column as the word "case".

+ {
+ value =
+ reloc_stack[reloc_stack_tos - 2] +
+ reloc_stack[reloc_stack_tos - 1];
+ reloc_stack_tos -= 2;
+ break;
+ }
+ case R_sub:
+ {
+ value =
+ reloc_stack[reloc_stack_tos - 2] -
+ reloc_stack[reloc_stack_tos - 1];
+ reloc_stack_tos -= 2;
+ break;
+ }
+ case R_mult:
+ {
+ value =
+ reloc_stack[reloc_stack_tos -
+ 2] * reloc_stack[reloc_stack_tos - 1];
+ reloc_stack_tos -= 2;
+ break;
+ }
+ case R_div:
+ {
+ value =
+ reloc_stack[reloc_stack_tos -
+ 2] / reloc_stack[reloc_stack_tos - 1];
+ reloc_stack_tos -= 2;
+ break;
+ }
+ case R_mod:
+ {
+ value =
+ reloc_stack[reloc_stack_tos -
+ 2] % reloc_stack[reloc_stack_tos - 1];
+ reloc_stack_tos -= 2;
+ break;
+ }
+ default:
+ {
+ printk(KERN_WARNING "module %s: unhandled reloction\n",
+ mod->name);
+ return 0;
+ }
+ }
+}
+
+/*************************************************************************/
+/* FUNCTION : apply_relocate_add */
+/* ABSTRACT : Blackfin specific relocation handling for the loadable */
+/* modules. Modules are expected to be .o files. */
+/* Arithmetic relocations are handled. */
+/* We do not expect LSETUP to be split and hence is not */
+/* handled. */
+/* R_byte and R_byte2 are also not handled as the gas */
+/* does not generate it. */
+/*************************************************************************/

Using kernel-doc doc/comment format would be much better for everyone.
See Documentation/kernel-doc-nano-HOWTO.txt for info/details.

+int
+apply_relocate_add(Elf_Shdr * sechdrs, const char *strtab,
+ unsigned int symindex, unsigned int relsec,
+ struct module *mod)
+{
+ case R_luimm16:

Align braces with "case".

+ {
+ unsigned short tmp;
+ DEBUGP("before %x after %x\n", *location16,
+ (value & 0xffff));
+ tmp = (value & 0xffff);
+ dma_memcpy(location16, &tmp, 2);
+ }
+ break;
+}
+
diff -urN linux-2.6.18/arch/blackfin/kernel/process.c linux-2.6.18.patch1/arch/blackfin/kernel/process.c
--- linux-2.6.18/arch/blackfin/kernel/process.c 1970-01-01 08:00:00.000000000 +0800
+++ linux-2.6.18.patch1/arch/blackfin/kernel/process.c 2006-09-21 09:29:49.000000000 +0800
@@ -0,0 +1,346 @@
+
+/*
+ * sys_execve() executes a new program.
+ */
+
+asmlinkage int sys_execve(char *name, char **argv, char **envp)
+{
+ int error;
+ char *filename;
+ struct pt_regs *regs = (struct pt_regs *)((&name) + 6);
+
+ lock_kernel();
+ filename = getname(name);
+ error = PTR_ERR(filename);
+ if (IS_ERR(filename))
+ goto out;
+ error = do_execve(filename, argv, envp, regs);
+ putname(filename);
+ out:

Unindent the label:

+ unlock_kernel();
+ return error;
+}
+
+unsigned long get_wchan(struct task_struct *p)
+{
+ unsigned long fp, pc;
+ unsigned long stack_page;
+ int count = 0;
+ if (!p || p == current || p->state == TASK_RUNNING)
+ return 0;
+
+ stack_page = (unsigned long)p;
+ fp = p->thread.usp;
+ do {
+ if (fp < stack_page + sizeof(struct thread_info) ||
+ fp >= 8184 + stack_page)
+ return 0;
+ pc = ((unsigned long *)fp)[1];
+ if (!in_sched_functions(pc))
+ return pc;
+ fp = *(unsigned long *)fp;
+ }
+ while (count++ < 16);

} while (count++ < 16);
See Documentation/CodingStyle :)

+ return 0;
+}
+
diff -urN linux-2.6.18/arch/blackfin/kernel/ptrace.c linux-2.6.18.patch1/arch/blackfin/kernel/ptrace.c
--- linux-2.6.18/arch/blackfin/kernel/ptrace.c 1970-01-01 08:00:00.000000000 +0800
+++ linux-2.6.18.patch1/arch/blackfin/kernel/ptrace.c 2006-09-21 09:29:49.000000000 +0800
@@ -0,0 +1,431 @@
+
+long arch_ptrace(struct task_struct *child, long request, long addr, long data)
+{
+ int ret;
+ int add = 0;
+
+ switch (request) {
+ /* when I and D space are separate, these will need to be fixed. */
+ case PTRACE_PEEKDATA:
+#ifdef DEBUG
+ printk("PTRACE_PEEKDATA\n");
+#endif
+ add = MAX_SHARED_LIBS * 4; /* space between text and data */
+ /* fall through */
+ case PTRACE_PEEKTEXT: /* read word at location addr. */
+ {

Braces at same column as "case".

+ unsigned long tmp = 0;
+ int copied;
+
+ ret = -EIO;
+ }
+
+ /* read the word at location addr in the USER area. */
+ case PTRACE_PEEKUSR:
+ {

ditto

+ unsigned long tmp;
+ break;
+ }
+
+ /* when I and D space are separate, this will have to be fixed. */
+ case PTRACE_POKEDATA:
+ printk(KERN_NOTICE "ptrace: PTRACE_PEEKDATA\n");
+ /* fall through */
+ case PTRACE_POKETEXT: /* write the word at location addr. */
+ {

ditto

+ break;
+ }
+
+ case PTRACE_SYSCALL: /* continue and stop at next (return from) syscall */
+ case PTRACE_CONT:
+ { /* restart after signal. */

ditto

+ long tmp;
+ break;
+ }
+
+/*
+ * make the child exit. Best I can do is send it a sigkill.
+ * perhaps it should be put in the status that it wants to
+ * exit.
+ */
+ case PTRACE_KILL:

Put braces at same column as "case" and then undent the entire block.
(many of these)

+ {
+ long tmp;
+ ret = 0;
+ if (child->exit_state == EXIT_ZOMBIE) /* already dead */
+ break;
+ child->exit_code = SIGKILL;
+ /* make sure the single step bit is not set. */
+ tmp = get_reg(child, PT_SYSCFG) & ~(TRACE_BITS);
+ put_reg(child, PT_SYSCFG, tmp);
+ wake_up_process(child);
+ break;
+ }
+
+ default:
+ printk(KERN_NOTICE "ptrace: *** Unhandled case **** %d\n",
+ (int)request);
+ ret = -EIO;
+ break;
+ }
+
+ return ret;
+}
+
diff -urN linux-2.6.18/arch/blackfin/kernel/setup.c linux-2.6.18.patch1/arch/blackfin/kernel/setup.c
--- linux-2.6.18/arch/blackfin/kernel/setup.c 1970-01-01 08:00:00.000000000 +0800
+++ linux-2.6.18.patch1/arch/blackfin/kernel/setup.c 2006-09-21 09:29:49.000000000 +0800
@@ -0,0 +1,941 @@
+
+u_long get_sclk()
+{
+ u_long ssel;
+
+ if (bfin_read_PLL_STAT() & 0x1)
+ return CONFIG_CLKIN_HZ;
+
+ ssel = (bfin_read_PLL_DIV() & 0xf);
+ if (0 == ssel) {

in Linux kernel, put constant on the right hand side:
if (ssel == 0)

+ printk(KERN_WARNING "Invalid System Clock\n");
+ ssel = 1;
+ }
+
+ return get_vco() / ssel;
+}
+
+EXPORT_SYMBOL(get_sclk);
+
diff -urN linux-2.6.18/arch/blackfin/kernel/signal.c linux-2.6.18.patch1/arch/blackfin/kernel/signal.c
--- linux-2.6.18/arch/blackfin/kernel/signal.c 1970-01-01 08:00:00.000000000 +0800
+++ linux-2.6.18.patch1/arch/blackfin/kernel/signal.c 2006-09-21 09:29:49.000000000 +0800
@@ -0,0 +1,715 @@
+
+asmlinkage void do_signal(struct pt_regs *regs);

unneeded prototype.

+static inline int
+restore_sigcontext(struct pt_regs *regs, struct sigcontext *usc, int *pr0)
+{
+ struct sigcontext context;
+ int err = 0;
+
+ /* get previous context */
+ if (copy_from_user(&context, usc, sizeof(context)))
+ goto badframe;
+ if (context.sc_pc == 0)
+ goto badframe;
+ /* restore passed registers */
+ wrusp(context.sc_usp);
+
+ *pr0 = context.sc_r0;
+ return err;
+
+ badframe:

Don't indent labels so much (either not at all or may 1-2 spaces).
Basically they should stand out (be easy to see), unlike this one.

+ return 1;
+}
+
+static inline int
+rt_restore_ucontext(struct pt_regs *regs, struct ucontext *uc, int *pr0)
+{
+ int temp;
+ greg_t *gregs = uc->uc_mcontext.gregs;
+ unsigned long usp;
+ int err;
+
+ err = __get_user(temp, &uc->uc_mcontext.version);
+ if (temp != MCONTEXT_VERSION)
+ goto badframe;
+ /* restore passed registers */
+ wrusp(usp);
+
+ if (do_sigaltstack(&uc->uc_stack, NULL, usp) == -EFAULT)
+ goto badframe;
+
+ *pr0 = regs->r0;
+ return err;
+
+ badframe:

Label indented too much.
(global comment, for this and other source files)

+ return 1;
+}
+
diff -urN linux-2.6.18/arch/blackfin/kernel/sys_bfin.c linux-2.6.18.patch1/arch/blackfin/kernel/sys_bfin.c
--- linux-2.6.18/arch/blackfin/kernel/sys_bfin.c 1970-01-01 08:00:00.000000000 +0800
+++ linux-2.6.18.patch1/arch/blackfin/kernel/sys_bfin.c 2006-09-21 09:29:49.000000000 +0800
@@ -0,0 +1,254 @@
+
+/* common code for old and new mmaps */
+static inline long
+do_mmap2(unsigned long addr, unsigned long len,
+ unsigned long prot, unsigned long flags,
+ unsigned long fd, unsigned long pgoff)
+{
+ int error = -EBADF;
+ struct file *file = NULL;
+
+ flags &= ~(MAP_EXECUTABLE | MAP_DENYWRITE);
+ if (!(flags & MAP_ANONYMOUS)) {
+ file = fget(fd);
+ if (!file)
+ goto out;
+ }
+
+ down_write(&current->mm->mmap_sem);
+ error = do_mmap_pgoff(file, addr, len, prot, flags, pgoff);
+ up_write(&current->mm->mmap_sem);
+
+ if (file)
+ fput(file);
+ out:

Undent (or outdent) label.
(more in same file)

+ return error;
+}
+
+/*
+ * sys_ipc() is the de-multiplexer for the SysV IPC calls..
+ *
+ * This is really horribly ugly.
+ */
+asmlinkage int
+sys_ipc(uint call, int first, int second, int third, void *ptr, long fifth)
+{
+ int version, ret;
+
+ version = call >> 16; /* hack for backward compatibility */
+ call &= 0xffff;
+
+ if (call <= SEMCTL)
+ switch (call) {
+ case SEMOP:
+ return sys_semop(first, (struct sembuf *)ptr, second);
+ case SEMGET:
+ return sys_semget(first, second, third);
+ case SEMCTL:

Align braces and "case" and then outdent the rest of the block.
More below [deleted].

+ {
+ union semun fourth;
+ if (!ptr)
+ return -EINVAL;
+ if (get_user(fourth.__pad, (void **)ptr))
+ return -EFAULT;
+ return sys_semctl(first, second, third, fourth);
+ }
+ default:
+ return -ENOSYS;
+ }
+}
+
diff -urN linux-2.6.18/arch/blackfin/kernel/time.c linux-2.6.18.patch1/arch/blackfin/kernel/time.c
--- linux-2.6.18/arch/blackfin/kernel/time.c 1970-01-01 08:00:00.000000000 +0800
+++ linux-2.6.18.patch1/arch/blackfin/kernel/time.c 2006-09-21 09:29:49.000000000 +0800
@@ -0,0 +1,336 @@
+
+static void
+time_sched_init(irqreturn_t(*timer_routine) (int, void *, struct pt_regs *))
+{
+ u32 tcount;
+
+ /* power up the timer, but don't enable it just yet */
+ bfin_write_TCNTL(1);
+ __builtin_bfin_csync();
+
+ /*
+ * the TSCALE prescaler counter.
+ */
+ bfin_write_TSCALE((TIME_SCALE - 1));
+
+ tcount = ((get_cclk() / (HZ * TIME_SCALE)) - 1);
+ bfin_write_TPERIOD(tcount);
+ bfin_write_TCOUNT(tcount);
+
+ /* now enable the timer */
+ __builtin_bfin_csync();
+
+ bfin_write_TCNTL(7);
+
+ bfin_timer_irq.handler = timer_routine;
+ /* call setup_irq instead of request_irq because request_irq calls kmalloc which has not been initialized yet */

Split comment onto multiple lines (line is too long).

+ setup_irq(IRQ_CORETMR, &bfin_timer_irq);
+}
+
+void do_gettimeofday(struct timeval *tv)
+{
+ unsigned long flags;
+ unsigned long lost, seq;
+ unsigned long usec, sec;
+
+ do {
+ seq = read_seqbegin_irqsave(&xtime_lock, flags);
+ usec = gettimeoffset();
+ lost = jiffies - wall_jiffies;
+ if (unlikely(lost))
+ usec += lost * (USEC_PER_SEC / HZ);
+ sec = xtime.tv_sec;
+ usec += (xtime.tv_nsec / NSEC_PER_USEC);
+ }
+ while (read_seqretry_irqrestore(&xtime_lock, seq, flags));

Combine } and while(expression) onto one line.

+
+ while (usec >= USEC_PER_SEC) {
+ usec -= USEC_PER_SEC;
+ sec++;
+ }
+
+ tv->tv_sec = sec;
+ tv->tv_usec = usec;
+}
+
+EXPORT_SYMBOL(do_gettimeofday);
+
diff -urN linux-2.6.18/arch/blackfin/lib/checksum.c linux-2.6.18.patch1/arch/blackfin/lib/checksum.c
--- linux-2.6.18/arch/blackfin/lib/checksum.c 1970-01-01 08:00:00.000000000 +0800
+++ linux-2.6.18.patch1/arch/blackfin/lib/checksum.c 2006-09-21 09:29:49.000000000 +0800
@@ -0,0 +1,139 @@
+
+/*
+ * computes the checksum of a memory block at buff, length len,
+ * and adds in "sum" (32-bit)
+ *
+ * returns a 32-bit number suitable for feeding into itself
+ * or csum_tcpudp_magic
+ *
+ * this function must be called with even lengths, except
+ * for the last fragment, which may be odd
+ *
+ * it's best to have buff aligned on a 32-bit boundary
+ */

Please just use kernel-doc notation for that function comment block.

+unsigned int csum_partial(const unsigned char *buff, int len, unsigned int sum)
+{
+ /*
+ * Just in case we get nasty checksum data...
+ * Like 0xffff6ec3 in the case of our IPv6 multicast header.
+ * We fold to begin with, as well as at the end.
+ */
+ sum = (sum & 0xffff) + (sum >> 16);
+
+ sum += do_csum(buff, len);
+
+ sum = (sum & 0xffff) + (sum >> 16);
+
+ return sum;
+}
+
diff -urN linux-2.6.18/arch/blackfin/mach-bf533/boards/cm_bf533.c linux-2.6.18.patch1/arch/blackfin/mach-bf533/boards/cm_bf533.c
--- linux-2.6.18/arch/blackfin/mach-bf533/boards/cm_bf533.c 1970-01-01 08:00:00.000000000 +0800
+++ linux-2.6.18.patch1/arch/blackfin/mach-bf533/boards/cm_bf533.c 2006-09-21 09:29:49.000000000 +0800
@@ -0,0 +1,236 @@
+
+#if defined(CONFIG_SPI_BFIN) || defined(CONFIG_SPI_BFIN_MODULE)
+/* all SPI perpherals info goes here */

peripherals

+static int __init cm_bf533_init(void)
+{
+ printk("%s(): registering device resources\n", __FUNCTION__);

KERN_INFO ?

+ return platform_add_devices(cm_bf533_devices,
+ ARRAY_SIZE(cm_bf533_devices));
+}
+
diff -urN linux-2.6.18/arch/blackfin/mach-bf533/boards/ezkit.c linux-2.6.18.patch1/arch/blackfin/mach-bf533/boards/ezkit.c
--- linux-2.6.18/arch/blackfin/mach-bf533/boards/ezkit.c 1970-01-01 08:00:00.000000000 +0800
+++ linux-2.6.18.patch1/arch/blackfin/mach-bf533/boards/ezkit.c 2006-09-21 09:29:49.000000000 +0800
@@ -0,0 +1,213 @@
+
+#if defined(CONFIG_SPI_BFIN) || defined(CONFIG_SPI_BFIN_MODULE)
+/* all SPI perpherals info goes here */

peripherals

+static int __init ezkit_init(void)
+{
+ printk("%s(): registering device resources\n", __FUNCTION__);

KERN_INFO ?

+ platform_add_devices(ezkit_devices, ARRAY_SIZE(ezkit_devices));
+#if defined(CONFIG_SPI_BFIN) || defined(CONFIG_SPI_BFIN_MODULE)
+ spi_register_board_info(bfin_spi_board_info,
+ ARRAY_SIZE(bfin_spi_board_info));
+#endif
+ return 0;
+}
+
diff -urN linux-2.6.18/arch/blackfin/mach-bf533/boards/generic_board.c linux-2.6.18.patch1/arch/blackfin/mach-bf533/boards/generic_board.c
--- linux-2.6.18/arch/blackfin/mach-bf533/boards/generic_board.c 1970-01-01 08:00:00.000000000 +0800
+++ linux-2.6.18.patch1/arch/blackfin/mach-bf533/boards/generic_board.c 2006-09-21 09:29:49.000000000 +0800
@@ -0,0 +1,78 @@
+
+#include <linux/device.h>
+#include <asm/irq.h>
+
+{
+ printk("%s(): registering device resources\n", __FUNCTION__);

KERN_INFO ?

+ return platform_add_devices(generic_board_devices,
+ ARRAY_SIZE(generic_board_devices));
+}
+
+arch_initcall(generic_board_init);
diff -urN linux-2.6.18/arch/blackfin/mach-bf533/boards/stamp.c linux-2.6.18.patch1/arch/blackfin/mach-bf533/boards/stamp.c
--- linux-2.6.18/arch/blackfin/mach-bf533/boards/stamp.c 1970-01-01 08:00:00.000000000 +0800
+++ linux-2.6.18.patch1/arch/blackfin/mach-bf533/boards/stamp.c 2006-09-21 09:29:49.000000000 +0800
@@ -0,0 +1,265 @@
+
+static int __init stamp_init(void)
+{
+ printk("%s(): registering device resources\n", __FUNCTION__);

KERN_INFO ?

+ platform_add_devices(stamp_devices, ARRAY_SIZE(stamp_devices));
+#if defined(CONFIG_SPI_BFIN) || defined(CONFIG_SPI_BFIN_MODULE)
+ spi_register_board_info(bfin_spi_board_info,
+ ARRAY_SIZE(bfin_spi_board_info));
+#endif
+ return 0;
+}
diff -urN linux-2.6.18/arch/blackfin/mach-bf533/cpu.c linux-2.6.18.patch1/arch/blackfin/mach-bf533/cpu.c
--- linux-2.6.18/arch/blackfin/mach-bf533/cpu.c 1970-01-01 08:00:00.000000000 +0800
+++ linux-2.6.18.patch1/arch/blackfin/mach-bf533/cpu.c 2006-09-21 09:29:49.000000000 +0800
@@ -0,0 +1,169 @@
+
+ /**/

Drop comment above.
Don't indent the function intro below.

+ static int bf533_target(struct cpufreq_policy *policy,
+ unsigned int target_freq, unsigned int relation)
+{
+}
+
diff -urN linux-2.6.18/arch/blackfin/mach-bf533/ints-priority.c linux-2.6.18.patch1/arch/blackfin/mach-bf533/ints-priority.c
--- linux-2.6.18/arch/blackfin/mach-bf533/ints-priority.c 1970-01-01 08:00:00.000000000 +0800
+++ linux-2.6.18.patch1/arch/blackfin/mach-bf533/ints-priority.c 2006-09-21 09:29:49.000000000 +0800
@@ -0,0 +1,69 @@
+
+void program_IAR(void);

Don't really need a function prototype when the function follows
immediately after it.

+/*Program the IAR registers*/
+
+void program_IAR(void)
+{
+};
diff -urN linux-2.6.18/arch/blackfin/mach-bf533/pm.c linux-2.6.18.patch1/arch/blackfin/mach-bf533/pm.c
--- linux-2.6.18/arch/blackfin/mach-bf533/pm.c 1970-01-01 08:00:00.000000000 +0800
+++ linux-2.6.18.patch1/arch/blackfin/mach-bf533/pm.c 2006-09-21 09:29:49.000000000 +0800
@@ -0,0 +1,152 @@
+
+#include <asm/io.h>
+
+/*
+ * When we call pm_suspend, that code enters into idle state.
+ * When there is any interrupt,the core will resume

space after comma

+ */
+void bf533_pm_suspend(void)
+{
+}
+
+/*

Change /* to /** and it will be in kernel-doc format.

+ * bf533_pm_prepare - Do preliminary suspend work.
+ * @state: suspend state we're entering.
+ *
+ */
+
+static int bf533_pm_prepare(suspend_state_t state)
+{
+}
+
+/*

Change /* to /**

+ * bf533_pm_enter - Actually enter a sleep state.
+ * @state: State we're entering.
+ *
+ */
+
+static int bf533_pm_enter(suspend_state_t state)
+{
+}
+
+/**

Ditto.

+ * bf533_pm_finish - Finish up suspend sequence.
+ * @state: State we're coming out of.
+ *
+ * This is called after we wake back up (or if entering the sleep state
+ * failed).
+ */
+
+static int bf533_pm_finish(suspend_state_t state)
+{
+ return 0;
+}
+
diff -urN linux-2.6.18/arch/blackfin/mach-bf537/boards/cm_bf537.c linux-2.6.18.patch1/arch/blackfin/mach-bf537/boards/cm_bf537.c
--- linux-2.6.18/arch/blackfin/mach-bf537/boards/cm_bf537.c 1970-01-01 08:00:00.000000000 +0800
+++ linux-2.6.18.patch1/arch/blackfin/mach-bf537/boards/cm_bf537.c 2006-09-21 09:29:49.000000000 +0800
@@ -0,0 +1,268 @@
+
+#if defined(CONFIG_SPI_BFIN) || defined(CONFIG_SPI_BFIN_MODULE)
+/* all SPI perpherals info goes here */

peripherals

diff -urN linux-2.6.18/arch/blackfin/mach-bf537/boards/generic_board.c linux-2.6.18.patch1/arch/blackfin/mach-bf537/boards/generic_board.c
--- linux-2.6.18/arch/blackfin/mach-bf537/boards/generic_board.c 1970-01-01 08:00:00.000000000 +0800
+++ linux-2.6.18.patch1/arch/blackfin/mach-bf537/boards/generic_board.c 2006-09-21 09:29:49.000000000 +0800
@@ -0,0 +1,469 @@
+
+#if defined(CONFIG_SPI_BFIN) || defined(CONFIG_SPI_BFIN_MODULE)
+/* all SPI perpherals info goes here */

peripherals

diff -urN linux-2.6.18/arch/blackfin/mach-bf537/boards/stamp.c linux-2.6.18.patch1/arch/blackfin/mach-bf537/boards/stamp.c
--- linux-2.6.18/arch/blackfin/mach-bf537/boards/stamp.c 1970-01-01 08:00:00.000000000 +0800
+++ linux-2.6.18.patch1/arch/blackfin/mach-bf537/boards/stamp.c 2006-09-21 09:29:49.000000000 +0800
@@ -0,0 +1,489 @@
+
+#if defined(CONFIG_SPI_BFIN) || defined(CONFIG_SPI_BFIN_MODULE)
+/* all SPI perpherals info goes here */

peripherals

diff -urN linux-2.6.18/arch/blackfin/mach-bf537/cpu.c linux-2.6.18.patch1/arch/blackfin/mach-bf537/cpu.c
--- linux-2.6.18/arch/blackfin/mach-bf537/cpu.c 1970-01-01 08:00:00.000000000 +0800
+++ linux-2.6.18.patch1/arch/blackfin/mach-bf537/cpu.c 2006-09-21 09:29:49.000000000 +0800
@@ -0,0 +1,169 @@
+
+ /**/

Lose the comment above and don't indent the function intro below.

+ static int bf537_target(struct cpufreq_policy *policy,
+ unsigned int target_freq, unsigned int relation)
+{
+ unsigned long cclk_mhz;
+ unsigned long vco_mhz;
+ unsigned long flags;
+ unsigned int index, vco_index;
+ int i;
+
+ struct cpufreq_freqs freqs;
+#if defined(BF533_CPU_DEBUG)
+ printk

KERN_DEBUG ?

+ ("cclk begin change to cclk %d,vco=%d,index=%d,target=%d,oldfreq=%d\n",
+ cclk_mhz, vco_mhz, index, target_freq, freqs.old);
+#endif
+}
+
+/* make sure that only the "userspace" governor is run -- anything else wouldn't make sense on

line above is too long.

+ * this platform, anyway.
+ */
+static int bf537_verify_speed(struct cpufreq_policy *policy)
+{
+ return cpufreq_frequency_table_verify(policy, &bf537_freq_table);
+}
diff -urN linux-2.6.18/arch/blackfin/mach-bf537/ints-priority.c linux-2.6.18.patch1/arch/blackfin/mach-bf537/ints-priority.c
--- linux-2.6.18/arch/blackfin/mach-bf537/ints-priority.c 1970-01-01 08:00:00.000000000 +0800
+++ linux-2.6.18.patch1/arch/blackfin/mach-bf537/ints-priority.c 2006-09-21 09:29:49.000000000 +0800
@@ -0,0 +1,79 @@
+
+void program_IAR(void);

Don't need the prototype.

+/*Program the IAR registers*/
+
+void program_IAR()
+{
+}
diff -urN linux-2.6.18/arch/blackfin/mach-bf537/pm.c linux-2.6.18.patch1/arch/blackfin/mach-bf537/pm.c
--- linux-2.6.18/arch/blackfin/mach-bf537/pm.c 1970-01-01 08:00:00.000000000 +0800
+++ linux-2.6.18.patch1/arch/blackfin/mach-bf537/pm.c 2006-09-21 09:29:49.000000000 +0800
@@ -0,0 +1,150 @@
+
+#include <asm/io.h>
+
+/*

Use /** for kernel-doc format.

+ * bf537_pm_prepare - Do preliminary suspend work.
+ * @state: suspend state we're entering.
+ *
+ */
+
+static int bf537_pm_prepare(suspend_state_t state)
+{
+}
+
+/*

Use /** for kernel-doc format.

+ * bf537_pm_enter - Actually enter a sleep state.
+ * @state: State we're entering.
+ *
+ */
+
+static int bf537_pm_enter(suspend_state_t state)
+{
+}
+
+/**

Good.

+ * bf537_pm_finish - Finish up suspend sequence.
+ * @state: State we're coming out of.
+ *
+ * This is called after we wake back up (or if entering the sleep state
+ * failed).
+ */
+
+static int bf537_pm_finish(suspend_state_t state)
+{
+}
+
diff -urN linux-2.6.18/arch/blackfin/mach-bf561/boards/ezkit.c linux-2.6.18.patch1/arch/blackfin/mach-bf561/boards/ezkit.c
--- linux-2.6.18/arch/blackfin/mach-bf561/boards/ezkit.c 1970-01-01 08:00:00.000000000 +0800
+++ linux-2.6.18.patch1/arch/blackfin/mach-bf561/boards/ezkit.c 2006-09-21 09:29:49.000000000 +0800
@@ -0,0 +1,84 @@
+
+static int __init ezkit_init(void)
+{
+ printk("%s(): registering device resources\n", __FUNCTION__);

KERN_INFO

+ return platform_add_devices(ezkit_devices, ARRAY_SIZE(ezkit_devices));
+}
+
+arch_initcall(ezkit_init);
diff -urN linux-2.6.18/arch/blackfin/mach-bf561/boards/generic_board.c linux-2.6.18.patch1/arch/blackfin/mach-bf561/boards/generic_board.c
--- linux-2.6.18/arch/blackfin/mach-bf561/boards/generic_board.c 1970-01-01 08:00:00.000000000 +0800
+++ linux-2.6.18.patch1/arch/blackfin/mach-bf561/boards/generic_board.c 2006-09-21 09:29:49.000000000 +0800
@@ -0,0 +1,78 @@
+
+static int __init generic_board_init(void)
+{
+ printk("%s(): registering device resources\n", __FUNCTION__);

KERN_INFO

+ return platform_add_devices(generic_board_devices,
+ ARRAY_SIZE(generic_board_devices));
+}
diff -urN linux-2.6.18/arch/blackfin/mach-bf561/coreb.c linux-2.6.18.patch1/arch/blackfin/mach-bf561/coreb.c
--- linux-2.6.18/arch/blackfin/mach-bf561/coreb.c 1970-01-01 08:00:00.000000000 +0800
+++ linux-2.6.18.patch1/arch/blackfin/mach-bf561/coreb.c 2006-09-21 09:29:49.000000000 +0800
@@ -0,0 +1,413 @@
+
+int __init bf561_coreb_init(void)
+{
+
Don't indent labels so much:

+ release_dma_src:
+ free_dma(CH_MEM_STREAM2_SRC);
+ release_dma_dest:
+ free_dma(CH_MEM_STREAM2_DEST);
+ release_data_a_sram:
+ release_mem_region(0xff400000, 0x8000);
+ release_data_b_sram:
+ release_mem_region(0xff500000, 0x8000);
+ release_instruction_b_sram:
+ release_mem_region(0xff610000, 0x4000);
+ release_instruction_a_sram:
+ release_mem_region(0xff600000, 0x4000);
+ exit:
+ return -ENOMEM;
+}
+
diff -urN linux-2.6.18/arch/blackfin/mach-bf561/ints-priority.c linux-2.6.18.patch1/arch/blackfin/mach-bf561/ints-priority.c
--- linux-2.6.18/arch/blackfin/mach-bf561/ints-priority.c 1970-01-01 08:00:00.000000000 +0800
+++ linux-2.6.18.patch1/arch/blackfin/mach-bf561/ints-priority.c 2006-09-21 09:29:49.000000000 +0800
@@ -0,0 +1,113 @@
+
+void program_IAR(void);

Drop the prototype.

+/*Program the IAR registers*/
+
+void program_IAR()
+{
+}
diff -urN linux-2.6.18/arch/blackfin/mach-common/bf5xx_rtc.c linux-2.6.18.patch1/arch/blackfin/mach-common/bf5xx_rtc.c
--- linux-2.6.18/arch/blackfin/mach-common/bf5xx_rtc.c 1970-01-01 08:00:00.000000000 +0800
+++ linux-2.6.18.patch1/arch/blackfin/mach-common/bf5xx_rtc.c 2006-09-21 09:29:49.000000000 +0800
@@ -0,0 +1,140 @@
+
+/* Read the time from the RTC_STAT.
+ * time_in_seconds is seconds since Jan 1970
+ */
+int rtc_get(time_t * time_in_seconds)

No space after '*': just use: time_t *time_in_seconds

+{
+ unsigned long cur_rtc_stat = 0;
+ int tm_sec = 0, tm_min = 0, tm_hour = 0, tm_day = 0;
+
+ if (time_in_seconds == NULL) {
+ return -1;
+ }

Drop the unneeded braces.

+ /* Read the RTC_STAT register */
+ cur_rtc_stat = bfin_read_RTC_STAT();
+}
diff -urN linux-2.6.18/arch/blackfin/mach-common/cplbinfo.c linux-2.6.18.patch1/arch/blackfin/mach-common/cplbinfo.c
--- linux-2.6.18/arch/blackfin/mach-common/cplbinfo.c 1970-01-01 08:00:00.000000000 +0800
+++ linux-2.6.18.patch1/arch/blackfin/mach-common/cplbinfo.c 2006-09-21 09:29:49.000000000 +0800
@@ -0,0 +1,212 @@
+
+static int cplbinfo_write_proc(struct file *file, const char *buffer,
+ unsigned long count, void *data)
+{
+ printk("Reset the CPLB swap in/out counts.\n");
+ memset(ipdt_swapcount_table, 0, 100 * sizeof(unsigned long));
+ memset(dpdt_swapcount_table, 0, 120 * sizeof(unsigned long));

Could you eliminate those magic numbers (100, 120) ?

+ return count;
+}
diff -urN linux-2.6.18/arch/blackfin/mach-common/ints-priority-dc.c linux-2.6.18.patch1/arch/blackfin/mach-common/ints-priority-dc.c
--- linux-2.6.18/arch/blackfin/mach-common/ints-priority-dc.c 1970-01-01 08:00:00.000000000 +0800
+++ linux-2.6.18.patch1/arch/blackfin/mach-common/ints-priority-dc.c 2006-09-21 09:29:49.000000000 +0800
@@ -0,0 +1,545 @@
+
+static void bf561_core_mask_irq(unsigned int irq)
+{
+ irq_flags &= ~(1 << irq);
+ if (!irqs_disabled())
+ local_irq_enable();

should that be local_irq_disable() ?
or you use local_irq_enable() in both core_mask_irq() and
core_unmask_irq() ?

+}
+
+static void bf561_core_unmask_irq(unsigned int irq)
+{
+ irq_flags |= 1 << irq;
+ /*
+ * If interrupts are enabled, IMASK must contain the same value
+ * as irq_flags. Make sure that invariant holds. If interrupts
+ * are currently disabled we need not do anything; one of the
+ * callers will take care of setting IMASK to the proper value
+ * when reenabling interrupts.
+ * local_irq_enable just does "STI irq_flags", so it's exactly
+ * what we need.
+ */
+ if (!irqs_disabled())
+ local_irq_enable();
+ return;
+}
+
diff -urN linux-2.6.18/arch/blackfin/mach-common/ints-priority-sc.c linux-2.6.18.patch1/arch/blackfin/mach-common/ints-priority-sc.c
--- linux-2.6.18/arch/blackfin/mach-common/ints-priority-sc.c 1970-01-01 08:00:00.000000000 +0800
+++ linux-2.6.18.patch1/arch/blackfin/mach-common/ints-priority-sc.c 2006-09-21 09:29:49.000000000 +0800
@@ -0,0 +1,619 @@
+
+volatile unsigned long irq_flags = 0;
+
+/* The number of spurious interrupts */
+volatile unsigned int num_spurious;

too much volatile.

+static void search_IAR(void);

Function prototype not needed.

+/*
+ * Search SIC_IAR and fill tables with the irqvalues
+ * and their positions in the SIC_ISR register.
+ */
+static void __init search_IAR(void)
+{
+ unsigned ivg, irq_pos = 0;
+ for (ivg = 0; ivg <= IVG13 - IVG7; ivg++) {
+ int irqn;
+
+ ivg7_13[ivg].istop = ivg7_13[ivg].ifirst = &ivg_table[irq_pos];
+
+ for (irqn = 0; irqn < NR_PERI_INTS; irqn++) {
+ int iar_shift = (irqn & 7) * 4;
+ if (ivg ==
+ (0xf &
+ bfin_read32((unsigned long *)SIC_IAR0 +
+ (irqn >> 3)) >> iar_shift)) {
+ ivg_table[irq_pos].irqno = IVG7 + irqn;
+ ivg_table[irq_pos].isrflag = 1 << irqn;
+ ivg7_13[ivg].istop++;
+ irq_pos++;
+ }
+ }
+ }
+}
+
+static void bf533_core_mask_irq(unsigned int irq)
+{
+ irq_flags &= ~(1 << irq);
+ if (!irqs_disabled())
+ local_irq_enable();

should that be local_irq_disable() ?

+}
+
+#ifdef CONFIG_IRQCHIP_DEMUX_GPIO
+static int gpio_enabled;
+static int gpio_edge_triggered;

General comment: it's probably not in CodingStyle, but we seem to prefer
to see a blank line between a function's local data and its executable
statements (applies to many functions in this patch).

+static void bf533_gpio_mask_irq(unsigned int irq)
+{
+ int gpionr = irq - IRQ_PF0;
+ int mask = (1L << gpionr);
+ bfin_write_FIO_FLAG_C(mask);
+ __builtin_bfin_ssync();
+ bfin_write_FIO_MASKB_C(mask);
+ __builtin_bfin_ssync();
+}
+
+void do_irq(int vec, struct pt_regs *fp)
+{
+ if (vec == EVT_IVTMR_P) {
+ vec = IRQ_CORETMR;

drop the extra/unneeded braces.

+ } else {
+ struct ivgx *ivg = ivg7_13[vec - IVG7].ifirst;
+ struct ivgx *ivg_stop = ivg7_13[vec - IVG7].istop;
+ unsigned long sic_status;
+
+ __builtin_bfin_ssync();
+ sic_status = bfin_read_SIC_IMASK() & bfin_read_SIC_ISR();
+
+ for (;; ivg++) {
+ if (ivg >= ivg_stop) {
+ num_spurious++;
+ return;
+ } else if (sic_status & ivg->isrflag)
+ break;
+ }
+ vec = ivg->irqno;
+ }
+ asm_do_IRQ(vec, fp);
+}
+
diff -urN linux-2.6.18/arch/blackfin/mach-common/irqpanic.c linux-2.6.18.patch1/arch/blackfin/mach-common/irqpanic.c
--- linux-2.6.18/arch/blackfin/mach-common/irqpanic.c 1970-01-01 08:00:00.000000000 +0800
+++ linux-2.6.18.patch1/arch/blackfin/mach-common/irqpanic.c 2006-09-21 09:29:49.000000000 +0800
@@ -0,0 +1,193 @@
+
+/*
+ * irq_panic - calls panic with string setup
+ */
+asmlinkage void irq_panic(int reason, struct pt_regs *regs)
+{
...
+ if (die) {
+ printk("icache coherency error\n");

KERN_ALERT or KERN_EMERGENCY ?

+ for (j = 0; j <= i; j++) {
+ printk
+ ("cache address : %08x cache value : %08x%08x\n",
+ bad[j][0], bad[j][1], bad[j][2]);
+ printk
+ ("physical address: %08x SDRAM value : %08x%08x\n",
+ bad[j][3], bad[j][4], bad[j][5]);
+ }
+ panic("icache coherency error");
+ } else {
+ printk("\n\nicache checked, and OK\n");
+ }
+#endif

Add KERN_ levels:

+ printk("\n\nException: IRQ 0x%x entered\n", reason);
+ printk(" code=[0x%08x], ", (unsigned int)regs->seqstat);
+ printk(" stack frame=0x%04x, ", (unsigned int)(unsigned long)regs);
+ printk(" bad PC=0x%04x\n", (unsigned int)regs->pc);
+ if (reason == 0x5) {
+ printk("\n----------- HARDWARE ERROR -----------\n\n");
+
+ /* There is only need to check for Hardware Errors, since other
+ * EXCEPTIONS are handled in TRAPS.c (MH)
+ */
+ switch (regs->seqstat & SEQSTAT_HWERRCAUSE) {
+ case (SEQSTAT_HWERRCAUSE_SYSTEM_MMR): /* System MMR Error */
+ info.si_code = BUS_ADRALN;
+ sig = SIGBUS;
+ printk(HWC_x2);
+ break;
+ case (SEQSTAT_HWERRCAUSE_EXTERN_ADDR): /* External Memory Addressing Error */
+ info.si_code = BUS_ADRERR;
+ sig = SIGBUS;
+ printk(KERN_EMERG HWC_x3);
+ break;
+ case (SEQSTAT_HWERRCAUSE_PERF_FLOW): /* Performance Monitor Overflow */
+ printk(KERN_EMERG HWC_x12);
+ break;
+ case (SEQSTAT_HWERRCAUSE_RAISE_5): /* RAISE 5 instruction */
+ printk(KERN_EMERG HWC_x18);
+ break;
+ default: /* Reserved */
+ printk(KERN_EMERG HWC_default);
+ break;
+ }
+ }
+
+ regs->ipend = bfin_read_IPEND();
+ dump_bfin_regs(regs, (void *)regs->pc);
+ if (0 == (info.si_signo = sig) || 0 == user_mode(regs)) /* in kernelspace */
+ panic("Unhandled IRQ or exceptions!\n");
+ else { /* in userspace */
+ info.si_errno = 0;
+ info.si_addr = (void *)regs->pc;
+ force_sig_info(sig, &info, current);
+ }
+}
+
diff -urN linux-2.6.18/arch/blackfin/mm/blackfin_sram.c linux-2.6.18.patch1/arch/blackfin/mm/blackfin_sram.c
--- linux-2.6.18/arch/blackfin/mm/blackfin_sram.c 1970-01-01 08:00:00.000000000 +0800
+++ linux-2.6.18.patch1/arch/blackfin/mm/blackfin_sram.c 2006-09-21 09:29:49.000000000 +0800
@@ -0,0 +1,532 @@
+
+int sram_free(const void *addr)
+{
+ if (0) {
+ }

Drop those 2 lines.

+}
+
+int sram_free_with_lsl(const void *addr)
+{
+ struct sram_list_struct *lsl, **tmp;
+ struct mm_struct *mm = current->mm;
+
+ for (tmp = &mm->context.sram_list; *tmp; tmp = &(*tmp)->next)
+ if ((*tmp)->addr == addr)
+ goto found;
+ return -1;
+ found:

Outdent the label.

+ lsl = *tmp;
+ sram_free(addr);
+ *tmp = lsl->next;
+ kfree(lsl);
+
+ return 0;
+}
+
diff -urN linux-2.6.18/arch/blackfin/oprofile/common.c linux-2.6.18.patch1/arch/blackfin/oprofile/common.c
--- linux-2.6.18/arch/blackfin/oprofile/common.c 1970-01-01 08:00:00.000000000 +0800
+++ linux-2.6.18.patch1/arch/blackfin/oprofile/common.c 2006-09-21 09:29:49.000000000 +0800
@@ -0,0 +1,170 @@
+
+static int op_bfin_create_files(struct super_block *sb, struct dentry *root)
+{
+ int i;
+
+ for (i = 0; i < model->num_counters; ++i) {
+ struct dentry *dir;
+ char buf[3];
+ printk("Oprofile: creating files... \n");

KERN_DEBUG ?

+ snprintf(buf, sizeof buf, "%d", i);
+ dir = oprofilefs_mkdir(sb, root, buf);
+
+ oprofilefs_create_ulong(sb, dir, "enabled", &ctr[i].enabled);
+ oprofilefs_create_ulong(sb, dir, "event", &ctr[i].event);
+ oprofilefs_create_ulong(sb, dir, "count", &ctr[i].count);
+ /*
+ * We dont support per counter user/kernel selection, but
+ * we leave the entries because userspace expects them
+ */
+ oprofilefs_create_ulong(sb, dir, "kernel", &ctr[i].kernel);
+ oprofilefs_create_ulong(sb, dir, "user", &ctr[i].user);
+ oprofilefs_create_ulong(sb, dir, "unit_mask",
+ &ctr[i].unit_mask);
+ }
+
+ return 0;
+}
+int __init oprofile_arch_init(struct oprofile_operations *ops)
+{
+#ifdef CONFIG_HARDWARE_PM
+ unsigned int dspid;
+
+ init_MUTEX(&pfmon_sem);
+
+ dspid = bfin_read_DSPID();
+
+ printk("Oprofile got the cpu id is 0x%x. \n", dspid);

Drop "the" and "is" ?

+ switch (dspid) {
+ case BFIN_533_ID:
+ model = &op_model_bfin533;
+ model->num_counters = 2;
+ break;
+ case BFIN_537_ID:
+ model = &op_model_bfin533;
+ model->num_counters = 2;
+ break;
+ default:
+ return -ENODEV;
+ }
+
diff -urN linux-2.6.18/arch/blackfin/oprofile/op_model_bf533.c linux-2.6.18.patch1/arch/blackfin/oprofile/op_model_bf533.c
--- linux-2.6.18/arch/blackfin/oprofile/op_model_bf533.c 1970-01-01 08:00:00.000000000 +0800
+++ linux-2.6.18.patch1/arch/blackfin/oprofile/op_model_bf533.c 2006-09-21 09:29:49.000000000 +0800
@@ -0,0 +1,168 @@
+
+#ifdef PM_DEBUG
+#define dbg(args...) printk(args)
+#else
+#define dbg(args...)
+#endif
+#define PM_ENABLE 0x01;

Drop the ";".

+#define PM_CTL1_ENABLE 0x18
+#define PM_CTL0_ENABLE 0xC000
+#define COUNT_EDGE_ONLY 0x3000000
+
+static int oprofile_running;
+
+unsigned curr_pfctl, curr_count[2];
+
+static int get_kernel(void)
+{
+ int ipend, is_kernel;
+
+ ipend = bfin_read_IPEND();
+
+ /* test bit 15 */
+ is_kernel = ((ipend & 0x8000) != 0);

Drop extra (outer) parens.

+ return is_kernel;
+}
+
diff -urN linux-2.6.18/arch/blackfin/oprofile/timer_int.c linux-2.6.18.patch1/arch/blackfin/oprofile/timer_int.c
--- linux-2.6.18/arch/blackfin/oprofile/timer_int.c 1970-01-01 08:00:00.000000000 +0800
+++ linux-2.6.18.patch1/arch/blackfin/oprofile/timer_int.c 2006-09-21 09:29:49.000000000 +0800
@@ -0,0 +1,79 @@
+static int sys_timer0_start(void)
+{
+ enable_sys_timer0();
+ int retval = request_irq(IVG11, sys_timer0_int_handler, 0,
+ "sys_timer0", NULL);
+ if (retval)
+ return retval;
+ return 0;

Isn't that the same as just doing:
return retval;
in all cases?

+}

###

2006-09-25 17:05:45

[permalink] [raw]

Subject: Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18

On Monday 25 September 2006 17:39, Aubrey wrote:
> 1) Timer interrupt will call do_irq(), then return_from_int().
>
> 2) return_from_int() will check if there is interrupt pending or
> signal pending, if so, it will call schedule_and_signal_from_int().
>
> 3) schedule_and_signal_from_int() will jump to resume_userspace()
>
> 4) resume_userspace() will call _schedule to run the user task.

I have a little trouble reading your assembly code, but your
return_from_int() function should normally not call
schedule_and_signal_from_int() when the interrupt happened
in kernel context (like in the idle function):

+ /* if not return to user mode, get out */
+ p2.l = lo(IPEND);
+ p2.h = hi(IPEND);
+ r0 = [p2];
+ r1 = 0x17(Z);
+ r2 = ~r1;
+ r2.h = 0;
+ r0 = r2 & r0;
+ r1 = 1;
+ r1 = r0 - r1;
+ r2 = r0 & r1;
+ cc = r2 == 0;
+ if !cc jump 2f;

This looks a lot like you user_mode() function, so you jump
over schedule_and_signal_from_int() here.

What you described would be a preemptive kernel
(CONFIG_PREEMPT), but you clearly don't have that enabled.

Arnd <><

2006-09-25 18:05:45

[permalink] [raw]

Subject: Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18

On 9/25/06, Randy Dunlap <[email protected]> wrote:
> Here are the rest of my comments (on .c files now).

i went through a bunch of our .c files this weekend with a hammer and
had fixed a bunch of points you raised here ... i'll try and grab a
bunch of the rest as well

cheers
-mike

2006-09-25 23:20:47

[permalink] [raw]

Subject: Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18

Arnd Bergmann wrote:
>On Saturday 23 September 2006 18:29, Robin Getz wrote:
> > I would be interested in the reasons why this is bad. We thought is
> > solved the problem, and our driver developers are Ok using it, and it
> > is catching more problems at compile time than we use to (which is the
> > main reason I thought to remove all the volatile casting.
>
>It adds a lot of unnecessary defines. The problem is mostly that for each
>register you add three macros.

That is what host machines are good for - isn't it - keeping track of
redirection? :)

>The simplest way to avoid this would be using your bfin_write16() and
>related functions directly instead of wrapping each register in a separate
>macro.

Right - but I am lazy, and I don't want to remember which is a 16 and which
is a 32 when I am maintaining the software.

Having the extra define is good in that it allows hardware nastiness to be
hidden. For example, we just fixed an issue about how to write to a
register that effects the PLL stability.

/* Writing to VR_CTL initiates a PLL relock sequence. */
static __inline__ void bfin_write_VR_CTL(unsigned int val) {
unsigned long flags, iwr ;

bfin_write16(VR_CTL,val);
__builtin_bfin_ssync();
/* Enable the PLL Wakeup bit in SIC IWR */
iwr = bfin_read32(SIC_IWR);
/* Only allow PPL Wakeup) */
bfin_write32(SIC_IWR, IWR_ENABLE(0));
local_irq_save(flags);
asm("IDLE;");
local_irq_restore(flags);
bfin_write32(SIC_IWR, iwr);
}

There are a few bits in the register that controls the on-chip MAC - in the
ethernet driver, the person was just writing to the register, without
understanding that it unlocked/re-locked the PLL.

This saves every person who writes to this register from duplicating the
code, and we ensures that it is actually done it properly.

> > >Now, there are
> > >at least two common methods for how to do this better, both involving
> > >the readl/writel low-level accessors (or something similar).
> >
> > The thing to understand about the Blackfin architecture - is not all
> > system register, or peripheral registers are 32 bits. Some are 16
> > bits, and some are 32. The Hardware will error if you access a 32 bit
> > instruction, with a
> > 16 bit access, or a 16 bit access on a 32 bit instruction.
> >
> > This is why we added:
> > bfin_write16(); bfin_read16(); bfin_write32(); bfin_read32();
>
>Sure, that's not unusual at all. Instead of readl(), you can use
>readw() for 16 bit accesses. Your bfin_{read,write}{16,32} functions are
>fine as well, but you should make the implementation more robust and change
>
>
>#define bfin_read16(addr) ({
> unsigned __v; \
> __asm__ __volatile__ ("NOP;\n\t %0 = w[%1] (z);\n\t" \
> : "=d"(__v) : "a"(addr)); \
> unsigned short)__v; \
>})
>
>to
>
>static inline __le16 bfin_read16(const __be16 __iomem *addr) {
> __be16 val;
> asm volatile("nop; \n\t %0 = w[%1] (z)" : "=d" (val) : "a"(addr);
> return val;
>}
>
>This adds strict type checking (size, endianess, io address space).
>You may prefer to use u16 instead of __be16 here, depending on your needs.

No problem - I can change/update that.

> > We can't use a common function, like:
> >
> > bfin_sica_read(int reg)
> >
> > or use the read16/read32 directly - it would be to hard for the driver
> > developers to keep the manual with them all the time to find out if a
> > register was 16 or 32 bits. It would move what we have today (compiler
> > errors), to run time errors if someone used the wrong function.
>
>Ok, in the example I picked they were all the same size, so I assumed that
>would be the case for most of your register areas.

[snip]

> > >
> > >The alternative approach uses a structure:
> >
> > We could use this approach, filling it up with 16 bit padding all over
> > the place (which is easy to do), but it is also difficult for the same
> reason.
> > (Although I really like this, and can see lots of value from it - I am
> > not sure we can use it).
> >
> > You are still using writel(reg, value) and readl(reg) - which is hard
> > to do, without a hardware reference beside you all the time - to
> > determine which time you should use a readl or reads.
> >
> > Unless I am completely missing something (which might be true).
>
>Ok, I see your point. If you have different size registers in the area for
>a single device, then it is better to use a structure as I showed below.
>
> > >static struct bfin_sica_regs __iomem *bfin_sica = (void __iomem
> > >*)0xffc00100ul;
> > >
>
> > Although I think the code is smaller, and nicer - it involves more run
> > time complexity, and will consume more processor cycles - the old example:
> >
> > static void bf561_internal_mask_irq(unsigned int irq) {
> > unsigned long irq_mask;
> > if ((irq - (IRQ_CORETMR + 1)) < 32) {
> > irq_mask = (1 << (irq - (IRQ_CORETMR + 1)));
> > bfin_write_SICA_IMASK0(bfin_read_SICA_IMASK0() &
> > ~irq_mask);
> > } else {
> > irq_mask = (1 << (irq - (IRQ_CORETMR + 1) - 32));
> > bfin_write_SICA_IMASK1(bfin_read_SICA_IMASK1() &
> > ~irq_mask);
> > }
> > }
> >
> > resolves all addresses at compile time, not run time. So, wouldn't
> > your example slow things down?
>
>The run time overhead of this is very small compared to an actual mmio
>register access, so you should not notice this at all.

I'm not sure I agree - on machines with tiny cache (only 16k), and high
Core Clocks (600+ MHz), and slow SDRAM clocks (133MHz), extra reads to
SDRAM will kill performance & pollute cache. It is not just the single
cycle read - if the core is writing something into SDRAM, it takes 12 SDRAM
cycles for the SDRAM to turn the bus from a write to a read.

run time overhead is critical to us. People are complaining already that
the overhead of our drivers is too high, and I don't want to add extra
reads to SDRAM if I don't have to.

>You can still do the same with a compile time constant by replacing
>
>static struct bfin_sica_regs __iomem *bfin_sica = (void __iomem
>*)0xffc00100ul;
>
>from my example with
>
>#define bfin_sica_regs ((struct bfin_sica_regs __iomem*)0xffc00100ul)
>
>That should result in the same object code you had before.
>
>However, I'm used to having a single kernel binary that can run on
>multiple hardware versions. Normally this means that you have the same
>register layout on every CPU, but on different physical addresses that are
>detected at boot time, so I like to avoid hardcoding absolute addresses in
>the object code.

People who use our architecture are OK with compiling for a specific platform.

>Moreover, if you ever want to run with an MMU, the virtual address of that
>device is computed by the ioremap function, like
>
>static struct bfin_sica_regs __iomem *bfin_sica;
>
>void __init bfin_init_sica(void)
>{
> bfin_sica = ioremap(0xffc00100ul);
>}

There are no plans to add a MMU to the Blackfin as is - it would require an
extensive change to the architecture.

But still - I can see the value of it for large scale systems, where people
are not willing to compile a kernel targeted at one specific implementation
- but the people who use this kernel are worse - they compile a kernel for
a specific board, not a just a specific processor. Anything extra - they
don't want it. Anything they need - they normally put it in the kernel, as
to reduce boot time/size.

The processor sells for less than a good cup of coffee. People are selling
complete systems (processor, SDRAM, Flash, etc) for less than $50 (US).

People want to do, and expect to do, all kinds of optimizations when they
are working at this level.

This is the hesitation of adding levels of indirection - any additional
overhead - and people will stop using Linux - but I will try out the
structure, and see how that works.

-Robin

2006-09-26 03:42:27

[permalink] [raw]

Subject: Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18

On 9/26/06, Arnd Bergmann <[email protected]> wrote:
> On Monday 25 September 2006 17:39, Aubrey wrote:
> > 1) Timer interrupt will call do_irq(), then return_from_int().
> >
> > 2) return_from_int() will check if there is interrupt pending or
> > signal pending, if so, it will call schedule_and_signal_from_int().
> >
> > 3) schedule_and_signal_from_int() will jump to resume_userspace()
> >
> > 4) resume_userspace() will call _schedule to run the user task.
>
> I have a little trouble reading your assembly code, but your
> return_from_int() function should normally not call
> schedule_and_signal_from_int() when the interrupt happened
> in kernel context (like in the idle function):
>
> + /* if not return to user mode, get out */
> + p2.l = lo(IPEND);
> + p2.h = hi(IPEND);
> + r0 = [p2];
> + r1 = 0x17(Z);
> + r2 = ~r1;
> + r2.h = 0;
> + r0 = r2 & r0;
> + r1 = 1;
> + r1 = r0 - r1;
> + r2 = r0 & r1;
> + cc = r2 == 0;
> + if !cc jump 2f;
>
> This looks a lot like you user_mode() function, so you jump
> over schedule_and_signal_from_int() here.
>
> What you described would be a preemptive kernel
> (CONFIG_PREEMPT), but you clearly don't have that enabled.
>

No, schedule_and_signal_from_int will be called.
The above code is checking if there are at least two bits set on, if
so, schedule_and_signal_from_int will be called.

Blackfin supports 3 processor mode: (1) user mode (2) supervisor mode
(3) emulation mode. In the kernel space, the processor should be in
the supervisor mode. To keep the processor in the supervisor mode, we
raise the lowest priority interrupt event. Kerenl actually in the
interrupt handler of the lowest priority interrupt event. See
arch/blackfin/mach-bf53x/head.S.
=================================
/* This section keeps the processor in supervisor mode
* during kernel boot. Switches to user mode at end of boot.
* See page 3-9 of Hardware Reference manual for documentation.
*/

/* EVT15 = _real_start */

p0.l = lo(EVT15);
p0.h = hi(EVT15);
p1.l = _real_start;
p1.h = _real_start;
[p0] = p1;
csync;

p0.l = lo(IMASK);
p0.h = hi(IMASK);
p1.l = IMASK_IVG15;
p1.h = 0x0;
[p0] = p1;
csync;

raise 15;
p0.l = .LWAIT_HERE;
p0.h = .LWAIT_HERE;
reti = p0;
rti;
===============================================
So, in the kernel space, there is always one bit in the IPEND register
is set. And if there comes a timer interrupt event, in the timer
interrupt handler, there should be two bits set in the IPEND register.
Therefore, schedule happens in the return_from_int.

So, I still say there is no latency here.

-Aubrey

2006-09-26 09:44:17

[permalink] [raw]

Subject: Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18

On Tuesday 26 September 2006 05:42, Aubrey wrote:
> So, in the kernel space, there is always one bit in the IPEND register
> is set. And if there comes a timer interrupt event, in the timer
> interrupt handler, there should be two bits set in the IPEND register.
> Therefore, schedule happens in the return_from_int.
>
> So, I still say there is no latency here.
>

Well, if that's true, you should change your idle function not to
explicitly call schedule().

I haven't really understood how you preempt the idle task, but
I guess you can simplify the standard

| while (1) {
| while (!need_resched())
| asm("idle");
| schedule();
| }

to

| while (1)
| asm("idle");

Arnd <><

2006-09-27 10:04:15

[permalink] [raw]

Subject: Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18

Hi Arnd,

Sorry, I would say I made a logic mistake here. The schedule in the
return_from_int() will only be called when the process steps from the
user space into the kerner space( i.e. sys call). So, what you
described are all correct.

And, we have already had a solution to fix the issue you pointed out.
==================
inline static void default_idle(void)
{
int flag;

while (!need_resched()) {
leds_switch(LED_OFF);
local_irq_save(flag);
if ( likely(!need_resched()) {
#if defined (ANOMALY_05000244) && defined (CONFIG_BLKFIN_CACHE)
__asm__("nop; nop;\n");
#endif
__asm__(".align 64;\n STI %0; IDLE;\n"
: %0 (flag): :"cc");
}
local_irq_restore(flag);
leds_switch(LED_ON);
}
}======================================

Here, according to design, it's not possible that interrupt occurs
between "STI %0"(enable interrupt) and "IDLE".

__asm__(".align 64; STI %0; IDLE;" : %0 (x): :"cc");

Robin can explain more details.

-Aubrey

On 9/26/06, Arnd Bergmann <[email protected]> wrote:
> On Monday 25 September 2006 17:39, Aubrey wrote:
> > 1) Timer interrupt will call do_irq(), then return_from_int().
> >
> > 2) return_from_int() will check if there is interrupt pending or
> > signal pending, if so, it will call schedule_and_signal_from_int().
> >
> > 3) schedule_and_signal_from_int() will jump to resume_userspace()
> >
> > 4) resume_userspace() will call _schedule to run the user task.
>
> I have a little trouble reading your assembly code, but your
> return_from_int() function should normally not call
> schedule_and_signal_from_int() when the interrupt happened
> in kernel context (like in the idle function):
>
> + /* if not return to user mode, get out */
> + p2.l = lo(IPEND);
> + p2.h = hi(IPEND);
> + r0 = [p2];
> + r1 = 0x17(Z);
> + r2 = ~r1;
> + r2.h = 0;
> + r0 = r2 & r0;
> + r1 = 1;
> + r1 = r0 - r1;
> + r2 = r0 & r1;
> + cc = r2 == 0;
> + if !cc jump 2f;
>
> This looks a lot like you user_mode() function, so you jump
> over schedule_and_signal_from_int() here.
>
> What you described would be a preemptive kernel
> (CONFIG_PREEMPT), but you clearly don't have that enabled.
>
> Arnd <><
>

2006-09-27 11:38:19

[permalink] [raw]

Subject: Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18

On Wednesday 27 September 2006 12:04, Aubrey wrote:
> inline static void default_idle(void)
> {
> ? ? ? ?int flag;
>
> ? ? ? ?while (!need_resched()) {
> ? ? ? ? ? ? ? ?leds_switch(LED_OFF);
> ? ? ? ? ? ? ? ?local_irq_save(flag);
> ? ? ? ? ? ? ? ?if ( likely(!need_resched()) {
> #if defined (ANOMALY_05000244) && defined (CONFIG_BLKFIN_CACHE)
> ? ? ? ? ? ? ? ? ? ? ?__asm__("nop; nop;\n");
> #endif
> ? ? ? ? ? ? ? ? ? ? ?__asm__(".align 64;\n STI %0; IDLE;\n"
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?: %0 (flag): :"cc");
> ? ? ? ? ? ? ? ?}
> ? ? ? ? ? ? ? ?local_irq_restore(flag);
> ? ? ? ? ? ? ? ?leds_switch(LED_ON);
> ? ? ? ?}
> }======================================
>
> Here, according to design, it's not possible that interrupt occurs
> between "STI %0"(enable interrupt) and "IDLE".
>
> __asm__(".align 64; STI %0; IDLE;" : %0 (x): ?:"cc");
>
> Robin can explain more details.

Ok, looks good now. Just a few details that don't impact the
functionality:

- Always use 'static inline', not 'inline static', because of C99
- In the kernel, it's more common to use 'asm' than '__asm__'.
- It should probably be 'asm volatile', since gcc might notice
that the output (flag) is not used anywhere and it can therefore
eliminate the asm.
- Usually, I recommend using local_irq_disable() instead of
local_irq_save(flags) when you know that interrupts are enabled
before. It uses one less local variable, which makes it more
efficient on some architectures.
- I'd insert the two NOPs unconditionally here for better
readability.

Arnd <><

2006-09-27 16:26:09

[permalink] [raw]

Subject: Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18

Arnd wrote:
>Ok, looks good now. Just a few details that don't impact the
>functionality:

[snip]

What I committed (to our source) is:

+++ process.c 27 Sep 2006 15:32:46 -0000 1.42
@@ -101,10 +101,10 @@
{
while (!need_resched()) {
leds_switch(LED_OFF);
- __asm__("nop;\n\t \
- nop;\n\t \
- nop;\n\t \
- idle;\n\t": : :"cc");
+ local_irq_disable();
+ if (likely( !need_resched()))
+ idle_with_irq_disabled();
+ local_irq_enable();
leds_switch(LED_ON);
}
}

And in system.h, this was added (because this is where all the other
inlines is which messes with the interrupts are - and irq_flags is already
defined here)

+++ system.h 27 Sep 2006 15:32:51 -0000 1.24

+#define idle_with_irq_disabled() do { \
+ __asm__ __volatile__ ( \
+ "nop; nop;\n" \
+ ".align 8;\n" \
+ "sti %0; idle;\n" \
+ ::"d" (irq_flags)); \
+} while (0)

It seems to work OK.

Thanks for your help on this.

I think we have been weeding through everyone's comments, and have most
things fixed up.

Are there any other major issues that you can see (that have not been
pointed out).

-Robin

2006-09-27 16:35:52

[permalink] [raw]

Subject: Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18

On Wed, 27 Sep 2006 12:25:17 -0400 Robin Getz wrote:

> Arnd wrote:
> >Ok, looks good now. Just a few details that don't impact the
> >functionality:
>
> [snip]
>
> What I committed (to our source) is:
>
> +++ process.c 27 Sep 2006 15:32:46 -0000 1.42
> @@ -101,10 +101,10 @@
> {
> while (!need_resched()) {
> leds_switch(LED_OFF);
> - __asm__("nop;\n\t \
> - nop;\n\t \
> - nop;\n\t \
> - idle;\n\t": : :"cc");
> + local_irq_disable();
> + if (likely( !need_resched()))
> + idle_with_irq_disabled();
> + local_irq_enable();
> leds_switch(LED_ON);
> }
> }
>
> And in system.h, this was added (because this is where all the other
> inlines is which messes with the interrupts are - and irq_flags is already
> defined here)
>
> +++ system.h 27 Sep 2006 15:32:51 -0000 1.24
>
> +#define idle_with_irq_disabled() do { \
> + __asm__ __volatile__ ( \
> + "nop; nop;\n" \
> + ".align 8;\n" \
> + "sti %0; idle;\n" \
> + ::"d" (irq_flags)); \
> +} while (0)
>
> It seems to work OK.
>
> Thanks for your help on this.
>
> I think we have been weeding through everyone's comments, and have most
> things fixed up.

except for coding style nits. E.g., the patch above:
a. uses spaces instead of tabs for indentation
b. has an extra (unwanted) space in:
> + if (likely( !need_resched()))
^

> Are there any other major issues that you can see (that have not been
> pointed out).

---
~Randy

2006-09-27 16:41:28

[permalink] [raw]

Subject: Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18

On Wednesday 27 September 2006 18:25, Robin Getz wrote:
> +#define idle_with_irq_disabled() do { ? \
> + ? ? ? ?__asm__ __volatile__ ( ? ? ? ? ?\
> + ? ? ? ? ? ? ? ?"nop; nop;\n" ? ? ? ? ? \
> + ? ? ? ? ? ? ? ?".align 8;\n" ? ? ? ? ? \
> + ? ? ? ? ? ? ? ?"sti %0; idle;\n" ? ? ? \
> + ? ? ? ? ? ? ? ?::"d" (irq_flags)); ? ? \
> +} while (0)
>

The irq_flags are not declared anywhere in the code you just posted,
I guess you could simply declare a local variable in this macro.

It would also be better to convert macros like this one to inline
functions in general. The rule is: if you can use either a macro
or an inline function with the same effect, use an inline function.

Arnd <><

2006-09-27 17:19:45

[permalink] [raw]

Subject: Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18

Arnd wrote:
>The irq_flags are not declared anywhere in the code you just posted,

Yeah - they are already defined, and used in other macros in system.h -
which is why I put the macro there.

>It would also be better to convert macros like this one to inline
>functions in general. The rule is: if you can use either a macro or an
>inline function with the same effect, use an inline function.

OK - I was just doing the similar thing to what already exists in
./asm-blackfin/system.h

#define local_irq_enable() do { \
__asm__ __volatile__ ( \
"sti %0;" \
::"d"(irq_flags)); \
} while (0)

which could be simplified to:

#define local_irq_enable() __asm__ __volatile__ ("sti %0;" ::"d"(irq_flags));

which is the same as what is in ./asm-i386/system.h - isn't it?

#define local_irq_disable() __asm__ __volatile__("cli": : :"memory")
#define local_irq_enable() __asm__ __volatile__("sti": : :"memory")

We can do it anyway that makes sense/improves readability - it all compiles
to the same thing...

-Robin

2006-09-27 17:46:59

[permalink] [raw]

Subject: Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18

Randy wrote:

>except for coding style nits. E.g., the patch above:
>a. uses spaces instead of tabs for indentation

yeah - my copy/paste/mailer is broken - when it copies tabs, it pastes
space into the mailer.

>b. has an extra (unwanted) space in:
> > + if (likely( !need_resched()))
> ^

There are still lots of places where we need to fix up broken white space
in our patches.

Does anyone have a script that identifies white space problems?

Thanks
-Robin

2006-09-27 19:19:44

by Jörn Engel

[permalink] [raw]

Subject: Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18

On Wed, 27 September 2006 13:47:16 -0400, Robin Getz wrote:
>
> Does anyone have a script that identifies white space problems?

If you use vim:
highlight RedundantSpaces ctermbg=red guibg=red
match RedundantSpaces /\s\+$\| \+\ze\t/

J?rn

--
A defeated army first battles and then seeks victory.
-- Sun Tzu

2006-09-27 20:56:50

[permalink] [raw]

Subject: Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18

Am Wednesday 27 September 2006 19:19 schrieb Robin Getz:
> OK - I was just doing the similar thing to what already exists in
> ./asm-blackfin/system.h
>
> #define local_irq_enable() do { ? ? ? ? \
> ? ? ? ? ?__asm__ __volatile__ ( ? ? ? ? ?\
> ? ? ? ? ? ? ? ? ?"sti %0;" ? ? ? ? ? ? ? \
> ? ? ? ? ? ? ? ? ?::"d"(irq_flags)); ? ? ?\
> } while (0)
>
> which could be simplified to:
>
> #define local_irq_enable() __asm__ __volatile__ ("sti %0;" ::"d"(irq_flags));

Actually, this one is slightly broken, because of the ';' at the end of the
macro (think of "if(x) local_irq_enable(); else somthing_else()").

What I was suggesting is to make a proper inline function, like

static inline void local_irq_enable(void)
{
unsigned long unused_flags;
asm volatile ("sti %0;" : : "d" (unused_flags));
}

That completely avoids all the problems you might hit with macro expansion,
while still compiling to the same code.

Arnd <><

2006-09-27 21:21:47

[permalink] [raw]

Subject: Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18

Arnd wrote:
>Am Wednesday 27 September 2006 19:19 schrieb Robin Getz:
> > OK - I was just doing the similar thing to what already exists in
> > ./asm-blackfin/system.h
> >
> > #define local_irq_enable() do { \
> > __asm__ __volatile__ ( \
> > "sti %0;" \
> > ::"d"(irq_flags)); \ } while (0)
> >
> > which could be simplified to:
> >
> > #define local_irq_enable() __asm__ __volatile__ ("sti %0;"
> > ::"d"(irq_flags));
>
>Actually, this one is slightly broken, because of the ';' at the end of
>the macro (think of "if(x) local_irq_enable(); else somthing_else()").

Ok - the extra ; is a typo in the email - not anything that I was proposing
as a submission. What you are pointing out is to be _really_ careful when
doing macros.

I was trying to say was that we are just doing what everyone else seems to
be doing (which doesn't make it correct or the proper thing to do).

Systems that use macros:
./asm-alpha/system.h:#define local_irq_enable()
./asm-arm26/system.h:#define local_irq_enable()
./asm-arm/system.h:#define local_irq_enable()
./asm-blackfin/system.h:#define local_irq_enable()
./asm-frv/system.h:#define local_irq_enable()
./asm-h8300/system.h:#define local_irq_enable()
./asm-i386/system.h:#define local_irq_enable()
./asm-ia64/system.h:#define local_irq_enable()
./asm-m32r/system.h:#define local_irq_enable()
./asm-m68knommu/system.h:#define local_irq_enable()
./asm-m68k/system.h:#define local_irq_enable()
./asm-parisc/system.h:#define local_irq_enable()
./asm-s390/system.h:#define local_irq_enable()
./asm-sparc64/system.h:#define local_irq_enable()
./asm/system.h:#define local_irq_enable()
./asm-v850/system.h:#define local_irq_enable()
./asm-x86_64/system.h:#define local_irq_enable()
./asm-x86_64/system.h:#define local_irq_enable()

Systems that use static inline:
./asm-m32r/system.h:static inline void local_irq_enable(void)
./asm-sh64/system.h:static __inline__ void local_irq_enable(void)
./asm-sh/system.h:static __inline__ void local_irq_enable(void)
./asm-xtensa/system.h:static inline void local_irq_enable(void)

With the "optimizations" that gcc4 is making with inline being only a
"suggestion", I think I would prefer to stick with the macro, unless there
is violent opposition.

As Mike pointed out - we are sheep - we just do what the majority (18/22)
of other people do.

-Robin

2006-09-27 21:36:20

[permalink] [raw]

Subject: Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18

Am Wednesday 27 September 2006 23:22 schrieb Robin Getz:
> Systems that use static inline:
> ./asm-m32r/system.h:static inline void local_irq_enable(void)
> ./asm-sh64/system.h:static __inline__ void local_irq_enable(void)
> ./asm-sh/system.h:static __inline__ void local_irq_enable(void)
> ./asm-xtensa/system.h:static inline void local_irq_enable(void)
>
> With the "optimizations" that gcc4 is making with inline being only a
> "suggestion", I think I would prefer to stick with the macro, unless there
> is violent opposition.

Note that the architectures that do the macro are the ones that
were there first, while the four above are relatively new. They may
well have gotten the same comment ;-)

For a single statement, it doesn't really matter much
whether you're using a macro or an inline function, but the longer
the macro gets, the more reason there is to change it.

In particular, new gcc versions actually do a pretty good job
at deciding when to use an out-of-line function and it may make
your code better if you let it.

> As Mike pointed out - we are sheep - we just do what the majority (18/22)
> of other people do.

Not a bad strategy in general. An even better strategy is to do
what the better architecture implementations in linux do and
to apply common sense. "better" of course is rather subjective,
but I typically recommend looking at arch/parisc as a good example.

Arnd <><

2006-09-27 22:55:59

[permalink] [raw]

Subject: Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18

J?rn told me:
>On Wed, 27 September 2006 13:47:16 -0400, Robin Getz wrote:
> >
> > Does anyone have a script that identifies white space problems?
>If you use vim:
>highlight RedundantSpaces ctermbg=red guibg=red
>match RedundantSpaces /\s\+$\| \+\ze\t/

which lead me to:
http://www.vim.org/tips/tip.php?tip_id=1040
and
http://www.vim.org/tips/tip.php?tip_id=811

When you know what to search for - things are much easier to find...

Thanks for the pointer.

-Robin

2006-09-27 23:00:45

[permalink] [raw]

Subject: Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18

Arnd wrote:
>An even better strategy is to do what the better architecture
>implementations in linux do and to apply common sense. "better" of course
>is rather subjective, but I typically recommend looking at arch/parisc as
>a good example.

Sure - we will go with that.

./linux-2.6.x/include/asm-parisc/system.h

/* interrupt control */
#define local_save_flags(x) __asm__ __volatile__("ssm 0, %0" : "=r" (x)
: : "memory")
#define local_irq_disable() __asm__ __volatile__("rsm %0,%%r0\n" : :
"i" (PSW_I) : "memory" )
#define local_irq_enable() __asm__ __volatile__("ssm %0,%%r0\n" : :
"i" (PSW_I) : "memory" )

:)

-Robin

2006-09-28 09:32:03

by Bernd Schmidt

[permalink] [raw]

Subject: Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18

Arnd Bergmann wrote:

> static inline void local_irq_enable(void)
> {
> unsigned long unused_flags;
> asm volatile ("sti %0;" : : "d" (unused_flags));
> }
>
> That completely avoids all the problems you might hit with macro expansion,
> while still compiling to the same code.

The operand is an input, not an output. We want to restore the proper
mask of enabled interrupts with the STI. That mask is in the global
irq_flags variable (which probably ought to have a different name that
doesn't invite clashes).

Bernd

2006-09-28 11:04:49

[permalink] [raw]

Subject: Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18

On Thursday 28 September 2006 11:31, Bernd Schmidt wrote:
> >
> > That completely avoids all the problems you might hit with macro expansion,
> > while still compiling to the same code.
>
> The operand is an input, not an output.

ok, my fault.

> We want to restore the proper
> mask of enabled interrupts with the STI. ?That mask is in the global
> irq_flags variable (which probably ought to have a different name that
> doesn't invite clashes).

Shouldn't you just use a constant expression here? A global variable
for it sounds rather strange, especially since the local_irq_disable()
calls are sometimes nested, not to mention the problems you'd hit on
SMP?

Arnd <><

2006-09-28 11:39:47

by Bernd Schmidt

[permalink] [raw]

Subject: Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18

Arnd Bergmann wrote:
>> We want to restore the proper
>> mask of enabled interrupts with the STI. That mask is in the global
>> irq_flags variable (which probably ought to have a different name that
>> doesn't invite clashes).
>
> Shouldn't you just use a constant expression here? A global variable
> for it sounds rather strange, especially since the local_irq_disable()
> calls are sometimes nested, not to mention the problems you'd hit on
> SMP?

It's not a constant - there are some {un,}mask_irq functions that may
change it. We don't have SMP, obviously it would have to be per-CPU if
we did.

Bernd

2006-09-28 12:35:35