[PATCH] dmaengine/dw_dmac: Reconfigure interrupt and chan_cfg register on resume

[PATCH] dmaengine/dw_dmac: Reconfigure interrupt and chan_cfg register on resume

[Viresh Kumar]

In S2R all DMA registers are reset by hardware and thus they are required to be
reprogrammed. The channels which aren't reprogrammed are channel configuration
and interrupt enable registers, which are currently programmed at chan_alloc
time.

This patch creates another routine to initialize a channel. It will try to
initialize channel on every dwc_dostart() call. If channel is already
initialised then it simply returns, otherwise it configures registers.

This routine will also initialize registers on wakeup from S2R, as we mark
channels as uninitialized on suspend.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---

Vinod,

This is rebased over your next branch with top commit:

commit 9b3fd2438c9cb19e7c7211c8dc33c45e1cb58bdf
Author: Ciaran McCormick <ciaranmccormick@gmail.com>
Date:   Mon Oct 31 19:29:26 2011 +0000

dma: fix spacing for method declaration, coding style issue in iop-adma.c
        
diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
index 9bfd6d3..8980893 100644
--- a/drivers/dma/dw_dmac.c
+++ b/drivers/dma/dw_dmac.c
@@ -166,6 +166,38 @@ dwc_assign_cookie(struct dw_dma_chan *dwc, struct dw_desc *desc)
 	return cookie;
 }
 
+static void dwc_initialize(struct dw_dma_chan *dwc)
+{
+	struct dw_dma *dw = to_dw_dma(dwc->chan.device);
+	struct dw_dma_slave *dws = dwc->chan.private;
+	u32 cfghi = DWC_CFGH_FIFO_MODE;
+	u32 cfglo = DWC_CFGL_CH_PRIOR(dwc->priority);
+
+	if (dwc->initialized == true)
+		return;
+
+	if (dws) {
+		/*
+		 * We need controller-specific data to set up slave
+		 * transfers.
+		 */
+		BUG_ON(!dws->dma_dev || dws->dma_dev != dw->dma.dev);
+
+		cfghi = dws->cfg_hi;
+		cfglo |= dws->cfg_lo & ~DWC_CFGL_CH_PRIOR_MASK;
+	}
+
+	channel_writel(dwc, CFG_LO, cfglo);
+	channel_writel(dwc, CFG_HI, cfghi);
+
+	/* Enable interrupts */
+	channel_set_bit(dw, MASK.XFER, dwc->mask);
+	channel_set_bit(dw, MASK.BLOCK, dwc->mask);
+	channel_set_bit(dw, MASK.ERROR, dwc->mask);
+
+	dwc->initialized = true;
+}
+
 /*----------------------------------------------------------------------*/
 
 /* Called with dwc->lock held and bh disabled */
@@ -189,6 +221,8 @@ static void dwc_dostart(struct dw_dma_chan *dwc, struct dw_desc *first)
 		return;
 	}
 
+	dwc_initialize(dwc);
+
 	channel_writel(dwc, LLP, first->txd.phys);
 	channel_writel(dwc, CTL_LO,
 			DWC_CTLL_LLP_D_EN | DWC_CTLL_LLP_S_EN);
@@ -959,10 +993,7 @@ static int dwc_alloc_chan_resources(struct dma_chan *chan)
 	struct dw_dma_chan	*dwc = to_dw_dma_chan(chan);
 	struct dw_dma		*dw = to_dw_dma(chan->device);
 	struct dw_desc		*desc;
-	struct dw_dma_slave	*dws;
 	int			i;
-	u32			cfghi;
-	u32			cfglo;
 	unsigned long		flags;
 
 	dev_vdbg(chan2dev(chan), "alloc_chan_resources\n");
@@ -975,26 +1006,6 @@ static int dwc_alloc_chan_resources(struct dma_chan *chan)
 
 	dwc->completed = chan->cookie = 1;
 
-	cfghi = DWC_CFGH_FIFO_MODE;
-	cfglo = 0;
-
-	dws = chan->private;
-	if (dws) {
-		/*
-		 * We need controller-specific data to set up slave
-		 * transfers.
-		 */
-		BUG_ON(!dws->dma_dev || dws->dma_dev != dw->dma.dev);
-
-		cfghi = dws->cfg_hi;
-		cfglo = dws->cfg_lo & ~DWC_CFGL_CH_PRIOR_MASK;
-	}
-
-	cfglo |= DWC_CFGL_CH_PRIOR(dwc->priority);
-
-	channel_writel(dwc, CFG_LO, cfglo);
-	channel_writel(dwc, CFG_HI, cfghi);
-
 	/*
 	 * NOTE: some controllers may have additional features that we
 	 * need to initialize here, like "scatter-gather" (which
@@ -1026,11 +1037,6 @@ static int dwc_alloc_chan_resources(struct dma_chan *chan)
 		i = ++dwc->descs_allocated;
 	}
 
-	/* Enable interrupts */
-	channel_set_bit(dw, MASK.XFER, dwc->mask);
-	channel_set_bit(dw, MASK.BLOCK, dwc->mask);
-	channel_set_bit(dw, MASK.ERROR, dwc->mask);
-
 	spin_unlock_irqrestore(&dwc->lock, flags);
 
 	dev_dbg(chan2dev(chan),
@@ -1058,6 +1064,7 @@ static void dwc_free_chan_resources(struct dma_chan *chan)
 	spin_lock_irqsave(&dwc->lock, flags);
 	list_splice_init(&dwc->free_list, &list);
 	dwc->descs_allocated = 0;
+	dwc->initialized = false;
 
 	/* Disable interrupts */
 	channel_clear_bit(dw, MASK.XFER, dwc->mask);
@@ -1335,6 +1342,8 @@ EXPORT_SYMBOL(dw_dma_cyclic_free);
 
 static void dw_dma_off(struct dw_dma *dw)
 {
+	int i;
+
 	dma_writel(dw, CFG, 0);
 
 	channel_clear_bit(dw, MASK.XFER, dw->all_chan_mask);
@@ -1345,6 +1354,9 @@ static void dw_dma_off(struct dw_dma *dw)
 
 	while (dma_readl(dw, CFG) & DW_CFG_DMA_EN)
 		cpu_relax();
+
+	for (i = 0; i < dw->dma.chancnt; i++)
+		dw->chan[i].initialized = false;
 }
 
 static int __init dw_probe(struct platform_device *pdev)
@@ -1533,6 +1545,7 @@ static int dw_suspend_noirq(struct device *dev)
 
 	dw_dma_off(platform_get_drvdata(pdev));
 	clk_disable(dw->clk);
+
 	return 0;
 }
 
diff --git a/drivers/dma/dw_dmac_regs.h b/drivers/dma/dw_dmac_regs.h
index c341951..5eef694 100644
--- a/drivers/dma/dw_dmac_regs.h
+++ b/drivers/dma/dw_dmac_regs.h
@@ -140,6 +140,7 @@ struct dw_dma_chan {
 	u8			mask;
 	u8			priority;
 	bool			paused;
+	bool			initialized;
 
 	spinlock_t		lock;
 
-- 
1.7.2.2

Re: [PATCH] dmaengine/dw_dmac: Reconfigure interrupt and chan_cfg register on resume

[Vinod Koul]

On Thu, 2011-11-17 at 16:01 +0530, Viresh Kumar wrote:
> In S2R all DMA registers are reset by hardware and thus they are required to be
When are they reset, whenever you leave channel idle or once you come
back from suspend?

Otherwise seems fine
> reprogrammed. The channels which aren't reprogrammed are channel configuration
> and interrupt enable registers, which are currently programmed at chan_alloc
> time.
> 
> This patch creates another routine to initialize a channel. It will try to
> initialize channel on every dwc_dostart() call. If channel is already
> initialised then it simply returns, otherwise it configures registers.
> 
> This routine will also initialize registers on wakeup from S2R, as we mark
> channels as uninitialized on suspend.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
> ---
> 
> Vinod,
> 
> This is rebased over your next branch with top commit:
> 
> commit 9b3fd2438c9cb19e7c7211c8dc33c45e1cb58bdf
> Author: Ciaran McCormick <ciaranmccormick@gmail.com>
> Date:   Mon Oct 31 19:29:26 2011 +0000
> 
> dma: fix spacing for method declaration, coding style issue in iop-adma.c
>         
> diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
> index 9bfd6d3..8980893 100644
> --- a/drivers/dma/dw_dmac.c
> +++ b/drivers/dma/dw_dmac.c
> @@ -166,6 +166,38 @@ dwc_assign_cookie(struct dw_dma_chan *dwc, struct dw_desc *desc)
>  	return cookie;
>  }
>  
> +static void dwc_initialize(struct dw_dma_chan *dwc)
> +{
> +	struct dw_dma *dw = to_dw_dma(dwc->chan.device);
> +	struct dw_dma_slave *dws = dwc->chan.private;
> +	u32 cfghi = DWC_CFGH_FIFO_MODE;
> +	u32 cfglo = DWC_CFGL_CH_PRIOR(dwc->priority);
> +
> +	if (dwc->initialized == true)
> +		return;
> +
> +	if (dws) {
> +		/*
> +		 * We need controller-specific data to set up slave
> +		 * transfers.
> +		 */
> +		BUG_ON(!dws->dma_dev || dws->dma_dev != dw->dma.dev);
> +
> +		cfghi = dws->cfg_hi;
> +		cfglo |= dws->cfg_lo & ~DWC_CFGL_CH_PRIOR_MASK;
> +	}
> +
> +	channel_writel(dwc, CFG_LO, cfglo);
> +	channel_writel(dwc, CFG_HI, cfghi);
> +
> +	/* Enable interrupts */
> +	channel_set_bit(dw, MASK.XFER, dwc->mask);
> +	channel_set_bit(dw, MASK.BLOCK, dwc->mask);
> +	channel_set_bit(dw, MASK.ERROR, dwc->mask);
> +
> +	dwc->initialized = true;
> +}
> +
>  /*----------------------------------------------------------------------*/
>  
>  /* Called with dwc->lock held and bh disabled */
> @@ -189,6 +221,8 @@ static void dwc_dostart(struct dw_dma_chan *dwc, struct dw_desc *first)
>  		return;
>  	}
>  
> +	dwc_initialize(dwc);
> +
>  	channel_writel(dwc, LLP, first->txd.phys);
>  	channel_writel(dwc, CTL_LO,
>  			DWC_CTLL_LLP_D_EN | DWC_CTLL_LLP_S_EN);
> @@ -959,10 +993,7 @@ static int dwc_alloc_chan_resources(struct dma_chan *chan)
>  	struct dw_dma_chan	*dwc = to_dw_dma_chan(chan);
>  	struct dw_dma		*dw = to_dw_dma(chan->device);
>  	struct dw_desc		*desc;
> -	struct dw_dma_slave	*dws;
>  	int			i;
> -	u32			cfghi;
> -	u32			cfglo;
>  	unsigned long		flags;
>  
>  	dev_vdbg(chan2dev(chan), "alloc_chan_resources\n");
> @@ -975,26 +1006,6 @@ static int dwc_alloc_chan_resources(struct dma_chan *chan)
>  
>  	dwc->completed = chan->cookie = 1;
>  
> -	cfghi = DWC_CFGH_FIFO_MODE;
> -	cfglo = 0;
> -
> -	dws = chan->private;
> -	if (dws) {
> -		/*
> -		 * We need controller-specific data to set up slave
> -		 * transfers.
> -		 */
> -		BUG_ON(!dws->dma_dev || dws->dma_dev != dw->dma.dev);
> -
> -		cfghi = dws->cfg_hi;
> -		cfglo = dws->cfg_lo & ~DWC_CFGL_CH_PRIOR_MASK;
> -	}
> -
> -	cfglo |= DWC_CFGL_CH_PRIOR(dwc->priority);
> -
> -	channel_writel(dwc, CFG_LO, cfglo);
> -	channel_writel(dwc, CFG_HI, cfghi);
> -
>  	/*
>  	 * NOTE: some controllers may have additional features that we
>  	 * need to initialize here, like "scatter-gather" (which
> @@ -1026,11 +1037,6 @@ static int dwc_alloc_chan_resources(struct dma_chan *chan)
>  		i = ++dwc->descs_allocated;
>  	}
>  
> -	/* Enable interrupts */
> -	channel_set_bit(dw, MASK.XFER, dwc->mask);
> -	channel_set_bit(dw, MASK.BLOCK, dwc->mask);
> -	channel_set_bit(dw, MASK.ERROR, dwc->mask);
> -
>  	spin_unlock_irqrestore(&dwc->lock, flags);
>  
>  	dev_dbg(chan2dev(chan),
> @@ -1058,6 +1064,7 @@ static void dwc_free_chan_resources(struct dma_chan *chan)
>  	spin_lock_irqsave(&dwc->lock, flags);
>  	list_splice_init(&dwc->free_list, &list);
>  	dwc->descs_allocated = 0;
> +	dwc->initialized = false;
>  
>  	/* Disable interrupts */
>  	channel_clear_bit(dw, MASK.XFER, dwc->mask);
> @@ -1335,6 +1342,8 @@ EXPORT_SYMBOL(dw_dma_cyclic_free);
>  
>  static void dw_dma_off(struct dw_dma *dw)
>  {
> +	int i;
> +
>  	dma_writel(dw, CFG, 0);
>  
>  	channel_clear_bit(dw, MASK.XFER, dw->all_chan_mask);
> @@ -1345,6 +1354,9 @@ static void dw_dma_off(struct dw_dma *dw)
>  
>  	while (dma_readl(dw, CFG) & DW_CFG_DMA_EN)
>  		cpu_relax();
> +
> +	for (i = 0; i < dw->dma.chancnt; i++)
> +		dw->chan[i].initialized =01a01f9db7dd061e6ec6ae587deb773d2864e3a0 false;
>  }
>  
>  static int __init dw_probe(struct platform_device *pdev)
> @@ -1533,6 +1545,7 @@ static int dw_suspend_noirq(struct device *dev)
>  
>  	dw_dma_off(platform_get_drvdata(pdev));
>  	clk_disable(dw->clk);
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/dma/dw_dmac_regs.h b/drivers/dma/dw_dmac_regs.h
> index c341951..5eef694 100644
> --- a/drivers/dma/dw_dmac_regs.h
> +++ b/drivers/dma/dw_dmac_regs.h
> @@ -140,6 +140,7 @@ struct dw_dma_chan {
>  	u8			mask;
>  	u8			priority;
>  	bool			paused;
> +	bool			initialized;
>  
>  	spinlock_t		lock;
>  


-- 
~Vinod

Re: [PATCH] dmaengine/dw_dmac: Reconfigure interrupt and chan_cfg register on resume

[Viresh Kumar]

On 11/22/2011 9:43 AM, Vinod Koul wrote:
> On Thu, 2011-11-17 at 16:01 +0530, Viresh Kumar wrote:
>> > In S2R all DMA registers are reset by hardware and thus they are required to be
> When are they reset, whenever you leave channel idle or once you come
> back from suspend?

Whenever we do S2R, many power islands are switched off and so DMAC controller looses
all its power. On resume, all registers are in their reset state.

-- 
viresh

Re: [PATCH] dmaengine/dw_dmac: Reconfigure interrupt and chan_cfg register on resume

[Vinod Koul]

On Tue, 2011-11-22 at 11:50 +0530, Viresh Kumar wrote:
> On 11/22/2011 9:43 AM, Vinod Koul wrote:
> > On Thu, 2011-11-17 at 16:01 +0530, Viresh Kumar wrote:
> >> > In S2R all DMA registers are reset by hardware and thus they are required to be
> > When are they reset, whenever you leave channel idle or once you come
> > back from suspend?
> 
> Whenever we do S2R, many power islands are switched off and so DMAC controller looses
> all its power. On resume, all registers are in their reset state.
then why not perform save and restore in your suspend and restore
callbacks. That way you don't pay penalty of doing that in every alloc
as you seem to be doing now?

-- 
~Vinod

Re: [PATCH] dmaengine/dw_dmac: Reconfigure interrupt and chan_cfg register on resume

[Viresh Kumar]

On 11/23/2011 4:19 PM, Vinod Koul wrote:
> then why not perform save and restore in your suspend and restore
> callbacks. That way you don't pay penalty of doing that in every alloc
> as you seem to be doing now?

Not actually. I am not saving anything to be restored after suspend.
There are few things (configuring CFG regs and enabling interrupts),
which are done earlier on alloc_channels. As this will not be called after
resume, so we need to set them again.

What i have done is, removed all this stuff from alloc channels and moved it
to dwc_dostart(). Here, i am checking if channel is already programmed or not.
If not (Can happen only in two cases: On resume and other on fresh channel alloc),
i configure them. So there is no save/restore actually done.

-- 
viresh

Re: [PATCH] dmaengine/dw_dmac: Reconfigure interrupt and chan_cfg register on resume

[Vinod Koul]

On Wed, 2011-11-23 at 16:41 +0530, Viresh Kumar wrote:
> On 11/23/2011 4:19 PM, Vinod Koul wrote:
> > then why not perform save and restore in your suspend and restore
> > callbacks. That way you don't pay penalty of doing that in every alloc
> > as you seem to be doing now?
> 
> Not actually. I am not saving anything to be restored after suspend.
> There are few things (configuring CFG regs and enabling interrupts),
> which are done earlier on alloc_channels. As this will not be called after
> resume, so we need to set them again.
> 
> What i have done is, removed all this stuff from alloc channels and moved it
> to dwc_dostart(). Here, i am checking if channel is already programmed or not.
> If not (Can happen only in two cases: On resume and other on fresh channel alloc),
> i configure them. So there is no save/restore actually done.
> 

okay, why were you writing to registers in alloc in the first place :)
it should have been in the dostart.

Okay now i get it, looks fine to me, I will apply it


-- 
~Vinod

Re: [PATCH] dmaengine/dw_dmac: Reconfigure interrupt and chan_cfg register on resume

[Viresh Kumar]

On 11/23/2011 5:02 PM, Vinod Koul wrote:
>> > 
> okay, why were you writing to registers in alloc in the first place :)
> it should have been in the dostart.
> 
> Okay now i get it, looks fine to me, I will apply it

Thanks.

BTW, any updates about following patchset. I didn't got any replies from Dan
on it:

[PATCH 1/3] dmaengine: Allow controller drivers to set channel ids
[PATCH 2/3] dmaengine/dw_dmac: Set channel id's in controller driver
[PATCH 3/3] dmaengine/dw_dmac: Don't use magic number for total number of channels

-- 
viresh

Re: [PATCH] dmaengine/dw_dmac: Reconfigure interrupt and chan_cfg register on resume

[Vinod Koul]

On Thu, 2011-11-17 at 16:01 +0530, Viresh Kumar wrote:
> In S2R all DMA registers are reset by hardware and thus they are
> required to be
> reprogrammed. The channels which aren't reprogrammed are channel
> configuration
> and interrupt enable registers, which are currently programmed at
> chan_alloc
> time.
> 
> This patch creates another routine to initialize a channel. It will
> try to
> initialize channel on every dwc_dostart() call. If channel is already
> initialised then it simply returns, otherwise it configures registers.
> 
> This routine will also initialize registers on wakeup from S2R, as we
> mark
> channels as uninitialized on suspend.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@st.com> 
Applied thanks

-- 
~Vinod