[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 06:07:53 CEST 2013
2013/5/15 Marek Vasut <marex at denx.de>:
> Dear Kuo-Jung Su,
>
>> 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
>
> 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
More information about the U-Boot
mailing list