| Message ID | 1394821351-21477-2-git-send-email-robherring2@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 14 March 2014 18:22, Rob Herring <robherring2@gmail.com> wrote: > From: Rob Herring <rob.herring@linaro.org> > > Intermittent issues have been seen where no serial input occurs. It > appears the pl011 gets in a state where the rx interrupt never fires > because the rx interrupt only asserts when crossing the fifo trigger > level. The fifo state appears to get out of sync when the pl011 is > re-configured. This combined with the rx timeout interrupt not being > modeled results in no more rx interrupts. This sounds like the bug is that we're not asserting the rx interrupt when we should, which is not what this patch is trying to change. Among other things, this patch doesn't actually cause a write to this register to make us recalculate and re-send the interrupt state, because there is no call to pl011_update() in this code path. > Disabling the fifo is the recommended way to clear the tx fifo in the > TRM (section 3.3.8). The behavior in this case for the rx fifo is > undefined in the TRM, but having fifo contents to be maintained during > configuration changes is not likely expected behavior. Reseting the > fifo state when the fifo size is changed is the simplest solution. > > Signed-off-by: Rob Herring <rob.herring@linaro.org> > --- > hw/char/pl011.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/hw/char/pl011.c b/hw/char/pl011.c > index a8ae6f4..f0c3fa3 100644 > --- a/hw/char/pl011.c > +++ b/hw/char/pl011.c > @@ -162,6 +162,10 @@ static void pl011_write(void *opaque, hwaddr offset, > s->fbrd = value; > break; > case 11: /* UARTLCR_H */ > + if (!((s->lcr ^ value) & 0x10)) { > + s->read_count = 0; > + s->read_pos = 0; > + } Am I misreading this, or do we clear the fifo on every write to this register where the fifo-enable bit is not changed? > s->lcr = value; > pl011_set_read_trigger(s); > break; > -- > 1.8.3.2 > I really think this patch is just covering over the actual problem and we need to identify and fix the actual bug. thanks -- PMM
On 16 March 2014 16:16, Peter Maydell <peter.maydell@linaro.org> wrote: > On 14 March 2014 18:22, Rob Herring <robherring2@gmail.com> wrote: >> From: Rob Herring <rob.herring@linaro.org> >> >> Intermittent issues have been seen where no serial input occurs. It >> appears the pl011 gets in a state where the rx interrupt never fires >> because the rx interrupt only asserts when crossing the fifo trigger >> level. The fifo state appears to get out of sync when the pl011 is >> re-configured. This combined with the rx timeout interrupt not being >> modeled results in no more rx interrupts. > > This sounds like the bug is that we're not asserting the rx interrupt > when we should, which is not what this patch is trying to change. > > Among other things, this patch doesn't actually cause a write to > this register to make us recalculate and re-send the interrupt state, > because there is no call to pl011_update() in this code path. > >> Disabling the fifo is the recommended way to clear the tx fifo in the >> TRM (section 3.3.8). The behavior in this case for the rx fifo is >> undefined in the TRM, but having fifo contents to be maintained during >> configuration changes is not likely expected behavior. Reseting the >> fifo state when the fifo size is changed is the simplest solution. >> >> Signed-off-by: Rob Herring <rob.herring@linaro.org> >> --- >> hw/char/pl011.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/hw/char/pl011.c b/hw/char/pl011.c >> index a8ae6f4..f0c3fa3 100644 >> --- a/hw/char/pl011.c >> +++ b/hw/char/pl011.c >> @@ -162,6 +162,10 @@ static void pl011_write(void *opaque, hwaddr offset, >> s->fbrd = value; >> break; >> case 11: /* UARTLCR_H */ >> + if (!((s->lcr ^ value) & 0x10)) { >> + s->read_count = 0; >> + s->read_pos = 0; >> + } > > Am I misreading this, or do we clear the fifo on every write > to this register where the fifo-enable bit is not changed? > I really think this patch is just covering over the actual problem > and we need to identify and fix the actual bug. That said, I'm fairly sure that read_count = read_pos = 0 is indeed the correct behaviour when the FIFO is disabled. thanks -- PMM
diff --git a/hw/char/pl011.c b/hw/char/pl011.c index a8ae6f4..f0c3fa3 100644 --- a/hw/char/pl011.c +++ b/hw/char/pl011.c @@ -162,6 +162,10 @@ static void pl011_write(void *opaque, hwaddr offset, s->fbrd = value; break; case 11: /* UARTLCR_H */ + if (!((s->lcr ^ value) & 0x10)) { + s->read_count = 0; + s->read_pos = 0; + } s->lcr = value; pl011_set_read_trigger(s); break;