Hi Lucas, On Tue, Oct 30, 2012 at 6:54 AM, Lucas Stach <d...@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 <d...@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 <d...@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 <d...@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 > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot