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

[Andre Przywara <andre.przywara@linaro.org>]

Hi,

On 18/10/17 17:32, Bhupinder Thakur wrote:
> Hi Andre,
> 
> I verified this patch on qualcomm platform. It is working fine.
> 
> On 18 October 2017 at 19:11, Andre Przywara <andre.przywara@arm.com> 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@arm.com>
>> ---
>> 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.

Are you sure? My understanding is that the semantics of the return value
of xencons_queued() differs between intf and outf:
- For intf, Xen fills that buffer with incoming characters. The
watermark is assumed to be (FIFO / 2), which translates into 16
characters. Now for the SBSA vUART RX side that means: "Assert the RX
interrupt if there is only room for 16 (or less) characters in the FIFO
(read: intf buffer in our case). Since we (ab)use the Xen buffer for the
FIFO, this means we warn if the number of queued characters exceeds
(buffersize - 16).
- For outf, the UART emulation fills the buffer. The SBSA vUART TX side
demands that the TX interrupt is asserted if the fill level of the
transmit FIFO is less than or equal to the 16 characters, which means:
number of queued characters is less than 16.

I think the key point is that our trigger level isn't symmetrical here,
since we have to emulate the architected 32-byte FIFO semantics for the
driver, but have a (secretly) much larger "FIFO" internally.

Do you agree with this reasoning and do I have a thinko here? Could well
be I am seriously misguided here.

Cheers,
Andre

>>      }
>>      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@lists.xen.org
https://lists.xen.org/xen-devel