Re: BUG: one patch causes imx51 Babbage board detect usb hub failed

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sat, May 19, 2012 at 12:43 AM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> On Fri, 18 May 2012, Alan Stern wrote:
>
>> On Fri, 18 May 2012, Chen Peter-B29397 wrote:
>>
>> > Hi Ming,
>> >
>> > I have debugged it at this afternoon, your description for this problem
>> > is correct, the ehci_reset override the ehci->command which is assigned
>> > at ehci_init, and ehci->command at ehci_init is correct.
>> >
>> > My opinion for this fix is just remove the
>> > " ehci->command = ehci_readl(ehci, &ehci->regs->command);" at ehci_reset.
>> > Even someone called ehci_reset to reset controller, it can still use command
>> > value assigned at ehci_init.
>>
>> Yes, I see the problem.  And you are right; the value assigned during
>> ehci_init() should not be overridden.
>>
>> However...  The value assigned in ehci_init() is not fully correct.
>> That routine does not examine the default value in the USBCMD register
>> before making its own settings.  (It can't -- the USBCMD register may
>> contain random garbage because the controller hasn't been reset at this
>> point.)
>>
>> This means that ehci_init() can't handle hardware updates.  New
>> versions of the hardware may define bits in USBCMD that the code
>> doesn't know about.  Those bits would be set properly by default when a
>> hardware reset occurs, but then ehci_init() would wipe them out.
>>
>> It looks like a bunch of code needs to be moved from ehci_init() to
>> ehci_reset() -- everything involved in computing ehci->command.  The
>> idea is that once the controller has been reset, we first set
>> ehci->command to the value in the USBCMD register (as is done now).
>> Then we can modify the bits we know about (as the code in ehci_init()
>> does) while leaving the other bits alone.
>
> A simpler idea just occurred to me.  All we need to do is change the
> line in ehci_reset() that assigns ehci->command.
>
> What it should do is mask out the bits that were set up in ehci_init().
> Those bits should be kept unchanged in ehci->command.  The remaining
> bits should be set according to the value read from the USBCMD
> register.

IMO, it is not a good idea.

Firstly, after writing RESET on command register, the default value of
CMD register
should be restored to '00080000h (00080B00h if Asynchronous Schedule Park
Capability is a one)'[1], and both 'Interrupt Threshold Control' and
'Park mode' has
been set in ehci_init(), so using ehci->command after ehci_reset is correct.

Secondly, remaining other bits from HW is not good since HW may be buggy
and the default value after reset may be uncertain, and it is better
to not depend
on them.  For some newer hardware, if the reserved fields are to be
used, just do
it in ehci_init like setting lpm bits.

Finally, before the commit 3d9545cc375d117554a9b35dfddadf9189c62775(
EHCI: maintain the ehci->command value properly), the ehci->command is used
just directly in ehci_run and the other HW bits are not kept from HW,
so it is better
to be consistent as before to avoid introducing possible regression.

[1], 4.1 Host Controller Initialization of EHCI Specification 1.0

> Would anyone like to code that up?

Looks removing the line of 'ehci->command = ehci_readl(ehci,
&ehci->regs->command);' in ehci_reset is OK.


Thanks,
--
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux