RE: [PATCH -next 3/8] dmaengine: xilinx_dma: Introduce xilinx_dma_get_residue
> -Original Message- > From: Radhey Shyam Pandey > Sent: Friday, September 27, 2019 10:46 AM > To: Vinod Koul > Cc: dan.j.willi...@intel.com; Michal Simek ; > nick.graum...@gmail.com; andrea.mere...@gmail.com; Appana Durga > Kedareswara Rao ; mcg...@kernel.org; > dmaeng...@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: RE: [PATCH -next 3/8] dmaengine: xilinx_dma: Introduce > xilinx_dma_get_residue > > > -Original Message- > > From: Vinod Koul > > Sent: Thursday, September 26, 2019 10:48 PM > > To: Radhey Shyam Pandey > > Cc: dan.j.willi...@intel.com; Michal Simek ; > > nick.graum...@gmail.com; andrea.mere...@gmail.com; Appana Durga > > Kedareswara Rao ; mcg...@kernel.org; > > dmaeng...@vger.kernel.org; linux-kernel@vger.kernel.org > > Subject: Re: [PATCH -next 3/8] dmaengine: xilinx_dma: Introduce > > xilinx_dma_get_residue > > > > On 26-09-19, 05:52, Radhey Shyam Pandey wrote: > > > > > > > + * VDMA and simple mode do not support residue reporting, > so the > > > > > + * residue field will always be 0. > > > > > + */ > > > > > + if (chan->xdev->dma_config->dmatype == > XDMA_TYPE_VDMA || > > > > !chan->has_sg) > > > > > + return residue; > > > > > > > > why not check this in status callback? > > > Assuming we mean to move vdma and non-sg check to > > xilinx_dma_tx_status. > > > Just a thought- Keeping this check in xilinx_dma_get_residue > > > provides an abstraction and caller can simply call this func with > > > knowing about IP config specific residue calculation. Considering > > > this point does it looks ok ? > > > > well you are checking either way, so calling the lower level function > > only when you need it makes more sense! > > Sure, will do it in v2. Just noticed that xilinx_dma_get_residue() is called at the multiple places i.e one in device_tx_status and other in _complete_descriptor. To avoid code duplication, I will create a helper function to check if residue calculation is supported. > > > > -- > > ~Vinod
RE: [PATCH -next 3/8] dmaengine: xilinx_dma: Introduce xilinx_dma_get_residue
> -Original Message- > From: Vinod Koul > Sent: Thursday, September 26, 2019 10:48 PM > To: Radhey Shyam Pandey > Cc: dan.j.willi...@intel.com; Michal Simek ; > nick.graum...@gmail.com; andrea.mere...@gmail.com; Appana Durga > Kedareswara Rao ; mcg...@kernel.org; > dmaeng...@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH -next 3/8] dmaengine: xilinx_dma: Introduce > xilinx_dma_get_residue > > On 26-09-19, 05:52, Radhey Shyam Pandey wrote: > > > > > +* VDMA and simple mode do not support residue reporting, so the > > > > +* residue field will always be 0. > > > > +*/ > > > > + if (chan->xdev->dma_config->dmatype == XDMA_TYPE_VDMA || > > > !chan->has_sg) > > > > + return residue; > > > > > > why not check this in status callback? > > Assuming we mean to move vdma and non-sg check to > xilinx_dma_tx_status. > > Just a thought- Keeping this check in xilinx_dma_get_residue provides > > an abstraction and caller can simply call this func with knowing about > > IP config specific residue calculation. Considering this point does it > > looks ok ? > > well you are checking either way, so calling the lower level function > only when you need it makes more sense! Sure, will do it in v2. > > -- > ~Vinod
Re: [PATCH -next 3/8] dmaengine: xilinx_dma: Introduce xilinx_dma_get_residue
On 26-09-19, 05:52, Radhey Shyam Pandey wrote: > > > + * VDMA and simple mode do not support residue reporting, so the > > > + * residue field will always be 0. > > > + */ > > > + if (chan->xdev->dma_config->dmatype == XDMA_TYPE_VDMA || > > !chan->has_sg) > > > + return residue; > > > > why not check this in status callback? > Assuming we mean to move vdma and non-sg check to xilinx_dma_tx_status. > Just a thought- Keeping this check in xilinx_dma_get_residue provides > an abstraction and caller can simply call this func with knowing about > IP config specific residue calculation. Considering this point does it > looks ok ? well you are checking either way, so calling the lower level function only when you need it makes more sense! -- ~Vinod
RE: [PATCH -next 3/8] dmaengine: xilinx_dma: Introduce xilinx_dma_get_residue
> -Original Message- > From: Vinod Koul > Sent: Thursday, September 26, 2019 2:31 AM > To: Radhey Shyam Pandey > Cc: dan.j.willi...@intel.com; Michal Simek ; > nick.graum...@gmail.com; andrea.mere...@gmail.com; Appana Durga > Kedareswara Rao ; mcg...@kernel.org; > dmaeng...@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH -next 3/8] dmaengine: xilinx_dma: Introduce > xilinx_dma_get_residue > > On 05-09-19, 22:06, Radhey Shyam Pandey wrote: > > From: Nicholas Graumann > > > > Introduce a function that can calculate residues for IPs that support it: > > AXI DMA and CDMA. > > > > Signed-off-by: Nicholas Graumann > > Signed-off-by: Radhey Shyam Pandey > > > --- > > drivers/dma/xilinx/xilinx_dma.c | 75 > > ++--- > > 1 file changed, 56 insertions(+), 19 deletions(-) > > > > diff --git a/drivers/dma/xilinx/xilinx_dma.c > > b/drivers/dma/xilinx/xilinx_dma.c index 9909bfb..4094adb 100644 > > --- a/drivers/dma/xilinx/xilinx_dma.c > > +++ b/drivers/dma/xilinx/xilinx_dma.c > > @@ -787,6 +787,51 @@ static void xilinx_dma_free_chan_resources(struct > > dma_chan *dchan) } > > > > /** > > + * xilinx_dma_get_residue - Compute residue for a given descriptor > > + * @chan: Driver specific dma channel > > + * @desc: dma transaction descriptor > > + * > > + * Return: The number of residue bytes for the descriptor. > > + */ > > +static u32 xilinx_dma_get_residue(struct xilinx_dma_chan *chan, > > + struct xilinx_dma_tx_descriptor *desc) { > > + struct xilinx_cdma_tx_segment *cdma_seg; > > + struct xilinx_axidma_tx_segment *axidma_seg; > > + struct xilinx_cdma_desc_hw *cdma_hw; > > + struct xilinx_axidma_desc_hw *axidma_hw; > > + struct list_head *entry; > > + u32 residue = 0; > > + > > + /** > > it should be: > /* > * comment... > Thanks for pointing it out. I will fix it in v2. > > +* VDMA and simple mode do not support residue reporting, so the > > +* residue field will always be 0. > > +*/ > > + if (chan->xdev->dma_config->dmatype == XDMA_TYPE_VDMA || > !chan->has_sg) > > + return residue; > > why not check this in status callback? Assuming we mean to move vdma and non-sg check to xilinx_dma_tx_status. Just a thought- Keeping this check in xilinx_dma_get_residue provides an abstraction and caller can simply call this func with knowing about IP config specific residue calculation. Considering this point does it looks ok ? > > > + > > + list_for_each(entry, >segments) { > > + if (chan->xdev->dma_config->dmatype == > XDMA_TYPE_CDMA) { > > + cdma_seg = list_entry(entry, > > + struct xilinx_cdma_tx_segment, > > + node); > > + cdma_hw = _seg->hw; > > + residue += (cdma_hw->control - cdma_hw->status) > & > > + chan->xdev->max_buffer_len; > > + } else { > > + axidma_seg = list_entry(entry, > > + struct > xilinx_axidma_tx_segment, > > + node); > > + axidma_hw = _seg->hw; > > + residue += (axidma_hw->control - axidma_hw- > >status) & > > + chan->xdev->max_buffer_len; > > + } > > + } > > + > > + return residue; > > +} > > + > > +/** > > * xilinx_dma_chan_handle_cyclic - Cyclic dma callback > > * @chan: Driver specific dma channel > > * @desc: dma transaction descriptor > > @@ -995,33 +1040,22 @@ static enum dma_status > > xilinx_dma_tx_status(struct dma_chan *dchan, { > > struct xilinx_dma_chan *chan = to_xilinx_chan(dchan); > > struct xilinx_dma_tx_descriptor *desc; > > - struct xilinx_axidma_tx_segment *segment; > > - struct xilinx_axidma_desc_hw *hw; > > enum dma_status ret; > > unsigned long flags; > > - u32 residue = 0; > > > > ret = dma_cookie_status(dchan, cookie, txstate); > > if (ret == DMA_COMPLETE || !txstate) > > return ret; > > > > - if (chan->xdev->dma_config->dmatype == XDMA_TYPE_AXIDMA) { > > - spin_lock_irqsave(>lock, flags); > > + spin_lock_irqsave(>lock, flags); > &g
Re: [PATCH -next 3/8] dmaengine: xilinx_dma: Introduce xilinx_dma_get_residue
On 05-09-19, 22:06, Radhey Shyam Pandey wrote: > From: Nicholas Graumann > > Introduce a function that can calculate residues for IPs that support it: > AXI DMA and CDMA. > > Signed-off-by: Nicholas Graumann > Signed-off-by: Radhey Shyam Pandey > --- > drivers/dma/xilinx/xilinx_dma.c | 75 > ++--- > 1 file changed, 56 insertions(+), 19 deletions(-) > > diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c > index 9909bfb..4094adb 100644 > --- a/drivers/dma/xilinx/xilinx_dma.c > +++ b/drivers/dma/xilinx/xilinx_dma.c > @@ -787,6 +787,51 @@ static void xilinx_dma_free_chan_resources(struct > dma_chan *dchan) > } > > /** > + * xilinx_dma_get_residue - Compute residue for a given descriptor > + * @chan: Driver specific dma channel > + * @desc: dma transaction descriptor > + * > + * Return: The number of residue bytes for the descriptor. > + */ > +static u32 xilinx_dma_get_residue(struct xilinx_dma_chan *chan, > + struct xilinx_dma_tx_descriptor *desc) > +{ > + struct xilinx_cdma_tx_segment *cdma_seg; > + struct xilinx_axidma_tx_segment *axidma_seg; > + struct xilinx_cdma_desc_hw *cdma_hw; > + struct xilinx_axidma_desc_hw *axidma_hw; > + struct list_head *entry; > + u32 residue = 0; > + > + /** it should be: /* * comment... > + * VDMA and simple mode do not support residue reporting, so the > + * residue field will always be 0. > + */ > + if (chan->xdev->dma_config->dmatype == XDMA_TYPE_VDMA || !chan->has_sg) > + return residue; why not check this in status callback? > + > + list_for_each(entry, >segments) { > + if (chan->xdev->dma_config->dmatype == XDMA_TYPE_CDMA) { > + cdma_seg = list_entry(entry, > + struct xilinx_cdma_tx_segment, > + node); > + cdma_hw = _seg->hw; > + residue += (cdma_hw->control - cdma_hw->status) & > +chan->xdev->max_buffer_len; > + } else { > + axidma_seg = list_entry(entry, > + struct xilinx_axidma_tx_segment, > + node); > + axidma_hw = _seg->hw; > + residue += (axidma_hw->control - axidma_hw->status) & > +chan->xdev->max_buffer_len; > + } > + } > + > + return residue; > +} > + > +/** > * xilinx_dma_chan_handle_cyclic - Cyclic dma callback > * @chan: Driver specific dma channel > * @desc: dma transaction descriptor > @@ -995,33 +1040,22 @@ static enum dma_status xilinx_dma_tx_status(struct > dma_chan *dchan, > { > struct xilinx_dma_chan *chan = to_xilinx_chan(dchan); > struct xilinx_dma_tx_descriptor *desc; > - struct xilinx_axidma_tx_segment *segment; > - struct xilinx_axidma_desc_hw *hw; > enum dma_status ret; > unsigned long flags; > - u32 residue = 0; > > ret = dma_cookie_status(dchan, cookie, txstate); > if (ret == DMA_COMPLETE || !txstate) > return ret; > > - if (chan->xdev->dma_config->dmatype == XDMA_TYPE_AXIDMA) { > - spin_lock_irqsave(>lock, flags); > + spin_lock_irqsave(>lock, flags); > > - desc = list_last_entry(>active_list, > -struct xilinx_dma_tx_descriptor, node); > - if (chan->has_sg) { > - list_for_each_entry(segment, >segments, node) { > - hw = >hw; > - residue += (hw->control - hw->status) & > -chan->xdev->max_buffer_len; > - } > - } > - spin_unlock_irqrestore(>lock, flags); > + desc = list_last_entry(>active_list, > +struct xilinx_dma_tx_descriptor, node); > + chan->residue = xilinx_dma_get_residue(chan, desc); > > - chan->residue = residue; > - dma_set_residue(txstate, chan->residue); > - } > + spin_unlock_irqrestore(>lock, flags); > + > + dma_set_residue(txstate, chan->residue); > > return ret; > } > @@ -2701,12 +2735,15 @@ static int xilinx_dma_probe(struct platform_device > *pdev) > xilinx_dma_prep_dma_cyclic; > xdev->common.device_prep_interleaved_dma = > xilinx_dma_prep_interleaved; > - /* Residue calculation is supported by only AXI DMA */ > + /* Residue calculation is supported by only AXI DMA and CDMA */ > xdev->common.residue_granularity = > DMA_RESIDUE_GRANULARITY_SEGMENT; > } else if (xdev->dma_config->dmatype