| Message ID | 1398373881-23369-1-git-send-email-ezequiel@vanguardiasur.com.ar (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Apr 24, 2014 at 11:11 PM, Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> 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. > > Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> Tested-by: Yegor Yefremov <yegorslists@googlemail.com> > -- > 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(-) > > diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi > index 9770e35..02e1eb6 100644 > --- a/arch/arm/boot/dts/am33xx.dtsi > +++ b/arch/arm/boot/dts/am33xx.dtsi > @@ -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@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 +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@48300000 { > -- > 1.9.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-omap" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On 4/29/2014 11:49 AM, Yegor Yefremov wrote: > On Thu, Apr 24, 2014 at 11:11 PM, Ezequiel Garcia > <ezequiel@vanguardiasur.com.ar> 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. 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. >> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> > Tested-by: Yegor Yefremov <yegorslists@googlemail.com> > >> -- >> 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(-) >> >> diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi >> index 9770e35..02e1eb6 100644 >> --- a/arch/arm/boot/dts/am33xx.dtsi >> +++ b/arch/arm/boot/dts/am33xx.dtsi >> @@ -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@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 +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@48300000 { >> -- >> 1.9.1 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-omap" in >> the body of a message to majordomo@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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/29/2014 09:58 AM, George Cherian wrote: >>> 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. Isn't this currently disabled because it blew up in the phy code? Sebastian
Hi Sebastian, On 4/29/2014 1:36 PM, Sebastian Andrzej Siewior wrote: > On 04/29/2014 09:58 AM, George Cherian wrote: >>>> 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. > 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 { .... .... }; }; > > Sebastian >
On 04/29/2014 10:27 AM, George Cherian wrote: > Hi Sebastian, Hi George, > On 4/29/2014 1:36 PM, Sebastian Andrzej Siewior wrote: >> On 04/29/2014 09:58 AM, George Cherian wrote: >>>>> 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. >> 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
On 29 April 2014 06:09, Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote: > On 04/29/2014 10:27 AM, George Cherian wrote: >> Hi Sebastian, > > Hi George, > >> On 4/29/2014 1:36 PM, Sebastian Andrzej Siewior wrote: >>> On 04/29/2014 09:58 AM, George Cherian wrote: >>>>>> 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. >>> 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. > 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.
Hi George, On 29 April 2014 04:58, George Cherian <george.cherian@ti.com> wrote: > On 4/29/2014 11:49 AM, Yegor Yefremov wrote: >> >> On Thu, Apr 24, 2014 at 11:11 PM, Ezequiel Garcia >> <ezequiel@vanguardiasur.com.ar> 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. > > 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. > 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?
On 5/8/2014 10:30 PM, Ezequiel García wrote: > Hi George, > > On 29 April 2014 04:58, George Cherian <george.cherian@ti.com> wrote: >> On 4/29/2014 11:49 AM, Yegor Yefremov wrote: >>> On Thu, Apr 24, 2014 at 11:11 PM, Ezequiel Garcia >>> <ezequiel@vanguardiasur.com.ar> 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. >> 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. >> > OK, this seems confusing: are you sure module removal works? No it does not . > > 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 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_1000 USB0_PHY 0x4740_1300 USB0_CORE 0x4740_1400 USB1 0x4740_1800 USB1_PHY 0x4740_1b00 USB1_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.
On 05/09/2014 08:22 AM, George Cherian wrote: > 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
On 5/9/2014 12:55 PM, Sebastian Andrzej Siewior wrote: > On 05/09/2014 08:22 AM, George Cherian wrote: >> 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. 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... > > Sebastian > -- > To unsubscribe from this list: send the line "unsubscribe linux-omap" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09 May 09:25 AM, Sebastian Andrzej Siewior wrote: > On 05/09/2014 08:22 AM, George Cherian wrote: > > 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. 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,
On 4/25/2014 2:41 AM, 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. > > Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> Acked-by: George Cherian <george.cherian@ti.com> > -- > 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(-) > > diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi > index 9770e35..02e1eb6 100644 > --- a/arch/arm/boot/dts/am33xx.dtsi > +++ b/arch/arm/boot/dts/am33xx.dtsi > @@ -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@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 +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@48300000 {
On 12 May 10:29 AM, George Cherian wrote: > On 4/25/2014 2:41 AM, 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. > > > >Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> > Acked-by: George Cherian <george.cherian@ti.com> 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.
diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi index 9770e35..02e1eb6 100644 --- a/arch/arm/boot/dts/am33xx.dtsi +++ b/arch/arm/boot/dts/am33xx.dtsi @@ -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@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 +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@48300000 {
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@vanguardiasur.com.ar> -- 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(-)