Messages in this thread | | | Date | Fri, 27 Mar 2009 11:25:30 +0100 (CET) | From | Thomas Gleixner <> | Subject | Re: [PATCH 9/13] score - New architecure port to SunplusCT S+CORE processor |
| |
On Fri, 27 Mar 2009, liqin.chen@sunplusct.com wrote: > + > +#include <linux/init.h> > +#include <linux/interrupt.h> > +#include <linux/irq.h> > +#include <linux/kernel_stat.h> > +#include <linux/proc_fs.h> > +#include <linux/seq_file.h> > + > +extern void interrupt_exception_vector(void);
Move this to a header file please.
> +/* > + * handles all normal device IRQs > + */ > +asmlinkage void do_IRQ(int irq) > +{ > + irq_enter(); > + generic_handle_irq(irq); > + irq_exit(); > +} > + > +/* > + * on-CPU PIC operations > + */ > +void ack_bad_irq(unsigned int irq) > +{ > + printk("unexpected IRQ # %d\n", irq); > +} > + > +static void score_mask(unsigned int irq_nr) > +{ > + unsigned int irq_source = 63 - irq_nr; > + > + if (irq_source < 32) > + rIMASK_L |= 1 << irq_source; > + else > + rIMASK_H |= 1 << (irq_source - 32); > +} > + > +static void score_unmask(unsigned int irq_nr) > +{ > + unsigned int irq_source = 63 - irq_nr; > + > + if (irq_source < 32) > + rIMASK_L &= ~(1 << irq_source); > + else > + rIMASK_H &= ~(1 << (irq_source - 32)); > +} > + > +struct irq_chip score_irq_chip = { > + .name = "Score7-level", > + .mask = score_mask, > + .mask_ack = score_mask, > + .unmask = score_unmask, > + .end = score_mask,
.end is not needed.
> +}; > + > +/* > + * initialise the interrupt system > + */ > +void __init init_IRQ(void) > +{ > + int index; > + unsigned long target_addr; > + > + for (index = 0; index < NR_IRQS; ++index) > + set_irq_chip_and_handler(index, &score_irq_chip, > + handle_level_irq); > + > + for (target_addr = IRQ_VECTOR_BASE_ADDR; > + target_addr <= IRQ_VECTOR_END_ADDR; > + target_addr += 0x10) > + memcpy((void*)target_addr, interrupt_exception_vector, > 0x10);
Magic number 0x10 ?
> + > + rIMASK_L=0xffffffff; > + rIMASK_H=0xffffffff;
lower case variable names please. What's this magic 0xfffffff initialization ?
> + > + __asm__ __volatile__( > + "mtcr %0,cr3\n\t" > + :: "r" (EXCEPTION_VECTOR_BASE_ADDR | 1));
Please explain what such asm constructs are doing either by adding a comment or by moving it into an inline function with a self explanatory function name. > + > +void *module_alloc(unsigned long size) > +{ > + if(size != 0)
missing blank: if (). Please run your patches through checkpatch.pl
> + return vmalloc(size); > + else > + return 0;
return NULL;
Please simplify to:
return size ? vmalloc(size) : NULL;
> + > +void module_arch_cleanup(struct module *mod) > +{}
either void func(arg) { } or void func(arg) { }
Also those empty functions can be implemented as static inline. That avoids the call to the empty function and gets optimized out by the compiler.
> + > +/* If or when software machine-restart is implemented, add code here. */ > +void machine_restart(char *command) > +{}
See above.
> +/* If or when software machine-halt is implemented, add code here. */ > +void machine_halt(void) > +{} > + > +/* If or when software machine-power-off is implemented, add code here. > */ > +void machine_power_off(void) > +{} > + > +/* > + * The idle thread. There's no useful work to be > + * done, so just try to conserve power and have a > + * low exit latency (ie sit in a loop waiting for > + * somebody to say that they'd like to reschedule) > + */ > +void __noreturn cpu_idle(void) > +{ > + /* endless idle loop with no priority at all */ > + while (1) { > + while (!need_resched()) { > + if (cpu_wait) > + (*cpu_wait)();
Please initialize cpu_wait with a default function and just call
cpu_wait();
> + } > + preempt_enable_no_resched(); > + schedule(); > + preempt_disable(); > + } > +} > + > +asmlinkage void ret_from_fork(void);
Declarations in header files please
> +void start_thread(struct pt_regs *regs, unsigned long pc, unsigned long > sp) > +{ > + unsigned long status; > + > + /* New thread loses kernel privileges. */ > + status = regs->cp0_psr & ~(KU_MASK); > + status |= KU_USER; > + regs->cp0_psr = status; > + regs->cp0_epc = pc; > + regs->regs[0] = sp; > +} > + > +void exit_thread(void) > +{} > + > +/* > + * When a process does an "exec", machine state like FPU and debug > + * registers need to be reset. This is a hook function for that. > + * Currently we don't have any such state to reset, so this is empty. > + */ > +void flush_thread(void) > +{} > + > +/* > + * set up the kernel stack and exception frames for a new process > + */ > +int copy_thread(int nr, unsigned long clone_flags, unsigned long usp, > + unsigned long stk_sz, struct task_struct *p, struct pt_regs *regs) > +{ > + struct thread_info *ti = task_thread_info(p); > + struct pt_regs *childregs = task_pt_regs(p); > + > + p->set_child_tid = p->clear_child_tid = NULL; > + > + *childregs = *regs; > + childregs->regs[7] = 0; /* Clear error flag */ > + childregs->regs[4] = 0; /* Child gets zero as > return value */ > + regs->regs[4] = p->pid; > + > + if (childregs->cp0_psr & 0x8) /* test kernel fork or > user fork */ > + childregs->regs[0] = usp; /* user fork */ > + else { > + childregs->regs[28] = (unsigned long) ti; /* kernel fork > */ > + childregs->regs[0] = (unsigned long) childregs; > + } > + p->thread.reg0 = (unsigned long) childregs; > + p->thread.reg3 = (unsigned long) ret_from_fork; > + p->thread.cp0_psr = 0; > + > + return 0; > +} > + > +/* Fill in the fpu structure for a core dump. */ > +int dump_fpu(struct pt_regs *regs, elf_fpregset_t *r) > +{ > + return 1; > +} > + > +void elf_dump_regs(elf_greg_t *gp, struct pt_regs *regs) > +{} > + > +int dump_task_regs(struct task_struct *tsk, elf_gregset_t *regs) > +{ > + return 1; > +} > + > +static void __noreturn kernel_thread_helper(void *unused0, int (*fn)(void > *), void *arg, void *unused1) > +{ > + do_exit(fn(arg)); > +} > + > +/* > + * Create a kernel thread. > + */ > +long kernel_thread(int (*fn)(void *), void *arg, unsigned long flags) > +{ > + struct pt_regs regs; > + > + memset(®s, 0, sizeof(regs)); > + > + regs.regs[6] = (unsigned long) arg; > + regs.regs[5] = (unsigned long) fn; > + regs.cp0_epc = (unsigned long) kernel_thread_helper; > + regs.cp0_psr = (regs.cp0_psr & ~(0x1|0x4|0x8)) | (regs.cp0_psr & > 0x3) <<2; > + > + return do_fork(flags | CLONE_VM | CLONE_UNTRACED, 0, ®s, 0, > NULL, NULL); > +} > + > +unsigned long thread_saved_pc(struct task_struct *tsk) > +{ > + return task_pt_regs(tsk)->cp0_epc; > +} > + > +unsigned long get_wchan(struct task_struct *task) > +{ > + unsigned long pc = 0; > + > + if (!task || task == current || task->state == TASK_RUNNING) > + goto out;
No goto needed. A simple return 0 is sufficient.
> + if (!task_stack_page(task)) > + goto out;
Ditto
> + pc = task_pt_regs(task)->cp0_epc;
Then this becomes
return task_pt_regs....;
> +out: > + return pc; > +} > +
Thanks,
tglx
| |