From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marek Vasut Date: Tue, 14 May 2013 15:47:45 +0200 Subject: [U-Boot] [PATCH v6 2/4] usb: ehci: add weak-aliased functions to portsc & tdi In-Reply-To: References: <1368410846-18038-1-git-send-email-dantesu@gmail.com> <201305131710.58010.marex@denx.de> Message-ID: <201305141547.45524.marex@denx.de> List-Id: 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, > 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 ? Can you pinpoint where does the req->index (resp. port) get set to -1 ? And which commit introduced this breakage ? Best regards, Marek Vasut