| Message ID | 1393551441-18041-1-git-send-email-robherring2@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 28 February 2014 01:37, Rob Herring <robherring2@gmail.com> wrote: > From: Rob Herring <rob.herring@linaro.org> > > Offset 4 is UARTRSR/UARTECR, not the UARTCR. As framing and parity errors > will never occur, we can ignore writes to this register. It's true that we'll never get framing or parity errors, but we can get break (and we ought to get overrun if we implemented it properly). This should be fairly easy to implement: * add rsr to the state struct * clear it on reset [ideally, extra patch to pull reset out of the pl011_init fn into an actual device reset method] * writes to UARTRSR clear rsr * reads from UARTRSR return rsr * in the code for read of UARTDR, copy bits [10:8] of the value we're about to return into rsr [2:1] (break/parity/framing) * for overrun, we should set the rsr bit in pl011_put_fifo if the fifo is full; however, it looks from that function as if we don't properly implement the documented behaviour for a full fifo (s2.4.3 "UART operation" of the PL011 TRM is a pretty clear description) so I'd be OK with just adding a comment to pl011_put_fifo(): /* FIXME: FIFO overrun handling of excess data and setting * overrun status bits is not correctly implemented: * see PL011 TRM s2.4.3 "UART operation". */ (Of course if you want to implement overrun properly feel free ;-)) thanks -- PMM
On Fri, Feb 28, 2014 at 4:41 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 28 February 2014 01:37, Rob Herring <robherring2@gmail.com> wrote: >> From: Rob Herring <rob.herring@linaro.org> >> >> Offset 4 is UARTRSR/UARTECR, not the UARTCR. As framing and parity errors >> will never occur, we can ignore writes to this register. > > It's true that we'll never get framing or parity errors, > but we can get break (and we ought to get overrun if we > implemented it properly). This should be fairly easy to > implement: > * add rsr to the state struct > * clear it on reset [ideally, extra patch to pull reset out > of the pl011_init fn into an actual device reset method] > * writes to UARTRSR clear rsr > * reads from UARTRSR return rsr > * in the code for read of UARTDR, copy bits [10:8] of > the value we're about to return into rsr [2:1] > (break/parity/framing) Okay, that's simple enough. > * for overrun, we should set the rsr bit in pl011_put_fifo > if the fifo is full; however, it looks from that function > as if we don't properly implement the documented behaviour > for a full fifo (s2.4.3 "UART operation" of the PL011 TRM > is a pretty clear description) so I'd be OK with just adding > a comment to pl011_put_fifo(): > /* FIXME: FIFO overrun handling of excess data and setting > * overrun status bits is not correctly implemented: > * see PL011 TRM s2.4.3 "UART operation". > */ > (Of course if you want to implement overrun properly > feel free ;-)) I don't think we can get an overrun. pl011_can_receive should prevent ever getting a 17th character. If we allowed that, then we would get overruns all the time. Effectively there is flow-control between qemu and the pl011 model since there is no baudrate to limit the receive speed. I suppose we could allow a 17th character modeling the shift register, but that seems to be rather pointless complicating of the model. We'd be better off spending time properly modelling the fifo trigger level and receive timeout. That would have some actual benefit. Rob
On 28 February 2014 13:41, Rob Herring <rob.herring@linaro.org> wrote: > On Fri, Feb 28, 2014 at 4:41 AM, Peter Maydell <peter.maydell@linaro.org> wrote: >> * for overrun, we should set the rsr bit in pl011_put_fifo >> if the fifo is full; however, it looks from that function >> as if we don't properly implement the documented behaviour >> for a full fifo (s2.4.3 "UART operation" of the PL011 TRM >> is a pretty clear description) so I'd be OK with just adding >> a comment to pl011_put_fifo(): >> /* FIXME: FIFO overrun handling of excess data and setting >> * overrun status bits is not correctly implemented: >> * see PL011 TRM s2.4.3 "UART operation". >> */ >> (Of course if you want to implement overrun properly >> feel free ;-)) > > I don't think we can get an overrun. pl011_can_receive should prevent > ever getting a 17th character. If we allowed that, then we would get > overruns all the time. Effectively there is flow-control between qemu > and the pl011 model since there is no baudrate to limit the receive > speed. Yes, you're right; I agree there's no point modelling overrun. thanks -- PMM
diff --git a/hw/char/pl011.c b/hw/char/pl011.c index a8ae6f4..8ced7cd 100644 --- a/hw/char/pl011.c +++ b/hw/char/pl011.c @@ -146,8 +146,8 @@ static void pl011_write(void *opaque, hwaddr offset, s->int_level |= PL011_INT_TX; pl011_update(s); break; - case 1: /* UARTCR */ - s->cr = value; + case 1: /* UARTRSR/UARTECR */ + /* Writes to Rx Status / Error Clear register are ignored. */ break; case 6: /* UARTFR */ /* Writes to Flag register are ignored. */