| Message ID | 1506068606-17066-28-git-send-email-bhupinder.thakur@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | SBSA UART emulation support in Xen | expand |
On Fri, Sep 22, 2017 at 01:53:26PM +0530, Bhupinder Thakur wrote: > This patch fixes the issue observed when pl011 patches were tested on > the junos hardware by Andre/Julien. It was observed that when large output is > generated such as on running 'find /', output was getting truncated intermittently > due to OUT ring buffer getting full. > > This issue was due to the fact that the SBSA UART driver expects that when > a TX interrupt is asserted then the FIFO queue should be atleast half empty and > that it can write N bytes in the FIFO, where N is half the FIFO queue size, without > the bytes getting dropped due to FIFO getting full. > > The SBSA UART emulation logic was asserting the TX interrupt as soon as > any space became available in the FIFO and the SBSA UART driver tried to write > more data (upto 16 bytes) in the FIFO expecting that there is enough space > available leading to dropped bytes. > > The SBSA spec [1] does not specify when the TX interrupt should be asserted > or de-asserted. Due to lack of clarity on the expected behavior, it is > assumed for now that TX interrupt should be asserted only when the FIFO goes > half empty. > > TBD: Once the SBSA spec is updated with the expected behavior, the implementation > will be modified to align with the spec requirement. > > [1] http://infocenter.arm.com/help/topic/com.arm.doc.ddi0183f/DDI0183.pdf > > Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org> > --- > CC: Julien Grall <julien.grall@arm.com> > CC: Andre Przywara <andre.przywara@arm.com> > CC: Stefano Stabellini <sstabellini@kernel.org> (Taking a quick look at this because I remember fighthing with FIFO behaviour issues when hacking the Linux driver -- but beware, I'm not a Xen guy...) Should this patch be flattened into the patches is fixes? Keeping known-wrong code in the series does not help reviewers (but maybe it's the Xen way). > Changes since v8: > - Used variables fifo_level/fifo_threshold for more clarity > - Added a new macro SBSA_UART_FIFO_SIZE instead of using a magic number What's sizeof(intf->in), sizeof(intf->out)? For correct operation, you assume that the total ring buffer size is >= SBSA_UART_FIFO_SIZE, but nothing enforces this. If the xencons ring buffer size is set elsewhere and can't be chosen by a driver, you may at least add a build-time assert to check that it's big enough. [...] > @@ -144,28 +148,41 @@ static void vpl011_write_data(struct domain *d, uint8_t data) [...] > + * Clear the TXI bit if the fifo level exceeds fifo_size/2 mark which > + * is the trigger level for asserting/de-assterting the TX interrupt. > */ > - vpl011->uartfr |= BUSY; > + fifo_threshold = sizeof (intf->out) - SBSA_UART_FIFO_SIZE/2; > + > + if ( fifo_level <= fifo_threshold ) > + vpl011->uartris |= TXI; > + else > + vpl011->uartris &= ~TXI; > } > + else > + gprintk(XENLOG_ERR, "vpl011: Unexpected OUT ring buffer full\n"); > > vpl011->uartfr &= ~TXFE; > [...] > @@ -353,37 +370,51 @@ static void vpl011_data_avail(struct domain *d) > > smp_rmb(); > > - in_ring_qsize = xencons_queued(in_prod, > + in_fifo_level = xencons_queued(in_prod, > in_cons, > sizeof(intf->in)); > > - out_ring_qsize = xencons_queued(out_prod, > + out_fifo_level = xencons_queued(out_prod, > out_cons, > sizeof(intf->out)); > > /* Update the uart rx state if the buffer is not empty. */ > - if ( in_ring_qsize != 0 ) > + if ( in_fifo_level != 0 ) > { > vpl011->uartfr &= ~RXFE; > - if ( in_ring_qsize == sizeof(intf->in) ) > + > + if ( in_fifo_level == sizeof(intf->in) ) > vpl011->uartfr |= RXFF; > + > vpl011->uartris |= RXI; > } > > /* Update the uart tx state if the buffer is not full. */ > - if ( out_ring_qsize != sizeof(intf->out) ) > + if ( out_fifo_level != sizeof(intf->out) ) > { > + unsigned int out_fifo_threshold; > + > vpl011->uartfr &= ~TXFF; > - vpl011->uartris |= TXI; > > /* > - * Clear the BUSY bit as soon as space becomes available > + * Clear the BUSY bit as soon as space becomes avaliable > * so that the SBSA UART driver can start writing more data > * without any further delay. > */ > vpl011->uartfr &= ~BUSY; > > - if ( out_ring_qsize == 0 ) > + /* > + * Set the TXI bit only when there is space for fifo_size/2 bytes which > + * is the trigger level for asserting/de-assterting the TX interrupt. > + */ > + out_fifo_threshold = sizeof(intf->out) - SBSA_UART_FIFO_SIZE/2; > + > + if ( out_fifo_level <= out_fifo_threshold ) > + vpl011->uartris |= TXI; > + else > + vpl011->uartris &= ~TXI; Should this logic be factored out? You do the same thing in _write_data(). Also, is there a reason why you implement the trigger threshold logic on the TX side only? It looks inconsistent now. I think a real PL011 implements the trigger logic in exactly the same way for RX and TX (except for swapping >= for <= in the threshold comparison). It doesn't look like the Linux pl011 driver relies on a correctly implemented RX trigger level today, but it may have done in the past -- I did some hacking in this area at some point, but can't remember the details now. Asserting RXI whenever the RX FIFO is nonempty would result in excessive interrupts if you are streaming the data from a slow source (such as a real UART) and pushing the chars one by one to the emulated UART: the guest would take an IRQ on each char rather than waiting until the RX FIFO is half-full. Cheers ---Dave
Hi Dave, On 09/26/2017 03:38 PM, Dave Martin wrote: > On Fri, Sep 22, 2017 at 01:53:26PM +0530, Bhupinder Thakur wrote: >> This patch fixes the issue observed when pl011 patches were tested on >> the junos hardware by Andre/Julien. It was observed that when large output is >> generated such as on running 'find /', output was getting truncated intermittently >> due to OUT ring buffer getting full. >> >> This issue was due to the fact that the SBSA UART driver expects that when >> a TX interrupt is asserted then the FIFO queue should be atleast half empty and >> that it can write N bytes in the FIFO, where N is half the FIFO queue size, without >> the bytes getting dropped due to FIFO getting full. >> >> The SBSA UART emulation logic was asserting the TX interrupt as soon as >> any space became available in the FIFO and the SBSA UART driver tried to write >> more data (upto 16 bytes) in the FIFO expecting that there is enough space >> available leading to dropped bytes. >> >> The SBSA spec [1] does not specify when the TX interrupt should be asserted >> or de-asserted. Due to lack of clarity on the expected behavior, it is >> assumed for now that TX interrupt should be asserted only when the FIFO goes >> half empty. >> >> TBD: Once the SBSA spec is updated with the expected behavior, the implementation >> will be modified to align with the spec requirement. >> >> [1] http://infocenter.arm.com/help/topic/com.arm.doc.ddi0183f/DDI0183.pdf >> >> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org> >> --- >> CC: Julien Grall <julien.grall@arm.com> >> CC: Andre Przywara <andre.przywara@arm.com> >> CC: Stefano Stabellini <sstabellini@kernel.org> > > (Taking a quick look at this because I remember fighthing with FIFO > behaviour issues when hacking the Linux driver -- but beware, I'm not a > Xen guy...) > > > Should this patch be flattened into the patches is fixes? Keeping > known-wrong code in the series does not help reviewers (but maybe it's > the Xen way). We usually prefer to have patch fold into the patches it fixes. However, I specifically ask Bhupinder to do a follow-up because the rest of the series is nearly ready. So we could merge the first 25 patches before the code freeze as the two other could be considered as bug fixes. This would allow us to get PL011 support in Xen 4.10. Cheers,
On Tue, Sep 26, 2017 at 04:50:44PM +0100, Julien Grall wrote: > Hi Dave, > > On 09/26/2017 03:38 PM, Dave Martin wrote: > >On Fri, Sep 22, 2017 at 01:53:26PM +0530, Bhupinder Thakur wrote: > >>This patch fixes the issue observed when pl011 patches were tested on > >>the junos hardware by Andre/Julien. It was observed that when large output is [...] > >(Taking a quick look at this because I remember fighthing with FIFO > >behaviour issues when hacking the Linux driver -- but beware, I'm not a > >Xen guy...) > > > > > >Should this patch be flattened into the patches is fixes? Keeping > >known-wrong code in the series does not help reviewers (but maybe it's > >the Xen way). > > We usually prefer to have patch fold into the patches it fixes. However, I > specifically ask Bhupinder to do a follow-up because the rest of the series > is nearly ready. > > So we could merge the first 25 patches before the code freeze as the two > other could be considered as bug fixes. This would allow us to get PL011 > support in Xen 4.10. That's fair -- I jumped into the series without much context, because I remember "fixing" the pl011 driver in this area in the past. Cheers ---Dave
Hi Dave, On 26 September 2017 at 20:08, Dave Martin <Dave.Martin@arm.com> wrote: > On Fri, Sep 22, 2017 at 01:53:26PM +0530, Bhupinder Thakur wrote: >> This patch fixes the issue observed when pl011 patches were tested on >> the junos hardware by Andre/Julien. It was observed that when large output is >> generated such as on running 'find /', output was getting truncated intermittently >> due to OUT ring buffer getting full. >> >> This issue was due to the fact that the SBSA UART driver expects that when >> a TX interrupt is asserted then the FIFO queue should be atleast half empty and >> that it can write N bytes in the FIFO, where N is half the FIFO queue size, without >> the bytes getting dropped due to FIFO getting full. >> >> The SBSA UART emulation logic was asserting the TX interrupt as soon as >> any space became available in the FIFO and the SBSA UART driver tried to write >> more data (upto 16 bytes) in the FIFO expecting that there is enough space >> available leading to dropped bytes. >> >> The SBSA spec [1] does not specify when the TX interrupt should be asserted >> or de-asserted. Due to lack of clarity on the expected behavior, it is >> assumed for now that TX interrupt should be asserted only when the FIFO goes >> half empty. >> >> TBD: Once the SBSA spec is updated with the expected behavior, the implementation >> will be modified to align with the spec requirement. >> >> [1] http://infocenter.arm.com/help/topic/com.arm.doc.ddi0183f/DDI0183.pdf >> >> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org> >> --- >> CC: Julien Grall <julien.grall@arm.com> >> CC: Andre Przywara <andre.przywara@arm.com> >> CC: Stefano Stabellini <sstabellini@kernel.org> > > (Taking a quick look at this because I remember fighthing with FIFO > behaviour issues when hacking the Linux driver -- but beware, I'm not a > Xen guy...) > > > Should this patch be flattened into the patches is fixes? Keeping > known-wrong code in the series does not help reviewers (but maybe it's > the Xen way). > >> Changes since v8: >> - Used variables fifo_level/fifo_threshold for more clarity >> - Added a new macro SBSA_UART_FIFO_SIZE instead of using a magic number > > What's sizeof(intf->in), sizeof(intf->out)? > > For correct operation, you assume that the total ring buffer size is >= > SBSA_UART_FIFO_SIZE, but nothing enforces this. If the xencons ring > buffer size is set elsewhere and can't be chosen by a driver, you may at > least add a build-time assert to check that it's big enough. > I will add an assert to check this condition. > [...] > >> @@ -144,28 +148,41 @@ static void vpl011_write_data(struct domain *d, uint8_t data) > > [...] > >> + * Clear the TXI bit if the fifo level exceeds fifo_size/2 mark which >> + * is the trigger level for asserting/de-assterting the TX interrupt. >> */ >> - vpl011->uartfr |= BUSY; >> + fifo_threshold = sizeof (intf->out) - SBSA_UART_FIFO_SIZE/2; >> + >> + if ( fifo_level <= fifo_threshold ) >> + vpl011->uartris |= TXI; >> + else >> + vpl011->uartris &= ~TXI; >> } >> + else >> + gprintk(XENLOG_ERR, "vpl011: Unexpected OUT ring buffer full\n"); >> >> vpl011->uartfr &= ~TXFE; >> > > [...] > >> @@ -353,37 +370,51 @@ static void vpl011_data_avail(struct domain *d) >> >> smp_rmb(); >> >> - in_ring_qsize = xencons_queued(in_prod, >> + in_fifo_level = xencons_queued(in_prod, >> in_cons, >> sizeof(intf->in)); >> >> - out_ring_qsize = xencons_queued(out_prod, >> + out_fifo_level = xencons_queued(out_prod, >> out_cons, >> sizeof(intf->out)); >> >> /* Update the uart rx state if the buffer is not empty. */ >> - if ( in_ring_qsize != 0 ) >> + if ( in_fifo_level != 0 ) >> { >> vpl011->uartfr &= ~RXFE; >> - if ( in_ring_qsize == sizeof(intf->in) ) >> + >> + if ( in_fifo_level == sizeof(intf->in) ) >> vpl011->uartfr |= RXFF; >> + >> vpl011->uartris |= RXI; >> } >> >> /* Update the uart tx state if the buffer is not full. */ >> - if ( out_ring_qsize != sizeof(intf->out) ) >> + if ( out_fifo_level != sizeof(intf->out) ) >> { >> + unsigned int out_fifo_threshold; >> + >> vpl011->uartfr &= ~TXFF; >> - vpl011->uartris |= TXI; >> >> /* >> - * Clear the BUSY bit as soon as space becomes available >> + * Clear the BUSY bit as soon as space becomes avaliable >> * so that the SBSA UART driver can start writing more data >> * without any further delay. >> */ >> vpl011->uartfr &= ~BUSY; >> >> - if ( out_ring_qsize == 0 ) >> + /* >> + * Set the TXI bit only when there is space for fifo_size/2 bytes which >> + * is the trigger level for asserting/de-assterting the TX interrupt. >> + */ >> + out_fifo_threshold = sizeof(intf->out) - SBSA_UART_FIFO_SIZE/2; >> + >> + if ( out_fifo_level <= out_fifo_threshold ) >> + vpl011->uartris |= TXI; >> + else >> + vpl011->uartris &= ~TXI; > > Should this logic be factored out? You do the same thing in > _write_data(). I will add a common function to set the TXI bit. > > Also, is there a reason why you implement the trigger threshold logic on > the TX side only? It looks inconsistent now. I did try with RX FIFO threshold also but it seems the current pl011 driver does not poll on the RX FIFO and just waits for the RX interrupt trigger to start processing the RX data. This makes RX very slow and if the RX FIFO is not filled sufficiently, it does not read data further. > > I think a real PL011 implements the trigger logic in exactly the same > way for RX and TX (except for swapping >= for <= in the threshold > comparison). > > > It doesn't look like the Linux pl011 driver relies on a correctly > implemented RX trigger level today, but it may have done in the past -- > I did some hacking in this area at some point, but can't remember the > details now. > The current pl011 driver > Asserting RXI whenever the RX FIFO is nonempty would result in excessive > interrupts if you are streaming the data from a slow source (such as a > real UART) and pushing the chars one by one to the emulated UART: the > guest would take an IRQ on each char rather than waiting until the RX > FIFO is half-full. > I agree it is an overhead. This may be an issue with the driver which is solely depending on the RX interrupt. I think it should switch to polling if there are no interrupts received recently. Regards, Bhupinder
On Wed, Oct 11, 2017 at 01:28:44PM +0530, Bhupinder Thakur wrote: > Hi Dave, > > On 26 September 2017 at 20:08, Dave Martin <Dave.Martin@arm.com> wrote: > > On Fri, Sep 22, 2017 at 01:53:26PM +0530, Bhupinder Thakur wrote: > >> This patch fixes the issue observed when pl011 patches were tested on > >> the junos hardware by Andre/Julien. It was observed that when large output is > >> generated such as on running 'find /', output was getting truncated intermittently > >> due to OUT ring buffer getting full. > >> > >> This issue was due to the fact that the SBSA UART driver expects that when > >> a TX interrupt is asserted then the FIFO queue should be atleast half empty and > >> that it can write N bytes in the FIFO, where N is half the FIFO queue size, without > >> the bytes getting dropped due to FIFO getting full. > >> > >> The SBSA UART emulation logic was asserting the TX interrupt as soon as > >> any space became available in the FIFO and the SBSA UART driver tried to write > >> more data (upto 16 bytes) in the FIFO expecting that there is enough space > >> available leading to dropped bytes. > >> > >> The SBSA spec [1] does not specify when the TX interrupt should be asserted > >> or de-asserted. Due to lack of clarity on the expected behavior, it is > >> assumed for now that TX interrupt should be asserted only when the FIFO goes > >> half empty. > >> > >> TBD: Once the SBSA spec is updated with the expected behavior, the implementation > >> will be modified to align with the spec requirement. > >> > >> [1] http://infocenter.arm.com/help/topic/com.arm.doc.ddi0183f/DDI0183.pdf > >> > >> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org> > >> --- > >> CC: Julien Grall <julien.grall@arm.com> > >> CC: Andre Przywara <andre.przywara@arm.com> > >> CC: Stefano Stabellini <sstabellini@kernel.org> > > > > (Taking a quick look at this because I remember fighthing with FIFO > > behaviour issues when hacking the Linux driver -- but beware, I'm not a > > Xen guy...) > > > > > > Should this patch be flattened into the patches is fixes? Keeping > > known-wrong code in the series does not help reviewers (but maybe it's > > the Xen way). > > > >> Changes since v8: > >> - Used variables fifo_level/fifo_threshold for more clarity > >> - Added a new macro SBSA_UART_FIFO_SIZE instead of using a magic number > > > > What's sizeof(intf->in), sizeof(intf->out)? > > > > For correct operation, you assume that the total ring buffer size is >= > > SBSA_UART_FIFO_SIZE, but nothing enforces this. If the xencons ring > > buffer size is set elsewhere and can't be chosen by a driver, you may at > > least add a build-time assert to check that it's big enough. > > > I will add an assert to check this condition. > > > [...] > > > >> @@ -144,28 +148,41 @@ static void vpl011_write_data(struct domain *d, uint8_t data) > > > > [...] > > > >> + * Clear the TXI bit if the fifo level exceeds fifo_size/2 mark which > >> + * is the trigger level for asserting/de-assterting the TX interrupt. > >> */ > >> - vpl011->uartfr |= BUSY; > >> + fifo_threshold = sizeof (intf->out) - SBSA_UART_FIFO_SIZE/2; > >> + > >> + if ( fifo_level <= fifo_threshold ) > >> + vpl011->uartris |= TXI; > >> + else > >> + vpl011->uartris &= ~TXI; > >> } > >> + else > >> + gprintk(XENLOG_ERR, "vpl011: Unexpected OUT ring buffer full\n"); > >> > >> vpl011->uartfr &= ~TXFE; > >> > > > > [...] > > > >> @@ -353,37 +370,51 @@ static void vpl011_data_avail(struct domain *d) > >> > >> smp_rmb(); > >> > >> - in_ring_qsize = xencons_queued(in_prod, > >> + in_fifo_level = xencons_queued(in_prod, > >> in_cons, > >> sizeof(intf->in)); > >> > >> - out_ring_qsize = xencons_queued(out_prod, > >> + out_fifo_level = xencons_queued(out_prod, > >> out_cons, > >> sizeof(intf->out)); > >> > >> /* Update the uart rx state if the buffer is not empty. */ > >> - if ( in_ring_qsize != 0 ) > >> + if ( in_fifo_level != 0 ) > >> { > >> vpl011->uartfr &= ~RXFE; > >> - if ( in_ring_qsize == sizeof(intf->in) ) > >> + > >> + if ( in_fifo_level == sizeof(intf->in) ) > >> vpl011->uartfr |= RXFF; > >> + > >> vpl011->uartris |= RXI; > >> } > >> > >> /* Update the uart tx state if the buffer is not full. */ > >> - if ( out_ring_qsize != sizeof(intf->out) ) > >> + if ( out_fifo_level != sizeof(intf->out) ) > >> { > >> + unsigned int out_fifo_threshold; > >> + > >> vpl011->uartfr &= ~TXFF; > >> - vpl011->uartris |= TXI; > >> > >> /* > >> - * Clear the BUSY bit as soon as space becomes available > >> + * Clear the BUSY bit as soon as space becomes avaliable > >> * so that the SBSA UART driver can start writing more data > >> * without any further delay. > >> */ > >> vpl011->uartfr &= ~BUSY; > >> > >> - if ( out_ring_qsize == 0 ) > >> + /* > >> + * Set the TXI bit only when there is space for fifo_size/2 bytes which > >> + * is the trigger level for asserting/de-assterting the TX interrupt. > >> + */ > >> + out_fifo_threshold = sizeof(intf->out) - SBSA_UART_FIFO_SIZE/2; > >> + > >> + if ( out_fifo_level <= out_fifo_threshold ) > >> + vpl011->uartris |= TXI; > >> + else > >> + vpl011->uartris &= ~TXI; > > > > Should this logic be factored out? You do the same thing in > > _write_data(). > I will add a common function to set the TXI bit. > > > > Also, is there a reason why you implement the trigger threshold logic on > > the TX side only? It looks inconsistent now. > I did try with RX FIFO threshold also but it seems the current pl011 > driver does not > poll on the RX FIFO and just waits for the RX interrupt trigger to > start processing the RX data. > This makes RX very slow and if the RX FIFO is not filled sufficiently, > it does not read data further. > > > > I think a real PL011 implements the trigger logic in exactly the same > > way for RX and TX (except for swapping >= for <= in the threshold > > comparison). > > > > > > It doesn't look like the Linux pl011 driver relies on a correctly > > implemented RX trigger level today, but it may have done in the past -- > > I did some hacking in this area at some point, but can't remember the > > details now. > > > The current pl011 driver > > Asserting RXI whenever the RX FIFO is nonempty would result in excessive > > interrupts if you are streaming the data from a slow source (such as a > > real UART) and pushing the chars one by one to the emulated UART: the > > guest would take an IRQ on each char rather than waiting until the RX > > FIFO is half-full. > > > I agree it is an overhead. This may be an issue with the driver which > is solely depending on the RX > interrupt. I think it should switch to polling if there are no > interrupts received recently. Hmmm, good point, but isn't that what the receive timeout interrupt is supposed to be for? The Linux driver seems to rely on the receive timeout interrupt to recover from an RX stall when the FIFO is not empty but also not full enough to trigger the RX FIFO interrupt. Does your driver actually implement the receive timeout interrupt? I'm not very familiar with the code, so I may have missed it. Cheers ---Dave
On 11 October 2017 at 15:38, Dave Martin <Dave.Martin@arm.com> wrote: > On Wed, Oct 11, 2017 at 01:28:44PM +0530, Bhupinder Thakur wrote: >> Hi Dave, >> >> On 26 September 2017 at 20:08, Dave Martin <Dave.Martin@arm.com> wrote: >> > On Fri, Sep 22, 2017 at 01:53:26PM +0530, Bhupinder Thakur wrote: >> >> This patch fixes the issue observed when pl011 patches were tested on >> >> the junos hardware by Andre/Julien. It was observed that when large output is >> >> generated such as on running 'find /', output was getting truncated intermittently >> >> due to OUT ring buffer getting full. >> >> >> >> This issue was due to the fact that the SBSA UART driver expects that when >> >> a TX interrupt is asserted then the FIFO queue should be atleast half empty and >> >> that it can write N bytes in the FIFO, where N is half the FIFO queue size, without >> >> the bytes getting dropped due to FIFO getting full. >> >> >> >> The SBSA UART emulation logic was asserting the TX interrupt as soon as >> >> any space became available in the FIFO and the SBSA UART driver tried to write >> >> more data (upto 16 bytes) in the FIFO expecting that there is enough space >> >> available leading to dropped bytes. >> >> >> >> The SBSA spec [1] does not specify when the TX interrupt should be asserted >> >> or de-asserted. Due to lack of clarity on the expected behavior, it is >> >> assumed for now that TX interrupt should be asserted only when the FIFO goes >> >> half empty. >> >> >> >> TBD: Once the SBSA spec is updated with the expected behavior, the implementation >> >> will be modified to align with the spec requirement. >> >> >> >> [1] http://infocenter.arm.com/help/topic/com.arm.doc.ddi0183f/DDI0183.pdf >> >> >> >> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org> >> >> --- >> >> CC: Julien Grall <julien.grall@arm.com> >> >> CC: Andre Przywara <andre.przywara@arm.com> >> >> CC: Stefano Stabellini <sstabellini@kernel.org> >> > >> > (Taking a quick look at this because I remember fighthing with FIFO >> > behaviour issues when hacking the Linux driver -- but beware, I'm not a >> > Xen guy...) >> > >> > >> > Should this patch be flattened into the patches is fixes? Keeping >> > known-wrong code in the series does not help reviewers (but maybe it's >> > the Xen way). >> > >> >> Changes since v8: >> >> - Used variables fifo_level/fifo_threshold for more clarity >> >> - Added a new macro SBSA_UART_FIFO_SIZE instead of using a magic number >> > >> > What's sizeof(intf->in), sizeof(intf->out)? >> > >> > For correct operation, you assume that the total ring buffer size is >= >> > SBSA_UART_FIFO_SIZE, but nothing enforces this. If the xencons ring >> > buffer size is set elsewhere and can't be chosen by a driver, you may at >> > least add a build-time assert to check that it's big enough. >> > >> I will add an assert to check this condition. >> >> > [...] >> > >> >> @@ -144,28 +148,41 @@ static void vpl011_write_data(struct domain *d, uint8_t data) >> > >> > [...] >> > >> >> + * Clear the TXI bit if the fifo level exceeds fifo_size/2 mark which >> >> + * is the trigger level for asserting/de-assterting the TX interrupt. >> >> */ >> >> - vpl011->uartfr |= BUSY; >> >> + fifo_threshold = sizeof (intf->out) - SBSA_UART_FIFO_SIZE/2; >> >> + >> >> + if ( fifo_level <= fifo_threshold ) >> >> + vpl011->uartris |= TXI; >> >> + else >> >> + vpl011->uartris &= ~TXI; >> >> } >> >> + else >> >> + gprintk(XENLOG_ERR, "vpl011: Unexpected OUT ring buffer full\n"); >> >> >> >> vpl011->uartfr &= ~TXFE; >> >> >> > >> > [...] >> > >> >> @@ -353,37 +370,51 @@ static void vpl011_data_avail(struct domain *d) >> >> >> >> smp_rmb(); >> >> >> >> - in_ring_qsize = xencons_queued(in_prod, >> >> + in_fifo_level = xencons_queued(in_prod, >> >> in_cons, >> >> sizeof(intf->in)); >> >> >> >> - out_ring_qsize = xencons_queued(out_prod, >> >> + out_fifo_level = xencons_queued(out_prod, >> >> out_cons, >> >> sizeof(intf->out)); >> >> >> >> /* Update the uart rx state if the buffer is not empty. */ >> >> - if ( in_ring_qsize != 0 ) >> >> + if ( in_fifo_level != 0 ) >> >> { >> >> vpl011->uartfr &= ~RXFE; >> >> - if ( in_ring_qsize == sizeof(intf->in) ) >> >> + >> >> + if ( in_fifo_level == sizeof(intf->in) ) >> >> vpl011->uartfr |= RXFF; >> >> + >> >> vpl011->uartris |= RXI; >> >> } >> >> >> >> /* Update the uart tx state if the buffer is not full. */ >> >> - if ( out_ring_qsize != sizeof(intf->out) ) >> >> + if ( out_fifo_level != sizeof(intf->out) ) >> >> { >> >> + unsigned int out_fifo_threshold; >> >> + >> >> vpl011->uartfr &= ~TXFF; >> >> - vpl011->uartris |= TXI; >> >> >> >> /* >> >> - * Clear the BUSY bit as soon as space becomes available >> >> + * Clear the BUSY bit as soon as space becomes avaliable >> >> * so that the SBSA UART driver can start writing more data >> >> * without any further delay. >> >> */ >> >> vpl011->uartfr &= ~BUSY; >> >> >> >> - if ( out_ring_qsize == 0 ) >> >> + /* >> >> + * Set the TXI bit only when there is space for fifo_size/2 bytes which >> >> + * is the trigger level for asserting/de-assterting the TX interrupt. >> >> + */ >> >> + out_fifo_threshold = sizeof(intf->out) - SBSA_UART_FIFO_SIZE/2; >> >> + >> >> + if ( out_fifo_level <= out_fifo_threshold ) >> >> + vpl011->uartris |= TXI; >> >> + else >> >> + vpl011->uartris &= ~TXI; >> > >> > Should this logic be factored out? You do the same thing in >> > _write_data(). >> I will add a common function to set the TXI bit. >> > >> > Also, is there a reason why you implement the trigger threshold logic on >> > the TX side only? It looks inconsistent now. >> I did try with RX FIFO threshold also but it seems the current pl011 >> driver does not >> poll on the RX FIFO and just waits for the RX interrupt trigger to >> start processing the RX data. >> This makes RX very slow and if the RX FIFO is not filled sufficiently, >> it does not read data further. >> > >> > I think a real PL011 implements the trigger logic in exactly the same >> > way for RX and TX (except for swapping >= for <= in the threshold >> > comparison). >> > >> > >> > It doesn't look like the Linux pl011 driver relies on a correctly >> > implemented RX trigger level today, but it may have done in the past -- >> > I did some hacking in this area at some point, but can't remember the >> > details now. >> > >> The current pl011 driver >> > Asserting RXI whenever the RX FIFO is nonempty would result in excessive >> > interrupts if you are streaming the data from a slow source (such as a >> > real UART) and pushing the chars one by one to the emulated UART: the >> > guest would take an IRQ on each char rather than waiting until the RX >> > FIFO is half-full. >> > >> I agree it is an overhead. This may be an issue with the driver which >> is solely depending on the RX >> interrupt. I think it should switch to polling if there are no >> interrupts received recently. > > Hmmm, good point, but isn't that what the receive timeout interrupt is > supposed to be for? > > The Linux driver seems to rely on the receive timeout interrupt > to recover from an RX stall when the FIFO is not empty but also not full > enough to trigger the RX FIFO interrupt. > > Does your driver actually implement the receive timeout interrupt? > I'm not very familiar with the code, so I may have missed it. This patch emulates the SBSA UART spec, which is a subset of the pl011 UART. The SBSA spec [1], Appendix B does not define the requirement of supporting RX timeout interrupt. [1] https://static.docs.arm.com/den0029/a/Server_Base_System_Architecture_v3_1_ARM_DEN_0029A.pdf Regards, Bhupinder
On Wed, Oct 11, 2017 at 07:21:43PM +0530, Bhupinder Thakur wrote: > On 11 October 2017 at 15:38, Dave Martin <Dave.Martin@arm.com> wrote: > > On Wed, Oct 11, 2017 at 01:28:44PM +0530, Bhupinder Thakur wrote: > >> Hi Dave, > >> > >> On 26 September 2017 at 20:08, Dave Martin <Dave.Martin@arm.com> wrote: > >> > On Fri, Sep 22, 2017 at 01:53:26PM +0530, Bhupinder Thakur wrote: > >> >> This patch fixes the issue observed when pl011 patches were tested on > >> >> the junos hardware by Andre/Julien. It was observed that when large output is > >> >> generated such as on running 'find /', output was getting truncated intermittently > >> >> due to OUT ring buffer getting full. > >> >> > >> >> This issue was due to the fact that the SBSA UART driver expects that when > >> >> a TX interrupt is asserted then the FIFO queue should be atleast half empty and > >> >> that it can write N bytes in the FIFO, where N is half the FIFO queue size, without > >> >> the bytes getting dropped due to FIFO getting full. > >> >> > >> >> The SBSA UART emulation logic was asserting the TX interrupt as soon as > >> >> any space became available in the FIFO and the SBSA UART driver tried to write > >> >> more data (upto 16 bytes) in the FIFO expecting that there is enough space > >> >> available leading to dropped bytes. > >> >> > >> >> The SBSA spec [1] does not specify when the TX interrupt should be asserted > >> >> or de-asserted. Due to lack of clarity on the expected behavior, it is > >> >> assumed for now that TX interrupt should be asserted only when the FIFO goes > >> >> half empty. > >> >> > >> >> TBD: Once the SBSA spec is updated with the expected behavior, the implementation > >> >> will be modified to align with the spec requirement. > >> >> > >> >> [1] http://infocenter.arm.com/help/topic/com.arm.doc.ddi0183f/DDI0183.pdf > >> >> > >> >> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org> > >> >> --- > >> >> CC: Julien Grall <julien.grall@arm.com> > >> >> CC: Andre Przywara <andre.przywara@arm.com> > >> >> CC: Stefano Stabellini <sstabellini@kernel.org> > >> > > >> > (Taking a quick look at this because I remember fighthing with FIFO > >> > behaviour issues when hacking the Linux driver -- but beware, I'm not a > >> > Xen guy...) > >> > > >> > > >> > Should this patch be flattened into the patches is fixes? Keeping > >> > known-wrong code in the series does not help reviewers (but maybe it's > >> > the Xen way). > >> > > >> >> Changes since v8: > >> >> - Used variables fifo_level/fifo_threshold for more clarity > >> >> - Added a new macro SBSA_UART_FIFO_SIZE instead of using a magic number > >> > > >> > What's sizeof(intf->in), sizeof(intf->out)? > >> > > >> > For correct operation, you assume that the total ring buffer size is >= > >> > SBSA_UART_FIFO_SIZE, but nothing enforces this. If the xencons ring > >> > buffer size is set elsewhere and can't be chosen by a driver, you may at > >> > least add a build-time assert to check that it's big enough. > >> > > >> I will add an assert to check this condition. > >> > >> > [...] > >> > > >> >> @@ -144,28 +148,41 @@ static void vpl011_write_data(struct domain *d, uint8_t data) > >> > > >> > [...] > >> > > >> >> + * Clear the TXI bit if the fifo level exceeds fifo_size/2 mark which > >> >> + * is the trigger level for asserting/de-assterting the TX interrupt. > >> >> */ > >> >> - vpl011->uartfr |= BUSY; > >> >> + fifo_threshold = sizeof (intf->out) - SBSA_UART_FIFO_SIZE/2; > >> >> + > >> >> + if ( fifo_level <= fifo_threshold ) > >> >> + vpl011->uartris |= TXI; > >> >> + else > >> >> + vpl011->uartris &= ~TXI; > >> >> } > >> >> + else > >> >> + gprintk(XENLOG_ERR, "vpl011: Unexpected OUT ring buffer full\n"); > >> >> > >> >> vpl011->uartfr &= ~TXFE; > >> >> > >> > > >> > [...] > >> > > >> >> @@ -353,37 +370,51 @@ static void vpl011_data_avail(struct domain *d) > >> >> > >> >> smp_rmb(); > >> >> > >> >> - in_ring_qsize = xencons_queued(in_prod, > >> >> + in_fifo_level = xencons_queued(in_prod, > >> >> in_cons, > >> >> sizeof(intf->in)); > >> >> > >> >> - out_ring_qsize = xencons_queued(out_prod, > >> >> + out_fifo_level = xencons_queued(out_prod, > >> >> out_cons, > >> >> sizeof(intf->out)); > >> >> > >> >> /* Update the uart rx state if the buffer is not empty. */ > >> >> - if ( in_ring_qsize != 0 ) > >> >> + if ( in_fifo_level != 0 ) > >> >> { > >> >> vpl011->uartfr &= ~RXFE; > >> >> - if ( in_ring_qsize == sizeof(intf->in) ) > >> >> + > >> >> + if ( in_fifo_level == sizeof(intf->in) ) > >> >> vpl011->uartfr |= RXFF; > >> >> + > >> >> vpl011->uartris |= RXI; > >> >> } > >> >> > >> >> /* Update the uart tx state if the buffer is not full. */ > >> >> - if ( out_ring_qsize != sizeof(intf->out) ) > >> >> + if ( out_fifo_level != sizeof(intf->out) ) > >> >> { > >> >> + unsigned int out_fifo_threshold; > >> >> + > >> >> vpl011->uartfr &= ~TXFF; > >> >> - vpl011->uartris |= TXI; > >> >> > >> >> /* > >> >> - * Clear the BUSY bit as soon as space becomes available > >> >> + * Clear the BUSY bit as soon as space becomes avaliable > >> >> * so that the SBSA UART driver can start writing more data > >> >> * without any further delay. > >> >> */ > >> >> vpl011->uartfr &= ~BUSY; > >> >> > >> >> - if ( out_ring_qsize == 0 ) > >> >> + /* > >> >> + * Set the TXI bit only when there is space for fifo_size/2 bytes which > >> >> + * is the trigger level for asserting/de-assterting the TX interrupt. > >> >> + */ > >> >> + out_fifo_threshold = sizeof(intf->out) - SBSA_UART_FIFO_SIZE/2; > >> >> + > >> >> + if ( out_fifo_level <= out_fifo_threshold ) > >> >> + vpl011->uartris |= TXI; > >> >> + else > >> >> + vpl011->uartris &= ~TXI; > >> > > >> > Should this logic be factored out? You do the same thing in > >> > _write_data(). > >> I will add a common function to set the TXI bit. > >> > > >> > Also, is there a reason why you implement the trigger threshold logic on > >> > the TX side only? It looks inconsistent now. > >> I did try with RX FIFO threshold also but it seems the current pl011 > >> driver does not > >> poll on the RX FIFO and just waits for the RX interrupt trigger to > >> start processing the RX data. > >> This makes RX very slow and if the RX FIFO is not filled sufficiently, > >> it does not read data further. > >> > > >> > I think a real PL011 implements the trigger logic in exactly the same > >> > way for RX and TX (except for swapping >= for <= in the threshold > >> > comparison). > >> > > >> > > >> > It doesn't look like the Linux pl011 driver relies on a correctly > >> > implemented RX trigger level today, but it may have done in the past -- > >> > I did some hacking in this area at some point, but can't remember the > >> > details now. > >> > > >> The current pl011 driver > >> > Asserting RXI whenever the RX FIFO is nonempty would result in excessive > >> > interrupts if you are streaming the data from a slow source (such as a > >> > real UART) and pushing the chars one by one to the emulated UART: the > >> > guest would take an IRQ on each char rather than waiting until the RX > >> > FIFO is half-full. > >> > > >> I agree it is an overhead. This may be an issue with the driver which > >> is solely depending on the RX > >> interrupt. I think it should switch to polling if there are no > >> interrupts received recently. > > > > Hmmm, good point, but isn't that what the receive timeout interrupt is > > supposed to be for? > > > > The Linux driver seems to rely on the receive timeout interrupt > > to recover from an RX stall when the FIFO is not empty but also not full > > enough to trigger the RX FIFO interrupt. > > > > Does your driver actually implement the receive timeout interrupt? > > I'm not very familiar with the code, so I may have missed it. > > This patch emulates the SBSA UART spec, which is a subset of the pl011 > UART. The SBSA spec [1], Appendix B does not define the requirement of > supporting RX timeout interrupt. I took another look at the SBSA spec -- it is certainly vague/ambiguous in this area. There is no statement that you must implement all the interrupts, but there is also no statement that you are allowed to not implement any of them. The RX FIFO interrupt and receive timeout interrupt are not treated differently in the spec. I think that an SBSA UART client driver _could_ legitimately assume that the RX FIFO interrupt is never asserted until the proper trigger threshold is reached, but this does not mean that there is any driver out in the wild that actually does this. A quick hack to the Linux PL011 driver suggests that polling the RXFE status before reading each character can safely be optimised away after a RX FIFO interrupt, until 16 or more chars have been read from the FIFO. This seems to work with the real PL011 on Juno r0 and the emulated PL011 in the ARM fast model. I don't plan to upstream this hack though. Probably there are already SBSA UART implementations out there that are incompatible on this. So for now, it's really a judgement call. I will raise a clarification request on the SBSA spec, but I suspect that you can get away with the current approach. It would be a good idea to add comments explaining the design decisions / shortcuts here, since this issue may come up again later. Cheers ---Dave My dodgy Linux driver hack below -- the debugfs stuff just provides a way to prove that both types of interrupt actually happen. --8<-- diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c index 111e6a9..36e00cb 100644 --- a/drivers/tty/serial/amba-pl011.c +++ b/drivers/tty/serial/amba-pl011.c @@ -59,6 +59,7 @@ #include <linux/sizes.h> #include <linux/io.h> #include <linux/acpi.h> +#include <linux/debugfs.h> #include "amba-pl011.h" @@ -73,6 +74,8 @@ #define UART_DR_ERROR (UART011_DR_OE|UART011_DR_BE|UART011_DR_PE|UART011_DR_FE) #define UART_DUMMY_DR_RX (1 << 16) +static u32 rxis_count, rtis_count; + static u16 pl011_std_offsets[REG_ARRAY_SIZE] = { [REG_DR] = UART01x_DR, [REG_FR] = UART01x_FR, @@ -325,15 +328,13 @@ static void pl011_write(unsigned int val, const struct uart_amba_port *uap, * inserts them into the TTY layer. Returns the number of characters * read from the FIFO. */ -static int pl011_fifo_to_tty(struct uart_amba_port *uap) +static int pl011_fifo_to_tty(struct uart_amba_port *uap, int avail) { - u16 status; unsigned int ch, flag, max_count = 256; int fifotaken = 0; while (max_count--) { - status = pl011_read(uap, REG_FR); - if (status & UART01x_FR_RXFE) + if (avail-- <= 0 && (pl011_read(uap, REG_FR) & UART01x_FR_RXFE)) break; /* Take chars from the FIFO and update status */ @@ -954,7 +955,7 @@ static void pl011_dma_rx_chars(struct uart_amba_port *uap, * will always find the error in the FIFO, never in the DMA * buffer. */ - fifotaken = pl011_fifo_to_tty(uap); + fifotaken = pl011_fifo_to_tty(uap, 0); } spin_unlock(&uap->port.lock); @@ -1361,11 +1362,11 @@ static void pl011_enable_ms(struct uart_port *port) pl011_write(uap->im, uap, REG_IMSC); } -static void pl011_rx_chars(struct uart_amba_port *uap) +static void pl011_rx_chars(struct uart_amba_port *uap, int avail) __releases(&uap->port.lock) __acquires(&uap->port.lock) { - pl011_fifo_to_tty(uap); + pl011_fifo_to_tty(uap, avail); spin_unlock(&uap->port.lock); tty_flip_buffer_push(&uap->port.state->port); @@ -1515,8 +1516,15 @@ static irqreturn_t pl011_int(int irq, void *dev_id) if (status & (UART011_RTIS|UART011_RXIS)) { if (pl011_dma_rx_running(uap)) pl011_dma_rx_irq(uap); - else - pl011_rx_chars(uap); + else { + if (status & UART011_RXIS) + ++rxis_count; + else + ++rtis_count; + + pl011_rx_chars(uap, + (status & UART011_RXIS) ? 16 : 1); + } } if (status & (UART011_DSRMIS|UART011_DCDMIS| UART011_CTSMIS|UART011_RIMIS)) @@ -2822,6 +2830,9 @@ static int __init pl011_init(void) { printk(KERN_INFO "Serial: AMBA PL011 UART driver\n"); + debugfs_create_u32("pl011-rxis", 0444, NULL, &rxis_count); + debugfs_create_u32("pl011-rtis", 0444, NULL, &rtis_count); + if (platform_driver_register(&arm_sbsa_uart_platform_driver)) pr_warn("could not register SBSA UART platform driver\n"); return amba_driver_register(&pl011_driver);
diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c index 36794d8..1f97261 100644 --- a/xen/arch/arm/vpl011.c +++ b/xen/arch/arm/vpl011.c @@ -91,20 +91,24 @@ static uint8_t vpl011_read_data(struct domain *d) */ if ( xencons_queued(in_prod, in_cons, sizeof(intf->in)) > 0 ) { + unsigned int fifo_level; + data = intf->in[xencons_mask(in_cons, sizeof(intf->in))]; in_cons += 1; smp_mb(); intf->in_cons = in_cons; + + fifo_level = xencons_queued(in_prod, in_cons, sizeof(intf->in)); + + if ( fifo_level == 0 ) + { + vpl011->uartfr |= RXFE; + vpl011->uartris &= ~RXI; + } } else gprintk(XENLOG_ERR, "vpl011: Unexpected IN ring buffer empty\n"); - if ( xencons_queued(in_prod, in_cons, sizeof(intf->in)) == 0 ) - { - vpl011->uartfr |= RXFE; - vpl011->uartris &= ~RXI; - } - vpl011->uartfr &= ~RXFF; vpl011_update_interrupt_status(d); @@ -144,28 +148,41 @@ static void vpl011_write_data(struct domain *d, uint8_t data) if ( xencons_queued(out_prod, out_cons, sizeof(intf->out)) != sizeof (intf->out) ) { + unsigned int fifo_level, fifo_threshold; + intf->out[xencons_mask(out_prod, sizeof(intf->out))] = data; out_prod += 1; smp_wmb(); intf->out_prod = out_prod; - } - else - gprintk(XENLOG_ERR, "vpl011: Unexpected OUT ring buffer full\n"); - if ( xencons_queued(out_prod, out_cons, sizeof(intf->out)) == - sizeof (intf->out) ) - { - vpl011->uartfr |= TXFF; - vpl011->uartris &= ~TXI; + fifo_level = xencons_queued(out_prod, out_cons, sizeof(intf->out)); + + if ( fifo_level == sizeof (intf->out) ) + { + vpl011->uartfr |= TXFF; + + /* + * 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; + } /* - * 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. + * Clear the TXI bit if the fifo level exceeds fifo_size/2 mark which + * is the trigger level for asserting/de-assterting the TX interrupt. */ - vpl011->uartfr |= BUSY; + fifo_threshold = sizeof (intf->out) - SBSA_UART_FIFO_SIZE/2; + + if ( fifo_level <= fifo_threshold ) + vpl011->uartris |= TXI; + else + vpl011->uartris &= ~TXI; } + else + gprintk(XENLOG_ERR, "vpl011: Unexpected OUT ring buffer full\n"); vpl011->uartfr &= ~TXFE; @@ -342,7 +359,7 @@ static void vpl011_data_avail(struct domain *d) struct vpl011 *vpl011 = &d->arch.vpl011; struct xencons_interface *intf = vpl011->ring_buf; XENCONS_RING_IDX in_cons, in_prod, out_cons, out_prod; - XENCONS_RING_IDX in_ring_qsize, out_ring_qsize; + XENCONS_RING_IDX in_fifo_level, out_fifo_level; VPL011_LOCK(d, flags); @@ -353,37 +370,51 @@ static void vpl011_data_avail(struct domain *d) smp_rmb(); - in_ring_qsize = xencons_queued(in_prod, + in_fifo_level = xencons_queued(in_prod, in_cons, sizeof(intf->in)); - out_ring_qsize = xencons_queued(out_prod, + out_fifo_level = xencons_queued(out_prod, out_cons, sizeof(intf->out)); /* Update the uart rx state if the buffer is not empty. */ - if ( in_ring_qsize != 0 ) + if ( in_fifo_level != 0 ) { vpl011->uartfr &= ~RXFE; - if ( in_ring_qsize == sizeof(intf->in) ) + + if ( in_fifo_level == sizeof(intf->in) ) vpl011->uartfr |= RXFF; + vpl011->uartris |= RXI; } /* Update the uart tx state if the buffer is not full. */ - if ( out_ring_qsize != sizeof(intf->out) ) + if ( out_fifo_level != sizeof(intf->out) ) { + unsigned int out_fifo_threshold; + vpl011->uartfr &= ~TXFF; - vpl011->uartris |= TXI; /* - * Clear the BUSY bit as soon as space becomes available + * Clear the BUSY bit as soon as space becomes avaliable * so that the SBSA UART driver can start writing more data * without any further delay. */ vpl011->uartfr &= ~BUSY; - if ( out_ring_qsize == 0 ) + /* + * Set the TXI bit only when there is space for fifo_size/2 bytes which + * is the trigger level for asserting/de-assterting the TX interrupt. + */ + out_fifo_threshold = sizeof(intf->out) - SBSA_UART_FIFO_SIZE/2; + + if ( out_fifo_level <= out_fifo_threshold ) + vpl011->uartris |= TXI; + else + vpl011->uartris &= ~TXI; + + if ( out_fifo_level == 0 ) vpl011->uartfr |= TXFE; } diff --git a/xen/include/asm-arm/vpl011.h b/xen/include/asm-arm/vpl011.h index 1b583da..db95ff8 100644 --- a/xen/include/asm-arm/vpl011.h +++ b/xen/include/asm-arm/vpl011.h @@ -28,6 +28,8 @@ #define VPL011_LOCK(d,flags) spin_lock_irqsave(&(d)->arch.vpl011.lock, flags) #define VPL011_UNLOCK(d,flags) spin_unlock_irqrestore(&(d)->arch.vpl011.lock, flags) +#define SBSA_UART_FIFO_SIZE 32 + struct vpl011 { void *ring_buf; struct page_info *ring_page;
This patch fixes the issue observed when pl011 patches were tested on the junos hardware by Andre/Julien. It was observed that when large output is generated such as on running 'find /', output was getting truncated intermittently due to OUT ring buffer getting full. This issue was due to the fact that the SBSA UART driver expects that when a TX interrupt is asserted then the FIFO queue should be atleast half empty and that it can write N bytes in the FIFO, where N is half the FIFO queue size, without the bytes getting dropped due to FIFO getting full. The SBSA UART emulation logic was asserting the TX interrupt as soon as any space became available in the FIFO and the SBSA UART driver tried to write more data (upto 16 bytes) in the FIFO expecting that there is enough space available leading to dropped bytes. The SBSA spec [1] does not specify when the TX interrupt should be asserted or de-asserted. Due to lack of clarity on the expected behavior, it is assumed for now that TX interrupt should be asserted only when the FIFO goes half empty. TBD: Once the SBSA spec is updated with the expected behavior, the implementation will be modified to align with the spec requirement. [1] http://infocenter.arm.com/help/topic/com.arm.doc.ddi0183f/DDI0183.pdf Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org> --- CC: Julien Grall <julien.grall@arm.com> CC: Andre Przywara <andre.przywara@arm.com> CC: Stefano Stabellini <sstabellini@kernel.org> Changes since v8: - Used variables fifo_level/fifo_threshold for more clarity - Added a new macro SBSA_UART_FIFO_SIZE instead of using a magic number - Renamed ring_qsize variables to fifo_level for consistency xen/arch/arm/vpl011.c | 87 ++++++++++++++++++++++++++++++-------------- xen/include/asm-arm/vpl011.h | 2 + 2 files changed, 61 insertions(+), 28 deletions(-)