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

[Sonic Zhang]

On Thu, Feb 7, 2013 at 6:11 PM, Wolfgang Denk <wd at denx.de> wrote:
> Dear Sonic Zhang,
>
> In message <1360223258-6945-5-git-send-email-sonic.adi at gmail.com> you wrote:
>> From: Bob Liu <lliubbo at gmail.com>
>>
>> Add dma support for bf60x.
>>
>> Signed-off-by: Bob Liu <lliubbo at gmail.com>
>> Signed-off-by: Sonic Zhang <sonic.zhang at 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