Re: [Xen-devel] [PATCH 26/27 v12] arm/xen: vpl011: Fix the slow early console SBSA UART output

[Andre Przywara <andre.przywara@xxxxxxx>]

Hi Bhupinder,

first thing: As the bulk of the series has been merged now, please
restart your patch and version numbering, so a (potential) next post
should be prefixed [PATCH v3 1/2]. And please have a cover letter giving
a brief overview what this series fixes.

On 13/10/17 11:40, Bhupinder Thakur wrote:
> The early console output uses pl011_early_write() to write data. This
> function waits for BUSY bit to get cleared before writing the next byte.

... which is questionable given the actual definition of the BUSY bit in
the PL011 TRM:

============
.... The BUSY signal goes HIGH as soon as data is written to the
transmit FIFO (that is, the FIFO is non-empty) and remains asserted
HIGH while data is being transmitted. BUSY is negated only when the
transmit FIFO is empty, and the last character has been transmitted from
the shift register, ....
============

(I take it you are talking about the Linux driver in a guest here).
I think the early_write routine tries to (deliberately?) ignore the
FIFO, possibly to make sure characters really get pushed out before a
system crashes, maybe.

> 
> In the SBSA UART emulation logic, the BUSY bit was set as soon one
> byte was written in the FIFO and it remained set until the FIFO was
> emptied.

Which is correct behaviour, as this matches the PL011 TRM as quoted above.

> This meant that the output was delayed as each character needed
> the BUSY to get cleared.

But this is true as well!

> Since the SBSA UART is getting emulated in Xen using ring buffers, it
> ensures that once the data is enqueued in the FIFO, it will be received
> by xenconsole so it is safe to set the BUSY bit only when FIFO becomes
> full. This will ensure that pl011_early_write() is not delayed unduly
> to write the data.

So I can confirm that this patch fixes the very slow earlycon output
observed with the current staging HEAD.

So while this is somewhat deviating from the spec, I can see the benefit
for an emulation scenario. I believe that emulations in general might
choose implementing things a bit differently, to cope with the
fundamental differences in their environment, like the virtually endless
"FIFO" and the lack of any timing restrictions on the emulated "wire".

So unless someone comes up with a better solution, I would support
taking this patch, as this fixes a real problem.

Cheers,
Andre

> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@xxxxxxxxxx>
> ---
> CC: Julien Grall <julien.grall@xxxxxxx>
> CC: Andre Przywara <andre.przywara@xxxxxxx>
> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> 
>  xen/arch/arm/vpl011.c | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
> index f7ddccb..0b07436 100644
> --- a/xen/arch/arm/vpl011.c
> +++ b/xen/arch/arm/vpl011.c
> @@ -159,9 +159,15 @@ static void vpl011_write_data(struct domain *d, uint8_t 
> data)
>      {
>          vpl011->uartfr |= TXFF;
>          vpl011->uartris &= ~TXI;
> -    }
>  
> -    vpl011->uartfr |= BUSY;
> +        /*
> +         * This bit is set only when FIFO becomes full. This ensures that
> +         * the SBSA UART driver can write the early console data as fast as
> +         * possible, without waiting for the BUSY bit to get cleared before
> +         * writing each byte.
> +         */
> +        vpl011->uartfr |= BUSY;
> +    }
>  
>      vpl011->uartfr &= ~TXFE;
>  
> @@ -371,11 +377,16 @@ static void vpl011_data_avail(struct domain *d)
>      {
>          vpl011->uartfr &= ~TXFF;
>          vpl011->uartris |= TXI;
> +
> +        /*
> +         * Clear the BUSY bit as soon as space becomes available
> +         * so that the SBSA UART driver can start writing more data
> +         * without any further delay.
> +         */
> +        vpl011->uartfr &= ~BUSY;
> +
>          if ( out_ring_qsize == 0 )
> -        {
> -            vpl011->uartfr &= ~BUSY;
>              vpl011->uartfr |= TXFE;
> -        }
>      }
>  
>      vpl011_update_interrupt_status(d);
> 

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