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

[Marek Vasut]

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

[...]

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