* Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18
2006-09-21 3:32 [PATCH 1/4] Blackfin: arch patch for 2.6.18 Luke Yang
@ 2006-09-21 9:59 ` Luke Yang
2006-09-23 0:18 ` Arnd Bergmann
` (2 subsequent siblings)
3 siblings, 0 replies; 53+ messages in thread
From: Luke Yang @ 2006-09-21 9:59 UTC (permalink / raw)
To: linux-kernel, Andrew Morton
We know this is a big one and really appreciate anyone who is
interested can review it. For any issues. We'll try to fix them ASAP.
Thank you!
On 9/21/06, Luke Yang <luke.adi@gmail.com> 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
>
> Signed-off-by: Luke Yang <luke.adi@gmail.com>
>
> arch/blackfin/Kconfig | 840 +++++++
> arch/blackfin/Kconfig.ide | 88
> arch/blackfin/Makefile | 78
> arch/blackfin/defconfig | 1088 +++++++++
> arch/blackfin/kernel/Makefile | 11
> arch/blackfin/kernel/asm-offsets.c | 138 +
> arch/blackfin/kernel/bfin_dma_5xx.c | 749 ++++++
> arch/blackfin/kernel/bfin_ksyms.c | 114
> arch/blackfin/kernel/dma-mapping.c | 174 +
> arch/blackfin/kernel/dualcore_test.c | 51
> arch/blackfin/kernel/entry.S | 99
> arch/blackfin/kernel/init_task.c | 63
> arch/blackfin/kernel/irqchip.c | 150 +
> arch/blackfin/kernel/module.c | 469 +++
> arch/blackfin/kernel/process.c | 346 ++
> arch/blackfin/kernel/ptrace.c | 431 +++
> arch/blackfin/kernel/setup.c | 941 +++++++
> arch/blackfin/kernel/signal.c | 715 +++++
> arch/blackfin/kernel/sys_bfin.c | 254 ++
> arch/blackfin/kernel/time.c | 336 ++
> arch/blackfin/kernel/traps.c | 640 +++++
> arch/blackfin/kernel/vmlinux.lds.S | 225 +
> arch/blackfin/lib/Makefile | 9
> arch/blackfin/lib/ashldi3.c | 56
> arch/blackfin/lib/ashrdi3.c | 57
> arch/blackfin/lib/checksum.c | 139 +
> arch/blackfin/lib/divsi3.S | 156 +
> arch/blackfin/lib/gcclib.h | 49
> arch/blackfin/lib/ins.S | 78
> arch/blackfin/lib/lshrdi3.c | 70
> arch/blackfin/lib/memchr.S | 64
> arch/blackfin/lib/memcmp.S | 108
> arch/blackfin/lib/memcpy.S | 128 +
> arch/blackfin/lib/memmove.S | 102
> arch/blackfin/lib/memset.S | 103
> arch/blackfin/lib/modsi3.S | 74
> arch/blackfin/lib/muldi3.c | 97
> arch/blackfin/lib/outs.S | 63
> arch/blackfin/lib/udivsi3.S | 157 +
> arch/blackfin/lib/umodsi3.S | 63
> arch/blackfin/mach-bf533/Kconfig | 103
> arch/blackfin/mach-bf533/Makefile | 10
> arch/blackfin/mach-bf533/boards/Makefile | 8
> arch/blackfin/mach-bf533/boards/cm_bf533.c | 236 +
> arch/blackfin/mach-bf533/boards/ezkit.c | 213 +
> arch/blackfin/mach-bf533/boards/generic_board.c | 78
> arch/blackfin/mach-bf533/boards/stamp.c | 265 ++
> arch/blackfin/mach-bf533/cpu.c | 169 +
> arch/blackfin/mach-bf533/head.S | 767 ++++++
> arch/blackfin/mach-bf533/ints-priority.c | 69
> arch/blackfin/mach-bf533/pm.c | 152 +
> arch/blackfin/mach-bf537/Kconfig | 147 +
> arch/blackfin/mach-bf537/Makefile | 10
> arch/blackfin/mach-bf537/boards/Makefile | 7
> arch/blackfin/mach-bf537/boards/cm_bf537.c | 268 ++
> arch/blackfin/mach-bf537/boards/ezkit.c | 213 +
> arch/blackfin/mach-bf537/boards/generic_board.c | 469 +++
> arch/blackfin/mach-bf537/boards/led.S | 183 +
> arch/blackfin/mach-bf537/boards/stamp.c | 489 ++++
> arch/blackfin/mach-bf537/cpu.c | 169 +
> arch/blackfin/mach-bf537/head.S | 584 ++++
> arch/blackfin/mach-bf537/ints-priority.c | 79
> arch/blackfin/mach-bf537/pm.c | 150 +
> arch/blackfin/mach-bf561/Kconfig | 224 +
> arch/blackfin/mach-bf561/Makefile | 9
> arch/blackfin/mach-bf561/boards/Makefile | 6
> arch/blackfin/mach-bf561/boards/ezkit.c | 84
> arch/blackfin/mach-bf561/boards/generic_board.c | 78
> arch/blackfin/mach-bf561/coreb.c | 413 +++
> arch/blackfin/mach-bf561/head.S | 504 ++++
> arch/blackfin/mach-bf561/ints-priority.c | 113
> arch/blackfin/mach-common/Makefile | 12
> arch/blackfin/mach-common/bf5xx_rtc.c | 140 +
> arch/blackfin/mach-common/cache.S | 255 ++
> arch/blackfin/mach-common/cacheinit.S | 140 +
> arch/blackfin/mach-common/cplbhdlr.S | 128 +
> arch/blackfin/mach-common/cplbinfo.c | 212 +
> arch/blackfin/mach-common/cplbmgr.S | 623 +++++
> arch/blackfin/mach-common/dpmc.S | 438 +++
> arch/blackfin/mach-common/entry.S | 1169 +++++++++
> arch/blackfin/mach-common/flush.S | 400 +++
> arch/blackfin/mach-common/interrupt.S | 255 ++
> arch/blackfin/mach-common/ints-priority-dc.c | 545 ++++
> arch/blackfin/mach-common/ints-priority-sc.c | 619 +++++
> arch/blackfin/mach-common/irqpanic.c | 193 +
> arch/blackfin/mach-common/lock.S | 215 +
> arch/blackfin/mm/Makefile | 5
> arch/blackfin/mm/blackfin_sram.c | 532 ++++
> arch/blackfin/mm/blackfin_sram.h | 40
> arch/blackfin/mm/init.c | 222 +
> arch/blackfin/mm/kmap.c | 86
> arch/blackfin/oprofile/Kconfig | 29
> arch/blackfin/oprofile/Makefile | 14
> arch/blackfin/oprofile/common.c | 170 +
> arch/blackfin/oprofile/op_blackfin.h | 100
> arch/blackfin/oprofile/op_model_bf533.c | 168 +
> arch/blackfin/oprofile/timer_int.c | 79
> fs/Kconfig.binfmt | 2
> include/asm-blackfin/a.out.h | 25
> include/asm-blackfin/atomic.h | 176 +
> include/asm-blackfin/auxvec.h | 4
> include/asm-blackfin/bf53x_timers.h | 137 +
> include/asm-blackfin/bf5xx_rtc.h | 19
> include/asm-blackfin/bfin-global.h | 126 +
> include/asm-blackfin/bfin5xx_spi.h | 170 +
> include/asm-blackfin/bfin_spi_channel.h | 180 +
> include/asm-blackfin/bfin_sport.h | 176 +
> include/asm-blackfin/bitops.h | 213 +
> include/asm-blackfin/blackfin.h | 13
> include/asm-blackfin/board/eagle.h | 4
> include/asm-blackfin/board/ezkit.h | 4
> include/asm-blackfin/board/hawk.h | 4
> include/asm-blackfin/board/pub.h | 17
> include/asm-blackfin/bug.h | 14
> include/asm-blackfin/bugs.h | 16
> include/asm-blackfin/byteorder.h | 48
> include/asm-blackfin/cache.h | 18
> include/asm-blackfin/cacheflush.h | 103
> include/asm-blackfin/checksum.h | 101
> include/asm-blackfin/cplb.h | 51
> include/asm-blackfin/cplbtab.h | 572 ++++
> include/asm-blackfin/cpumask.h | 6
> include/asm-blackfin/cputime.h | 6
> include/asm-blackfin/current.h | 23
> include/asm-blackfin/delay.h | 41
> include/asm-blackfin/div64.h | 1
> include/asm-blackfin/dma-mapping.h | 68
> include/asm-blackfin/dma.h | 212 +
> include/asm-blackfin/dpmc.h | 66
> include/asm-blackfin/elf.h | 127 +
> include/asm-blackfin/emergency-restart.h | 6
> include/asm-blackfin/entry.h | 367 +++
> include/asm-blackfin/errno.h | 6
> include/asm-blackfin/fcntl.h | 87
> include/asm-blackfin/flat.h | 128 +
> include/asm-blackfin/futex.h | 6
> include/asm-blackfin/hardirq.h | 41
> include/asm-blackfin/hw_irq.h | 6
> include/asm-blackfin/ide.h | 31
> include/asm-blackfin/io.h | 155 +
> include/asm-blackfin/ioctl.h | 1
> include/asm-blackfin/ioctls.h | 82
> include/asm-blackfin/ipc.h | 31
> include/asm-blackfin/ipcbuf.h | 30
> include/asm-blackfin/irq.h | 85
> include/asm-blackfin/kmap_types.h | 21
> include/asm-blackfin/l1layout.h | 30
> include/asm-blackfin/linkage.h | 7
> include/asm-blackfin/local.h | 6
> include/asm-blackfin/mach-bf533/anomaly.h | 172 +
> include/asm-blackfin/mach-bf533/bf533.h | 288 ++
> include/asm-blackfin/mach-bf533/bfin_serial_5xx.h | 81
> include/asm-blackfin/mach-bf533/blackfin.h | 48
> include/asm-blackfin/mach-bf533/cdefBF532.h | 691 +++++
> include/asm-blackfin/mach-bf533/defBF532.h | 1202 ++++++++++
> include/asm-blackfin/mach-bf533/dma.h | 56
> include/asm-blackfin/mach-bf533/irq.h | 178 +
> include/asm-blackfin/mach-bf533/mem_init.h | 314 ++
> include/asm-blackfin/mach-bf533/mem_map.h | 135 +
> include/asm-blackfin/mach-bf535/bf535.h | 1285 ++++++++++
> include/asm-blackfin/mach-bf535/bf535_serial.h | 109
> include/asm-blackfin/mach-bf535/blackfin.h | 44
> include/asm-blackfin/mach-bf535/cdefBF535.h | 121 +
> include/asm-blackfin/mach-bf535/cdefblackfin.h | 69
> include/asm-blackfin/mach-bf535/defBF535.h | 1154 +++++++++
> include/asm-blackfin/mach-bf535/defblackfin.h | 444 +++
> include/asm-blackfin/mach-bf535/irq.h | 125 +
> include/asm-blackfin/mach-bf537/anomaly.h | 118
> include/asm-blackfin/mach-bf537/bf537.h | 268 ++
> include/asm-blackfin/mach-bf537/bfin_serial_5xx.h | 101
> include/asm-blackfin/mach-bf537/blackfin.h | 440 +++
> include/asm-blackfin/mach-bf537/cdefBF534.h | 1805 +++++++++++++++
> include/asm-blackfin/mach-bf537/cdefBF537.h | 209 +
> include/asm-blackfin/mach-bf537/defBF534.h | 2520 +++++++++++++++++++++
> include/asm-blackfin/mach-bf537/defBF537.h | 404 +++
> include/asm-blackfin/mach-bf537/dma.h | 55
> include/asm-blackfin/mach-bf537/irq.h | 185 +
> include/asm-blackfin/mach-bf537/mem_init.h | 328 ++
> include/asm-blackfin/mach-bf537/mem_map.h | 143 +
> include/asm-blackfin/mach-bf561/anomaly.h | 182 +
> include/asm-blackfin/mach-bf561/bf561.h | 378 +++
> include/asm-blackfin/mach-bf561/blackfin.h | 54
> include/asm-blackfin/mach-bf561/cdefBF561.h | 1528 ++++++++++++
> include/asm-blackfin/mach-bf561/defBF561.h | 1713 ++++++++++++++
> include/asm-blackfin/mach-bf561/dma.h | 36
> include/asm-blackfin/mach-bf561/irq.h | 451 +++
> include/asm-blackfin/mach-bf561/mem_init.h | 283 ++
> include/asm-blackfin/mach-bf561/mem_map.h | 61
> include/asm-blackfin/mach-common/cdef_LPBlackfin.h | 474 +++
> include/asm-blackfin/mach-common/def_LPBlackfin.h | 706 +++++
> include/asm-blackfin/macros.h | 95
> include/asm-blackfin/mem_map.h | 12
> include/asm-blackfin/mman.h | 45
> include/asm-blackfin/mmu.h | 30
> include/asm-blackfin/mmu_context.h | 130 +
> include/asm-blackfin/module.h | 19
> include/asm-blackfin/msgbuf.h | 31
> include/asm-blackfin/mutex.h | 9
> include/asm-blackfin/namei.h | 19
> include/asm-blackfin/page.h | 89
> include/asm-blackfin/page_offset.h | 6
> include/asm-blackfin/param.h | 22
> include/asm-blackfin/pci.h | 148 +
> include/asm-blackfin/percpu.h | 6
> include/asm-blackfin/pgalloc.h | 8
> include/asm-blackfin/pgtable.h | 62
> include/asm-blackfin/poll.h | 24
> include/asm-blackfin/posix_types.h | 65
> include/asm-blackfin/processor.h | 104
> include/asm-blackfin/ptrace.h | 102
> include/asm-blackfin/resource.h | 6
> include/asm-blackfin/scatterlist.h | 26
> include/asm-blackfin/sections.h | 7
> include/asm-blackfin/segment.h | 7
> include/asm-blackfin/semaphore-helper.h | 82
> include/asm-blackfin/semaphore.h | 106
> include/asm-blackfin/sembuf.h | 25
> include/asm-blackfin/setup.h | 17
> include/asm-blackfin/shmbuf.h | 42
> include/asm-blackfin/shmparam.h | 6
> include/asm-blackfin/sigcontext.h | 50
> include/asm-blackfin/siginfo.h | 35
> include/asm-blackfin/signal.h | 159 +
> include/asm-blackfin/socket.h | 53
> include/asm-blackfin/sockios.h | 12
> include/asm-blackfin/spinlock.h | 6
> include/asm-blackfin/stat.h | 77
> include/asm-blackfin/statfs.h | 6
> include/asm-blackfin/string.h | 97
> include/asm-blackfin/system.h | 212 +
> include/asm-blackfin/termbits.h | 173 +
> include/asm-blackfin/termios.h | 106
> include/asm-blackfin/thread_info.h | 142 +
> include/asm-blackfin/timex.h | 18
> include/asm-blackfin/tlb.h | 16
> include/asm-blackfin/tlbflush.h | 62
> include/asm-blackfin/topology.h | 6
> include/asm-blackfin/traps.h | 75
> include/asm-blackfin/types.h | 66
> include/asm-blackfin/uaccess.h | 260 ++
> include/asm-blackfin/ucontext.h | 30
> include/asm-blackfin/unaligned.h | 6
> include/asm-blackfin/unistd.h | 545 ++++
> include/asm-blackfin/user.h | 91
> include/linux/elf-em.h | 1
> include/linux/usb_sl811.h | 26
> init/Kconfig | 3
> init/Kconfig.orig | 516 ++++
> lib/Kconfig.debug | 4
> scripts/genksyms/genksyms.c | 3
> scripts/mod/mk_elfconfig.c | 3
> 251 files changed, 49661 insertions(+), 6 deletions(-)
>
> http://blackfin.uclinux.org/frs/download.php/1010/blackfin_arch_2.6.18.patch
> (same as above link)
> --
> Best regards,
> Luke Yang
> luke.adi@gmail.com
>
--
Best regards,
Luke Yang
luke.adi@gmail.com
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18
2006-09-21 3:32 [PATCH 1/4] Blackfin: arch patch for 2.6.18 Luke Yang
2006-09-21 9:59 ` Luke Yang
@ 2006-09-23 0:18 ` Arnd Bergmann
2006-09-23 1:18 ` Randy.Dunlap
` (2 more replies)
2006-09-23 21:27 ` David Woodhouse
2006-09-25 16:52 ` Randy Dunlap
3 siblings, 3 replies; 53+ messages in thread
From: Arnd Bergmann @ 2006-09-23 0:18 UTC (permalink / raw)
To: Luke Yang; +Cc: linux-kernel, Andrew Morton
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 <><
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18
2006-09-23 0:18 ` Arnd Bergmann
@ 2006-09-23 1:18 ` Randy.Dunlap
2006-09-23 1:24 ` Roland Dreier
2006-09-23 6:50 ` Mike Frysinger
2006-09-24 3:35 ` Aubrey
2 siblings, 1 reply; 53+ messages in thread
From: Randy.Dunlap @ 2006-09-23 1:18 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: Luke Yang, linux-kernel, Andrew Morton
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
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18
2006-09-23 1:18 ` Randy.Dunlap
@ 2006-09-23 1:24 ` Roland Dreier
2006-09-23 1:58 ` Mike Frysinger
0 siblings, 1 reply; 53+ messages in thread
From: Roland Dreier @ 2006-09-23 1:24 UTC (permalink / raw)
To: Randy.Dunlap; +Cc: Arnd Bergmann, Luke Yang, linux-kernel, Andrew Morton
> > > +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.
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18
2006-09-23 1:24 ` Roland Dreier
@ 2006-09-23 1:58 ` Mike Frysinger
0 siblings, 0 replies; 53+ messages in thread
From: Mike Frysinger @ 2006-09-23 1:58 UTC (permalink / raw)
To: Roland Dreier
Cc: Randy.Dunlap, Arnd Bergmann, Luke Yang, linux-kernel,
Andrew Morton
On 9/22/06, Roland Dreier <rdreier@cisco.com> 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
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18
2006-09-23 0:18 ` Arnd Bergmann
2006-09-23 1:18 ` Randy.Dunlap
@ 2006-09-23 6:50 ` Mike Frysinger
2006-09-23 11:03 ` Arnd Bergmann
` (2 more replies)
2006-09-24 3:35 ` Aubrey
2 siblings, 3 replies; 53+ messages in thread
From: Mike Frysinger @ 2006-09-23 6:50 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: Luke Yang, linux-kernel, Andrew Morton
On 9/22/06, Arnd Bergmann <arnd@arndb.de> 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
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18
2006-09-23 6:50 ` Mike Frysinger
@ 2006-09-23 11:03 ` Arnd Bergmann
2006-09-23 11:15 ` Mike Frysinger
2006-09-23 11:28 ` Matthieu CASTET
2006-09-23 19:43 ` Arnd Bergmann
2 siblings, 1 reply; 53+ messages in thread
From: Arnd Bergmann @ 2006-09-23 11:03 UTC (permalink / raw)
To: Mike Frysinger; +Cc: Luke Yang, linux-kernel, Andrew Morton
On Saturday 23 September 2006 08:50, Mike Frysinger wrote:
> On 9/22/06, Arnd Bergmann <arnd@arndb.de> 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 <><
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18
2006-09-23 11:03 ` Arnd Bergmann
@ 2006-09-23 11:15 ` Mike Frysinger
2006-09-23 11:29 ` Arnd Bergmann
2006-09-23 13:07 ` Arnd Bergmann
0 siblings, 2 replies; 53+ messages in thread
From: Mike Frysinger @ 2006-09-23 11:15 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: Luke Yang, linux-kernel, Andrew Morton
On 9/23/06, Arnd Bergmann <arnd@arndb.de> wrote:
> On Saturday 23 September 2006 08:50, Mike Frysinger wrote:
> > On 9/22/06, Arnd Bergmann <arnd@arndb.de> 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
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18
2006-09-23 11:15 ` Mike Frysinger
@ 2006-09-23 11:29 ` Arnd Bergmann
2006-09-23 13:07 ` Arnd Bergmann
1 sibling, 0 replies; 53+ messages in thread
From: Arnd Bergmann @ 2006-09-23 11:29 UTC (permalink / raw)
To: Mike Frysinger; +Cc: Luke Yang, linux-kernel, Andrew Morton
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 <><
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18
2006-09-23 11:15 ` Mike Frysinger
2006-09-23 11:29 ` Arnd Bergmann
@ 2006-09-23 13:07 ` Arnd Bergmann
1 sibling, 0 replies; 53+ messages in thread
From: Arnd Bergmann @ 2006-09-23 13:07 UTC (permalink / raw)
To: Mike Frysinger; +Cc: Luke Yang, linux-kernel, Andrew Morton
On Saturday 23 September 2006 13:15, Mike Frysinger wrote:
> On 9/23/06, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Saturday 23 September 2006 08:50, Mike Frysinger wrote:
> > > On 9/22/06, Arnd Bergmann <arnd@arndb.de> 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 <><
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18
2006-09-23 6:50 ` Mike Frysinger
2006-09-23 11:03 ` Arnd Bergmann
@ 2006-09-23 11:28 ` Matthieu CASTET
2006-09-23 11:35 ` Mike Frysinger
2006-09-23 19:43 ` Arnd Bergmann
2 siblings, 1 reply; 53+ messages in thread
From: Matthieu CASTET @ 2006-09-23 11:28 UTC (permalink / raw)
To: linux-kernel
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)
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18
2006-09-23 11:28 ` Matthieu CASTET
@ 2006-09-23 11:35 ` Mike Frysinger
0 siblings, 0 replies; 53+ messages in thread
From: Mike Frysinger @ 2006-09-23 11:35 UTC (permalink / raw)
To: Matthieu CASTET; +Cc: linux-kernel
On 9/23/06, Matthieu CASTET <castet.matthieu@free.fr> 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
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18
2006-09-23 6:50 ` Mike Frysinger
2006-09-23 11:03 ` Arnd Bergmann
2006-09-23 11:28 ` Matthieu CASTET
@ 2006-09-23 19:43 ` Arnd Bergmann
2006-09-24 3:49 ` Luke Yang
2 siblings, 1 reply; 53+ messages in thread
From: Arnd Bergmann @ 2006-09-23 19:43 UTC (permalink / raw)
To: Mike Frysinger; +Cc: Luke Yang, linux-kernel, Andrew Morton
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 <><
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18
2006-09-23 19:43 ` Arnd Bergmann
@ 2006-09-24 3:49 ` Luke Yang
0 siblings, 0 replies; 53+ messages in thread
From: Luke Yang @ 2006-09-24 3:49 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: Mike Frysinger, linux-kernel, Andrew Morton
On 9/24/06, Arnd Bergmann <arnd@arndb.de> 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
luke.adi@gmail.com
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18
2006-09-23 0:18 ` Arnd Bergmann
2006-09-23 1:18 ` Randy.Dunlap
2006-09-23 6:50 ` Mike Frysinger
@ 2006-09-24 3:35 ` Aubrey
2006-09-24 3:50 ` Randy Dunlap
2006-09-25 6:54 ` Arnd Bergmann
2 siblings, 2 replies; 53+ messages in thread
From: Aubrey @ 2006-09-24 3:35 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: Luke Yang, linux-kernel, Andrew Morton
On 9/23/06, Arnd Bergmann <arnd@arndb.de> 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
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18
2006-09-24 3:35 ` Aubrey
@ 2006-09-24 3:50 ` Randy Dunlap
2006-09-24 4:28 ` Aubrey
2006-09-25 6:54 ` Arnd Bergmann
1 sibling, 1 reply; 53+ messages in thread
From: Randy Dunlap @ 2006-09-24 3:50 UTC (permalink / raw)
To: Aubrey; +Cc: Arnd Bergmann, Luke Yang, linux-kernel, Andrew Morton
On Sun, 24 Sep 2006 11:35:31 +0800 Aubrey wrote:
> On 9/23/06, Arnd Bergmann <arnd@arndb.de> 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
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18
2006-09-24 3:50 ` Randy Dunlap
@ 2006-09-24 4:28 ` Aubrey
0 siblings, 0 replies; 53+ messages in thread
From: Aubrey @ 2006-09-24 4:28 UTC (permalink / raw)
To: Randy Dunlap; +Cc: Arnd Bergmann, Luke Yang, linux-kernel, Andrew Morton
On 9/24/06, Randy Dunlap <rdunlap@xenotime.net> wrote:
> On Sun, 24 Sep 2006 11:35:31 +0800 Aubrey wrote:
>
> > On 9/23/06, Arnd Bergmann <arnd@arndb.de> 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.
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18
2006-09-24 3:35 ` Aubrey
2006-09-24 3:50 ` Randy Dunlap
@ 2006-09-25 6:54 ` Arnd Bergmann
2006-09-25 7:49 ` Aubrey
1 sibling, 1 reply; 53+ messages in thread
From: Arnd Bergmann @ 2006-09-25 6:54 UTC (permalink / raw)
To: Aubrey; +Cc: Luke Yang, linux-kernel, Andrew Morton
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 <><
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18
2006-09-25 6:54 ` Arnd Bergmann
@ 2006-09-25 7:49 ` Aubrey
2006-09-25 9:26 ` Arnd Bergmann
0 siblings, 1 reply; 53+ messages in thread
From: Aubrey @ 2006-09-25 7:49 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: Luke Yang, linux-kernel, Andrew Morton
On 9/25/06, Arnd Bergmann <arnd@arndb.de> 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
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18
2006-09-25 7:49 ` Aubrey
@ 2006-09-25 9:26 ` Arnd Bergmann
2006-09-25 9:39 ` Luke Yang
` (2 more replies)
0 siblings, 3 replies; 53+ messages in thread
From: Arnd Bergmann @ 2006-09-25 9:26 UTC (permalink / raw)
To: Aubrey; +Cc: Luke Yang, linux-kernel, Andrew Morton
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 <><
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18
2006-09-25 9:26 ` Arnd Bergmann
@ 2006-09-25 9:39 ` Luke Yang
2006-09-25 9:45 ` Aubrey
2006-09-25 15:39 ` Aubrey
2 siblings, 0 replies; 53+ messages in thread
From: Luke Yang @ 2006-09-25 9:39 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: Aubrey, linux-kernel, Andrew Morton
On 9/25/06, Arnd Bergmann <arnd@arndb.de> 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
luke.adi@gmail.com
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18
2006-09-25 9:26 ` Arnd Bergmann
2006-09-25 9:39 ` Luke Yang
@ 2006-09-25 9:45 ` Aubrey
2006-09-25 15:39 ` Aubrey
2 siblings, 0 replies; 53+ messages in thread
From: Aubrey @ 2006-09-25 9:45 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: Luke Yang, linux-kernel, Andrew Morton
On 9/25/06, Arnd Bergmann <arnd@arndb.de> 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
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18
2006-09-25 9:26 ` Arnd Bergmann
2006-09-25 9:39 ` Luke Yang
2006-09-25 9:45 ` Aubrey
@ 2006-09-25 15:39 ` Aubrey
2006-09-25 17:05 ` Arnd Bergmann
2 siblings, 1 reply; 53+ messages in thread
From: Aubrey @ 2006-09-25 15:39 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: Luke Yang, linux-kernel, Andrew Morton
On 9/25/06, Arnd Bergmann <arnd@arndb.de> 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
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18
2006-09-25 15:39 ` Aubrey
@ 2006-09-25 17:05 ` Arnd Bergmann
2006-09-26 3:42 ` Aubrey
2006-09-27 10:04 ` Aubrey
0 siblings, 2 replies; 53+ messages in thread
From: Arnd Bergmann @ 2006-09-25 17:05 UTC (permalink / raw)
To: Aubrey; +Cc: Luke Yang, linux-kernel, Andrew Morton
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 <><
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18
2006-09-25 17:05 ` Arnd Bergmann
@ 2006-09-26 3:42 ` Aubrey
2006-09-26 9:43 ` Arnd Bergmann
2006-09-27 10:04 ` Aubrey
1 sibling, 1 reply; 53+ messages in thread
From: Aubrey @ 2006-09-26 3:42 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: Luke Yang, linux-kernel, Andrew Morton
On 9/26/06, Arnd Bergmann <arnd@arndb.de> 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
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18
2006-09-26 3:42 ` Aubrey
@ 2006-09-26 9:43 ` Arnd Bergmann
0 siblings, 0 replies; 53+ messages in thread
From: Arnd Bergmann @ 2006-09-26 9:43 UTC (permalink / raw)
To: Aubrey; +Cc: Luke Yang, linux-kernel, Andrew Morton
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 <><
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18
2006-09-25 17:05 ` Arnd Bergmann
2006-09-26 3:42 ` Aubrey
@ 2006-09-27 10:04 ` Aubrey
2006-09-27 11:37 ` Arnd Bergmann
1 sibling, 1 reply; 53+ messages in thread
From: Aubrey @ 2006-09-27 10:04 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: Luke Yang, linux-kernel, Andrew Morton, Getz, Robin
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 <arnd@arndb.de> 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 <><
>
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18
2006-09-27 10:04 ` Aubrey
@ 2006-09-27 11:37 ` Arnd Bergmann
0 siblings, 0 replies; 53+ messages in thread
From: Arnd Bergmann @ 2006-09-27 11:37 UTC (permalink / raw)
To: Aubrey; +Cc: Luke Yang, linux-kernel, Andrew Morton, Getz, Robin
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 <><
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18
2006-09-21 3:32 [PATCH 1/4] Blackfin: arch patch for 2.6.18 Luke Yang
2006-09-21 9:59 ` Luke Yang
2006-09-23 0:18 ` Arnd Bergmann
@ 2006-09-23 21:27 ` David Woodhouse
2006-09-25 16:52 ` Randy Dunlap
3 siblings, 0 replies; 53+ messages in thread
From: David Woodhouse @ 2006-09-23 21:27 UTC (permalink / raw)
To: Luke Yang; +Cc: linux-kernel, Andrew Morton
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
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18
2006-09-21 3:32 [PATCH 1/4] Blackfin: arch patch for 2.6.18 Luke Yang
` (2 preceding siblings ...)
2006-09-23 21:27 ` David Woodhouse
@ 2006-09-25 16:52 ` Randy Dunlap
2006-09-25 18:05 ` Mike Frysinger
3 siblings, 1 reply; 53+ messages in thread
From: Randy Dunlap @ 2006-09-25 16:52 UTC (permalink / raw)
To: Luke Yang; +Cc: linux-kernel, Andrew Morton
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(¤t->mm->mmap_sem);
+ error = do_mmap_pgoff(file, addr, len, prot, flags, pgoff);
+ up_write(¤t->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?
+}
###
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18
2006-09-25 16:52 ` Randy Dunlap
@ 2006-09-25 18:05 ` Mike Frysinger
0 siblings, 0 replies; 53+ messages in thread
From: Mike Frysinger @ 2006-09-25 18:05 UTC (permalink / raw)
To: Randy Dunlap; +Cc: Luke Yang, linux-kernel, Andrew Morton
On 9/25/06, Randy Dunlap <rdunlap@xenotime.net> 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
^ permalink raw reply [flat|nested] 53+ messages in thread