Re: [PATCH -mm 1/5] Blackfin: blackfin architecture patch update

From: Paul Mundt
Date: Mon Mar 05 2007 - 04:26:42 EST


On Thu, Mar 01, 2007 at 12:14:40PM +0800, Wu, Bryan wrote:
> 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

It would be nice if this could be split and posted incrementally for
review. It's a bit of a pain reading through the entire thing in one
shot.

Anyways..

> Index: linux-2.6/arch/blackfin/Kconfig
> ===================================================================
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6/arch/blackfin/Kconfig 2007-03-01 10:30:27.000000000 +0800
[snip]
> +config ZONE_DMA
> + bool
> + default y
> +
You don't need this, just get rid of it and shove everything in
ZONE_NORMAL.

> +config BLACKFIN
> + bool
> + default y
> +
> +config BFIN
> + bool
> + default y
> +
Why are there two of these?

> +config GENERIC_IRQ_PROBE
> + bool
> + default y
> +
Whitespace damage.

> +config UCLINUX
> + bool
> + default y
> +
Dead symbol.

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

> +choice
> + prompt "Uncached SDRAM region"
> + default DMA_UNCACHED_1M
> + depends BFIN_DMA_5XX
> +config DMA_UNCACHED_2M
> + bool "Enable 2M DMA Zone"
> +config DMA_UNCACHED_1M
> + bool "Enable 1M DMA Zone"
> +config DMA_UNCACHED_NONE
> + bool "Disable DMA Zone"
> +endchoice
> +
Contrary to the comment, this is not a zone.

> +config DEBUG_HUNT_FOR_ZERO
> + bool "Catch NULL pointer reads/writes"
> + default y
> + help
> + Say Y here to catch reads/writes to anywhere in the memory range
> + from 0x0000 - 0x0FFF (the first 4k) of memory. This is useful in
> + catching common programming errors such as NULL pointer dereferences.
> +
> + Misbehaving applications will be killed (generate a SEGV) while the
> + kernel will trigger a panic.
> +
> + Enabling this option will take up an extra entry in CPLB table.
> + Otherwise, there is no extra overhead.
> +
Is this sane to have conditional?

> +config BOOTPARAM
> + bool "Compiled-in Kernel Boot Parameter"
> +
> +config BOOTPARAM_STRING
> + string "Kernel Boot Parameter"
> + default "console=ttyS0,57600"
> + depends on BOOTPARAM
> +
Any reason not to use CMDLINE_BOOL/CMDLINE like every other platform?

> +config NO_KERNEL_MSG
> + bool "Suppress Kernel BUG Messages"
> + help
> + Do not output any debug BUG messages within the kernel.
> +
This is useless. For starters, CONFIG_BUG already does this. Secondly,
this isn't used anywhere.

> +int __init blackfin_dma_init(void)
> +{
> + int i;
> +
> + printk(KERN_INFO "Blackfin DMA Controller\n");
> +
> + for (i = 0; i < MAX_BLACKFIN_DMA_CHANNEL; i++) {
> + dma_ch[i].chan_status = DMA_CHANNEL_FREE;
> + dma_ch[i].regs = base_addr[i];
> + init_MUTEX(&(dma_ch[i].dmalock));

The dmalock is only ever used as a mutex, use that instead.

> +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);
> + memset(dma_page, 0, PAGE_SIZE);
> + dma_base = PAGE_ALIGN(start);
> + dma_size = PAGE_ALIGN(end) - PAGE_ALIGN(start);
> + dma_pages = dma_size >> PAGE_SHIFT;
> + memset((void *)dma_base, 0, DMA_UNCACHED_REGION);
> + dma_initialized = 1;
> +
> + printk(KERN_INFO "%s: dma_page @ 0x%p - %d pages at 0x%08lx\n", __FUNCTION__,
> + dma_page, dma_pages, dma_base);
> +}

That's an "interesting" way of doing a bitmap. Please use a proper bitmap
for dma_page, there's infrastructure for all of this already without
having to come up with new schemes. dma_declare_coherent() is a good
example.

> +void *dma_alloc_coherent(struct device *dev, size_t size,
> + dma_addr_t * dma_handle, gfp_t gfp)
> +{
> + void *ret;
> +
> + ret = (void *)__alloc_dma_pages(get_pages(size));
> +
> + if (ret) {
> + memset(ret, 0, size);
> + *dma_handle = virt_to_phys(ret);
> + }
> +
No dcache write-back?

> +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)
> +{
> + int i;
> +
> + BUG_ON(direction == DMA_NONE);
> +
> + for (i = 0; i < nents; i++)
> + invalidate_dcache_range(sg_dma_address(&sg[i]),
> + sg_dma_address(&sg[i]) +
> + sg_dma_len(&sg[i]));
> +

Why are you using the different flushing routines here?

> +#ifdef DEBUG_SERIAL_EARLY_INIT
> + bfin_console_init(); /* early console registration */
> + /* this give a chance to get printk() working before crash. */
> +#endif
> +
Why not expose this as a sensible early_printk implementation?

> +#ifdef CONFIG_CONSOLE
> +#ifdef CONFIG_FRAMEBUFFER
> + conswitchp = &fb_con;
> +#else
> + conswitchp = 0;
> +#endif
> +#endif
> +
Can't you do this in the defconfig?

> + /* check the size of the l1 area */
> + l1_length = _etext_l1 - _stext_l1;
> + if (l1_length > L1_CODE_LENGTH)
> + panic("L1 memory overflow\n");
> +
> + l1_length = _ebss_l1 - _sdata_l1;
> + if (l1_length > L1_DATA_A_LENGTH)
> + panic("L1 memory overflow\n");
> +
That's not very nice. You can figure this out already at link time.

> +void do_gettimeofday(struct timeval *tv)
> +int do_settimeofday(struct timespec *tv)

These can use CONFIG_GENERIC_TIME.

> +static struct platform_device *cm_bf533_devices[] __initdata = {
> +#if defined(CONFIG_SERIAL_BFIN) || defined(CONFIG_SERIAL_BFIN_MODULE)
> + &bfin_uart_device,
> +#endif
> +
Why? You'll already free this up if nothing claims it.

One of the benefits of having the driver model is that we're able to get
rid of this sort of ifdef abortion.

> + /*
> + * initialize the bad page table and bad page to point
> + * to a couple of allocated pages
> + */
> + empty_bad_page_table = (unsigned long)alloc_bootmem_pages(PAGE_SIZE);
> + empty_bad_page = (unsigned long)alloc_bootmem_pages(PAGE_SIZE);
> + empty_zero_page = (unsigned long)alloc_bootmem_pages(PAGE_SIZE);
> + memset((void *)empty_zero_page, 0, PAGE_SIZE);
> +
dcache handling?

> + tmp = (unsigned long)l1sram_alloc(sizeof(struct l1_scratch_task_info));
> + if (tmp != (unsigned long)L1_SCRATCH_TASK_INFO) {
> + printk(KERN_EMERG "mem_init(): Did not get the right address from l1sram_alloc: %08lx != %08lx\n",
> + tmp, (unsigned long)L1_SCRATCH_TASK_INFO);
> + panic("No L1, time to give up\n");
> + }

This platform seems to really want to panic() at the first sign of
trouble. This is not a good policy, especially for something that's only
a micro-optimization.

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

All of this can be inlined in io.h, there's nothing noteworthy here.

> +static struct semaphore pfmon_sem;
> +
Use a mutex.

> +int __init oprofile_arch_init(struct oprofile_operations *ops)
> +{
> +#ifdef CONFIG_HARDWARE_PM
[snip]
> +#else
> + return -1;
> +#endif
> +}
> +
Uh.. fix your dependencies.

> +unsigned curr_pfctl, curr_count[2];
> +
Globals?

> Index: linux-2.6/include/asm-blackfin/bug.h
> ===================================================================
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6/include/asm-blackfin/bug.h 2007-03-01 10:30:27.000000000 +0800
> @@ -0,0 +1,15 @@
> +#ifndef _BLACKFIN_BUG_H
> +#define _BLACKFIN_BUG_H
> +
> +#ifdef CONFIG_BUG
> +#define HAVE_ARCH_BUG
> +#define BUG() do { \
> + dump_stack(); \
> + printk(KERN_WARNING "\nkernel BUG at %s:%d!\n",\
> + __FILE__, __LINE__); \
> + 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..

> +#ifndef __ARCH_BLACKFIN_CACHE_H
> +#define __ARCH_BLACKFIN_CACHE_H
> +
> +/* bytes per L1 cache line */
> +#define L1_CACHE_SHIFT 5 /* BlackFin loads 32 bytes for cache */
> +#define L1_CACHE_BYTES (1 << L1_CACHE_SHIFT)
> +
> +/* For speed we do need to align these ...MaTed---*/
> +/* But include/linux/cache.h does this for us if we DO not define ...MaTed---*/
> +#define __cacheline_aligned /***** maybe no need this Tony *****/
> +#define ____cacheline_aligned
> +
What the hell?

> +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
> +
> +# if defined(CONFIG_BLKFIN_CACHE)
> + blackfin_icache_flush_range((start), (end));
> +# endif
> +# if defined(CONFIG_BLKFIN_DCACHE)
> + blackfin_dcache_flush_range((start), (end));
> +# endif
> +
> +#endif
> +}
> +
This would probably be cleaner out-of-line, you can hide most of this
ugliness in your Makefile instead.

> +#pragma pack(2)
> +struct dmasg_t {
> + unsigned long next_desc_addr;
> + unsigned long start_addr;
> + unsigned short cfg;
> + unsigned short x_count;
> + short x_modify;
> + unsigned short y_count;
> + short y_modify;
> +};
> +#pragma pack()
> +
Do you really need to use pragma?

> +struct dma_register_t {

Why is there a _t here?

> +#define STR(X) STR1(X)
> +#define STR1(X) #X

You can use __stringify() for this if you feel you must.

> +static inline unsigned long bfin_get_addr_from_rp(unsigned long *ptr,
> + unsigned long relval,
> + unsigned long flags,
> + unsigned long *persistent)
> +{

Some of these look frighteningly large to be inlined..

> +#define dma_cache_inv(_start,_size) do { blkfin_inv_cache_all();} while (0)
> +#define dma_cache_wback(_start,_size) do { } while (0)
> +#define dma_cache_wback_inv(_start,_size) do { blkfin_inv_cache_all();} while (0)

What's the point of having selective flushing if you're 1) going to blow
it all away, and 2) not use these in the dma-mapping cases?

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

What sort of magical abstraction is this? Is there some reason you can't
just read and write the registers directly rather than having a wrapper
for _every_ possible register?

There are literally _thousands_ of lines of this stuff, and I imagine
that careful auditing would reveal that not even 5% of them are actually
used by the port. Presumably you have a processor manual, use that when
you need it, rather than inlining this crap in the kernel.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/