Re: [Xen-devel] [PATCH 27/27 v11] xen/arm: vpl011: Correct the logic for asserting/de-asserting SBSA UART TX interrupt

[Andre Przywara <andre.przywara@xxxxxxx>]

Hi,

can you please re-break the commit message to fit into 72 characters?
git show looks rather ugly as it is now.

On 27/09/17 07:13, Bhupinder Thakur wrote:
> This patch fixes the issue observed when pl011 patches were tested on
> the junos hardware by Andre/Julien. It was observed that when large output is
> generated such as on running 'find /', output was getting truncated 
> intermittently
> due to OUT ring buffer getting full.
> 
> This issue was due to the fact that the SBSA UART driver expects that when
> a TX interrupt is asserted then the FIFO queue should be atleast half empty 
> and
> that it can write N bytes in the FIFO, where N is half the FIFO queue size, 
> without
> the bytes getting dropped due to FIFO getting full.
> 
> The SBSA UART emulation logic was asserting the TX interrupt as soon as
> any space became available in the FIFO and the SBSA UART driver tried to write
> more data (upto 16 bytes) in the FIFO expecting that there is enough space
> available leading to dropped bytes.
> 
> The SBSA spec [1] does not specify when the TX interrupt should be asserted
> or de-asserted. Due to lack of clarity on the expected behavior, it is
> assumed for now that TX interrupt should be asserted only when the FIFO goes
> half empty.
> 
> TBD: Once the SBSA spec is updated with the expected behavior, the 
> implementation
> will be modified to align with the spec requirement.
> 
> [1] http://infocenter.arm.com/help/topic/com.arm.doc.ddi0183f/DDI0183.pdf
> 
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@xxxxxxxxxx>

Only some minor things left below, but in general this looks much better
to me now.

> ---
> CC: Julien Grall <julien.grall@xxxxxxx>
> CC: Andre Przywara <andre.przywara@xxxxxxx>
> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> 
> Changes since v8:
> - Used variables fifo_level/fifo_threshold for more clarity
> - Added a new macro SBSA_UART_FIFO_SIZE instead of using a magic number
> - Renamed ring_qsize variables to fifo_level for consistency 
> 
>  xen/arch/arm/vpl011.c        | 87 
> ++++++++++++++++++++++++++++++--------------
>  xen/include/asm-arm/vpl011.h |  2 +
>  2 files changed, 61 insertions(+), 28 deletions(-)
> 
> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
> index 36794d8..1f97261 100644
> --- a/xen/arch/arm/vpl011.c
> +++ b/xen/arch/arm/vpl011.c
> @@ -91,20 +91,24 @@ static uint8_t vpl011_read_data(struct domain *d)
>       */
>      if ( xencons_queued(in_prod, in_cons, sizeof(intf->in)) > 0 )
>      {
> +        unsigned int fifo_level;
> +
>          data = intf->in[xencons_mask(in_cons, sizeof(intf->in))];
>          in_cons += 1;
>          smp_mb();
>          intf->in_cons = in_cons;
> +
> +        fifo_level = xencons_queued(in_prod, in_cons, sizeof(intf->in));
> +
> +        if ( fifo_level == 0 )
> +        {
> +            vpl011->uartfr |= RXFE;
> +            vpl011->uartris &= ~RXI;
> +        }
>      }
>      else
>          gprintk(XENLOG_ERR, "vpl011: Unexpected IN ring buffer empty\n");
>  
> -    if ( xencons_queued(in_prod, in_cons, sizeof(intf->in)) == 0 )
> -    {
> -        vpl011->uartfr |= RXFE;
> -        vpl011->uartris &= ~RXI;
> -    }
> -
>      vpl011->uartfr &= ~RXFF;
>  
>      vpl011_update_interrupt_status(d);
> @@ -144,28 +148,41 @@ static void vpl011_write_data(struct domain *d, uint8_t 
> data)
>      if ( xencons_queued(out_prod, out_cons, sizeof(intf->out)) !=
>           sizeof (intf->out) )
>      {
> +        unsigned int fifo_level, fifo_threshold;
> +
>          intf->out[xencons_mask(out_prod, sizeof(intf->out))] = data;
>          out_prod += 1;
>          smp_wmb();
>          intf->out_prod = out_prod;
> -    }
> -    else
> -        gprintk(XENLOG_ERR, "vpl011: Unexpected OUT ring buffer full\n");
>  
> -    if ( xencons_queued(out_prod, out_cons, sizeof(intf->out)) ==
> -         sizeof (intf->out) )
> -    {
> -        vpl011->uartfr |= TXFF;
> -        vpl011->uartris &= ~TXI;
> +        fifo_level = xencons_queued(out_prod, out_cons, sizeof(intf->out));
> +
> +        if ( fifo_level == sizeof (intf->out) )
> +        {
> +            vpl011->uartfr |= TXFF;
> +
> +            /*
> +             * This bit is set only when FIFO becomes full. This ensures that
> +             * the SBSA UART driver can write the early console data as fast 
> as
> +             * possible, without waiting for the BUSY bit to get cleared 
> before
> +             * writing each byte.
> +             */
> +            vpl011->uartfr |= BUSY;
> +        }
>  
>          /*
> -         * This bit is set only when FIFO becomes full. This ensures that
> -         * the SBSA UART driver can write the early console data as fast as
> -         * possible, without waiting for the BUSY bit to get cleared before
> -         * writing each byte.
> +         * Clear the TXI bit if the fifo level exceeds fifo_size/2 mark which
> +         * is the trigger level for asserting/de-assterting the TX interrupt.
>           */
> -        vpl011->uartfr |= BUSY;
> +        fifo_threshold = sizeof (intf->out) - SBSA_UART_FIFO_SIZE/2;
> +
> +        if ( fifo_level <= fifo_threshold )
> +            vpl011->uartris |= TXI;
> +        else
> +            vpl011->uartris &= ~TXI;

I think we can move the call to vpl011_update_interrupt_status() here.
Currently it's called below, but here is the only place in this function
where the interrupt status can change.
This isn't a showstopper, but makes more sense and might come in handy
once we get level trigger vIRQ support.

>      }
> +    else
> +        gprintk(XENLOG_ERR, "vpl011: Unexpected OUT ring buffer full\n");
>  
>      vpl011->uartfr &= ~TXFE;
>  
> @@ -342,7 +359,7 @@ static void vpl011_data_avail(struct domain *d)
>      struct vpl011 *vpl011 = &d->arch.vpl011;
>      struct xencons_interface *intf = vpl011->ring_buf;
>      XENCONS_RING_IDX in_cons, in_prod, out_cons, out_prod;
> -    XENCONS_RING_IDX in_ring_qsize, out_ring_qsize;
> +    XENCONS_RING_IDX in_fifo_level, out_fifo_level;
>  
>      VPL011_LOCK(d, flags);
>  
> @@ -353,37 +370,51 @@ static void vpl011_data_avail(struct domain *d)
>  
>      smp_rmb();
>  
> -    in_ring_qsize = xencons_queued(in_prod,
> +    in_fifo_level = xencons_queued(in_prod,
>                                     in_cons,
>                                     sizeof(intf->in));
>  
> -    out_ring_qsize = xencons_queued(out_prod,
> +    out_fifo_level = xencons_queued(out_prod,
>                                      out_cons,
>                                      sizeof(intf->out));
>  
>      /* Update the uart rx state if the buffer is not empty. */
> -    if ( in_ring_qsize != 0 )
> +    if ( in_fifo_level != 0 )
>      {
>          vpl011->uartfr &= ~RXFE;
> -        if ( in_ring_qsize == sizeof(intf->in) )
> +
> +        if ( in_fifo_level == sizeof(intf->in) )
>              vpl011->uartfr |= RXFF;
> +
>          vpl011->uartris |= RXI;
>      }
>  
>      /* Update the uart tx state if the buffer is not full. */
> -    if ( out_ring_qsize != sizeof(intf->out) )
> +    if ( out_fifo_level != sizeof(intf->out) )
>      {
> +        unsigned int out_fifo_threshold;
> +
>          vpl011->uartfr &= ~TXFF;
> -        vpl011->uartris |= TXI;
>  
>          /*
> -         * Clear the BUSY bit as soon as space becomes available
> +         * Clear the BUSY bit as soon as space becomes avaliable

Looks like a newly introduced typo to me.

>           * so that the SBSA UART driver can start writing more data
>           * without any further delay.
>           */
>          vpl011->uartfr &= ~BUSY;
>  
> -        if ( out_ring_qsize == 0 )
> +        /*
> +         * 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.
> +         */
> +        out_fifo_threshold = sizeof(intf->out) - SBSA_UART_FIFO_SIZE/2;
> +
> +        if ( out_fifo_level <= out_fifo_threshold )
> +            vpl011->uartris |= TXI;
> +        else
> +            vpl011->uartris &= ~TXI;

Same here with moving the call to vpl011_update_interrupt_status().
Eventually this could become a direct call to the function setting the
line level of the vIRQ.

Cheers,
Andre.

> +
> +        if ( out_fifo_level == 0 )
>              vpl011->uartfr |= TXFE;
>      }
>  
> diff --git a/xen/include/asm-arm/vpl011.h b/xen/include/asm-arm/vpl011.h
> index 1b583da..db95ff8 100644
> --- a/xen/include/asm-arm/vpl011.h
> +++ b/xen/include/asm-arm/vpl011.h
> @@ -28,6 +28,8 @@
>  #define VPL011_LOCK(d,flags) spin_lock_irqsave(&(d)->arch.vpl011.lock, flags)
>  #define VPL011_UNLOCK(d,flags) 
> spin_unlock_irqrestore(&(d)->arch.vpl011.lock, flags)
>  
> +#define SBSA_UART_FIFO_SIZE 32
> +
>  struct vpl011 {
>      void *ring_buf;
>      struct page_info *ring_page;
> 

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