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

[Bhupinder Thakur <bhupinder.thakur@xxxxxxxxxx>]

Hi,

On 23 October 2017 at 21:31, Andre Przywara <andre.przywara@xxxxxxxxxx> wrote:
> 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@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.
>
> 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.
>
ok. I agree with the description as it will expose the same behavior
to the driver as it would be there for a real UART where only FIFO/2
space is left.

Regards,
Bhupinder

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