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

Sonic Zhang
2013-02-08T04:17:48Z

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