Re: [patch v3 2/3] at91_udc HW glitch

Re: [patch v3 2/3] at91_udc HW glitch

[Andrew Victor]

hi,

> Add some delay to avoid reading CSR TXCOUNT too early after updating it.
>
> Signed-off-by: Anti Sullin <anti.sullin@artecdesign.ee>
> Signed-off-by: Harro Haan <hrhaan@gmail.com>
> Acked-by: Remy Bohmer <linux@bohmer.net>
> ---
>  drivers/usb/gadget/at91_udc.c |    7 +++++++
>  1 file changed, 7 insertions(+)
>
> Index: linux-2.6.31/drivers/usb/gadget/at91_udc.c
> ===================================================================
> --- linux-2.6.31.orig/drivers/usb/gadget/at91_udc.c
> +++ linux-2.6.31/drivers/usb/gadget/at91_udc.c
> @@ -366,6 +366,13 @@ rescan:
>        if (is_done)
>                done(ep, req, 0);
>        else if (ep->is_pingpong) {
> +               /*
> +                * One dummy read to delay the code because of a HW glitch:
> +                * CSR returns bad RXCOUNT when read too soon after updating
> +                * RX_DATA_BK flags.
> +                */
> +               csr = __raw_readl(creg);
> +
>                bufferspace -= count;
>                buf += count;
>                goto rescan;

I see in the data-sheet (SAM9261 / SAM9263), the following for the
UDP_ CSRx registers:

"WARNING: Due to synchronization between MCK and UDPCK, the software
application must wait for the end of the write
operation before executing another write by polling the bits which
must be set/cleared."

//! Clear flags of UDP UDP_CSR register and waits for synchronization
#define Udp_ep_clr_flag(pInterface, endpoint, flags) { \
    while (pInterface->UDP_CSR[endpoint] & (flags)) \
        pInterface->UDP_CSR[endpoint] &= ~(flags); \
     }
//! Set flags of UDP UDP_CSR register and waits for synchronization
#define Udp_ep_set_flag(pInterface, endpoint, flags) { \
    while ( (pInterface->UDP_CSR[endpoint] & (flags)) != (flags) ) \
       pInterface->UDP_CSR[endpoint] |= (flags); \
    }

The at91_udc driver does not seem to do that for its CSR register writes.
So I was wondering if we implement what the datasheet says, would we
still need the "fix" above.


Regards,
  Andrew Victor

Re: [patch v3 2/3] at91_udc HW glitch

[Harro Haan]

On 23 March 2010 21:26, Andrew Victor <avictor.za@gmail.com> wrote:
>
> hi,
>
>
> I see in the data-sheet (SAM9261 / SAM9263), the following for the
> UDP_ CSRx registers:
>
> "WARNING: Due to synchronization between MCK and UDPCK, the software
> application must wait for the end of the write
> operation before executing another write by polling the bits which
> must be set/cleared."
>
> //! Clear flags of UDP UDP_CSR register and waits for synchronization
> #define Udp_ep_clr_flag(pInterface, endpoint, flags) { \
>    while (pInterface->UDP_CSR[endpoint] & (flags)) \
>        pInterface->UDP_CSR[endpoint] &= ~(flags); \
>     }
> //! Set flags of UDP UDP_CSR register and waits for synchronization
> #define Udp_ep_set_flag(pInterface, endpoint, flags) { \
>    while ( (pInterface->UDP_CSR[endpoint] & (flags)) != (flags) ) \
>       pInterface->UDP_CSR[endpoint] |= (flags); \
>    }
>
> The at91_udc driver does not seem to do that for its CSR register writes.
> So I was wondering if we implement what the datasheet says, would we
> still need the "fix" above.
>
>
> Regards,
>  Andrew Victor

According to the following post it did not solve the problem:

"There are references to syncronization issues between clock domains
in documentation and source code. These references describe required
delays when setting or clearing bits in CSR until the bit actually
changes and the required delays between writing CSR and reading the
data register. Incorporating the delays suggested in datasheet to code
did not change anything, so I dug deeper."
source:
http://lists.arm.linux.org.uk/lurker/message/20090325.150843.f515c02f.en.html

I did not test this myself, maybe Anti Sullin can give more details.

Regards,

Harro Haan

BTW: same message, but now in plain text.

Re: [patch v3 2/3] at91_udc HW glitch

[Anti Sullin]

Harro Haan wrote:
> On 23 March 2010 21:26, Andrew Victor <avictor.za@gmail.com> wrote:
>> The at91_udc driver does not seem to do that for its CSR register writes.
>> So I was wondering if we implement what the datasheet says, would we
>> still need the "fix" above.

Another read is still needed after verifying that the flag is changed.

We are writing a bit that does not have a register behind it. It just
triggers the acknowledge that the data has been read.
We could poll if the corresponding data ready flag is cleared to check
if the write has propagated. But this leads to another problem:
the reads do not seem to be re-syncronized between clock domains.
We just get what is there at the moment the read is performed. This
means that even if the "data ready" bit is cleared, we could not be sure
at that moment that the rest of the changes have been performed
that were triggered by the same write. We even get reads, where
some bits in rx data counter have changed and some bits are old.
So to be fully sure, we should wait until the bit has been cleared
and then perform another read to be sure that the rest of the bits
have been changed as well. See my 2009-03-25 17:08 e-mail for details.

>From practical standpoint: I have not seen a case where the
second read was not ready. If this delay is adequate for the worst
case propagate time it takes for the bits to change, it is not
needed to read the register more than twice. I determined this
experimentally by reading the register 3 times right after write
and comparing the  2nd and 3rd read when doing different transfers
between PC and ARM. As I have tested it only on 9261, somebody should
either run the same kind of tests on other SoC's as well or figure
out the worst case timings on all of them.
The datasheet describes that some changes are performed in
3 USB clock + 3 master clock periods. If so, then one/some extra reads
could create the master-clock dependant small delay needed to
be sure that everything is ready.

Actually, this leads to a another problem. We are able to read the
rx fifo count when the bits are changing there. If some data is being
received at the very moment when we read the register, we're in
trouble. When some bits are old and some new, we can get values that
are larger or smaller than the actual value (ie 0111->1000 change).
This is a rare condition, but it might happen. Should this register
be read always twice to check that nothing was unstable during the
first read?

I would still leave in this extra read because it is known to be
unstable. If it is needed on some SoC's, we could read out the
register value until we get 2 same results to verify that it is
stable. But there is no point of reading the first (known bad) value.

-- 
Anti Sullin
Embedded Software Engineer
Artec Design LLC
Teaduspargi 6/2, 12618, Tallinn, Estonia
http://www.artecdesign.ee