| Message ID | 1567701424-25658-4-git-send-email-radhey.shyam.pandey@xilinx.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | AXI DMA driver improvements | expand |
On 05-09-19, 22:06, Radhey Shyam Pandey wrote: > From: Nicholas Graumann <nick.graumann@gmail.com> > > Introduce a function that can calculate residues for IPs that support it: > AXI DMA and CDMA. > > Signed-off-by: Nicholas Graumann <nick.graumann@gmail.com> > Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com> > --- > 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, &desc->segments) { > + if (chan->xdev->dma_config->dmatype == XDMA_TYPE_CDMA) { > + cdma_seg = list_entry(entry, > + struct xilinx_cdma_tx_segment, > + node); > + cdma_hw = &cdma_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 = &axidma_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(&chan->lock, flags); > + spin_lock_irqsave(&chan->lock, flags); > > - desc = list_last_entry(&chan->active_list, > - struct xilinx_dma_tx_descriptor, node); > - if (chan->has_sg) { > - list_for_each_entry(segment, &desc->segments, node) { > - hw = &segment->hw; > - residue += (hw->control - hw->status) & > - chan->xdev->max_buffer_len; > - } > - } > - spin_unlock_irqrestore(&chan->lock, flags); > + desc = list_last_entry(&chan->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(&chan->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 == XDMA_TYPE_CDMA) { > dma_cap_set(DMA_MEMCPY, xdev->common.cap_mask); > xdev->common.device_prep_dma_memcpy = xilinx_cdma_prep_memcpy; > + /* Residue calculation is supported by only AXI DMA and CDMA */ > + xdev->common.residue_granularity = > + DMA_RESIDUE_GRANULARITY_SEGMENT; > } else { > xdev->common.device_prep_interleaved_dma = > xilinx_vdma_dma_prep_interleaved; > -- > 2.7.4
> -----Original Message----- > From: Vinod Koul <vkoul@kernel.org> > Sent: Thursday, September 26, 2019 2:31 AM > To: Radhey Shyam Pandey <radheys@xilinx.com> > Cc: dan.j.williams@intel.com; Michal Simek <michals@xilinx.com>; > nick.graumann@gmail.com; andrea.merello@gmail.com; Appana Durga > Kedareswara Rao <appanad@xilinx.com>; mcgrof@kernel.org; > dmaengine@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 <nick.graumann@gmail.com> > > > > Introduce a function that can calculate residues for IPs that support it: > > AXI DMA and CDMA. > > > > Signed-off-by: Nicholas Graumann <nick.graumann@gmail.com> > > Signed-off-by: Radhey Shyam Pandey > <radhey.shyam.pandey@xilinx.com> > > --- > > 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, &desc->segments) { > > + if (chan->xdev->dma_config->dmatype == > XDMA_TYPE_CDMA) { > > + cdma_seg = list_entry(entry, > > + struct xilinx_cdma_tx_segment, > > + node); > > + cdma_hw = &cdma_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 = &axidma_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(&chan->lock, flags); > > + spin_lock_irqsave(&chan->lock, flags); > > > > - desc = list_last_entry(&chan->active_list, > > - struct xilinx_dma_tx_descriptor, node); > > - if (chan->has_sg) { > > - list_for_each_entry(segment, &desc->segments, > node) { > > - hw = &segment->hw; > > - residue += (hw->control - hw->status) & > > - chan->xdev->max_buffer_len; > > - } > > - } > > - spin_unlock_irqrestore(&chan->lock, flags); > > + desc = list_last_entry(&chan->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(&chan->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 == XDMA_TYPE_CDMA) { > > dma_cap_set(DMA_MEMCPY, xdev->common.cap_mask); > > xdev->common.device_prep_dma_memcpy = > xilinx_cdma_prep_memcpy; > > + /* Residue calculation is supported by only AXI DMA and > CDMA */ > > + xdev->common.residue_granularity = > > + > DMA_RESIDUE_GRANULARITY_SEGMENT; > > } else { > > xdev->common.device_prep_interleaved_dma = > > xilinx_vdma_dma_prep_interleaved; > > -- > > 2.7.4 > > -- > ~Vinod
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!
> -----Original Message----- > From: Vinod Koul <vkoul@kernel.org> > Sent: Thursday, September 26, 2019 10:48 PM > To: Radhey Shyam Pandey <radheys@xilinx.com> > Cc: dan.j.williams@intel.com; Michal Simek <michals@xilinx.com>; > nick.graumann@gmail.com; andrea.merello@gmail.com; Appana Durga > Kedareswara Rao <appanad@xilinx.com>; mcgrof@kernel.org; > dmaengine@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
> -----Original Message----- > From: Radhey Shyam Pandey > Sent: Friday, September 27, 2019 10:46 AM > To: Vinod Koul <vkoul@kernel.org> > Cc: dan.j.williams@intel.com; Michal Simek <michals@xilinx.com>; > nick.graumann@gmail.com; andrea.merello@gmail.com; Appana Durga > Kedareswara Rao <appanad@xilinx.com>; mcgrof@kernel.org; > dmaengine@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 <vkoul@kernel.org> > > Sent: Thursday, September 26, 2019 10:48 PM > > To: Radhey Shyam Pandey <radheys@xilinx.com> > > Cc: dan.j.williams@intel.com; Michal Simek <michals@xilinx.com>; > > nick.graumann@gmail.com; andrea.merello@gmail.com; Appana Durga > > Kedareswara Rao <appanad@xilinx.com>; mcgrof@kernel.org; > > dmaengine@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
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; + + /** + * 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; + + list_for_each(entry, &desc->segments) { + if (chan->xdev->dma_config->dmatype == XDMA_TYPE_CDMA) { + cdma_seg = list_entry(entry, + struct xilinx_cdma_tx_segment, + node); + cdma_hw = &cdma_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 = &axidma_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(&chan->lock, flags); + spin_lock_irqsave(&chan->lock, flags); - desc = list_last_entry(&chan->active_list, - struct xilinx_dma_tx_descriptor, node); - if (chan->has_sg) { - list_for_each_entry(segment, &desc->segments, node) { - hw = &segment->hw; - residue += (hw->control - hw->status) & - chan->xdev->max_buffer_len; - } - } - spin_unlock_irqrestore(&chan->lock, flags); + desc = list_last_entry(&chan->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(&chan->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 == XDMA_TYPE_CDMA) { dma_cap_set(DMA_MEMCPY, xdev->common.cap_mask); xdev->common.device_prep_dma_memcpy = xilinx_cdma_prep_memcpy; + /* Residue calculation is supported by only AXI DMA and CDMA */ + xdev->common.residue_granularity = + DMA_RESIDUE_GRANULARITY_SEGMENT; } else { xdev->common.device_prep_interleaved_dma = xilinx_vdma_dma_prep_interleaved;