This is another workaroud of 'https://lkml.org/lkml/2012/1/17/104'
with additional analysis.Bootloader can transfer control to kernel and
there are some pending interrupts. In this case, RXFE of the flag
register is set by clearing FEN(LCRH) even if rx data remains in the
fifo. It seems that the fifo's status is initiailized. Interrupt
handler can not get any data from data register because of the below
break condtion.
pl011_fifo_to_tty
while (max_count--) {
if (status & UART01x_FR_RXFE)
break;
Then, Rx interrupt is never cleared. cpu is looping in ISR. System is
hang. If we don't guarantee that no interrupt is pended until fifo is
disabled by calling 'writew(0, uap->port.membase + uap->lcrh_rx)',
this misbehave of the interrupt handelr can be occurred. So, All
pending interrupts should be cleared just after fifo is disabled under
the protection from interrupt. Also,'clear error interrupts' routine
can be removed becuase all interrupts are cleared before.
Signed-off-by: Chanho Min <[email protected]>
---
drivers/tty/serial/amba-pl011.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 6800f5f..8b5a824 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -1397,7 +1397,11 @@ static int pl011_startup(struct uart_port *port)
writew(cr, uap->port.membase + UART011_CR);
writew(0, uap->port.membase + UART011_FBRD);
writew(1, uap->port.membase + UART011_IBRD);
+ spin_lock_irq(&uap->port.lock);
writew(0, uap->port.membase + uap->lcrh_rx);
+ /* Clear all pending interrupts. */
+ writew(0xffff, uap->port.membase + UART011_ICR);
+ spin_unlock_irq(&uap->port.lock);
if (uap->lcrh_tx != uap->lcrh_rx) {
int i;
/*
@@ -1417,10 +1421,6 @@ static int pl011_startup(struct uart_port *port)
cr |= UART01x_CR_UARTEN | UART011_CR_RXE | UART011_CR_TXE;
writew(cr, uap->port.membase + UART011_CR);
- /* Clear pending error interrupts */
- writew(UART011_OEIS | UART011_BEIS | UART011_PEIS | UART011_FEIS,
- uap->port.membase + UART011_ICR);
-
/*
* initialise the old status of the modem signals
*/
--
1.7.0.4
On Mon, Feb 27, 2012 at 10:30 AM, Chanho Min <[email protected]> wrote:
> This is another workaroud of ?'https://lkml.org/lkml/2012/1/17/104'
> with additional analysis.Bootloader can transfer control to kernel and
> there are some pending interrupts. In this case, RXFE of the flag
> register is set by clearing FEN(LCRH) even if rx data remains in the
> fifo. It seems that the fifo's status is initiailized. Interrupt
> handler can not get any data from data register because of the below
> break condtion.
>
> pl011_fifo_to_tty
> ?while (max_count--) {
> ? if (status & UART01x_FR_RXFE)
> ? ? ? ?break;
>
> Then, Rx interrupt is never cleared. cpu is looping in ISR. System is
> hang. If we don't guarantee that no interrupt is pended until fifo is
> disabled by calling 'writew(0, uap->port.membase + uap->lcrh_rx)',
> this misbehave of the interrupt handelr can be occurred. So, All
> pending interrupts should be cleared just after fifo is disabled under
> the protection from interrupt. Also,'clear error interrupts' routine
> can be removed becuase all interrupts are cleared before.
>
> Signed-off-by: Chanho Min <[email protected]>
Looks correct to me.
Acked-by: Linus Walleij <[email protected]>
Thanks,
Linus Walleij
On Mon, Feb 27, 2012 at 06:30:20PM +0900, Chanho Min wrote:
> This is another workaroud of 'https://lkml.org/lkml/2012/1/17/104'
> with additional analysis.Bootloader can transfer control to kernel and
> there are some pending interrupts. In this case, RXFE of the flag
> register is set by clearing FEN(LCRH) even if rx data remains in the
> fifo. It seems that the fifo's status is initiailized. Interrupt
> handler can not get any data from data register because of the below
> break condtion.
>
> pl011_fifo_to_tty
> while (max_count--) {
> if (status & UART01x_FR_RXFE)
> break;
>
> Then, Rx interrupt is never cleared. cpu is looping in ISR. System is
> hang. If we don't guarantee that no interrupt is pended until fifo is
> disabled by calling 'writew(0, uap->port.membase + uap->lcrh_rx)',
> this misbehave of the interrupt handelr can be occurred. So, All
> pending interrupts should be cleared just after fifo is disabled under
> the protection from interrupt. Also,'clear error interrupts' routine
> can be removed becuase all interrupts are cleared before.
I'd much prefer to only clear those interrupts which actually need to be
cleared at this point. So, I'd suggest this approach instead:
diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 6800f5f..a13a825 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -1381,6 +1381,10 @@ static int pl011_startup(struct uart_port *port)
uap->port.uartclk = clk_get_rate(uap->clk);
+ /* Clear pending error and receive interrupts */
+ writew(UART011_OEIS | UART011_BEIS | UART011_PEIS | UART011_FEIS |
+ UART011_RTIS | UART011_RXIS, uap->port.membase + UART011_ICR);
+
/*
* Allocate the IRQ
*/
@@ -1417,10 +1421,6 @@ static int pl011_startup(struct uart_port *port)
cr |= UART01x_CR_UARTEN | UART011_CR_RXE | UART011_CR_TXE;
writew(cr, uap->port.membase + UART011_CR);
- /* Clear pending error interrupts */
- writew(UART011_OEIS | UART011_BEIS | UART011_PEIS | UART011_FEIS,
- uap->port.membase + UART011_ICR);
-
/*
* initialise the old status of the modem signals
*/
On Mon, Feb 27, 2012 at 10:48:58AM +0000, Russell King - ARM Linux wrote:
> I'd much prefer to only clear those interrupts which actually need to be
> cleared at this point. So, I'd suggest this approach instead:
Thinking about this a little more, we definitely want to mask and clear
interrupts at probe time as well:
diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 6800f5f..6b781bd 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -1381,6 +1381,10 @@ static int pl011_startup(struct uart_port *port)
uap->port.uartclk = clk_get_rate(uap->clk);
+ /* Clear pending error and receive interrupts */
+ writew(UART011_OEIS | UART011_BEIS | UART011_PEIS | UART011_FEIS |
+ UART011_RTIS | UART011_RXIS, uap->port.membase + UART011_ICR);
+
/*
* Allocate the IRQ
*/
@@ -1417,10 +1421,6 @@ static int pl011_startup(struct uart_port *port)
cr |= UART01x_CR_UARTEN | UART011_CR_RXE | UART011_CR_TXE;
writew(cr, uap->port.membase + UART011_CR);
- /* Clear pending error interrupts */
- writew(UART011_OEIS | UART011_BEIS | UART011_PEIS | UART011_FEIS,
- uap->port.membase + UART011_ICR);
-
/*
* initialise the old status of the modem signals
*/
@@ -1927,6 +1927,10 @@ static int pl011_probe(struct amba_device *dev, const struct amba_id *id)
goto unmap;
}
+ /* Ensure interrupts from this UART are masked and cleared */
+ writew(0, uap->port.membase + UART011_IMSC);
+ writew(0xffff, uap->port.membase + UART011_ICR);
+
uap->vendor = vendor;
uap->lcrh_rx = vendor->lcrh_rx;
uap->lcrh_tx = vendor->lcrh_tx;
On Mon, Feb 27, 2012 at 12:02 PM, Russell King - ARM Linux
<[email protected]> wrote:
> On Mon, Feb 27, 2012 at 10:48:58AM +0000, Russell King - ARM Linux wrote:
>> I'd much prefer to only clear those interrupts which actually need to be
>> cleared at this point. ?So, I'd suggest this approach instead:
>
> Thinking about this a little more, we definitely want to mask and clear
> interrupts at probe time as well:
Acked-by: Linus Walleij <[email protected]>
On the RMK-improved patch.
Thanks,
Linus Walleij
On Mon, Feb 27, 2012 at 8:02 PM, Russell King - ARM Linux
<[email protected]> wrote:
> On Mon, Feb 27, 2012 at 10:48:58AM +0000, Russell King - ARM Linux wrote:
>> I'd much prefer to only clear those interrupts which actually need to be
>> cleared at this point. ?So, I'd suggest this approach instead:
>
> Thinking about this a little more, we definitely want to mask and clear
> interrupts at probe time as well:
>
I'm not satisfied with this completely. RIS has some pending
interrupts even if interrupts are masked/disabled in IMSC. If your
patch is applied, interrupt can be pended as bellows and RXFE of the
flag register is set as well.
writew(UART011_OEIS | UART011_BEIS | UART011_PEIS | UART011_FEIS |
UART011_RTIS | UART011_RXIS, uap->port.membase + UART011_ICR);
...
Interrupt is occured and pended in RIS
..
writew(uap->im, uap->port.membase + UART011_IMSC);
Root cause is that Rx interrupt set but Rx fifo is empty. If we just
remove the sentence for clearing LCRH, nothing happens and interrupt
handler don't this misbehave. Also I don't fully understand why we
need to clear interrupts at probe time. If we prefer to only clear
those interrupts which actually need to be cleared, this is the
improved patch.
Thanks,
Signed-off-by: Chanho Min <[email protected]>
---
drivers/tty/serial/amba-pl011.c | 9 +++++----
1 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 6800f5f..96d1828 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -1397,7 +1397,12 @@ static int pl011_startup(struct uart_port *port)
writew(cr, uap->port.membase + UART011_CR);
writew(0, uap->port.membase + UART011_FBRD);
writew(1, uap->port.membase + UART011_IBRD);
+ spin_lock_irq(&uap->port.lock);
writew(0, uap->port.membase + uap->lcrh_rx);
+ /* Clear pending error and receive interrupts */
+ writew(UART011_OEIS | UART011_BEIS | UART011_PEIS | UART011_FEIS |
+ UART011_RTIS | UART011_RXIS, uap->port.membase + UART011_ICR);
+ spin_unlock_irq(&uap->port.lock);
if (uap->lcrh_tx != uap->lcrh_rx) {
int i;
/*
@@ -1417,10 +1422,6 @@ static int pl011_startup(struct uart_port *port)
cr |= UART01x_CR_UARTEN | UART011_CR_RXE | UART011_CR_TXE;
writew(cr, uap->port.membase + UART011_CR);
- /* Clear pending error interrupts */
- writew(UART011_OEIS | UART011_BEIS | UART011_PEIS | UART011_FEIS,
- uap->port.membase + UART011_ICR);
-
/*
* initialise the old status of the modem signals
*/
--
1.7.0.4
On Tue, Feb 28, 2012 at 10:35:30AM +0900, Chanho Min wrote:
> On Mon, Feb 27, 2012 at 8:02 PM, Russell King - ARM Linux
> <[email protected]> wrote:
> > On Mon, Feb 27, 2012 at 10:48:58AM +0000, Russell King - ARM Linux wrote:
> >> I'd much prefer to only clear those interrupts which actually need to be
> >> cleared at this point. ?So, I'd suggest this approach instead:
> >
> > Thinking about this a little more, we definitely want to mask and clear
> > interrupts at probe time as well:
> >
> I'm not satisfied with this completely. RIS has some pending
> interrupts even if interrupts are masked/disabled in IMSC. If your
> patch is applied, interrupt can be pended as bellows and RXFE of the
> flag register is set as well.
RXFE _will_ be set. Think about it - RXFE means Receive Fifo Empty.
If the receive fifo is empty, it _will_ be set.
And RIS is the _Raw_ interrupt status. That's the status _before_ the
mask is acted upon.
> writew(UART011_OEIS | UART011_BEIS | UART011_PEIS | UART011_FEIS |
> UART011_RTIS | UART011_RXIS, uap->port.membase + UART011_ICR);
> ...
> Interrupt is occured and pended in RIS
But it won't be delivered because the mask register is zero.
> ..
> writew(uap->im, uap->port.membase + UART011_IMSC);
>
> Root cause is that Rx interrupt set but Rx fifo is empty. If we just
> remove the sentence for clearing LCRH, nothing happens and interrupt
> handler don't this misbehave.
No.
> RXFE _will_ be set. ?Think about it - RXFE means Receive Fifo Empty.
> If the receive fifo is empty, it _will_ be set.
I know meaning of the RXFE. I also don't understand why RXFE is set by
clearing FEN. We checked this by bellow debug codes.
fr_before = readw(uap->port.membase + UART01x_FR);
writew(0, uap->port.membase + uap->lcrh_rx);
fr_after = readw(uap->port.membase + UART01x_FR);
If rx interrupt is ocurred before, fr_after becomes 0x90 but fr_before is 0x80.
> And RIS is the _Raw_ interrupt status. ?That's the status _before_ the
> mask is acted upon.
>
> But it won't be delivered because the mask register is zero.
It can be delivered just after mask register is set to 1.
>> Root cause is that Rx interrupt set but Rx fifo is empty. If we just
>> remove the sentence for clearing LCRH, nothing happens and interrupt
>> handler don't this misbehave.
>
> No.
When we just removed the sentence for clearing LCRH, this hangup doesn't happen.
On Tue, Feb 28, 2012 at 06:16:10PM +0900, Chanho Min wrote:
> > RXFE _will_ be set. ?Think about it - RXFE means Receive Fifo Empty.
> > If the receive fifo is empty, it _will_ be set.
> I know meaning of the RXFE. I also don't understand why RXFE is set by
> clearing FEN. We checked this by bellow debug codes.
>
> fr_before = readw(uap->port.membase + UART01x_FR);
> writew(0, uap->port.membase + uap->lcrh_rx);
> fr_after = readw(uap->port.membase + UART01x_FR);
>
> If rx interrupt is ocurred before, fr_after becomes 0x90 but fr_before is 0x80.
Because the flags are manipulated to give the illusion of a one byte
FIFO, as stated in the TRM.
> > And RIS is the _Raw_ interrupt status. ?That's the status _before_ the
> > mask is acted upon.
> >
> > But it won't be delivered because the mask register is zero.
> It can be delivered just after mask register is set to 1.
And we don't set the mask register to 1 until later.
> >> Root cause is that Rx interrupt set but Rx fifo is empty. If we just
> >> remove the sentence for clearing LCRH, nothing happens and interrupt
> >> handler don't this misbehave.
> >
> > No.
> When we just removed the sentence for clearing LCRH, this hangup doesn't
> happen.
But we want to do the transmit interrupt provocation with the FIFO disabled.
> Because the flags are manipulated to give the illusion of a one byte
> FIFO, as stated in the TRM.
Yes. It is the problem that rx interrupt is pended with this status as
I mentioned.
> And we don't set the mask register to 1 until later.
In the last part of startup, set to 1. Interrupt can be occurred just after it.
uap->im = UART011_RTIM;
if (!pl011_dma_rx_running(uap))
uap->im |= UART011_RXIM;
writew(uap->im, uap->port.membase + UART011_IMSC);
> But we want to do the transmit interrupt provocation with the FIFO disabled.
I know. It's test only.
On Tue, Feb 28, 2012 at 06:46:12PM +0900, Chanho Min wrote:
> > Because the flags are manipulated to give the illusion of a one byte
> > FIFO, as stated in the TRM.
> Yes. It is the problem that rx interrupt is pended with this status as
> I mentioned.
Which is why my patch explicitly clears the receive interrupt status
before requesting the interrupt. Have you read my patch?
> > And we don't set the mask register to 1 until later.
> In the last part of startup, set to 1. Interrupt can be occurred just
> after it.
>
> uap->im = UART011_RTIM;
> if (!pl011_dma_rx_running(uap))
> uap->im |= UART011_RXIM;
> writew(uap->im, uap->port.membase + UART011_IMSC);
>
> > But we want to do the transmit interrupt provocation with the FIFO disabled.
> I know. It's test only.
Wrong, it's fundamental to the UARTs operation.
> Which is why my patch explicitly clears the receive interrupt status
> before requesting the interrupt. ?Have you read my patch?
This is the hang-up scenario with your patch.
pl011_startup(struct uart_port *port)
uap->port.uartclk = clk_get_rate(uap->clk);
/* Clear pending error and receive interrupts */
writew(UART011_OEIS | UART011_BEIS | UART011_PEIS | UART011_FEIS |
UART011_RTIS | UART011_RXIS, uap->port.membase + UART011_ICR);
...
1. RX Interrupt is occurred after interrupt is cleared. Even if
interrupt is masked/disabled before(or probe time), RIS's Rx interrupt
is set to 1. Of course, masked status is zero.
...
2. RXFE of flag register is zero and fifo is not empty before LCRH is cleared.
writew(0, uap->port.membase + uap->lcrh_rx);
3. RXFE of flag register is changed to '1' after LCRH is cleared. but
the fifo is not actually empty.
...
4. Finally, We enable interrupts.
spin_lock_irq(&uap->port.lock);
uap->im = UART011_RTIM;
if (!pl011_dma_rx_running(uap))
uap->im |= UART011_RXIM;
writew(uap->im, uap->port.membase + UART011_IMSC);
spin_unlock_irq(&uap->port.lock);
5. The RIS's field which is enabled by IMSC is reflected to MIS as
soon as the interrupt enable. (We checked this on our ARM platform )
6. IRQ context is started. pl011_fifo_to_tty is called by pl011_int.
static int pl011_fifo_to_tty(struct uart_amba_port *uap)
...
while (max_count--) {
status = readw(uap->port.membase + UART01x_FR);
if (status & UART01x_FR_RXFE)
break;
...
7. pl011_fifo_to_tty can't read any data from DR because of the break
condition for RXFE. Rx interrupt can't be cleared. cpu is looping in
irq context.
This is why we need to be cleared just after LCRH is cleared not
before irq_request. Let's get back to my patch. Even if data is
received before or after interrupt is cleared, flag register will show
actual fifo status. Interrupt handler runs normally after the uart
operation is started up by enabling interrupt.
+ spin_lock_irq(&uap->port.lock);
writew(0, uap->port.membase + uap->lcrh_rx);
+ /* Clear pending error and receive interrupts */
+ writew(UART011_OEIS | UART011_BEIS | UART011_PEIS | UART011_FEIS |
+ UART011_RTIS | UART011_RXIS, uap->port.membase + UART011_ICR);
+ spin_unlock_irq(&uap->port.lock);
Thanks,
Chanho Min