arm: edma: Fix xbar mapping

PATCH New Thomas Gleixner 4 participants 6 messages Apr 13, 2014 – May 11, 2014

Subsystems (4)

Device tree bindings Arm (32-bit) dmaengine Texas Instruments DMA drivers

lore

Share & tools

lore

Thomas Gleixner

author

2014-04-13 20:48 UTC #0 lore raw

PATCH 1/1 arm: edma: Fix xbar mapping

Subject: arm: edma: Fix xbar mapping
From: Thomas Gleixner <[email protected]>
Date: Sun, 13 Apr 2014 20:44:46 +0200

This is another great example of trainwreck engineering:

commit 2646a0e529 (ARM: edma: Add EDMA crossbar event mux support) added support for using EDMA on peripherals which have no direct EDMA event mapping.

The code compiles and does not explode in your face, but that's it.

1) Reading an u16 array from an u32 device tree array simply does not work. Even if the function is named "edma_of_read_u32_to_s16_array".

It merily calls of_property_read_u16_array. So the resulting 16bit array will have every other entry = 0.

2) The DT entry for the xbar registers related to xbar has length 0x10 instead of the real length: 0xfd0 - 0xf90 = 0x40.

Not a real problem as it does not cross a page boundary, but wrong nevertheless.

3) But none of this matters as the mapping never happens:

After reading nonsense edma_of_read_u32_to_s16_array() invalidates the first array entry pair, so nobody can ever notice the braindamage by immediate explosion.

Seems the QA criteria for this code was solely not to explode when someone adds edma-xbar-event-map entries to the DT. Goal achieved, congratulations!

Not really helpful if someone wants to use edma on a device which requires a xbar mapping.

Fix the issues by:

- annotating the device tree entry with "/bits/ 16" as documented in the of_property_read_u16_array kernel doc

- make the size of the xbar register mapping correct

- invalidating the end of the array and not the start

This convoluted mess wants to be completely rewritten as there is no point to keep the xbar_chan array memory and the iomapping of the xbar regs around forever. Marking the xbar mapped channels as used should be done right there.

But that's a different issue and this patch is small enough to make it work and allows a simple backport for stable.

Signed-off-by Thomas Gleixner <[email protected]>

---
 Documentation/devicetree/bindings/dma/ti-edma.txt |    4 -
 arch/arm/boot/dts/am33xx.dtsi                     |    2 
 arch/arm/common/edma.c                            |   48 ++++++----------------
 3 files changed, 18 insertions(+), 36 deletions(-)

Documentation/devicetree/bindings/dma/ti-edma.txt +2 -2

Index: linux-2.6/Documentation/devicetree/bindings/dma/ti-edma.txt
===================================================================
@@ -29,6 +29,6 @@ edma: edma at 49000000 {
 	dma-channels = <64>;
 	ti,edma-regions = <4>;
 	ti,edma-slots = <256>;
-	ti,edma-xbar-event-map = <1 12
-				  2 13>;
+	ti,edma-xbar-event-map = /bits/ 16 <1 12
+				  	    2 13>;
 };

arch/arm/boot/dts/am33xx.dtsi +1 -1

Index: linux-2.6/arch/arm/boot/dts/am33xx.dtsi
===================================================================
@@ -144,7 +144,7 @@
 			compatible = "ti,edma3";
 			ti,hwmods = "tpcc", "tptc0", "tptc1", "tptc2";
 			reg =	<0x49000000 0x10000>,
-				<0x44e10f90 0x10>;
+				<0x44e10f90 0x40>;
 			interrupts = <12 13 14>;
 			#dma-cells = <1>;
 			dma-channels = <64>;

arch/arm/common/edma.c +15 ~~-33~~

78 lines not on this page · raw message

Sekhar Nori

2014-04-14 14:14 UTC #1 lore raw

PATCH 1/1 arm: edma: Fix xbar mapping

+ Matt with current e-mail address.

Quoted 9 lines · Thomas Gleixner

Subject: arm: edma: Fix xbar mapping
From: Thomas Gleixner <[email protected]>
Date: Sun, 13 Apr 2014 20:44:46 +0200
This is another great example of trainwreck engineering:
commit 2646a0e529 (ARM: edma: Add EDMA crossbar event mux support) added support for using EDMA on peripherals which have no direct EDMA event mapping.

I committed it, so the blame goes to me (at least in part).

Quoted 42 lines

The code compiles and does not explode in your face, but that's it.
1) Reading an u16 array from an u32 device tree array simply does not work. Even if the function is named "edma_of_read_u32_to_s16_array".
It merily calls of_property_read_u16_array. So the resulting 16bit array will have every other entry = 0.
2) The DT entry for the xbar registers related to xbar has length 0x10 instead of the real length: 0xfd0 - 0xf90 = 0x40.
Not a real problem as it does not cross a page boundary, but wrong nevertheless.
3) But none of this matters as the mapping never happens:
After reading nonsense edma_of_read_u32_to_s16_array() invalidates the first array entry pair, so nobody can ever notice the braindamage by immediate explosion.
Seems the QA criteria for this code was solely not to explode when someone adds edma-xbar-event-map entries to the DT. Goal achieved, congratulations!
Not really helpful if someone wants to use edma on a device which requires a xbar mapping.
Fix the issues by:
- annotating the device tree entry with "/bits/ 16" as documented in the of_property_read_u16_array kernel doc
- make the size of the xbar register mapping correct
- invalidating the end of the array and not the start
This convoluted mess wants to be completely rewritten as there is no point to keep the xbar_chan array memory and the iomapping of the xbar regs around forever. Marking the xbar mapped channels as used should be done right there.

Quoted 4 lines

But that's a different issue and this patch is small enough to make it work and allows a simple backport for stable.
Signed-off-by Thomas Gleixner <[email protected]>

This time, I tested this patch and FWIW you can add:

Acked-by Sekhar Nori <[email protected]>

Thanks,
Sekhar

Thomas Gleixner

author

2014-04-28 18:32 UTC #2 lore raw

PATCH 1/1 arm: edma: Fix xbar mapping

Quoted 14 lines · Sekhar Nori

Quoted 14 lines
Fix the issues by:
- annotating the device tree entry with "/bits/ 16" as documented in the of_property_read_u16_array kernel doc
- make the size of the xbar register mapping correct
- invalidating the end of the array and not the start
This convoluted mess wants to be completely rewritten as there is no point to keep the xbar_chan array memory and the iomapping of the xbar regs around forever. Marking the xbar mapped channels as used should be done right there.

Quoted 8 lines

Quoted 4 lines
But that's a different issue and this patch is small enough to make it work and allows a simple backport for stable.
Signed-off-by Thomas Gleixner <[email protected]>
This time, I tested this patch and FWIW you can add:
Acked-by Sekhar Nori <[email protected]>

Can someone pick this up please?

Vinod Koul

2014-04-29 06:07 UTC #3 lore raw

PATCH 1/1 arm: edma: Fix xbar mapping

Quoted 18 lines · Thomas Gleixner

Quoted 17 lines · Sekhar Nori
Quoted 17 lines
Fix the issues by:
- annotating the device tree entry with "/bits/ 16" as documented in the of_property_read_u16_array kernel doc
- make the size of the xbar register mapping correct
- invalidating the end of the array and not the start
This convoluted mess wants to be completely rewritten as there is no point to keep the xbar_chan array memory and the iomapping of the xbar regs around forever. Marking the xbar mapped channels as used should be done right there.
But that's a different issue and this patch is small enough to make it work and allows a simple backport for stable.

Quoted 7 lines

Quoted 5 lines
Quoted 1 line
Signed-off-by Thomas Gleixner <[email protected]>
This time, I tested this patch and FWIW you can add:
Acked-by Sekhar Nori <[email protected]>
Can someone pick this up please?

Shouldnt this go thru ARM tree?

Signature · 1 line

~Vinod

Sekhar Nori

2014-04-29 06:40 UTC #4 lore raw

PATCH 1/1 arm: edma: Fix xbar mapping

Quoted 19 lines · Vinod Koul

Quoted 18 lines · Thomas Gleixner
Quoted 17 lines · Sekhar Nori
Quoted 17 lines
Fix the issues by:
- annotating the device tree entry with "/bits/ 16" as documented in the of_property_read_u16_array kernel doc
- make the size of the xbar register mapping correct
- invalidating the end of the array and not the start
This convoluted mess wants to be completely rewritten as there is no point to keep the xbar_chan array memory and the iomapping of the xbar regs around forever. Marking the xbar mapped channels as used should be done right there.
But that's a different issue and this patch is small enough to make it work and allows a simple backport for stable.

Quoted 8 lines

Quoted 7 lines
Quoted 5 lines
Quoted 1 line
Signed-off-by Thomas Gleixner <[email protected]>
This time, I tested this patch and FWIW you can add:
Acked-by Sekhar Nori <[email protected]>
Can someone pick this up please?
Shouldnt this go thru ARM tree?

I will send a pull request to ARM-SoC.

Thanks,
Sekhar

Olof Johansson

2014-05-11 05:16 UTC #5 lore raw

PATCH 1/1 arm: edma: Fix xbar mapping

In reply to Sekhar Nori

Quoted 7 lines · Sekhar Nori

+ Matt with current e-mail address.
Quoted 4 lines · Thomas Gleixner
Subject: arm: edma: Fix xbar mapping
From: Thomas Gleixner <[email protected]>
Date: Sun, 13 Apr 2014 20:44:46 +0200

Quoted 7 lines

Quoted 5 lines
This is another great example of trainwreck engineering:
commit 2646a0e529 (ARM: edma: Add EDMA crossbar event mux support) added support for using EDMA on peripherals which have no direct EDMA event mapping.
I committed it, so the blame goes to me (at least in part).

Another part of the problem is that it's a feature without in-tree users -- there's not a single DT that defines the nodes (and thus can be used to test it). Sure, there are no strict requirements to have all bindings implemented on a platform but it sure does help catch problems if you can test without applying out-of-tree patches to make a feature work.

(I just merged the pull request from Sekhar, I had missed it before. Should make -rc6).

-Olof