[RFC] am33xx: create child nodes for the two musb controllers

[RFC] am33xx: create child nodes for the two musb controllers

[Sebastian Andrzej Siewior]

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.

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: [RFC] am33xx: create child nodes for the two musb controllers

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 3010 bytes --]

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 ??

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC] am33xx: create child nodes for the two musb controllers

[Sebastian Andrzej Siewior]

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

Re: [RFC] am33xx: create child nodes for the two musb controllers

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 928 bytes --]

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 :-)

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC] am33xx: create child nodes for the two musb controllers

[Benoit Cousson]

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

Re: [RFC] am33xx: create child nodes for the two musb controllers

[Sebastian Andrzej Siewior]

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

Re: [RFC] am33xx: create child nodes for the two musb controllers

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 3490 bytes --]

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>

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]