[#4060] bfin_5xx bug/anomaly workaround

Submitted By: Greg Semeraro

Open Date

2008-04-24 14:35:07 Close Date

2008-04-24 18:23:42

Priority:

Medium High Assignee:

Sonic Zhang

Status:

Closed Fixed In Release:

N/A

Found In Release:

N/A Release:

Linux version 2.6.22.16-ADI-2

Category:

Drivers Board:

N/A

Processor:

N/A Silicon Revision:

BF526 0.0

Is this bug repeatable?:

Yes Resolution:

Out of Date

Uboot version or rev.:

1.1.6-svn440 (ADI-2007R2-pre) Toolchain version or rev.:

gcc version 4.1.2 (ADI svn)

App binary format:

N/A

Summary: bfin_5xx bug/anomaly workaround

Details:

Attempting to open() /dev/ttyBF1 fails on BF-526/BF-527 CPUs due to what seems like a hardware anomaly. The first time that bfin_serial_set_termios() is called for ttyBF1 (which occurs during the open()) the TEMT bit in the lsr register is cleared and never becomes set. That is, this loop:

do {

lsr = UART_GET_LSR(uart);

} while (!(lsr & TEMT));

never exits. Since this loop is infinite if the TEMT bit in the lsr register remains cleared and since this is within a spinlock the entire system locks up. It seems that there is a hardware anomaly that TEMT is cleared at power for UART1, since nothing has been transmitted at power up TEMT should be set; note that this situation does not occur for UART0, only UART1. Also note that this situation only occurs the first time bfin_serial_set_termios() is called. If you can get through it once the TEMT bit then correctly reflects the state of the transmit holding registers. The workaround that I have implemented and tested simply gives up on waiting for the transmit holding registers to become empty if the loop has executed a large number of times (30,000 seems to work). There is also a potential software bug in this code related to using a variable which is mapped to a hardware register within a loop without declaring the variable as volatile. In this particular case the function call within UART_GET_LSR(); *probably* precludes the compiler from making lsr a register variable but IMHO volatile should be used to ensure correctness independent of the implementation of UART_GET_LSR().

These two modifications are contained in the attached bfin_5xx.c file and should be incorporated into the released code. Look for UART1_TEMT_WORKAROUND.

Greg Semeraro

Follow-ups

--- Mike Frysinger 2008-04-24 15:06:50

dupe of [#3995] ... the fix is do delete that loop

--- Robin Getz 2008-04-24 15:16:10

Greg:

Looks like a couple problems, and some that are not

- there should be some sort of fail/timeout on the loop - agree 100%.

UART_GET_LSR is defined as:

include/asm-blackfin/mach-bf527/bfin_serial_5xx.h

/* The hardware clears the LSR bits upon read, so we need to cache

* some of the more fun bits in software so they don't get lost

* when checking the LSR in other code paths (TX).

static inline unsigned int UART_GET_LSR(struct bfin_serial_port *uart)

{

unsigned int lsr = bfin_read16(uart->port.membase + OFFSET_LSR);

uart->lsr |= (lsr & (BI|FE|PE|OE));

return lsr | uart->lsr;

}

And in include/asm-blackfin/mach-common/def_LPBlackfin.h:

#define bfin_read16(addr) ({ \

uint32_t __v; \

__asm__ __volatile__( \

NOP_PAD_ANOMALY_05000198 \

"%0 = w[%1] (z);" \

: "=d" (__v) \

: "a" (addr) \

); \

__v; })

So, I think it is Ok with respect to being volatile - do you agree?

As for what you are saying about TEMT not being set/cleared properly - it will

take a little more time to poke at. Do you only see this in the kernel, or in

U-Boot as well?

-Robin

--- Robin Getz 2008-04-24 18:23:41

Ok - this is why we need locking on bugs - the loop was already removed as Mike

indicated

Closing.

--- Greg Semeraro 2008-04-25 07:40:19

I agree that the loop should be deleted...simply from a

philosophical point of view, if the serial port is being configured/opened and

the caller didn't check that all the previous data was transmitted I think that

is just too bad for the caller. Anyway, I agree, either

remove the loop or allow it to timeout.

I do not see this in u-boot because I only use one serial port in u-boot, UART0

is the console for both. In my application I then open ttyBF1 and my

application only runs in linux. As far as I know u-boot never initializes or

uses UART1.

As for volatile....I believe that any variable which receives a

hardware register value (lsr in this case) should be volatile since

it is the variable that is being checked in the loop termination condition.

If the implementation of UART_GET_LSR() were to change or be

different then having lsr not declared as volatile would be a bug. I think it

is better to err on the side of caution when testing

a variable which is supposed to contain a hardware register value.

As I said, in this particular case, given the implementation of bfin_read16(),

the code is correct without

volatile but in my opinion it would be "better" with it.

Of course, if you remove the loop that point is moot.

All that being said, I believe that the root cause of the problem is a hardware

anomaly

where TEMT does not reflect the correct state of the transmit

holding registers at power up for UART1. I suspect that all is needed is to

poke the UART and then the bit is correct, unfortunately

with that loop as it was that can't happen.

Greg

Files

Changes

Commits

Dependencies

Duplicates

Associations

Partner Forums

Partner Libraries

[#4060] bfin_5xx bug/anomaly workaround