Re: ADI Blackfin patch for kernel 2.6.14

[Luke Yang <luke.adi@gmail.com>]

>
> We've taken arch patches in a single hit before.  It's not such a bad thing.
>
> A basic requirement should be "it should all compile before and after the
> patch".  That's pretty hard to do in this case if it's split up.
>
> That being said, a 1.6MB patch is a bit hard to review, mainly because it
> doesn't fit through the email server.
>
> From a quick look:
>
> > +static spinlock_t dma_page_lock = SPIN_LOCK_UNLOCKED;
>
> DEFINE_SPIN_LOCK()
>
> > +static unsigned int dma_initialized = 0;
>
> Remove the `= 0'
>
> +/*
> + * Initial task structure.
> + *
> + * All other task structs will be allocated on slabs in fork.c
> + */
> +__asm__(".align 4");
> +struct task_struct init_task = INIT_TASK(init_task);
>
> weird.  That align will probably go into .text, rather than where you want
> it.  Use __attribute__((__aligned__(4))) or ____cacheline_aligned, or just
> remove it - the compiler will align this guy on a 4-byte boundary anyway.
>
>
> Does this architecture support SMP?  I see it's BROKEN_ON_SMP, but there
> seems to be some smp-style stuff in there.

   It doesn't support SMP now.

>
>
> One concern when adding a new architecture is: will it be maintained
> long-term?  We don't want to merge an arch and then have it bitrot.  Who is
> behind this port, and how do we know that they'll still be around and doing
> things in two years' time?

   I don't clearly know the process of maintaining an arch in kernel. 
But I am sure we can follow the right process.  My question is: How do
they maintain the m68knommu arch? I think it need the uclinux patch to
run on real platfrom. What is the process like?

>
>
> Can this arch use the generic IRQ handling code in kernel/irq/?

     Yes.

>
>
> The idle routines don't appear to be up-to-date wrt post-2.6.14 changes.
> Or if they are, they won't be after I merge Nick's stuff ;)
>
>
> get_reg() is way too big to be inlined.
>
> Ditto put_reg().
>
>
> Can this arch use the generic lib/semaphore-sleepers.c?
>
> > +extern void icache_init(void);
> > +extern void dcache_init(void);
> > +extern int read_iloc(void);
> > +extern unsigned long ipdt_table[];
> > +extern unsigned long dpdt_table[];
> > +extern unsigned long icplb_table[];
> > +extern unsigned long dcplb_table[];
> > +int DmaMemCpy(char *dest_addr, char *source_addr, unsigned short size);
> > +int DmaMemCpy16(char *dest_addr, char *source_addr, int size);
>
> extern decls should always go in header files.  If things like
> icache_init() aren't in any headers, well mutter.  It'd be nice to fix
> that.  Involves touching all architectures, yeah, not your job...
>
>
> touch_l1_data() can have static scope.  Please review all global symbols
> for this.
>
>
> Does a new arch need to support old_mmap()?
>
>
> old_select()?
>
>
> > +void time_sched_init(irqreturn_t(*timer_routine)
> > +                   (int, void *, struct pt_regs *));
> > +unsigned long gettimeoffset(void);
> > +extern unsigned long wall_jiffies;
> > +extern int setup_irq(unsigned int, struct irqaction *);
> > +inline static void do_leds(void);
> > +
> > +extern u_long get_cclk(void);
>
> More extern-decls-in-c-files
>
> > +asmlinkage int sys_bfin_spinlock(int *spinlock)
>
> Whoa, that's a syscall I never expected to see.  What's it do?  Shouldn't
> it be using get_user() and put_user()?  Or will this forever be a nommu
> arch?
>
>
> What _is_ a bluefin, anyway?
>
>
> Are precompiled cross-complers/assemblers available anywhere?

    Yes, please visit blackfin.uclinux.org to get our toolchain.

>
>
> bix:/home/akpm> grep volatile bfin_r2_4kernel-2.6.14.patch | wc -l
>   2901
>
> Cow.  You know that volatile in-kernel is basically always wrong?
>
    To the other detail questions/issues, I'll write to you soon.
Thank you for your help.

Luke Yang