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

[Kuo-Jung Su]

2013/5/14 Marek Vasut <marex at denx.de>:
> Dear Kuo-Jung Su,
>
>> 2013/5/13 Marek Vasut <marex at denx.de>:
>> > Dear Kuo-Jung Su,
>> >
>> >> 2013/5/13 Marek Vasut <marex at denx.de>:
>> >> > Dear Kuo-Jung Su,
>> >> >
>> >> >> From: Kuo-Jung Su <dantesu at 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 at faraday-tech.com>
>> >> >> CC: Marek Vasut <marex at 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