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

[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'

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