diff mbox

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

Message ID 1351588973-20699-5-git-send-email-dev@lynxeye.de
State Superseded
Delegated to: Marek Vasut
Headers show

Commit Message

Lucas Stach Oct. 30, 2012, 9:22 a.m. UTC
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(-)

Comments

Marek Vasut Oct. 30, 2012, 10:59 a.m. UTC | #1
Dear Lucas Stach,

> 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)

Fix the comment instead of removing it?

[...]

Best regards,
Marek Vasut
Lucas Stach Oct. 30, 2012, 12:12 p.m. UTC | #2
Hello Marek,

Am Dienstag, den 30.10.2012, 11:59 +0100 schrieb Marek Vasut:
> Dear Lucas Stach,
> 
> > 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)
> 
> Fix the comment instead of removing it?
> 
I don't think that this comment adds any real value. The whole function
which this comment refers to is removed and it's content split between
board_usb_init and ehci_hcd_init, which are self explanatory.

Regards,
Lucas
Marek Vasut Oct. 30, 2012, 12:33 p.m. UTC | #3
Dear Lucas Stach,

[...]

> > > -static int add_port(struct fdt_usb *config)
> > 
> > Fix the comment instead of removing it?
> 
> I don't think that this comment adds any real value. The whole function
> which this comment refers to is removed and it's content split between
> board_usb_init and ehci_hcd_init, which are self explanatory.

Then add a proper comment please. Call me a docu-nazi, but I'd really love u-
boot nicely and properly documented, please.

[1] http://www.denx.de/wiki/U-Boot/CodingStyle

Best regards,
Marek Vasut
Lucas Stach Oct. 30, 2012, 12:44 p.m. UTC | #4
Hi Marek,

Am Dienstag, den 30.10.2012, 13:33 +0100 schrieb Marek Vasut:
> Dear Lucas Stach,
> 
> [...]
> 
> > > > -static int add_port(struct fdt_usb *config)
> > > 
> > > Fix the comment instead of removing it?
> > 
> > I don't think that this comment adds any real value. The whole function
> > which this comment refers to is removed and it's content split between
> > board_usb_init and ehci_hcd_init, which are self explanatory.
> 
> Then add a proper comment please. Call me a docu-nazi, but I'd really love u-
> boot nicely and properly documented, please.
> 
I'm all in favour of adding proper documentation, but I'm opposed to add
it in the middle of this cleanup/movement series.

I'll send a patch on top of this series to add doc, so it doesn't
interfere with the review of this series.

Regards,
Lucas
Simon Glass Oct. 30, 2012, 1:27 p.m. UTC | #5
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?

Regards,
Simon

>
> -int tegrausb_start_port(int portnum, u32 *hccr, u32 *hcor)
> -{
> -       struct usb_ctlr *usbctlr;
> -
> -       if (portnum >= port_count)
> -               return -1;
> -       set_host_mode(&port[portnum]);
> -
> -       usbctlr = port[portnum].reg;
> +       usbctlr = config->reg;
>         *hccr = (u32)&usbctlr->cap_length;
>         *hcor = (u32)&usbctlr->usb_cmd;
>         return 0;
> @@ -539,6 +524,12 @@ int board_usb_init(const void *blob)
>         count = fdtdec_find_aliases_for_id(blob, "usb",
>                         COMPAT_NVIDIA_TEGRA20_USB, node_list, USB_PORTS_MAX);
>         for (i = 0; i < count; i++) {
> +               if (port_count == USB_PORTS_MAX) {
> +                       printf("tegrausb: Cannot register more than %d ports\n",
> +                               USB_PORTS_MAX);
> +                       return -1;
> +               }
> +
>                 debug("USB %d: ", i);
>                 node = node_list[i];
>                 if (!node)
> @@ -549,9 +540,7 @@ int board_usb_init(const void *blob)
>                         return -1;
>                 }
>
> -               if (add_port(&config))
> -                       return -1;
> -               set_host_mode(&config);
> +               port[port_count++] = config;
>         }
>
>         return 0;
> --
> 1.7.11.7
>
Lucas Stach Oct. 30, 2012, 1:37 p.m. UTC | #6
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.

Regards,
Lucas
Simon Glass Oct. 30, 2012, 1:48 p.m. UTC | #7
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).

Regards,
Simon

>
> Regards,
> Lucas
>
>
Lucas Stach Oct. 30, 2012, 1:54 p.m. UTC | #8
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.

Regards,
Lucas
Simon Glass Oct. 30, 2012, 2:03 p.m. UTC | #9
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
>
Stephen Warren Oct. 30, 2012, 6:34 p.m. UTC | #10
On 10/30/2012 06:44 AM, Lucas Stach wrote:
> Hi Marek,
> 
> Am Dienstag, den 30.10.2012, 13:33 +0100 schrieb Marek Vasut:
>> Dear Lucas Stach,
>>
>> [...]
>>
>>>>> -static int add_port(struct fdt_usb *config)
>>>>
>>>> Fix the comment instead of removing it?
>>>
>>> I don't think that this comment adds any real value. The whole function
>>> which this comment refers to is removed and it's content split between
>>> board_usb_init and ehci_hcd_init, which are self explanatory.
>>
>> Then add a proper comment please. Call me a docu-nazi, but I'd really love u-
>> boot nicely and properly documented, please.
>>
> I'm all in favour of adding proper documentation, but I'm opposed to add
> it in the middle of this cleanup/movement series.
> 
> I'll send a patch on top of this series to add doc, so it doesn't
> interfere with the review of this series.

If the docs already exist and the function is simply changing its exact
semantics, the docs should remain and simply be updated.
diff mbox

Patch

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);
 
-int tegrausb_start_port(int portnum, u32 *hccr, u32 *hcor)
-{
-	struct usb_ctlr *usbctlr;
-
-	if (portnum >= port_count)
-		return -1;
-	set_host_mode(&port[portnum]);
-
-	usbctlr = port[portnum].reg;
+	usbctlr = config->reg;
 	*hccr = (u32)&usbctlr->cap_length;
 	*hcor = (u32)&usbctlr->usb_cmd;
 	return 0;
@@ -539,6 +524,12 @@  int board_usb_init(const void *blob)
 	count = fdtdec_find_aliases_for_id(blob, "usb",
 			COMPAT_NVIDIA_TEGRA20_USB, node_list, USB_PORTS_MAX);
 	for (i = 0; i < count; i++) {
+		if (port_count == USB_PORTS_MAX) {
+			printf("tegrausb: Cannot register more than %d ports\n",
+				USB_PORTS_MAX);
+			return -1;
+		}
+
 		debug("USB %d: ", i);
 		node = node_list[i];
 		if (!node)
@@ -549,9 +540,7 @@  int board_usb_init(const void *blob)
 			return -1;
 		}
 
-		if (add_port(&config))
-			return -1;
-		set_host_mode(&config);
+		port[port_count++] = config;
 	}
 
 	return 0;