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

Marek Vasut
2013-05-14T13:47:45Z

Dear Kuo-Jung Su,

> 2013/5/13 Marek Vasut <marex@denx.de>:
> > Dear Kuo-Jung Su,
> >
> >> 2013/5/13 Marek Vasut <marex@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 ...
> >>
> >> 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)@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;

Maybe we should return fail, no ? Can you pinpoint where does the req->index
(resp. port) get set to -1 ? And which commit introduced this breakage ?

Best regards,
Marek Vasut