[U-Boot] usb: ehci: Take advantage of the new multi-controller feature for MXC

[U-Boot] usb: ehci: Take advantage of the new multi-controller feature for MXC

[Benoît Thébaudeau]

Hi Marek,

Thanks to Lucas' series coming with commits c7e3b2b and 676ae06, I'd like to use
the multi-controller feature on MXC since most of these SoCs come with a USB IP
supporting an OTG controller and multiple host-only controllers.

Currently the MXC code in ehci-mx{c|5|6}.c just ignores the index passed to
ehci_hcd_init() and the like, and there are 3 port-specific configs
(CONFIG_MXC_USB_PORT, CONFIG_MXC_USB_FLAGS and CONFIG_MXC_USB_PORTSC).

Not all USB ports from the USB IP will be available on each board for a given
SoC, so we need a logical to physical USB port mapping.

I would suggest something like the following.

board.h:
#define CONFIG_MXC_USB	{ \
	{ \
		0, \
		MXC_EHCI_INTERNAL_PHY, \
		MXC_EHCI_UTMI_16BIT | MXC_EHCI_MODE_UTMI \
	}, { \
		1, \
		MXC_EHCI_POWER_PINS_ENABLED | MXC_EHCI_PWR_PIN_ACTIVE_HIGH | \
		MXC_EHCI_OC_PIN_ACTIVE_LOW, \
		MXC_EHCI_MODE_ULPI \
	}, \
}

ehci-fsl.h:
struct mxc_ehci_cfg {
	int port;
	u32 flags;
	u32 portsc;
};

ehci-mx{c|5|6}.c:
static const struct mxc_ehci_cfg cfg[CONFIG_USB_MAX_CONTROLLER_COUNT] =
		CONFIG_MXC_USB;

Then, in ehci_hcd_init(), there would be the following replacements:
 - CONFIG_MXC_USB_PORT -> cfg[index].port,
 - CONFIG_MXC_USB_FLAGS -> cfg[index].flags,
 - CONFIG_MXC_USB_PORTSC -> cfg[index].portsc.

What do you think?

Best regards,
Beno?t

[U-Boot] usb: ehci: Take advantage of the new multi-controller feature for MXC

[Marek Vasut]

Dear Beno?t Th?baudeau,

> Hi Marek,
> 
> Thanks to Lucas' series coming with commits c7e3b2b and 676ae06, I'd like
> to use the multi-controller feature on MXC since most of these SoCs come
> with a USB IP supporting an OTG controller and multiple host-only
> controllers.
> 
> Currently the MXC code in ehci-mx{c|5|6}.c just ignores the index passed to
> ehci_hcd_init() and the like, and there are 3 port-specific configs
> (CONFIG_MXC_USB_PORT, CONFIG_MXC_USB_FLAGS and CONFIG_MXC_USB_PORTSC).
> 
> Not all USB ports from the USB IP will be available on each board for a
> given SoC, so we need a logical to physical USB port mapping.
> 
> I would suggest something like the following.
> 
> board.h:
> #define CONFIG_MXC_USB	{ \
> 	{ \
> 		0, \
> 		MXC_EHCI_INTERNAL_PHY, \
> 		MXC_EHCI_UTMI_16BIT | MXC_EHCI_MODE_UTMI \
> 	}, { \
> 		1, \
> 		MXC_EHCI_POWER_PINS_ENABLED | MXC_EHCI_PWR_PIN_ACTIVE_HIGH | \
> 		MXC_EHCI_OC_PIN_ACTIVE_LOW, \
> 		MXC_EHCI_MODE_ULPI \
> 	}, \
> }
> 
> ehci-fsl.h:
> struct mxc_ehci_cfg {
> 	int port;
> 	u32 flags;
> 	u32 portsc;
> };
> 
> ehci-mx{c|5|6}.c:
> static const struct mxc_ehci_cfg cfg[CONFIG_USB_MAX_CONTROLLER_COUNT] =
> 		CONFIG_MXC_USB;
> 
> Then, in ehci_hcd_init(), there would be the following replacements:
>  - CONFIG_MXC_USB_PORT -> cfg[index].port,
>  - CONFIG_MXC_USB_FLAGS -> cfg[index].flags,
>  - CONFIG_MXC_USB_PORTSC -> cfg[index].portsc.
> 
> What do you think?

What about passing port private / platform data instead of ID ?

Best regards,
Marek Vasut

[U-Boot] usb: ehci: Take advantage of the new multi-controller feature for MXC

[Benoît Thébaudeau]

Dear Marek Vasut,

On Monday, November 5, 2012 11:54:12 PM, Marek Vasut wrote:
> Dear Beno?t Th?baudeau,
> 
> > Hi Marek,
> > 
> > Thanks to Lucas' series coming with commits c7e3b2b and 676ae06,
> > I'd like
> > to use the multi-controller feature on MXC since most of these SoCs
> > come
> > with a USB IP supporting an OTG controller and multiple host-only
> > controllers.
> > 
> > Currently the MXC code in ehci-mx{c|5|6}.c just ignores the index
> > passed to
> > ehci_hcd_init() and the like, and there are 3 port-specific configs
> > (CONFIG_MXC_USB_PORT, CONFIG_MXC_USB_FLAGS and
> > CONFIG_MXC_USB_PORTSC).
> > 
> > Not all USB ports from the USB IP will be available on each board
> > for a
> > given SoC, so we need a logical to physical USB port mapping.
> > 
> > I would suggest something like the following.
> > 
> > board.h:
> > #define CONFIG_MXC_USB	{ \
> > 	{ \
> > 		0, \
> > 		MXC_EHCI_INTERNAL_PHY, \
> > 		MXC_EHCI_UTMI_16BIT | MXC_EHCI_MODE_UTMI \
> > 	}, { \
> > 		1, \
> > 		MXC_EHCI_POWER_PINS_ENABLED | MXC_EHCI_PWR_PIN_ACTIVE_HIGH | \
> > 		MXC_EHCI_OC_PIN_ACTIVE_LOW, \
> > 		MXC_EHCI_MODE_ULPI \
> > 	}, \
> > }
> > 
> > ehci-fsl.h:
> > struct mxc_ehci_cfg {
> > 	int port;
> > 	u32 flags;
> > 	u32 portsc;
> > };
> > 
> > ehci-mx{c|5|6}.c:
> > static const struct mxc_ehci_cfg
> > cfg[CONFIG_USB_MAX_CONTROLLER_COUNT] =
> > 		CONFIG_MXC_USB;
> > 
> > Then, in ehci_hcd_init(), there would be the following
> > replacements:
> >  - CONFIG_MXC_USB_PORT -> cfg[index].port,
> >  - CONFIG_MXC_USB_FLAGS -> cfg[index].flags,
> >  - CONFIG_MXC_USB_PORTSC -> cfg[index].portsc.
> > 
> > What do you think?
> 
> What about passing port private / platform data instead of ID ?

The ID is already passed to ehci_hcd_init(), so we have to live with it if we
don't want to change the newly introduced multi-controller infrastructure.

Or, perhaps this is what you meant, we could have some:
int ehci_mxc_register(int index, const struct mxc_ehci_cfg *cfg);
This function would simply fill an entry in the cfg array in ehci-mx{c|5|6}.c,
this array becoming an array of pointers to struct mxc_ehci_cfg. This looks
nicer, but it needs more code to do just the same thing as the CONFIG_MXC_USB
would do, without adding any feature. The only benefit would be if index were
actually the same as port here, but ehci_hcd_init() would still be called for
all indexes, so it would have to fail e.g. if port 0 is unused but port 1 is
used, which would probably generate some error noise for the user.

Or did you mean something else?

Best regards,
Beno?t

[U-Boot] usb: ehci: Take advantage of the new multi-controller feature for MXC

[Marek Vasut]

Dear Beno?t Th?baudeau,

> Dear Marek Vasut,
> 
> On Monday, November 5, 2012 11:54:12 PM, Marek Vasut wrote:
> > Dear Beno?t Th?baudeau,
> > 
> > > Hi Marek,
> > > 
> > > Thanks to Lucas' series coming with commits c7e3b2b and 676ae06,
> > > I'd like
> > > to use the multi-controller feature on MXC since most of these SoCs
> > > come
> > > with a USB IP supporting an OTG controller and multiple host-only
> > > controllers.
> > > 
> > > Currently the MXC code in ehci-mx{c|5|6}.c just ignores the index
> > > passed to
> > > ehci_hcd_init() and the like, and there are 3 port-specific configs
> > > (CONFIG_MXC_USB_PORT, CONFIG_MXC_USB_FLAGS and
> > > CONFIG_MXC_USB_PORTSC).
> > > 
> > > Not all USB ports from the USB IP will be available on each board
> > > for a
> > > given SoC, so we need a logical to physical USB port mapping.
> > > 
> > > I would suggest something like the following.
> > > 
> > > board.h:
> > > #define CONFIG_MXC_USB	{ \
> > > 
> > > 	{ \
> > > 	
> > > 		0, \
> > > 		MXC_EHCI_INTERNAL_PHY, \
> > > 		MXC_EHCI_UTMI_16BIT | MXC_EHCI_MODE_UTMI \
> > > 	
> > > 	}, { \
> > > 	
> > > 		1, \
> > > 		MXC_EHCI_POWER_PINS_ENABLED | MXC_EHCI_PWR_PIN_ACTIVE_HIGH | \
> > > 		MXC_EHCI_OC_PIN_ACTIVE_LOW, \
> > > 		MXC_EHCI_MODE_ULPI \
> > > 	
> > > 	}, \
> > > 
> > > }
> > > 
> > > ehci-fsl.h:
> > > struct mxc_ehci_cfg {
> > > 
> > > 	int port;
> > > 	u32 flags;
> > > 	u32 portsc;
> > > 
> > > };
> > > 
> > > ehci-mx{c|5|6}.c:
> > > static const struct mxc_ehci_cfg
> > > cfg[CONFIG_USB_MAX_CONTROLLER_COUNT] =
> > > 
> > > 		CONFIG_MXC_USB;
> > > 
> > > Then, in ehci_hcd_init(), there would be the following
> > > 
> > > replacements:
> > >  - CONFIG_MXC_USB_PORT -> cfg[index].port,
> > >  - CONFIG_MXC_USB_FLAGS -> cfg[index].flags,
> > >  - CONFIG_MXC_USB_PORTSC -> cfg[index].portsc.
> > > 
> > > What do you think?
> > 
> > What about passing port private / platform data instead of ID ?
> 
> The ID is already passed to ehci_hcd_init(), so we have to live with it if
> we don't want to change the newly introduced multi-controller
> infrastructure.

Let's change it .... remove the ID and pass some generic pdata.

> Or, perhaps this is what you meant, we could have some:
> int ehci_mxc_register(int index, const struct mxc_ehci_cfg *cfg);
> This function would simply fill an entry in the cfg array in
> ehci-mx{c|5|6}.c, this array becoming an array of pointers to struct
> mxc_ehci_cfg. This looks nicer, but it needs more code to do just the same
> thing as the CONFIG_MXC_USB would do, without adding any feature. The only
> benefit would be if index were actually the same as port here, but
> ehci_hcd_init() would still be called for all indexes, so it would have to
> fail e.g. if port 0 is unused but port 1 is used, which would probably
> generate some error noise for the user.
> 
> Or did you mean something else?
> 
> Best regards,
> Beno?t

[U-Boot] usb: ehci: Take advantage of the new multi-controller feature for MXC

[Lucas Stach]

Am Dienstag, den 06.11.2012, 00:56 +0100 schrieb Marek Vasut:
> Dear Beno?t Th?baudeau,
> 
> > Dear Marek Vasut,
> > 
> > On Monday, November 5, 2012 11:54:12 PM, Marek Vasut wrote:
> > > Dear Beno?t Th?baudeau,
> > > 
> > > > Hi Marek,
> > > > 
> > > > Thanks to Lucas' series coming with commits c7e3b2b and 676ae06,
> > > > I'd like
> > > > to use the multi-controller feature on MXC since most of these SoCs
> > > > come
> > > > with a USB IP supporting an OTG controller and multiple host-only
> > > > controllers.
> > > > 
> > > > Currently the MXC code in ehci-mx{c|5|6}.c just ignores the index
> > > > passed to
> > > > ehci_hcd_init() and the like, and there are 3 port-specific configs
> > > > (CONFIG_MXC_USB_PORT, CONFIG_MXC_USB_FLAGS and
> > > > CONFIG_MXC_USB_PORTSC).
> > > > 
> > > > Not all USB ports from the USB IP will be available on each board
> > > > for a
> > > > given SoC, so we need a logical to physical USB port mapping.
> > > > 
> > > > I would suggest something like the following.
> > > > 
> > > > board.h:
> > > > #define CONFIG_MXC_USB	{ \
> > > > 
> > > > 	{ \
> > > > 	
> > > > 		0, \
> > > > 		MXC_EHCI_INTERNAL_PHY, \
> > > > 		MXC_EHCI_UTMI_16BIT | MXC_EHCI_MODE_UTMI \
> > > > 	
> > > > 	}, { \
> > > > 	
> > > > 		1, \
> > > > 		MXC_EHCI_POWER_PINS_ENABLED | MXC_EHCI_PWR_PIN_ACTIVE_HIGH | \
> > > > 		MXC_EHCI_OC_PIN_ACTIVE_LOW, \
> > > > 		MXC_EHCI_MODE_ULPI \
> > > > 	
> > > > 	}, \
> > > > 
> > > > }
> > > > 
> > > > ehci-fsl.h:
> > > > struct mxc_ehci_cfg {
> > > > 
> > > > 	int port;
> > > > 	u32 flags;
> > > > 	u32 portsc;
> > > > 
> > > > };
> > > > 
> > > > ehci-mx{c|5|6}.c:
> > > > static const struct mxc_ehci_cfg
> > > > cfg[CONFIG_USB_MAX_CONTROLLER_COUNT] =
> > > > 
> > > > 		CONFIG_MXC_USB;
> > > > 
> > > > Then, in ehci_hcd_init(), there would be the following
> > > > 
> > > > replacements:
> > > >  - CONFIG_MXC_USB_PORT -> cfg[index].port,
> > > >  - CONFIG_MXC_USB_FLAGS -> cfg[index].flags,
> > > >  - CONFIG_MXC_USB_PORTSC -> cfg[index].portsc.
> > > > 
> > > > What do you think?
> > > 
> > > What about passing port private / platform data instead of ID ?
> > 
> > The ID is already passed to ehci_hcd_init(), so we have to live with it if
> > we don't want to change the newly introduced multi-controller
> > infrastructure.
> 
> Let's change it .... remove the ID and pass some generic pdata.
> 
I don't like the idea of passing around data at this level. It's
breaking the abstraction, as we have to pass low-level usb information
around in the higher USB stack levels.

The USB driver code should be able to do the virt-to-phys controller
mapping on it's own. In the Tegra world we use the information we get
from device tree to do so, but I don't see a reason why your USB host
driver code wouldn't be able to just require an array with configuration
data from the board file.

There is really no need to pass this information through all the USB
stack interfaces.

Regards,
Lucas

[U-Boot] usb: ehci: Take advantage of the new multi-controller feature for MXC

[Benoît Thébaudeau]

Dear Lucas Stach,

On Tuesday, November 6, 2012 8:43:43 AM, Lucas Stach wrote:
> Am Dienstag, den 06.11.2012, 00:56 +0100 schrieb Marek Vasut:
> > Dear Beno?t Th?baudeau,
> > 
> > > Dear Marek Vasut,
> > > 
> > > On Monday, November 5, 2012 11:54:12 PM, Marek Vasut wrote:
> > > > Dear Beno?t Th?baudeau,
> > > > 
> > > > > Hi Marek,
> > > > > 
> > > > > Thanks to Lucas' series coming with commits c7e3b2b and
> > > > > 676ae06,
> > > > > I'd like
> > > > > to use the multi-controller feature on MXC since most of
> > > > > these SoCs
> > > > > come
> > > > > with a USB IP supporting an OTG controller and multiple
> > > > > host-only
> > > > > controllers.
> > > > > 
> > > > > Currently the MXC code in ehci-mx{c|5|6}.c just ignores the
> > > > > index
> > > > > passed to
> > > > > ehci_hcd_init() and the like, and there are 3 port-specific
> > > > > configs
> > > > > (CONFIG_MXC_USB_PORT, CONFIG_MXC_USB_FLAGS and
> > > > > CONFIG_MXC_USB_PORTSC).
> > > > > 
> > > > > Not all USB ports from the USB IP will be available on each
> > > > > board
> > > > > for a
> > > > > given SoC, so we need a logical to physical USB port mapping.
> > > > > 
> > > > > I would suggest something like the following.
> > > > > 
> > > > > board.h:
> > > > > #define CONFIG_MXC_USB	{ \
> > > > > 
> > > > > 	{ \
> > > > > 	
> > > > > 		0, \
> > > > > 		MXC_EHCI_INTERNAL_PHY, \
> > > > > 		MXC_EHCI_UTMI_16BIT | MXC_EHCI_MODE_UTMI \
> > > > > 	
> > > > > 	}, { \
> > > > > 	
> > > > > 		1, \
> > > > > 		MXC_EHCI_POWER_PINS_ENABLED | MXC_EHCI_PWR_PIN_ACTIVE_HIGH
> > > > > 		| \
> > > > > 		MXC_EHCI_OC_PIN_ACTIVE_LOW, \
> > > > > 		MXC_EHCI_MODE_ULPI \
> > > > > 	
> > > > > 	}, \
> > > > > 
> > > > > }
> > > > > 
> > > > > ehci-fsl.h:
> > > > > struct mxc_ehci_cfg {
> > > > > 
> > > > > 	int port;
> > > > > 	u32 flags;
> > > > > 	u32 portsc;
> > > > > 
> > > > > };
> > > > > 
> > > > > ehci-mx{c|5|6}.c:
> > > > > static const struct mxc_ehci_cfg
> > > > > cfg[CONFIG_USB_MAX_CONTROLLER_COUNT] =
> > > > > 
> > > > > 		CONFIG_MXC_USB;
> > > > > 
> > > > > Then, in ehci_hcd_init(), there would be the following
> > > > > 
> > > > > replacements:
> > > > >  - CONFIG_MXC_USB_PORT -> cfg[index].port,
> > > > >  - CONFIG_MXC_USB_FLAGS -> cfg[index].flags,
> > > > >  - CONFIG_MXC_USB_PORTSC -> cfg[index].portsc.
> > > > > 
> > > > > What do you think?
> > > > 
> > > > What about passing port private / platform data instead of ID ?
> > > 
> > > The ID is already passed to ehci_hcd_init(), so we have to live
> > > with it if
> > > we don't want to change the newly introduced multi-controller
> > > infrastructure.
> > 
> > Let's change it .... remove the ID and pass some generic pdata.
> > 
> I don't like the idea of passing around data at this level. It's
> breaking the abstraction, as we have to pass low-level usb
> information
> around in the higher USB stack levels.
> 
> The USB driver code should be able to do the virt-to-phys controller
> mapping on it's own. In the Tegra world we use the information we get
> from device tree to do so, but I don't see a reason why your USB host
> driver code wouldn't be able to just require an array with
> configuration
> data from the board file.
> 
> There is really no need to pass this information through all the USB
> stack interfaces.

I agree, all the more ehci_hcd_init() is called from cmd_usb.c, completely
outside of any board init context, so collecting the platform data would be a
real pain, without bringing much. And moving usb_init() calls to board init
context would also not be good because of the added boot time.

IMHO, the best solutions here are either a CONFIG_MXC_USB as I suggested, or the
same structure passed to some init function specific to these EHCI drivers
(which would add more code for little benefit).

Marek? Stefano?

Best regards,
Beno?t

[U-Boot] usb: ehci: Take advantage of the new multi-controller feature for MXC

[Marek Vasut]

Dear Beno?t Th?baudeau,

> Dear Lucas Stach,
> 
> On Tuesday, November 6, 2012 8:43:43 AM, Lucas Stach wrote:
> > Am Dienstag, den 06.11.2012, 00:56 +0100 schrieb Marek Vasut:
> > > Dear Beno?t Th?baudeau,
> > > 
> > > > Dear Marek Vasut,
> > > > 
> > > > On Monday, November 5, 2012 11:54:12 PM, Marek Vasut wrote:
> > > > > Dear Beno?t Th?baudeau,
> > > > > 
> > > > > > Hi Marek,
> > > > > > 
> > > > > > Thanks to Lucas' series coming with commits c7e3b2b and
> > > > > > 676ae06,
> > > > > > I'd like
> > > > > > to use the multi-controller feature on MXC since most of
> > > > > > these SoCs
> > > > > > come
> > > > > > with a USB IP supporting an OTG controller and multiple
> > > > > > host-only
> > > > > > controllers.
> > > > > > 
> > > > > > Currently the MXC code in ehci-mx{c|5|6}.c just ignores the
> > > > > > index
> > > > > > passed to
> > > > > > ehci_hcd_init() and the like, and there are 3 port-specific
> > > > > > configs
> > > > > > (CONFIG_MXC_USB_PORT, CONFIG_MXC_USB_FLAGS and
> > > > > > CONFIG_MXC_USB_PORTSC).
> > > > > > 
> > > > > > Not all USB ports from the USB IP will be available on each
> > > > > > board
> > > > > > for a
> > > > > > given SoC, so we need a logical to physical USB port mapping.
> > > > > > 
> > > > > > I would suggest something like the following.
> > > > > > 
> > > > > > board.h:
> > > > > > #define CONFIG_MXC_USB	{ \
> > > > > > 
> > > > > > 	{ \
> > > > > > 	
> > > > > > 		0, \
> > > > > > 		MXC_EHCI_INTERNAL_PHY, \
> > > > > > 		MXC_EHCI_UTMI_16BIT | MXC_EHCI_MODE_UTMI \
> > > > > > 	
> > > > > > 	}, { \
> > > > > > 	
> > > > > > 		1, \
> > > > > > 		MXC_EHCI_POWER_PINS_ENABLED | 
MXC_EHCI_PWR_PIN_ACTIVE_HIGH
> > > > > > 		
> > > > > > 		| \
> > > > > > 		
> > > > > > 		MXC_EHCI_OC_PIN_ACTIVE_LOW, \
> > > > > > 		MXC_EHCI_MODE_ULPI \
> > > > > > 	
> > > > > > 	}, \
> > > > > > 
> > > > > > }
> > > > > > 
> > > > > > ehci-fsl.h:
> > > > > > struct mxc_ehci_cfg {
> > > > > > 
> > > > > > 	int port;
> > > > > > 	u32 flags;
> > > > > > 	u32 portsc;
> > > > > > 
> > > > > > };
> > > > > > 
> > > > > > ehci-mx{c|5|6}.c:
> > > > > > static const struct mxc_ehci_cfg
> > > > > > cfg[CONFIG_USB_MAX_CONTROLLER_COUNT] =
> > > > > > 
> > > > > > 		CONFIG_MXC_USB;
> > > > > > 
> > > > > > Then, in ehci_hcd_init(), there would be the following
> > > > > > 
> > > > > > replacements:
> > > > > >  - CONFIG_MXC_USB_PORT -> cfg[index].port,
> > > > > >  - CONFIG_MXC_USB_FLAGS -> cfg[index].flags,
> > > > > >  - CONFIG_MXC_USB_PORTSC -> cfg[index].portsc.
> > > > > > 
> > > > > > What do you think?
> > > > > 
> > > > > What about passing port private / platform data instead of ID ?
> > > > 
> > > > The ID is already passed to ehci_hcd_init(), so we have to live
> > > > with it if
> > > > we don't want to change the newly introduced multi-controller
> > > > infrastructure.
> > > 
> > > Let's change it .... remove the ID and pass some generic pdata.
> > 
> > I don't like the idea of passing around data at this level. It's
> > breaking the abstraction, as we have to pass low-level usb
> > information
> > around in the higher USB stack levels.
> > 
> > The USB driver code should be able to do the virt-to-phys controller
> > mapping on it's own. In the Tegra world we use the information we get
> > from device tree to do so, but I don't see a reason why your USB host
> > driver code wouldn't be able to just require an array with
> > configuration
> > data from the board file.
> > 
> > There is really no need to pass this information through all the USB
> > stack interfaces.
> 
> I agree, all the more ehci_hcd_init() is called from cmd_usb.c, completely
> outside of any board init context, so collecting the platform data would be
> a real pain, without bringing much. And moving usb_init() calls to board
> init context would also not be good because of the added boot time.
> 
> IMHO, the best solutions here are either a CONFIG_MXC_USB as I suggested,
> or the same structure passed to some init function specific to these EHCI
> drivers (which would add more code for little benefit).

I disagree ... mapping function is fine, but I'd like to be able to pass around 
pointer to some platform data.

[U-Boot] usb: ehci: Take advantage of the new multi-controller feature for MXC

[Marek Vasut]

Dear Lucas Stach,

[...]

> > > > > What do you think?
> > > > 
> > > > What about passing port private / platform data instead of ID ?
> > > 
> > > The ID is already passed to ehci_hcd_init(), so we have to live with it
> > > if we don't want to change the newly introduced multi-controller
> > > infrastructure.
> > 
> > Let's change it .... remove the ID and pass some generic pdata.
> 
> I don't like the idea of passing around data at this level. It's
> breaking the abstraction, as we have to pass low-level usb information
> around in the higher USB stack levels.

Good, what do you suggest we do when we apply driver model onto this stuff?

> The USB driver code should be able to do the virt-to-phys controller
> mapping on it's own. In the Tegra world

Tegra is completely unimportant part of the usb ecosystem.

> we use the information we get
> from device tree to do so, but I don't see a reason why your USB host
> driver code wouldn't be able to just require an array with configuration
> data from the board file.

I don't see how you transfer DT information into controller # ...

> There is really no need to pass this information through all the USB
> stack interfaces.

Please explain.

> Regards,
> Lucas

[U-Boot] usb: ehci: Take advantage of the new multi-controller feature for MXC

[Lucas Stach]

Dear Marek Vasut,

Am Dienstag, den 06.11.2012, 23:35 +0100 schrieb Marek Vasut:
> Dear Lucas Stach,
> 
> [...]
> 
> > > > > > What do you think?
> > > > > 
> > > > > What about passing port private / platform data instead of ID ?
> > > > 
> > > > The ID is already passed to ehci_hcd_init(), so we have to live with it
> > > > if we don't want to change the newly introduced multi-controller
> > > > infrastructure.
> > > 
> > > Let's change it .... remove the ID and pass some generic pdata.
> > 
> > I don't like the idea of passing around data at this level. It's
> > breaking the abstraction, as we have to pass low-level usb information
> > around in the higher USB stack levels.
> 
> Good, what do you suggest we do when we apply driver model onto this stuff?
> 
Sadly I have not found the time to take a deeper look into the driver
model. But see below.

> > The USB driver code should be able to do the virt-to-phys controller
> > mapping on it's own. In the Tegra world
> 
> Tegra is completely unimportant part of the usb ecosystem.
> 
I know that your views are centred around a different point, which is
fine with me, but please don't make the mistake to downplay the
importance of _any_ part of the ecosystem.

> > we use the information we get
> > from device tree to do so, but I don't see a reason why your USB host
> > driver code wouldn't be able to just require an array with configuration
> > data from the board file.
> 
> I don't see how you transfer DT information into controller # ...
> 
> > There is really no need to pass this information through all the USB
> > stack interfaces.
> 
> Please explain.

Tegra has a two step initialisation:

1. Init the driver at board_init time
This is the step where we parse all the DT information and     fill in
all needed driver internal structures. At this point we do the virt to
phys controller ID mapping.

2. For every controller that U-Boot really uses we activate host mode
and do the real hardware initialisation at ehci_hcd_init time.

If I'm not completely mistaken such a model should align nicely with the
upcoming driver model. The driver gets instantiated with information it
gathers from global platform data, may it be device tree or any other
form of driver related information.

In this case the ehci_hcd_init|stop entry points are only used to
init/stop one specific controller, which is completely different matter
from the driver being instantiated and as such should not carry any
platform data. IMHO all platform data should be contained in the boards
global data.

Regards,
Lucas

[U-Boot] usb: ehci: Take advantage of the new multi-controller feature for MXC

[Marek Vasut]

Dear Lucas Stach,

> Dear Marek Vasut,
> 
> Am Dienstag, den 06.11.2012, 23:35 +0100 schrieb Marek Vasut:
> > Dear Lucas Stach,
> > 
> > [...]
> > 
> > > > > > > What do you think?
> > > > > > 
> > > > > > What about passing port private / platform data instead of ID ?
> > > > > 
> > > > > The ID is already passed to ehci_hcd_init(), so we have to live
> > > > > with it if we don't want to change the newly introduced
> > > > > multi-controller infrastructure.
> > > > 
> > > > Let's change it .... remove the ID and pass some generic pdata.
> > > 
> > > I don't like the idea of passing around data at this level. It's
> > > breaking the abstraction, as we have to pass low-level usb information
> > > around in the higher USB stack levels.
> > 
> > Good, what do you suggest we do when we apply driver model onto this
> > stuff?
> 
> Sadly I have not found the time to take a deeper look into the driver
> model. But see below.

You might want to ... I suspect instead of passing ID, we should start passing 
some USB pdata. EHCI Pdata for ehci I guess ...

> > > The USB driver code should be able to do the virt-to-phys controller
> > > mapping on it's own. In the Tegra world
> > 
> > Tegra is completely unimportant part of the usb ecosystem.
> 
> I know that your views are centred around a different point, which is
> fine with me, but please don't make the mistake to downplay the
> importance of _any_ part of the ecosystem.

On the contrary, I'm trying to avoid making the mistake of focusing on any SoC 
too much.

> > > we use the information we get
> > > from device tree to do so, but I don't see a reason why your USB host
> > > driver code wouldn't be able to just require an array with
> > > configuration data from the board file.
> > 
> > I don't see how you transfer DT information into controller # ...
> > 
> > > There is really no need to pass this information through all the USB
> > > stack interfaces.
> > 
> > Please explain.
> 
> Tegra has a two step initialisation:
> 
> 1. Init the driver at board_init time
> This is the step where we parse all the DT information and     fill in
> all needed driver internal structures. At this point we do the virt to
> phys controller ID mapping.

Hm ... thinking about it, maybe you can do generic USB Pdata which would contain 
the controller # and additional pdata (like mmap address etc).

> 2. For every controller that U-Boot really uses we activate host mode
> and do the real hardware initialisation at ehci_hcd_init time.

Good.

> If I'm not completely mistaken such a model should align nicely with the
> upcoming driver model. The driver gets instantiated with information it
> gathers from global platform data, may it be device tree or any other
> form of driver related information.

Yes, but you don't pass such data through the driver (yet). You need to do that 
and that's what I asked you to do.

> In this case the ehci_hcd_init|stop entry points are only used to
> init/stop one specific controller, which is completely different matter
> from the driver being instantiated and as such should not carry any
> platform data. IMHO all platform data should be contained in the boards
> global data.

I believe you should be passing pdata to the ehci_hcd_init ... they might 
contain some register frobbing etc., but this is probably the part where we 
missed each ones point.

> Regards,
> Lucas

Best regards,
Marek Vasut

[U-Boot] usb: ehci: Take advantage of the new multi-controller feature for MXC

[Lucas Stach]

Dear Marek Vasut,

Am Mittwoch, den 07.11.2012, 14:25 +0100 schrieb Marek Vasut:
> Dear Lucas Stach,
> 
> > Dear Marek Vasut,
> > 
> > Am Dienstag, den 06.11.2012, 23:35 +0100 schrieb Marek Vasut:
> > > I don't see how you transfer DT information into controller # ...
> > > 
> > > > There is really no need to pass this information through all the USB
> > > > stack interfaces.
> > > 
> > > Please explain.
> > 
> > Tegra has a two step initialisation:
> > 
> > 1. Init the driver at board_init time
> > This is the step where we parse all the DT information and     fill in
> > all needed driver internal structures. At this point we do the virt to
> > phys controller ID mapping.
> 
> Hm ... thinking about it, maybe you can do generic USB Pdata which would contain 
> the controller # and additional pdata (like mmap address etc).
> 
> > 2. For every controller that U-Boot really uses we activate host mode
> > and do the real hardware initialisation at ehci_hcd_init time.
> 
> Good.
> 
> > If I'm not completely mistaken such a model should align nicely with the
> > upcoming driver model. The driver gets instantiated with information it
> > gathers from global platform data, may it be device tree or any other
> > form of driver related information.
> 
> Yes, but you don't pass such data through the driver (yet). You need to do that 
> and that's what I asked you to do.
> 
We do pass this data to the driver in the form of gd->fdt_blob. This
data is driver (not controller) specific, so why would you pass this in
at ehci_hcd_init time?

But while writing this I think I now see why we miss each others point:
the Tegra EHCI driver is only instantiated once and used for all
controllers. This probably has to be reworked for the driver model.

Now I would still argue that we should keep the two step init model,
first we instantiate the driver with some form of pdata (we can
certainly come up with a one-struct-fits-all for this) and later when we
are really going to use one specific controller we do the real hardware
init.

Now we seem to differ about the meaning of the usb stack functions. From
your mails I see that you want ehci_hcd_init as the first init step
where we instantiate the driver (and therefore need the pdata), where I
treated it as the second step, because currently it is the point where
the upper USB stack levels indicate that they are going to use a
specific controller.

We should probably come up with some consensus about this before going
forward. Sadly my free time is really limited right now, so it's hard
for me to keep up even with things I planned to do in the next few
weeks, not to speak about playing around with the driver model.

Regards,
Lucas

[U-Boot] usb: ehci: Take advantage of the new multi-controller feature for MXC

[Marek Vasut]

Dear Lucas Stach,

> Dear Marek Vasut,
> 
> Am Mittwoch, den 07.11.2012, 14:25 +0100 schrieb Marek Vasut:
> > Dear Lucas Stach,
> > 
> > > Dear Marek Vasut,
> > > 
> > > Am Dienstag, den 06.11.2012, 23:35 +0100 schrieb Marek Vasut:
> > > > I don't see how you transfer DT information into controller # ...
> > > > 
> > > > > There is really no need to pass this information through all the
> > > > > USB stack interfaces.
> > > > 
> > > > Please explain.
> > > 
> > > Tegra has a two step initialisation:
> > > 
> > > 1. Init the driver at board_init time
> > > This is the step where we parse all the DT information and     fill in
> > > all needed driver internal structures. At this point we do the virt to
> > > phys controller ID mapping.
> > 
> > Hm ... thinking about it, maybe you can do generic USB Pdata which would
> > contain the controller # and additional pdata (like mmap address etc).
> > 
> > > 2. For every controller that U-Boot really uses we activate host mode
> > > and do the real hardware initialisation at ehci_hcd_init time.
> > 
> > Good.
> > 
> > > If I'm not completely mistaken such a model should align nicely with
> > > the upcoming driver model. The driver gets instantiated with
> > > information it gathers from global platform data, may it be device
> > > tree or any other form of driver related information.
> > 
> > Yes, but you don't pass such data through the driver (yet). You need to
> > do that and that's what I asked you to do.
> 
> We do pass this data to the driver in the form of gd->fdt_blob. This
> data is driver (not controller) specific, so why would you pass this in
> at ehci_hcd_init time?

Sorry, I don't understand you.

> But while writing this I think I now see why we miss each others point:
> the Tegra EHCI driver is only instantiated once and used for all
> controllers. This probably has to be reworked for the driver model.

What do you mean "instantiated once"? There ALWAYS has to be only a single 
instance per one controller.

> Now I would still argue that we should keep the two step init model,
> first we instantiate the driver with some form of pdata (we can
> certainly come up with a one-struct-fits-all for this) and later when we
> are really going to use one specific controller we do the real hardware
> init.
> 
> Now we seem to differ about the meaning of the usb stack functions. From
> your mails I see that you want ehci_hcd_init as the first init step
> where we instantiate the driver (and therefore need the pdata)

No, I don't care what you do in your ehci_hcd_init. And I don't care if you 
instantiate it there. But I suspect I understand your problem. I suspect the 
driver shall be instantiated from elsewhere and ehci_hcd_init() call shall only 
be used to fine-tune (or work around) controller bugs.

> where I
> treated it as the second step, because currently it is the point where
> the upper USB stack levels indicate that they are going to use a
> specific controller.
> 
> We should probably come up with some consensus about this before going
> forward. Sadly my free time is really limited right now, so it's hard
> for me to keep up even with things I planned to do in the next few
> weeks, not to speak about playing around with the driver model.

You're actually free to not work on that. Concensus is, I think the multi-
controller thing is misdesigned and we rather fix it sooner than later.

See my comment above about how I'd like to see it.

> Regards,
> Lucas

Best regards,
Marek Vasut

[U-Boot] usb: ehci: Take advantage of the new multi-controller feature for MXC

[Benoît Thébaudeau]

Dear Marek Vasut,

On Wednesday, November 7, 2012 3:13:51 PM, Marek Vasut wrote:
> Dear Lucas Stach,
> 
> > Dear Marek Vasut,
> > 
> > Am Mittwoch, den 07.11.2012, 14:25 +0100 schrieb Marek Vasut:
> > Now I would still argue that we should keep the two step init
> > model,
> > first we instantiate the driver with some form of pdata (we can
> > certainly come up with a one-struct-fits-all for this) and later
> > when we
> > are really going to use one specific controller we do the real
> > hardware
> > init.
> > 
> > Now we seem to differ about the meaning of the usb stack functions.
> > From
> > your mails I see that you want ehci_hcd_init as the first init step
> > where we instantiate the driver (and therefore need the pdata)
> 
> No, I don't care what you do in your ehci_hcd_init. And I don't care
> if you
> instantiate it there. But I suspect I understand your problem. I
> suspect the
> driver shall be instantiated from elsewhere and ehci_hcd_init() call
> shall only
> be used to fine-tune (or work around) controller bugs.

Not only for controller bugs, but also for related board operations through
board_ehci_hcd_init(), or simply to perform a clean new init following a stop
(e.g. in the case of the "usb reset" command).

> > where I
> > treated it as the second step, because currently it is the point
> > where
> > the upper USB stack levels indicate that they are going to use a
> > specific controller.
> > 
> > We should probably come up with some consensus about this before
> > going
> > forward. Sadly my free time is really limited right now, so it's
> > hard
> > for me to keep up even with things I planned to do in the next few
> > weeks, not to speak about playing around with the driver model.
> 
> You're actually free to not work on that. Concensus is, I think the
> multi-
> controller thing is misdesigned and we rather fix it sooner than
> later.
> 
> See my comment above about how I'd like to see it.

If I understand correctly what you said, ehci_hcd_init() can be left unchanged
because you don't care about what it does, so it will keep using the USB
controller index from the command line. And we should add some
"int ehci_hcd_bind(void *pdata)" that would be called by the board init files to
perform the driver instantiation.

Until the driver model is applied, this instantiation would be an empty
operation except for the drivers like ehci-mxc.c that need some platform data.
Hence, for now, this ehci_hcd_bind() function would have to be implemented only
for such drivers, which would be a small change that can be done step by step.

Correct me if I'm wrong above. My goal here is only to find a quick and simple
solution to take advantage of the multi-controller feature on MXC. I don't have
enough time to rework the whole infrastructure, so if this goal is incompatible
with the current infrastructure and how you want to make it evolve, I'll come
back when the infrastructure allows to truly use this feature.

Best regards,
Beno?t

[U-Boot] usb: ehci: Take advantage of the new multi-controller feature for MXC

[Marek Vasut]

Dear Beno?t Th?baudeau,

> Dear Marek Vasut,
> 
> On Wednesday, November 7, 2012 3:13:51 PM, Marek Vasut wrote:
> > Dear Lucas Stach,
> > 
> > > Dear Marek Vasut,
> > > 
> > > Am Mittwoch, den 07.11.2012, 14:25 +0100 schrieb Marek Vasut:
> > > Now I would still argue that we should keep the two step init
> > > model,
> > > first we instantiate the driver with some form of pdata (we can
> > > certainly come up with a one-struct-fits-all for this) and later
> > > when we
> > > are really going to use one specific controller we do the real
> > > hardware
> > > init.
> > > 
> > > Now we seem to differ about the meaning of the usb stack functions.
> > > From
> > > your mails I see that you want ehci_hcd_init as the first init step
> > > where we instantiate the driver (and therefore need the pdata)
> > 
> > No, I don't care what you do in your ehci_hcd_init. And I don't care
> > if you
> > instantiate it there. But I suspect I understand your problem. I
> > suspect the
> > driver shall be instantiated from elsewhere and ehci_hcd_init() call
> > shall only
> > be used to fine-tune (or work around) controller bugs.
> 
> Not only for controller bugs, but also for related board operations through
> board_ehci_hcd_init(), or simply to perform a clean new init following a
> stop (e.g. in the case of the "usb reset" command).
> 
> > > where I
> > > treated it as the second step, because currently it is the point
> > > where
> > > the upper USB stack levels indicate that they are going to use a
> > > specific controller.
> > > 
> > > We should probably come up with some consensus about this before
> > > going
> > > forward. Sadly my free time is really limited right now, so it's
> > > hard
> > > for me to keep up even with things I planned to do in the next few
> > > weeks, not to speak about playing around with the driver model.
> > 
> > You're actually free to not work on that. Concensus is, I think the
> > multi-
> > controller thing is misdesigned and we rather fix it sooner than
> > later.
> > 
> > See my comment above about how I'd like to see it.
> 
> If I understand correctly what you said, ehci_hcd_init() can be left
> unchanged because you don't care about what it does, so it will keep using
> the USB controller index from the command line. And we should add some
> "int ehci_hcd_bind(void *pdata)" that would be called by the board init
> files to perform the driver instantiation.
> 
> Until the driver model is applied, this instantiation would be an empty
> operation except for the drivers like ehci-mxc.c that need some platform
> data. Hence, for now, this ehci_hcd_bind() function would have to be
> implemented only for such drivers, which would be a small change that can
> be done step by step.
> 
> Correct me if I'm wrong above. My goal here is only to find a quick and
> simple solution to take advantage of the multi-controller feature on MXC.
> I don't have enough time to rework the whole infrastructure, so if this
> goal is incompatible with the current infrastructure and how you want to
> make it evolve, I'll come back when the infrastructure allows to truly use
> this feature.

Yes, let's try ehci_hcd_bind().

Best regards,
Marek Vasut