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

[Bhupinder Thakur <bhupinder.thakur@xxxxxxxxxx>]

On 9 October 2017 at 22:48, Andre Przywara <andre.przywara@xxxxxxx> wrote:
> Hi,
>
> can you please re-break the commit message to fit into 72 characters?
> git show looks rather ugly as it is now.
ok.
>
> 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.
>
ok.

>>      }
>> +    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.
Thanks. I will fix it.

>
>>           * 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.
In the above if block, it sets the RXI bit in uartris. In that case
also we need to trigger the interrupt. But I will fix the same thing
in the other function vpl011_read_data().

Regards,
Bhupinder

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