Re: [PATCH 2/2] dma: Add Spreadtrum DMA controller driver

[Baolin Wang]

Hi,

On 25 July 2017 at 11:38, Vinod Koul  wrote:
> On Mon, Jul 24, 2017 at 02:46:00PM +0800, Baolin Wang wrote:
>> Hi,
>>
>> On 六,  7月 22, 2017 at 01:27:31下午 +0530, Vinod Koul wrote:
>> > On Tue, Jul 18, 2017 at 03:06:12PM +0800, Baolin Wang wrote:
>
>> > > +static void sprd_dma_set_uid(struct sprd_dma_chn *mchan)
>> > > +{
>> > > + struct sprd_dma_dev *sdev = to_sprd_dma_dev(>chan);
>> > > + u32 dev_id = mchan->dev_id;
>> > > +
>> > > + if (dev_id != DMA_SOFTWARE_UID)
>> >
>> > Whats a UID?
>>
>> It is for users, every user was assigned one unique hardware ID.
>> Then the user can trigger the DMA to transfer by the user ID.
>
> sounds like a slave id to me (hint read again struct dma_slave_config)

Yes.

>
>> > > + u32 fragmens_len;
>> > > + u32 block_len;
>> >
>> > oh please, I think I will stop here now :(
>> >
>> > > + u32 transcation_len;
>> > > + u32 src_step;
>> > > + u32 des_step;
>> > > + u32 src_frag_step;
>> > > + u32 dst_frag_step;
>> > > + u32 src_blk_step;
>> > > + u32 dst_blk_step;
>> > > + u32 wrap_ptr;
>> > > + u32 wrap_to;
>> > > + u32 dev_id;
>> > > + enum dma_end_type is_end;
>> >
>> > Looking at this I think these are overkill, many of them can be handled
>> > properly by current dmaengine interfaces, so please use those before you
>> > invent your own...
>> >
>> > Also the code is bloated because you don't use virt-dma, pls use that. I
>> > skipped many parts of the driver as I feel driver needs more work.
>>
>> OK. I will check the virt-dma. Thanks for your commnets.
>
> Ok, but the bigger concern is that people have defined generic interfaces
> for everyone to use, so you should also use them, the hw doesn't seem
> anything special which cannot be accommodated in the current fwk, if not do
> tell me which parts don't fit before you invent your own interfaces...

OK. I think I missed something and I will check the generic interfaces
again. Thanks for your help.

-- 
Baolin.wang
Best Regards

Re: [PATCH 2/2] dma: Add Spreadtrum DMA controller driver

[Baolin Wang]

Hi,

On 25 July 2017 at 11:38, Vinod Koul  wrote:
> On Mon, Jul 24, 2017 at 02:46:00PM +0800, Baolin Wang wrote:
>> Hi,
>>
>> On 六,  7月 22, 2017 at 01:27:31下午 +0530, Vinod Koul wrote:
>> > On Tue, Jul 18, 2017 at 03:06:12PM +0800, Baolin Wang wrote:
>
>> > > +static void sprd_dma_set_uid(struct sprd_dma_chn *mchan)
>> > > +{
>> > > + struct sprd_dma_dev *sdev = to_sprd_dma_dev(>chan);
>> > > + u32 dev_id = mchan->dev_id;
>> > > +
>> > > + if (dev_id != DMA_SOFTWARE_UID)
>> >
>> > Whats a UID?
>>
>> It is for users, every user was assigned one unique hardware ID.
>> Then the user can trigger the DMA to transfer by the user ID.
>
> sounds like a slave id to me (hint read again struct dma_slave_config)

Yes.

>
>> > > + u32 fragmens_len;
>> > > + u32 block_len;
>> >
>> > oh please, I think I will stop here now :(
>> >
>> > > + u32 transcation_len;
>> > > + u32 src_step;
>> > > + u32 des_step;
>> > > + u32 src_frag_step;
>> > > + u32 dst_frag_step;
>> > > + u32 src_blk_step;
>> > > + u32 dst_blk_step;
>> > > + u32 wrap_ptr;
>> > > + u32 wrap_to;
>> > > + u32 dev_id;
>> > > + enum dma_end_type is_end;
>> >
>> > Looking at this I think these are overkill, many of them can be handled
>> > properly by current dmaengine interfaces, so please use those before you
>> > invent your own...
>> >
>> > Also the code is bloated because you don't use virt-dma, pls use that. I
>> > skipped many parts of the driver as I feel driver needs more work.
>>
>> OK. I will check the virt-dma. Thanks for your commnets.
>
> Ok, but the bigger concern is that people have defined generic interfaces
> for everyone to use, so you should also use them, the hw doesn't seem
> anything special which cannot be accommodated in the current fwk, if not do
> tell me which parts don't fit before you invent your own interfaces...

OK. I think I missed something and I will check the generic interfaces
again. Thanks for your help.

-- 
Baolin.wang
Best Regards

Re: [PATCH 2/2] dma: Add Spreadtrum DMA controller driver

[Vinod Koul]

On Mon, Jul 24, 2017 at 02:46:00PM +0800, Baolin Wang wrote:
> Hi,
> 
> On 六,  7月 22, 2017 at 01:27:31下午 +0530, Vinod Koul wrote:
> > On Tue, Jul 18, 2017 at 03:06:12PM +0800, Baolin Wang wrote:

> > > +static void sprd_dma_set_uid(struct sprd_dma_chn *mchan)
> > > +{
> > > + struct sprd_dma_dev *sdev = to_sprd_dma_dev(>chan);
> > > + u32 dev_id = mchan->dev_id;
> > > +
> > > + if (dev_id != DMA_SOFTWARE_UID)
> > 
> > Whats a UID?
> 
> It is for users, every user was assigned one unique hardware ID.
> Then the user can trigger the DMA to transfer by the user ID.

sounds like a slave id to me (hint read again struct dma_slave_config)

> > > + u32 fragmens_len;
> > > + u32 block_len;
> > 
> > oh please, I think I will stop here now :(
> > 
> > > + u32 transcation_len;
> > > + u32 src_step;
> > > + u32 des_step;
> > > + u32 src_frag_step;
> > > + u32 dst_frag_step;
> > > + u32 src_blk_step;
> > > + u32 dst_blk_step;
> > > + u32 wrap_ptr;
> > > + u32 wrap_to;
> > > + u32 dev_id;
> > > + enum dma_end_type is_end;
> > 
> > Looking at this I think these are overkill, many of them can be handled
> > properly by current dmaengine interfaces, so please use those before you
> > invent your own...
> > 
> > Also the code is bloated because you don't use virt-dma, pls use that. I
> > skipped many parts of the driver as I feel driver needs more work.
> 
> OK. I will check the virt-dma. Thanks for your commnets.

Ok, but the bigger concern is that people have defined generic interfaces
for everyone to use, so you should also use them, the hw doesn't seem
anything special which cannot be accommodated in the current fwk, if not do
tell me which parts don't fit before you invent your own interfaces...

-- 
~Vinod

Re: [PATCH 2/2] dma: Add Spreadtrum DMA controller driver

[Vinod Koul]

On Mon, Jul 24, 2017 at 02:46:00PM +0800, Baolin Wang wrote:
> Hi,
> 
> On 六,  7月 22, 2017 at 01:27:31下午 +0530, Vinod Koul wrote:
> > On Tue, Jul 18, 2017 at 03:06:12PM +0800, Baolin Wang wrote:

> > > +static void sprd_dma_set_uid(struct sprd_dma_chn *mchan)
> > > +{
> > > + struct sprd_dma_dev *sdev = to_sprd_dma_dev(>chan);
> > > + u32 dev_id = mchan->dev_id;
> > > +
> > > + if (dev_id != DMA_SOFTWARE_UID)
> > 
> > Whats a UID?
> 
> It is for users, every user was assigned one unique hardware ID.
> Then the user can trigger the DMA to transfer by the user ID.

sounds like a slave id to me (hint read again struct dma_slave_config)

> > > + u32 fragmens_len;
> > > + u32 block_len;
> > 
> > oh please, I think I will stop here now :(
> > 
> > > + u32 transcation_len;
> > > + u32 src_step;
> > > + u32 des_step;
> > > + u32 src_frag_step;
> > > + u32 dst_frag_step;
> > > + u32 src_blk_step;
> > > + u32 dst_blk_step;
> > > + u32 wrap_ptr;
> > > + u32 wrap_to;
> > > + u32 dev_id;
> > > + enum dma_end_type is_end;
> > 
> > Looking at this I think these are overkill, many of them can be handled
> > properly by current dmaengine interfaces, so please use those before you
> > invent your own...
> > 
> > Also the code is bloated because you don't use virt-dma, pls use that. I
> > skipped many parts of the driver as I feel driver needs more work.
> 
> OK. I will check the virt-dma. Thanks for your commnets.

Ok, but the bigger concern is that people have defined generic interfaces
for everyone to use, so you should also use them, the hw doesn't seem
anything special which cannot be accommodated in the current fwk, if not do
tell me which parts don't fit before you invent your own interfaces...

-- 
~Vinod

Re: [PATCH 2/2] dma: Add Spreadtrum DMA controller driver

[Baolin Wang]

Hi,

On 六,  7月 22, 2017 at 01:27:31下午 +0530, Vinod Koul wrote:
> On Tue, Jul 18, 2017 at 03:06:12PM +0800, Baolin Wang wrote:
> 
> > +/* DMA global registers definition */
> > +#define DMA_GLB_PAUSE  0x0
> > +#define DMA_GLB_FRAG_WAIT  0x4
> > +#define DMA_GLB_REQ_PEND0_EN   0x8
> > +#define DMA_GLB_REQ_PEND1_EN   0xc
> > +#define DMA_GLB_INT_RAW_STS0x10
> > +#define DMA_GLB_INT_MSK_STS0x14
> > +#define DMA_GLB_REQ_STS0x18
> > +#define DMA_GLB_CHN_EN_STS 0x1c
> > +#define DMA_GLB_DEBUG_STS  0x20
> > +#define DMA_GLB_ARB_SEL_STS0x24
> > +#define DMA_GLB_CHN_START_CHN_CFG1 0x28
> > +#define DMA_GLB_CHN_START_CHN_CFG2 0x2c
> > +#define DMA_CHN_LLIST_OFFSET   0x10
> > +#define DMA_GLB_REQ_CID(base, uid) \
> > +   ((unsigned long)(base) + 0x2000 + 0x4 * ((uid) - 1))
> 
> why do you need a cast here...

Since the UID registers address is calculated by the UID number. But
I can remove the cast and define the UID global macros.

> 
> > +/* DMA_CHN_INTC register definition */
> > +#define DMA_CHN_INT_MASK   GENMASK(4, 0)
> > +#define DMA_CHN_INT_CLR_OFFSET 24
> 
> Why cant this be expressed in GENMASK?

Since this is not one mask, it is one offset. If we use GENMASK(4, 3)
instead of magic number 24, it is not very clear for the offset bit.

> 
> > +/* DMA_CHN_CFG register definition */
> > +#define DMA_CHN_EN BIT(0)
> > +#define DMA_CHN_PRIORITY_OFFSET12
> > +#define LLIST_EN_OFFSET4
> > +#define CHN_WAIT_BDONE 24
> > +#define DMA_DONOT_WAIT_BDONE   1
> 
> All these too...
> 
> > +static void sprd_dma_set_uid(struct sprd_dma_chn *mchan)
> > +{
> > +   struct sprd_dma_dev *sdev = to_sprd_dma_dev(>chan);
> > +   u32 dev_id = mchan->dev_id;
> > +
> > +   if (dev_id != DMA_SOFTWARE_UID)
> 
> Whats a UID?

It is for users, every user was assigned one unique hardware ID.
Then the user can trigger the DMA to transfer by the user ID.

> 
> > +   writel(mchan->chn_num + 1, (void __iomem *)DMA_GLB_REQ_CID(
> > +  sdev->glb_base, dev_id));
> 
> And again the cast, I dont think we need casts like this...

OK.

> 
> > +static void sprd_dma_clear_int(struct sprd_dma_chn *mchan)
> > +{
> > +   u32 intc = readl(mchan->chn_base + DMA_CHN_INTC);
> > +
> > +   intc |= DMA_CHN_INT_MASK << DMA_CHN_INT_CLR_OFFSET;
> > +   writel(intc, mchan->chn_base + DMA_CHN_INTC);
> 
> How about adding a updatel macro and use that here and few other places.

OK.

> 
> > +static void sprd_dma_stop_and_disable(struct sprd_dma_chn *mchan)
> > +{
> > +   u32 cfg = readl(mchan->chn_base + DMA_CHN_CFG);
> > +   u32 pause, timeout = DMA_CHN_PAUSE_CNT;
> > +
> > +   if (!(cfg & DMA_CHN_EN))
> > +   return;
> > +
> > +   pause = readl(mchan->chn_base + DMA_CHN_PAUSE);
> > +   pause |= DMA_CHN_PAUSE_EN;
> > +   writel(pause, mchan->chn_base + DMA_CHN_PAUSE);
> > +
> > +   do {
> > +   pause = readl(mchan->chn_base + DMA_CHN_PAUSE);
> > +   if (pause & DMA_CHN_PAUSE_STS)
> > +   break;
> > +
> > +   cpu_relax();
> > +   } while (--timeout > 0);
> 
> We should check here if timeout occured and at least log the error

Yes, will add one warning meesage.

> 
> > +static enum dma_int_type sprd_dma_get_int_type(struct sprd_dma_chn *mchan)
> > +{
> > +   u32 intc_reg = readl(mchan->chn_base + DMA_CHN_INTC);
> > +
> > +   if (intc_reg & CFGERR_INT_STS)
> > +   return CONFIG_ERR;
> > +   else if (intc_reg & LLIST_INT_STS)
> > +   return LIST_DONE;
> > +   else if (intc_reg & TRSC_INT_STS)
> > +   return TRANS_DONE;
> > +   else if (intc_reg & BLK_INT_STS)
> > +   return BLK_DONE;
> > +   else if (intc_reg & FRAG_INT_STS)
> > +   return FRAG_DONE;
> 
> this seems a good case for using switch-cases

OK.

> 
> > +static int sprd_dma_chn_start_chn(struct sprd_dma_chn *mchan,
> > + struct sprd_dma_desc *mdesc)
> > +{
> > +   struct sprd_dma_dev *sdev = to_sprd_dma_dev(>chan);
> > +   enum dma_flags flag = mdesc->dma_flags;
> > +   int chn = mchan->chn_num + 1;
> > +   unsigned int cfg_group1, cfg_group2, start_mode = 0;
> > +
> > +   if (!(flag & (DMA_GROUP1_SRC | DMA_GROUP2_SRC | DMA_GROUP1_DST |
> > + DMA_GROUP2_DST)))
> > +   return 0;
> > +
> > +   if (flag & (DMA_GROUP1_SRC | DMA_GROUP2_SRC)) {
> > +   switch (flag & (DMA_MUTL_FRAG_DONE | DMA_MUTL_BLK_DONE |
> > +   DMA_MUTL_TRANS_DONE | DMA_MUTL_LIST_DONE)) {
> > +   case DMA_MUTL_FRAG_DONE:
> > +   start_mode = 0;
> > +   break;
> 
> empty line after each break helps with readability

OK.

> 
> > +/*
> > + * struct sprd_dma_cfg: DMA configuration for users
> > + * @config: salve config structure
> 
> typo

Sorry, will fix it.

> 
> > + * 

Re: [PATCH 2/2] dma: Add Spreadtrum DMA controller driver

[Baolin Wang]

Hi,

On 六,  7月 22, 2017 at 01:27:31下午 +0530, Vinod Koul wrote:
> On Tue, Jul 18, 2017 at 03:06:12PM +0800, Baolin Wang wrote:
> 
> > +/* DMA global registers definition */
> > +#define DMA_GLB_PAUSE  0x0
> > +#define DMA_GLB_FRAG_WAIT  0x4
> > +#define DMA_GLB_REQ_PEND0_EN   0x8
> > +#define DMA_GLB_REQ_PEND1_EN   0xc
> > +#define DMA_GLB_INT_RAW_STS0x10
> > +#define DMA_GLB_INT_MSK_STS0x14
> > +#define DMA_GLB_REQ_STS0x18
> > +#define DMA_GLB_CHN_EN_STS 0x1c
> > +#define DMA_GLB_DEBUG_STS  0x20
> > +#define DMA_GLB_ARB_SEL_STS0x24
> > +#define DMA_GLB_CHN_START_CHN_CFG1 0x28
> > +#define DMA_GLB_CHN_START_CHN_CFG2 0x2c
> > +#define DMA_CHN_LLIST_OFFSET   0x10
> > +#define DMA_GLB_REQ_CID(base, uid) \
> > +   ((unsigned long)(base) + 0x2000 + 0x4 * ((uid) - 1))
> 
> why do you need a cast here...

Since the UID registers address is calculated by the UID number. But
I can remove the cast and define the UID global macros.

> 
> > +/* DMA_CHN_INTC register definition */
> > +#define DMA_CHN_INT_MASK   GENMASK(4, 0)
> > +#define DMA_CHN_INT_CLR_OFFSET 24
> 
> Why cant this be expressed in GENMASK?

Since this is not one mask, it is one offset. If we use GENMASK(4, 3)
instead of magic number 24, it is not very clear for the offset bit.

> 
> > +/* DMA_CHN_CFG register definition */
> > +#define DMA_CHN_EN BIT(0)
> > +#define DMA_CHN_PRIORITY_OFFSET12
> > +#define LLIST_EN_OFFSET4
> > +#define CHN_WAIT_BDONE 24
> > +#define DMA_DONOT_WAIT_BDONE   1
> 
> All these too...
> 
> > +static void sprd_dma_set_uid(struct sprd_dma_chn *mchan)
> > +{
> > +   struct sprd_dma_dev *sdev = to_sprd_dma_dev(>chan);
> > +   u32 dev_id = mchan->dev_id;
> > +
> > +   if (dev_id != DMA_SOFTWARE_UID)
> 
> Whats a UID?

It is for users, every user was assigned one unique hardware ID.
Then the user can trigger the DMA to transfer by the user ID.

> 
> > +   writel(mchan->chn_num + 1, (void __iomem *)DMA_GLB_REQ_CID(
> > +  sdev->glb_base, dev_id));
> 
> And again the cast, I dont think we need casts like this...

OK.

> 
> > +static void sprd_dma_clear_int(struct sprd_dma_chn *mchan)
> > +{
> > +   u32 intc = readl(mchan->chn_base + DMA_CHN_INTC);
> > +
> > +   intc |= DMA_CHN_INT_MASK << DMA_CHN_INT_CLR_OFFSET;
> > +   writel(intc, mchan->chn_base + DMA_CHN_INTC);
> 
> How about adding a updatel macro and use that here and few other places.

OK.

> 
> > +static void sprd_dma_stop_and_disable(struct sprd_dma_chn *mchan)
> > +{
> > +   u32 cfg = readl(mchan->chn_base + DMA_CHN_CFG);
> > +   u32 pause, timeout = DMA_CHN_PAUSE_CNT;
> > +
> > +   if (!(cfg & DMA_CHN_EN))
> > +   return;
> > +
> > +   pause = readl(mchan->chn_base + DMA_CHN_PAUSE);
> > +   pause |= DMA_CHN_PAUSE_EN;
> > +   writel(pause, mchan->chn_base + DMA_CHN_PAUSE);
> > +
> > +   do {
> > +   pause = readl(mchan->chn_base + DMA_CHN_PAUSE);
> > +   if (pause & DMA_CHN_PAUSE_STS)
> > +   break;
> > +
> > +   cpu_relax();
> > +   } while (--timeout > 0);
> 
> We should check here if timeout occured and at least log the error

Yes, will add one warning meesage.

> 
> > +static enum dma_int_type sprd_dma_get_int_type(struct sprd_dma_chn *mchan)
> > +{
> > +   u32 intc_reg = readl(mchan->chn_base + DMA_CHN_INTC);
> > +
> > +   if (intc_reg & CFGERR_INT_STS)
> > +   return CONFIG_ERR;
> > +   else if (intc_reg & LLIST_INT_STS)
> > +   return LIST_DONE;
> > +   else if (intc_reg & TRSC_INT_STS)
> > +   return TRANS_DONE;
> > +   else if (intc_reg & BLK_INT_STS)
> > +   return BLK_DONE;
> > +   else if (intc_reg & FRAG_INT_STS)
> > +   return FRAG_DONE;
> 
> this seems a good case for using switch-cases

OK.

> 
> > +static int sprd_dma_chn_start_chn(struct sprd_dma_chn *mchan,
> > + struct sprd_dma_desc *mdesc)
> > +{
> > +   struct sprd_dma_dev *sdev = to_sprd_dma_dev(>chan);
> > +   enum dma_flags flag = mdesc->dma_flags;
> > +   int chn = mchan->chn_num + 1;
> > +   unsigned int cfg_group1, cfg_group2, start_mode = 0;
> > +
> > +   if (!(flag & (DMA_GROUP1_SRC | DMA_GROUP2_SRC | DMA_GROUP1_DST |
> > + DMA_GROUP2_DST)))
> > +   return 0;
> > +
> > +   if (flag & (DMA_GROUP1_SRC | DMA_GROUP2_SRC)) {
> > +   switch (flag & (DMA_MUTL_FRAG_DONE | DMA_MUTL_BLK_DONE |
> > +   DMA_MUTL_TRANS_DONE | DMA_MUTL_LIST_DONE)) {
> > +   case DMA_MUTL_FRAG_DONE:
> > +   start_mode = 0;
> > +   break;
> 
> empty line after each break helps with readability

OK.

> 
> > +/*
> > + * struct sprd_dma_cfg: DMA configuration for users
> > + * @config: salve config structure
> 
> typo

Sorry, will fix it.

> 
> > + * 

Re: [PATCH 2/2] dma: Add Spreadtrum DMA controller driver

[Vinod Koul]

On Tue, Jul 18, 2017 at 03:06:12PM +0800, Baolin Wang wrote:

> +/* DMA global registers definition */
> +#define DMA_GLB_PAUSE0x0
> +#define DMA_GLB_FRAG_WAIT0x4
> +#define DMA_GLB_REQ_PEND0_EN 0x8
> +#define DMA_GLB_REQ_PEND1_EN 0xc
> +#define DMA_GLB_INT_RAW_STS  0x10
> +#define DMA_GLB_INT_MSK_STS  0x14
> +#define DMA_GLB_REQ_STS  0x18
> +#define DMA_GLB_CHN_EN_STS   0x1c
> +#define DMA_GLB_DEBUG_STS0x20
> +#define DMA_GLB_ARB_SEL_STS  0x24
> +#define DMA_GLB_CHN_START_CHN_CFG1   0x28
> +#define DMA_GLB_CHN_START_CHN_CFG2   0x2c
> +#define DMA_CHN_LLIST_OFFSET 0x10
> +#define DMA_GLB_REQ_CID(base, uid)   \
> + ((unsigned long)(base) + 0x2000 + 0x4 * ((uid) - 1))

why do you need a cast here...

> +/* DMA_CHN_INTC register definition */
> +#define DMA_CHN_INT_MASK GENMASK(4, 0)
> +#define DMA_CHN_INT_CLR_OFFSET   24

Why cant this be expressed in GENMASK?

> +/* DMA_CHN_CFG register definition */
> +#define DMA_CHN_EN   BIT(0)
> +#define DMA_CHN_PRIORITY_OFFSET  12
> +#define LLIST_EN_OFFSET  4
> +#define CHN_WAIT_BDONE   24
> +#define DMA_DONOT_WAIT_BDONE 1

All these too...

> +static void sprd_dma_set_uid(struct sprd_dma_chn *mchan)
> +{
> + struct sprd_dma_dev *sdev = to_sprd_dma_dev(>chan);
> + u32 dev_id = mchan->dev_id;
> +
> + if (dev_id != DMA_SOFTWARE_UID)

Whats a UID?

> + writel(mchan->chn_num + 1, (void __iomem *)DMA_GLB_REQ_CID(
> +sdev->glb_base, dev_id));

And again the cast, I dont think we need casts like this...

> +static void sprd_dma_clear_int(struct sprd_dma_chn *mchan)
> +{
> + u32 intc = readl(mchan->chn_base + DMA_CHN_INTC);
> +
> + intc |= DMA_CHN_INT_MASK << DMA_CHN_INT_CLR_OFFSET;
> + writel(intc, mchan->chn_base + DMA_CHN_INTC);

How about adding a updatel macro and use that here and few other places.

> +static void sprd_dma_stop_and_disable(struct sprd_dma_chn *mchan)
> +{
> + u32 cfg = readl(mchan->chn_base + DMA_CHN_CFG);
> + u32 pause, timeout = DMA_CHN_PAUSE_CNT;
> +
> + if (!(cfg & DMA_CHN_EN))
> + return;
> +
> + pause = readl(mchan->chn_base + DMA_CHN_PAUSE);
> + pause |= DMA_CHN_PAUSE_EN;
> + writel(pause, mchan->chn_base + DMA_CHN_PAUSE);
> +
> + do {
> + pause = readl(mchan->chn_base + DMA_CHN_PAUSE);
> + if (pause & DMA_CHN_PAUSE_STS)
> + break;
> +
> + cpu_relax();
> + } while (--timeout > 0);

We should check here if timeout occured and at least log the error

> +static enum dma_int_type sprd_dma_get_int_type(struct sprd_dma_chn *mchan)
> +{
> + u32 intc_reg = readl(mchan->chn_base + DMA_CHN_INTC);
> +
> + if (intc_reg & CFGERR_INT_STS)
> + return CONFIG_ERR;
> + else if (intc_reg & LLIST_INT_STS)
> + return LIST_DONE;
> + else if (intc_reg & TRSC_INT_STS)
> + return TRANS_DONE;
> + else if (intc_reg & BLK_INT_STS)
> + return BLK_DONE;
> + else if (intc_reg & FRAG_INT_STS)
> + return FRAG_DONE;

this seems a good case for using switch-cases

> +static int sprd_dma_chn_start_chn(struct sprd_dma_chn *mchan,
> +   struct sprd_dma_desc *mdesc)
> +{
> + struct sprd_dma_dev *sdev = to_sprd_dma_dev(>chan);
> + enum dma_flags flag = mdesc->dma_flags;
> + int chn = mchan->chn_num + 1;
> + unsigned int cfg_group1, cfg_group2, start_mode = 0;
> +
> + if (!(flag & (DMA_GROUP1_SRC | DMA_GROUP2_SRC | DMA_GROUP1_DST |
> +   DMA_GROUP2_DST)))
> + return 0;
> +
> + if (flag & (DMA_GROUP1_SRC | DMA_GROUP2_SRC)) {
> + switch (flag & (DMA_MUTL_FRAG_DONE | DMA_MUTL_BLK_DONE |
> + DMA_MUTL_TRANS_DONE | DMA_MUTL_LIST_DONE)) {
> + case DMA_MUTL_FRAG_DONE:
> + start_mode = 0;
> + break;

empty line after each break helps with readability

> +/*
> + * struct sprd_dma_cfg: DMA configuration for users
> + * @config: salve config structure

typo

> + * @chn_pri: the channel priority
> + * @datawidth: the data width
> + * @req_mode: the request mode
> + * @irq_mode: the interrupt mode
> + * @swt_mode: the switch mode
> + * @link_cfg_v: point to the virtual memory address to save link-list DMA
> + * configuration
> + * @link_cfg_p: point to the physical memory address to save link-list DMA
> + * configuration
> + * @src_addr: the source address
> + * @des_addr: the destination address
> + * @fragmens_len: one fragment request length
> + * @block_len; one block request length
> + * @transcation_len: one transaction request length
> + * @src_step: source side transfer step
> + * @des_step: destination side transfer step
> + * @src_frag_step: source fragment 

Re: [PATCH 2/2] dma: Add Spreadtrum DMA controller driver

[Vinod Koul]

On Tue, Jul 18, 2017 at 03:06:12PM +0800, Baolin Wang wrote:

> +/* DMA global registers definition */
> +#define DMA_GLB_PAUSE0x0
> +#define DMA_GLB_FRAG_WAIT0x4
> +#define DMA_GLB_REQ_PEND0_EN 0x8
> +#define DMA_GLB_REQ_PEND1_EN 0xc
> +#define DMA_GLB_INT_RAW_STS  0x10
> +#define DMA_GLB_INT_MSK_STS  0x14
> +#define DMA_GLB_REQ_STS  0x18
> +#define DMA_GLB_CHN_EN_STS   0x1c
> +#define DMA_GLB_DEBUG_STS0x20
> +#define DMA_GLB_ARB_SEL_STS  0x24
> +#define DMA_GLB_CHN_START_CHN_CFG1   0x28
> +#define DMA_GLB_CHN_START_CHN_CFG2   0x2c
> +#define DMA_CHN_LLIST_OFFSET 0x10
> +#define DMA_GLB_REQ_CID(base, uid)   \
> + ((unsigned long)(base) + 0x2000 + 0x4 * ((uid) - 1))

why do you need a cast here...

> +/* DMA_CHN_INTC register definition */
> +#define DMA_CHN_INT_MASK GENMASK(4, 0)
> +#define DMA_CHN_INT_CLR_OFFSET   24

Why cant this be expressed in GENMASK?

> +/* DMA_CHN_CFG register definition */
> +#define DMA_CHN_EN   BIT(0)
> +#define DMA_CHN_PRIORITY_OFFSET  12
> +#define LLIST_EN_OFFSET  4
> +#define CHN_WAIT_BDONE   24
> +#define DMA_DONOT_WAIT_BDONE 1

All these too...

> +static void sprd_dma_set_uid(struct sprd_dma_chn *mchan)
> +{
> + struct sprd_dma_dev *sdev = to_sprd_dma_dev(>chan);
> + u32 dev_id = mchan->dev_id;
> +
> + if (dev_id != DMA_SOFTWARE_UID)

Whats a UID?

> + writel(mchan->chn_num + 1, (void __iomem *)DMA_GLB_REQ_CID(
> +sdev->glb_base, dev_id));

And again the cast, I dont think we need casts like this...

> +static void sprd_dma_clear_int(struct sprd_dma_chn *mchan)
> +{
> + u32 intc = readl(mchan->chn_base + DMA_CHN_INTC);
> +
> + intc |= DMA_CHN_INT_MASK << DMA_CHN_INT_CLR_OFFSET;
> + writel(intc, mchan->chn_base + DMA_CHN_INTC);

How about adding a updatel macro and use that here and few other places.

> +static void sprd_dma_stop_and_disable(struct sprd_dma_chn *mchan)
> +{
> + u32 cfg = readl(mchan->chn_base + DMA_CHN_CFG);
> + u32 pause, timeout = DMA_CHN_PAUSE_CNT;
> +
> + if (!(cfg & DMA_CHN_EN))
> + return;
> +
> + pause = readl(mchan->chn_base + DMA_CHN_PAUSE);
> + pause |= DMA_CHN_PAUSE_EN;
> + writel(pause, mchan->chn_base + DMA_CHN_PAUSE);
> +
> + do {
> + pause = readl(mchan->chn_base + DMA_CHN_PAUSE);
> + if (pause & DMA_CHN_PAUSE_STS)
> + break;
> +
> + cpu_relax();
> + } while (--timeout > 0);

We should check here if timeout occured and at least log the error

> +static enum dma_int_type sprd_dma_get_int_type(struct sprd_dma_chn *mchan)
> +{
> + u32 intc_reg = readl(mchan->chn_base + DMA_CHN_INTC);
> +
> + if (intc_reg & CFGERR_INT_STS)
> + return CONFIG_ERR;
> + else if (intc_reg & LLIST_INT_STS)
> + return LIST_DONE;
> + else if (intc_reg & TRSC_INT_STS)
> + return TRANS_DONE;
> + else if (intc_reg & BLK_INT_STS)
> + return BLK_DONE;
> + else if (intc_reg & FRAG_INT_STS)
> + return FRAG_DONE;

this seems a good case for using switch-cases

> +static int sprd_dma_chn_start_chn(struct sprd_dma_chn *mchan,
> +   struct sprd_dma_desc *mdesc)
> +{
> + struct sprd_dma_dev *sdev = to_sprd_dma_dev(>chan);
> + enum dma_flags flag = mdesc->dma_flags;
> + int chn = mchan->chn_num + 1;
> + unsigned int cfg_group1, cfg_group2, start_mode = 0;
> +
> + if (!(flag & (DMA_GROUP1_SRC | DMA_GROUP2_SRC | DMA_GROUP1_DST |
> +   DMA_GROUP2_DST)))
> + return 0;
> +
> + if (flag & (DMA_GROUP1_SRC | DMA_GROUP2_SRC)) {
> + switch (flag & (DMA_MUTL_FRAG_DONE | DMA_MUTL_BLK_DONE |
> + DMA_MUTL_TRANS_DONE | DMA_MUTL_LIST_DONE)) {
> + case DMA_MUTL_FRAG_DONE:
> + start_mode = 0;
> + break;

empty line after each break helps with readability

> +/*
> + * struct sprd_dma_cfg: DMA configuration for users
> + * @config: salve config structure

typo

> + * @chn_pri: the channel priority
> + * @datawidth: the data width
> + * @req_mode: the request mode
> + * @irq_mode: the interrupt mode
> + * @swt_mode: the switch mode
> + * @link_cfg_v: point to the virtual memory address to save link-list DMA
> + * configuration
> + * @link_cfg_p: point to the physical memory address to save link-list DMA
> + * configuration
> + * @src_addr: the source address
> + * @des_addr: the destination address
> + * @fragmens_len: one fragment request length
> + * @block_len; one block request length
> + * @transcation_len: one transaction request length
> + * @src_step: source side transfer step
> + * @des_step: destination side transfer step
> + * @src_frag_step: source fragment 

Re: [PATCH 2/2] dma: Add Spreadtrum DMA controller driver

[Baolin Wang]

Hi Chunyan,

On 21 July 2017 at 10:50, Chunyan Zhang  wrote:
> Hi Baolin,
>
> On 18 July 2017 at 15:06, Baolin Wang  wrote:
>> This patch adds the DMA controller driver for Spreadtrum SC9860 platform.
>
> I guess this driver is not only for SC9860, instead it should work for
> all most all Spreadtrum's platforms?

No, now this driver is only for SC9860 platform. If we make this
driver work on other Spreadtrum platform, we should submit new patches
with modifing DT binding files and compatible string.

>
>>
>> Signed-off-by: Baolin Wang 
>> ---
>>  drivers/dma/Kconfig  |7 +
>>  drivers/dma/Makefile |1 +
>>  drivers/dma/sprd-dma.c   | 1451 
>> ++
>>  include/linux/dma/sprd-dma.h |  270 
>>  4 files changed, 1729 insertions(+)
>>  create mode 100644 drivers/dma/sprd-dma.c
>>  create mode 100644 include/linux/dma/sprd-dma.h
>>
>> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
>> index fa8f9c0..961f6ea 100644
>> --- a/drivers/dma/Kconfig
>> +++ b/drivers/dma/Kconfig
>> @@ -477,6 +477,13 @@ config STM32_DMA
>>   If you have a board based on such a MCU and wish to use DMA say Y
>>   here.
>>
>> +config SPRD_DMA
>> +   bool "Spreadtrum DMA support"
>> +   depends on ARCH_SPRD
>> +   select DMA_ENGINE
>> +   help
>> + Enable support for the on-chip DMA controller on Spreadtrum 
>> platform.
>> +
>>  config S3C24XX_DMAC
>> bool "Samsung S3C24XX DMA support"
>> depends on ARCH_S3C24XX || COMPILE_TEST
>> diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile
>> index d12ab29..0fee561 100644
>> --- a/drivers/dma/Makefile
>> +++ b/drivers/dma/Makefile
>> @@ -58,6 +58,7 @@ obj-$(CONFIG_RENESAS_DMA) += sh/
>>  obj-$(CONFIG_SIRF_DMA) += sirf-dma.o
>>  obj-$(CONFIG_STE_DMA40) += ste_dma40.o ste_dma40_ll.o
>>  obj-$(CONFIG_STM32_DMA) += stm32-dma.o
>> +obj-$(CONFIG_SPRD_DMA) += sprd-dma.o
>>  obj-$(CONFIG_S3C24XX_DMAC) += s3c24xx-dma.o
>>  obj-$(CONFIG_TXX9_DMAC) += txx9dmac.o
>>  obj-$(CONFIG_TEGRA20_APB_DMA) += tegra20-apb-dma.o
>> diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
>> new file mode 100644
>> index 000..64eaef7
>> --- /dev/null
>> +++ b/drivers/dma/sprd-dma.c
>> @@ -0,0 +1,1451 @@
>> +/*
>> + * Copyright (C) 2017 Spreadtrum Communications Inc.
>> + *
>> + * This software is licensed under the terms of the GNU General Public
>> + * License version 2, as published by the Free Software Foundation, and
>> + * may be copied, distributed, and modified under those terms.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>
> In order to keep consistent with other Spreadtrum's drivers/ DT files
> that had been upstreamed, I suggest to use SPDX-License-Identifier
> tag.

Since I saw other new drivers still use this license and I am not sure
we must change to SPDX-License-Identifier tag. But it is fine for me
to change the license tag.

-- 
Baolin.wang
Best Regards

Re: [PATCH 2/2] dma: Add Spreadtrum DMA controller driver

[Baolin Wang]

Hi Chunyan,

On 21 July 2017 at 10:50, Chunyan Zhang  wrote:
> Hi Baolin,
>
> On 18 July 2017 at 15:06, Baolin Wang  wrote:
>> This patch adds the DMA controller driver for Spreadtrum SC9860 platform.
>
> I guess this driver is not only for SC9860, instead it should work for
> all most all Spreadtrum's platforms?

No, now this driver is only for SC9860 platform. If we make this
driver work on other Spreadtrum platform, we should submit new patches
with modifing DT binding files and compatible string.

>
>>
>> Signed-off-by: Baolin Wang 
>> ---
>>  drivers/dma/Kconfig  |7 +
>>  drivers/dma/Makefile |1 +
>>  drivers/dma/sprd-dma.c   | 1451 
>> ++
>>  include/linux/dma/sprd-dma.h |  270 
>>  4 files changed, 1729 insertions(+)
>>  create mode 100644 drivers/dma/sprd-dma.c
>>  create mode 100644 include/linux/dma/sprd-dma.h
>>
>> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
>> index fa8f9c0..961f6ea 100644
>> --- a/drivers/dma/Kconfig
>> +++ b/drivers/dma/Kconfig
>> @@ -477,6 +477,13 @@ config STM32_DMA
>>   If you have a board based on such a MCU and wish to use DMA say Y
>>   here.
>>
>> +config SPRD_DMA
>> +   bool "Spreadtrum DMA support"
>> +   depends on ARCH_SPRD
>> +   select DMA_ENGINE
>> +   help
>> + Enable support for the on-chip DMA controller on Spreadtrum 
>> platform.
>> +
>>  config S3C24XX_DMAC
>> bool "Samsung S3C24XX DMA support"
>> depends on ARCH_S3C24XX || COMPILE_TEST
>> diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile
>> index d12ab29..0fee561 100644
>> --- a/drivers/dma/Makefile
>> +++ b/drivers/dma/Makefile
>> @@ -58,6 +58,7 @@ obj-$(CONFIG_RENESAS_DMA) += sh/
>>  obj-$(CONFIG_SIRF_DMA) += sirf-dma.o
>>  obj-$(CONFIG_STE_DMA40) += ste_dma40.o ste_dma40_ll.o
>>  obj-$(CONFIG_STM32_DMA) += stm32-dma.o
>> +obj-$(CONFIG_SPRD_DMA) += sprd-dma.o
>>  obj-$(CONFIG_S3C24XX_DMAC) += s3c24xx-dma.o
>>  obj-$(CONFIG_TXX9_DMAC) += txx9dmac.o
>>  obj-$(CONFIG_TEGRA20_APB_DMA) += tegra20-apb-dma.o
>> diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
>> new file mode 100644
>> index 000..64eaef7
>> --- /dev/null
>> +++ b/drivers/dma/sprd-dma.c
>> @@ -0,0 +1,1451 @@
>> +/*
>> + * Copyright (C) 2017 Spreadtrum Communications Inc.
>> + *
>> + * This software is licensed under the terms of the GNU General Public
>> + * License version 2, as published by the Free Software Foundation, and
>> + * may be copied, distributed, and modified under those terms.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>
> In order to keep consistent with other Spreadtrum's drivers/ DT files
> that had been upstreamed, I suggest to use SPDX-License-Identifier
> tag.

Since I saw other new drivers still use this license and I am not sure
we must change to SPDX-License-Identifier tag. But it is fine for me
to change the license tag.

-- 
Baolin.wang
Best Regards

Re: [PATCH 2/2] dma: Add Spreadtrum DMA controller driver

[Chunyan Zhang]

Hi Baolin,

On 18 July 2017 at 15:06, Baolin Wang  wrote:
> This patch adds the DMA controller driver for Spreadtrum SC9860 platform.

I guess this driver is not only for SC9860, instead it should work for
all most all Spreadtrum's platforms?

>
> Signed-off-by: Baolin Wang 
> ---
>  drivers/dma/Kconfig  |7 +
>  drivers/dma/Makefile |1 +
>  drivers/dma/sprd-dma.c   | 1451 
> ++
>  include/linux/dma/sprd-dma.h |  270 
>  4 files changed, 1729 insertions(+)
>  create mode 100644 drivers/dma/sprd-dma.c
>  create mode 100644 include/linux/dma/sprd-dma.h
>
> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
> index fa8f9c0..961f6ea 100644
> --- a/drivers/dma/Kconfig
> +++ b/drivers/dma/Kconfig
> @@ -477,6 +477,13 @@ config STM32_DMA
>   If you have a board based on such a MCU and wish to use DMA say Y
>   here.
>
> +config SPRD_DMA
> +   bool "Spreadtrum DMA support"
> +   depends on ARCH_SPRD
> +   select DMA_ENGINE
> +   help
> + Enable support for the on-chip DMA controller on Spreadtrum 
> platform.
> +
>  config S3C24XX_DMAC
> bool "Samsung S3C24XX DMA support"
> depends on ARCH_S3C24XX || COMPILE_TEST
> diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile
> index d12ab29..0fee561 100644
> --- a/drivers/dma/Makefile
> +++ b/drivers/dma/Makefile
> @@ -58,6 +58,7 @@ obj-$(CONFIG_RENESAS_DMA) += sh/
>  obj-$(CONFIG_SIRF_DMA) += sirf-dma.o
>  obj-$(CONFIG_STE_DMA40) += ste_dma40.o ste_dma40_ll.o
>  obj-$(CONFIG_STM32_DMA) += stm32-dma.o
> +obj-$(CONFIG_SPRD_DMA) += sprd-dma.o
>  obj-$(CONFIG_S3C24XX_DMAC) += s3c24xx-dma.o
>  obj-$(CONFIG_TXX9_DMAC) += txx9dmac.o
>  obj-$(CONFIG_TEGRA20_APB_DMA) += tegra20-apb-dma.o
> diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> new file mode 100644
> index 000..64eaef7
> --- /dev/null
> +++ b/drivers/dma/sprd-dma.c
> @@ -0,0 +1,1451 @@
> +/*
> + * Copyright (C) 2017 Spreadtrum Communications Inc.
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +

In order to keep consistent with other Spreadtrum's drivers/ DT files
that had been upstreamed, I suggest to use SPDX-License-Identifier
tag.

Thanks,
Chunyan

> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "dmaengine.h"
> +
> +#define SPRD_DMA_DESCRIPTORS   16
> +#define SPRD_DMA_CFG_COUNT 32
> +#define SPRD_DMA_CHN_REG_OFFSET0x1000
> +#define SPRD_DMA_CHN_REG_LENGTH0x40
> +#define SPRD_DMA_MEMCPY_MIN_SIZE   64
> +
> +/* DMA global registers definition */
> +#define DMA_GLB_PAUSE  0x0
> +#define DMA_GLB_FRAG_WAIT  0x4
> +#define DMA_GLB_REQ_PEND0_EN   0x8
> +#define DMA_GLB_REQ_PEND1_EN   0xc
> +#define DMA_GLB_INT_RAW_STS0x10
> +#define DMA_GLB_INT_MSK_STS0x14
> +#define DMA_GLB_REQ_STS0x18
> +#define DMA_GLB_CHN_EN_STS 0x1c
> +#define DMA_GLB_DEBUG_STS  0x20
> +#define DMA_GLB_ARB_SEL_STS0x24
> +#define DMA_GLB_CHN_START_CHN_CFG1 0x28
> +#define DMA_GLB_CHN_START_CHN_CFG2 0x2c
> +#define DMA_CHN_LLIST_OFFSET   0x10
> +#define DMA_GLB_REQ_CID(base, uid) \
> +   ((unsigned long)(base) + 0x2000 + 0x4 * ((uid) - 1))
> +
> +/* DMA_GLB_CHN_START_CHN_CFG register definition */
> +#define SRC_CHN_OFFSET 0
> +#define DST_CHN_OFFSET 8
> +#define START_MODE_OFFSET  16
> +#define CHN_START_CHN  BIT(24)
> +
> +/* DMA channel registers definition */
> +#define DMA_CHN_PAUSE  0x0
> +#define DMA_CHN_REQ0x4
> +#define DMA_CHN_CFG0x8
> +#define DMA_CHN_INTC   0xc
> +#define DMA_CHN_SRC_ADDR   0x10
> +#define DMA_CHN_DES_ADDR   0x14
> +#define DMA_CHN_FRG_LEN0x18
> +#define DMA_CHN_BLK_LEN0x1c
> +#define DMA_CHN_TRSC_LEN   0x20
> +#define DMA_CHN_TRSF_STEP  0x24
> +#define DMA_CHN_WARP_PTR   0x28
> +#define DMA_CHN_WARP_TO0x2c
> +#define DMA_CHN_LLIST_PTR  0x30
> +#define DMA_CHN_FRAG_STEP  0x34
> +#define DMA_CHN_SRC_BLK_STEP   0x38
> +#define 

Re: [PATCH 2/2] dma: Add Spreadtrum DMA controller driver

[Chunyan Zhang]

Hi Baolin,

On 18 July 2017 at 15:06, Baolin Wang  wrote:
> This patch adds the DMA controller driver for Spreadtrum SC9860 platform.

I guess this driver is not only for SC9860, instead it should work for
all most all Spreadtrum's platforms?

>
> Signed-off-by: Baolin Wang 
> ---
>  drivers/dma/Kconfig  |7 +
>  drivers/dma/Makefile |1 +
>  drivers/dma/sprd-dma.c   | 1451 
> ++
>  include/linux/dma/sprd-dma.h |  270 
>  4 files changed, 1729 insertions(+)
>  create mode 100644 drivers/dma/sprd-dma.c
>  create mode 100644 include/linux/dma/sprd-dma.h
>
> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
> index fa8f9c0..961f6ea 100644
> --- a/drivers/dma/Kconfig
> +++ b/drivers/dma/Kconfig
> @@ -477,6 +477,13 @@ config STM32_DMA
>   If you have a board based on such a MCU and wish to use DMA say Y
>   here.
>
> +config SPRD_DMA
> +   bool "Spreadtrum DMA support"
> +   depends on ARCH_SPRD
> +   select DMA_ENGINE
> +   help
> + Enable support for the on-chip DMA controller on Spreadtrum 
> platform.
> +
>  config S3C24XX_DMAC
> bool "Samsung S3C24XX DMA support"
> depends on ARCH_S3C24XX || COMPILE_TEST
> diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile
> index d12ab29..0fee561 100644
> --- a/drivers/dma/Makefile
> +++ b/drivers/dma/Makefile
> @@ -58,6 +58,7 @@ obj-$(CONFIG_RENESAS_DMA) += sh/
>  obj-$(CONFIG_SIRF_DMA) += sirf-dma.o
>  obj-$(CONFIG_STE_DMA40) += ste_dma40.o ste_dma40_ll.o
>  obj-$(CONFIG_STM32_DMA) += stm32-dma.o
> +obj-$(CONFIG_SPRD_DMA) += sprd-dma.o
>  obj-$(CONFIG_S3C24XX_DMAC) += s3c24xx-dma.o
>  obj-$(CONFIG_TXX9_DMAC) += txx9dmac.o
>  obj-$(CONFIG_TEGRA20_APB_DMA) += tegra20-apb-dma.o
> diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> new file mode 100644
> index 000..64eaef7
> --- /dev/null
> +++ b/drivers/dma/sprd-dma.c
> @@ -0,0 +1,1451 @@
> +/*
> + * Copyright (C) 2017 Spreadtrum Communications Inc.
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +

In order to keep consistent with other Spreadtrum's drivers/ DT files
that had been upstreamed, I suggest to use SPDX-License-Identifier
tag.

Thanks,
Chunyan

> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "dmaengine.h"
> +
> +#define SPRD_DMA_DESCRIPTORS   16
> +#define SPRD_DMA_CFG_COUNT 32
> +#define SPRD_DMA_CHN_REG_OFFSET0x1000
> +#define SPRD_DMA_CHN_REG_LENGTH0x40
> +#define SPRD_DMA_MEMCPY_MIN_SIZE   64
> +
> +/* DMA global registers definition */
> +#define DMA_GLB_PAUSE  0x0
> +#define DMA_GLB_FRAG_WAIT  0x4
> +#define DMA_GLB_REQ_PEND0_EN   0x8
> +#define DMA_GLB_REQ_PEND1_EN   0xc
> +#define DMA_GLB_INT_RAW_STS0x10
> +#define DMA_GLB_INT_MSK_STS0x14
> +#define DMA_GLB_REQ_STS0x18
> +#define DMA_GLB_CHN_EN_STS 0x1c
> +#define DMA_GLB_DEBUG_STS  0x20
> +#define DMA_GLB_ARB_SEL_STS0x24
> +#define DMA_GLB_CHN_START_CHN_CFG1 0x28
> +#define DMA_GLB_CHN_START_CHN_CFG2 0x2c
> +#define DMA_CHN_LLIST_OFFSET   0x10
> +#define DMA_GLB_REQ_CID(base, uid) \
> +   ((unsigned long)(base) + 0x2000 + 0x4 * ((uid) - 1))
> +
> +/* DMA_GLB_CHN_START_CHN_CFG register definition */
> +#define SRC_CHN_OFFSET 0
> +#define DST_CHN_OFFSET 8
> +#define START_MODE_OFFSET  16
> +#define CHN_START_CHN  BIT(24)
> +
> +/* DMA channel registers definition */
> +#define DMA_CHN_PAUSE  0x0
> +#define DMA_CHN_REQ0x4
> +#define DMA_CHN_CFG0x8
> +#define DMA_CHN_INTC   0xc
> +#define DMA_CHN_SRC_ADDR   0x10
> +#define DMA_CHN_DES_ADDR   0x14
> +#define DMA_CHN_FRG_LEN0x18
> +#define DMA_CHN_BLK_LEN0x1c
> +#define DMA_CHN_TRSC_LEN   0x20
> +#define DMA_CHN_TRSF_STEP  0x24
> +#define DMA_CHN_WARP_PTR   0x28
> +#define DMA_CHN_WARP_TO0x2c
> +#define DMA_CHN_LLIST_PTR  0x30
> +#define DMA_CHN_FRAG_STEP  0x34
> +#define DMA_CHN_SRC_BLK_STEP   0x38
> +#define DMA_CHN_DES_BLK_STEP   0x3c
> +
> +/* DMA_CHN_INTC 

[PATCH 2/2] dma: Add Spreadtrum DMA controller driver

[Baolin Wang]

This patch adds the DMA controller driver for Spreadtrum SC9860 platform.

Signed-off-by: Baolin Wang 
---
 drivers/dma/Kconfig  |7 +
 drivers/dma/Makefile |1 +
 drivers/dma/sprd-dma.c   | 1451 ++
 include/linux/dma/sprd-dma.h |  270 
 4 files changed, 1729 insertions(+)
 create mode 100644 drivers/dma/sprd-dma.c
 create mode 100644 include/linux/dma/sprd-dma.h

diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
index fa8f9c0..961f6ea 100644
--- a/drivers/dma/Kconfig
+++ b/drivers/dma/Kconfig
@@ -477,6 +477,13 @@ config STM32_DMA
  If you have a board based on such a MCU and wish to use DMA say Y
  here.
 
+config SPRD_DMA
+   bool "Spreadtrum DMA support"
+   depends on ARCH_SPRD
+   select DMA_ENGINE
+   help
+ Enable support for the on-chip DMA controller on Spreadtrum platform.
+
 config S3C24XX_DMAC
bool "Samsung S3C24XX DMA support"
depends on ARCH_S3C24XX || COMPILE_TEST
diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile
index d12ab29..0fee561 100644
--- a/drivers/dma/Makefile
+++ b/drivers/dma/Makefile
@@ -58,6 +58,7 @@ obj-$(CONFIG_RENESAS_DMA) += sh/
 obj-$(CONFIG_SIRF_DMA) += sirf-dma.o
 obj-$(CONFIG_STE_DMA40) += ste_dma40.o ste_dma40_ll.o
 obj-$(CONFIG_STM32_DMA) += stm32-dma.o
+obj-$(CONFIG_SPRD_DMA) += sprd-dma.o
 obj-$(CONFIG_S3C24XX_DMAC) += s3c24xx-dma.o
 obj-$(CONFIG_TXX9_DMAC) += txx9dmac.o
 obj-$(CONFIG_TEGRA20_APB_DMA) += tegra20-apb-dma.o
diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
new file mode 100644
index 000..64eaef7
--- /dev/null
+++ b/drivers/dma/sprd-dma.c
@@ -0,0 +1,1451 @@
+/*
+ * Copyright (C) 2017 Spreadtrum Communications Inc.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "dmaengine.h"
+
+#define SPRD_DMA_DESCRIPTORS   16
+#define SPRD_DMA_CFG_COUNT 32
+#define SPRD_DMA_CHN_REG_OFFSET0x1000
+#define SPRD_DMA_CHN_REG_LENGTH0x40
+#define SPRD_DMA_MEMCPY_MIN_SIZE   64
+
+/* DMA global registers definition */
+#define DMA_GLB_PAUSE  0x0
+#define DMA_GLB_FRAG_WAIT  0x4
+#define DMA_GLB_REQ_PEND0_EN   0x8
+#define DMA_GLB_REQ_PEND1_EN   0xc
+#define DMA_GLB_INT_RAW_STS0x10
+#define DMA_GLB_INT_MSK_STS0x14
+#define DMA_GLB_REQ_STS0x18
+#define DMA_GLB_CHN_EN_STS 0x1c
+#define DMA_GLB_DEBUG_STS  0x20
+#define DMA_GLB_ARB_SEL_STS0x24
+#define DMA_GLB_CHN_START_CHN_CFG1 0x28
+#define DMA_GLB_CHN_START_CHN_CFG2 0x2c
+#define DMA_CHN_LLIST_OFFSET   0x10
+#define DMA_GLB_REQ_CID(base, uid) \
+   ((unsigned long)(base) + 0x2000 + 0x4 * ((uid) - 1))
+
+/* DMA_GLB_CHN_START_CHN_CFG register definition */
+#define SRC_CHN_OFFSET 0
+#define DST_CHN_OFFSET 8
+#define START_MODE_OFFSET  16
+#define CHN_START_CHN  BIT(24)
+
+/* DMA channel registers definition */
+#define DMA_CHN_PAUSE  0x0
+#define DMA_CHN_REQ0x4
+#define DMA_CHN_CFG0x8
+#define DMA_CHN_INTC   0xc
+#define DMA_CHN_SRC_ADDR   0x10
+#define DMA_CHN_DES_ADDR   0x14
+#define DMA_CHN_FRG_LEN0x18
+#define DMA_CHN_BLK_LEN0x1c
+#define DMA_CHN_TRSC_LEN   0x20
+#define DMA_CHN_TRSF_STEP  0x24
+#define DMA_CHN_WARP_PTR   0x28
+#define DMA_CHN_WARP_TO0x2c
+#define DMA_CHN_LLIST_PTR  0x30
+#define DMA_CHN_FRAG_STEP  0x34
+#define DMA_CHN_SRC_BLK_STEP   0x38
+#define DMA_CHN_DES_BLK_STEP   0x3c
+
+/* DMA_CHN_INTC register definition */
+#define DMA_CHN_INT_MASK   GENMASK(4, 0)
+#define DMA_CHN_INT_CLR_OFFSET 24
+#define FRAG_INT_ENBIT(0)
+#define BLK_INT_EN BIT(1)
+#define TRANS_INT_EN   BIT(2)
+#define LIST_INT_ENBIT(3)
+#define CFG_ERROR_INT_EN   BIT(4)
+
+/* DMA_CHN_CFG register definition */
+#define DMA_CHN_EN BIT(0)
+#define DMA_CHN_PRIORITY_OFFSET12
+#define LLIST_EN_OFFSET4
+#define 

[PATCH 2/2] dma: Add Spreadtrum DMA controller driver

[Baolin Wang]

This patch adds the DMA controller driver for Spreadtrum SC9860 platform.

Signed-off-by: Baolin Wang 
---
 drivers/dma/Kconfig  |7 +
 drivers/dma/Makefile |1 +
 drivers/dma/sprd-dma.c   | 1451 ++
 include/linux/dma/sprd-dma.h |  270 
 4 files changed, 1729 insertions(+)
 create mode 100644 drivers/dma/sprd-dma.c
 create mode 100644 include/linux/dma/sprd-dma.h

diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
index fa8f9c0..961f6ea 100644
--- a/drivers/dma/Kconfig
+++ b/drivers/dma/Kconfig
@@ -477,6 +477,13 @@ config STM32_DMA
  If you have a board based on such a MCU and wish to use DMA say Y
  here.
 
+config SPRD_DMA
+   bool "Spreadtrum DMA support"
+   depends on ARCH_SPRD
+   select DMA_ENGINE
+   help
+ Enable support for the on-chip DMA controller on Spreadtrum platform.
+
 config S3C24XX_DMAC
bool "Samsung S3C24XX DMA support"
depends on ARCH_S3C24XX || COMPILE_TEST
diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile
index d12ab29..0fee561 100644
--- a/drivers/dma/Makefile
+++ b/drivers/dma/Makefile
@@ -58,6 +58,7 @@ obj-$(CONFIG_RENESAS_DMA) += sh/
 obj-$(CONFIG_SIRF_DMA) += sirf-dma.o
 obj-$(CONFIG_STE_DMA40) += ste_dma40.o ste_dma40_ll.o
 obj-$(CONFIG_STM32_DMA) += stm32-dma.o
+obj-$(CONFIG_SPRD_DMA) += sprd-dma.o
 obj-$(CONFIG_S3C24XX_DMAC) += s3c24xx-dma.o
 obj-$(CONFIG_TXX9_DMAC) += txx9dmac.o
 obj-$(CONFIG_TEGRA20_APB_DMA) += tegra20-apb-dma.o
diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
new file mode 100644
index 000..64eaef7
--- /dev/null
+++ b/drivers/dma/sprd-dma.c
@@ -0,0 +1,1451 @@
+/*
+ * Copyright (C) 2017 Spreadtrum Communications Inc.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "dmaengine.h"
+
+#define SPRD_DMA_DESCRIPTORS   16
+#define SPRD_DMA_CFG_COUNT 32
+#define SPRD_DMA_CHN_REG_OFFSET0x1000
+#define SPRD_DMA_CHN_REG_LENGTH0x40
+#define SPRD_DMA_MEMCPY_MIN_SIZE   64
+
+/* DMA global registers definition */
+#define DMA_GLB_PAUSE  0x0
+#define DMA_GLB_FRAG_WAIT  0x4
+#define DMA_GLB_REQ_PEND0_EN   0x8
+#define DMA_GLB_REQ_PEND1_EN   0xc
+#define DMA_GLB_INT_RAW_STS0x10
+#define DMA_GLB_INT_MSK_STS0x14
+#define DMA_GLB_REQ_STS0x18
+#define DMA_GLB_CHN_EN_STS 0x1c
+#define DMA_GLB_DEBUG_STS  0x20
+#define DMA_GLB_ARB_SEL_STS0x24
+#define DMA_GLB_CHN_START_CHN_CFG1 0x28
+#define DMA_GLB_CHN_START_CHN_CFG2 0x2c
+#define DMA_CHN_LLIST_OFFSET   0x10
+#define DMA_GLB_REQ_CID(base, uid) \
+   ((unsigned long)(base) + 0x2000 + 0x4 * ((uid) - 1))
+
+/* DMA_GLB_CHN_START_CHN_CFG register definition */
+#define SRC_CHN_OFFSET 0
+#define DST_CHN_OFFSET 8
+#define START_MODE_OFFSET  16
+#define CHN_START_CHN  BIT(24)
+
+/* DMA channel registers definition */
+#define DMA_CHN_PAUSE  0x0
+#define DMA_CHN_REQ0x4
+#define DMA_CHN_CFG0x8
+#define DMA_CHN_INTC   0xc
+#define DMA_CHN_SRC_ADDR   0x10
+#define DMA_CHN_DES_ADDR   0x14
+#define DMA_CHN_FRG_LEN0x18
+#define DMA_CHN_BLK_LEN0x1c
+#define DMA_CHN_TRSC_LEN   0x20
+#define DMA_CHN_TRSF_STEP  0x24
+#define DMA_CHN_WARP_PTR   0x28
+#define DMA_CHN_WARP_TO0x2c
+#define DMA_CHN_LLIST_PTR  0x30
+#define DMA_CHN_FRAG_STEP  0x34
+#define DMA_CHN_SRC_BLK_STEP   0x38
+#define DMA_CHN_DES_BLK_STEP   0x3c
+
+/* DMA_CHN_INTC register definition */
+#define DMA_CHN_INT_MASK   GENMASK(4, 0)
+#define DMA_CHN_INT_CLR_OFFSET 24
+#define FRAG_INT_ENBIT(0)
+#define BLK_INT_EN BIT(1)
+#define TRANS_INT_EN   BIT(2)
+#define LIST_INT_ENBIT(3)
+#define CFG_ERROR_INT_EN   BIT(4)
+
+/* DMA_CHN_CFG register definition */
+#define DMA_CHN_EN BIT(0)
+#define DMA_CHN_PRIORITY_OFFSET12
+#define LLIST_EN_OFFSET4
+#define CHN_WAIT_BDONE