Hi,
On 18/10/17 11:17, Bhupinder Thakur wrote:
> Hi Andre,
>
> On 17 October 2017 at 15:21, Andre Przywara <andre.przyw...@arm.com> 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.
>>
> Should I resend the patch series with a cover letter? I will also add
> a reported-by tag.
Please wait until we have settled upon a solution, especially for that
other patch. We can talk about this in our meeting later today.
Cheers,
Andre.
>> 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.tha...@linaro.org>
>>> ---
>>> CC: Julien Grall <julien.gr...@arm.com>
>>> CC: Andre Przywara <andre.przyw...@arm.com>
>>> CC: Stefano Stabellini <sstabell...@kernel.org>
>>>
>>> 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);
>>>
>
> Regards,
> Bhupinder
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
