From patchwork Wed May 15 02:42:57 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kuo-Jung Su X-Patchwork-Id: 243869 X-Patchwork-Delegate: marek.vasut@gmail.com Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from theia.denx.de (theia.denx.de [85.214.87.163]) by ozlabs.org (Postfix) with ESMTP id 5E56C2C0082 for ; Wed, 15 May 2013 12:43:21 +1000 (EST) Received: from localhost (localhost [127.0.0.1]) by theia.denx.de (Postfix) with ESMTP id 17FE24A029; Wed, 15 May 2013 04:43:18 +0200 (CEST) X-Virus-Scanned: Debian amavisd-new at theia.denx.de Received: from theia.denx.de ([127.0.0.1]) by localhost (theia.denx.de [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id P3tBqBF+mk4V; Wed, 15 May 2013 04:43:16 +0200 (CEST) Received: from theia.denx.de (localhost [127.0.0.1]) by theia.denx.de (Postfix) with ESMTP id 2959A4A023; Wed, 15 May 2013 04:43:16 +0200 (CEST) Received: from localhost (localhost [127.0.0.1]) by theia.denx.de (Postfix) with ESMTP id BAA654A023 for ; Wed, 15 May 2013 04:43:09 +0200 (CEST) X-Virus-Scanned: Debian amavisd-new at theia.denx.de Received: from theia.denx.de ([127.0.0.1]) by localhost (theia.denx.de [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 19gmJmroYRVg for ; Wed, 15 May 2013 04:43:04 +0200 (CEST) X-policyd-weight: NOT_IN_SBL_XBL_SPAMHAUS=-1.5 NOT_IN_SPAMCOP=-1.5 NOT_IN_BL_NJABL=-1.5 (only DNSBL check requested) Received: from mail-ie0-f175.google.com (mail-ie0-f175.google.com [209.85.223.175]) by theia.denx.de (Postfix) with ESMTPS id 0DC194A020 for ; Wed, 15 May 2013 04:42:59 +0200 (CEST) Received: by mail-ie0-f175.google.com with SMTP id s9so2575423iec.6 for ; Tue, 14 May 2013 19:42:57 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:x-received:in-reply-to:references:date:message-id :subject:from:to:cc:content-type; bh=ljJiW+MK8lX6mU31VoNHqkhNZc+7dYPvsU0RczK+S7s=; b=xnM7nTgdBQSopx4EEDq0AA7xH2GvB4LHCaeeg4fNsx3gvHfz4qod1f+nafSUQ7RQlt 0joXzV36T6qT2fu944I0LTboJT782tzDa+xz7fDu4YM7PsQzbiJGJpuD4nMv3QkajHnZ NxxLU5xS5LneQBhsaOe3V19iTAJ94JMmTE5SrmQgAZUxCyLYohKdjVxET5Acu8wKQmot PG0/RJe5WKJxJq+1n/6128mko7wnuR1AgAjdpItNvPXHlLYoiQ9F9qSnqZJz/fACwU+j MDq88jqszQSgoLVmAABpmOyUVCPLLS41Li9i4k7G60GX5hYqCTrqmDfptwNZPC85VFZY lKHw== MIME-Version: 1.0 X-Received: by 10.50.130.11 with SMTP id oa11mr4115439igb.108.1368585777368; Tue, 14 May 2013 19:42:57 -0700 (PDT) Received: by 10.64.65.106 with HTTP; Tue, 14 May 2013 19:42:57 -0700 (PDT) In-Reply-To: References: <1368410846-18038-1-git-send-email-dantesu@gmail.com> <201305131710.58010.marex@denx.de> <201305141547.45524.marex@denx.de> Date: Wed, 15 May 2013 10:42:57 +0800 Message-ID: From: Kuo-Jung Su To: Marek Vasut Cc: U-Boot list , Kuo-Jung Su , Vivek Gautam Subject: Re: [U-Boot] [PATCH v6 2/4] usb: ehci: add weak-aliased functions to portsc & tdi X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.11 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: u-boot-bounces@lists.denx.de Errors-To: u-boot-bounces@lists.denx.de 2013/5/15 Kuo-Jung Su : > 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) at 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'. > It's from usb_new_device() --> usb_get_descriptor(), and thus it's definitely correct to set index = 0 right here. The only problem is 'We shall not always report an portsc error for all request!' Please refer to the patch attached at the tail of this mail. >> 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. > Shame on me.... It's nothing to do with Vivek Gautam's USB patch or the old USB ehci stack, it's only mattered while reporting an portsc error. :P >> >> Best regards, >> Marek Vasut > > Here is the quick fix to this issue: debug("req=%u (%#x), type=%u (%#x), value=%u, index=%u\n", @@ -717,6 +709,8 @@ ehci_submit_root(struct usb_device *dev, unsigned long pipe, void *buffer, srclen = 2; break; case USB_REQ_GET_STATUS | ((USB_RT_PORT | USB_DIR_IN) << 8): + if (!status_reg) + return -1; memset(tmpbuf, 0, 4); reg = ehci_readl(status_reg); if (reg & EHCI_PS_CS) @@ -761,6 +755,8 @@ ehci_submit_root(struct usb_device *dev, unsigned long pipe, void *buffer, srclen = 4; break; case USB_REQ_SET_FEATURE | ((USB_DIR_OUT | USB_RT_PORT) << 8): + if (!status_reg) + return -1; reg = ehci_readl(status_reg); reg &= ~EHCI_PS_CLEAR; switch (le16_to_cpu(req->value)) { @@ -825,6 +821,8 @@ ehci_submit_root(struct usb_device *dev, unsigned long pipe, void *buffer, (void) ehci_readl(&ctrl->hcor->or_usbcmd); break; case USB_REQ_CLEAR_FEATURE | ((USB_DIR_OUT | USB_RT_PORT) << 8): + if (!status_reg) + return -1; reg = ehci_readl(status_reg); switch (le16_to_cpu(req->value)) { case USB_PORT_FEAT_ENABLE: diff --git a/drivers/usb/host/ehci-faraday.c b/drivers/usb/host/ehci-faraday.c index 2cf8b15..14f0894 100644 --- a/drivers/usb/host/ehci-faraday.c +++ b/drivers/usb/host/ehci-faraday.c @@ -136,16 +136,10 @@ int ehci_get_port_speed(struct ehci_hcor *hcor, uint32_t reg) */ 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; - /* Faraday EHCI has one and only one portsc register */ if (port) { - printf("The request port(%d) is not configured\n", port); + /* ! Enable the print would lead to a timing issue ! */ + debug("The request port(%d) is not configured\n", port); return NULL; } diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c index be18a02..67d7d3f 100644 --- a/drivers/usb/host/ehci-hcd.c +++ b/drivers/usb/host/ehci-hcd.c @@ -607,15 +607,9 @@ fail: __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; - - if (port >= CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS) { - printf("The request port(%u) is not configured\n", port); + if (port < 0 || port >= CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS) { + /* ! Enable the print would lead to a timing issue ! */ + debug("The request port(%u) is not configured\n", port); return NULL; } @@ -636,8 +630,6 @@ ehci_submit_root(struct usb_device *dev, unsigned long pipe, void *buffer, struct ehci_ctrl *ctrl = dev->controller; status_reg = ehci_get_portsc_register(ctrl->hcor, port - 1); - if (!status_reg) - return -1; srclen = 0;