| Message ID | 1368410846-18038-3-git-send-email-dantesu@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Marek Vasut |
| Headers | show |
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 ... [...] > @@ -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). > return -1; > - } > - status_reg = (uint32_t *)&ctrl->hcor->or_portsc[ > - le16_to_cpu(req->index) - 1]; > srclen = 0; > > debug("req=%u (%#x), type=%u (%#x), value=%u, index=%u\n", [...] Best regards, Marek Vasut
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' >> return -1; >> - } >> - status_reg = (uint32_t *)&ctrl->hcor->or_portsc[ >> - le16_to_cpu(req->index) - 1]; >> srclen = 0; >> >> debug("req=%u (%#x), type=%u (%#x), value=%u, index=%u\n", > > [...] > > Best regards, > Marek Vasut -- Best wishes, Kuo-Jung Su
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 ? Best regards, Marek Vasut
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; if (port >= CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS) { printf("The request port(%u) is not configured\n", port); return NULL; } return (uint32_t *)&hcor->or_portsc[port]; } However I found that I've stupidly copied the logic ' if (port >= CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS) ' into ehci-faraday.c, so I'll post a v8 for this few minutes later.... > Best regards, > Marek Vasut -- Best wishes, Kuo-Jung Su
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 ? Can you pinpoint where does the req->index (resp. port) get set to -1 ? And which commit introduced this breakage ? Best regards, Marek Vasut
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'. > 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
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) + +/* put TDI/ARC silicon into EHCI mode */ +__weak void ehci_tdi_reset(struct ehci_hcor *hcor) +{ + uint32_t tmp, *reg_ptr; + + reg_ptr = (uint32_t *)((uint8_t *) + USBMODE); + tmp = ehci_readl(reg_ptr); + tmp |= USBMODE_CM_HC; +#if defined(CONFIG_EHCI_MMIO_BIG_ENDIAN) + tmp |= USBMODE_BE; #endif + ehci_writel(reg_ptr, tmp); +} + +__weak int ehci_port_speed(struct ehci_hcor *hcor, unsigned int portsc) +{ + int ret = 0; + + switch (PORTSC_PSPD(portsc)) { + case PORTSC_PSPD_FS: + break; + case PORTSC_PSPD_LS: + ret = USB_PORT_STAT_LOW_SPEED; + break; + case PORTSC_PSPD_HS: + default: + ret = USB_PORT_STAT_HIGH_SPEED; + break; + } + + return ret; +} + +#else /* CONFIG_EHCI_IS_TDI */ +# define ehci_is_TDI() (0) +#endif /* CONFIG_EHCI_IS_TDI */ void __ehci_powerup_fixup(uint32_t *status_reg, uint32_t *reg) { @@ -149,8 +183,6 @@ static int handshake(uint32_t *ptr, uint32_t mask, uint32_t done, int usec) static int ehci_reset(int index) { uint32_t cmd; - uint32_t tmp; - uint32_t *reg_ptr; int ret = 0; cmd = ehci_readl(&ehcic[index].hcor->or_usbcmd); @@ -163,15 +195,8 @@ static int ehci_reset(int index) goto out; } - if (ehci_is_TDI()) { - reg_ptr = (uint32_t *)((u8 *)ehcic[index].hcor + USBMODE); - tmp = ehci_readl(reg_ptr); - tmp |= USBMODE_CM_HC; -#if defined(CONFIG_EHCI_MMIO_BIG_ENDIAN) - tmp |= USBMODE_BE; -#endif - ehci_writel(reg_ptr, tmp); - } + if (ehci_is_TDI()) + ehci_tdi_reset(ehcic[index].hcor); #ifdef CONFIG_USB_EHCI_TXFIFO_THRESH cmd = ehci_readl(&ehcic[index].hcor->or_txfilltuning); @@ -597,6 +622,16 @@ static inline int min3(int a, int b, int c) return a; } +__weak uint32_t *ehci_get_portsc_register(struct ehci_hcor *hcor, int port) +{ + if (port >= CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS) { + printf("The request port(%d) is not configured\n", port); + return NULL; + } + + return (uint32_t *)&hcor->or_portsc[port]; +} + int ehci_submit_root(struct usb_device *dev, unsigned long pipe, void *buffer, int length, struct devrequest *req) @@ -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) return -1; - } - status_reg = (uint32_t *)&ctrl->hcor->or_portsc[ - le16_to_cpu(req->index) - 1]; srclen = 0; debug("req=%u (%#x), type=%u (%#x), value=%u, index=%u\n", @@ -709,23 +741,10 @@ ehci_submit_root(struct usb_device *dev, unsigned long pipe, void *buffer, tmpbuf[0] |= USB_PORT_STAT_RESET; if (reg & EHCI_PS_PP) tmpbuf[1] |= USB_PORT_STAT_POWER >> 8; - - if (ehci_is_TDI()) { - switch (PORTSC_PSPD(reg)) { - case PORTSC_PSPD_FS: - break; - case PORTSC_PSPD_LS: - tmpbuf[1] |= USB_PORT_STAT_LOW_SPEED >> 8; - break; - case PORTSC_PSPD_HS: - default: - tmpbuf[1] |= USB_PORT_STAT_HIGH_SPEED >> 8; - break; - } - } else { + if (ehci_is_TDI()) + tmpbuf[1] |= ehci_port_speed(ctrl->hcor, reg) >> 8; + else tmpbuf[1] |= USB_PORT_STAT_HIGH_SPEED >> 8; - } - if (reg & EHCI_PS_CSC) tmpbuf[2] |= USB_PORT_STAT_C_CONNECTION; if (reg & EHCI_PS_PEC)