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

[Bhupinder Thakur]

Hi,

On 23 October 2017 at 21:31, 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  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 
>>> ---
>>> 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@lists.xen.org
https://lists.xen.org/xen-devel

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

[Bhupinder Thakur]

Hi Andre,


On 24 October 2017 at 16:57, Andre Przywara  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 
 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 
> ---
> 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

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

[Andre Przywara]

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

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

[Julien Grall]

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  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 
---
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.


Cheers,

--
Julien Grall

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

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

[Andre Przywara]

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  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 
>> ---
>> 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
>> - * 

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

[Bhupinder Thakur]

Hi Andre,

I verified this patch on qualcomm platform. It is working fine.

On 18 October 2017 at 19:11, Andre Przywara  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 
> ---
> 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.

>  }
>  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 |= 

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

[Andre Przywara]

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 
---
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);
 }
 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;
-}
 
-/* 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


___
Xen-devel mailing list