This is the mail archive of the ecos-patches@sources.redhat.com mailing list for the eCos project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Typo fix for nec_upd985xx USB device


>>>>> "Andrew" == Andrew Lunn <andrew.lunn@ascom.ch> writes:

> -    if (0 == (*USBS_IMR2 * IBUS_SWAP32(USBS_GSR2_URST))) {
> +    if (0 == (*USBS_IMR2 & IBUS_SWAP32(USBS_GSR2_URST))) {

    >> If it's known to fix a problem it almost doesn't matter whether
    >> it was a typo or not ;-). Bart's on vacation, so if you (or
    >> Andrew) can just confirm that, I'll check it in.

    Andrew> I cannot confirm it. I was just reading the code and
    Andrew> thought this was strange. I've not spent the time to
    Andrew> understand the code so it could be correct. I asked RedHat
    Andrew> to check it....

It is indeed a typo. If (*USBS_IMR2 * random_bits) had some really
clever effect that could not be achieved by some combination of and's
and or's, I would have written a suitable comment explaining why the
code was being clever.

What did confuse me for a while is why the current code works. It
should not. This code relates to USB resets generated by the host
controller. Such reset signals are generated when you connect the USB
peripheral to a host, to ensure that the peripheral is in a known
initial state. The signal is supposed to last at least 10ms, to give
the peripheral a chance to perform any cleanup operations that are
necessary.

The upd985xx USB devices have a problem (incidentally shared with the
SA1110 USB device) when dealing with such reset signals. The USB
device will raise an interrupt when the reset is detected. However
this interrupt is effectively level-triggered, not edge triggered. So
the interrupt gets raised, the ISR runs and processes it, interrupts
are reenabled before running pending DSRs, and the interrupt
immediately gets raised again. The net result is an interrupt storm
lasting about 10ms.

To avoid this, the ISR will mask out the USB reset interrupt via
USBS_IMR2. This interrupt must be unmasked again when the USB host
stops asserting reset. It so happens that the first operation after
a reset will always be a control message to the peripheral's endpoint
0, so ep0_rx_dsr() is a good place to unmask the reset interrupt. It
could be done unconditionally, but only doing it when necessary
involves slightly less manipulation of the USB hardware.

    if (0 == (*USBS_IMR2 & IBUS_SWAP32(USBS_GSR2_URST))) {
        *USBS_IMR2              |= IBUS_SWAP32(USBS_GSR2_URST); FLUSH_IBUS();
        usbs_upd985xx_gsr2_mask |= USBS_GSR2_URST;
    }

There are two other interesting bits in USBS_IMR2, USBS_GSR2_URSM and
USBS_GSR2_USPD, and these bits are always enabled. There are also lots
of uninterestings bits in this interrupt mask register, left disabled.
So the expression will evaluate as either:

    if (0 == ((URSM | USPD) * URST))

or

    if (0 == ((URSM | USPD | URST) * URST))

depending on whether or not the reset interrupt is currently enabled.
It would seem that this test will always fail, so the reset interrupt
will never be reenabled.

Linux and Windows hosts behave slightly differently. On a Linux host
this would mean that you can connect a peripheral to a host, but when
you disconnect and reconnect it things would go wrong - the required
reset operation on the reconnect would not be processed. A Windows
host actually performs two 10ms resets when a USB peripheral is
connected, so it would be impossible to connect a peripheral even
once. I think I would have noticed either of these problems while
developing the code...

The reason why the current code actually does work is because of the
large number of bits in the USBS_IMR2 register, giving:

    #define USBS_GSR2_URSM                  (0x01 << 18)
    #define USBS_GSR2_URST                  (0x01 << 17)
    #define USBS_GSR2_USPD                  (0x01 << 16)

If you multiply ((URSM | USPD) * URST), or ((URSM | USPD | URST) * URST),
there is a 32-bit overflow and the result of the calculation is always
0. Luckily. So the "if" test always succeeds, the interrupt mask
register always gets updated even when not necessary, and things
happen to work anyway.

Now that I understand what is going on, I'll apply the patch :-)

Bart


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]