Hi Lucas,
On Tue, Oct 30, 2012 at 6:54 AM, Lucas Stach dev@lynxeye.de wrote:
Am Dienstag, den 30.10.2012, 06:48 -0700 schrieb Simon Glass:
Hi Lucas,
On Tue, Oct 30, 2012 at 6:37 AM, Lucas Stach dev@lynxeye.de wrote:
Am Dienstag, den 30.10.2012, 06:27 -0700 schrieb Simon Glass:
Hi Lucas,
On Tue, Oct 30, 2012 at 2:22 AM, Lucas Stach dev@lynxeye.de wrote:
There is no need to init a USB controller before the upper layers indicate that they are actually going to use it.
board_usb_init now only parses the device tree and sets up the common pll.
Signed-off-by: Lucas Stach dev@lynxeye.de
arch/arm/cpu/armv7/tegra20/usb.c | 47 +++++++++++++++------------------------- 1 Datei geändert, 18 Zeilen hinzugefügt(+), 29 Zeilen entfernt(-)
diff --git a/arch/arm/cpu/armv7/tegra20/usb.c b/arch/arm/cpu/armv7/tegra20/usb.c index cf800b1..e372b8b 100644 --- a/arch/arm/cpu/armv7/tegra20/usb.c +++ b/arch/arm/cpu/armv7/tegra20/usb.c @@ -417,44 +417,29 @@ static int init_ulpi_usb_controller(struct fdt_usb *config) } #endif
-/**
- Add a new USB port to the list of available ports.
- @param config USB port configuration
- @return 0 if ok, -1 if error (too many ports)
- */
-static int add_port(struct fdt_usb *config) +int tegrausb_start_port(int portnum, u32 *hccr, u32 *hcor) {
if (port_count == USB_PORTS_MAX) {printf("tegrausb: Cannot register more than %d ports\n",USB_PORTS_MAX);
struct fdt_usb *config;struct usb_ctlr *usbctlr;if (portnum >= port_count) return -1;
}
config = &port[portnum]; if (config->utmi && init_utmi_usb_controller(config)) {
printf("tegrausb: Cannot init port\n");
printf("tegrausb: Cannot init port %d\n", portnum); return -1; } if (config->ulpi && init_ulpi_usb_controller(config)) {
printf("tegrausb: Cannot init port\n");
printf("tegrausb: Cannot init port %d\n", portnum); return -1; }
port[port_count++] = *config;return 0;-}
set_host_mode(config);This is good, but now I think you will re-init the USB peripheral at every 'usb start'. Perhaps you should remember whether it has been inited and only do it the first time?
I have to look this up, but the upper USB layers should not call those lowlevel init functions repeatedly unless explicitly asked for it through a "usb reset" or the like. If it actually does so it's a bug in the upper layer and should not be fixed up in the lowlevel functions.
Perhaps, but you have to write your code in the environment that exists. At present usb_lowlevel_init() is called on every 'usb start' (and ehci_hcd_init() from that).
After all this is open source and I would rather spin a patch to fix this at the right spot if we do the wrong thing, than having to cope with the bug at a lower level. Even with bug present we are not failing in any severe way, we are just wasting time bringing up a controller which is already up.
OK, but perhaps my broader point is that this may in fact be the intended behaviour of U-Boot. Until you bring this up and submit a patch to change it, you may not know. I would suggest you change the patch order here - first send a patch making usb_lowlevel_init() work the way you want, then a patch to change Tegra to init each time usb_lowlevel_init() is called.
I'm not sure about the time penalty - I suspect it is small - but why have any such penalty? Boot time is a key concern (at least for me!). Plus re-init of already-inited hardware can be problematic.
Or you could fix this for now by remembering what is inited and not doing it again - just another flag in struct fdt_usb. It is good that you don't init USB until needed, and that would solve the problem in the interim, until you get usb_lowlevel_init() changed. Ultimately with the device model we may be able to go further and not even do the fdt decode until needed.
Regards, Simon
Regards, Lucas