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

[Rob Herring <robh@xxxxxxxxxx>]

On Tue, Oct 17, 2017 at 6:19 AM, Dave Martin <Dave.Martin@xxxxxxx> wrote:
> On Tue, Oct 17, 2017 at 10:51:07AM +0100, Andre Przywara wrote:
>> 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.
>
> I think you get away with this, but it does violate the spec in order
> to work around a feature of a correctly implemented driver.
>
> Software can now see this, for example:
>
>         uart_write(ch, UARTDR);
>         busy = uart_read(UARTFR) & UARTFR_BUSY;
>         BUG_ON(!(uart_read(UARTFR) & UARTFR_TXFE) && !busy);
>
> which violates the spec, though I can't currently think of a good reason
> for software to rely on that.
>
>
> [+Rob, who wrote the original earlycon code in the amba-pl011 driver:
> 0d3c673e7881 ("tty/serial: pl011: add generic earlycon support")
>
> Is there any actualy reason why we poll for !BUSY after each char in
> pl011_putc()?  pl011_putc() is not exposed at all: it's only called by
> pl011_console_write().
>
> This will result in stuttering output even on hardware, though this
> doesn't typically matter.
>
> I think if the poll for !BUSY were moved to the end of
> pl011_console_write(), the effect would be much less bad.]

I just copied the code from the arm64 earlyprintk code... Maybe it was
because on simulation (which was the main platform at the time) folks
wanted the character "on the wire". It seems to be that you could just
drop it.

Rob

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