mbox series

[PATCH-for-8.2,v4,00/10] hw/char/pl011: Implement TX (async) FIFO to avoid blocking the main loop

Message ID 20231109192814.95977-1-philmd@linaro.org
Headers show
Series hw/char/pl011: Implement TX (async) FIFO to avoid blocking the main loop | expand

Message

Philippe Mathieu-Daudé Nov. 9, 2023, 7:28 p.m. UTC
Missing review: #10

Hi,

This series add support for (async) FIFO on the transmit path
of the PL011 UART.

Since v3:
- Document migration bits (Alex, Richard)
- Just check FIFO is not empty in pl011_xmit_fifo_state_needed (rth)
- In pl011_xmit check TX enabled first, and ignore < 8-bit TX (rth)

Since v2:
- Added R-b tags
- Addressed Richard comments on migration

Since v1:
- Restrict pl011_ops[] impl access_size,
- Do not check transmitter is enabled (Peter),
- Addressed Alex's review comments,
- Simplified migration trying to care about backward compat,
  but still unsure...

Philippe Mathieu-Daudé (10):
  util/fifo8: Allow fifo8_pop_buf() to not populate popped length
  util/fifo8: Introduce fifo8_peek_buf()
  hw/char/pl011: Split RX/TX path of pl011_reset_fifo()
  hw/char/pl011: Extract pl011_write_txdata() from pl011_write()
  hw/char/pl011: Extract pl011_read_rxdata() from pl011_read()
  hw/char/pl011: Warn when using disabled transmitter
  hw/char/pl011: Check if receiver is enabled
  hw/char/pl011: Rename RX FIFO methods
  hw/char/pl011: Add transmit FIFO to PL011State
  hw/char/pl011: Implement TX FIFO

 include/hw/char/pl011.h |   2 +
 include/qemu/fifo8.h    |  37 ++++++-
 hw/char/pl011.c         | 239 +++++++++++++++++++++++++++++++++-------
 util/fifo8.c            |  28 ++++-
 hw/char/trace-events    |   8 +-
 5 files changed, 263 insertions(+), 51 deletions(-)

Comments

Philippe Mathieu-Daudé Nov. 9, 2023, 8:59 p.m. UTC | #1
Hi Peter,

On 9/11/23 20:29, Peter Maydell wrote:
> On Thu, 9 Nov 2023 at 19:28, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> Missing review: #10
>>
>> Hi,
>>
>> This series add support for (async) FIFO on the transmit path
>> of the PL011 UART.
> 
> Hi; what's the rationale for the "for-8.2" targeting here?
> What bug are we fixing?

The bug is on Trusted Substrate when the ZynqMP machine is used:
https://linaro.atlassian.net/browse/TRS-149?focusedCommentId=149574

Regards,

Phil.
Peter Maydell Nov. 13, 2023, 1:11 p.m. UTC | #2
On Thu, 9 Nov 2023 at 20:59, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Hi Peter,
>
> On 9/11/23 20:29, Peter Maydell wrote:
> > On Thu, 9 Nov 2023 at 19:28, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> >>
> >> Missing review: #10
> >>
> >> Hi,
> >>
> >> This series add support for (async) FIFO on the transmit path
> >> of the PL011 UART.
> >
> > Hi; what's the rationale for the "for-8.2" targeting here?
> > What bug are we fixing?
>
> The bug is on Trusted Substrate when the ZynqMP machine is used:
> https://linaro.atlassian.net/browse/TRS-149?focusedCommentId=149574

And have we confirmed that the async FIFO support fixes that problem?
That bug report seems to have mostly just speculation in it that
maybe this XXX comment is why...

-- PMM
Philippe Mathieu-Daudé Nov. 13, 2023, 3:44 p.m. UTC | #3
Hi Peter,

Cc'ing Mikko.

On 13/11/23 14:11, Peter Maydell wrote:
> On Thu, 9 Nov 2023 at 20:59, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> Hi Peter,
>>
>> On 9/11/23 20:29, Peter Maydell wrote:
>>> On Thu, 9 Nov 2023 at 19:28, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>>>
>>>> Missing review: #10
>>>>
>>>> Hi,
>>>>
>>>> This series add support for (async) FIFO on the transmit path
>>>> of the PL011 UART.
>>>
>>> Hi; what's the rationale for the "for-8.2" targeting here?
>>> What bug are we fixing?
>>
>> The bug is on Trusted Substrate when the ZynqMP machine is used:
>> https://linaro.atlassian.net/browse/TRS-149?focusedCommentId=149574
> 
> And have we confirmed that the async FIFO support fixes that problem?
> That bug report seems to have mostly just speculation in it that
> maybe this XXX comment is why...

Mikko tested the v2 (or v1?) and confirmed it was fixing their problem
with TRS.

That said, besides the v4 last-minute review from Richard just before
his US holiday WE, I have to sadly recognize -- although I could argue
this is a bug fix -- this series is not yet ready, thus will miss the
8.2 release :(

(Mikko: no need to test this one, I'll Cc you on the next one to get
  your Tested-by tag on the list).

Regards,

Phil.
Alex Bennée Nov. 24, 2023, 10:24 a.m. UTC | #4
Peter Maydell <peter.maydell@linaro.org> writes:

> On Thu, 9 Nov 2023 at 20:59, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> Hi Peter,
>>
>> On 9/11/23 20:29, Peter Maydell wrote:
>> > On Thu, 9 Nov 2023 at 19:28, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>> >>
>> >> Missing review: #10
>> >>
>> >> Hi,
>> >>
>> >> This series add support for (async) FIFO on the transmit path
>> >> of the PL011 UART.
>> >
>> > Hi; what's the rationale for the "for-8.2" targeting here?
>> > What bug are we fixing?
>>
>> The bug is on Trusted Substrate when the ZynqMP machine is used:
>> https://linaro.atlassian.net/browse/TRS-149?focusedCommentId=149574
>
> And have we confirmed that the async FIFO support fixes that problem?
> That bug report seems to have mostly just speculation in it that
> maybe this XXX comment is why...

I've been fighting with numerous issues with the TRS build over the last
week so I can confirm I have seen a) a lock up with pl011_write blocking
everything under the BQL because data wasn't read fast enough and b) the
problem goes away with Philippe's patches. So have a:

Tested-by: Alex Bennée <alex.bennee@linaro.org>

for the series.

>
> -- PMM
Mark Cave-Ayland Jan. 5, 2024, 7:50 a.m. UTC | #5
On 09/11/2023 19:28, Philippe Mathieu-Daudé wrote:

> Missing review: #10
> 
> Hi,
> 
> This series add support for (async) FIFO on the transmit path
> of the PL011 UART.
> 
> Since v3:
> - Document migration bits (Alex, Richard)
> - Just check FIFO is not empty in pl011_xmit_fifo_state_needed (rth)
> - In pl011_xmit check TX enabled first, and ignore < 8-bit TX (rth)
> 
> Since v2:
> - Added R-b tags
> - Addressed Richard comments on migration
> 
> Since v1:
> - Restrict pl011_ops[] impl access_size,
> - Do not check transmitter is enabled (Peter),
> - Addressed Alex's review comments,
> - Simplified migration trying to care about backward compat,
>    but still unsure...
> 
> Philippe Mathieu-Daudé (10):
>    util/fifo8: Allow fifo8_pop_buf() to not populate popped length
>    util/fifo8: Introduce fifo8_peek_buf()
>    hw/char/pl011: Split RX/TX path of pl011_reset_fifo()
>    hw/char/pl011: Extract pl011_write_txdata() from pl011_write()
>    hw/char/pl011: Extract pl011_read_rxdata() from pl011_read()
>    hw/char/pl011: Warn when using disabled transmitter
>    hw/char/pl011: Check if receiver is enabled
>    hw/char/pl011: Rename RX FIFO methods
>    hw/char/pl011: Add transmit FIFO to PL011State
>    hw/char/pl011: Implement TX FIFO
> 
>   include/hw/char/pl011.h |   2 +
>   include/qemu/fifo8.h    |  37 ++++++-
>   hw/char/pl011.c         | 239 +++++++++++++++++++++++++++++++++-------
>   util/fifo8.c            |  28 ++++-
>   hw/char/trace-events    |   8 +-
>   5 files changed, 263 insertions(+), 51 deletions(-)

Hi Phil,

Happy New Year! Are there plans to queue this series for 9.0 soon? I'm particularly 
interested in the first 2 patches as I've made use of the new fifo8_peek_buf() 
function as part of my latest ESP updates.


ATB,

Mark.
Mark Cave-Ayland Jan. 10, 2024, 7:05 a.m. UTC | #6
On 05/01/2024 07:50, Mark Cave-Ayland wrote:

> On 09/11/2023 19:28, Philippe Mathieu-Daudé wrote:
> 
>> Missing review: #10
>>
>> Hi,
>>
>> This series add support for (async) FIFO on the transmit path
>> of the PL011 UART.
>>
>> Since v3:
>> - Document migration bits (Alex, Richard)
>> - Just check FIFO is not empty in pl011_xmit_fifo_state_needed (rth)
>> - In pl011_xmit check TX enabled first, and ignore < 8-bit TX (rth)
>>
>> Since v2:
>> - Added R-b tags
>> - Addressed Richard comments on migration
>>
>> Since v1:
>> - Restrict pl011_ops[] impl access_size,
>> - Do not check transmitter is enabled (Peter),
>> - Addressed Alex's review comments,
>> - Simplified migration trying to care about backward compat,
>>    but still unsure...
>>
>> Philippe Mathieu-Daudé (10):
>>    util/fifo8: Allow fifo8_pop_buf() to not populate popped length
>>    util/fifo8: Introduce fifo8_peek_buf()
>>    hw/char/pl011: Split RX/TX path of pl011_reset_fifo()
>>    hw/char/pl011: Extract pl011_write_txdata() from pl011_write()
>>    hw/char/pl011: Extract pl011_read_rxdata() from pl011_read()
>>    hw/char/pl011: Warn when using disabled transmitter
>>    hw/char/pl011: Check if receiver is enabled
>>    hw/char/pl011: Rename RX FIFO methods
>>    hw/char/pl011: Add transmit FIFO to PL011State
>>    hw/char/pl011: Implement TX FIFO
>>
>>   include/hw/char/pl011.h |   2 +
>>   include/qemu/fifo8.h    |  37 ++++++-
>>   hw/char/pl011.c         | 239 +++++++++++++++++++++++++++++++++-------
>>   util/fifo8.c            |  28 ++++-
>>   hw/char/trace-events    |   8 +-
>>   5 files changed, 263 insertions(+), 51 deletions(-)
> 
> Hi Phil,
> 
> Happy New Year! Are there plans to queue this series for 9.0 soon? I'm particularly 
> interested in the first 2 patches as I've made use of the new fifo8_peek_buf() 
> function as part of my latest ESP updates.

I've spoken to Phil, and as patches 1 and 2 implementing fifo8_peek_buf() have R-B 
tags he is happy for me to take them separately via my qemu-sparc branch. I'll send a 
PR with those patches shortly.


ATB,

Mark.