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

Marek Vasut
2013-05-13T02:32:12Z

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