* [PATCH] dmaengine: stm32-mdma: avoid 64-bit division
@ 2017-10-11 14:01 Arnd Bergmann
2017-10-11 14:27 ` Benjamin Gaignard
0 siblings, 1 reply; 6+ messages in thread
From: Arnd Bergmann @ 2017-10-11 14:01 UTC (permalink / raw)
To: Vinod Koul, Maxime Coquelin, Alexandre Torgue
Cc: Arnd Bergmann, Dan Williams, Pierre-Yves MORDRET,
M'boumba Cedric Madianga, dmaengine, linux-arm-kernel,
linux-kernel
When building with a 64-bit dma_addr_t, we run into a link
error:
drivers/dma/stm32-mdma.o: In function `stm32_mdma_prep_dma_memcpy':
stm32-mdma.c:(.text+0x16a3): undefined reference to `__umoddi3'
Using a 64-bit division here is way too expensive, since the
divisor is a known power-of-two value in reality. This moves
the modulo operation into stm32_mdma_get_max_width(), where
the compiler can optimize out that code, and we can use a 32-bit
division to be on the safe side.
Fixes: a4ffb13c8946 ("dmaengine: Add STM32 MDMA driver")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/dma/stm32-mdma.c | 27 ++++++++++++---------------
1 file changed, 12 insertions(+), 15 deletions(-)
diff --git a/drivers/dma/stm32-mdma.c b/drivers/dma/stm32-mdma.c
index 0db59a7e80e0..55151c2c9fae 100644
--- a/drivers/dma/stm32-mdma.c
+++ b/drivers/dma/stm32-mdma.c
@@ -387,7 +387,9 @@ static int stm32_mdma_get_width(struct stm32_mdma_chan *chan,
}
}
-static enum dma_slave_buswidth stm32_mdma_get_max_width(u32 buf_len, u32 tlen)
+static enum dma_slave_buswidth stm32_mdma_get_max_width(u32 buf_len,
+ u32 addr,
+ u32 tlen)
{
enum dma_slave_buswidth max_width = DMA_SLAVE_BUSWIDTH_8_BYTES;
@@ -398,6 +400,9 @@ static enum dma_slave_buswidth stm32_mdma_get_max_width(u32 buf_len, u32 tlen)
break;
}
+ if (addr % max_width)
+ max_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+
return max_width;
}
@@ -567,7 +572,7 @@ static int stm32_mdma_set_xfer_param(struct stm32_mdma_chan *chan,
ctcr |= STM32_MDMA_CTCR_DBURST((ilog2(dst_best_burst)));
/* Set memory data size */
- src_addr_width = stm32_mdma_get_max_width(buf_len, tlen);
+ src_addr_width = stm32_mdma_get_max_width(buf_len, 0, tlen);
chan->mem_width = src_addr_width;
src_bus_width = stm32_mdma_get_width(chan, src_addr_width);
if (src_bus_width < 0)
@@ -611,7 +616,7 @@ static int stm32_mdma_set_xfer_param(struct stm32_mdma_chan *chan,
ctcr |= STM32_MDMA_CTCR_SBURST((ilog2(src_best_burst)));
/* Set memory data size */
- dst_addr_width = stm32_mdma_get_max_width(buf_len, tlen);
+ dst_addr_width = stm32_mdma_get_max_width(buf_len, 0, tlen);
chan->mem_width = dst_addr_width;
dst_bus_width = stm32_mdma_get_width(chan, dst_addr_width);
if (dst_bus_width < 0)
@@ -956,9 +961,7 @@ stm32_mdma_prep_dma_memcpy(struct dma_chan *c, dma_addr_t dest, dma_addr_t src,
ctcr |= STM32_MDMA_CTCR_TLEN((tlen - 1));
/* Set source best burst size */
- max_width = stm32_mdma_get_max_width(len, tlen);
- if (src % max_width)
- max_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+ max_width = stm32_mdma_get_max_width(len, src, tlen);
src_bus_width = stm32_mdma_get_width(chan, max_width);
max_burst = tlen / max_width;
@@ -971,9 +974,7 @@ stm32_mdma_prep_dma_memcpy(struct dma_chan *c, dma_addr_t dest, dma_addr_t src,
STM32_MDMA_CTCR_SINCOS(src_bus_width);
/* Set destination best burst size */
- max_width = stm32_mdma_get_max_width(len, tlen);
- if (dest % max_width)
- max_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+ max_width = stm32_mdma_get_max_width(len, dest, tlen);
dst_bus_width = stm32_mdma_get_width(chan, max_width);
max_burst = tlen / max_width;
@@ -1014,9 +1015,7 @@ stm32_mdma_prep_dma_memcpy(struct dma_chan *c, dma_addr_t dest, dma_addr_t src,
STM32_MDMA_MAX_BLOCK_LEN);
/* Set source best burst size */
- max_width = stm32_mdma_get_max_width(len, tlen);
- if (src % max_width)
- max_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+ max_width = stm32_mdma_get_max_width(len, src, tlen);
src_bus_width = stm32_mdma_get_width(chan, max_width);
max_burst = tlen / max_width;
@@ -1030,9 +1029,7 @@ stm32_mdma_prep_dma_memcpy(struct dma_chan *c, dma_addr_t dest, dma_addr_t src,
STM32_MDMA_CTCR_SINCOS(src_bus_width);
/* Set destination best burst size */
- max_width = stm32_mdma_get_max_width(len, tlen);
- if (dest % max_width)
- max_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+ max_width = stm32_mdma_get_max_width(len, dest, tlen);
dst_bus_width = stm32_mdma_get_width(chan, max_width);
max_burst = tlen / max_width;
--
2.9.0
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] dmaengine: stm32-mdma: avoid 64-bit division
2017-10-11 14:01 [PATCH] dmaengine: stm32-mdma: avoid 64-bit division Arnd Bergmann
@ 2017-10-11 14:27 ` Benjamin Gaignard
2017-10-11 14:39 ` Arnd Bergmann
0 siblings, 1 reply; 6+ messages in thread
From: Benjamin Gaignard @ 2017-10-11 14:27 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Vinod Koul, Maxime Coquelin, Alexandre Torgue,
Linux Kernel Mailing List, Pierre-Yves MORDRET, dmaengine,
M'boumba Cedric Madianga, Dan Williams, linux-arm-kernel
2017-10-11 16:01 GMT+02:00 Arnd Bergmann <arnd@arndb.de>:
> When building with a 64-bit dma_addr_t, we run into a link
> error:
>
> drivers/dma/stm32-mdma.o: In function `stm32_mdma_prep_dma_memcpy':
> stm32-mdma.c:(.text+0x16a3): undefined reference to `__umoddi3'
>
> Using a 64-bit division here is way too expensive, since the
> divisor is a known power-of-two value in reality. This moves
> the modulo operation into stm32_mdma_get_max_width(), where
> the compiler can optimize out that code, and we can use a 32-bit
> division to be on the safe side.
>
> Fixes: a4ffb13c8946 ("dmaengine: Add STM32 MDMA driver")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> drivers/dma/stm32-mdma.c | 27 ++++++++++++---------------
> 1 file changed, 12 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/dma/stm32-mdma.c b/drivers/dma/stm32-mdma.c
> index 0db59a7e80e0..55151c2c9fae 100644
> --- a/drivers/dma/stm32-mdma.c
> +++ b/drivers/dma/stm32-mdma.c
> @@ -387,7 +387,9 @@ static int stm32_mdma_get_width(struct stm32_mdma_chan *chan,
> }
> }
>
> -static enum dma_slave_buswidth stm32_mdma_get_max_width(u32 buf_len, u32 tlen)
> +static enum dma_slave_buswidth stm32_mdma_get_max_width(u32 buf_len,
> + u32 addr,
> + u32 tlen)
> {
> enum dma_slave_buswidth max_width = DMA_SLAVE_BUSWIDTH_8_BYTES;
>
> @@ -398,6 +400,9 @@ static enum dma_slave_buswidth stm32_mdma_get_max_width(u32 buf_len, u32 tlen)
> break;
> }
>
> + if (addr % max_width)
> + max_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
> +
I'm only half-convince by the implicite 32 bits cast done into
function prototype.
If we keep using dma_addr_t and use do_div() instead of %
does compiler can still optimize the code ?
> return max_width;
> }
>
> @@ -567,7 +572,7 @@ static int stm32_mdma_set_xfer_param(struct stm32_mdma_chan *chan,
> ctcr |= STM32_MDMA_CTCR_DBURST((ilog2(dst_best_burst)));
>
> /* Set memory data size */
> - src_addr_width = stm32_mdma_get_max_width(buf_len, tlen);
> + src_addr_width = stm32_mdma_get_max_width(buf_len, 0, tlen);
> chan->mem_width = src_addr_width;
> src_bus_width = stm32_mdma_get_width(chan, src_addr_width);
> if (src_bus_width < 0)
> @@ -611,7 +616,7 @@ static int stm32_mdma_set_xfer_param(struct stm32_mdma_chan *chan,
> ctcr |= STM32_MDMA_CTCR_SBURST((ilog2(src_best_burst)));
>
> /* Set memory data size */
> - dst_addr_width = stm32_mdma_get_max_width(buf_len, tlen);
> + dst_addr_width = stm32_mdma_get_max_width(buf_len, 0, tlen);
> chan->mem_width = dst_addr_width;
> dst_bus_width = stm32_mdma_get_width(chan, dst_addr_width);
> if (dst_bus_width < 0)
> @@ -956,9 +961,7 @@ stm32_mdma_prep_dma_memcpy(struct dma_chan *c, dma_addr_t dest, dma_addr_t src,
> ctcr |= STM32_MDMA_CTCR_TLEN((tlen - 1));
>
> /* Set source best burst size */
> - max_width = stm32_mdma_get_max_width(len, tlen);
> - if (src % max_width)
> - max_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
> + max_width = stm32_mdma_get_max_width(len, src, tlen);
> src_bus_width = stm32_mdma_get_width(chan, max_width);
>
> max_burst = tlen / max_width;
> @@ -971,9 +974,7 @@ stm32_mdma_prep_dma_memcpy(struct dma_chan *c, dma_addr_t dest, dma_addr_t src,
> STM32_MDMA_CTCR_SINCOS(src_bus_width);
>
> /* Set destination best burst size */
> - max_width = stm32_mdma_get_max_width(len, tlen);
> - if (dest % max_width)
> - max_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
> + max_width = stm32_mdma_get_max_width(len, dest, tlen);
> dst_bus_width = stm32_mdma_get_width(chan, max_width);
>
> max_burst = tlen / max_width;
> @@ -1014,9 +1015,7 @@ stm32_mdma_prep_dma_memcpy(struct dma_chan *c, dma_addr_t dest, dma_addr_t src,
> STM32_MDMA_MAX_BLOCK_LEN);
>
> /* Set source best burst size */
> - max_width = stm32_mdma_get_max_width(len, tlen);
> - if (src % max_width)
> - max_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
> + max_width = stm32_mdma_get_max_width(len, src, tlen);
> src_bus_width = stm32_mdma_get_width(chan, max_width);
>
> max_burst = tlen / max_width;
> @@ -1030,9 +1029,7 @@ stm32_mdma_prep_dma_memcpy(struct dma_chan *c, dma_addr_t dest, dma_addr_t src,
> STM32_MDMA_CTCR_SINCOS(src_bus_width);
>
> /* Set destination best burst size */
> - max_width = stm32_mdma_get_max_width(len, tlen);
> - if (dest % max_width)
> - max_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
> + max_width = stm32_mdma_get_max_width(len, dest, tlen);
> dst_bus_width = stm32_mdma_get_width(chan, max_width);
>
> max_burst = tlen / max_width;
> --
> 2.9.0
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
Benjamin Gaignard
Graphic Study Group
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] dmaengine: stm32-mdma: avoid 64-bit division
2017-10-11 14:27 ` Benjamin Gaignard
@ 2017-10-11 14:39 ` Arnd Bergmann
2017-10-11 14:46 ` Benjamin Gaignard
0 siblings, 1 reply; 6+ messages in thread
From: Arnd Bergmann @ 2017-10-11 14:39 UTC (permalink / raw)
To: Benjamin Gaignard
Cc: Vinod Koul, Maxime Coquelin, Alexandre Torgue,
Linux Kernel Mailing List, Pierre-Yves MORDRET, dmaengine,
M'boumba Cedric Madianga, Dan Williams, Linux ARM
On Wed, Oct 11, 2017 at 4:27 PM, Benjamin Gaignard
<benjamin.gaignard@linaro.org> wrote:
> 2017-10-11 16:01 GMT+02:00 Arnd Bergmann <arnd@arndb.de>:
>
>> @@ -398,6 +400,9 @@ static enum dma_slave_buswidth stm32_mdma_get_max_width(u32 buf_len, u32 tlen)
>> break;
>> }
>>
>> + if (addr % max_width)
>> + max_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
>> +
>
> I'm only half-convince by the implicite 32 bits cast done into
> function prototype.
> If we keep using dma_addr_t and use do_div() instead of %
> does compiler can still optimize the code ?
>
I wouldn't want to add a do_div() here, since it's guaranteed
not to be needed. Would you prefer an explicit cast here
and leave the argument as dma_addr_t?
We could also use a bit mask here like
if (addr & (max_width-1))
or we could combined it with the check above:
if ((((buf_len | addr) & (max_width - 1)) == 0) &&
(tlen >= max_width))
Arnd
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] dmaengine: stm32-mdma: avoid 64-bit division
2017-10-11 14:39 ` Arnd Bergmann
@ 2017-10-11 14:46 ` Benjamin Gaignard
2017-10-11 15:13 ` Arnd Bergmann
0 siblings, 1 reply; 6+ messages in thread
From: Benjamin Gaignard @ 2017-10-11 14:46 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Vinod Koul, Maxime Coquelin, Alexandre Torgue,
Linux Kernel Mailing List, Pierre-Yves MORDRET, dmaengine,
M'boumba Cedric Madianga, Dan Williams, Linux ARM
2017-10-11 16:39 GMT+02:00 Arnd Bergmann <arnd@arndb.de>:
> On Wed, Oct 11, 2017 at 4:27 PM, Benjamin Gaignard
> <benjamin.gaignard@linaro.org> wrote:
>> 2017-10-11 16:01 GMT+02:00 Arnd Bergmann <arnd@arndb.de>:
>>
>>> @@ -398,6 +400,9 @@ static enum dma_slave_buswidth stm32_mdma_get_max_width(u32 buf_len, u32 tlen)
>>> break;
>>> }
>>>
>>> + if (addr % max_width)
>>> + max_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
>>> +
>>
>> I'm only half-convince by the implicite 32 bits cast done into
>> function prototype.
>> If we keep using dma_addr_t and use do_div() instead of %
>> does compiler can still optimize the code ?
>>
>
> I wouldn't want to add a do_div() here, since it's guaranteed
> not to be needed. Would you prefer an explicit cast here
> and leave the argument as dma_addr_t?
>
> We could also use a bit mask here like
>
> if (addr & (max_width-1))
That sound better for me since it doesn't limit the code to 32 bits architecture
>
> or we could combined it with the check above:
>
> if ((((buf_len | addr) & (max_width - 1)) == 0) &&
> (tlen >= max_width))
No it is more simple to read with two checks
Benjamin
>
> Arnd
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] dmaengine: stm32-mdma: avoid 64-bit division
2017-10-11 14:46 ` Benjamin Gaignard
@ 2017-10-11 15:13 ` Arnd Bergmann
2017-10-11 15:53 ` Pierre Yves MORDRET
0 siblings, 1 reply; 6+ messages in thread
From: Arnd Bergmann @ 2017-10-11 15:13 UTC (permalink / raw)
To: Benjamin Gaignard
Cc: Vinod Koul, Maxime Coquelin, Alexandre Torgue,
Linux Kernel Mailing List, Pierre-Yves MORDRET, dmaengine,
M'boumba Cedric Madianga, Dan Williams, Linux ARM
On Wed, Oct 11, 2017 at 4:46 PM, Benjamin Gaignard
<benjamin.gaignard@linaro.org> wrote:
> 2017-10-11 16:39 GMT+02:00 Arnd Bergmann <arnd@arndb.de>:
>> On Wed, Oct 11, 2017 at 4:27 PM, Benjamin Gaignard
>> <benjamin.gaignard@linaro.org> wrote:
>>> 2017-10-11 16:01 GMT+02:00 Arnd Bergmann <arnd@arndb.de>:
>>>
>>>> @@ -398,6 +400,9 @@ static enum dma_slave_buswidth stm32_mdma_get_max_width(u32 buf_len, u32 tlen)
>>>> break;
>>>> }
>>>>
>>>> + if (addr % max_width)
>>>> + max_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
>>>> +
>>>
>>> I'm only half-convince by the implicite 32 bits cast done into
>>> function prototype.
>>> If we keep using dma_addr_t and use do_div() instead of %
>>> does compiler can still optimize the code ?
>>>
>>
>> I wouldn't want to add a do_div() here, since it's guaranteed
>> not to be needed. Would you prefer an explicit cast here
>> and leave the argument as dma_addr_t?
>>
>> We could also use a bit mask here like
>>
>> if (addr & (max_width-1))
>
> That sound better for me since it doesn't limit the code to 32 bits architecture
FWIW, I used the u32 type here because that's the limit of the
dma driver, the dma_addr_t gets converted to that anyway
later.
>>
>> or we could combined it with the check above:
>>
>> if ((((buf_len | addr) & (max_width - 1)) == 0) &&
>> (tlen >= max_width))
>
> No it is more simple to read with two checks
I should have mentioned that this variant would also change
behavior: the current code falls back to byte access when
the address alignment is less than the length alignment.
The change I suggested here would change that to use
the maximum possible address width that fits the alignment
of either size or address.
I don't know what behavior we actually want though, or
if that change would be correct.
Arnd
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] dmaengine: stm32-mdma: avoid 64-bit division
2017-10-11 15:13 ` Arnd Bergmann
@ 2017-10-11 15:53 ` Pierre Yves MORDRET
0 siblings, 0 replies; 6+ messages in thread
From: Pierre Yves MORDRET @ 2017-10-11 15:53 UTC (permalink / raw)
To: Arnd Bergmann, Benjamin Gaignard
Cc: Vinod Koul, Maxime Coquelin, Alexandre Torgue,
Linux Kernel Mailing List, dmaengine,
M'boumba Cedric Madianga, Dan Williams, Linux ARM
On 10/11/2017 05:13 PM, Arnd Bergmann wrote:
> On Wed, Oct 11, 2017 at 4:46 PM, Benjamin Gaignard
> <benjamin.gaignard@linaro.org> wrote:
>> 2017-10-11 16:39 GMT+02:00 Arnd Bergmann <arnd@arndb.de>:
>>> On Wed, Oct 11, 2017 at 4:27 PM, Benjamin Gaignard
>>> <benjamin.gaignard@linaro.org> wrote:
>>>> 2017-10-11 16:01 GMT+02:00 Arnd Bergmann <arnd@arndb.de>:
>>>>
>>>>> @@ -398,6 +400,9 @@ static enum dma_slave_buswidth stm32_mdma_get_max_width(u32 buf_len, u32 tlen)
>>>>> break;
>>>>> }
>>>>>
>>>>> + if (addr % max_width)
>>>>> + max_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
>>>>> +
>>>>
>>>> I'm only half-convince by the implicite 32 bits cast done into
>>>> function prototype.
>>>> If we keep using dma_addr_t and use do_div() instead of %
>>>> does compiler can still optimize the code ?
>>>>
>>>
>>> I wouldn't want to add a do_div() here, since it's guaranteed
>>> not to be needed. Would you prefer an explicit cast here
>>> and leave the argument as dma_addr_t?
>>>
>>> We could also use a bit mask here like
>>>
>>> if (addr & (max_width-1))
>>
>> That sound better for me since it doesn't limit the code to 32 bits architecture
>
> FWIW, I used the u32 type here because that's the limit of the
> dma driver, the dma_addr_t gets converted to that anyway
> later.
>
>>>
>>> or we could combined it with the check above:
>>>
>>> if ((((buf_len | addr) & (max_width - 1)) == 0) &&
>>> (tlen >= max_width))
>>
>> No it is more simple to read with two checks
>
> I should have mentioned that this variant would also change
> behavior: the current code falls back to byte access when
> the address alignment is less than the length alignment.
> The change I suggested here would change that to use
> the maximum possible address width that fits the alignment
> of either size or address.
Both alignment are required on address and length.
The main advantage result is maximized in term of width. As for now I don't see
any drawback except a short explanation.
Nonetheless I need to think a little bit more about this change.
>
> I don't know what behavior we actually want though, or
> if that change would be correct.
>
> Arnd
>
Regards
Py
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-10-11 15:54 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-11 14:01 [PATCH] dmaengine: stm32-mdma: avoid 64-bit division Arnd Bergmann
2017-10-11 14:27 ` Benjamin Gaignard
2017-10-11 14:39 ` Arnd Bergmann
2017-10-11 14:46 ` Benjamin Gaignard
2017-10-11 15:13 ` Arnd Bergmann
2017-10-11 15:53 ` Pierre Yves MORDRET
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®