2013/5/13 Marek Vasut <ma...@denx.de>:
> Dear Kuo-Jung Su,
>
>> 2013/5/13 Marek Vasut <ma...@denx.de>:
>> > Dear Kuo-Jung Su,
>> >
>> >> From: Kuo-Jung Su <dant...@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 <dant...@faraday-tech.com>
>> >> CC: Marek Vasut <ma...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
