dmaengine: pl330: Fix linker error "undefined reference to `__aeabi_uldivmod'"
PATCH 1/1 dmaengine: pl330: Fix linker error "undefined reference to `__aeabi_uldivmod'"
32-bit ARM kernels may have a 64-bit dma_addr_t but have no implementation of the compiler helper for 64-bit unsigned division, therefore the use of the modulo operator in pl330_prep_dma_memcpy causes the link error "undefined reference to `__aeabi_uldivmod'"
As the burst value is always a power of two we can fix the problem, and make the code more efficient, by replacing "% burst" with "& (burst-1)".
---
Vinod. I haven't added a 'Fixes:' line because I was unsure if the patch in linux-next is part of a stable branch or if the SHA1 might change before hitting mainline. If it stable then the line should be...
drivers/dma/pl330.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
drivers/dma/pl330.c +1 -4
@@ -2464,11 +2464,8 @@ pl330_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dst,* parameters because our DMA programming algorithm doesn't cope with * transfers which straddle an entry in the DMA device's MFIFO. */ -while (burst >1 ) { -if (!((src | dst | len) % burst)) -break ; +while ((src | dst | len) & (burst -1 )) burst /=2 ; - } desc->rqcfg.brst_size =0 ;while (burst != (1 << desc->rqcfg.brst_size))
Signature · 1 line
2.1.1
Quoted 11 lines · On Thursday 13 November 2014 16:27:27 Jon Medhurst wrote:
32-bit ARM kernels may have a 64-bit
dma_addr_tbut have no implementation of the compiler helper for 64-bit unsigned division, therefore the use of the modulo operator inpl330_prep_dma_memcpycauses the link error "undefined reference to `__aeabi_uldivmod'"As the burst value is always a power of two we can fix the problem, and make the code more efficient, by replacing "% burst" with "& (burst-1)".
Reported-by kbuild test robot <[email protected]>Signed-off-by Jon Medhurst <[email protected]>
Just saw the same thing and was going to send a different patch, but yours is better.
Quoted 10 lines · Jon Medhurst (Tixy)
32-bit ARM kernels may have a 64-bit
dma_addr_tbut have no implementation of the compiler helper for 64-bit unsigned division, therefore the use of the modulo operator inpl330_prep_dma_memcpycauses the link error "undefined reference to `__aeabi_uldivmod'"As the burst value is always a power of two we can fix the problem, and make the code more efficient, by replacing "% burst" with "& (burst-1)".
Reported-by kbuild test robot <[email protected]>Signed-off-by Jon Medhurst <[email protected]>
Quoted 7 lines
---
Vinod. I haven't added a 'Fixes:' line because I was unsure if the patch in linux-next is part of a stable branch or if the SHA1 might change before hitting mainline. If it stable then the line should be...
Fixes 63369d0a96dc ("dmaengine: pl330: Align DMA memcpy operations to MFIFO width")
I have applied this for now but...
While at it and also related to Fixes, typically the fixes branch wont be rebased before its sent to Linus and merged. But this is introduced in patch which is sent, should I just fold it in and not cause this regression in first place...?
Signature · 1 line
~Vinod
Quoted 26 lines
drivers/dma/pl330.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)drivers/dma/pl330.c +1
-4@@ -2464,11 +2464,8 @@ pl330_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dst,* parameters because our DMA programming algorithm doesn't cope with * transfers which straddle an entry in the DMA device's MFIFO. */ -while (burst >1 ) { -if (!((src | dst | len) % burst)) -break ; +while ((src | dst | len) & (burst -1 )) burst /=2 ; - } desc->rqcfg.brst_size =0 ;while (burst != (1 << desc->rqcfg.brst_size))Signature · 1 line
2.1.1
Quoted 17 lines · Vinod Koul
Quoted 16 lines · Jon Medhurst (Tixy)
32-bit ARM kernels may have a 64-bit
dma_addr_tbut have no implementation of the compiler helper for 64-bit unsigned division, therefore the use of the modulo operator inpl330_prep_dma_memcpycauses the link error "undefined reference to `__aeabi_uldivmod'"As the burst value is always a power of two we can fix the problem, and make the code more efficient, by replacing "% burst" with "& (burst-1)".
Reported-by kbuild test robot <[email protected]>Signed-off-by Jon Medhurst <[email protected]>---
Vinod. I haven't added a 'Fixes:' line because I was unsure if the patch in linux-next is part of a stable branch or if the SHA1 might change before hitting mainline. If it stable then the line should be...
Quoted 7 lines
Quoted 1 line
Fixes 63369d0a96dc ("dmaengine: pl330: Align DMA memcpy operations to MFIFO width")I have applied this for now but...
While at it and also related to Fixes, typically the fixes branch wont be rebased before its sent to Linus and merged. But this is introduced in patch which is sent, should I just fold it in and not cause this regression in first place...?
I have no objection to folding it in, but then doesn't that remove credit for Fengguang Wu's test system for finding and reporting errors?
Signature · 1 line
Tixy
Quoted 21 lines · Jon Medhurst (Tixy)
Quoted 20 lines · Vinod Koul
Quoted 10 lines · Jon Medhurst (Tixy)
32-bit ARM kernels may have a 64-bit
dma_addr_tbut have no implementation of the compiler helper for 64-bit unsigned division, therefore the use of the modulo operator inpl330_prep_dma_memcpycauses the link error "undefined reference to `__aeabi_uldivmod'"As the burst value is always a power of two we can fix the problem, and make the code more efficient, by replacing "% burst" with "& (burst-1)".
Reported-by kbuild test robot <[email protected]>Signed-off-by Jon Medhurst <[email protected]>Quoted 7 lines
---
Vinod. I haven't added a 'Fixes:' line because I was unsure if the patch in linux-next is part of a stable branch or if the SHA1 might change before hitting mainline. If it stable then the line should be...
Fixes 63369d0a96dc ("dmaengine: pl330: Align DMA memcpy operations to MFIFO width")I have applied this for now but...
Quoted 7 lines
Quoted 4 lines
While at it and also related to Fixes, typically the fixes branch wont be rebased before its sent to Linus and merged. But this is introduced in patch which is sent, should I just fold it in and not cause this regression in first place...?
I have no objection to folding it in, but then doesn't that remove credit for Fengguang Wu's test system for finding and reporting errors?
I added entry for that and retiained credit to him.
Signature · 1 line
~Vinod
Quoted 27 lines · Jon Medhurst (Tixy)
32-bit ARM kernels may have a 64-bit
dma_addr_tbut have no implementation of the compiler helper for 64-bit unsigned division, therefore the use of the modulo operator inpl330_prep_dma_memcpycauses the link error "undefined reference to `__aeabi_uldivmod'"As the burst value is always a power of two we can fix the problem, and make the code more efficient, by replacing "% burst" with "& (burst-1)".
Reported-by kbuild test robot <[email protected]>Signed-off-by Jon Medhurst <[email protected]>---
Vinod. I haven't added a 'Fixes:' line because I was unsure if the patch in linux-next is part of a stable branch or if the SHA1 might change before hitting mainline. If it stable then the line should be...
Fixes 63369d0a96dc ("dmaengine: pl330: Align DMA memcpy operations to MFIFO width")drivers/dma/pl330.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)drivers/dma/pl330.c +0
-0
drivers/dma/pl330.c pl330_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dst, · line 2471
Quoted 9 lines
drivers/dma/pl330.c +1
-4@@ -2464,11 +2464,8 @@ pl330_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dst,* parameters because our DMA programming algorithm doesn't cope with * transfers which straddle an entry in the DMA device's MFIFO. */ -while (burst >1 ) { -if (!((src | dst | len) % burst)) -break ; +while ((src | dst | len) & (burst -1 )) burst /=2 ; - }
Maybe something like:
div = ffs(src | dst | len); if (burst > 1 && div) burst >>= div;
?
dunno if dma_addr_t src or dst can ever be a 64 bit value for AMBA or not. If so, the ffs would need to be different.
Maybe:
if (sizeof(dma_addr_t) == sizeof(u64)) div = __ffs64(src | dst | len); else div = ffs(src | dst | len); if (burst > 1 && div) burst >>= div;
Quoted 40 lines · Joe Perches
Quoted 27 lines · Jon Medhurst (Tixy)
32-bit ARM kernels may have a 64-bit
dma_addr_tbut have no implementation of the compiler helper for 64-bit unsigned division, therefore the use of the modulo operator inpl330_prep_dma_memcpycauses the link error "undefined reference to `__aeabi_uldivmod'"As the burst value is always a power of two we can fix the problem, and make the code more efficient, by replacing "% burst" with "& (burst-1)".
Reported-by kbuild test robot <[email protected]>Signed-off-by Jon Medhurst <[email protected]>---
Vinod. I haven't added a 'Fixes:' line because I was unsure if the patch in linux-next is part of a stable branch or if the SHA1 might change before hitting mainline. If it stable then the line should be...
Fixes 63369d0a96dc ("dmaengine: pl330: Align DMA memcpy operations to MFIFO width")drivers/dma/pl330.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)drivers/dma/pl330.c +0
-0drivers/dma/pl330.c pl330_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dst, · line 2471
Quoted 9 lines
drivers/dma/pl330.c +1
-4@@ -2464,11 +2464,8 @@ pl330_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dst,* parameters because our DMA programming algorithm doesn't cope with * transfers which straddle an entry in the DMA device's MFIFO. */ -while (burst >1 ) { -if (!((src | dst | len) % burst)) -break ; +while ((src | dst | len) & (burst -1 )) burst /=2 ; - }Maybe something like:
Quoted 3 lines
div = ffs(src | dst | len); if (burst > 1 && div) burst >>= div;
That doesn't work, the code is trying to limit burst to make it a factor of src, dst and len, so it would need to be something like
div = ffs(src | dst | len); if (div)
burst = min(burst, 1 << div);
There are many ways to code the limiting of the burst width, but as it starts out as the data bus width the DMA can handle (maximum 16 bytes) then@most we'll be going round the existing while loop 4 times so I don't think it's that much overhead, and probably less code size than using ffs.
And as the driver has been broken for the unaligned memcpy case since the day it was added then I can't see that anyone is actually using it that way anyway, so all existing users (if any) must already be doing bus aligned copies and the current while loop will iterate zero times.
That's probably enough bikeshedding from me :-)
Quoted 4 lines
?
dunno if
dma_addr_tsrc or dst can ever be a 64 bit value for AMBA or not.
The pl330 TRM I have and the current Linux driver explicitly have 32-bit addresses, so you would need an IOMMU to access addresses above 4GB.
Signature · 1 line
Tixy
Quoted 5 lines · Jon Medhurst (Tixy)
There are many ways to code the limiting of the burst width, but as it starts out as the data bus width the DMA can handle (maximum 16 bytes) then at most we'll be going round the existing while loop 4 times so I don't think it's that much overhead, and probably less code size than using ffs.
For arm, isn't ffs just a few instruction with no loops?
Quoted 4 lines
And as the driver has been broken for the unaligned memcpy case since the day it was added then I can't see that anyone is actually using it that way anyway, so all existing users (if any) must already be doing bus aligned copies and the current while loop will iterate zero times.
That's probably right, I just don't like reading
while loops where ffs/fls might be suitable.
Quoted 1 line
That's probably enough bikeshedding from me :-)
;) Me too. cheers, Joe