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

[Bhupinder Thakur <bhupinder.thakur@linaro.org>]

Hi Andre,


On 24 October 2017 at 16:57, Andre Przywara <andre.przywara@linaro.org> wrote:
> 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@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.
>>
>> 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.
It is fine with me. Thanks.

Regards,
Bhupinder

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