| Message ID | CAK65tU5JpGNgpYDyDFh3RZE9ABd=ZfyLgPJS44yf-gXHnhXz4g@mail.gmail.com |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Marek Vasut |
| Headers | show |
Dear Kuo-Jung Su, > 2013/5/15 Kuo-Jung Su <dantesu@gmail.com>: > > 2013/5/14 Marek Vasut <marex@denx.de>: > >> 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) 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 Ok, I think I almost see it there. Can you just make a patchset out of this stuff so I can digest it a little bit easier? Maybe stick the fixes first so I can pick them ASAP and the FHCI stuff after that.
2013/5/15 Marek Vasut <marex@denx.de>: > Dear Kuo-Jung Su, > >> 2013/5/15 Kuo-Jung Su <dantesu@gmail.com>: >> > 2013/5/14 Marek Vasut <marex@denx.de>: >> >> 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) 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 > > Ok, I think I almost see it there. Can you just make a patchset out of this > stuff so I can digest it a little bit easier? Maybe stick the fixes first so I > can pick them ASAP and the FHCI stuff after that. Sure, I'll post a new patchset with this as 1st part. -- Best wishes, Kuo-Jung Su
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;