[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-arm] [Qemu-trivial] [PATCH] cadence_uart: Check if receiver ti
From: |
Andrew Gacek |
Subject: |
Re: [Qemu-arm] [Qemu-trivial] [PATCH] cadence_uart: Check if receiver timeout counter is disabled |
Date: |
Thu, 8 Dec 2016 05:25:26 -0600 |
Laurent, thanks for CC'ing Xilinx Zynq maintainers. I read that part
of the submission guidelines, then completely forgot to do it.
Edgar, thanks for the comments. I wasn't sure if I needed to call that
function or not. Also, one other discrepancy with the technical
reference manual is that writing to R_RTOR should mask off all but the
lower 8 bits. I'm not sure how important that actually is (since the
timer isn't really implemented in qemu), but I thought I'd mention it.
For my use case, the current patch fixes the issue we were having.
-Andrew
On Thu, Dec 8, 2016 at 5:21 AM, Andrew Gacek <address@hidden> wrote:
> When register Rcvr_timeout_reg0 (R_RTOR in cadence_uart.c) is set to
> 0, the receiver timeout counter should be disabled. See page 1801 of
> "Zynq-7000 AP SoC Technical Reference Manual". This commit adds a
> such a check before setting the receive timeout interrupt.
>
> Signed-off-by: Andrew Gacek <address@hidden>
> Reviewed-by: Edgar E. Iglesias <address@hidden>
> ---
> hw/char/cadence_uart.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
> index 0215d65..378630d 100644
> --- a/hw/char/cadence_uart.c
> +++ b/hw/char/cadence_uart.c
> @@ -138,9 +138,11 @@ static void fifo_trigger_update(void *opaque)
> {
> CadenceUARTState *s = opaque;
>
> - s->r[R_CISR] |= UART_INTR_TIMEOUT;
> + if (s->r[R_RTOR]) {
> + s->r[R_CISR] |= UART_INTR_TIMEOUT;
>
> - uart_update_status(s);
> + uart_update_status(s);
> + }
> }
>
> static void uart_rx_reset(CadenceUARTState *s)
> --
> 2.7.4
>
> On Thu, Dec 8, 2016 at 4:42 AM, Edgar E. Iglesias
> <address@hidden> wrote:
>> On Thu, Dec 08, 2016 at 08:50:40AM +0100, Laurent Vivier wrote:
>>> I CC: Xilinx Zynq Maintainers.
>>>
>>> Laurent
>>
>> Thanks
>>
>>>
>>> On 07/12/2016 22:12, Andrew Gacek wrote:
>>> > When register Rcvr_timeout_reg0 (R_RTOR in cadence_uart.c) is set to
>>> > 0, the receiver timeout counter should be disabled. See page 1801 of
>>> > "Zynq-7000 AP SoC Technical Reference Manual". This commit adds a
>>> > such a check before setting the receive timeout interrupt.
>>
>> We could also try to disable the timer when rtor is zero but I think
>> that exposes a bunch of corner cases that would complicate the model a bit.
>> So IMO, this patch is good.
>>
>>
>>> > Signed-off-by: Andrew Gacek <address@hidden>
>>> > ---
>>> > hw/char/cadence_uart.c | 4 +++-
>>> > 1 file changed, 3 insertions(+), 1 deletion(-)
>>> >
>>> > diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
>>> > index 0215d65..54194b1 100644
>>> > --- a/hw/char/cadence_uart.c
>>> > +++ b/hw/char/cadence_uart.c
>>> > @@ -138,7 +138,9 @@ static void fifo_trigger_update(void *opaque)
>>> > {
>>> > CadenceUARTState *s = opaque;
>>> >
>>> > - s->r[R_CISR] |= UART_INTR_TIMEOUT;
>>> > + if (s->r[R_RTOR]) {
>>> > + s->r[R_CISR] |= UART_INTR_TIMEOUT;
>>> > + }
>>> >
>>> > uart_update_status(s);
>>
>> Since you are not modifying the IRQ state when the timeout is disabled, you
>> can avoid calling uart_update_status too (because it will only end up
>> recomputing the same state).
>>
>> With that fix:
>> Reviewed-by: Edgar E. Iglesias <address@hidden>
>>
>> Best regards,
>> Edgar