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 13, 2013, 2:07 a.m.
Message ID <1368410846-18038-3-git-send-email-dantesu@gmail.com>
Download mbox | patch
Permalink /patch/243236/
State Superseded
Delegated to: Marek Vasut
Headers show

Comments

Kuo-Jung Su - May 13, 2013, 2:07 a.m.
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(-)

--
1.7.9.5
Marek Vasut - May 13, 2013, 2:32 a.m.
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 ...

[...]

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

>  		return -1;
> -	}
> -	status_reg = (uint32_t *)&ctrl->hcor->or_portsc[
> -						le16_to_cpu(req->index) - 1];
>  	srclen = 0;
> 
>  	debug("req=%u (%#x), type=%u (%#x), value=%u, index=%u\n",

[...]

Best regards,
Marek Vasut
Kuo-Jung Su - May 13, 2013, 8:09 a.m.
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'

>>               return -1;
>> -     }
>> -     status_reg = (uint32_t *)&ctrl->hcor->or_portsc[
>> -                                             le16_to_cpu(req->index) - 1];
>>       srclen = 0;
>>
>>       debug("req=%u (%#x), type=%u (%#x), value=%u, index=%u\n",
>
> [...]
>
> Best regards,
> Marek Vasut



--
Best wishes,
Kuo-Jung Su
Marek Vasut - May 13, 2013, 3:10 p.m.
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 ?

Best regards,
Marek Vasut
Kuo-Jung Su - May 14, 2013, 1:26 a.m.
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;

 if (port >= CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS) {
  printf("The request port(%u) is not configured\n", port);
  return NULL;
 }

 return (uint32_t *)&hcor->or_portsc[port];
}

However I found that I've stupidly copied the logic

' if (port >= CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS) '

into ehci-faraday.c, so I'll post a v8 for this few minutes later....

> Best regards,
> Marek Vasut



--
Best wishes,
Kuo-Jung Su
Marek Vasut - May 14, 2013, 1:47 p.m.
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 ? Can you pinpoint where does the req->index 
(resp. port) get set to -1 ? And which commit introduced this breakage ?

Best regards,
Marek Vasut
Kuo-Jung Su - May 15, 2013, 1:03 a.m.
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'.

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

>
> Best regards,
> Marek Vasut



--
Best wishes,
Kuo-Jung Su

Patch

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)
+
+/* put TDI/ARC silicon into EHCI mode */
+__weak void ehci_tdi_reset(struct ehci_hcor *hcor)
+{
+	uint32_t tmp, *reg_ptr;
+
+	reg_ptr = (uint32_t *)((uint8_t *) + USBMODE);
+	tmp = ehci_readl(reg_ptr);
+	tmp |= USBMODE_CM_HC;
+#if defined(CONFIG_EHCI_MMIO_BIG_ENDIAN)
+	tmp |= USBMODE_BE;
 #endif
+	ehci_writel(reg_ptr, tmp);
+}
+
+__weak int ehci_port_speed(struct ehci_hcor *hcor, unsigned int portsc)
+{
+	int ret = 0;
+
+	switch (PORTSC_PSPD(portsc)) {
+	case PORTSC_PSPD_FS:
+		break;
+	case PORTSC_PSPD_LS:
+		ret = USB_PORT_STAT_LOW_SPEED;
+		break;
+	case PORTSC_PSPD_HS:
+	default:
+		ret = USB_PORT_STAT_HIGH_SPEED;
+		break;
+	}
+
+	return ret;
+}
+
+#else  /* CONFIG_EHCI_IS_TDI */
+# define ehci_is_TDI()	(0)
+#endif /* CONFIG_EHCI_IS_TDI */

 void __ehci_powerup_fixup(uint32_t *status_reg, uint32_t *reg)
 {
@@ -149,8 +183,6 @@  static int handshake(uint32_t *ptr, uint32_t mask, uint32_t done, int usec)
 static int ehci_reset(int index)
 {
 	uint32_t cmd;
-	uint32_t tmp;
-	uint32_t *reg_ptr;
 	int ret = 0;

 	cmd = ehci_readl(&ehcic[index].hcor->or_usbcmd);
@@ -163,15 +195,8 @@  static int ehci_reset(int index)
 		goto out;
 	}

-	if (ehci_is_TDI()) {
-		reg_ptr = (uint32_t *)((u8 *)ehcic[index].hcor + USBMODE);
-		tmp = ehci_readl(reg_ptr);
-		tmp |= USBMODE_CM_HC;
-#if defined(CONFIG_EHCI_MMIO_BIG_ENDIAN)
-		tmp |= USBMODE_BE;
-#endif
-		ehci_writel(reg_ptr, tmp);
-	}
+	if (ehci_is_TDI())
+		ehci_tdi_reset(ehcic[index].hcor);

 #ifdef CONFIG_USB_EHCI_TXFIFO_THRESH
 	cmd = ehci_readl(&ehcic[index].hcor->or_txfilltuning);
@@ -597,6 +622,16 @@  static inline int min3(int a, int b, int c)
 	return a;
 }

+__weak uint32_t *ehci_get_portsc_register(struct ehci_hcor *hcor, int port)
+{
+	if (port >= CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS) {
+		printf("The request port(%d) is not configured\n", port);
+		return NULL;
+	}
+
+	return (uint32_t *)&hcor->or_portsc[port];
+}
+
 int
 ehci_submit_root(struct usb_device *dev, unsigned long pipe, void *buffer,
 		 int length, struct devrequest *req)
@@ -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)
 		return -1;
-	}
-	status_reg = (uint32_t *)&ctrl->hcor->or_portsc[
-						le16_to_cpu(req->index) - 1];
 	srclen = 0;

 	debug("req=%u (%#x), type=%u (%#x), value=%u, index=%u\n",
@@ -709,23 +741,10 @@  ehci_submit_root(struct usb_device *dev, unsigned long pipe, void *buffer,
 			tmpbuf[0] |= USB_PORT_STAT_RESET;
 		if (reg & EHCI_PS_PP)
 			tmpbuf[1] |= USB_PORT_STAT_POWER >> 8;
-
-		if (ehci_is_TDI()) {
-			switch (PORTSC_PSPD(reg)) {
-			case PORTSC_PSPD_FS:
-				break;
-			case PORTSC_PSPD_LS:
-				tmpbuf[1] |= USB_PORT_STAT_LOW_SPEED >> 8;
-				break;
-			case PORTSC_PSPD_HS:
-			default:
-				tmpbuf[1] |= USB_PORT_STAT_HIGH_SPEED >> 8;
-				break;
-			}
-		} else {
+		if (ehci_is_TDI())
+			tmpbuf[1] |= ehci_port_speed(ctrl->hcor, reg) >> 8;
+		else
 			tmpbuf[1] |= USB_PORT_STAT_HIGH_SPEED >> 8;
-		}
-
 		if (reg & EHCI_PS_CSC)
 			tmpbuf[2] |= USB_PORT_STAT_C_CONNECTION;
 		if (reg & EHCI_PS_PEC)