[U-Boot] [PATCH 5/8] tegra: usb: move controller init into start_port

[Simon Glass]

Hi Lucas,

On Tue, Oct 30, 2012 at 6:54 AM, Lucas Stach <dev at 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 at 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 at 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 at 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
>