From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kuo-Jung Su Date: Wed, 15 May 2013 09:03:44 +0800 Subject: [U-Boot] [PATCH v6 2/4] usb: ehci: add weak-aliased functions to portsc & tdi In-Reply-To: <201305141547.45524.marex@denx.de> References: <1368410846-18038-1-git-send-email-dantesu@gmail.com> <201305131710.58010.marex@denx.de> <201305141547.45524.marex@denx.de> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 2013/5/14 Marek Vasut : > Dear Kuo-Jung Su, > >> 2013/5/13 Marek Vasut : >> > Dear Kuo-Jung Su, >> > >> >> 2013/5/13 Marek Vasut : >> >> > Dear Kuo-Jung Su, >> >> > >> >> >> From: Kuo-Jung Su >> >> >> >> >> >> 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 >> >> >> CC: Marek Vasut >> >> >> --- >> >> >> >> >> >> 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 ? No, it would make the 'usb start' to terminate immediately, and results in a port scan failure to at least Faraday EHCI. > Can you pinpoint where does the req->index > (resp. port) get set to -1 ? Later I'll try to find out where we have 'req->index' set as a '0' in 'usb start'. > And which commit introduced this breakage ? I believe it's there long ago, we just fortunately bypass the error at old day, and now one of Vivek Gautam's USB patch make us face up to this issue. > > Best regards, > Marek Vasut -- Best wishes, Kuo-Jung Su