| 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 |
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.
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
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.
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
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.
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.