[PATCH] ARM: dts: am33xx: Move the cppi41dma node so it's probed early

[PATCH] ARM: dts: am33xx: Move the cppi41dma node so it's probed early

[Ezequiel Garcia]

The DMA controller is needed for the USB controller to be correctly
registered. Therefore, if the DMA node is located at the end an unecessary
probe deferral is produced systematically.

This is easily fixed by moving the node at the beggining of the child list,
so it's probed first.

Signed-off-by: Ezequiel Garcia <ezequiel-30ULvvUtt6G51wMPkGsGjgyUoB5FGQPZ@public.gmane.org>
---
Felipe, Sebastian:

I cannot see why the cppi41dma node must be placed inside the
"ti,am33xx-usb" compatible node. Tried to move it out
so it's probed just like the edma engine, but the USB doesn't work
properly in that case.

Can you enlighten me?

In any case, this change is good enough to remove the deferral probe.

Thanks!

 arch/arm/boot/dts/am33xx.dtsi | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
index 9770e35..c673be4 100644
--- a/arch/arm/boot/dts/am33xx.dtsi
+++ b/arch/arm/boot/dts/am33xx.dtsi
@@ -453,6 +453,21 @@
 			ti,hwmods = "usb_otg_hs";
 			status = "disabled";
 
+			cppi41dma: dma-controller@47402000 {
+				compatible = "ti,am3359-cppi41";
+				reg =  <0x47400000 0x1000
+					0x47402000 0x1000
+					0x47403000 0x1000
+					0x47404000 0x4000>;
+				reg-names = "glue", "controller", "scheduler", "queuemgr";
+				interrupts = <17>;
+				interrupt-names = "glue";
+				#dma-cells = <2>;
+				#dma-channels = <30>;
+				#dma-requests = <256>;
+				status = "disabled";
+			};
+
 			usb_ctrl_mod: control@44e10620 {
 				compatible = "ti,am335x-usb-ctrl-module";
 				reg = <0x44e10620 0x10
@@ -556,20 +571,6 @@
 					"tx14", "tx15";
 			};
 
-			cppi41dma: dma-controller@47402000 {
-				compatible = "ti,am3359-cppi41";
-				reg =  <0x47400000 0x1000
-					0x47402000 0x1000
-					0x47403000 0x1000
-					0x47404000 0x4000>;
-				reg-names = "glue", "controller", "scheduler", "queuemgr";
-				interrupts = <17>;
-				interrupt-names = "glue";
-				#dma-cells = <2>;
-				#dma-channels = <30>;
-				#dma-requests = <256>;
-				status = "disabled";
-			};
 		};
 
 		epwmss0: epwmss@48300000 {
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ARM: dts: am33xx: Move the cppi41dma node so it's probed early

[Sebastian Andrzej Siewior]

On 04/22/2014 04:19 PM, Ezequiel Garcia wrote:
> The DMA controller is needed for the USB controller to be correctly
> registered. Therefore, if the DMA node is located at the end an unecessary
> probe deferral is produced systematically.
> 
> This is easily fixed by moving the node at the beggining of the child list,
> so it's probed first.

So you do not change anything except for the order of child nodes. So
this should be fine and without a compatibility problem.

I added them according to the memory offset so you might want to add a
comment why you moved this because Mr. Structured & Organized might
noticed this one day and move it back.

> Signed-off-by: Ezequiel Garcia <ezequiel-30ULvvUtt6G51wMPkGsGjgyUoB5FGQPZ@public.gmane.org>
> ---
> Felipe, Sebastian:
> 
> I cannot see why the cppi41dma node must be placed inside the
> "ti,am33xx-usb" compatible node. Tried to move it out
> so it's probed just like the edma engine, but the USB doesn't work
> properly in that case.
> 
> Can you enlighten me?

So If I remember correctly it was a big bag of crap. If you look at the
parent node, you notice that it has a ti,hwmods member while the other
do not have such a property. According to the manual only the whole IP
block as-it has this. It has to be activated if you use one of those
devices this includes the two USB-IP cores and the DMA-IP core. I
didn't manage to come up with something else except to make one parent
node which creates the childs to have a proper relation here.

There was also something with parent - child relation in the way musb
expected it. I think this was only glue code + musb child node and had
nothing to do with the DMA engine. But I am not 100% sure…

> 
> In any case, this change is good enough to remove the deferral probe.

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ARM: dts: am33xx: Move the cppi41dma node so it's probed early

[Ezequiel Garcia]

On Apr 23, Sebastian Andrzej Siewior wrote:
> On 04/22/2014 04:19 PM, Ezequiel Garcia wrote:
> > The DMA controller is needed for the USB controller to be correctly
> > registered. Therefore, if the DMA node is located at the end an unecessary
> > probe deferral is produced systematically.
> > 
> > This is easily fixed by moving the node at the beggining of the child list,
> > so it's probed first.
> 
> So you do not change anything except for the order of child nodes. So
> this should be fine and without a compatibility problem.
> 
> I added them according to the memory offset so you might want to add a
> comment why you moved this because Mr. Structured & Organized might
> noticed this one day and move it back.
> 

Yes, good catch. I'll prepare a v2.

> > Signed-off-by: Ezequiel Garcia <ezequiel-30ULvvUtt6G51wMPkGsGjgyUoB5FGQPZ@public.gmane.org>
> > ---
> > Felipe, Sebastian:
> > 
> > I cannot see why the cppi41dma node must be placed inside the
> > "ti,am33xx-usb" compatible node. Tried to move it out
> > so it's probed just like the edma engine, but the USB doesn't work
> > properly in that case.
> > 
> > Can you enlighten me?
> 
> So If I remember correctly it was a big bag of crap. If you look at the
> parent node, you notice that it has a ti,hwmods member while the other
> do not have such a property. According to the manual only the whole IP
> block as-it has this. It has to be activated if you use one of those
> devices this includes the two USB-IP cores and the DMA-IP core. I
> didn't manage to come up with something else except to make one parent
> node which creates the childs to have a proper relation here.
> 

Ah, this could be...

> There was also something with parent - child relation in the way musb
> expected it. I think this was only glue code + musb child node and had
> nothing to do with the DMA engine. But I am not 100% sure…
> 

Well, from my poor and unexperienced code inspection, I could not see anything
relating the parent of the DMA controller node to the node itself, and so that's
why it seemed possible to move it out.

Once the dma-controller is moved out of the USB devicetree node block,
the dmaengine driver is registered correctly (apparently), and so does the USB
driver.

However, upon connection of some USB storage device, detection begins
but then things don't go well, although I don't have the exact error here,
I remember this warning was hitted on dma/cppi41.c at line 602:

  WARN_ON(!c->td_retry);

Just in case someone want to debug this further.

Thanks a lot for the feedback!
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html