From mboxrd@z Thu Jan  1 00:00:00 1970
From: =?utf-8?Q?Beno=C3=AEt_Th=C3=A9baudeau?= <benoit.thebaudeau@advansee.com>
Date: Tue, 6 Nov 2012 20:59:31 +0100 (CET)
Subject: [U-Boot] usb: ehci: Take advantage of the new multi-controller
 feature for MXC
In-Reply-To: <1352187823.1483.6.camel@tellur>
Message-ID: <1117518064.667572.1352231971732.JavaMail.root@advansee.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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