From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marek Vasut <marex@denx.de>
Date: Mon, 13 May 2013 04:32:12 +0200
Subject: [U-Boot] [PATCH v6 2/4] usb: ehci: add weak-aliased functions
	to portsc & tdi
In-Reply-To: <1368410846-18038-3-git-send-email-dantesu@gmail.com>
References: <1368410846-18038-1-git-send-email-dantesu@gmail.com>
	<1368410846-18038-3-git-send-email-dantesu@gmail.com>
Message-ID: <201305130432.12699.marex@denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.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 ...

[...]

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