[Xen-devel,26/27,v8] xen/arm: vpl011: Correct the logic for asserting/de-asserting SBSA UART TX interrupt

This patch fixes the issue observed when pl011 patches were tested on
the junos hardware by Andre/Julien. It was observed that when large output is
generated such as on running 'find /', output was getting truncated intermittently
due to OUT ring buffer getting full.

This issue was due to the fact that the SBSA UART driver expects that when
a TX interrupt is asserted then the FIFO queue should be atleast half empty and
that it can write N bytes in the FIFO, where N is half the FIFO queue size, without
the bytes getting dropped due to FIFO getting full.

This requirement is as per section 3.4.2 of [1], which is:

-------------------------------------------------------------------------------
UARTTXINTR

If the FIFOs are enabled and the transmit FIFO reaches the programmed
trigger level. When this happens, the transmit interrupt is asserted HIGH. The
transmit interrupt is cleared by writing data to the transmit FIFO until it
becomes greater than the trigger level, or by clearing the interrupt.
-------------------------------------------------------------------------------

The SBSA UART fifo size is 32 bytes and so it expects that space for 16 bytes
should be available when TX interrupt is asserted.

The pl011 emulation logic was asserting the TX interrupt as soon as
any space became available in the FIFO and the SBSA UART driver tried to write
more data (upto 16 bytes) in the FIFO expecting that there is enough space
available.

The fix was to ensure that the TX interriupt is raised only when there
is space available for 16 bytes or more in the FIFO.

[1] http://infocenter.arm.com/help/topic/com.arm.doc.ddi0183f/DDI0183.pdf

Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
---
CC: Julien Grall <julien.grall@arm.com>
CC: Andre Przywara <andre.przywara@arm.com>
CC: Stefano Stabellini <sstabellini@kernel.org>

 xen/arch/arm/vpl011.c | 29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

Hi,

On 28/08/17 09:56, Bhupinder Thakur wrote:
> This patch fixes the issue observed when pl011 patches were tested on
> the junos hardware by Andre/Julien. It was observed that when large output is
> generated such as on running 'find /', output was getting truncated intermittently
> due to OUT ring buffer getting full.
> 
> This issue was due to the fact that the SBSA UART driver expects that when
> a TX interrupt is asserted then the FIFO queue should be atleast half empty and
> that it can write N bytes in the FIFO, where N is half the FIFO queue size, without
> the bytes getting dropped due to FIFO getting full.
> 
> This requirement is as per section 3.4.2 of [1], which is:
> 
> -------------------------------------------------------------------------------
> UARTTXINTR
> 
> If the FIFOs are enabled and the transmit FIFO reaches the programmed
> trigger level. When this happens, the transmit interrupt is asserted HIGH. The
> transmit interrupt is cleared by writing data to the transmit FIFO until it
> becomes greater than the trigger level, or by clearing the interrupt.
> -------------------------------------------------------------------------------
> 
> The SBSA UART fifo size is 32 bytes and so it expects that space for 16 bytes
> should be available when TX interrupt is asserted.
> 
> The pl011 emulation logic was asserting the TX interrupt as soon as
> any space became available in the FIFO and the SBSA UART driver tried to write
> more data (upto 16 bytes) in the FIFO expecting that there is enough space
> available.
> 
> The fix was to ensure that the TX interriupt is raised only when there
> is space available for 16 bytes or more in the FIFO.
> 
> [1] http://infocenter.arm.com/help/topic/com.arm.doc.ddi0183f/DDI0183.pdf
> 
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
> ---
> CC: Julien Grall <julien.grall@arm.com>
> CC: Andre Przywara <andre.przywara@arm.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> 
>  xen/arch/arm/vpl011.c | 29 +++++++++++++++++++++++------
>  1 file changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
> index 56d9cbe..1e72fca 100644
> --- a/xen/arch/arm/vpl011.c
> +++ b/xen/arch/arm/vpl011.c
> @@ -152,12 +152,20 @@ static void vpl011_write_data(struct domain *d, uint8_t data)
>      else
>          gprintk(XENLOG_ERR, "vpl011: Unexpected OUT ring buffer full\n");
>  
> -    if ( xencons_queued(out_prod, out_cons, sizeof(intf->out)) ==
> -         sizeof (intf->out) )
> -    {
> -        vpl011->uartfr |= TXFF;
> +    /*
> +     * Ensure that there is space for atleast 16 bytes before asserting the
> +     * TXI interrupt status bit because the SBSA UART driver may write
> +     * 16 bytes (i.e. half the SBSA UART fifo size of 32) on getting
> +     * a TX interrupt.
> +     */
> +    if ( xencons_queued(out_prod, out_cons, sizeof(intf->out)) <=
> +         (sizeof (intf->out) - 16) )
> +        vpl011->uartris |= TXI;
> +    else if ( xencons_queued(out_prod, out_cons, sizeof(intf->out)) !=
> +              sizeof (intf->out) )

Now this is really hard to read. Can't you use:

    fifo_level = xencons_queued(out_prod, out_cons, sizeof(intf->out));

Also I think you could start the patch a few lines above, where you
check for any free space in the buffer.

>          vpl011->uartris &= ~TXI;
> -    }
> +    else
> +        vpl011->uartfr |= TXFF;

And I believe we should separate the FIFO full condition from the
interrupt condition. I think it should more look like:

    vpl011->uartfr |= BUSY;
    vpl011->uartfr &= ~TXFE;

    if ( fifo_level == sizeof(intf->out) )
	vpl011->uartfr |= TXFF;

    if ( fifo_level >= sizeof(intf->out) - 16 )
	vpl011->uartris &= ~TXI;

Which is much easier to read and understand, also follows the spec
closely. The "16" should be either expressed at FIFOSIZE / 2 or
explained in a comment.

>  
>      vpl011->uartfr |= BUSY;
>  
> @@ -368,7 +376,16 @@ static void vpl011_data_avail(struct domain *d)
>      if ( out_ring_qsize != sizeof(intf->out) )
>      {
>          vpl011->uartfr &= ~TXFF;
> -        vpl011->uartris |= TXI;
> +
> +        /*
> +         * Ensure that there is space for atleast 16 bytes before asserting the
> +         * TXI interrupt status bit because the SBSA UART driver may write upto
> +         * 16 bytes (i.e. half the SBSA UART fifo size of 32) on getting
> +         * a TX interrupt.

The comment sounds a bit like this is hack, where it actually is a
totally legit spec requirement (the interrupt is asserted/deasserted
around the *trigger level*, which is half way by default and always half
for the SBSA).

Also I think the same logic/fix needs to be applied to the receiving side.

And while I see that Julien requested a follow-up patch, I believe this
should eventually be squashed into 02/27, to not have wrong code in the
repo. But can could be done at commit time, I guess.

Cheers,
Andre.

> +         */
> +        if ( out_ring_qsize <= (sizeof(intf->out) - 16) )
> +            vpl011->uartris |= TXI;
> +
>          if ( out_ring_qsize == 0 )
>          {
>              vpl011->uartfr &= ~BUSY;
>

Hi Andre,


>> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
>> index 56d9cbe..1e72fca 100644
>> --- a/xen/arch/arm/vpl011.c
>> +++ b/xen/arch/arm/vpl011.c
>> @@ -152,12 +152,20 @@ static void vpl011_write_data(struct domain *d, uint8_t data)
>>      else
>>          gprintk(XENLOG_ERR, "vpl011: Unexpected OUT ring buffer full\n");
>>
>> -    if ( xencons_queued(out_prod, out_cons, sizeof(intf->out)) ==
>> -         sizeof (intf->out) )
>> -    {
>> -        vpl011->uartfr |= TXFF;
>> +    /*
>> +     * Ensure that there is space for atleast 16 bytes before asserting the
>> +     * TXI interrupt status bit because the SBSA UART driver may write
>> +     * 16 bytes (i.e. half the SBSA UART fifo size of 32) on getting
>> +     * a TX interrupt.
>> +     */
>> +    if ( xencons_queued(out_prod, out_cons, sizeof(intf->out)) <=
>> +         (sizeof (intf->out) - 16) )
>> +        vpl011->uartris |= TXI;
>> +    else if ( xencons_queued(out_prod, out_cons, sizeof(intf->out)) !=
>> +              sizeof (intf->out) )
>
> Now this is really hard to read. Can't you use:
>
>     fifo_level = xencons_queued(out_prod, out_cons, sizeof(intf->out));

ok.

>
> Also I think you could start the patch a few lines above, where you
> check for any free space in the buffer.

ok. I will move the logic inside the case when there is space in the buffer.

>
>>          vpl011->uartris &= ~TXI;
>> -    }
>> +    else
>> +        vpl011->uartfr |= TXFF;
>
> And I believe we should separate the FIFO full condition from the
> interrupt condition. I think it should more look like:
>
>     vpl011->uartfr |= BUSY;
>     vpl011->uartfr &= ~TXFE;
>
>     if ( fifo_level == sizeof(intf->out) )
>         vpl011->uartfr |= TXFF;
>
>     if ( fifo_level >= sizeof(intf->out) - 16 )
>         vpl011->uartris &= ~TXI;
>
> Which is much easier to read and understand, also follows the spec
> closely. The "16" should be either expressed at FIFOSIZE / 2 or
> explained in a comment.
ok.

>
>>
>>      vpl011->uartfr |= BUSY;
>>
>> @@ -368,7 +376,16 @@ static void vpl011_data_avail(struct domain *d)
>>      if ( out_ring_qsize != sizeof(intf->out) )
>>      {
>>          vpl011->uartfr &= ~TXFF;
>> -        vpl011->uartris |= TXI;
>> +
>> +        /*
>> +         * Ensure that there is space for atleast 16 bytes before asserting the
>> +         * TXI interrupt status bit because the SBSA UART driver may write upto
>> +         * 16 bytes (i.e. half the SBSA UART fifo size of 32) on getting
>> +         * a TX interrupt.
>
> The comment sounds a bit like this is hack, where it actually is a
> totally legit spec requirement (the interrupt is asserted/deasserted
> around the *trigger level*, which is half way by default and always half
> for the SBSA).
ok. I will modify the comment to mention that TX interrupt is
asserted/de-aserted based
on the trigger level which is fifo_size/2.

>
> Also I think the same logic/fix needs to be applied to the receiving side.
>
That may delay the processing of incoming data. To verify that I
changed the code to assert the RX interrupt only
when the RX FIFO becomes half full. With that change, the input data
is delayed as the SBSA UART driver starts
processing the incoming data only when the RX interrupt is asserted.

> And while I see that Julien requested a follow-up patch, I believe this
> should eventually be squashed into 02/27, to not have wrong code in the
> repo. But can could be done at commit time, I guess.
>
ok.

Regards,
Bhupinder

Hi,

On 07/09/17 15:37, Andre Przywara wrote:
>>   
>>       vpl011->uartfr |= BUSY;
>>   
>> @@ -368,7 +376,16 @@ static void vpl011_data_avail(struct domain *d)
>>       if ( out_ring_qsize != sizeof(intf->out) )
>>       {
>>           vpl011->uartfr &= ~TXFF;
>> -        vpl011->uartris |= TXI;
>> +
>> +        /*
>> +         * Ensure that there is space for atleast 16 bytes before asserting the
>> +         * TXI interrupt status bit because the SBSA UART driver may write upto
>> +         * 16 bytes (i.e. half the SBSA UART fifo size of 32) on getting
>> +         * a TX interrupt.
> 
> The comment sounds a bit like this is hack, where it actually is a
> totally legit spec requirement (the interrupt is asserted/deasserted
> around the *trigger level*, which is half way by default and always half
> for the SBSA).
> 
> Also I think the same logic/fix needs to be applied to the receiving side.
> 
> And while I see that Julien requested a follow-up patch, I believe this
> should eventually be squashed into 02/27, to not have wrong code in the
> repo. But can could be done at commit time, I guess.

There is nothing wrong to keep this patch separately. This can be 
considered as a bug fix and I like the idea of having it separately 
because it explain the rationale behind it. After all, there are nothing 
said on the SBSA so far and just an assumption on how the spec will be 
updated.

Cheers,

Hi Bhupinder,

I am just commenting on the commit message, Andre already commented the 
code.

On 28/08/17 09:56, Bhupinder Thakur wrote:
> This patch fixes the issue observed when pl011 patches were tested on
> the junos hardware by Andre/Julien. It was observed that when large output is
> generated such as on running 'find /', output was getting truncated intermittently
> due to OUT ring buffer getting full.
> 
> This issue was due to the fact that the SBSA UART driver expects that when
> a TX interrupt is asserted then the FIFO queue should be atleast half empty and
> that it can write N bytes in the FIFO, where N is half the FIFO queue size, without
> the bytes getting dropped due to FIFO getting full.
> 
> This requirement is as per section 3.4.2 of [1], which is:
> 
> -------------------------------------------------------------------------------
> UARTTXINTR

This register does not exist in the SBSA, so you cannot say it is a 
requirement from the specification. The emulation should be based on the 
specification and not how a driver behave. You don't know how other OS 
have implemented the driver.

As I mentioned in my previous answer, we are in process to clarify in 
the specification and we can currently assume the interrupt will be 
triggered at halfway.

So the commit message should reflect that.

Cheers,