Blackfin: blackfin architecture patch update

The whole patch is located at URL:
https://blackfin.uclinux.org/gf/download/frsrelease/39/2583/blackfin-arch.p
atch The incremental patch is located at URL:
https://blackfin.uclinux.org/gf/download/frsrelease/39/2584/blackfin-arch-m
m2-update.patch

Quoted 2 lines · Bryan

The whole patch is located at URL:

https://blackfin.uclinux.org/gf/download/frsrelease/39/2583/blackfin-arch.p

Quoted 2 lines

atch The incremental patch is located at URL:

https://blackfin.uclinux.org/gf/download/frsrelease/39/2584/blackfin-arch-m

Quoted 1 line

m2-update.patch

I'm not sure if that was intentional, but the second patch does not apply
on top of the -mm kernel but rather patch the the patch old itself. This basically makes it impossible to review just that part, so better
provide the diff between the kernel with the old patch and the kernel with the new patch next time.

Here is the update version of blackfin-arch.patch in -mm tree. simply add support to utrace and it was tested on blackfin STAMP board as well as other following patches.

(quoted diff) +5 -0

+EXPORT_SYMBOL(__ioremap);
+EXPORT_SYMBOL(strcmp);
+EXPORT_SYMBOL(strncmp);
+EXPORT_SYMBOL(dump_thread);
+

(quoted diff) +13 -0

+#if defined(CONFIG_MTD_UCLINUX)
+	/* generic memory mapped MTD driver */
+	memory_mtd_end = memory_end;
+
+	mtd_phys = _ramstart;
+	mtd_size = PAGE_ALIGN(*((unsigned long *)(mtd_phys + 8)));
+
+# if defined(CONFIG_EXT2_FS) || defined(CONFIG_EXT3_FS)
+	if (*((unsigned short *)(mtd_phys + 0x438)) == EXT2_SUPER_MAGIC)
+		mtd_size =
+		    PAGE_ALIGN(*((unsigned long *)(mtd_phys + 0x404)) << 10);
+# endif
+

(quoted diff) +5 -0

+#if defined(CONFIG_BF561)
+static struct cpu cpu[2];
+#else
+static struct cpu cpu[1];
+#endif

+/*
+ * ++roman (07/09/96): implemented signal stacks (specially for tosemu on
+ * Atari :-) Current limitation: Only one sigstack can be active at one

+ * If a second signal with SA_ONSTACK set arrives while working on a

+ * SA_ONSTACK is ignored. This behaviour avoids lots of trouble with nested
+ * signal handlers!
+ */

+extern int setup_irq(unsigned int irq, struct irqaction *handler);

+# define CHK_DEBUGGER_TRAP_MAYBE() do { if (kgdb_connected)

(quoted diff) +38 -0

+#else
+# define CHK_DEBUGGER_TRAP() do { } while (0)
+# define CHK_DEBUGGER_TRAP_MAYBE() do { } while (0)
+#endif
+
+	trace_buffer_save(j);
+
+	/* trap_c() will be called for exceptions. During exceptions
+	 * processing, the pc value should be set with retx value.
+	 * With this change we can cleanup some code in signal.c- TODO
+	 */
+	fp->orig_pc = fp->retx;
+	/* printk("exception: 0x%x, ipend=%x, reti=%x, retx=%x\n", 
+		trapnr, fp->ipend, fp->pc, fp->retx); */
+
+	/* send the appropriate signal to the user program */
+	switch (trapnr) {
+
+	/* This table works in conjuction with the one in ./mach-common/entry.S
+	 * Some exceptions are handled there (in assembly, in exception space)
+	 * Some are handled here, (in C, in interrupt space)
+	 * Some, like CPLB, are handled in both, where the normal path is
+	 * handled in assembly/exception space, and the error path is handled
+	 * here
+	 */
+
+	/* 0x00 - Linux Syscall, getting here is an error */
+	/* 0x01 - userspace gdb breakpoint, handled here */
+	case VEC_EXCPT01:
+		info.si_code = TRAP_ILLTRAP;
+		sig = SIGTRAP;
+		CHK_DEBUGGER_TRAP_MAYBE();
+		/* Check if this is a breakpoint in kernel space */
+		if (fp->ipend & 0xffc0)
+			return;
+		else
+			break;
+#ifdef CONFIG_KGDB

(quoted diff) +6 -0

+asmlinkage int sys_bfin_spinlock(int *spinlock)
+{
+	int ret = 0;
+	int tmp = 0;
+
+	local_irq_disable();

(quoted diff) +7 -0

+/* BASE LEVEL interrupt handler routines */
+asmlinkage void evt_emulation(void);
+asmlinkage void evt_exception(void);
+asmlinkage void trap(void);
+asmlinkage void evt_ivhw(void);
+asmlinkage void evt_timer(void);
+asmlinkage void evt_evt2(void);

(quoted diff) +6 -0

+/* BASE LEVEL interrupt handler routines */
+asmlinkage void evt_emulation(void);
+asmlinkage void evt_exception(void);
+asmlinkage void trap(void);
+asmlinkage void evt_ivhw(void);
+asmlinkage void evt_timer(void);

(quoted diff) +8 -0

+static __inline__ void atomic_add(int i, atomic_t * v)
+{
+	long flags;
+
+	local_irq_save(flags);
+	v->counter += i;
+	local_irq_restore(flags);
+}

(quoted diff) +16 -0

+struct spi_device_t {
+	char *dev_name;
+
+	unsigned short flag;
+	unsigned short bdrate;
+
+	unsigned short enable;
+	unsigned short master;
+	unsigned short out_opendrain;
+	unsigned short polar;
+	unsigned short phase;
+	unsigned short byteorder;	/* 0: MSB first; 1: LSB first; */
+	unsigned short size;	/* 0: 8 bits; 1: 16 bits */
+	unsigned short emiso;
+	unsigned short send_zero;
+	unsigned short more_data;

(quoted diff) +12 -0

+static __inline__ void set_bit(int nr, volatile unsigned long *addr)
+{
+	int *a = (int *)addr;
+	int mask;
+	unsigned long flags;
+
+	a += nr >> 5;
+	mask = 1 << (nr & 0x1f);
+	local_irq_save(flags);
+	*a |= mask;
+	local_irq_restore(flags);
+}

(quoted diff) +32 -0

+#if defined(ANOMALY_05000312) && defined(ANOMALY_05000244) 
+#define SSYNC() do { 	int _tmp; \
+                       __asm__ __volatile__ ("cli %0;\n\t"\
+	         			    "nop;nop;\n\t"\
+                                            "ssync;\n\t"\
+                                            "sti %0;\n\t" \
+                                             :"=d"(_tmp):);\
+ } while (0)
+#elif defined(ANOMALY_05000312) && !defined(ANOMALY_05000244) 
+#define SSYNC() do { 	int _tmp; \
+                       __asm__ __volatile__ ("cli %0;\n\t"\
+                                            "ssync;\n\t"\
+                                            "sti %0;\n\t" \
+                                             :"=d"(_tmp):);\
+ } while (0)
+#elif !defined(ANOMALY_05000312) && defined(ANOMALY_05000244) 
+#define SSYNC() do {__builtin_bfin_ssync();} while (0)	
+#elif !defined(ANOMALY_05000312) && !defined(ANOMALY_05000244) 
+#define SSYNC() do {__asm__ __volatile__ ("ssync;\n\t") } while (0)
+#endif
+
+
+#if defined(ANOMALY_05000312) && defined(ANOMALY_05000244) 
+#define CSYNC() do { 	int _tmp; \
+                       __asm__ __volatile__ ("cli %0;\n\t"\
+	         			    "nop;nop;\n\t"\
+                                            "csync;\n\t"\
+                                            "sti %0;\n\t" \
+                                             :"=d"(_tmp):);\
+ } while (0)
+#elif defined(ANOMALY_05000312) && !defined(ANOMALY_05000244) 
+#define CSYNC() do { 	int _tmp; \

+static inline struct task_struct *get_current(void) __attribute__

(quoted diff) +30 -0

+#ifndef _BLACKFIN_DPMC_H_
+#define _BLACKFIN_DPMC_H_
+
+#define SLEEP_MODE		1
+#define DEEP_SLEEP_MODE		2
+#define ACTIVE_PLL_DISABLED	3
+#define FULLON_MODE		4
+#define ACTIVE_PLL_ENABLED	5
+#define HIBERNATE_MODE		6
+
+#define IOCTL_FULL_ON_MODE	_IO('s', 0xA0)
+#define IOCTL_ACTIVE_MODE	_IO('s', 0xA1)
+#define IOCTL_SLEEP_MODE	_IO('s', 0xA2)
+#define IOCTL_DEEP_SLEEP_MODE	_IO('s', 0xA3)
+#define IOCTL_HIBERNATE_MODE	_IO('s', 0xA4)
+#define IOCTL_CHANGE_FREQUENCY	_IOW('s', 0xA5, unsigned long)
+#define IOCTL_CHANGE_VOLTAGE	_IOW('s', 0xA6, unsigned long)
+#define IOCTL_SET_CCLK		_IOW('s', 0xA7, unsigned long)
+#define IOCTL_SET_SCLK		_IOW('s', 0xA8, unsigned long)
+#define IOCTL_GET_PLLSTATUS	_IOW('s', 0xA9, unsigned long)
+#define IOCTL_GET_CORECLOCK	_IOW('s', 0xAA, unsigned long)
+#define IOCTL_GET_SYSTEMCLOCK	_IOW('s', 0xAB, unsigned long)
+#define IOCTL_GET_VCO		_IOW('s', 0xAC, unsigned long)
+#define IOCTL_DISABLE_WDOG_TIMER _IO('s', 0xAD)
+#define IOCTL_UNMASK_WDOG_WAKEUP_EVENT _IO('s',0xAE)
+#define IOCTL_PROGRAM_WDOG_TIMER _IOW('s',0xAF,unsigned long)
+#define IOCTL_CLEAR_WDOG_WAKEUP_EVENT _IO('s',0xB0)
+#define IOCTL_SLEEP_DEEPER_MODE _IO('s',0xB1)
+
+#define DPMC_MINOR		254

+#endif

(quoted diff) +5 -0

+#define readb(addr) ({ unsigned __v; \
+		       int _tmp; \
+		       __asm__ __volatile__ ("cli %1;\n\t"\
+	         			 "NOP;NOP;SSYNC;\n\t"\
+					     "%0 = b [%2] (z);\n\t"\

(quoted diff) +33 -0

+#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 TIOCSPTLCK _IOW('T',0x31, int) /* Lock/unlock Pty */

the driver writer doesn't want to know the size of the variable, and if it changes, they need to change every instance in the code, not just the macro.

These macros allow us to work around hardware bugs when accessing the registers by simply redefining the accessor to do something more complex.

It might be nicer to make this

	for (;;)
		asm volatile ("idle");

Quoted 2 lines

      for (;;)
              asm volatile ("idle");

This looks remarkably like relax_cpu()

Quoted 3 lines · Bryan

Here is the update version of blackfin-arch.patch in -mm tree. simply add support to utrace and it was tested on blackfin STAMP board as well as other following patches.

Wow, this has come a long way since I looked at the patches last year, good work!

I've gone through the complete patch again now, and these are the issues I've found in it. None of these are show-stoppers and I'd like to see it all go in during the next merge window. There should be enough time until then to address these points:

Quoted 11 lines

(quoted diff) +11 -0

+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);

Review

Quoted 4 lines

(quoted diff) +4 -0

+EXPORT_SYMBOL(__down_trylock);
+EXPORT_SYMBOL(__down_interruptible);
+
+EXPORT_SYMBOL(is_in_rom);

In general, please put EXPORT_SYMBOL lines below the definition of the symbol itself. This list of exports should only be used for symbols that come from assembly files.

You should probably also think about whether some of them are better done as EXPORT_SYMBOL_GPL.

Review

Quoted 7 lines

(quoted diff) +6 -0

+     pending = bfin_read_IPEND() & ~0x8000;
+     other_ints = pending & (pending - 1);
+     if (other_ints == 0)
+             lower_to_irq14();
+     irq_exit();
+

The last line here has trailing whitespace. While this gets automatically removed by akpm's scripts, you're normally better off not adding it in the first place, because it may cause your follow-on patches not to apply, aside from being wrong to start with.

Review

Quoted 11 lines

(quoted diff) +11 -0

+void machine_halt(void)
+{
+     for (;;)
+             /* nothing */ ;
+}
+
+void machine_power_off(void)
+{
+     for (;;)
+             /* nothing */ ;
+}

It might be nicer to make this

        for (;;)
                asm volatile ("idle");

Otherwise you end up burning CPU cycles after a halt without any particular need.

Quoted 22 lines

(quoted diff) +21 -0

+#if defined(CONFIG_MTD_UCLINUX)
+     /* generic memory mapped MTD driver */
+     memory_mtd_end = memory_end;
+
+     mtd_phys = _ramstart;
+     mtd_size = PAGE_ALIGN(*((unsigned long *)(mtd_phys + 8)));
+
+# if defined(CONFIG_EXT2_FS) || defined(CONFIG_EXT3_FS)
+     if (*((unsigned short *)(mtd_phys + 0x438)) == EXT2_SUPER_MAGIC)
+             mtd_size =
+                 PAGE_ALIGN(*((unsigned long *)(mtd_phys + 0x404)) << 10);
+# endif
+
+# if defined(CONFIG_CRAMFS)
+     if (*((unsigned long *)(mtd_phys)) == CRAMFS_MAGIC)
+             mtd_size = PAGE_ALIGN(*((unsigned long *)(mtd_phys + 0x4)));
+# endif
+
+# if defined(CONFIG_ROMFS_FS)
+     if (((unsigned long *)mtd_phys)[0] == ROMSB_WORD0
+         && ((unsigned long *)mtd_phys)[1] == ROMSB_WORD1)

Review

Quoted 2 lines

(quoted diff) +2 -0

+             mtd_size =
+                 PAGE_ALIGN(be32_to_cpu(((unsigned long *)mtd_phys)[2]));

This detection seems to me like a strange thing to do in setup_arch(). It should be possible to do this much later, at a point where the system is much less fragile and e.g. printk works. It could even be moved into some place in the mtd code itself, since other architectures might want to do the same thing.

Quoted 6 lines

(quoted diff) +5 -0

+#if defined(CONFIG_BF561)
+static struct cpu cpu[2];
+#else
+static struct cpu cpu[1];
+#endif

Review

Quoted 10 lines

(quoted diff) +10 -0

+static int __init topology_init(void)
+{
+#if defined (CONFIG_BF561)
+     register_cpu(&cpu[0], 0);
+     register_cpu(&cpu[1], 1);
+     return 0;
+#else
+     return register_cpu(cpu, 0);
+#endif
+}

I think you should try to avoid the special-case stuff here. You can have CONFIG_NR_CPUS in Kconfig set dependent on CONFIG_BF561 and change the code here (and similarly in other places) to

static struct cpu cpu[NR_CPUS];
static int __init topology_init(void)

{
        int i;
        for (i=0; i< NR_CPUS; i++) {
                register_cpu(&cpu[i], i);
        return 0;
}

Review

Quoted 8 lines

(quoted diff) +8 -0

+     for (i = ZERO_P; i <= L2_MEM; i++) {
+
+             if (cplb_data[i].valid) {
+
+                     as_1m = cplb_data[i].start % SIZE_1M;
+
+                     /* We need to make sure all sections are properly 1M aligned
+                      * However between Kernel Memory and the Kernel mtd section, depending

on the

Review

Quoted 8 lines

(quoted diff) +8 -0

+                      * rootfs size, there can be overlapping memory areas.
+                      */
+
+                     if (as_1m) {
+#ifdef CONFIG_MTD_UCLINUX
+                             if (i == SDRAM_RAM_MTD) {
+                                     if ((cplb_data[SDRAM_KERN].end + 1) > cplb_data[SDRAM_RAM_MTD].start)
+                                             cplb_data[SDRAM_RAM_MTD].start = (cplb_data[i].start & (-2*SIZE_1M))

+ SIZE_1M;

I count 6 levels of indentation, which severely limits readability, especially when you have terms this complex in the last level. Please try to split up functions like this into smaller units.

Quoted 3 lines

+/*
+ * ++roman (07/09/96): implemented signal stacks (specially for tosemu on
+ * Atari :-) Current limitation: Only one sigstack can be active at one

time.

Quoted 1 line

+ * If a second signal with SA_ONSTACK set arrives while working on a

sigstack,

Quoted 3 lines

+ * SA_ONSTACK is ignored. This behaviour avoids lots of trouble with nested
+ * signal handlers!
+ */

This comment is probably outdated.

Quoted 1 line

+extern int setup_irq(unsigned int irq, struct irqaction *handler);

this extern should be in a header.

Review

Quoted 8 lines

(quoted diff) +8 -0

+asmlinkage void trap_c(struct pt_regs *fp)
+{
+     int j, sig = 0;
+     siginfo_t info;
+     unsigned long trapnr = fp->seqstat & SEQSTAT_EXCAUSE;
+
+#ifdef CONFIG_KGDB
+# define CHK_DEBUGGER_TRAP() do { CHK_DEBUGGER(trapnr, sig, info.si_code,

fp,); } while (0)

Quoted 1 line

+# define CHK_DEBUGGER_TRAP_MAYBE() do { if (kgdb_connected)

CHK_DEBUGGER_TRAP(); } while (0)

Quoted 38 lines

(quoted diff) +38 -0

+#else
+# define CHK_DEBUGGER_TRAP() do { } while (0)
+# define CHK_DEBUGGER_TRAP_MAYBE() do { } while (0)
+#endif
+
+     trace_buffer_save(j);
+
+     /* trap_c() will be called for exceptions. During exceptions
+      * processing, the pc value should be set with retx value.
+      * With this change we can cleanup some code in signal.c- TODO
+      */
+     fp->orig_pc = fp->retx;
+     /* printk("exception: 0x%x, ipend=%x, reti=%x, retx=%x\n",
+             trapnr, fp->ipend, fp->pc, fp->retx); */
+
+     /* send the appropriate signal to the user program */
+     switch (trapnr) {
+
+     /* This table works in conjuction with the one in ./mach-common/entry.S
+      * Some exceptions are handled there (in assembly, in exception space)
+      * Some are handled here, (in C, in interrupt space)
+      * Some, like CPLB, are handled in both, where the normal path is
+      * handled in assembly/exception space, and the error path is handled
+      * here
+      */
+
+     /* 0x00 - Linux Syscall, getting here is an error */
+     /* 0x01 - userspace gdb breakpoint, handled here */
+     case VEC_EXCPT01:
+             info.si_code = TRAP_ILLTRAP;
+             sig = SIGTRAP;
+             CHK_DEBUGGER_TRAP_MAYBE();
+             /* Check if this is a breakpoint in kernel space */
+             if (fp->ipend & 0xffc0)
+                     return;
+             else
+                     break;
+#ifdef CONFIG_KGDB

Review

Quoted 10 lines

(quoted diff) +10 -0

+     case VEC_EXCPT02 :               /* gdb connection */
+             info.si_code = TRAP_ILLTRAP;
+             sig = SIGTRAP;
+             CHK_DEBUGGER_TRAP();
+             return;
+#else
+     /* 0x02 - User Defined, Caught by default */
+#endif
+     /* 0x03  - Atomic test and set */
+     case VEC_EXCPT03:

The table here probably gets converted by gcc into a lookup table, but I think it would be clearer to make that explicit and force the table here with something like:

static void trap_breakpoint(struct pt_regs *fp, siginfo_t *info,
unsigned int trapnr)

{
        info->si_code = TRAP_ILLTRAP;
        CHK_DEBUGGER_TRAP_MAYBE();
        if (fp->ipend & 0xffc0)
                return 0;
        return SIGTRAP;
}

#ifdef CONFIG_KGDB
static void trap_kgdb(struct pt_regs *fp, siginfo_t *info,
unsigned int trapnr)

{
        info->si_code = TRAP_ILLTRAP;
        CHK_DEBUGGER_TRAP();
        return SIGTRAP;
}

#endif

static void (traps[])(struct pt_regs *fp, siginfo_t *info,

                                 unsigned int trapnr) = {
        [VEC_EXCPT01] &trap_breakpoint, /* 0x01 - userspace gdb breakpoint */
#ifdef CONFIG_KGDB
        [VEC_EXCPT02] &trap_kgdb,  /* gdb connection */
#endif
        ...
};

asmlinkage void trap_c(struct pt_regs *fp)

{
        ...
        if (trapnr > 0x3f)
                goto error;
        if (!traps[trapnr])
                goto out;
        ret = traps[trapno](fp, &info, trapnr);
        if (!ret)
                goto out;
        ...

That should make sure you get the efficient trap handling almost and might even
save some code space (you'd have to try that).

Quoted 12 lines

(quoted diff) +12 -0

+asmlinkage int sys_bfin_spinlock(int *spinlock)
+{
+     int ret = 0;
+     int tmp = 0;
+
+     local_irq_disable();
+     ret = get_user(tmp, spinlock);
+     if (ret == 0) {
+             if (tmp)
+                     ret = 1;
+             tmp = 1;
+             put_user(tmp, spinlock);

Review

Quoted 4 lines

(quoted diff) +4 -0

+     }
+     local_irq_enable();
+     return ret;
+}

I'm curious: In your dual-core bf561, don't you actually need to implement something that maintains atomicity across cores rather than just across processes?

Review

Quoted 6 lines

(quoted diff) +5 -0

+#include <net/checksum.h>
+#include <asm/checksum.h>
+
+#ifdef CONFIG_IP_CHECKSUM_L1
+static unsigned short do_csum(const unsigned char *buff, int

len)__attribute__((l1_text));

Review

Quoted 13 lines

(quoted diff) +13 -0

+#endif
+
+static unsigned short do_csum(const unsigned char *buff, int len)
+{
+     register unsigned long sum = 0;
+     int swappem = 0;
+
+     if (1 & (unsigned long)buff) {
+             sum = *buff << 8;
+             buff++;
+             len--;
+             ++swappem;
+     }

For the non-L1 case, is there actually anything specific to blackfin in this code? I wonder if we should instead have a totally generic version of that in lib/csum.c for those architectures that don't want their own optimizations for this.

Review

Quoted 12 lines

(quoted diff) +12 -0

+static struct platform_device *stamp_devices[] __initdata = {
+#if defined(CONFIG_RTC_DRV_BFIN) || defined(CONFIG_RTC_DRV_BFIN_MODULE)
+     &rtc_device,
+#endif
+
+#if defined(CONFIG_BFIN_CFPCMCIA) || defined(CONFIG_BFIN_CFPCMCIA_MODULE)
+     &bfin_pcmcia_cf_device,
+#endif
+
+#if defined(CONFIG_USB_SL811_HCD) || defined(CONFIG_USB_SL811_HCD_MODULE)
+     &sl811_hcd_device,
+#endif

This array reminds me of the bad old days when all initialization functions had to be called from a common function. Normally I don't like to recommend using section magic for anything, but in this case I guess you'd be better off if you just do your own thing here, like:

#define BFIN_PLATFORM_DEVICE(name) struct platform_device name \
__attribute((section("initdata.platformdev")))

then declare your platform devices in the respective driver, like

static BFIN_PLATFORM_DEVICE(rtc_device) = {
        ...
};

and loop through the new section like you do for the initcalls.

Review

Quoted 10 lines

(quoted diff) +10 -0

+void get_bf537_ether_addr(char *addr)
+{
+     /* currently the mac addr is saved in flash */
+     int flash_mac = 0x203f0000;
+     *(u32 *)(&(addr[0])) = *(int *)flash_mac;
+     flash_mac += 4;
+     *(u16 *)(&(addr[4])) = (u16)*(int *)flash_mac;
+}
+
+EXPORT_SYMBOL(get_bf537_ether_addr);

This looks wrong in a number of aspects. Are there other things stored in the flash so you can make this a more general flash abstraction? Is there more than one driver using this at all? If not, it should probably be part of the driver, with the exact address being a compile-time constant for that driver.

It should probably also use bfin_readXX instead direct dereferences.

Review

Quoted 3 lines

(quoted diff) +2 -0

+/* The number of spurious interrupts */
+volatile unsigned int num_spurious;

Why volatile? It probably does not what you intended, maybe it should be atomic_t instead.

Quoted 7 lines

(quoted diff) +7 -0

+/* BASE LEVEL interrupt handler routines */
+asmlinkage void evt_emulation(void);
+asmlinkage void evt_exception(void);
+asmlinkage void trap(void);
+asmlinkage void evt_ivhw(void);
+asmlinkage void evt_timer(void);
+asmlinkage void evt_evt2(void);

Review

Quoted 10 lines

(quoted diff) +10 -0

+asmlinkage void evt_evt7(void);
+asmlinkage void evt_evt8(void);
+asmlinkage void evt_evt9(void);
+asmlinkage void evt_evt10(void);
+asmlinkage void evt_evt11(void);
+asmlinkage void evt_evt12(void);
+asmlinkage void evt_evt13(void);
+asmlinkage void evt_soft_int1(void);
+asmlinkage void evt_system_call(void);
+asmlinkage void init_exception_buff(void);

move the extern definitions to a header file please

Quoted 6 lines

(quoted diff) +6 -0

+/* BASE LEVEL interrupt handler routines */
+asmlinkage void evt_emulation(void);
+asmlinkage void evt_exception(void);
+asmlinkage void trap(void);
+asmlinkage void evt_ivhw(void);
+asmlinkage void evt_timer(void);

Review

Quoted 10 lines

(quoted diff) +10 -0

+asmlinkage void evt_evt2(void);
+asmlinkage void evt_evt7(void);
+asmlinkage void evt_evt8(void);
+asmlinkage void evt_evt9(void);
+asmlinkage void evt_evt10(void);
+asmlinkage void evt_evt11(void);
+asmlinkage void evt_evt12(void);
+asmlinkage void evt_evt13(void);
+asmlinkage void evt_soft_int1(void);
+asmlinkage void evt_system_call(void);

Then you don't have to write them twice ;-)

Quoted 8 lines

(quoted diff) +8 -0

+static __inline__ void atomic_add(int i, atomic_t * v)
+{
+     long flags;
+
+     local_irq_save(flags);
+     v->counter += i;
+     local_irq_restore(flags);
+}

Review

Quoted 10 lines

(quoted diff) +10 -0

+
+static __inline__ void atomic_sub(int i, atomic_t * v)
+{
+     long flags;
+
+     local_irq_save(flags);
+     v->counter -= i;
+     local_irq_restore(flags);
+
+}

The implementation of the atomic_t type looks very generic. Maybe it could be moved to asm-generic/atomic-irqdisable.h or similar to be shared by all architectures without atomic operations.

Quoted 21 lines

(quoted diff) +21 -0

+struct spi_device_t {
+     char *dev_name;
+
+     unsigned short flag;
+     unsigned short bdrate;
+
+     unsigned short enable;
+     unsigned short master;
+     unsigned short out_opendrain;
+     unsigned short polar;
+     unsigned short phase;
+     unsigned short byteorder;       /* 0: MSB first; 1: LSB first; */
+     unsigned short size;    /* 0: 8 bits; 1: 16 bits */
+     unsigned short emiso;
+     unsigned short send_zero;
+     unsigned short more_data;
+     unsigned short slave_sel;
+     unsigned short ti_mod;
+
+     unsigned short dma;     /* use dma mode or not */
+     unsigned short dma_config;      /* only valid if dma enabled */

Review

Quoted 5 lines

(quoted diff) +5 -0

+
+      irqreturn_t(*irq_handler) (int irq, void *dev_id,
+                                 struct pt_regs *regs);
+     void *priv_data;
+};

How does this fit in with the generic SPI code? Does it duplicate stuff from there, or do you use it?

Quoted 13 lines

(quoted diff) +12 -0

+static __inline__ void set_bit(int nr, volatile unsigned long *addr)
+{
+     int *a = (int *)addr;
+     int mask;
+     unsigned long flags;
+
+     a += nr >> 5;
+     mask = 1 << (nr & 0x1f);
+     local_irq_save(flags);
+     *a |= mask;
+     local_irq_restore(flags);
+}

Review

Quoted 10 lines

(quoted diff) +10 -0

+
+static __inline__ void __set_bit(int nr, volatile unsigned long *addr)
+{
+     int *a = (int *)addr;
+     int mask;
+
+     a += nr >> 5;
+     mask = 1 << (nr & 0x1f);
+     *a |= mask;
+}

same as for the atomic header, this could become generic.

Quoted 32 lines

(quoted diff) +32 -0

+#if defined(ANOMALY_05000312) && defined(ANOMALY_05000244)
+#define SSYNC() do {         int _tmp; \
+                       __asm__ __volatile__ ("cli %0;\n\t"\
+                                         "nop;nop;\n\t"\
+                                            "ssync;\n\t"\
+                                            "sti %0;\n\t" \
+                                             :"=d"(_tmp):);\
+ } while (0)
+#elif defined(ANOMALY_05000312) && !defined(ANOMALY_05000244)
+#define SSYNC() do {         int _tmp; \
+                       __asm__ __volatile__ ("cli %0;\n\t"\
+                                            "ssync;\n\t"\
+                                            "sti %0;\n\t" \
+                                             :"=d"(_tmp):);\
+ } while (0)
+#elif !defined(ANOMALY_05000312) && defined(ANOMALY_05000244)
+#define SSYNC() do {__builtin_bfin_ssync();} while (0)
+#elif !defined(ANOMALY_05000312) && !defined(ANOMALY_05000244)
+#define SSYNC() do {__asm__ __volatile__ ("ssync;\n\t") } while (0)
+#endif
+
+
+#if defined(ANOMALY_05000312) && defined(ANOMALY_05000244)
+#define CSYNC() do {         int _tmp; \
+                       __asm__ __volatile__ ("cli %0;\n\t"\
+                                         "nop;nop;\n\t"\
+                                            "csync;\n\t"\
+                                            "sti %0;\n\t" \
+                                             :"=d"(_tmp):);\
+ } while (0)
+#elif defined(ANOMALY_05000312) && !defined(ANOMALY_05000244)
+#define CSYNC() do {         int _tmp; \

Review

Quoted 10 lines

(quoted diff) +10 -0

+                       __asm__ __volatile__ ("cli %0;\n\t"\
+                                            "csync;\n\t"\
+                                            "sti %0;\n\t" \
+                                             :"=d"(_tmp):);\
+ } while (0)
+#elif !defined(ANOMALY_05000312) && defined(ANOMALY_05000244)
+#define CSYNC() do {__builtin_bfin_csync();} while (0)
+#elif !defined(ANOMALY_05000312) && !defined(ANOMALY_05000244)
+#define CSYNC() do {__asm__ __volatile__ ("csync;\n\t") } while (0)
+#endif

it would be nice if you could convert these to inline functions instead of macros.

Quoted 1 line

+static inline struct task_struct *get_current(void) __attribute__

((__const__));

Review

Quoted 6 lines

(quoted diff) +6 -0

+static inline struct task_struct *get_current(void)
+{
+     return (current_thread_info()->task);
+}
+
+#define      current (get_current())

You might want to maintain the current variable in the L1 memory, but probably that difference it not measurable.

Review

Quoted 9 lines

(quoted diff) +9 -0

+#ifdef BFIN_DMA_NDEBUG
+#define assert(expr) do {} while(0)
+#else
+#define assert(expr)                                                 \
+     if (!(expr)) {                                          \
+     printk(KERN_INFO "Assertion failed! %s, %s, %s, line=%d \n",    \
+     #expr, __FILE__,__FUNCTION__,__LINE__);                 \
+     }
+#endif

Please don't define your own assert() functions, in the kernel we have BUG_ON and WARN_ON for this.

Quoted 30 lines

(quoted diff) +30 -0

+#ifndef _BLACKFIN_DPMC_H_
+#define _BLACKFIN_DPMC_H_
+
+#define SLEEP_MODE           1
+#define DEEP_SLEEP_MODE              2
+#define ACTIVE_PLL_DISABLED  3
+#define FULLON_MODE          4
+#define ACTIVE_PLL_ENABLED   5
+#define HIBERNATE_MODE               6
+
+#define IOCTL_FULL_ON_MODE   _IO('s', 0xA0)
+#define IOCTL_ACTIVE_MODE    _IO('s', 0xA1)
+#define IOCTL_SLEEP_MODE     _IO('s', 0xA2)
+#define IOCTL_DEEP_SLEEP_MODE        _IO('s', 0xA3)
+#define IOCTL_HIBERNATE_MODE _IO('s', 0xA4)
+#define IOCTL_CHANGE_FREQUENCY       _IOW('s', 0xA5, unsigned long)
+#define IOCTL_CHANGE_VOLTAGE _IOW('s', 0xA6, unsigned long)
+#define IOCTL_SET_CCLK               _IOW('s', 0xA7, unsigned long)
+#define IOCTL_SET_SCLK               _IOW('s', 0xA8, unsigned long)
+#define IOCTL_GET_PLLSTATUS  _IOW('s', 0xA9, unsigned long)
+#define IOCTL_GET_CORECLOCK  _IOW('s', 0xAA, unsigned long)
+#define IOCTL_GET_SYSTEMCLOCK        _IOW('s', 0xAB, unsigned long)
+#define IOCTL_GET_VCO                _IOW('s', 0xAC, unsigned long)
+#define IOCTL_DISABLE_WDOG_TIMER _IO('s', 0xAD)
+#define IOCTL_UNMASK_WDOG_WAKEUP_EVENT _IO('s',0xAE)
+#define IOCTL_PROGRAM_WDOG_TIMER _IOW('s',0xAF,unsigned long)
+#define IOCTL_CLEAR_WDOG_WAKEUP_EVENT _IO('s',0xB0)
+#define IOCTL_SLEEP_DEEPER_MODE _IO('s',0xB1)
+
+#define DPMC_MINOR           254

Review

Quoted 10 lines

(quoted diff) +10 -0

+
+#define ON   0
+#define OFF  1
+
+unsigned long calc_volt(void);
+int calc_vlev(int vlt);
+unsigned long change_voltage(unsigned long volt);
+int calc_msel(int vco_hz);
+unsigned long change_frequency(unsigned long vco_mhz);
+int set_pll_div(unsigned short sel, unsigned char flag);

In general, you should be careful not to mix in-kernel declarations with user-visible parts. In this case, the ioctl definitions need to be exported via 'make headers_install', while the function declarations clearly are not useful for user space and should therefore be inside of '#ifdef __KERNEL__'. You should then list this file as 'unifdef-y' in the Kbuild file.

Review

Quoted 3 lines

(quoted diff) +3 -0

+
+#ifdef __KERNEL__
+#define SET_PERSONALITY(ex, ibcs2)

set_personality((ibcs2)?PER_SVR4:PER_LINUX)

Quoted 1 line

+#endif

This looks bogus, I don't think you implement personalities.

Quoted 5 lines

(quoted diff) +5 -0

+#define readb(addr) ({ unsigned __v; \
+                    int _tmp; \
+                    __asm__ __volatile__ ("cli %1;\n\t"\
+                                      "NOP;NOP;SSYNC;\n\t"\
+                                          "%0 = b [%2] (z);\n\t"\

Review

Quoted 10 lines

(quoted diff) +10 -0

+                                          "sti %1;\n\t" \
+  : "=d"(__v), "=d"(_tmp): "a"(addr)); (unsigned char)__v; })
+
+#define readw(addr) ({ unsigned __v; \
+                                        int _tmp; \
+                       __asm__ __volatile__ ("cli %1;\n\t"\
+                                                                      "NOP;NOP;SSYNC;\n\t"\
+                                                                      "%0 = w [%2] (z);\n\t"\
+                                             "sti %1;\n\t" \
+  : "=d"(__v), "=d"(_tmp): "a"(addr)); (unsigned short)__v; })

They should probably be inline functions.

Quoted 33 lines

(quoted diff) +33 -0

+#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

Review

Quoted 10 lines

(quoted diff) +10 -0

+#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) */

Quoted 1 line

+#define TIOCSPTLCK _IOW('T',0x31, int) /* Lock/unlock Pty */

These look like another good candidate for an asm-generic version.

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+extern void sys_free_irq(unsigned int irq, void *dev_id);

This functions should probably be renamed. Anything that starts with 'sys_' is normally a system call.

+

Review

Quoted 10 lines

(quoted diff) +10 -0

+/* 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)
+#define bfin_write_PLL_STAT(val)             bfin_write16(PLL_STAT,val)
+#define bfin_read_PLL_LOCKCNT()              bfin_read16(PLL_LOCKCNT)
+#define bfin_write_PLL_LOCKCNT(val)          bfin_write16(PLL_LOCKCNT,val)
+#define bfin_read_CHIPID()                   bfin_read32(CHIPID)
+#define bfin_read_SWRST()                    bfin_read16(SWRST)
+#define bfin_write_SWRST(val)                bfin_write16(SWRST,val)

I remember that we have discussed these macro abstractions before, but don't recall the result of the discussion. You have around 5000 such macros, and I still think it's not a helpful abstraction. It is much more common for device drivers to just use the read/write accessors directly. IIRC the objections that were raised (and my replies to them) were roughly:

Quoted 3 lines

the driver writer doesn't want to know the size of the variable, and if it changes, they need to change every instance in the code, not just the macro.

A driver writer should better know the type of variable he is changing, e.g. because of the type he passes in and out. Also, hardware registers don't suddenly change in size.

Quoted 3 lines

These macros allow us to work around hardware bugs when accessing the registers by simply redefining the accessor to do something more complex.

If there is a bug in the hardware, the workaround belongs into the driver. You can then still define a special inline function to access that particular register.

Quoted 3 lines · Arnd Bergmann

In general, please put EXPORT_SYMBOL lines below the definition of the symbol itself. This list of exports should only be used for symbols that come from assembly files.

What is the right way to export symbol coming from c files?

Quoted 5 lines

This detection seems to me like a strange thing to do in setup_arch(). It should be possible to do this much later, at a point where the system is much less fragile and e.g. printk works. It could even be moved into some place in the mtd code itself, since other architectures might want to do the same thing.

After download the rootfs image from host to the target ram, we need to move the image to the right place, so we need to know the size of the image at this time.

Quoted 3 lines

I'm curious: In your dual-core bf561, don't you actually need to implement something that maintains atomicity across cores rather than just across processes?

Yes, bf561 is a dual-core processor, but we are using only one core of bf561 now.
IMHO, BF561 architecture was not designed for SMP or NUMA.

Quoted 2 lines

How does this fit in with the generic SPI code? Does it duplicate stuff from there, or do you use it?

We use our own. We have dma which can be used for SPI operations.

Quoted 7 lines · Aubrey Li

Quoted 3 lines · Arnd Bergmann

In general, please put EXPORT_SYMBOL lines below the definition of the symbol itself. This list of exports should only be used for symbols that come from assembly files.

What is the right way to export symbol coming from c files?

As I said, below the symbol definition, like

int global_var;
EXPORT_SYMBOL(global_var);

int global_function(void)

{
	return 3;
}

EXPORT_SYMBOL(global_function);

Quoted 4 lines

Quoted 3 lines

I'm curious: In your dual-core bf561, don't you actually need to implement something that maintains atomicity across cores rather than just across processes?

Quoted 3 lines

Yes, bf561 is a dual-core processor, but we are using only one core of bf561 now.
IMHO, BF561 architecture was not designed for SMP or NUMA.

Interesting, so what is the intended use of the other core? Does the hardware have any way of supporting concurrency between the cores, other than sending interrupts between them?

Quoted 4 lines

Quoted 2 lines

How does this fit in with the generic SPI code? Does it duplicate stuff from there, or do you use it?

We use our own. We have dma which can be used for SPI operations.

I just looked again at your code. My question was more directed at whether you use your own SPI abstraction layer instead of drivers/spi, which you fortunately don't. The piece I was missing however is the spi_bfin5xx.c driver, which was not part of this patch, though you seem to rely on it. Is that already part of the -mm kernel?

	Arnd <><

Maybe NUMA is a solution, but it is not a wonderful solution.

In some application product, BF561 core A is running Linux kernel +Applications while BF561 core B is just for some complicated video/audio codec algorithm.

Any Linux multicore solution in BF561 situation is highly welcome.

Another question: when is the merge point from -mm to linus mainline, is it the same as the merge window after 2.6.21 released?

Quoted 2 lines · Bryan

Here is the update version of blackfin-arch.patch in -mm tree. simply add support to utrace and it was tested on blackfin STAMP

board

Quoted 1 line

as well as other following patches.

Wow, this has come a long way since I looked at the patches last year, good work!

I've gone through the complete patch again now, and these are the issues I've found in it. None of these are show-stoppers and I'd like to see it all go in during the next merge window. There should be enough time until then to address these points:

+

Review

Quoted 3 lines

(quoted diff) +3 -0

+/* 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)

Review

Quoted 2 lines

(quoted diff) +2 -0

+#define bfin_read_PLL_STAT()                 bfin_read16(PLL_STAT)
+#define bfin_write_PLL_STAT(val)

bfin_write16(PLL_STAT,val)

Quoted 1 line

+#define bfin_read_PLL_LOCKCNT()

bfin_read16(PLL_LOCKCNT)

Quoted 1 line

+#define bfin_write_PLL_LOCKCNT(val)

bfin_write16(PLL_LOCKCNT,val)

Review

Quoted 3 lines

(quoted diff) +3 -0

+#define bfin_read_CHIPID()                   bfin_read32(CHIPID)
+#define bfin_read_SWRST()                    bfin_read16(SWRST)
+#define bfin_write_SWRST(val)

bfin_write16(SWRST,val)

I remember that we have discussed these macro abstractions before, but don't
recall the result of the discussion. You have around 5000 such macros, and I still think it's not a helpful abstraction. It is much more common
for device drivers to just use the read/write accessors directly. IIRC the objections that were raised (and my replies to them) were roughly:

Quoted 3 lines

the driver writer doesn't want to know the size of the variable, and if it changes, they need to change every instance in the code, not just the macro.

A driver writer should better know the type of variable he is changing,
e.g. because of the type he passes in and out. Also, hardware registers
don't suddenly change in size.

Quoted 1 line

These macros allow us to work around hardware bugs when accessing

the

Quoted 2 lines

registers by simply redefining the accessor to do something more complex.

If there is a bug in the hardware, the workaround belongs into the driver. You can then still define a special inline function to access that particular register.

So could please give us some information about the merge window schedule, we may try to catch this.

Oh, if we should fix this issue, there are lots of work to do because tons of drivers rely on this. Maybe after some team internal discussion, we will give a solution to this.

Quoted 21 lines

(quoted diff) +21 -0

+#if defined(CONFIG_MTD_UCLINUX)
+     /* generic memory mapped MTD driver */
+     memory_mtd_end = memory_end;
+
+     mtd_phys = _ramstart;
+     mtd_size = PAGE_ALIGN(*((unsigned long *)(mtd_phys + 8)));
+
+# if defined(CONFIG_EXT2_FS) || defined(CONFIG_EXT3_FS)
+     if (*((unsigned short *)(mtd_phys + 0x438)) == EXT2_SUPER_MAGIC)
+             mtd_size =
+                 PAGE_ALIGN(*((unsigned long *)(mtd_phys + 0x404)) << 10);
+# endif
+
+# if defined(CONFIG_CRAMFS)
+     if (*((unsigned long *)(mtd_phys)) == CRAMFS_MAGIC)
+             mtd_size = PAGE_ALIGN(*((unsigned long *)(mtd_phys + 0x4)));
+# endif
+
+# if defined(CONFIG_ROMFS_FS)
+     if (((unsigned long *)mtd_phys)[0] == ROMSB_WORD0
+         && ((unsigned long *)mtd_phys)[1] == ROMSB_WORD1)

Review

Quoted 2 lines

(quoted diff) +2 -0

+             mtd_size =
+                 PAGE_ALIGN(be32_to_cpu(((unsigned long *)mtd_phys)[2]));

This detection seems to me like a strange thing to do in setup_arch(). It should be possible to do this much later, at a point where the system is much less fragile and e.g. printk works. It could even be moved into some place in the mtd code itself, since other architectures might want to do the same thing.

Quoted 33 lines

(quoted diff) +33 -0

+#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

Review

Quoted 10 lines

(quoted diff) +10 -0

+#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) */

Quoted 1 line

+#define TIOCSPTLCK _IOW('T',0x31, int) /* Lock/unlock Pty */

These look like another good candidate for an asm-generic version.

Quoted 3 lines

These macros allow us to work around hardware bugs when accessing the registers by simply redefining the accessor to do something more complex.

If there is a bug in the hardware, the workaround belongs into the driver. You can then still define a special inline function to access that particular register.

Here is the update version of blackfin-arch.patch in -mm tree. simply add support to utrace and it was tested on blackfin STAMP board as well as other following patches.

The whole patch is located at URL:
https://blackfin.uclinux.org/gf/download/frsrelease/39/2583/blackfin-arch.patch

Index: linux-2.6/arch/blackfin/Kconfig
===================================================================

arch/blackfin/Kconfig new file +0 -0

arch/blackfin/Kconfig new file +4 -0

+config DEBUG_HUNT_FOR_ZERO
+	bool "Catch NULL pointer reads/writes"
+	default y
+	help

arch/blackfin/Kconfig new file +6 -0

+void dma_alloc_init(unsigned long start, unsigned long end)
+{
+	spin_lock_init(&dma_page_lock);
+	dma_initialized = 0;
+
+	dma_page = (unsigned int *)__get_free_page(GFP_KERNEL);

arch/blackfin/Kconfig new file +15 -0

+dma_addr_t
+dma_map_single(struct device *dev, void *ptr, size_t size,
+	       enum dma_data_direction direction)
+{
+	BUG_ON(direction == DMA_NONE);
+
+	blackfin_dcache_invalidate_range((unsigned long)ptr,
+					 (unsigned long)ptr + size);
+
+	return (dma_addr_t) ptr;
+}
+
+int
+dma_map_sg(struct device *dev, struct scatterlist *sg, int nents,
+	   enum dma_data_direction direction)

Index: linux-2.6/arch/blackfin/mm/kmap.c

arch/blackfin/Kconfig new file +3 -0

+int __init oprofile_arch_init(struct oprofile_operations *ops)
+{
+#ifdef CONFIG_HARDWARE_PM

Index: linux-2.6/include/asm-blackfin/bug.h
===================================================================

include/asm-blackfin/bug.h new file +5 -0

@@ -0,0 +1,15 @@
+#ifndef _BLACKFIN_BUG_H
+#define _BLACKFIN_BUG_H
+
+#ifdef CONFIG_BUG
+#define HAVE_ARCH_BUG

include/asm-blackfin/bug.h new file +12 -0

+static inline void flush_icache_range(unsigned start, unsigned end)
+{
+#if defined(CONFIG_BLKFIN_DCACHE) && defined(CONFIG_BLKFIN_CACHE)
+
+# if defined(CONFIG_BLKFIN_WT)
+	blackfin_icache_flush_range((start), (end));
+# else
+	blackfin_icache_dcache_flush_range((start), (end));
+# endif
+
+#else
+

+struct dma_register_t {

Quoted 6 lines

(quoted diff) +6 -0

+comment "Memory Optimizations"
+
+config I_ENTRY_L1
+	bool "Locate interrupt entry code in L1 Memory"
+	default y
+	help

Review

Quoted 3 lines

(quoted diff) +3 -0

+	  If enabled interrupt entry code (STORE/RESTORE CONTEXT) is linked
+	  into L1 instruction memory.(less latency)
+

Wow, this is really crying out for a special linker section with slightly more intelligent relocation logic. You should flag the performance critical parts to be located in L1 memory directly with a section attribute, rather than making everything selectable. If you overflow you can simply spill in to main memory.

Quoted 6 lines · Paul Mundt

Quoted 6 lines

(quoted diff) +6 -0

+comment "Memory Optimizations"
+
+config I_ENTRY_L1
+	bool "Locate interrupt entry code in L1 Memory"
+	default y
+	help

Quoted 8 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

+	  If enabled interrupt entry code (STORE/RESTORE CONTEXT) is linked
+	  into L1 instruction memory.(less latency)
+

Wow, this is really crying out for a special linker section with slightly more intelligent relocation logic. You should flag the performance critical parts to be located in L1 memory directly with a section attribute, rather than making everything selectable. If you overflow you can simply spill in to main memory.

This is done intentionally, because it's also possible for user code to be loaded into L1 memory. We want to give users the option to avoid filling it all up with kernel code.

Quoted 17 lines

Quoted 15 lines · Paul Mundt

Quoted 9 lines

(quoted diff) +7 -0

+comment "Memory Optimizations"
+
+config I_ENTRY_L1
+   bool "Locate interrupt entry code in L1 Memory"
+   default y
+   help
+     If enabled interrupt entry code (STORE/RESTORE CONTEXT) is

linked +     into L1 instruction memory.(less latency) +

Wow, this is really crying out for a special linker section with slightly more intelligent relocation logic. You should flag the performance critical parts to be located in L1 memory directly with a section attribute, rather than making everything selectable. If you overflow you can simply spill in to main memory.

Quoted 3 lines

This is done intentionally, because it's also possible for user code to be loaded into L1 memory.  We want to give users the option to avoid filling it all up with kernel code.

So then why not make the userspace component of it optional and allow a
size cap for kernel usage that's configurable if it's enabled? This degree of abstraction is almost worse than no abstraction.

On Mon 5 Mar 2007 07:39, Paul Mundt pondered:

Quoted 8 lines · Bernd Schmidt

Quoted 3 lines

This is done intentionally, because it's also possible for user code to be loaded into L1 memory.?? We want to give users the option to avoid filling it all up with kernel code.

So then why not make the userspace component of it optional and allow a
size cap for kernel usage that's configurable if it's enabled? This degree of abstraction is almost worse than no abstraction.

I don't understand why you think lots of options are a bad thing??

For most embedded targets, people want/need easy knobs to turn to try and optimise the system level performance. I would guess that SH users want to do the same thing?

That is what this does - it is just a easy to use knob.

Quoted 5 lines

On Mon 5 Mar 2007 07:39, Paul Mundt pondered:

Quoted 1 line · Bernd Schmidt

That is what this does - it is just a easy to use knob.

This is hardly a knob, you're adding one config option per function to relocate in to the L1 memory, leaving it up to the user to decide what's best positioned there from the kernel point of view and what's left with userspace to play with. This is simply _not_ how you want to do this sort
of interface, rather than making any sort of usability decisions, you've pushed it all on the user under the label of flexibility.

What happens now if you suddenly start having other blocks of SRAM in future parts that are either shared across CPUs or just more buffer space
for a single CPU? Do you start to repeat the config options for that space, too?

There are things that will be a win to have located in on-chip SRAM, and others that will be less important. If you're concerned about this, you should simply pinpoint the hot paths that benefit the most from being relocated and weight that against a build-time configuration of how much room the user wants to play with. This way you can figure out all of your
limits directly at link time, as you're arguably looking at an effectively static configuration anyways.

If you really want to break it down on priority, use something similar to
initcall levels. Start with the most critical bits, and stash as much as possible in whatever on-chip memory you have available (while heeding the
user constraints) and then spill the rest to system memory.

Throwing this all at the user simply shows that the functions being relocated haven't been profiled adequately with real workloads.

You can't seriously expect your users to define what's the most timing critical and hope to get useful result.

These are simply not things the user should ever _care_ about.

If a user
wants to use on-chip memory, presumably they have a target application in mind, and they know how much space they need. Beyond that, they expect the kernel to do the best it can with the space that's left over for it to play with.

If a user has to sit around profiling their workload to figure out what config options to set to chew through the rest of the L1 memory, you've completely lost at intuitive design.

This is like taking the KDE approach to UI design and applying it to the kernel, exposing every possible setting as a user-settable option and avoiding setting any sort of sane default in the hope that user knows best. This simply doesn't work.

On Mon 5 Mar 2007 09:00, Paul Mundt pondered:

Quoted 2 lines

Throwing this all at the user simply shows that the functions being relocated haven't been profiled adequately with real workloads.

Actually - that is not true at all - we have profiled these extensively across many workloads on various real world applications from fax machine to bar code scanner, to software radio, to internet radio - and that is the point - it all depends on what the user application/drivers that are loaded are doing. I can't decided what is best for the user, because I don't know what they are doing.

Quoted 4 lines

If a user wants to use on-chip memory, presumably they have a target application in mind, and they know how much space they need. Beyond that, they expect the kernel to do the best it can with the space that's left over for it to play with.

And that is the problem - the kernel doesn't know how much on chip memory an application which will be loaded in the future might need. The user needs to be able to control the amount of on-chip memory that the kernel uses - hence the knobs.

Quoted 3 lines

If a user has to sit around profiling their workload to figure out what config options to set to chew through the rest of the L1 memory, you've completely lost at intuitive design.

Embedded has never been intuitive.

Normally, the majority of end users leave things set by the default, meaning their application doesn't use the on-chip memory, and the kernel is allowed to. If they want to start using on-chip memory, they are forced to do a re-compile, with the kernel functions using off-chip memory, and see how much of on-chip memory their application uses. They calculate the difference, and do a little system level profiling, and put the most used kernel functions back.

You are asking me to try to decide what is best for all the potential end users, with out knowing what the end user is doing. What ends up happening is that people make modifications (normally poor ones) and then still ask for help when their modifications break.

I would rather give them a supported (but arguablely confusing) method of customisation. (This is why it was added - end users were asking for it, and when we didn't have it, they would try to make the modifications themselves, break it, and report a bug. After telling 8 different people the same thing, when do you decide this is a feature you need to support?)

Quoted 4 lines

On Mon 5 Mar 2007 09:00, Paul Mundt pondered:

Quoted 2 lines

Throwing this all at the user simply shows that the functions being relocated haven't been profiled adequately with real workloads.

Quoted 8 lines

Actually - that is not true at all - we have profiled these extensively
across many workloads on various real world applications from fax machine to bar code scanner, to software radio, to internet radio - and that is the point - it all depends on what the user application/drivers
that are loaded are doing. I can't decided what is best for the user, because I don't know what they are doing.

And there are just as many cases that are common regardless of the workload that are simply good decisions. Exception vectors are a good example, either you get a boost from having them in L1 memory or you don't, the workload really doesn't matter.

Workloads will of course have variations, but it's optimizing the hot paths that end up being quite common that end up being the most useful. These are not things that should be required from the end user.

Quoted 9 lines

Quoted 4 lines

If a user wants to use on-chip memory, presumably they have a target application in mind, and they know how much space they need. Beyond that, they expect the kernel to do the best it can with the space that's left over for it to play with.

And that is the problem - the kernel doesn't know how much on chip memory an application which will be loaded in the future might need. The user needs to be able to control the amount of on-chip memory that the kernel uses - hence the knobs.

That's crap, for the exact reason that these are not run-time configurable knobs, they are static build-time components. If a user has to scale back the kernel users until their application fits, then they already know how much space they're going to need. Once you know that, reserving a chunk for userspace and limiting the kernel's usage is quite simple.

Quoted 5 lines

Quoted 3 lines

If a user has to sit around profiling their workload to figure out what config options to set to chew through the rest of the L1 memory, you've completely lost at intuitive design.

Embedded has never been intuitive.

Embedded has also never been about throwing everything at the user that's possible. Flexibility is good. Intelligent design is good. Blindly tossing everything the user's way and hoping for the best is not.

Quoted 8 lines

Normally, the majority of end users leave things set by the default, meaning their application doesn't use the on-chip memory, and the kernel is allowed to. If they want to start using on-chip memory, they are forced to do a re-compile, with the kernel functions using off-chip memory, and see how much of on-chip memory their application uses. They calculate the difference, and do a little system level profiling, and put the most used kernel functions back.

Yes, precisely. This is all something that should be done by the kernel _for_ the user. They can of course add their own instrumentation to the hot paths that their workload hits that aren't generally handled, but this should not be a required step.

If I'm working on an embedded application, I expect my kernel to do some sensible things with the hardware available. I don't want to have to manually configure ever possible thing in order to get a system that behaves in a reasonable manner, and I certainly don't want to have to do this multiple times with a profiler for paths that are inevitably going to be quite common anyways.

Quoted 4 lines

You are asking me to try to decide what is best for all the potential end users, with out knowing what the end user is doing. What ends up happening is that people make modifications (normally poor ones) and then still ask for help when their modifications break.

No, I'm asking you to set some sensible defaults, provide an interface that works for the common cases, and allow the special cases to remain simply that.

Quoted 6 lines

I would rather give them a supported (but arguablely confusing) method of customisation. (This is why it was added - end users were asking for it, and when we didn't have it, they would try to make the modifications themselves, break it, and report a bug. After telling 8 different people the same thing,

There's also a substantial differences between what customers are telling
you they want, what they're trying to accomplish, and what they actually need. If you want to add something simple, setup an attribute tag for the
section and tell the customers to flag the cases they're most concerned with. Obviously you expect them to be sitting there with a profiler open anyways, so this should not be a problem.

Also note that we are not required to accept bad code in the kernel because you happen to have customer requirements for it.

The point is, doing this as a config option design is just broken. It doesn't scale for new code, and it doesn't scale as soon as you start throwing in more blocks of SRAM with varying locality, latencies, etc. Littering around ifdefs for this stuff all over the code is equally ugly.

I'll reiterate my original suggestion, allow the user to cap the amount of memory that's used by the kernel, then add something akin to initcall levels that you can use for prioritizing items to be relocated in on-chip memory, while spilling the rest to RAM. With this sort of scheme you can also trivially spill in to additional SRAM pools and whatever else you have handy as you go along.

You can of course argue that as an embedded platform the user must make all of these decisions on their own, but there can be a compromise if the kernel's at least half-way intelligent about matters.

While it may not count for much, until something is done about this and the other issues that were raised earlier in the thread, I'd have to NAK this patch. If not wanting to manually select everything that goes in to L1 memory makes me too unsophisticated by blackfin standards, so be it ;-)

Review

Quoted 3 lines

(quoted diff) +3 -0

+config DEBUG_HUNT_FOR_ZERO
+     bool "Catch NULL pointer reads/writes"
+     default y

Is this sane to have conditional?

Quoted 6 lines

(quoted diff) +6 -0

+#ifdef CONFIG_BUG
+#define HAVE_ARCH_BUG
+#define BUG() do { \
+     dump_stack(); \
+     printk(KERN_WARNING "\nkernel BUG at %s:%d!\n",\
+              __FILE__, __LINE__); \

Review

Quoted 6 lines

(quoted diff) +6 -0

+     panic("BUG!"); \
+} while (0)
+#endif
+
+#include <asm-generic/bug.h>
+#endif

What do you need HAVE_ARCH_BUG for? You're not doing anything with it..

Quoted 3 lines

(quoted diff) +3 -0

+int __init oprofile_arch_init(struct oprofile_operations *ops)
+{
+#ifdef CONFIG_HARDWARE_PM

[snip]

Review

Quoted 4 lines

(quoted diff) +4 -0

+#else
+     return -1;
+#endif
+}

Uh.. fix your dependencies.

Quoted 4 lines · Paul Mundt

Quoted 3 lines

(quoted diff) +3 -0

+int __init oprofile_arch_init(struct oprofile_operations *ops)
+{
+#ifdef CONFIG_HARDWARE_PM

[snip]

Quoted 6 lines

Review

Quoted 4 lines

(quoted diff) +4 -0

+#else
+     return -1;
+#endif
+}

Uh.. fix your dependencies.

can you elaborate ?