[PATCH 0/7] Fix some bugs and add new feature for Spreadtrum DMA engine

[PATCH 0/7] Fix some bugs and add new feature for Spreadtrum DMA engine

[Baolin Wang]

Hi,

This patch set fixes some DMA engine bugs and adds interrupt support
for 2-stage transfer.

Baolin Wang (3):
  dmaengine: sprd: Fix the possible crash when getting engine status
  dmaengine: sprd: Add validation of current descriptor in irq handler
  dmaengine: sprd: Add interrupt support for 2-stage transfer

Eric Long (4):
  dmaengine: sprd: Fix the incorrect start for 2-stage destination
    channels
  dmaengine: sprd: Add device validation to support multiple
    controllers
  dmaengine: sprd: Fix block length overflow
  dmaengine: sprd: Fix the right place to configure 2-stage transfer

 drivers/dma/sprd-dma.c |   54 ++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 43 insertions(+), 11 deletions(-)

-- 
1.7.9.5

[PATCH 1/7] dmaengine: sprd: Fix the possible crash when getting engine status

[Baolin Wang]

We will get a NULL virtual descriptor by vchan_find_desc() when the descriptor
has been submitted, that will crash the kernel when getting the engine status.

In this case, since the descriptor has been submitted, which means the pointer
'schan->cur_desc' will point to the current descriptor, then we can use
'schan->cur_desc' to get the engine status to avoid this issue.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 drivers/dma/sprd-dma.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
index 48431e2..e29342a 100644
--- a/drivers/dma/sprd-dma.c
+++ b/drivers/dma/sprd-dma.c
@@ -625,7 +625,7 @@ static enum dma_status sprd_dma_tx_status(struct dma_chan *chan,
 		else
 			pos = 0;
 	} else if (schan->cur_desc && schan->cur_desc->vd.tx.cookie == cookie) {
-		struct sprd_dma_desc *sdesc = to_sprd_dma_desc(vd);
+		struct sprd_dma_desc *sdesc = schan->cur_desc;
 
 		if (sdesc->dir == DMA_DEV_TO_MEM)
 			pos = sprd_dma_get_dst_addr(schan);
-- 
1.7.9.5

Re: [PATCH 1/7] dmaengine: sprd: Fix the possible crash when getting engine status

[Vinod Koul]

On 15-04-19, 20:14, Baolin Wang wrote:
> We will get a NULL virtual descriptor by vchan_find_desc() when the descriptor
> has been submitted, that will crash the kernel when getting the engine status.

No that is wrong, status is for descriptor and not engine!

> In this case, since the descriptor has been submitted, which means the pointer
> 'schan->cur_desc' will point to the current descriptor, then we can use
> 'schan->cur_desc' to get the engine status to avoid this issue.

Nope, since the descriptor is completed, you return with residue as 0
and DMA_COMPLETE status!

> 
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
>  drivers/dma/sprd-dma.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> index 48431e2..e29342a 100644
> --- a/drivers/dma/sprd-dma.c
> +++ b/drivers/dma/sprd-dma.c
> @@ -625,7 +625,7 @@ static enum dma_status sprd_dma_tx_status(struct dma_chan *chan,
>  		else
>  			pos = 0;
>  	} else if (schan->cur_desc && schan->cur_desc->vd.tx.cookie == cookie) {
> -		struct sprd_dma_desc *sdesc = to_sprd_dma_desc(vd);
> +		struct sprd_dma_desc *sdesc = schan->cur_desc;
>  
>  		if (sdesc->dir == DMA_DEV_TO_MEM)
>  			pos = sprd_dma_get_dst_addr(schan);
> -- 
> 1.7.9.5

-- 
~Vinod

Re: [PATCH 1/7] dmaengine: sprd: Fix the possible crash when getting engine status

[Baolin Wang]

Hi Vinod,

On Mon, 29 Apr 2019 at 19:36, Vinod Koul <vkoul@kernel.org> wrote:
>
> On 15-04-19, 20:14, Baolin Wang wrote:
> > We will get a NULL virtual descriptor by vchan_find_desc() when the descriptor
> > has been submitted, that will crash the kernel when getting the engine status.
>
> No that is wrong, status is for descriptor and not engine!

Sure, will fix the commit message.

>
> > In this case, since the descriptor has been submitted, which means the pointer
> > 'schan->cur_desc' will point to the current descriptor, then we can use
> > 'schan->cur_desc' to get the engine status to avoid this issue.
>
> Nope, since the descriptor is completed, you return with residue as 0
> and DMA_COMPLETE status!

No, the descriptor is not completed now. If it is completed, we will
return 0 with DMA_COMPLETE status. But now the descriptor is on
progress, we should get the descriptor to return current residue.
Sorry for confusing description.

>
> >
> > Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> > ---
> >  drivers/dma/sprd-dma.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> > index 48431e2..e29342a 100644
> > --- a/drivers/dma/sprd-dma.c
> > +++ b/drivers/dma/sprd-dma.c
> > @@ -625,7 +625,7 @@ static enum dma_status sprd_dma_tx_status(struct dma_chan *chan,
> >               else
> >                       pos = 0;
> >       } else if (schan->cur_desc && schan->cur_desc->vd.tx.cookie == cookie) {
> > -             struct sprd_dma_desc *sdesc = to_sprd_dma_desc(vd);
> > +             struct sprd_dma_desc *sdesc = schan->cur_desc;
> >
> >               if (sdesc->dir == DMA_DEV_TO_MEM)
> >                       pos = sprd_dma_get_dst_addr(schan);
> > --
> > 1.7.9.5
>
> --
> ~Vinod



-- 
Baolin Wang
Best Regards

Re: [PATCH 1/7] dmaengine: sprd: Fix the possible crash when getting engine status

[Vinod Koul]

On 29-04-19, 19:49, Baolin Wang wrote:
> Hi Vinod,
> 
> On Mon, 29 Apr 2019 at 19:36, Vinod Koul <vkoul@kernel.org> wrote:
> >
> > On 15-04-19, 20:14, Baolin Wang wrote:
> > > We will get a NULL virtual descriptor by vchan_find_desc() when the descriptor
> > > has been submitted, that will crash the kernel when getting the engine status.
> >
> > No that is wrong, status is for descriptor and not engine!
> 
> Sure, will fix the commit message.
> 
> >
> > > In this case, since the descriptor has been submitted, which means the pointer
> > > 'schan->cur_desc' will point to the current descriptor, then we can use
> > > 'schan->cur_desc' to get the engine status to avoid this issue.
> >
> > Nope, since the descriptor is completed, you return with residue as 0
> > and DMA_COMPLETE status!
> 
> No, the descriptor is not completed now. If it is completed, we will
> return 0 with DMA_COMPLETE status. But now the descriptor is on
> progress, we should get the descriptor to return current residue.
> Sorry for confusing description.

OKay will wait for updated description to understand the fix

> 
> >
> > >
> > > Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> > > ---
> > >  drivers/dma/sprd-dma.c |    2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> > > index 48431e2..e29342a 100644
> > > --- a/drivers/dma/sprd-dma.c
> > > +++ b/drivers/dma/sprd-dma.c
> > > @@ -625,7 +625,7 @@ static enum dma_status sprd_dma_tx_status(struct dma_chan *chan,
> > >               else
> > >                       pos = 0;
> > >       } else if (schan->cur_desc && schan->cur_desc->vd.tx.cookie == cookie) {
> > > -             struct sprd_dma_desc *sdesc = to_sprd_dma_desc(vd);
> > > +             struct sprd_dma_desc *sdesc = schan->cur_desc;
> > >
> > >               if (sdesc->dir == DMA_DEV_TO_MEM)
> > >                       pos = sprd_dma_get_dst_addr(schan);
> > > --
> > > 1.7.9.5
> >
> > --
> > ~Vinod
> 
> 
> 
> -- 
> Baolin Wang
> Best Regards

-- 
~Vinod

[PATCH 2/7] dmaengine: sprd: Add validation of current descriptor in irq handler

[Baolin Wang]

When user terminates one DMA channel to free all its descriptors, but
at the same time one transaction interrupt was triggered possibly, now
we should not handle this interrupt by validating if the 'schan->cur_desc'
was set as NULL to avoid crashing the kernel.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 drivers/dma/sprd-dma.c |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
index e29342a..431e289 100644
--- a/drivers/dma/sprd-dma.c
+++ b/drivers/dma/sprd-dma.c
@@ -552,12 +552,17 @@ static irqreturn_t dma_irq_handle(int irq, void *dev_id)
 		schan = &sdev->channels[i];
 
 		spin_lock(&schan->vc.lock);
+
+		sdesc = schan->cur_desc;
+		if (!sdesc) {
+			spin_unlock(&schan->vc.lock);
+			return IRQ_HANDLED;
+		}
+
 		int_type = sprd_dma_get_int_type(schan);
 		req_type = sprd_dma_get_req_type(schan);
 		sprd_dma_clear_int(schan);
 
-		sdesc = schan->cur_desc;
-
 		/* cyclic mode schedule callback */
 		cyclic = schan->linklist.phy_addr ? true : false;
 		if (cyclic == true) {
-- 
1.7.9.5

[PATCH 3/7] dmaengine: sprd: Fix the incorrect start for 2-stage destination channels

[Baolin Wang]

From: Eric Long <eric.long@unisoc.com>

The 2-stage destination channel will be triggered by source channel
automatically, which means we should not trigger it by software request.

Signed-off-by: Eric Long <eric.long@unisoc.com>
Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 drivers/dma/sprd-dma.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
index 431e289..0f92e60 100644
--- a/drivers/dma/sprd-dma.c
+++ b/drivers/dma/sprd-dma.c
@@ -510,7 +510,9 @@ static void sprd_dma_start(struct sprd_dma_chn *schan)
 	sprd_dma_set_uid(schan);
 	sprd_dma_enable_chn(schan);
 
-	if (schan->dev_id == SPRD_DMA_SOFTWARE_UID)
+	if (schan->dev_id == SPRD_DMA_SOFTWARE_UID &&
+	    schan->chn_mode != SPRD_DMA_DST_CHN0 &&
+	    schan->chn_mode != SPRD_DMA_DST_CHN1)
 		sprd_dma_soft_request(schan);
 }
 
-- 
1.7.9.5

[PATCH 4/7] dmaengine: sprd: Add device validation to support multiple controllers

[Baolin Wang]

From: Eric Long <eric.long@unisoc.com>

Since we can support multiple DMA engine controllers, we should add
device validation in filter function to check if the correct controller
to be requested.

Signed-off-by: Eric Long <eric.long@unisoc.com>
Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 drivers/dma/sprd-dma.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
index 0f92e60..9f99d4b 100644
--- a/drivers/dma/sprd-dma.c
+++ b/drivers/dma/sprd-dma.c
@@ -1020,8 +1020,13 @@ static void sprd_dma_free_desc(struct virt_dma_desc *vd)
 static bool sprd_dma_filter_fn(struct dma_chan *chan, void *param)
 {
 	struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
+	struct of_phandle_args *dma_spec =
+		container_of(param, struct of_phandle_args, args[0]);
 	u32 slave_id = *(u32 *)param;
 
+	if (chan->device->dev->of_node != dma_spec->np)
+		return false;
+
 	schan->dev_id = slave_id;
 	return true;
 }
-- 
1.7.9.5

Re: [PATCH 4/7] dmaengine: sprd: Add device validation to support multiple controllers

[Vinod Koul]

On 15-04-19, 20:14, Baolin Wang wrote:
> From: Eric Long <eric.long@unisoc.com>
> 
> Since we can support multiple DMA engine controllers, we should add
> device validation in filter function to check if the correct controller
> to be requested.
> 
> Signed-off-by: Eric Long <eric.long@unisoc.com>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
>  drivers/dma/sprd-dma.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> index 0f92e60..9f99d4b 100644
> --- a/drivers/dma/sprd-dma.c
> +++ b/drivers/dma/sprd-dma.c
> @@ -1020,8 +1020,13 @@ static void sprd_dma_free_desc(struct virt_dma_desc *vd)
>  static bool sprd_dma_filter_fn(struct dma_chan *chan, void *param)
>  {
>  	struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
> +	struct of_phandle_args *dma_spec =
> +		container_of(param, struct of_phandle_args, args[0]);
>  	u32 slave_id = *(u32 *)param;
>  
> +	if (chan->device->dev->of_node != dma_spec->np)

Are you not using of_dma_find_controller() that does this, so this would
be useless!

> +		return false;
> +
>  	schan->dev_id = slave_id;
>  	return true;
>  }
> -- 
> 1.7.9.5

-- 
~Vinod

Re: [PATCH 4/7] dmaengine: sprd: Add device validation to support multiple controllers

[Baolin Wang]

On Mon, 29 Apr 2019 at 19:57, Vinod Koul <vkoul@kernel.org> wrote:
>
> On 15-04-19, 20:14, Baolin Wang wrote:
> > From: Eric Long <eric.long@unisoc.com>
> >
> > Since we can support multiple DMA engine controllers, we should add
> > device validation in filter function to check if the correct controller
> > to be requested.
> >
> > Signed-off-by: Eric Long <eric.long@unisoc.com>
> > Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> > ---
> >  drivers/dma/sprd-dma.c |    5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> > index 0f92e60..9f99d4b 100644
> > --- a/drivers/dma/sprd-dma.c
> > +++ b/drivers/dma/sprd-dma.c
> > @@ -1020,8 +1020,13 @@ static void sprd_dma_free_desc(struct virt_dma_desc *vd)
> >  static bool sprd_dma_filter_fn(struct dma_chan *chan, void *param)
> >  {
> >       struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
> > +     struct of_phandle_args *dma_spec =
> > +             container_of(param, struct of_phandle_args, args[0]);
> >       u32 slave_id = *(u32 *)param;
> >
> > +     if (chan->device->dev->of_node != dma_spec->np)
>
> Are you not using of_dma_find_controller() that does this, so this would
> be useless!

Yes, we can use of_dma_find_controller(), but that will be a little
complicated than current solution. Since we need introduce one
structure to save the node to validate in the filter function like
below, which seems make things complicated. But if you still like to
use of_dma_find_controller(), I can change to use it in next version.
Thank.

struct sprd_dma_filter_param {
     struct device_node *np;
};

static struct dma_chan* sprd_dma_xlate(struct of_phandle_args
*dma_spec, struct of_dma *of_dma)
{
    param.np = dma_spec->node;

    return dma_request_channel(xxx);
}

of_dma_controller_register(np, sprd_dma_xlate, sdev);

-- 
Baolin Wang
Best Regards

Re: [PATCH 4/7] dmaengine: sprd: Add device validation to support multiple controllers

[Vinod Koul]

On 29-04-19, 20:20, Baolin Wang wrote:
> On Mon, 29 Apr 2019 at 19:57, Vinod Koul <vkoul@kernel.org> wrote:
> >
> > On 15-04-19, 20:14, Baolin Wang wrote:
> > > From: Eric Long <eric.long@unisoc.com>
> > >
> > > Since we can support multiple DMA engine controllers, we should add
> > > device validation in filter function to check if the correct controller
> > > to be requested.
> > >
> > > Signed-off-by: Eric Long <eric.long@unisoc.com>
> > > Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> > > ---
> > >  drivers/dma/sprd-dma.c |    5 +++++
> > >  1 file changed, 5 insertions(+)
> > >
> > > diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> > > index 0f92e60..9f99d4b 100644
> > > --- a/drivers/dma/sprd-dma.c
> > > +++ b/drivers/dma/sprd-dma.c
> > > @@ -1020,8 +1020,13 @@ static void sprd_dma_free_desc(struct virt_dma_desc *vd)
> > >  static bool sprd_dma_filter_fn(struct dma_chan *chan, void *param)
> > >  {
> > >       struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
> > > +     struct of_phandle_args *dma_spec =
> > > +             container_of(param, struct of_phandle_args, args[0]);
> > >       u32 slave_id = *(u32 *)param;
> > >
> > > +     if (chan->device->dev->of_node != dma_spec->np)
> >
> > Are you not using of_dma_find_controller() that does this, so this would
> > be useless!
> 
> Yes, we can use of_dma_find_controller(), but that will be a little
> complicated than current solution. Since we need introduce one
> structure to save the node to validate in the filter function like
> below, which seems make things complicated. But if you still like to
> use of_dma_find_controller(), I can change to use it in next version.

Sorry I should have clarified more..

of_dma_find_controller() is called by xlate, so you already run this
check, so why use this :)

-- 
~Vinod

Re: [PATCH 4/7] dmaengine: sprd: Add device validation to support multiple controllers

[Baolin Wang]

On Mon, 29 Apr 2019 at 22:05, Vinod Koul <vkoul@kernel.org> wrote:
>
> On 29-04-19, 20:20, Baolin Wang wrote:
> > On Mon, 29 Apr 2019 at 19:57, Vinod Koul <vkoul@kernel.org> wrote:
> > >
> > > On 15-04-19, 20:14, Baolin Wang wrote:
> > > > From: Eric Long <eric.long@unisoc.com>
> > > >
> > > > Since we can support multiple DMA engine controllers, we should add
> > > > device validation in filter function to check if the correct controller
> > > > to be requested.
> > > >
> > > > Signed-off-by: Eric Long <eric.long@unisoc.com>
> > > > Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> > > > ---
> > > >  drivers/dma/sprd-dma.c |    5 +++++
> > > >  1 file changed, 5 insertions(+)
> > > >
> > > > diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> > > > index 0f92e60..9f99d4b 100644
> > > > --- a/drivers/dma/sprd-dma.c
> > > > +++ b/drivers/dma/sprd-dma.c
> > > > @@ -1020,8 +1020,13 @@ static void sprd_dma_free_desc(struct virt_dma_desc *vd)
> > > >  static bool sprd_dma_filter_fn(struct dma_chan *chan, void *param)
> > > >  {
> > > >       struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
> > > > +     struct of_phandle_args *dma_spec =
> > > > +             container_of(param, struct of_phandle_args, args[0]);
> > > >       u32 slave_id = *(u32 *)param;
> > > >
> > > > +     if (chan->device->dev->of_node != dma_spec->np)
> > >
> > > Are you not using of_dma_find_controller() that does this, so this would
> > > be useless!
> >
> > Yes, we can use of_dma_find_controller(), but that will be a little
> > complicated than current solution. Since we need introduce one
> > structure to save the node to validate in the filter function like
> > below, which seems make things complicated. But if you still like to
> > use of_dma_find_controller(), I can change to use it in next version.
>
> Sorry I should have clarified more..
>
> of_dma_find_controller() is called by xlate, so you already run this
> check, so why use this :)

The of_dma_find_controller() can save the requested device node into
dma_spec, and in the of_dma_simple_xlate() function, it will call
dma_request_channel() to request one channel, but it did not validate
the device node to find the corresponding dma device in
dma_request_channel(). So we should in our filter function to validate
the device node with the device node specified by the dma_spec. Hope I
make things clear.

-- 
Baolin Wang
Best Regards

Re: [PATCH 4/7] dmaengine: sprd: Add device validation to support multiple controllers

[Vinod Koul]

On 30-04-19, 13:30, Baolin Wang wrote:
> On Mon, 29 Apr 2019 at 22:05, Vinod Koul <vkoul@kernel.org> wrote:
> >
> > On 29-04-19, 20:20, Baolin Wang wrote:
> > > On Mon, 29 Apr 2019 at 19:57, Vinod Koul <vkoul@kernel.org> wrote:
> > > >
> > > > On 15-04-19, 20:14, Baolin Wang wrote:
> > > > > From: Eric Long <eric.long@unisoc.com>
> > > > >
> > > > > Since we can support multiple DMA engine controllers, we should add
> > > > > device validation in filter function to check if the correct controller
> > > > > to be requested.
> > > > >
> > > > > Signed-off-by: Eric Long <eric.long@unisoc.com>
> > > > > Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> > > > > ---
> > > > >  drivers/dma/sprd-dma.c |    5 +++++
> > > > >  1 file changed, 5 insertions(+)
> > > > >
> > > > > diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> > > > > index 0f92e60..9f99d4b 100644
> > > > > --- a/drivers/dma/sprd-dma.c
> > > > > +++ b/drivers/dma/sprd-dma.c
> > > > > @@ -1020,8 +1020,13 @@ static void sprd_dma_free_desc(struct virt_dma_desc *vd)
> > > > >  static bool sprd_dma_filter_fn(struct dma_chan *chan, void *param)
> > > > >  {
> > > > >       struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
> > > > > +     struct of_phandle_args *dma_spec =
> > > > > +             container_of(param, struct of_phandle_args, args[0]);
> > > > >       u32 slave_id = *(u32 *)param;
> > > > >
> > > > > +     if (chan->device->dev->of_node != dma_spec->np)
> > > >
> > > > Are you not using of_dma_find_controller() that does this, so this would
> > > > be useless!
> > >
> > > Yes, we can use of_dma_find_controller(), but that will be a little
> > > complicated than current solution. Since we need introduce one
> > > structure to save the node to validate in the filter function like
> > > below, which seems make things complicated. But if you still like to
> > > use of_dma_find_controller(), I can change to use it in next version.
> >
> > Sorry I should have clarified more..
> >
> > of_dma_find_controller() is called by xlate, so you already run this
> > check, so why use this :)
> 
> The of_dma_find_controller() can save the requested device node into
> dma_spec, and in the of_dma_simple_xlate() function, it will call
> dma_request_channel() to request one channel, but it did not validate
> the device node to find the corresponding dma device in
> dma_request_channel(). So we should in our filter function to validate
> the device node with the device node specified by the dma_spec. Hope I
> make things clear.

But dma_request_channel() calls of_dma_request_slave_channel() which
invokes of_dma_find_controller() why is it broken for you if that is the
case..

-- 
~Vinod

Re: [PATCH 4/7] dmaengine: sprd: Add device validation to support multiple controllers

[Baolin Wang]

On Tue, 30 Apr 2019 at 16:30, Vinod Koul <vkoul@kernel.org> wrote:
>
> On 30-04-19, 13:30, Baolin Wang wrote:
> > On Mon, 29 Apr 2019 at 22:05, Vinod Koul <vkoul@kernel.org> wrote:
> > >
> > > On 29-04-19, 20:20, Baolin Wang wrote:
> > > > On Mon, 29 Apr 2019 at 19:57, Vinod Koul <vkoul@kernel.org> wrote:
> > > > >
> > > > > On 15-04-19, 20:14, Baolin Wang wrote:
> > > > > > From: Eric Long <eric.long@unisoc.com>
> > > > > >
> > > > > > Since we can support multiple DMA engine controllers, we should add
> > > > > > device validation in filter function to check if the correct controller
> > > > > > to be requested.
> > > > > >
> > > > > > Signed-off-by: Eric Long <eric.long@unisoc.com>
> > > > > > Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> > > > > > ---
> > > > > >  drivers/dma/sprd-dma.c |    5 +++++
> > > > > >  1 file changed, 5 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> > > > > > index 0f92e60..9f99d4b 100644
> > > > > > --- a/drivers/dma/sprd-dma.c
> > > > > > +++ b/drivers/dma/sprd-dma.c
> > > > > > @@ -1020,8 +1020,13 @@ static void sprd_dma_free_desc(struct virt_dma_desc *vd)
> > > > > >  static bool sprd_dma_filter_fn(struct dma_chan *chan, void *param)
> > > > > >  {
> > > > > >       struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
> > > > > > +     struct of_phandle_args *dma_spec =
> > > > > > +             container_of(param, struct of_phandle_args, args[0]);
> > > > > >       u32 slave_id = *(u32 *)param;
> > > > > >
> > > > > > +     if (chan->device->dev->of_node != dma_spec->np)
> > > > >
> > > > > Are you not using of_dma_find_controller() that does this, so this would
> > > > > be useless!
> > > >
> > > > Yes, we can use of_dma_find_controller(), but that will be a little
> > > > complicated than current solution. Since we need introduce one
> > > > structure to save the node to validate in the filter function like
> > > > below, which seems make things complicated. But if you still like to
> > > > use of_dma_find_controller(), I can change to use it in next version.
> > >
> > > Sorry I should have clarified more..
> > >
> > > of_dma_find_controller() is called by xlate, so you already run this
> > > check, so why use this :)
> >
> > The of_dma_find_controller() can save the requested device node into
> > dma_spec, and in the of_dma_simple_xlate() function, it will call
> > dma_request_channel() to request one channel, but it did not validate
> > the device node to find the corresponding dma device in
> > dma_request_channel(). So we should in our filter function to validate
> > the device node with the device node specified by the dma_spec. Hope I
> > make things clear.
>
> But dma_request_channel() calls of_dma_request_slave_channel() which
> invokes of_dma_find_controller() why is it broken for you if that is the
> case..

No,the calling process should be:
dma_request_slave_channel()
--->dma_request_chan()--->of_dma_request_slave_channel()---->of_dma_simple_xlate()
----> dma_request_channel().

-- 
Baolin Wang
Best Regards

Re: [PATCH 4/7] dmaengine: sprd: Add device validation to support multiple controllers

[Baolin Wang]

Hi Vinod,

On Tue, 30 Apr 2019 at 16:34, Baolin Wang <baolin.wang@linaro.org> wrote:
>
> On Tue, 30 Apr 2019 at 16:30, Vinod Koul <vkoul@kernel.org> wrote:
> >
> > On 30-04-19, 13:30, Baolin Wang wrote:
> > > On Mon, 29 Apr 2019 at 22:05, Vinod Koul <vkoul@kernel.org> wrote:
> > > >
> > > > On 29-04-19, 20:20, Baolin Wang wrote:
> > > > > On Mon, 29 Apr 2019 at 19:57, Vinod Koul <vkoul@kernel.org> wrote:
> > > > > >
> > > > > > On 15-04-19, 20:14, Baolin Wang wrote:
> > > > > > > From: Eric Long <eric.long@unisoc.com>
> > > > > > >
> > > > > > > Since we can support multiple DMA engine controllers, we should add
> > > > > > > device validation in filter function to check if the correct controller
> > > > > > > to be requested.
> > > > > > >
> > > > > > > Signed-off-by: Eric Long <eric.long@unisoc.com>
> > > > > > > Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> > > > > > > ---
> > > > > > >  drivers/dma/sprd-dma.c |    5 +++++
> > > > > > >  1 file changed, 5 insertions(+)
> > > > > > >
> > > > > > > diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> > > > > > > index 0f92e60..9f99d4b 100644
> > > > > > > --- a/drivers/dma/sprd-dma.c
> > > > > > > +++ b/drivers/dma/sprd-dma.c
> > > > > > > @@ -1020,8 +1020,13 @@ static void sprd_dma_free_desc(struct virt_dma_desc *vd)
> > > > > > >  static bool sprd_dma_filter_fn(struct dma_chan *chan, void *param)
> > > > > > >  {
> > > > > > >       struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
> > > > > > > +     struct of_phandle_args *dma_spec =
> > > > > > > +             container_of(param, struct of_phandle_args, args[0]);
> > > > > > >       u32 slave_id = *(u32 *)param;
> > > > > > >
> > > > > > > +     if (chan->device->dev->of_node != dma_spec->np)
> > > > > >
> > > > > > Are you not using of_dma_find_controller() that does this, so this would
> > > > > > be useless!
> > > > >
> > > > > Yes, we can use of_dma_find_controller(), but that will be a little
> > > > > complicated than current solution. Since we need introduce one
> > > > > structure to save the node to validate in the filter function like
> > > > > below, which seems make things complicated. But if you still like to
> > > > > use of_dma_find_controller(), I can change to use it in next version.
> > > >
> > > > Sorry I should have clarified more..
> > > >
> > > > of_dma_find_controller() is called by xlate, so you already run this
> > > > check, so why use this :)
> > >
> > > The of_dma_find_controller() can save the requested device node into
> > > dma_spec, and in the of_dma_simple_xlate() function, it will call
> > > dma_request_channel() to request one channel, but it did not validate
> > > the device node to find the corresponding dma device in
> > > dma_request_channel(). So we should in our filter function to validate
> > > the device node with the device node specified by the dma_spec. Hope I
> > > make things clear.
> >
> > But dma_request_channel() calls of_dma_request_slave_channel() which
> > invokes of_dma_find_controller() why is it broken for you if that is the
> > case..
>
> No,the calling process should be:
> dma_request_slave_channel()
> --->dma_request_chan()--->of_dma_request_slave_channel()---->of_dma_simple_xlate()
> ----> dma_request_channel().
>

You can check other drivers, they also will save the device node to
validate in their filter function.
For example the imx-sdma driver:
https://elixir.bootlin.com/linux/v5.1-rc6/source/drivers/dma/imx-sdma.c#L1931

-- 
Baolin Wang
Best Regards

Re: [PATCH 4/7] dmaengine: sprd: Add device validation to support multiple controllers

[Vinod Koul]

On 30-04-19, 16:53, Baolin Wang wrote:
> Hi Vinod,
> 
> On Tue, 30 Apr 2019 at 16:34, Baolin Wang <baolin.wang@linaro.org> wrote:
> >
> > On Tue, 30 Apr 2019 at 16:30, Vinod Koul <vkoul@kernel.org> wrote:
> > >
> > > On 30-04-19, 13:30, Baolin Wang wrote:
> > > > On Mon, 29 Apr 2019 at 22:05, Vinod Koul <vkoul@kernel.org> wrote:
> > > > >
> > > > > On 29-04-19, 20:20, Baolin Wang wrote:
> > > > > > On Mon, 29 Apr 2019 at 19:57, Vinod Koul <vkoul@kernel.org> wrote:
> > > > > > >
> > > > > > > On 15-04-19, 20:14, Baolin Wang wrote:
> > > > > > > > From: Eric Long <eric.long@unisoc.com>
> > > > > > > >
> > > > > > > > Since we can support multiple DMA engine controllers, we should add
> > > > > > > > device validation in filter function to check if the correct controller
> > > > > > > > to be requested.
> > > > > > > >
> > > > > > > > Signed-off-by: Eric Long <eric.long@unisoc.com>
> > > > > > > > Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> > > > > > > > ---
> > > > > > > >  drivers/dma/sprd-dma.c |    5 +++++
> > > > > > > >  1 file changed, 5 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> > > > > > > > index 0f92e60..9f99d4b 100644
> > > > > > > > --- a/drivers/dma/sprd-dma.c
> > > > > > > > +++ b/drivers/dma/sprd-dma.c
> > > > > > > > @@ -1020,8 +1020,13 @@ static void sprd_dma_free_desc(struct virt_dma_desc *vd)
> > > > > > > >  static bool sprd_dma_filter_fn(struct dma_chan *chan, void *param)
> > > > > > > >  {
> > > > > > > >       struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
> > > > > > > > +     struct of_phandle_args *dma_spec =
> > > > > > > > +             container_of(param, struct of_phandle_args, args[0]);
> > > > > > > >       u32 slave_id = *(u32 *)param;
> > > > > > > >
> > > > > > > > +     if (chan->device->dev->of_node != dma_spec->np)
> > > > > > >
> > > > > > > Are you not using of_dma_find_controller() that does this, so this would
> > > > > > > be useless!
> > > > > >
> > > > > > Yes, we can use of_dma_find_controller(), but that will be a little
> > > > > > complicated than current solution. Since we need introduce one
> > > > > > structure to save the node to validate in the filter function like
> > > > > > below, which seems make things complicated. But if you still like to
> > > > > > use of_dma_find_controller(), I can change to use it in next version.
> > > > >
> > > > > Sorry I should have clarified more..
> > > > >
> > > > > of_dma_find_controller() is called by xlate, so you already run this
> > > > > check, so why use this :)
> > > >
> > > > The of_dma_find_controller() can save the requested device node into
> > > > dma_spec, and in the of_dma_simple_xlate() function, it will call
> > > > dma_request_channel() to request one channel, but it did not validate
> > > > the device node to find the corresponding dma device in
> > > > dma_request_channel(). So we should in our filter function to validate
> > > > the device node with the device node specified by the dma_spec. Hope I
> > > > make things clear.
> > >
> > > But dma_request_channel() calls of_dma_request_slave_channel() which
> > > invokes of_dma_find_controller() why is it broken for you if that is the
> > > case..
> >
> > No,the calling process should be:
> > dma_request_slave_channel()
> > --->dma_request_chan()--->of_dma_request_slave_channel()---->of_dma_simple_xlate()
> > ----> dma_request_channel().

The thing is that this is a generic issue, so fix should be in the core
and not in the driver. Agree in you case of_dma_find_controller() is not
invoked, so we should fix that in core

> 
> You can check other drivers, they also will save the device node to
> validate in their filter function.
> For example the imx-sdma driver:
> https://elixir.bootlin.com/linux/v5.1-rc6/source/drivers/dma/imx-sdma.c#L1931

Exactly, more the reason this should be in core :)

-- 
~Vinod

Re: [PATCH 4/7] dmaengine: sprd: Add device validation to support multiple controllers

[Baolin Wang]

Hi Vinod,

On Thu, 2 May 2019 at 14:01, Vinod Koul <vkoul@kernel.org> wrote:
>
> On 30-04-19, 16:53, Baolin Wang wrote:
> > Hi Vinod,
> >
> > On Tue, 30 Apr 2019 at 16:34, Baolin Wang <baolin.wang@linaro.org> wrote:
> > >
> > > On Tue, 30 Apr 2019 at 16:30, Vinod Koul <vkoul@kernel.org> wrote:
> > > >
> > > > On 30-04-19, 13:30, Baolin Wang wrote:
> > > > > On Mon, 29 Apr 2019 at 22:05, Vinod Koul <vkoul@kernel.org> wrote:
> > > > > >
> > > > > > On 29-04-19, 20:20, Baolin Wang wrote:
> > > > > > > On Mon, 29 Apr 2019 at 19:57, Vinod Koul <vkoul@kernel.org> wrote:
> > > > > > > >
> > > > > > > > On 15-04-19, 20:14, Baolin Wang wrote:
> > > > > > > > > From: Eric Long <eric.long@unisoc.com>
> > > > > > > > >
> > > > > > > > > Since we can support multiple DMA engine controllers, we should add
> > > > > > > > > device validation in filter function to check if the correct controller
> > > > > > > > > to be requested.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Eric Long <eric.long@unisoc.com>
> > > > > > > > > Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> > > > > > > > > ---
> > > > > > > > >  drivers/dma/sprd-dma.c |    5 +++++
> > > > > > > > >  1 file changed, 5 insertions(+)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> > > > > > > > > index 0f92e60..9f99d4b 100644
> > > > > > > > > --- a/drivers/dma/sprd-dma.c
> > > > > > > > > +++ b/drivers/dma/sprd-dma.c
> > > > > > > > > @@ -1020,8 +1020,13 @@ static void sprd_dma_free_desc(struct virt_dma_desc *vd)
> > > > > > > > >  static bool sprd_dma_filter_fn(struct dma_chan *chan, void *param)
> > > > > > > > >  {
> > > > > > > > >       struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
> > > > > > > > > +     struct of_phandle_args *dma_spec =
> > > > > > > > > +             container_of(param, struct of_phandle_args, args[0]);
> > > > > > > > >       u32 slave_id = *(u32 *)param;
> > > > > > > > >
> > > > > > > > > +     if (chan->device->dev->of_node != dma_spec->np)
> > > > > > > >
> > > > > > > > Are you not using of_dma_find_controller() that does this, so this would
> > > > > > > > be useless!
> > > > > > >
> > > > > > > Yes, we can use of_dma_find_controller(), but that will be a little
> > > > > > > complicated than current solution. Since we need introduce one
> > > > > > > structure to save the node to validate in the filter function like
> > > > > > > below, which seems make things complicated. But if you still like to
> > > > > > > use of_dma_find_controller(), I can change to use it in next version.
> > > > > >
> > > > > > Sorry I should have clarified more..
> > > > > >
> > > > > > of_dma_find_controller() is called by xlate, so you already run this
> > > > > > check, so why use this :)
> > > > >
> > > > > The of_dma_find_controller() can save the requested device node into
> > > > > dma_spec, and in the of_dma_simple_xlate() function, it will call
> > > > > dma_request_channel() to request one channel, but it did not validate
> > > > > the device node to find the corresponding dma device in
> > > > > dma_request_channel(). So we should in our filter function to validate
> > > > > the device node with the device node specified by the dma_spec. Hope I
> > > > > make things clear.
> > > >
> > > > But dma_request_channel() calls of_dma_request_slave_channel() which
> > > > invokes of_dma_find_controller() why is it broken for you if that is the
> > > > case..
> > >
> > > No,the calling process should be:
> > > dma_request_slave_channel()
> > > --->dma_request_chan()--->of_dma_request_slave_channel()---->of_dma_simple_xlate()
> > > ----> dma_request_channel().
>
> The thing is that this is a generic issue, so fix should be in the core
> and not in the driver. Agree in you case of_dma_find_controller() is not
> invoked, so we should fix that in core
>
> >
> > You can check other drivers, they also will save the device node to
> > validate in their filter function.
> > For example the imx-sdma driver:
> > https://elixir.bootlin.com/linux/v5.1-rc6/source/drivers/dma/imx-sdma.c#L1931
>
> Exactly, more the reason this should be in core :)

Sorry for late reply due to my holiday.

OK, I can move the fix into the core. So I think I will drop this
patch from my patchset, and I will create another patch set to fix the
device node validation issue with converting other drivers which did
the similar things. Thanks.

-- 
Baolin Wang
Best Regards

[PATCH 5/7] dmaengine: sprd: Fix block length overflow

[Baolin Wang]

From: Eric Long <eric.long@unisoc.com>

The maximum value of block length is 0xffff, so if the configured transfer length
is more than 0xffff, that will cause block length overflow to lead a configuration
error.

Thus we can set block length as the maximum burst length to avoid this issue, since
the maximum burst length will not be a big value which is more than 0xffff.

Signed-off-by: Eric Long <eric.long@unisoc.com>
Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 drivers/dma/sprd-dma.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
index 9f99d4b..a64271e 100644
--- a/drivers/dma/sprd-dma.c
+++ b/drivers/dma/sprd-dma.c
@@ -778,7 +778,7 @@ static int sprd_dma_fill_desc(struct dma_chan *chan,
 	temp |= slave_cfg->src_maxburst & SPRD_DMA_FRG_LEN_MASK;
 	hw->frg_len = temp;
 
-	hw->blk_len = len & SPRD_DMA_BLK_LEN_MASK;
+	hw->blk_len = slave_cfg->src_maxburst & SPRD_DMA_BLK_LEN_MASK;
 	hw->trsc_len = len & SPRD_DMA_TRSC_LEN_MASK;
 
 	temp = (dst_step & SPRD_DMA_TRSF_STEP_MASK) << SPRD_DMA_DEST_TRSF_STEP_OFFSET;
-- 
1.7.9.5

[PATCH 6/7] dmaengine: sprd: Fix the right place to configure 2-stage transfer

[Baolin Wang]

From: Eric Long <eric.long@unisoc.com>

Move the 2-stage configuration before configuring the link-list mode,
since we will use some 2-stage configuration to fill the link-list
configuration.

Signed-off-by: Eric Long <eric.long@unisoc.com>
Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 drivers/dma/sprd-dma.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
index a64271e..cc9c24d 100644
--- a/drivers/dma/sprd-dma.c
+++ b/drivers/dma/sprd-dma.c
@@ -911,6 +911,12 @@ static int sprd_dma_fill_linklist_desc(struct dma_chan *chan,
 		schan->linklist.virt_addr = 0;
 	}
 
+	/* Set channel mode and trigger mode for 2-stage transfer */
+	schan->chn_mode =
+		(flags >> SPRD_DMA_CHN_MODE_SHIFT) & SPRD_DMA_CHN_MODE_MASK;
+	schan->trg_mode =
+		(flags >> SPRD_DMA_TRG_MODE_SHIFT) & SPRD_DMA_TRG_MODE_MASK;
+
 	sdesc = kzalloc(sizeof(*sdesc), GFP_NOWAIT);
 	if (!sdesc)
 		return NULL;
@@ -944,12 +950,6 @@ static int sprd_dma_fill_linklist_desc(struct dma_chan *chan,
 		}
 	}
 
-	/* Set channel mode and trigger mode for 2-stage transfer */
-	schan->chn_mode =
-		(flags >> SPRD_DMA_CHN_MODE_SHIFT) & SPRD_DMA_CHN_MODE_MASK;
-	schan->trg_mode =
-		(flags >> SPRD_DMA_TRG_MODE_SHIFT) & SPRD_DMA_TRG_MODE_MASK;
-
 	ret = sprd_dma_fill_desc(chan, &sdesc->chn_hw, 0, 0, src, dst, len,
 				 dir, flags, slave_cfg);
 	if (ret) {
-- 
1.7.9.5

[PATCH 7/7] dmaengine: sprd: Add interrupt support for 2-stage transfer

[Baolin Wang]

For 2-stage transfer, some users like Audio still need transaction interrupt
to notify when the 2-stage transfer is completed. Thus we should enable
2-stage transfer interrupt to support this feature.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 drivers/dma/sprd-dma.c |   22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
index cc9c24d..4c18f44 100644
--- a/drivers/dma/sprd-dma.c
+++ b/drivers/dma/sprd-dma.c
@@ -62,6 +62,8 @@
 /* SPRD_DMA_GLB_2STAGE_GRP register definition */
 #define SPRD_DMA_GLB_2STAGE_EN		BIT(24)
 #define SPRD_DMA_GLB_CHN_INT_MASK	GENMASK(23, 20)
+#define SPRD_DMA_GLB_DEST_INT		BIT(22)
+#define SPRD_DMA_GLB_SRC_INT		BIT(20)
 #define SPRD_DMA_GLB_LIST_DONE_TRG	BIT(19)
 #define SPRD_DMA_GLB_TRANS_DONE_TRG	BIT(18)
 #define SPRD_DMA_GLB_BLOCK_DONE_TRG	BIT(17)
@@ -135,6 +137,7 @@
 /* define DMA channel mode & trigger mode mask */
 #define SPRD_DMA_CHN_MODE_MASK		GENMASK(7, 0)
 #define SPRD_DMA_TRG_MODE_MASK		GENMASK(7, 0)
+#define SPRD_DMA_INT_TYPE_MASK		GENMASK(7, 0)
 
 /* define the DMA transfer step type */
 #define SPRD_DMA_NONE_STEP		0
@@ -190,6 +193,7 @@ struct sprd_dma_chn {
 	u32			dev_id;
 	enum sprd_dma_chn_mode	chn_mode;
 	enum sprd_dma_trg_mode	trg_mode;
+	enum sprd_dma_int_type	int_type;
 	struct sprd_dma_desc	*cur_desc;
 };
 
@@ -429,6 +433,9 @@ static int sprd_dma_set_2stage_config(struct sprd_dma_chn *schan)
 		val = chn & SPRD_DMA_GLB_SRC_CHN_MASK;
 		val |= BIT(schan->trg_mode - 1) << SPRD_DMA_GLB_TRG_OFFSET;
 		val |= SPRD_DMA_GLB_2STAGE_EN;
+		if (schan->int_type != SPRD_DMA_NO_INT)
+			val |= SPRD_DMA_GLB_SRC_INT;
+
 		sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP1, val, val);
 		break;
 
@@ -436,6 +443,9 @@ static int sprd_dma_set_2stage_config(struct sprd_dma_chn *schan)
 		val = chn & SPRD_DMA_GLB_SRC_CHN_MASK;
 		val |= BIT(schan->trg_mode - 1) << SPRD_DMA_GLB_TRG_OFFSET;
 		val |= SPRD_DMA_GLB_2STAGE_EN;
+		if (schan->int_type != SPRD_DMA_NO_INT)
+			val |= SPRD_DMA_GLB_SRC_INT;
+
 		sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP2, val, val);
 		break;
 
@@ -443,6 +453,9 @@ static int sprd_dma_set_2stage_config(struct sprd_dma_chn *schan)
 		val = (chn << SPRD_DMA_GLB_DEST_CHN_OFFSET) &
 			SPRD_DMA_GLB_DEST_CHN_MASK;
 		val |= SPRD_DMA_GLB_2STAGE_EN;
+		if (schan->int_type != SPRD_DMA_NO_INT)
+			val |= SPRD_DMA_GLB_DEST_INT;
+
 		sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP1, val, val);
 		break;
 
@@ -450,6 +463,9 @@ static int sprd_dma_set_2stage_config(struct sprd_dma_chn *schan)
 		val = (chn << SPRD_DMA_GLB_DEST_CHN_OFFSET) &
 			SPRD_DMA_GLB_DEST_CHN_MASK;
 		val |= SPRD_DMA_GLB_2STAGE_EN;
+		if (schan->int_type != SPRD_DMA_NO_INT)
+			val |= SPRD_DMA_GLB_DEST_INT;
+
 		sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP2, val, val);
 		break;
 
@@ -911,11 +927,15 @@ static int sprd_dma_fill_linklist_desc(struct dma_chan *chan,
 		schan->linklist.virt_addr = 0;
 	}
 
-	/* Set channel mode and trigger mode for 2-stage transfer */
+	/*
+	 * Set channel mode, interrupt mode and trigger mode for 2-stage
+	 * transfer.
+	 */
 	schan->chn_mode =
 		(flags >> SPRD_DMA_CHN_MODE_SHIFT) & SPRD_DMA_CHN_MODE_MASK;
 	schan->trg_mode =
 		(flags >> SPRD_DMA_TRG_MODE_SHIFT) & SPRD_DMA_TRG_MODE_MASK;
+	schan->int_type = flags & SPRD_DMA_INT_TYPE_MASK;
 
 	sdesc = kzalloc(sizeof(*sdesc), GFP_NOWAIT);
 	if (!sdesc)
-- 
1.7.9.5

Re: [PATCH 7/7] dmaengine: sprd: Add interrupt support for 2-stage transfer

[Vinod Koul]

On 15-04-19, 20:15, Baolin Wang wrote:
> For 2-stage transfer, some users like Audio still need transaction interrupt
> to notify when the 2-stage transfer is completed. Thus we should enable
> 2-stage transfer interrupt to support this feature.
> 
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
>  drivers/dma/sprd-dma.c |   22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> index cc9c24d..4c18f44 100644
> --- a/drivers/dma/sprd-dma.c
> +++ b/drivers/dma/sprd-dma.c
> @@ -62,6 +62,8 @@
>  /* SPRD_DMA_GLB_2STAGE_GRP register definition */
>  #define SPRD_DMA_GLB_2STAGE_EN		BIT(24)
>  #define SPRD_DMA_GLB_CHN_INT_MASK	GENMASK(23, 20)
> +#define SPRD_DMA_GLB_DEST_INT		BIT(22)
> +#define SPRD_DMA_GLB_SRC_INT		BIT(20)
>  #define SPRD_DMA_GLB_LIST_DONE_TRG	BIT(19)
>  #define SPRD_DMA_GLB_TRANS_DONE_TRG	BIT(18)
>  #define SPRD_DMA_GLB_BLOCK_DONE_TRG	BIT(17)
> @@ -135,6 +137,7 @@
>  /* define DMA channel mode & trigger mode mask */
>  #define SPRD_DMA_CHN_MODE_MASK		GENMASK(7, 0)
>  #define SPRD_DMA_TRG_MODE_MASK		GENMASK(7, 0)
> +#define SPRD_DMA_INT_TYPE_MASK		GENMASK(7, 0)
>  
>  /* define the DMA transfer step type */
>  #define SPRD_DMA_NONE_STEP		0
> @@ -190,6 +193,7 @@ struct sprd_dma_chn {
>  	u32			dev_id;
>  	enum sprd_dma_chn_mode	chn_mode;
>  	enum sprd_dma_trg_mode	trg_mode;
> +	enum sprd_dma_int_type	int_type;
>  	struct sprd_dma_desc	*cur_desc;
>  };
>  
> @@ -429,6 +433,9 @@ static int sprd_dma_set_2stage_config(struct sprd_dma_chn *schan)
>  		val = chn & SPRD_DMA_GLB_SRC_CHN_MASK;
>  		val |= BIT(schan->trg_mode - 1) << SPRD_DMA_GLB_TRG_OFFSET;
>  		val |= SPRD_DMA_GLB_2STAGE_EN;
> +		if (schan->int_type != SPRD_DMA_NO_INT)

Who configure int_type?

> +			val |= SPRD_DMA_GLB_SRC_INT;
> +
>  		sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP1, val, val);
>  		break;
>  
> @@ -436,6 +443,9 @@ static int sprd_dma_set_2stage_config(struct sprd_dma_chn *schan)
>  		val = chn & SPRD_DMA_GLB_SRC_CHN_MASK;
>  		val |= BIT(schan->trg_mode - 1) << SPRD_DMA_GLB_TRG_OFFSET;
>  		val |= SPRD_DMA_GLB_2STAGE_EN;
> +		if (schan->int_type != SPRD_DMA_NO_INT)
> +			val |= SPRD_DMA_GLB_SRC_INT;
> +
>  		sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP2, val, val);
>  		break;
>  
> @@ -443,6 +453,9 @@ static int sprd_dma_set_2stage_config(struct sprd_dma_chn *schan)
>  		val = (chn << SPRD_DMA_GLB_DEST_CHN_OFFSET) &
>  			SPRD_DMA_GLB_DEST_CHN_MASK;
>  		val |= SPRD_DMA_GLB_2STAGE_EN;
> +		if (schan->int_type != SPRD_DMA_NO_INT)
> +			val |= SPRD_DMA_GLB_DEST_INT;
> +
>  		sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP1, val, val);
>  		break;
>  
> @@ -450,6 +463,9 @@ static int sprd_dma_set_2stage_config(struct sprd_dma_chn *schan)
>  		val = (chn << SPRD_DMA_GLB_DEST_CHN_OFFSET) &
>  			SPRD_DMA_GLB_DEST_CHN_MASK;
>  		val |= SPRD_DMA_GLB_2STAGE_EN;
> +		if (schan->int_type != SPRD_DMA_NO_INT)
> +			val |= SPRD_DMA_GLB_DEST_INT;
> +
>  		sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP2, val, val);
>  		break;
>  
> @@ -911,11 +927,15 @@ static int sprd_dma_fill_linklist_desc(struct dma_chan *chan,
>  		schan->linklist.virt_addr = 0;
>  	}
>  
> -	/* Set channel mode and trigger mode for 2-stage transfer */
> +	/*
> +	 * Set channel mode, interrupt mode and trigger mode for 2-stage
> +	 * transfer.
> +	 */
>  	schan->chn_mode =
>  		(flags >> SPRD_DMA_CHN_MODE_SHIFT) & SPRD_DMA_CHN_MODE_MASK;
>  	schan->trg_mode =
>  		(flags >> SPRD_DMA_TRG_MODE_SHIFT) & SPRD_DMA_TRG_MODE_MASK;
> +	schan->int_type = flags & SPRD_DMA_INT_TYPE_MASK;
>  
>  	sdesc = kzalloc(sizeof(*sdesc), GFP_NOWAIT);
>  	if (!sdesc)
> -- 
> 1.7.9.5

-- 
~Vinod

Re: [PATCH 7/7] dmaengine: sprd: Add interrupt support for 2-stage transfer

[Baolin Wang]

On Mon, 29 Apr 2019 at 20:01, Vinod Koul <vkoul@kernel.org> wrote:
>
> On 15-04-19, 20:15, Baolin Wang wrote:
> > For 2-stage transfer, some users like Audio still need transaction interrupt
> > to notify when the 2-stage transfer is completed. Thus we should enable
> > 2-stage transfer interrupt to support this feature.
> >
> > Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> > ---
> >  drivers/dma/sprd-dma.c |   22 +++++++++++++++++++++-
> >  1 file changed, 21 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> > index cc9c24d..4c18f44 100644
> > --- a/drivers/dma/sprd-dma.c
> > +++ b/drivers/dma/sprd-dma.c
> > @@ -62,6 +62,8 @@
> >  /* SPRD_DMA_GLB_2STAGE_GRP register definition */
> >  #define SPRD_DMA_GLB_2STAGE_EN               BIT(24)
> >  #define SPRD_DMA_GLB_CHN_INT_MASK    GENMASK(23, 20)
> > +#define SPRD_DMA_GLB_DEST_INT                BIT(22)
> > +#define SPRD_DMA_GLB_SRC_INT         BIT(20)
> >  #define SPRD_DMA_GLB_LIST_DONE_TRG   BIT(19)
> >  #define SPRD_DMA_GLB_TRANS_DONE_TRG  BIT(18)
> >  #define SPRD_DMA_GLB_BLOCK_DONE_TRG  BIT(17)
> > @@ -135,6 +137,7 @@
> >  /* define DMA channel mode & trigger mode mask */
> >  #define SPRD_DMA_CHN_MODE_MASK               GENMASK(7, 0)
> >  #define SPRD_DMA_TRG_MODE_MASK               GENMASK(7, 0)
> > +#define SPRD_DMA_INT_TYPE_MASK               GENMASK(7, 0)
> >
> >  /* define the DMA transfer step type */
> >  #define SPRD_DMA_NONE_STEP           0
> > @@ -190,6 +193,7 @@ struct sprd_dma_chn {
> >       u32                     dev_id;
> >       enum sprd_dma_chn_mode  chn_mode;
> >       enum sprd_dma_trg_mode  trg_mode;
> > +     enum sprd_dma_int_type  int_type;
> >       struct sprd_dma_desc    *cur_desc;
> >  };
> >
> > @@ -429,6 +433,9 @@ static int sprd_dma_set_2stage_config(struct sprd_dma_chn *schan)
> >               val = chn & SPRD_DMA_GLB_SRC_CHN_MASK;
> >               val |= BIT(schan->trg_mode - 1) << SPRD_DMA_GLB_TRG_OFFSET;
> >               val |= SPRD_DMA_GLB_2STAGE_EN;
> > +             if (schan->int_type != SPRD_DMA_NO_INT)
>
> Who configure int_type?

The int_type is configured through the flags of
sprd_dma_prep_slave_sg() by users, see:
https://elixir.bootlin.com/linux/v5.1-rc6/source/include/linux/dma/sprd-dma.h#L9

>
> > +                     val |= SPRD_DMA_GLB_SRC_INT;
> > +
> >               sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP1, val, val);
> >               break;
> >
> > @@ -436,6 +443,9 @@ static int sprd_dma_set_2stage_config(struct sprd_dma_chn *schan)
> >               val = chn & SPRD_DMA_GLB_SRC_CHN_MASK;
> >               val |= BIT(schan->trg_mode - 1) << SPRD_DMA_GLB_TRG_OFFSET;
> >               val |= SPRD_DMA_GLB_2STAGE_EN;
> > +             if (schan->int_type != SPRD_DMA_NO_INT)
> > +                     val |= SPRD_DMA_GLB_SRC_INT;
> > +
> >               sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP2, val, val);
> >               break;
> >
> > @@ -443,6 +453,9 @@ static int sprd_dma_set_2stage_config(struct sprd_dma_chn *schan)
> >               val = (chn << SPRD_DMA_GLB_DEST_CHN_OFFSET) &
> >                       SPRD_DMA_GLB_DEST_CHN_MASK;
> >               val |= SPRD_DMA_GLB_2STAGE_EN;
> > +             if (schan->int_type != SPRD_DMA_NO_INT)
> > +                     val |= SPRD_DMA_GLB_DEST_INT;
> > +
> >               sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP1, val, val);
> >               break;
> >
> > @@ -450,6 +463,9 @@ static int sprd_dma_set_2stage_config(struct sprd_dma_chn *schan)
> >               val = (chn << SPRD_DMA_GLB_DEST_CHN_OFFSET) &
> >                       SPRD_DMA_GLB_DEST_CHN_MASK;
> >               val |= SPRD_DMA_GLB_2STAGE_EN;
> > +             if (schan->int_type != SPRD_DMA_NO_INT)
> > +                     val |= SPRD_DMA_GLB_DEST_INT;
> > +
> >               sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP2, val, val);
> >               break;
> >
> > @@ -911,11 +927,15 @@ static int sprd_dma_fill_linklist_desc(struct dma_chan *chan,
> >               schan->linklist.virt_addr = 0;
> >       }
> >
> > -     /* Set channel mode and trigger mode for 2-stage transfer */
> > +     /*
> > +      * Set channel mode, interrupt mode and trigger mode for 2-stage
> > +      * transfer.
> > +      */
> >       schan->chn_mode =
> >               (flags >> SPRD_DMA_CHN_MODE_SHIFT) & SPRD_DMA_CHN_MODE_MASK;
> >       schan->trg_mode =
> >               (flags >> SPRD_DMA_TRG_MODE_SHIFT) & SPRD_DMA_TRG_MODE_MASK;
> > +     schan->int_type = flags & SPRD_DMA_INT_TYPE_MASK;
> >
> >       sdesc = kzalloc(sizeof(*sdesc), GFP_NOWAIT);
> >       if (!sdesc)
> > --
> > 1.7.9.5
>
> --
> ~Vinod



-- 
Baolin Wang
Best Regards

Re: [PATCH 7/7] dmaengine: sprd: Add interrupt support for 2-stage transfer

[Vinod Koul]

On 29-04-19, 20:11, Baolin Wang wrote:
> On Mon, 29 Apr 2019 at 20:01, Vinod Koul <vkoul@kernel.org> wrote:
> > On 15-04-19, 20:15, Baolin Wang wrote:

> > > @@ -429,6 +433,9 @@ static int sprd_dma_set_2stage_config(struct sprd_dma_chn *schan)
> > >               val = chn & SPRD_DMA_GLB_SRC_CHN_MASK;
> > >               val |= BIT(schan->trg_mode - 1) << SPRD_DMA_GLB_TRG_OFFSET;
> > >               val |= SPRD_DMA_GLB_2STAGE_EN;
> > > +             if (schan->int_type != SPRD_DMA_NO_INT)
> >
> > Who configure int_type?
> 
> The int_type is configured through the flags of
> sprd_dma_prep_slave_sg() by users, see:
> https://elixir.bootlin.com/linux/v5.1-rc6/source/include/linux/dma/sprd-dma.h#L9

Please use DMA_PREP_INTERRUPT flag instead!

-- 
~Vinod

Re: [PATCH 7/7] dmaengine: sprd: Add interrupt support for 2-stage transfer

[Baolin Wang]

On Mon, 29 Apr 2019 at 22:10, Vinod Koul <vkoul@kernel.org> wrote:
>
> On 29-04-19, 20:11, Baolin Wang wrote:
> > On Mon, 29 Apr 2019 at 20:01, Vinod Koul <vkoul@kernel.org> wrote:
> > > On 15-04-19, 20:15, Baolin Wang wrote:
>
> > > > @@ -429,6 +433,9 @@ static int sprd_dma_set_2stage_config(struct sprd_dma_chn *schan)
> > > >               val = chn & SPRD_DMA_GLB_SRC_CHN_MASK;
> > > >               val |= BIT(schan->trg_mode - 1) << SPRD_DMA_GLB_TRG_OFFSET;
> > > >               val |= SPRD_DMA_GLB_2STAGE_EN;
> > > > +             if (schan->int_type != SPRD_DMA_NO_INT)
> > >
> > > Who configure int_type?
> >
> > The int_type is configured through the flags of
> > sprd_dma_prep_slave_sg() by users, see:
> > https://elixir.bootlin.com/linux/v5.1-rc6/source/include/linux/dma/sprd-dma.h#L9
>
> Please use DMA_PREP_INTERRUPT flag instead!

We can not use DMA_PREP_INTERRUPT flag, since we have some Spreadtrum
specific DMA interrupt flags configured by users, which I think we
have made a consensus before. See:
https://elixir.bootlin.com/linux/v5.1-rc6/source/include/linux/dma/sprd-dma.h#L105

-- 
Baolin Wang
Best Regards