Message ID | CAHgzvFjyvHOSYdGyi8_ug+tO1rE_0TZXVQwc9g=NnZJxQ2Dtdg@mail.gmail.com |
---|---|
State | New |
Headers | show |
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 <andrew.gacek@gmail.com> 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 <andrew.gacek@gmail.com> > Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > --- > 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 > <edgar.iglesias@gmail.com> 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 <andrew.gacek@gmail.com> >>> > --- >>> > 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 <edgar.iglesias@xilinx.com> >> >> Best regards, >> Edgar
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)