Re: [PATCH v5 1/3] dmaengine: xilinx_dma: Check for channel idle state before submitting dma descriptor

[Vinod Koul]

On Fri, Jan 13, 2017 at 04:28:11AM +, Appana Durga Kedareswara Rao wrote:
> Hi Vinod,
> 
>   Thanks for the review...
> > 
> > On Sat, Jan 07, 2017 at 12:15:28PM +0530, Kedareswara rao Appana wrote:
> > > Add channel idle state to ensure that dma descriptor is not
> > > submitted when VDMA engine is in progress.
> > 
> > any reason why you want to make your own varible and not use the HW to
> > query
> > as done earlier. It is not clear to me why that is removed from description
> 
> We need to poll for a bit in the status register to know the dma state.
> We are currently doing that in the driver hot path
> To avoid this using own variables.

It would be worthwhile to document these, down the line people may not
remeber the motivation


-- 
~Vinod

RE: [PATCH v5 1/3] dmaengine: xilinx_dma: Check for channel idle state before submitting dma descriptor

[Appana Durga Kedareswara Rao]

Hi Vinod,

Thanks for the review...
> 
> On Fri, Jan 13, 2017 at 04:28:11AM +, Appana Durga Kedareswara Rao
> wrote:
> > Hi Vinod,
> >
> > Thanks for the review...
> > >
> > > On Sat, Jan 07, 2017 at 12:15:28PM +0530, Kedareswara rao Appana wrote:
> > > > Add channel idle state to ensure that dma descriptor is not
> > > > submitted when VDMA engine is in progress.
> > >
> > > any reason why you want to make your own varible and not use the HW
> > > to query as done earlier. It is not clear to me why that is removed
> > > from description
> >
> > We need to poll for a bit in the status register to know the dma state.
> > We are currently doing that in the driver hot path To avoid this using
> > own variables.
> 
> It would be worthwhile to document these, down the line people may not
> remeber the motivation

Sure will add comments during the variable initialization
In the driver...

Regards,
Kedar.

> 
> 
> --
> ~Vinod

Re: [PATCH v5 1/3] dmaengine: xilinx_dma: Check for channel idle state before submitting dma descriptor

[Vinod Koul]

On Fri, Jan 13, 2017 at 04:28:11AM +, Appana Durga Kedareswara Rao wrote:
> Hi Vinod,
> 
>   Thanks for the review...
> > 
> > On Sat, Jan 07, 2017 at 12:15:28PM +0530, Kedareswara rao Appana wrote:
> > > Add channel idle state to ensure that dma descriptor is not
> > > submitted when VDMA engine is in progress.
> > 
> > any reason why you want to make your own varible and not use the HW to
> > query
> > as done earlier. It is not clear to me why that is removed from description
> 
> We need to poll for a bit in the status register to know the dma state.
> We are currently doing that in the driver hot path
> To avoid this using own variables.

It would be worthwhile to document these, down the line people may not
remeber the motivation


-- 
~Vinod

RE: [PATCH v5 1/3] dmaengine: xilinx_dma: Check for channel idle state before submitting dma descriptor

[Appana Durga Kedareswara Rao]

Hi Vinod,

Thanks for the review...
> 
> On Fri, Jan 13, 2017 at 04:28:11AM +, Appana Durga Kedareswara Rao
> wrote:
> > Hi Vinod,
> >
> > Thanks for the review...
> > >
> > > On Sat, Jan 07, 2017 at 12:15:28PM +0530, Kedareswara rao Appana wrote:
> > > > Add channel idle state to ensure that dma descriptor is not
> > > > submitted when VDMA engine is in progress.
> > >
> > > any reason why you want to make your own varible and not use the HW
> > > to query as done earlier. It is not clear to me why that is removed
> > > from description
> >
> > We need to poll for a bit in the status register to know the dma state.
> > We are currently doing that in the driver hot path To avoid this using
> > own variables.
> 
> It would be worthwhile to document these, down the line people may not
> remeber the motivation

Sure will add comments during the variable initialization
In the driver...

Regards,
Kedar.

> 
> 
> --
> ~Vinod

RE: [PATCH v5 1/3] dmaengine: xilinx_dma: Check for channel idle state before submitting dma descriptor

[Appana Durga Kedareswara Rao]

Hi Vinod,

Thanks for the review...
> 
> On Sat, Jan 07, 2017 at 12:15:28PM +0530, Kedareswara rao Appana wrote:
> > Add channel idle state to ensure that dma descriptor is not
> > submitted when VDMA engine is in progress.
> 
> any reason why you want to make your own varible and not use the HW to
> query
> as done earlier. It is not clear to me why that is removed from description

We need to poll for a bit in the status register to know the dma state.
We are currently doing that in the driver hot path
To avoid this using own variables.

Regards,
Kedar.

RE: [PATCH v5 1/3] dmaengine: xilinx_dma: Check for channel idle state before submitting dma descriptor

[Appana Durga Kedareswara Rao]

Hi Vinod,

Thanks for the review...
> 
> On Sat, Jan 07, 2017 at 12:15:28PM +0530, Kedareswara rao Appana wrote:
> > Add channel idle state to ensure that dma descriptor is not
> > submitted when VDMA engine is in progress.
> 
> any reason why you want to make your own varible and not use the HW to
> query
> as done earlier. It is not clear to me why that is removed from description

We need to poll for a bit in the status register to know the dma state.
We are currently doing that in the driver hot path
To avoid this using own variables.

Regards,
Kedar.

Re: [PATCH v5 1/3] dmaengine: xilinx_dma: Check for channel idle state before submitting dma descriptor

[Vinod Koul]

On Sat, Jan 07, 2017 at 12:15:28PM +0530, Kedareswara rao Appana wrote:
> Add channel idle state to ensure that dma descriptor is not
> submitted when VDMA engine is in progress.

any reason why you want to make your own varible and not use the HW to query
as done earlier. It is not clear to me why that is removed from description

> 
> Reviewed-by: Jose Abreu 
> Signed-off-by: Kedareswara rao Appana 
> ---
> Changes for v5:
> ---> None.
> Changes for v4:
> ---> None.
> Changes for v3:
> ---> None.
> Changes for v2:
> ---> Add idle check in the reset as suggested by Jose Abreu
> ---> Removed xilinx_dma_is_running/xilinx_dma_is_idle checks
> in the driver and used common idle checks across the driver
> as suggested by Laurent Pinchart.
> 
>  drivers/dma/xilinx/xilinx_dma.c | 56 
> +
>  1 file changed, 17 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
> index 8288fe4..be7eb41 100644
> --- a/drivers/dma/xilinx/xilinx_dma.c
> +++ b/drivers/dma/xilinx/xilinx_dma.c
> @@ -321,6 +321,7 @@ struct xilinx_dma_tx_descriptor {
>   * @cyclic: Check for cyclic transfers.
>   * @genlock: Support genlock mode
>   * @err: Channel has errors
> + * @idle: Check for channel idle
>   * @tasklet: Cleanup work after irq
>   * @config: Device configuration info
>   * @flush_on_fsync: Flush on Frame sync
> @@ -351,6 +352,7 @@ struct xilinx_dma_chan {
>   bool cyclic;
>   bool genlock;
>   bool err;
> + bool idle;
>   struct tasklet_struct tasklet;
>   struct xilinx_vdma_config config;
>   bool flush_on_fsync;
> @@ -920,32 +922,6 @@ static enum dma_status xilinx_dma_tx_status(struct 
> dma_chan *dchan,
>  }
>  
>  /**
> - * xilinx_dma_is_running - Check if DMA channel is running
> - * @chan: Driver specific DMA channel
> - *
> - * Return: '1' if running, '0' if not.
> - */
> -static bool xilinx_dma_is_running(struct xilinx_dma_chan *chan)
> -{
> - return !(dma_ctrl_read(chan, XILINX_DMA_REG_DMASR) &
> -  XILINX_DMA_DMASR_HALTED) &&
> - (dma_ctrl_read(chan, XILINX_DMA_REG_DMACR) &
> -  XILINX_DMA_DMACR_RUNSTOP);
> -}
> -
> -/**
> - * xilinx_dma_is_idle - Check if DMA channel is idle
> - * @chan: Driver specific DMA channel
> - *
> - * Return: '1' if idle, '0' if not.
> - */
> -static bool xilinx_dma_is_idle(struct xilinx_dma_chan *chan)
> -{
> - return dma_ctrl_read(chan, XILINX_DMA_REG_DMASR) &
> - XILINX_DMA_DMASR_IDLE;
> -}
> -
> -/**
>   * xilinx_dma_halt - Halt DMA channel
>   * @chan: Driver specific DMA channel
>   */
> @@ -966,6 +942,7 @@ static void xilinx_dma_halt(struct xilinx_dma_chan *chan)
>   chan, dma_ctrl_read(chan, XILINX_DMA_REG_DMASR));
>   chan->err = true;
>   }
> + chan->idle = true;
>  }
>  
>  /**
> @@ -1007,6 +984,9 @@ static void xilinx_vdma_start_transfer(struct 
> xilinx_dma_chan *chan)
>   if (chan->err)
>   return;
>  
> + if (!chan->idle)
> + return;
> +
>   if (list_empty(>pending_list))
>   return;
>  
> @@ -1018,13 +998,6 @@ static void xilinx_vdma_start_transfer(struct 
> xilinx_dma_chan *chan)
>   tail_segment = list_last_entry(_desc->segments,
>  struct xilinx_vdma_tx_segment, node);
>  
> - /* If it is SG mode and hardware is busy, cannot submit */
> - if (chan->has_sg && xilinx_dma_is_running(chan) &&
> - !xilinx_dma_is_idle(chan)) {
> - dev_dbg(chan->dev, "DMA controller still busy\n");
> - return;
> - }
> -
>   /*
>* If hardware is idle, then all descriptors on the running lists are
>* done, start new transfers
> @@ -1110,6 +1083,7 @@ static void xilinx_vdma_start_transfer(struct 
> xilinx_dma_chan *chan)
>   vdma_desc_write(chan, XILINX_DMA_REG_VSIZE, last->hw.vsize);
>   }
>  
> + chan->idle = false;
>   if (!chan->has_sg) {
>   list_del(>node);
>   list_add_tail(>node, >active_list);
> @@ -1136,6 +1110,9 @@ static void xilinx_cdma_start_transfer(struct 
> xilinx_dma_chan *chan)
>   if (chan->err)
>   return;
>  
> + if (!chan->idle)
> + return;
> +
>   if (list_empty(>pending_list))
>   return;
>  
> @@ -1181,6 +1158,7 @@ static void xilinx_cdma_start_transfer(struct 
> xilinx_dma_chan *chan)
>  
>   list_splice_tail_init(>pending_list, >active_list);
>   chan->desc_pendingcount = 0;
> + chan->idle = false;
>  }
>  
>  /**
> @@ -1196,15 +1174,11 @@ static void xilinx_dma_start_transfer(struct 
> xilinx_dma_chan *chan)
>   if (chan->err)
>   return;
>  
> - if (list_empty(>pending_list))
> + if (!chan->idle)
>   return;
>  
> - /* If it is SG mode and hardware is busy, cannot submit */
> - if (chan->has_sg && 

Re: [PATCH v5 1/3] dmaengine: xilinx_dma: Check for channel idle state before submitting dma descriptor

[Vinod Koul]

On Sat, Jan 07, 2017 at 12:15:28PM +0530, Kedareswara rao Appana wrote:
> Add channel idle state to ensure that dma descriptor is not
> submitted when VDMA engine is in progress.

any reason why you want to make your own varible and not use the HW to query
as done earlier. It is not clear to me why that is removed from description

> 
> Reviewed-by: Jose Abreu 
> Signed-off-by: Kedareswara rao Appana 
> ---
> Changes for v5:
> ---> None.
> Changes for v4:
> ---> None.
> Changes for v3:
> ---> None.
> Changes for v2:
> ---> Add idle check in the reset as suggested by Jose Abreu
> ---> Removed xilinx_dma_is_running/xilinx_dma_is_idle checks
> in the driver and used common idle checks across the driver
> as suggested by Laurent Pinchart.
> 
>  drivers/dma/xilinx/xilinx_dma.c | 56 
> +
>  1 file changed, 17 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
> index 8288fe4..be7eb41 100644
> --- a/drivers/dma/xilinx/xilinx_dma.c
> +++ b/drivers/dma/xilinx/xilinx_dma.c
> @@ -321,6 +321,7 @@ struct xilinx_dma_tx_descriptor {
>   * @cyclic: Check for cyclic transfers.
>   * @genlock: Support genlock mode
>   * @err: Channel has errors
> + * @idle: Check for channel idle
>   * @tasklet: Cleanup work after irq
>   * @config: Device configuration info
>   * @flush_on_fsync: Flush on Frame sync
> @@ -351,6 +352,7 @@ struct xilinx_dma_chan {
>   bool cyclic;
>   bool genlock;
>   bool err;
> + bool idle;
>   struct tasklet_struct tasklet;
>   struct xilinx_vdma_config config;
>   bool flush_on_fsync;
> @@ -920,32 +922,6 @@ static enum dma_status xilinx_dma_tx_status(struct 
> dma_chan *dchan,
>  }
>  
>  /**
> - * xilinx_dma_is_running - Check if DMA channel is running
> - * @chan: Driver specific DMA channel
> - *
> - * Return: '1' if running, '0' if not.
> - */
> -static bool xilinx_dma_is_running(struct xilinx_dma_chan *chan)
> -{
> - return !(dma_ctrl_read(chan, XILINX_DMA_REG_DMASR) &
> -  XILINX_DMA_DMASR_HALTED) &&
> - (dma_ctrl_read(chan, XILINX_DMA_REG_DMACR) &
> -  XILINX_DMA_DMACR_RUNSTOP);
> -}
> -
> -/**
> - * xilinx_dma_is_idle - Check if DMA channel is idle
> - * @chan: Driver specific DMA channel
> - *
> - * Return: '1' if idle, '0' if not.
> - */
> -static bool xilinx_dma_is_idle(struct xilinx_dma_chan *chan)
> -{
> - return dma_ctrl_read(chan, XILINX_DMA_REG_DMASR) &
> - XILINX_DMA_DMASR_IDLE;
> -}
> -
> -/**
>   * xilinx_dma_halt - Halt DMA channel
>   * @chan: Driver specific DMA channel
>   */
> @@ -966,6 +942,7 @@ static void xilinx_dma_halt(struct xilinx_dma_chan *chan)
>   chan, dma_ctrl_read(chan, XILINX_DMA_REG_DMASR));
>   chan->err = true;
>   }
> + chan->idle = true;
>  }
>  
>  /**
> @@ -1007,6 +984,9 @@ static void xilinx_vdma_start_transfer(struct 
> xilinx_dma_chan *chan)
>   if (chan->err)
>   return;
>  
> + if (!chan->idle)
> + return;
> +
>   if (list_empty(>pending_list))
>   return;
>  
> @@ -1018,13 +998,6 @@ static void xilinx_vdma_start_transfer(struct 
> xilinx_dma_chan *chan)
>   tail_segment = list_last_entry(_desc->segments,
>  struct xilinx_vdma_tx_segment, node);
>  
> - /* If it is SG mode and hardware is busy, cannot submit */
> - if (chan->has_sg && xilinx_dma_is_running(chan) &&
> - !xilinx_dma_is_idle(chan)) {
> - dev_dbg(chan->dev, "DMA controller still busy\n");
> - return;
> - }
> -
>   /*
>* If hardware is idle, then all descriptors on the running lists are
>* done, start new transfers
> @@ -1110,6 +1083,7 @@ static void xilinx_vdma_start_transfer(struct 
> xilinx_dma_chan *chan)
>   vdma_desc_write(chan, XILINX_DMA_REG_VSIZE, last->hw.vsize);
>   }
>  
> + chan->idle = false;
>   if (!chan->has_sg) {
>   list_del(>node);
>   list_add_tail(>node, >active_list);
> @@ -1136,6 +1110,9 @@ static void xilinx_cdma_start_transfer(struct 
> xilinx_dma_chan *chan)
>   if (chan->err)
>   return;
>  
> + if (!chan->idle)
> + return;
> +
>   if (list_empty(>pending_list))
>   return;
>  
> @@ -1181,6 +1158,7 @@ static void xilinx_cdma_start_transfer(struct 
> xilinx_dma_chan *chan)
>  
>   list_splice_tail_init(>pending_list, >active_list);
>   chan->desc_pendingcount = 0;
> + chan->idle = false;
>  }
>  
>  /**
> @@ -1196,15 +1174,11 @@ static void xilinx_dma_start_transfer(struct 
> xilinx_dma_chan *chan)
>   if (chan->err)
>   return;
>  
> - if (list_empty(>pending_list))
> + if (!chan->idle)
>   return;
>  
> - /* If it is SG mode and hardware is busy, cannot submit */
> - if (chan->has_sg && xilinx_dma_is_running(chan) &&
> -