[U-Boot] [PATCH v6 2/4] usb: ehci: add weak-aliased functions to portsc & tdi
Kuo-Jung Su
dantesu at gmail.com
Wed May 15 04:42:57 CEST 2013
2013/5/15 Kuo-Jung Su <dantesu at gmail.com>:
> 2013/5/14 Marek Vasut <marex at denx.de>:
>> Dear Kuo-Jung Su,
>>
>>> 2013/5/13 Marek Vasut <marex at denx.de>:
>>> > Dear Kuo-Jung Su,
>>> >
>>> >> 2013/5/13 Marek Vasut <marex at denx.de>:
>>> >> > Dear Kuo-Jung Su,
>>> >> >
>>> >> >> From: Kuo-Jung Su <dantesu at 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 at faraday-tech.com>
>>> >> >> CC: Marek Vasut <marex at 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
>
>
Here is the quick fix to this issue:
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;
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:
More information about the U-Boot
mailing list