* [1/6] dmaengine: xilinx_dma: fix splitting transfer causes misalignments
@ 2018-06-20 8:36 Andrea Merello
0 siblings, 0 replies; 3+ messages in thread
From: Andrea Merello @ 2018-06-20 8:36 UTC (permalink / raw)
To: vkoul, dan.j.williams, michal.simek, appana.durga.rao, dmaengine
Cc: linux-arm-kernel, linux-kernel, Andrea Merello
Whenever a single or cyclic transaction is prepared, the driver
could eventually split it over several SG descriptors in order
to deal with the HW maximum transfer length.
This could end up in DMA operations starting from a misaligned
address. This seems fatal for the HW.
This patch eventually adjusts the transfer size in order to make sure
all operations start from an aligned address.
Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
---
drivers/dma/xilinx/xilinx_dma.c | 27 ++++++++++++++++++++-------
1 file changed, 20 insertions(+), 7 deletions(-)
diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
index 27b523530c4a..a516e7ffef21 100644
--- a/drivers/dma/xilinx/xilinx_dma.c
+++ b/drivers/dma/xilinx/xilinx_dma.c
@@ -376,6 +376,7 @@ struct xilinx_dma_chan {
void (*start_transfer)(struct xilinx_dma_chan *chan);
int (*stop_transfer)(struct xilinx_dma_chan *chan);
u16 tdest;
+ u32 copy_mask;
};
/**
@@ -1789,10 +1790,14 @@ static struct dma_async_tx_descriptor *xilinx_dma_prep_slave_sg(
/*
* Calculate the maximum number of bytes to transfer,
- * making sure it is less than the hw limit
+ * making sure it is less than the hw limit and that
+ * the next chuck start address is aligned
*/
- copy = min_t(size_t, sg_dma_len(sg) - sg_used,
- XILINX_DMA_MAX_TRANS_LEN);
+ copy = sg_dma_len(sg) - sg_used;
+ if (copy > XILINX_DMA_MAX_TRANS_LEN)
+ copy = XILINX_DMA_MAX_TRANS_LEN &
+ chan->copy_mask;
+
hw = &segment->hw;
/* Fill in the descriptor */
@@ -1894,10 +1899,14 @@ static struct dma_async_tx_descriptor *xilinx_dma_prep_dma_cyclic(
/*
* Calculate the maximum number of bytes to transfer,
- * making sure it is less than the hw limit
+ * making sure it is less than the hw limit and that
+ * the next chuck start address is aligned
*/
- copy = min_t(size_t, period_len - sg_used,
- XILINX_DMA_MAX_TRANS_LEN);
+ copy = period_len - sg_used;
+ if (copy > XILINX_DMA_MAX_TRANS_LEN)
+ copy = XILINX_DMA_MAX_TRANS_LEN &
+ chan->copy_mask;
+
hw = &segment->hw;
xilinx_axidma_buf(chan, hw, buf_addr, sg_used,
period_len * i);
@@ -2402,8 +2411,12 @@ static int xilinx_dma_chan_probe(struct xilinx_dma_device *xdev,
if (width > 8)
has_dre = false;
- if (!has_dre)
+ if (has_dre) {
+ chan->copy_mask = ~0;
+ } else {
xdev->common.copy_align = fls(width - 1);
+ chan->copy_mask = ~(width - 1);
+ }
if (of_device_is_compatible(node, "xlnx,axi-vdma-mm2s-channel") ||
of_device_is_compatible(node, "xlnx,axi-dma-mm2s-channel") ||
^ permalink raw reply related [flat|nested] 3+ messages in thread
* [1/6] dmaengine: xilinx_dma: fix splitting transfer causes misalignments
@ 2018-06-20 11:37 Radhey Shyam Pandey
0 siblings, 0 replies; 3+ messages in thread
From: Radhey Shyam Pandey @ 2018-06-20 11:37 UTC (permalink / raw)
To: Andrea Merello, vkoul@kernel.org, dan.j.williams@intel.com,
Michal Simek, Appana Durga Kedareswara Rao,
dmaengine@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
> -----Original Message-----
> From: dmaengine-owner@vger.kernel.org [mailto:dmaengine-
> owner@vger.kernel.org] On Behalf Of Andrea Merello
> Sent: Wednesday, June 20, 2018 2:07 PM
> To: vkoul@kernel.org; dan.j.williams@intel.com; Michal Simek
> <michals@xilinx.com>; Appana Durga Kedareswara Rao
> <appanad@xilinx.com>; dmaengine@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> Andrea Merello <andrea.merello@gmail.com>
> Subject: [PATCH 1/6] dmaengine: xilinx_dma: fix splitting transfer causes
> misalignments
We should rephrase commit message to something like "In axidma
slave_sg and dma_cylic mode align split descriptors"
>
> Whenever a single or cyclic transaction is prepared, the driver
> could eventually split it over several SG descriptors in order
> to deal with the HW maximum transfer length.
>
> This could end up in DMA operations starting from a misaligned
> address. This seems fatal for the HW.
This seems fatal for the HW if DRE is not enabled.
>
> This patch eventually adjusts the transfer size in order to make sure
> all operations start from an aligned address.
>
> Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
> ---
> drivers/dma/xilinx/xilinx_dma.c | 27 ++++++++++++++++++++-------
> 1 file changed, 20 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
> index 27b523530c4a..a516e7ffef21 100644
> --- a/drivers/dma/xilinx/xilinx_dma.c
> +++ b/drivers/dma/xilinx/xilinx_dma.c
> @@ -376,6 +376,7 @@ struct xilinx_dma_chan {
> void (*start_transfer)(struct xilinx_dma_chan *chan);
> int (*stop_transfer)(struct xilinx_dma_chan *chan);
> u16 tdest;
> + u32 copy_mask;
We can reuse copy_align itself. See below.
> };
>
> /**
> @@ -1789,10 +1790,14 @@ static struct dma_async_tx_descriptor
> *xilinx_dma_prep_slave_sg(
>
> /*
> * Calculate the maximum number of bytes to transfer,
> - * making sure it is less than the hw limit
> + * making sure it is less than the hw limit and that
> + * the next chuck start address is aligned
/s/chuck/chunk
> */
> - copy = min_t(size_t, sg_dma_len(sg) - sg_used,
> - XILINX_DMA_MAX_TRANS_LEN);
> + copy = sg_dma_len(sg) - sg_used;
> + if (copy > XILINX_DMA_MAX_TRANS_LEN)
> + copy = XILINX_DMA_MAX_TRANS_LEN &
> + chan->copy_mask;
> +
In below implementation, we can reuse copy_align.
Same for dma_cyclic.
if ((copy + sg_used < sg_dma_len(sg)) &&
chan->xdev->common.copy_align) {
/* If this is not the last descriptor, make sure
* the next one will be properly aligned
*/
copy = rounddown(copy,
(1 << chan->xdev->common.copy_align));
}
> hw = &segment->hw;
>
> /* Fill in the descriptor */
> @@ -1894,10 +1899,14 @@ static struct dma_async_tx_descriptor
> *xilinx_dma_prep_dma_cyclic(
>
> /*
> * Calculate the maximum number of bytes to transfer,
> - * making sure it is less than the hw limit
> + * making sure it is less than the hw limit and that
> + * the next chuck start address is aligned
> */
> - copy = min_t(size_t, period_len - sg_used,
> - XILINX_DMA_MAX_TRANS_LEN);
> + copy = period_len - sg_used;
> + if (copy > XILINX_DMA_MAX_TRANS_LEN)
> + copy = XILINX_DMA_MAX_TRANS_LEN &
> + chan->copy_mask;
> +
> hw = &segment->hw;
> xilinx_axidma_buf(chan, hw, buf_addr, sg_used,
> period_len * i);
> @@ -2402,8 +2411,12 @@ static int xilinx_dma_chan_probe(struct
> xilinx_dma_device *xdev,
> if (width > 8)
> has_dre = false;
>
> - if (!has_dre)
> + if (has_dre) {
> + chan->copy_mask = ~0;
> + } else {
> xdev->common.copy_align = fls(width - 1);
> + chan->copy_mask = ~(width - 1);
> + }
As mentioned above we don't need this additional field.
>
> if (of_device_is_compatible(node, "xlnx,axi-vdma-mm2s-channel") ||
> of_device_is_compatible(node, "xlnx,axi-dma-mm2s-channel") ||
> --
> 2.17.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe dmaengine" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
---
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 3+ messages in thread
* [1/6] dmaengine: xilinx_dma: fix splitting transfer causes misalignments
@ 2018-06-20 13:15 Andrea Merello
0 siblings, 0 replies; 3+ messages in thread
From: Andrea Merello @ 2018-06-20 13:15 UTC (permalink / raw)
To: Radhey Shyam Pandey
Cc: vkoul@kernel.org, dan.j.williams@intel.com, Michal Simek,
Appana Durga Kedareswara Rao, dmaengine@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
On Wed, Jun 20, 2018 at 1:37 PM, Radhey Shyam Pandey <radheys@xilinx.com> wrote:
>> -----Original Message-----
>> From: dmaengine-owner@vger.kernel.org [mailto:dmaengine-
>> owner@vger.kernel.org] On Behalf Of Andrea Merello
>> Sent: Wednesday, June 20, 2018 2:07 PM
>> To: vkoul@kernel.org; dan.j.williams@intel.com; Michal Simek
>> <michals@xilinx.com>; Appana Durga Kedareswara Rao
>> <appanad@xilinx.com>; dmaengine@vger.kernel.org
>> Cc: linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
>> Andrea Merello <andrea.merello@gmail.com>
>> Subject: [PATCH 1/6] dmaengine: xilinx_dma: fix splitting transfer causes
>> misalignments
>
> We should rephrase commit message to something like "In axidma
> slave_sg and dma_cylic mode align split descriptors"
OK
>>
>> Whenever a single or cyclic transaction is prepared, the driver
>> could eventually split it over several SG descriptors in order
>> to deal with the HW maximum transfer length.
>>
>> This could end up in DMA operations starting from a misaligned
>> address. This seems fatal for the HW.
> This seems fatal for the HW if DRE is not enabled.
OK
>>
>> This patch eventually adjusts the transfer size in order to make sure
>> all operations start from an aligned address.
>>
>> Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
>> ---
>> drivers/dma/xilinx/xilinx_dma.c | 27 ++++++++++++++++++++-------
>> 1 file changed, 20 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
>> index 27b523530c4a..a516e7ffef21 100644
>> --- a/drivers/dma/xilinx/xilinx_dma.c
>> +++ b/drivers/dma/xilinx/xilinx_dma.c
>> @@ -376,6 +376,7 @@ struct xilinx_dma_chan {
>> void (*start_transfer)(struct xilinx_dma_chan *chan);
>> int (*stop_transfer)(struct xilinx_dma_chan *chan);
>> u16 tdest;
>> + u32 copy_mask;
> We can reuse copy_align itself. See below.
OK
>> };
>>
>> /**
>> @@ -1789,10 +1790,14 @@ static struct dma_async_tx_descriptor
>> *xilinx_dma_prep_slave_sg(
>>
>> /*
>> * Calculate the maximum number of bytes to transfer,
>> - * making sure it is less than the hw limit
>> + * making sure it is less than the hw limit and that
>> + * the next chuck start address is aligned
>
> /s/chuck/chunk
OK
>> */
>> - copy = min_t(size_t, sg_dma_len(sg) - sg_used,
>> - XILINX_DMA_MAX_TRANS_LEN);
>> + copy = sg_dma_len(sg) - sg_used;
>> + if (copy > XILINX_DMA_MAX_TRANS_LEN)
>> + copy = XILINX_DMA_MAX_TRANS_LEN &
>> + chan->copy_mask;
>> +
>
>
> In below implementation, we can reuse copy_align.
> Same for dma_cyclic.
>
> if ((copy + sg_used < sg_dma_len(sg)) &&
> chan->xdev->common.copy_align) {
> /* If this is not the last descriptor, make sure
> * the next one will be properly aligned
> */
> copy = rounddown(copy,
> (1 << chan->xdev->common.copy_align));
> }
OK for the general idea. But to me it seems a bit more complicated than needed:
What's the point in setting 'copy' with min_t, performing also the
subtraction sg_dma_len(sg) - sg_used, and then add sg_used again? What
about something like:
- copy = min_t(size_t, sg_dma_len(sg) - sg_used,
- XILINX_DMA_MAX_TRANS_LEN);
+ copy = sg_dma_len(sg) - sg_used;
+ if (copy > XILINX_DMA_MAX_TRANS_LEN &&
+ chan->xdev->common.copy_align)
+ copy = rounddown(XILINX_DMA_MAX_TRANS_LEN,
+ (1 << chan->xdev->common.copy_align));
+
>> hw = &segment->hw;
>>
>> /* Fill in the descriptor */
>> @@ -1894,10 +1899,14 @@ static struct dma_async_tx_descriptor
>> *xilinx_dma_prep_dma_cyclic(
>>
>> /*
>> * Calculate the maximum number of bytes to transfer,
>> - * making sure it is less than the hw limit
>> + * making sure it is less than the hw limit and that
>> + * the next chuck start address is aligned
>> */
>> - copy = min_t(size_t, period_len - sg_used,
>> - XILINX_DMA_MAX_TRANS_LEN);
>> + copy = period_len - sg_used;
>> + if (copy > XILINX_DMA_MAX_TRANS_LEN)
>> + copy = XILINX_DMA_MAX_TRANS_LEN &
>> + chan->copy_mask;
>> +
>> hw = &segment->hw;
>> xilinx_axidma_buf(chan, hw, buf_addr, sg_used,
>> period_len * i);
>> @@ -2402,8 +2411,12 @@ static int xilinx_dma_chan_probe(struct
>> xilinx_dma_device *xdev,
>> if (width > 8)
>> has_dre = false;
>>
>> - if (!has_dre)
>> + if (has_dre) {
>> + chan->copy_mask = ~0;
>> + } else {
>> xdev->common.copy_align = fls(width - 1);
>> + chan->copy_mask = ~(width - 1);
>> + }
>
> As mentioned above we don't need this additional field.
OK
>>
>> if (of_device_is_compatible(node, "xlnx,axi-vdma-mm2s-channel") ||
>> of_device_is_compatible(node, "xlnx,axi-dma-mm2s-channel") ||
>> --
>> 2.17.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe dmaengine" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
---
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-06-20 13:15 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-06-20 8:36 [1/6] dmaengine: xilinx_dma: fix splitting transfer causes misalignments Andrea Merello
-- strict thread matches above, loose matches on Subject: below --
2018-06-20 11:37 Radhey Shyam Pandey
2018-06-20 13:15 Andrea Merello
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).