Re: [Xen-devel] [PATCH RFC] ARM: vPL011: use receive timeout interrupt

[Bhupinder Thakur <bhupinder.thakur@xxxxxxxxxx>]

Hi Andre,

I verified this patch on qualcomm platform. It is working fine.

On 18 October 2017 at 19:11, Andre Przywara <andre.przywara@xxxxxxx> wrote:
> Instead of asserting the receive interrupt (RXI) on the first character
> in the FIFO, lets (ab)use the receive timeout interrupt (RTI) for that
> purpose. That seems to be closer to the spec and what hardware does.
> Improve the readability of vpl011_data_avail() on the way.
>
> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> ---
> Hi,
>
> this one is the approach I mentioned in the email earlier today.
> It goes on top of Bhupinders v12 27/27, but should eventually be merged
> into this one once we agreed on the subject. I just carved it out here
> for clarity to make it clearer what has been changed.
> Would be good if someone could test it.
>
> Cheers,
> Andre.
>  xen/arch/arm/vpl011.c | 61 
> ++++++++++++++++++++++++---------------------------
>  1 file changed, 29 insertions(+), 32 deletions(-)
>
> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
> index adf1711571..ae18bddd81 100644
> --- a/xen/arch/arm/vpl011.c
> +++ b/xen/arch/arm/vpl011.c
> @@ -105,9 +105,13 @@ static uint8_t vpl011_read_data(struct domain *d)
>          if ( fifo_level == 0 )
>          {
>              vpl011->uartfr |= RXFE;
> -            vpl011->uartris &= ~RXI;
> -            vpl011_update_interrupt_status(d);
> +            vpl011->uartris &= ~RTI;
>          }
> +
> +        if ( fifo_level < sizeof(intf->in) - SBSA_UART_FIFO_SIZE / 2 )
> +            vpl011->uartris &= ~RXI;
> +
> +        vpl011_update_interrupt_status(d);
I think we check if ( fifo_level < SBSA_UART_FIFO_SIZE / 2 ) which
should be a valid condition to clear the RX interrupt.

>      }
>      else
>          gprintk(XENLOG_ERR, "vpl011: Unexpected IN ring buffer empty\n");
> @@ -129,7 +133,7 @@ static void vpl011_update_tx_fifo_status(struct vpl011 
> *vpl011,
>                                           unsigned int fifo_level)
>  {
>      struct xencons_interface *intf = vpl011->ring_buf;
> -    unsigned int fifo_threshold;
> +    unsigned int fifo_threshold = sizeof(intf->out) - SBSA_UART_FIFO_SIZE/2;
>
>      BUILD_BUG_ON(sizeof (intf->out) < SBSA_UART_FIFO_SIZE);
>
> @@ -137,8 +141,6 @@ static void vpl011_update_tx_fifo_status(struct vpl011 
> *vpl011,
>       * Set the TXI bit only when there is space for fifo_size/2 bytes which
>       * is the trigger level for asserting/de-assterting the TX interrupt.
>       */
> -    fifo_threshold = sizeof(intf->out) - SBSA_UART_FIFO_SIZE/2;
> -
>      if ( fifo_level <= fifo_threshold )
>          vpl011->uartris |= TXI;
>      else
> @@ -390,35 +392,30 @@ static void vpl011_data_avail(struct domain *d)
>                                      out_cons,
>                                      sizeof(intf->out));
>
> -    /* Update the uart rx state if the buffer is not empty. */
> -    if ( in_fifo_level != 0 )
> -    {
> +    /**** Update the UART RX state ****/
> +
> +    /* Clear the FIFO_EMPTY bit if the FIFO holds at least one character. */
> +    if ( in_fifo_level > 0 )
>          vpl011->uartfr &= ~RXFE;
>
> -        if ( in_fifo_level == sizeof(intf->in) )
> -            vpl011->uartfr |= RXFF;
> +    /* Set the FIFO_FULL bit if the ring buffer is full. */
> +    if ( in_fifo_level == sizeof(intf->in) )
> +        vpl011->uartfr |= RXFF;
>
> -        /*
> -         * Currently, the RXI bit is getting set even if there is a single
> -         * byte of data in the rx fifo. Ideally, the RXI bit should be set
> -         * only if the rx fifo level reaches the threshold.
> -         *
> -         * However, since currently RX timeout interrupt is not
> -         * implemented as there is not enough clarity in the SBSA spec,
> -         * the guest may keep waiting for an interrupt to read more
> -         * data. To ensure that guest reads all the data without
> -         * any delay, the RXI interrupt is raised if there is RX data
> -         * available without checking whether fifo level has reached
> -         * the threshold.
> -         *
> -         * TBD: Once there is more clarity in the SBSA spec on whether RX
> -         * timeout interrupt needs to be implemented, the RXI interrupt
> -         * will be raised only when rx fifo level reaches the threshold.
> -         */
> +    /* The FIFO trigger level is fixed to half of the FIFO. */
> +    if ( in_fifo_level >= sizeof(intf->in) - SBSA_UART_FIFO_SIZE / 2 )
>          vpl011->uartris |= RXI;
Here also should not we check if ( in_fifo_level >=
SBSA_UART_FIFO_SIZE / 2 ) since it is a valid condition to raise the
RX interrupt?

> -    }
>
> -    /* Update the uart tx state if the buffer is not full. */
> +    /*
> +     * If the input queue is not empty, we assert the receive timeout 
> interrupt.
> +     * As we don't emulate any timing here, we ignore the actual timeout
> +     * of 32 bit periods.
> +     */
> +    if ( in_fifo_level > 0 )
> +        vpl011->uartris |= RTI;
> +
> +    /**** Update the UART TX state ****/
> +
>      if ( out_fifo_level != sizeof(intf->out) )
>      {
>          vpl011->uartfr &= ~TXFF;
> @@ -431,13 +428,13 @@ static void vpl011_data_avail(struct domain *d)
>          vpl011->uartfr &= ~BUSY;
>
>          vpl011_update_tx_fifo_status(vpl011, out_fifo_level);
> -
> -        if ( out_fifo_level == 0 )
> -            vpl011->uartfr |= TXFE;
>      }
>
>      vpl011_update_interrupt_status(d);
>
> +    if ( out_fifo_level == 0 )
> +        vpl011->uartfr |= TXFE;
> +
>      VPL011_UNLOCK(d, flags);
>  }
>
> --
> 2.14.1
>

Regards,
Bhupinder

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel