[Qemu-devel] [PATCH] pl011: fix corrupting the UARTCR value

[Qemu-devel] [PATCH] pl011: fix corrupting the UARTCR value

[Rob Herring]

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.

Signed-off-by: Rob Herring <rob.herring@linaro.org>
---
 hw/char/pl011.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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.  */
-- 
1.8.3.2

Re: [Qemu-devel] [PATCH] pl011: fix corrupting the UARTCR value

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH] pl011: fix corrupting the UARTCR value

[Rob Herring]

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

Re: [Qemu-devel] [PATCH] pl011: fix corrupting the UARTCR value

[Peter Maydell]

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