ARM: dts: am33xx: Move the cppi41dma node so it's probed early
PATCH v2 1/1 ARM: dts: am33xx: Move the cppi41dma node so it's probed early
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.
--v1->v2:
* Added a comment to prevent a future clean-up based on the memory offset.
arch/arm/boot/dts/am33xx.dtsi | 34 ++++++++++++++++++++-------------- 1 file changed, 20 insertions(+), 14 deletions(-)
arch/arm/boot/dts/am33xx.dtsi +20 -14
@@ -453,6 +453,26 @@ ti,hwmods = "usb_otg_hs"; status = "disabled"; + /* + * The probe order matches the child ordering so the + * dma-controller node must be the first one to prevent + * spurious probe deferrals. + */ + cppi41dma: dma-controller at 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 at 44e10620 { compatible = "ti,am335x-usb-ctrl-module"; reg = <0x44e10620 0x10
@@ -556,20 +576,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 at 48300000 {
Signature · 1 line
1.9.1
PATCH v2 1/1 ARM: dts: am33xx: Move the cppi41dma node so it's probed early
Quoted 8 lines · 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 <[email protected]>
Quoted 66 lines
--
v1->v2:
* Added a comment to prevent a future clean-up based on the memory offset.arch/arm/boot/dts/am33xx.dtsi | 34 ++++++++++++++++++++-------------- 1 file changed, 20 insertions(+), 14 deletions(-)arch/arm/boot/dts/am33xx.dtsi +20
-15@@ -453,6 +453,26 @@ ti,hwmods = "usb_otg_hs"; status = "disabled"; + /* + * The probe order matches the child ordering so the + * dma-controller node must be the first one to prevent + * spurious probe deferrals. + */ + cppi41dma: dma-controller at 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 at 44e10620 { compatible = "ti,am335x-usb-ctrl-module"; reg = <0x44e10620 0x10@@ -556,20 +576,6 @@ "tx14", "tx15"; }; - cppi41dma: dma-controller at 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 at 48300000 { --1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH v2 1/1 ARM: dts: am33xx: Move the cppi41dma node so it's probed early
Quoted 8 lines · Yegor Yefremov
Quoted 6 lines · 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.
This will give issues on module removal.
Since we use device_for_each_child in remove patch, it will try to remove cppi dma controller, while the channel
is still in use by musb node.
Quoted 73 lines
Quoted 1 line
Signed-off-by Ezequiel Garcia <[email protected]>Tested-by Yegor Yefremov <[email protected]>Quoted 62 lines
--
v1->v2:
* Added a comment to prevent a future clean-up based on the memory offset.arch/arm/boot/dts/am33xx.dtsi | 34 ++++++++++++++++++++-------------- 1 file changed, 20 insertions(+), 14 deletions(-)arch/arm/boot/dts/am33xx.dtsi +20
-15@@ -453,6 +453,26 @@ ti,hwmods = "usb_otg_hs"; status = "disabled"; + /* + * The probe order matches the child ordering so the + * dma-controller node must be the first one to prevent + * spurious probe deferrals. + */ + cppi41dma: dma-controller at 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 at 44e10620 { compatible = "ti,am335x-usb-ctrl-module"; reg = <0x44e10620 0x10@@ -556,20 +576,6 @@ "tx14", "tx15"; }; - cppi41dma: dma-controller at 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 at 48300000 { --1.9.1
Quoted 4 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Signature · 1 line
-George
PATCH v2 1/1 ARM: dts: am33xx: Move the cppi41dma node so it's probed early
Quoted 7 lines · George Cherian
Quoted 3 lines
Quoted 3 lines
This is easily fixed by moving the node at the beggining of the child list,
so it's probed first.This will give issues on module removal.
Since we usedevice_for_each_childin remove patch, it will try to remove cppi dma controller, while the channel
is still in use by musb node.
Isn't this currently disabled because it blew up in the phy code?
Sebastian
PATCH v2 1/1 ARM: dts: am33xx: Move the cppi41dma node so it's probed early
Hi Sebastian,
Quoted 9 lines · Sebastian Andrzej Siewior
Quoted 7 lines · George Cherian
Quoted 3 lines
Quoted 3 lines
This is easily fixed by moving the node at the beggining of the child list,
so it's probed first.This will give issues on module removal.
Since we usedevice_for_each_childin remove patch, it will try to remove cppi dma controller, while the channel
is still in use by musb node.Isn't this currently disabled because it blew up in the phy code?
Yes. So how if the dt looks like this
usb {
....
....
usb_ctrl_mod {
....
....
};
usb0 {
....
....
};
usb0_phy {
....
....
};
usb1 {
....
....
};
usb1_phy {
....
....
};
cppi41dma {
....
....
};
};Quoted 3 lines
Sebastian
Signature · 1 line
-George
PATCH v2 1/1 ARM: dts: am33xx: Move the cppi41dma node so it's probed early
Quoted 1 line · George Cherian
Hi Sebastian,
Hi George,
Quoted 11 lines · Sebastian Andrzej Siewior
Quoted 9 lines · George Cherian
Quoted 7 lines
Quoted 3 lines
Quoted 3 lines
This is easily fixed by moving the node at the beggining of the child list,
so it's probed first.This will give issues on module removal.
Since we usedevice_for_each_childin remove patch, it will try to remove cppi dma controller, while the channel
is still in use by musb node.Isn't this currently disabled because it blew up in the phy code?
Yes. So how if the dt looks like this
No. I remember we talked about this and we decided not to duct tape the bug by moving the nodes around. Instead we wanted to disable the child removal part (one tiny module that can't be removed) until the PHY code does no longer blow up.
If the same case is for the DMA driver then it should be fixed, too. The order of the nodes in .dts should not crash the kernel under any circumstances. If a different node order accelerates the probing then fine. But a crash is a no no.
Sebastian
PATCH v2 1/1 ARM: dts: am33xx: Move the cppi41dma node so it's probed early
Quoted 17 lines · Sebastian Andrzej Siewior
Quoted 1 line · George Cherian
Hi Sebastian,
Hi George,
Quoted 11 lines · Sebastian Andrzej Siewior
Quoted 9 lines · George Cherian
Quoted 7 lines
Quoted 3 lines
Quoted 3 lines
This is easily fixed by moving the node at the beggining of the child list,
so it's probed first.This will give issues on module removal.
Since we usedevice_for_each_childin remove patch, it will try to remove cppi dma controller, while the channel
is still in use by musb node.Isn't this currently disabled because it blew up in the phy code?
Yes. So how if the dt looks like this
Quoted 9 lines
No. I remember we talked about this and we decided not to duct tape the bug by moving the nodes around. Instead we wanted to disable the child removal part (one tiny module that can't be removed) until the PHY code does no longer blow up.
If the same case is for the DMA driver then it should be fixed, too. The order of the nodes in .dts should not crash the kernel under any circumstances. If a different node order accelerates the probing then fine. But a crash is a no no.
I still don't see any reason to have the DMA controller node as a child of the USB.
However, it seems we need it this way for now; at least until we get rid of the hwmod
altogether.
That said, this DT-ordering to fix other issues is starting to seem a very bad idea.
I'll prepare a v3 to leave the DT as it is, and instead force the driver to probe and
remove the childs so the DMA is probed first and the removed last.
We can always get rid of such workaround, once we can remove the DMA controller out of the USB - as it should be.
Signature · 2 lines
Ezequiel Garc?a, VanguardiaSur www.vanguardiasur.com.ar
PATCH v2 1/1 ARM: dts: am33xx: Move the cppi41dma node so it's probed early
In reply to George CherianHi George,
Quoted 10 lines · George Cherian
Quoted 9 lines · Yegor Yefremov
Quoted 6 lines · 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.
Quoted 9 lines
Quoted 3 lines
Quoted 3 lines
This is easily fixed by moving the node at the beggining of the child list,
so it's probed first.This will give issues on module removal.
Since we usedevice_for_each_childin remove patch, it will try to remove cppi dma controller, while the channel
is still in use by musb node.
OK, this seems confusing: are you sure module removal works?
Doing this simple test on v3.15-rcN:
$ modprobe musb_dsps $ modprobe musb_am335x $ modprobe musb_am335x -r
And the kernel blows up :-(
I've been debugging this and I think we simply cannot support removal of the musb_am335x
module.
Had this ever worked before?
Signature · 2 lines
Ezequiel Garc?a, VanguardiaSur www.vanguardiasur.com.ar
PATCH v2 1/1 ARM: dts: am33xx: Move the cppi41dma node so it's probed early
Quoted 14 lines · Ezequiel Garc?a
Hi George,
Quoted 11 lines · George Cherian
Quoted 10 lines · Yegor Yefremov
Quoted 8 lines · 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.
Quoted 6 lines
Quoted 5 lines
This will give issues on module removal.
Since we usedevice_for_each_childin remove patch, it will try to remove cppi dma controller, while the channel
is still in use by musb node.OK, this seems confusing: are you sure module removal works?
No it does not .
Quoted 7 lines
Doing this simple test on v3.15-rcN:
$ modprobe musb_dsps $ modprobe musb_am335x $ modprobe musb_am335x -r
Quoted 7 lines
And the kernel blows up :-(
I've been debugging this and I think we simply cannot support removal of the
musb_am335x
module.Had this ever worked before
Nope. I feel the whole problem is because how its modeled in dt.
If you look at the TRM following are the memory maps for the USB modules USB control Module 0x44e10_0620
USBSS 0x4740_0000
USB0 0x4740_1000USB0_PHY 0x4740_1300USB0_CORE 0x4740_1400
USB1 0x4740_1800USB1_PHY 0x4740_1b00USB1_CORE 0x4740_1c00
CPPI DMA Controller 0x4740_2000
CPPI DMA Scheduler 0x4740_3000
Queue Manager 0x4740_4000
Now in the curent DT we have the follwoing
USBSS {
usb_ctrl_mod: {
0x44e10_0620
}
usb0_phy:{
0x4740_1300
}
usb0: {
0x4740_1000
0x4740_1400
}
usb1_phy: {
0x4740_1b00
}
usb1:{
0x4740_1800
0x4740_1c00
}
cppi41dma: {
0x4740_2000
0x4740_3000
0x4740_4000
}
}Just by remodelling the dt the whole problem can be solved. I am still not convinced why we should not be doing it? Because neither ways its not the exact representation of the H/W.
I will send a patch as RFC for the same.
Signature · 1 line
-George
PATCH v2 1/1 ARM: dts: am33xx: Move the cppi41dma node so it's probed early
Quoted 3 lines · George Cherian
Just by remodelling the dt the whole problem can be solved. I am still not convinced why we should not be doing it? Because neither ways its not the exact representation of the H/W.
Ha. Now I am confused. First I assumed that the musb_am335x module is built-in only to duct-tape the bug you are seeing. So this patch never made it mainline then.
The problem is as far as I remember the way the phy-core does things and should be fixed. Re-arranging does not help because you can still oops the kernel (the same oops you have now) by removing the devices manually via sysfs.
Sebastian
PATCH v2 1/1 ARM: dts: am33xx: Move the cppi41dma node so it's probed early
Quoted 11 lines · Sebastian Andrzej Siewior
Quoted 3 lines · George Cherian
Just by remodelling the dt the whole problem can be solved. I am still not convinced why we should not be doing it? Because neither ways its not the exact representation of the H/W.
Ha. Now I am confused. First I assumed that the
musb_am335xmodule is built-in only to duct-tape the bug you are seeing. So this patch never made it mainline then.
The problem is as far as I remember the way the phy-core does things and should be fixed. Re-arranging does not help because you can still oops the kernel (the same oops you have now) by removing the devices manually via sysfs.
Okay...
So, You mean to say if I unbind the phy device and then try to remove musb_am335x module,
then it will oops.
Now I got it...
Quoted 6 lines
Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Signature · 1 line
-George
PATCH v2 1/1 ARM: dts: am33xx: Move the cppi41dma node so it's probed early
In reply to Sebastian Andrzej SiewiorQuoted 8 lines · Sebastian Andrzej Siewior
Quoted 3 lines · George Cherian
Just by remodelling the dt the whole problem can be solved. I am still not convinced why we should not be doing it? Because neither ways its not the exact representation of the H/W.
Ha. Now I am confused. First I assumed that the
musb_am335xmodule is built-in only to duct-tape the bug you are seeing. So this patch never made it mainline then.
Mainline panics easily upon module removal:
$ modprobe musb_am335x $ modprobe musb_dsps
Here you insert something to the USB
$ modprobe musb_am335x -r ... bang! the kernel panics.
It works fine if you remove the musb_dsps and musb_am335x in that order, but the dependency is not enforced anywhere.
So I guess preventing the module removal should be fine for now. In that case, mind testing/acking/whatever this patch:
http://www.spinics.net/lists/linux-usb/msg107244.html
Regards,
Signature · 2 lines
Ezequiel Garcia, VanguardiaSur www.vanguardiasur.com.ar
PATCH v2 1/1 ARM: dts: am33xx: Move the cppi41dma node so it's probed early
In reply to Ezequiel GarciaQuoted 8 lines · 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 <[email protected]>
Quoted 59 lines
--
v1->v2:
* Added a comment to prevent a future clean-up based on the memory offset.arch/arm/boot/dts/am33xx.dtsi | 34 ++++++++++++++++++++-------------- 1 file changed, 20 insertions(+), 14 deletions(-)arch/arm/boot/dts/am33xx.dtsi +20
-14@@ -453,6 +453,26 @@ ti,hwmods = "usb_otg_hs"; status = "disabled"; + /* + * The probe order matches the child ordering so the + * dma-controller node must be the first one to prevent + * spurious probe deferrals. + */ + cppi41dma: dma-controller at 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 at 44e10620 { compatible = "ti,am335x-usb-ctrl-module"; reg = <0x44e10620 0x10@@ -556,20 +576,6 @@ "tx14", "tx15"; }; - cppi41dma: dma-controller at 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 at 48300000 {
Signature · 1 line
-George
PATCH v2 1/1 ARM: dts: am33xx: Move the cppi41dma node so it's probed early
Quoted 10 lines · George Cherian
Quoted 8 lines · 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 <[email protected]>Acked-by George Cherian <[email protected]>
Actually, I'm having second thoughts about this patch. I think it's really fragile to depend on the devicetree ordering. Instead, I've prepared a patch that forces the driver to probe the nodes in strict order, based in the compatible string.
Signature · 2 lines
Ezequiel Garcia, VanguardiaSur www.vanguardiasur.com.ar