| Message ID | 20130626153325.GA24075@linutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Wed, Jun 26, 2013 at 05:33:26PM +0200, Sebastian Andrzej Siewior wrote: > I've been thinkig about creating two child nodes for the independent musb > controllers on the am33. I've been thinking about the following: > > diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi > index 8e1248f..6aa9506 100644 > --- a/arch/arm/boot/dts/am33xx.dtsi > +++ b/arch/arm/boot/dts/am33xx.dtsi > @@ -326,21 +326,78 @@ > status = "disabled"; > }; > > usb@47400000 { > compatible = "ti,musb-am33xx"; > - reg = <0x47400000 0x1000 /* usbss */ > - 0x47401000 0x800 /* musb instance 0 */ > - 0x47401800 0x800>; /* musb instance 1 */ > - interrupts = <17 /* usbss */ > - 18 /* musb instance 0 */ > - 19>; /* musb instance 1 */ > - multipoint = <1>; > - num-eps = <16>; > - ram-bits = <12>; > - port0-mode = <3>; > - port1-mode = <3>; > - power = <250>; > + reg = <0x47400000 0x1000>; /* usbss */ > + interrupts = <17>; /* usbss */ > ti,hwmods = "usb_otg_hs"; > + > + usb0@0x47401000 { > + reg = <0x47401000 0x800>; /* musb instance 0 */ > + interrupts = <18>; /* musb instance 0 */ > + multipoint = <1>; > + num-eps = <16>; > + ram-bits = <12>; > + port-mode = <3>; > + power = <250>; > + > + phys = <&nopphy0>; > + dmas = <&cppi41dma 0 > + &cppi41dma 1 > + &cppi41dma 2 > + &cppi41dma 3 > + &cppi41dma 4 > + &cppi41dma 32 > + &cppi41dma 34 > + &cppi41dma 36 > + &cppi41dma 38 > + &cppi41dma 40>; > + dma-names = "rx1", "rx2", "rx3", "rx4", "rx5", > + "tx1", "tx2", "tx3", "tx4", "tx5"; > + }; > + > + usb0@0x47401800 { > + reg = <0x47401800 0x800>; /* musb instance 1 */ > + interrupts = <19>; /* musb instance 1 */ > + multipoint = <1>; > + num-eps = <16>; > + ram-bits = <12>; > + port-mode = <3>; > + power = <250>; > + > + phys = <&nopphy1>; > + dmas = <&cppi41dma 0 > + &cppi41dma 1 > + &cppi41dma 2 > + &cppi41dma 3 > + &cppi41dma 4 > + &cppi41dma 62 > + &cppi41dma 64 > + &cppi41dma 66 > + &cppi41dma 68 > + &cppi41dma 70>; > + dma-names = "rx1", "rx2", "rx3", "rx4", "rx5", > + "tx1", "tx2", "tx3", "tx4", "tx5"; > + }; > + }; > > Please keep in mind that his is not a proper patch it should point out > what I have in mind. > This would easy the for instance the dma channel assignment. Also, it will > move back to the port-mode property instead of portX-mode one which it has > now. This is kind of confusing since it is not a root hub or anything like > that it is a complete musb controller. Plus for the phys property could > easily take the one avaible and would not require which instance it > really it is. the patch is alright, but what about the giant amoutn of function pointers we have ? Are you planning to use of_dev_auxdata ??
On 06/27/2013 08:51 AM, Felipe Balbi wrote: > Hi, Hi Felipe, > the patch is alright, but what about the giant amoutn of function > pointers we have ? Are you planning to use of_dev_auxdata ?? I didn't plan to use of_dev_auxdata. What do you mean by "giant amount of function pointers"? 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
Hi, On Thu, Jun 27, 2013 at 09:31:34AM +0200, Sebastian Andrzej Siewior wrote: > > the patch is alright, but what about the giant amoutn of function > > pointers we have ? Are you planning to use of_dev_auxdata ?? > > I didn't plan to use of_dev_auxdata. What do you mean by "giant amount > of function pointers"? every glue layer has: 467 static const struct musb_platform_ops omap2430_ops = { 468 .init = omap2430_musb_init, 469 .exit = omap2430_musb_exit, 470 471 .set_mode = omap2430_musb_set_mode, 472 .try_idle = omap2430_musb_try_idle, 473 474 .set_vbus = omap2430_musb_set_vbus, 475 476 .enable = omap2430_musb_enable, 477 .disable = omap2430_musb_disable, 478 }; note that in the glue layer, they have access to musb itself (biggest mistake of my life, btw :-)
Hi Sebastian, On 06/26/2013 05:33 PM, Sebastian Andrzej Siewior wrote: > I've been thinkig about creating two child nodes for the independent musb > controllers on the am33. I've been thinking about the following: > > diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi > index 8e1248f..6aa9506 100644 > --- a/arch/arm/boot/dts/am33xx.dtsi > +++ b/arch/arm/boot/dts/am33xx.dtsi > @@ -326,21 +326,78 @@ > status = "disabled"; > }; > > usb@47400000 { > compatible = "ti,musb-am33xx"; > - reg = <0x47400000 0x1000 /* usbss */ > - 0x47401000 0x800 /* musb instance 0 */ > - 0x47401800 0x800>; /* musb instance 1 */ > - interrupts = <17 /* usbss */ > - 18 /* musb instance 0 */ > - 19>; /* musb instance 1 */ > - multipoint = <1>; > - num-eps = <16>; > - ram-bits = <12>; > - port0-mode = <3>; > - port1-mode = <3>; > - power = <250>; > + reg = <0x47400000 0x1000>; /* usbss */ > + interrupts = <17>; BTW, why do have so many DMA compared to the previous version? /* usbss */ > ti,hwmods = "usb_otg_hs"; > + > + usb0@0x47401000 { > + reg = <0x47401000 0x800>; /* musb instance 0 */ > + interrupts = <18>; /* musb instance 0 */ > + multipoint = <1>; > + num-eps = <16>; > + ram-bits = <12>; > + port-mode = <3>; > + power = <250>; > + > + phys = <&nopphy0>; > + dmas = <&cppi41dma 0 > + &cppi41dma 1 > + &cppi41dma 2 > + &cppi41dma 3 > + &cppi41dma 4 > + &cppi41dma 32 > + &cppi41dma 34 > + &cppi41dma 36 > + &cppi41dma 38 > + &cppi41dma 40>; > + dma-names = "rx1", "rx2", "rx3", "rx4", "rx5", > + "tx1", "tx2", "tx3", "tx4", "tx5"; > + }; > + > + usb0@0x47401800 { > + reg = <0x47401800 0x800>; /* musb instance 1 */ > + interrupts = <19>; /* musb instance 1 */ > + multipoint = <1>; > + num-eps = <16>; > + ram-bits = <12>; > + port-mode = <3>; > + power = <250>; > + > + phys = <&nopphy1>; > + dmas = <&cppi41dma 0 > + &cppi41dma 1 > + &cppi41dma 2 > + &cppi41dma 3 > + &cppi41dma 4 > + &cppi41dma 62 > + &cppi41dma 64 > + &cppi41dma 66 > + &cppi41dma 68 > + &cppi41dma 70>; > + dma-names = "rx1", "rx2", "rx3", "rx4", "rx5", > + "tx1", "tx2", "tx3", "tx4", "tx5"; > + }; > + }; > > Please keep in mind that his is not a proper patch it should point out > what I have in mind. > This would easy the for instance the dma channel assignment. Also, it will > move back to the port-mode property instead of portX-mode one which it has > now. This is kind of confusing since it is not a root hub or anything like > that it is a complete musb controller. Plus for the phys property could > easily take the one avaible and would not require which instance it > really it is. I don't know the HW in detail, but that looks indeed much better and seems to reflect the partitioning with multiple instances accurately. If Felipe is OK with that partitioning, I'll take it. Thanks, Benoit -- 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 06/27/2013 01:23 PM, Benoit Cousson wrote: > Hi Sebastian, Hi Benoit, > BTW, why do have so many DMA compared to the previous version? I added them, the previous had none and is PIO only. I currently use three cells per dma channel (the posted example had two). In general I think I have to request 2 * 15 dma channels for the 15 TX and RX endpoints. So we end up with 30 channels per device (and we have two of them). The DMA engine has specific queues which depend on the USB endpoint that is used. Instead of hacking this information in some kind of platform data I decided to push this detail into device tree. > I don't know the HW in detail, but that looks indeed much better and > seems to reflect the partitioning with multiple instances accurately. > > If Felipe is OK with that partitioning, I'll take it. Okay, thanks. > > Thanks, > Benoit 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 Thu, Jun 27, 2013 at 01:23:59PM +0200, Benoit Cousson wrote: > Hi Sebastian, > > On 06/26/2013 05:33 PM, Sebastian Andrzej Siewior wrote: > > I've been thinkig about creating two child nodes for the independent musb > > controllers on the am33. I've been thinking about the following: > > > > diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi > > index 8e1248f..6aa9506 100644 > > --- a/arch/arm/boot/dts/am33xx.dtsi > > +++ b/arch/arm/boot/dts/am33xx.dtsi > > @@ -326,21 +326,78 @@ > > status = "disabled"; > > }; > > > > usb@47400000 { > > compatible = "ti,musb-am33xx"; > > - reg = <0x47400000 0x1000 /* usbss */ > > - 0x47401000 0x800 /* musb instance 0 */ > > - 0x47401800 0x800>; /* musb instance 1 */ > > - interrupts = <17 /* usbss */ > > - 18 /* musb instance 0 */ > > - 19>; /* musb instance 1 */ > > - multipoint = <1>; > > - num-eps = <16>; > > - ram-bits = <12>; > > - port0-mode = <3>; > > - port1-mode = <3>; > > - power = <250>; > > + reg = <0x47400000 0x1000>; /* usbss */ > > + interrupts = <17>; trailing whitespaces here > BTW, why do have so many DMA compared to the previous version? > /* usbss */ > > ti,hwmods = "usb_otg_hs"; > > + > > + usb0@0x47401000 { > > + reg = <0x47401000 0x800>; /* musb instance 0 */ > > + interrupts = <18>; /* musb instance 0 */ > > + multipoint = <1>; > > + num-eps = <16>; > > + ram-bits = <12>; > > + port-mode = <3>; > > + power = <250>; > > + > > + phys = <&nopphy0>; > > + dmas = <&cppi41dma 0 > > + &cppi41dma 1 > > + &cppi41dma 2 > > + &cppi41dma 3 > > + &cppi41dma 4 > > + &cppi41dma 32 > > + &cppi41dma 34 > > + &cppi41dma 36 > > + &cppi41dma 38 > > + &cppi41dma 40>; > > + dma-names = "rx1", "rx2", "rx3", "rx4", "rx5", > > + "tx1", "tx2", "tx3", "tx4", "tx5"; > > + }; > > + > > + usb0@0x47401800 { > > + reg = <0x47401800 0x800>; /* musb instance 1 */ > > + interrupts = <19>; /* musb instance 1 */ > > + multipoint = <1>; > > + num-eps = <16>; > > + ram-bits = <12>; > > + port-mode = <3>; > > + power = <250>; > > + > > + phys = <&nopphy1>; > > + dmas = <&cppi41dma 0 > > + &cppi41dma 1 > > + &cppi41dma 2 > > + &cppi41dma 3 > > + &cppi41dma 4 > > + &cppi41dma 62 > > + &cppi41dma 64 > > + &cppi41dma 66 > > + &cppi41dma 68 > > + &cppi41dma 70>; > > + dma-names = "rx1", "rx2", "rx3", "rx4", "rx5", > > + "tx1", "tx2", "tx3", "tx4", "tx5"; > > + }; > > + }; > > > > Please keep in mind that his is not a proper patch it should point out > > what I have in mind. > > This would easy the for instance the dma channel assignment. Also, it will > > move back to the port-mode property instead of portX-mode one which it has > > now. This is kind of confusing since it is not a root hub or anything like > > that it is a complete musb controller. Plus for the phys property could > > easily take the one avaible and would not require which instance it > > really it is. > > I don't know the HW in detail, but that looks indeed much better and > seems to reflect the partitioning with multiple instances accurately. > > If Felipe is OK with that partitioning, I'll take it. it's alright with me Acked-by: Felipe Balbi <balbi@ti.com>
diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi index 8e1248f..6aa9506 100644 --- a/arch/arm/boot/dts/am33xx.dtsi +++ b/arch/arm/boot/dts/am33xx.dtsi @@ -326,21 +326,78 @@ status = "disabled"; }; usb@47400000 { compatible = "ti,musb-am33xx"; - reg = <0x47400000 0x1000 /* usbss */ - 0x47401000 0x800 /* musb instance 0 */ - 0x47401800 0x800>; /* musb instance 1 */ - interrupts = <17 /* usbss */ - 18 /* musb instance 0 */ - 19>; /* musb instance 1 */ - multipoint = <1>; - num-eps = <16>; - ram-bits = <12>; - port0-mode = <3>; - port1-mode = <3>; - power = <250>; + reg = <0x47400000 0x1000>; /* usbss */ + interrupts = <17>; /* usbss */ ti,hwmods = "usb_otg_hs"; + + usb0@0x47401000 { + reg = <0x47401000 0x800>; /* musb instance 0 */ + interrupts = <18>; /* musb instance 0 */ + multipoint = <1>; + num-eps = <16>; + ram-bits = <12>; + port-mode = <3>; + power = <250>; + + phys = <&nopphy0>; + dmas = <&cppi41dma 0 + &cppi41dma 1 + &cppi41dma 2 + &cppi41dma 3 + &cppi41dma 4 + &cppi41dma 32 + &cppi41dma 34 + &cppi41dma 36 + &cppi41dma 38 + &cppi41dma 40>; + dma-names = "rx1", "rx2", "rx3", "rx4", "rx5", + "tx1", "tx2", "tx3", "tx4", "tx5"; + }; + + usb0@0x47401800 { + reg = <0x47401800 0x800>; /* musb instance 1 */ + interrupts = <19>; /* musb instance 1 */ + multipoint = <1>; + num-eps = <16>; + ram-bits = <12>; + port-mode = <3>; + power = <250>; + + phys = <&nopphy1>; + dmas = <&cppi41dma 0 + &cppi41dma 1 + &cppi41dma 2 + &cppi41dma 3 + &cppi41dma 4 + &cppi41dma 62 + &cppi41dma 64 + &cppi41dma 66 + &cppi41dma 68 + &cppi41dma 70>; + dma-names = "rx1", "rx2", "rx3", "rx4", "rx5", + "tx1", "tx2", "tx3", "tx4", "tx5"; + }; + }; Please keep in mind that his is not a proper patch it should point out what I have in mind. This would easy the for instance the dma channel assignment. Also, it will move back to the port-mode property instead of portX-mode one which it has now. This is kind of confusing since it is not a root hub or anything like that it is a complete musb controller. Plus for the phys property could easily take the one avaible and would not require which instance it really it is. 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