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

[Andre Przywara <andre.przywara@xxxxxxxxxx>]

Hi,

On 24/10/17 12:00, Julien Grall wrote:
> Hi,
> 
> On 23/10/2017 17:01, Andre Przywara 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.
> 
> xencons_queued calculates how many bytes are currently on the ring. So I
> think your description makes sense.
> 
> With (fifo_level < (SBSA_UART_FIFO_SIZE / 2)), you would only clear it
> when the ring has less than 16 bytes queued.
> 
> I have a few requests on those patches for the resender:
>     - Please introduce a define for SBSA_UART_FIFO_SIZE / 2 and use it
> everywhere.
>     - Please add a bit more documentation on top of the checks in
> vpl011_read_data function. The checks in vpl011_write_data looks
> well-documented.

I am just at rewording the commit message and was planning on re-sending
the (merged) patches later today (keeping Bhupinder's authorship, of
course).

I hope that Bhupinder doesn't mind or this doesn't clash with any of his
plans.

Cheers,
Andre.


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