Re: [U-Boot] [PATCH 04/11] Blackfin: bf60x: add dma support

On Thu, Feb 7, 2013 at 6:11 PM, Wolfgang Denk wd@denx.de wrote:

Dear Sonic Zhang,

In message 1360223258-6945-5-git-send-email-sonic.adi@gmail.com you wrote:

...

From: Bob Liu lliubbo@gmail.com

Add dma support for bf60x.

Signed-off-by: Bob Liu lliubbo@gmail.com Signed-off-by: Sonic Zhang sonic.zhang@analog.com

---

arch/blackfin/include/asm/dma.h | 113 ++++++++++++---------- arch/blackfin/include/asm/mach-common/bits/dma.h | 48 +++++++++- arch/blackfin/lib/string.c | 97 ++++++++++--------- 3 files changed, 159 insertions(+), 99 deletions(-)

There are checkpatch errors and warnings. Please fix these!!

No problem.

...

struct dmasg_large { void *next_desc_addr;

• unsigned long start_addr;
  

• unsigned short cfg;
  

• unsigned short x_count;
  

• short x_modify;
  

• unsigned short y_count;
  

• short y_modify;
  

• u32 start_addr;
  

• u16 cfg;
  

• u16 x_count;
  

• s16 x_modify;
  

• u16 y_count;
  

• s16 y_modify;
  

} __attribute__((packed));

struct dmasg {

• unsigned long start_addr;
  

• unsigned short cfg;
  

• unsigned short x_count;
  

• short x_modify;
  

• unsigned short y_count;
  

• short y_modify;
  

• u32 start_addr;
  

• u16 cfg;
  

• u16 x_count;
  

• s16 x_modify;
  

• u16 y_count;
  

• s16 y_modify;
  

} __attribute__((packed));

These appear totally unrelated changes. These should be split out into separate patches.

OK, I will move these changes out as a new patch.

...

+/*

• • All Blackfin system MMRs are padded to 32bits even if the register
• • itself is only 16bits. So use a helper macro to streamline this.
• */

+#define __BFP(m) m; u16 __pad_##m struct dma_register {

• void *next_desc_ptr;    /* DMA Next Descriptor Pointer register */
  

• unsigned long start_addr;       /* DMA Start address  register */
  

• unsigned short cfg;     /* DMA Configuration register */
  

• unsigned short dummy1;  /* DMA Configuration register */
  

• unsigned long reserved;
  

• unsigned short x_count; /* DMA x_count register */
  

• unsigned short dummy2;
  

• short x_modify; /* DMA x_modify register */
  

• unsigned short dummy3;
  

• unsigned short y_count; /* DMA y_count register */
  

• unsigned short dummy4;
  

• short y_modify; /* DMA y_modify register */
  

• unsigned short dummy5;
  

• void *curr_desc_ptr;    /* DMA Current Descriptor Pointer
  

•                                    register */
  

• unsigned long curr_addr_ptr;    /* DMA Current Address Pointer
  

•                                            register */
  

• unsigned short irq_status;      /* DMA irq status register */
  

• unsigned short dummy6;
  

• unsigned short peripheral_map;  /* DMA peripheral map register */
  

• unsigned short dummy7;
  

• unsigned short curr_x_count;    /* DMA Current x-count register */
  

• unsigned short dummy8;
  

• unsigned long reserved2;
  

• unsigned short curr_y_count;    /* DMA Current y-count register */
  

• unsigned short dummy9;
  

• unsigned long reserved3;
  

+#ifdef __ADSPBF60x__

• void *next_desc_ptr;
  

• u32 start_addr;
  

• u32 config;
  

You drop all the comments; please don't do that.

Also, "cfg" used to be a short, now it gets replaced by "config" which is a u32. Is this correct?

Yes, this is correct for BF60x. The DMA config MMR on BF60x is 32 bits while it is 16 bits on BF5xx.

...

• void *next_desc_ptr;
  

• u32 start_addr;
  

• u16 __BFP(config);
  

• u32 __pad0;
  

• u16 __BFP(x_count);
  

• s16 __BFP(x_modify);
  

• u16 __BFP(y_count);
  

• s16 __BFP(y_modify);
  

• void *curr_desc_ptr;
  

• u32 curr_addr_ptr;
  

• u16 __BFP(status);
  

• u16 __BFP(peripheral_map);
  

• u16 __BFP(curr_x_count);
  

• u32 __pad1;
  

• u16 __BFP(curr_y_count);
  

• u32 __pad2;
  

Sorry, but this is unreadable. NAK.

OK, I will change back to the former definition style.

Sonic Zhang