Patchwork [U-Boot,v6,2/4] usb: ehci: add weak-aliased functions to portsc & tdi

login
register
mail settings
Submitter Kuo-Jung Su
Date May 15, 2013, 2:42 a.m.
Message ID <CAK65tU5JpGNgpYDyDFh3RZE9ABd=ZfyLgPJS44yf-gXHnhXz4g@mail.gmail.com>
Download mbox | patch
Permalink /patch/243869/
State Awaiting Upstream
Delegated to: Marek Vasut
Headers show

Comments

Kuo-Jung Su - May 15, 2013, 2:42 a.m.
2013/5/15 Kuo-Jung Su <dantesu@gmail.com>:
> 2013/5/14 Marek Vasut <marex@denx.de>:
>> Dear Kuo-Jung Su,
>>
>>> 2013/5/13 Marek Vasut <marex@denx.de>:
>>> > Dear Kuo-Jung Su,
>>> >
>>> >> 2013/5/13 Marek Vasut <marex@denx.de>:
>>> >> > Dear Kuo-Jung Su,
>>> >> >
>>> >> >> From: Kuo-Jung Su <dantesu@faraday-tech.com>
>>> >> >>
>>> >> >> There is at least one non-EHCI compliant controller (i.e. Faraday
>>> >> >> EHCI) known to implement a non-standard TDI stuff.
>>> >> >> Futhermore, it not only leave reserved and CONFIGFLAG registers
>>> >> >> un-implemented but also has their address spaces removed.
>>> >> >>
>>> >> >> And thus, we need weak-aliased functions to both TDI stuff
>>> >> >> and PORTSC registers for interface abstraction.
>>> >> >>
>>> >> >> Signed-off-by: Kuo-Jung Su <dantesu@faraday-tech.com>
>>> >> >> CC: Marek Vasut <marex@denx.de>
>>> >> >> ---
>>> >> >>
>>> >> >> Changes for v6:
>>> >> >>    - Simplify weak aliased function declaration
>>> >> >>    - Drop redundant line feed
>>> >> >>
>>> >> >> Changes for v5:
>>> >> >>    - Split up from Faraday EHCI patch
>>> >> >>
>>> >> >> Changes for v2 - v4:
>>> >> >>    - See 'usb: ehci: add Faraday USB 2.0 EHCI support'
>>> >> >>
>>> >> >>  drivers/usb/host/ehci-hcd.c |   91
>>> >> >>
>>> >> >> ++++++++++++++++++++++++++----------------- 1 file changed, 55
>>> >> >> insertions(+), 36 deletions(-)
>>> >> >>
>>> >> >> diff --git a/drivers/usb/host/ehci-hcd.c
>>> >> >> b/drivers/usb/host/ehci-hcd.c index c816878..ae3f2a4 100644
>>> >> >> --- a/drivers/usb/host/ehci-hcd.c
>>> >> >> +++ b/drivers/usb/host/ehci-hcd.c
>>> >> >> @@ -117,10 +117,44 @@ static struct descriptor {
>>> >> >>
>>> >> >>  };
>>> >> >>
>>> >> >>  #if defined(CONFIG_EHCI_IS_TDI)
>>> >> >>
>>> >> >> -#define ehci_is_TDI()        (1)
>>> >> >> -#else
>>> >> >> -#define ehci_is_TDI()        (0)
>>> >> >> +# define ehci_is_TDI()       (1)
>>> >> >
>>> >> > btw you can remove those braces around (1) and (0) below. But I have
>>> >> > one more question ...
>>> >>
>>> >> Got it, thanks
>>> >>
>>> >> > [...]
>>> >> >
>>> >> >> @@ -609,13 +644,10 @@ ehci_submit_root(struct usb_device *dev,
>>> >> >> unsigned long pipe, void *buffer, uint32_t *status_reg;
>>> >> >>
>>> >> >>       struct ehci_ctrl *ctrl = dev->controller;
>>> >> >>
>>> >> >> -     if (le16_to_cpu(req->index) >
>>> >> >> CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS) { -             printf("The
>>> >> >> request port(%d) is not configured\n", -
>>> >> >>
>>> >> >>                  le16_to_cpu(req->index) - 1);
>>> >> >>
>>> >> >> +     status_reg = ehci_get_portsc_register(ctrl->hcor,
>>> >> >> +             le16_to_cpu(req->index) - 1);
>>> >> >> +     if (!status_reg)
>>> >> >
>>> >> > What happens here if req->index is zero ?
>>> >> >
>>> >> > Hint: the above code always does unsigned comparison ...
>>> >> >
>>> >> > I think you should make the second argument of
>>> >> > ehci_get_portsc_register() unsigned short too (as is req->index in
>>> >> > struct devrequest).
>>> >>
>>> >> Sorry, but I'll prefer 'int' over 'unsigned short', since it looks to me
>>> >> that the u-boot would set 'req->index' to 0 at startup, which results in
>>> >> a 'port = -1' to be passed to ehci_get_portsc_register().
>>> >>
>>> >> And I think '-1' is a better self-explain value, so I'd like to stick
>>> >> with 'int'
>>> >
>>> > Sure, but then the comparison is signed, not unsigned. Besides, it's
>>> > unnecessary change to the logic of the code. Or did I miss something ?
>>>
>>> 1. There is a bug in ehci_submit_root() of usb ehci:
>>>
>>>     int ehci_submit_root()
>>>     {
>>>          ......
>>>          if (port > CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS) {
>>>             printf("The request port(%d) is not configured\n", port - 1);
>>>             return -1;
>>>          }
>>>          status_reg = (uint32_t *)&ctrl->hcor->or_portsc[port - 1];
>>>          ......
>>>     }
>>>
>>>     The 'port' is actually a '0' at start-up, so we actually accessed
>>> a wrong register.
>>>     But fortunately the wrong register actually points to CONFIGFLAG(0x40)
>>> with a safe value for the following codes.
>>>
>>> 2. One of Vivek Gautam's usb patches has altered the logic of usb host
>>>     upon launching 'usb start', if we report a error upon (port - 1 < 0),
>>>     the current u-boot usb would failed to scan ports. (At least it
>>> failed at Faraday platforms.)
>>>     However it looks to me that it's o.k to report a error upon (port
>>> - 1 < 0) at old usb ehci stack.
>>>     (i.e. 10 days ago, in master branch of u-boot)
>>>
>>> And thus I add a quick check to PATCH v7.
>>>
>>> __weak uint32_t *ehci_get_portsc_register(struct ehci_hcor *hcor, int port)
>>> {
>>>  /*
>>>   * The u-boot would somehow set port=-1 at usb start-up,
>>>   * so this quick fix is necessary.
>>>   */
>>>  if (port < 0)
>>>   port = 0;
>>
>> Maybe we should return fail, no ?
>
> No, it would make the 'usb start' to terminate immediately,
> and results in a port scan failure to at least Faraday EHCI.
>
>> Can you pinpoint where does the req->index
>> (resp. port) get set to -1 ?
>
> Later I'll try to find out where we have 'req->index' set as a '0' in
> 'usb start'.
>

It's from usb_new_device() --> usb_get_descriptor(), and thus
it's definitely correct to set index = 0 right here.
The only problem is 'We shall not always report an portsc error for
all request!'
Please refer to the patch attached at the tail of this mail.

>> And which commit introduced this breakage ?
>
> I believe it's there long ago, we just fortunately bypass the error at old day,
> and now one of Vivek Gautam's USB patch make us face up to this issue.
>

Shame on me....
It's nothing to do with Vivek Gautam's USB patch or the old USB ehci stack,
it's only mattered while reporting an portsc error. :P

>>
>> Best regards,
>> Marek Vasut
>
>

Here is the quick fix to this issue:


  debug("req=%u (%#x), type=%u (%#x), value=%u, index=%u\n",
@@ -717,6 +709,8 @@ ehci_submit_root(struct usb_device *dev, unsigned
long pipe, void *buffer,
  srclen = 2;
  break;
  case USB_REQ_GET_STATUS | ((USB_RT_PORT | USB_DIR_IN) << 8):
+ if (!status_reg)
+ return -1;
  memset(tmpbuf, 0, 4);
  reg = ehci_readl(status_reg);
  if (reg & EHCI_PS_CS)
@@ -761,6 +755,8 @@ ehci_submit_root(struct usb_device *dev, unsigned
long pipe, void *buffer,
  srclen = 4;
  break;
  case USB_REQ_SET_FEATURE | ((USB_DIR_OUT | USB_RT_PORT) << 8):
+ if (!status_reg)
+ return -1;
  reg = ehci_readl(status_reg);
  reg &= ~EHCI_PS_CLEAR;
  switch (le16_to_cpu(req->value)) {
@@ -825,6 +821,8 @@ ehci_submit_root(struct usb_device *dev, unsigned
long pipe, void *buffer,
  (void) ehci_readl(&ctrl->hcor->or_usbcmd);
  break;
  case USB_REQ_CLEAR_FEATURE | ((USB_DIR_OUT | USB_RT_PORT) << 8):
+ if (!status_reg)
+ return -1;
  reg = ehci_readl(status_reg);
  switch (le16_to_cpu(req->value)) {
  case USB_PORT_FEAT_ENABLE:
Marek Vasut - May 15, 2013, 3:29 a.m.
Dear Kuo-Jung Su,

> 2013/5/15 Kuo-Jung Su <dantesu@gmail.com>:
> > 2013/5/14 Marek Vasut <marex@denx.de>:
> >> Dear Kuo-Jung Su,
> >> 
> >>> 2013/5/13 Marek Vasut <marex@denx.de>:
> >>> > Dear Kuo-Jung Su,
> >>> > 
> >>> >> 2013/5/13 Marek Vasut <marex@denx.de>:
> >>> >> > Dear Kuo-Jung Su,
> >>> >> > 
> >>> >> >> From: Kuo-Jung Su <dantesu@faraday-tech.com>
> >>> >> >> 
> >>> >> >> There is at least one non-EHCI compliant controller (i.e. Faraday
> >>> >> >> EHCI) known to implement a non-standard TDI stuff.
> >>> >> >> Futhermore, it not only leave reserved and CONFIGFLAG registers
> >>> >> >> un-implemented but also has their address spaces removed.
> >>> >> >> 
> >>> >> >> And thus, we need weak-aliased functions to both TDI stuff
> >>> >> >> and PORTSC registers for interface abstraction.
> >>> >> >> 
> >>> >> >> Signed-off-by: Kuo-Jung Su <dantesu@faraday-tech.com>
> >>> >> >> CC: Marek Vasut <marex@denx.de>
> >>> >> >> ---
> >>> >> >> 
> >>> >> >> Changes for v6:
> >>> >> >>    - Simplify weak aliased function declaration
> >>> >> >>    - Drop redundant line feed
> >>> >> >> 
> >>> >> >> Changes for v5:
> >>> >> >>    - Split up from Faraday EHCI patch
> >>> >> >> 
> >>> >> >> Changes for v2 - v4:
> >>> >> >>    - See 'usb: ehci: add Faraday USB 2.0 EHCI support'
> >>> >> >>  
> >>> >> >>  drivers/usb/host/ehci-hcd.c |   91
> >>> >> >> 
> >>> >> >> ++++++++++++++++++++++++++----------------- 1 file changed, 55
> >>> >> >> insertions(+), 36 deletions(-)
> >>> >> >> 
> >>> >> >> diff --git a/drivers/usb/host/ehci-hcd.c
> >>> >> >> b/drivers/usb/host/ehci-hcd.c index c816878..ae3f2a4 100644
> >>> >> >> --- a/drivers/usb/host/ehci-hcd.c
> >>> >> >> +++ b/drivers/usb/host/ehci-hcd.c
> >>> >> >> @@ -117,10 +117,44 @@ static struct descriptor {
> >>> >> >> 
> >>> >> >>  };
> >>> >> >>  
> >>> >> >>  #if defined(CONFIG_EHCI_IS_TDI)
> >>> >> >> 
> >>> >> >> -#define ehci_is_TDI()        (1)
> >>> >> >> -#else
> >>> >> >> -#define ehci_is_TDI()        (0)
> >>> >> >> +# define ehci_is_TDI()       (1)
> >>> >> > 
> >>> >> > btw you can remove those braces around (1) and (0) below. But I
> >>> >> > have one more question ...
> >>> >> 
> >>> >> Got it, thanks
> >>> >> 
> >>> >> > [...]
> >>> >> > 
> >>> >> >> @@ -609,13 +644,10 @@ ehci_submit_root(struct usb_device *dev,
> >>> >> >> unsigned long pipe, void *buffer, uint32_t *status_reg;
> >>> >> >> 
> >>> >> >>       struct ehci_ctrl *ctrl = dev->controller;
> >>> >> >> 
> >>> >> >> -     if (le16_to_cpu(req->index) >
> >>> >> >> CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS) { -             printf("The
> >>> >> >> request port(%d) is not configured\n", -
> >>> >> >> 
> >>> >> >>                  le16_to_cpu(req->index) - 1);
> >>> >> >> 
> >>> >> >> +     status_reg = ehci_get_portsc_register(ctrl->hcor,
> >>> >> >> +             le16_to_cpu(req->index) - 1);
> >>> >> >> +     if (!status_reg)
> >>> >> > 
> >>> >> > What happens here if req->index is zero ?
> >>> >> > 
> >>> >> > Hint: the above code always does unsigned comparison ...
> >>> >> > 
> >>> >> > I think you should make the second argument of
> >>> >> > ehci_get_portsc_register() unsigned short too (as is req->index in
> >>> >> > struct devrequest).
> >>> >> 
> >>> >> Sorry, but I'll prefer 'int' over 'unsigned short', since it looks
> >>> >> to me that the u-boot would set 'req->index' to 0 at startup, which
> >>> >> results in a 'port = -1' to be passed to
> >>> >> ehci_get_portsc_register().
> >>> >> 
> >>> >> And I think '-1' is a better self-explain value, so I'd like to
> >>> >> stick with 'int'
> >>> > 
> >>> > Sure, but then the comparison is signed, not unsigned. Besides, it's
> >>> > unnecessary change to the logic of the code. Or did I miss something
> >>> > ?
> >>> 
> >>> 1. There is a bug in ehci_submit_root() of usb ehci:
> >>>     int ehci_submit_root()
> >>>     {
> >>>     
> >>>          ......
> >>>          if (port > CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS) {
> >>>          
> >>>             printf("The request port(%d) is not configured\n", port -
> >>>             1); return -1;
> >>>          
> >>>          }
> >>>          status_reg = (uint32_t *)&ctrl->hcor->or_portsc[port - 1];
> >>>          ......
> >>>     
> >>>     }
> >>>     
> >>>     The 'port' is actually a '0' at start-up, so we actually accessed
> >>> 
> >>> a wrong register.
> >>> 
> >>>     But fortunately the wrong register actually points to
> >>>     CONFIGFLAG(0x40)
> >>> 
> >>> with a safe value for the following codes.
> >>> 
> >>> 2. One of Vivek Gautam's usb patches has altered the logic of usb host
> >>> 
> >>>     upon launching 'usb start', if we report a error upon (port - 1 <
> >>>     0), the current u-boot usb would failed to scan ports. (At least
> >>>     it
> >>> 
> >>> failed at Faraday platforms.)
> >>> 
> >>>     However it looks to me that it's o.k to report a error upon (port
> >>> 
> >>> - 1 < 0) at old usb ehci stack.
> >>> 
> >>>     (i.e. 10 days ago, in master branch of u-boot)
> >>> 
> >>> And thus I add a quick check to PATCH v7.
> >>> 
> >>> __weak uint32_t *ehci_get_portsc_register(struct ehci_hcor *hcor, int
> >>> port) {
> >>> 
> >>>  /*
> >>>  
> >>>   * The u-boot would somehow set port=-1 at usb start-up,
> >>>   * so this quick fix is necessary.
> >>>   */
> >>>  
> >>>  if (port < 0)
> >>>  
> >>>   port = 0;
> >> 
> >> Maybe we should return fail, no ?
> > 
> > No, it would make the 'usb start' to terminate immediately,
> > and results in a port scan failure to at least Faraday EHCI.
> > 
> >> Can you pinpoint where does the req->index
> >> (resp. port) get set to -1 ?
> > 
> > Later I'll try to find out where we have 'req->index' set as a '0' in
> > 'usb start'.
> 
> It's from usb_new_device() --> usb_get_descriptor(), and thus
> it's definitely correct to set index = 0 right here.
> The only problem is 'We shall not always report an portsc error for
> all request!'
> Please refer to the patch attached at the tail of this mail.
> 
> >> And which commit introduced this breakage ?
> > 
> > I believe it's there long ago, we just fortunately bypass the error at
> > old day, and now one of Vivek Gautam's USB patch make us face up to this
> > issue.
> 
> Shame on me....
> It's nothing to do with Vivek Gautam's USB patch or the old USB ehci stack,
> it's only mattered while reporting an portsc error. :P
> 
> >> Best regards,
> >> Marek Vasut

Ok, I think I almost see it there. Can you just make a patchset out of this 
stuff so I can digest it a little bit easier? Maybe stick the fixes first so I 
can pick them ASAP and the FHCI stuff after that.
Kuo-Jung Su - May 15, 2013, 4:07 a.m.
2013/5/15 Marek Vasut <marex@denx.de>:
> Dear Kuo-Jung Su,
>
>> 2013/5/15 Kuo-Jung Su <dantesu@gmail.com>:
>> > 2013/5/14 Marek Vasut <marex@denx.de>:
>> >> Dear Kuo-Jung Su,
>> >>
>> >>> 2013/5/13 Marek Vasut <marex@denx.de>:
>> >>> > Dear Kuo-Jung Su,
>> >>> >
>> >>> >> 2013/5/13 Marek Vasut <marex@denx.de>:
>> >>> >> > Dear Kuo-Jung Su,
>> >>> >> >
>> >>> >> >> From: Kuo-Jung Su <dantesu@faraday-tech.com>
>> >>> >> >>
>> >>> >> >> There is at least one non-EHCI compliant controller (i.e. Faraday
>> >>> >> >> EHCI) known to implement a non-standard TDI stuff.
>> >>> >> >> Futhermore, it not only leave reserved and CONFIGFLAG registers
>> >>> >> >> un-implemented but also has their address spaces removed.
>> >>> >> >>
>> >>> >> >> And thus, we need weak-aliased functions to both TDI stuff
>> >>> >> >> and PORTSC registers for interface abstraction.
>> >>> >> >>
>> >>> >> >> Signed-off-by: Kuo-Jung Su <dantesu@faraday-tech.com>
>> >>> >> >> CC: Marek Vasut <marex@denx.de>
>> >>> >> >> ---
>> >>> >> >>
>> >>> >> >> Changes for v6:
>> >>> >> >>    - Simplify weak aliased function declaration
>> >>> >> >>    - Drop redundant line feed
>> >>> >> >>
>> >>> >> >> Changes for v5:
>> >>> >> >>    - Split up from Faraday EHCI patch
>> >>> >> >>
>> >>> >> >> Changes for v2 - v4:
>> >>> >> >>    - See 'usb: ehci: add Faraday USB 2.0 EHCI support'
>> >>> >> >>
>> >>> >> >>  drivers/usb/host/ehci-hcd.c |   91
>> >>> >> >>
>> >>> >> >> ++++++++++++++++++++++++++----------------- 1 file changed, 55
>> >>> >> >> insertions(+), 36 deletions(-)
>> >>> >> >>
>> >>> >> >> diff --git a/drivers/usb/host/ehci-hcd.c
>> >>> >> >> b/drivers/usb/host/ehci-hcd.c index c816878..ae3f2a4 100644
>> >>> >> >> --- a/drivers/usb/host/ehci-hcd.c
>> >>> >> >> +++ b/drivers/usb/host/ehci-hcd.c
>> >>> >> >> @@ -117,10 +117,44 @@ static struct descriptor {
>> >>> >> >>
>> >>> >> >>  };
>> >>> >> >>
>> >>> >> >>  #if defined(CONFIG_EHCI_IS_TDI)
>> >>> >> >>
>> >>> >> >> -#define ehci_is_TDI()        (1)
>> >>> >> >> -#else
>> >>> >> >> -#define ehci_is_TDI()        (0)
>> >>> >> >> +# define ehci_is_TDI()       (1)
>> >>> >> >
>> >>> >> > btw you can remove those braces around (1) and (0) below. But I
>> >>> >> > have one more question ...
>> >>> >>
>> >>> >> Got it, thanks
>> >>> >>
>> >>> >> > [...]
>> >>> >> >
>> >>> >> >> @@ -609,13 +644,10 @@ ehci_submit_root(struct usb_device *dev,
>> >>> >> >> unsigned long pipe, void *buffer, uint32_t *status_reg;
>> >>> >> >>
>> >>> >> >>       struct ehci_ctrl *ctrl = dev->controller;
>> >>> >> >>
>> >>> >> >> -     if (le16_to_cpu(req->index) >
>> >>> >> >> CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS) { -             printf("The
>> >>> >> >> request port(%d) is not configured\n", -
>> >>> >> >>
>> >>> >> >>                  le16_to_cpu(req->index) - 1);
>> >>> >> >>
>> >>> >> >> +     status_reg = ehci_get_portsc_register(ctrl->hcor,
>> >>> >> >> +             le16_to_cpu(req->index) - 1);
>> >>> >> >> +     if (!status_reg)
>> >>> >> >
>> >>> >> > What happens here if req->index is zero ?
>> >>> >> >
>> >>> >> > Hint: the above code always does unsigned comparison ...
>> >>> >> >
>> >>> >> > I think you should make the second argument of
>> >>> >> > ehci_get_portsc_register() unsigned short too (as is req->index in
>> >>> >> > struct devrequest).
>> >>> >>
>> >>> >> Sorry, but I'll prefer 'int' over 'unsigned short', since it looks
>> >>> >> to me that the u-boot would set 'req->index' to 0 at startup, which
>> >>> >> results in a 'port = -1' to be passed to
>> >>> >> ehci_get_portsc_register().
>> >>> >>
>> >>> >> And I think '-1' is a better self-explain value, so I'd like to
>> >>> >> stick with 'int'
>> >>> >
>> >>> > Sure, but then the comparison is signed, not unsigned. Besides, it's
>> >>> > unnecessary change to the logic of the code. Or did I miss something
>> >>> > ?
>> >>>
>> >>> 1. There is a bug in ehci_submit_root() of usb ehci:
>> >>>     int ehci_submit_root()
>> >>>     {
>> >>>
>> >>>          ......
>> >>>          if (port > CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS) {
>> >>>
>> >>>             printf("The request port(%d) is not configured\n", port -
>> >>>             1); return -1;
>> >>>
>> >>>          }
>> >>>          status_reg = (uint32_t *)&ctrl->hcor->or_portsc[port - 1];
>> >>>          ......
>> >>>
>> >>>     }
>> >>>
>> >>>     The 'port' is actually a '0' at start-up, so we actually accessed
>> >>>
>> >>> a wrong register.
>> >>>
>> >>>     But fortunately the wrong register actually points to
>> >>>     CONFIGFLAG(0x40)
>> >>>
>> >>> with a safe value for the following codes.
>> >>>
>> >>> 2. One of Vivek Gautam's usb patches has altered the logic of usb host
>> >>>
>> >>>     upon launching 'usb start', if we report a error upon (port - 1 <
>> >>>     0), the current u-boot usb would failed to scan ports. (At least
>> >>>     it
>> >>>
>> >>> failed at Faraday platforms.)
>> >>>
>> >>>     However it looks to me that it's o.k to report a error upon (port
>> >>>
>> >>> - 1 < 0) at old usb ehci stack.
>> >>>
>> >>>     (i.e. 10 days ago, in master branch of u-boot)
>> >>>
>> >>> And thus I add a quick check to PATCH v7.
>> >>>
>> >>> __weak uint32_t *ehci_get_portsc_register(struct ehci_hcor *hcor, int
>> >>> port) {
>> >>>
>> >>>  /*
>> >>>
>> >>>   * The u-boot would somehow set port=-1 at usb start-up,
>> >>>   * so this quick fix is necessary.
>> >>>   */
>> >>>
>> >>>  if (port < 0)
>> >>>
>> >>>   port = 0;
>> >>
>> >> Maybe we should return fail, no ?
>> >
>> > No, it would make the 'usb start' to terminate immediately,
>> > and results in a port scan failure to at least Faraday EHCI.
>> >
>> >> Can you pinpoint where does the req->index
>> >> (resp. port) get set to -1 ?
>> >
>> > Later I'll try to find out where we have 'req->index' set as a '0' in
>> > 'usb start'.
>>
>> It's from usb_new_device() --> usb_get_descriptor(), and thus
>> it's definitely correct to set index = 0 right here.
>> The only problem is 'We shall not always report an portsc error for
>> all request!'
>> Please refer to the patch attached at the tail of this mail.
>>
>> >> And which commit introduced this breakage ?
>> >
>> > I believe it's there long ago, we just fortunately bypass the error at
>> > old day, and now one of Vivek Gautam's USB patch make us face up to this
>> > issue.
>>
>> Shame on me....
>> It's nothing to do with Vivek Gautam's USB patch or the old USB ehci stack,
>> it's only mattered while reporting an portsc error. :P
>>
>> >> Best regards,
>> >> Marek Vasut
>
> Ok, I think I almost see it there. Can you just make a patchset out of this
> stuff so I can digest it a little bit easier? Maybe stick the fixes first so I
> can pick them ASAP and the FHCI stuff after that.

Sure, I'll post a new patchset with this as 1st part.

--
Best wishes,
Kuo-Jung Su

Patch

diff --git a/drivers/usb/host/ehci-faraday.c b/drivers/usb/host/ehci-faraday.c
index 2cf8b15..14f0894 100644
--- a/drivers/usb/host/ehci-faraday.c
+++ b/drivers/usb/host/ehci-faraday.c
@@ -136,16 +136,10 @@  int ehci_get_port_speed(struct ehci_hcor *hcor,
uint32_t reg)
  */
 uint32_t *ehci_get_portsc_register(struct ehci_hcor *hcor, int port)
 {
- /*
- * The u-boot would somehow set port=-1 at usb start-up,
- * so this quick fix is necessary.
- */
- if (port < 0)
- port = 0;
-
  /* Faraday EHCI has one and only one portsc register */
  if (port) {
- printf("The request port(%d) is not configured\n", port);
+ /* ! Enable the print would lead to a timing issue ! */
+ debug("The request port(%d) is not configured\n", port);
  return NULL;
  }

diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index be18a02..67d7d3f 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -607,15 +607,9 @@  fail:

 __weak uint32_t *ehci_get_portsc_register(struct ehci_hcor *hcor, int port)
 {
- /*
- * The u-boot would somehow set port=-1 at usb start-up,
- * so this quick fix is necessary.
- */
- if (port < 0)
- port = 0;
-
- if (port >= CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS) {
- printf("The request port(%u) is not configured\n", port);
+ if (port < 0 || port >= CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS) {
+ /* ! Enable the print would lead to a timing issue ! */
+ debug("The request port(%u) is not configured\n", port);
  return NULL;
  }

@@ -636,8 +630,6 @@  ehci_submit_root(struct usb_device *dev, unsigned
long pipe, void *buffer,
  struct ehci_ctrl *ctrl = dev->controller;

  status_reg = ehci_get_portsc_register(ctrl->hcor, port - 1);
- if (!status_reg)
- return -1;
  srclen = 0;