| Message ID | 1507891231-4386-2-git-send-email-bhupinder.thakur@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Oct 13, 2017 at 04:10:31PM +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> > --- [...] > diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c [...] > @@ -355,28 +382,46 @@ static void vpl011_data_avail(struct domain *d) [...] > + /* > + * Currently, the RXI bit is getting set even if there is a single > + * byte of data in the rx fifo. Ideally, the RXI bit should be set > + * only if the rx fifo level reaches the threshold. > + * > + * However, since currently RX timeout interrupt is not > + * implemented as there is not enough clarity in the SBSA spec, > + * the guest may keep waiting for an interrupt to read more > + * data. To ensure that guest reads all the data without > + * any delay, the RXI interrupt is raised if there is RX data > + * available without checking whether fifo level has reached > + * the threshold. > + * > + * TBD: Once there is more clarity in the SBSA spec on whether RX > + * timeout interrupt needs to be implemented, the RXI interrupt > + * will be raised only when rx fifo level reaches the threshold. > + */ This looks OK to me: it makes the issues clear to future maintainers of this code. [...] Cheers ---Dave
Hi, On 13/10/17 11:40, 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. So similar to the other patch: - I can confirm that this patch fixes the dropped characters issue we see with current staging HEAD. And, differently from the first patch, this one fixes a correctness issue (we are loosing characters at the moment) rather than just a performance problem. So I think we definitely need something along those lines. However ... ;-) Asserting the receive interrupt at the first character, while it is architected to be only triggered at half the FIFO level, is not right. Instead what we probably want it to use the timeout interrupt instead. I quickly hacked something up like that: - In vpl011_data_avail() we assert the timeout interrupt (RTI) if the in-FIFO is not empty. This is following the idea that when this function is called, Xen says: this is all the data I have at the moment. The guest should be able to see the data, because Xen has no idea when and if more data will come in. - If we drain the in-FIFO in vpl011_mmio_read() (fifo_level becomes 0), we clear RTI. - We handle RXI like described in the spec: assert it in data_avail() if the FIFO has 16 or less characters left, clear it in mmio_read() if the FIFO has space for more than 16 characters. This basically moves the trick of asserting RXI to asserting RTI instead, which sounds architecturally cleaner. Let me try to clean up my approach and post it. Cheers, Andre. > > [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> > CC: Dave Martin <dave.martin@arm.com> > > Changes since v11: > - Add a build assert to check that ring buffer size is more than minimum rx fif size of 32 > - Added a comment to explain why threshold based logic is not implemented for rx fifo > - Moved calls to vpl011_update_interrupt_status() near where TXI/RXI status bit is set > > 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 | 113 ++++++++++++++++++++++++++++++------------- > xen/include/asm-arm/vpl011.h | 2 + > 2 files changed, 82 insertions(+), 33 deletions(-) > > diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c > index 0b07436..adf1711 100644 > --- a/xen/arch/arm/vpl011.c > +++ b/xen/arch/arm/vpl011.c > @@ -93,24 +93,27 @@ 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; > + vpl011_update_interrupt_status(d); > + } > } > 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); > - > VPL011_UNLOCK(d, flags); > > /* > @@ -122,6 +125,26 @@ static uint8_t vpl011_read_data(struct domain *d) > return data; > } > > +static void vpl011_update_tx_fifo_status(struct vpl011 *vpl011, > + unsigned int fifo_level) > +{ > + struct xencons_interface *intf = vpl011->ring_buf; > + unsigned int fifo_threshold; > + > + BUILD_BUG_ON(sizeof (intf->out) < SBSA_UART_FIFO_SIZE); > + > + /* > + * 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. > + */ > + fifo_threshold = sizeof(intf->out) - SBSA_UART_FIFO_SIZE/2; > + > + if ( fifo_level <= fifo_threshold ) > + vpl011->uartris |= TXI; > + else > + vpl011->uartris &= ~TXI; > +} > + > static void vpl011_write_data(struct domain *d, uint8_t data) > { > unsigned long flags; > @@ -146,33 +169,37 @@ 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; > + > 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)); > > - /* > - * 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; > + 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; > + } > + > + vpl011_update_tx_fifo_status(vpl011, fifo_level); > + > + vpl011_update_interrupt_status(d); > } > + else > + gprintk(XENLOG_ERR, "vpl011: Unexpected OUT ring buffer full\n"); > > vpl011->uartfr &= ~TXFE; > > - vpl011_update_interrupt_status(d); > - > VPL011_UNLOCK(d, flags); > > /* > @@ -344,7 +371,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); > > @@ -355,28 +382,46 @@ 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; > + > + /* > + * Currently, the RXI bit is getting set even if there is a single > + * byte of data in the rx fifo. Ideally, the RXI bit should be set > + * only if the rx fifo level reaches the threshold. > + * > + * However, since currently RX timeout interrupt is not > + * implemented as there is not enough clarity in the SBSA spec, > + * the guest may keep waiting for an interrupt to read more > + * data. To ensure that guest reads all the data without > + * any delay, the RXI interrupt is raised if there is RX data > + * available without checking whether fifo level has reached > + * the threshold. > + * > + * TBD: Once there is more clarity in the SBSA spec on whether RX > + * timeout interrupt needs to be implemented, the RXI interrupt > + * will be raised only when rx fifo level reaches the threshold. > + */ > 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) ) > { > vpl011->uartfr &= ~TXFF; > - vpl011->uartris |= TXI; > > /* > * Clear the BUSY bit as soon as space becomes available > @@ -385,7 +430,9 @@ static void vpl011_data_avail(struct domain *d) > */ > vpl011->uartfr &= ~BUSY; > > - if ( out_ring_qsize == 0 ) > + vpl011_update_tx_fifo_status(vpl011, out_fifo_level); > + > + 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; >
On 18 October 2017 at 15:56, Andre Przywara <andre.przywara@arm.com> wrote: > Hi, > > On 13/10/17 11:40, 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. > > So similar to the other patch: > > - I can confirm that this patch fixes the dropped characters issue we > see with current staging HEAD. And, differently from the first patch, > this one fixes a correctness issue (we are loosing characters at the > moment) rather than just a performance problem. So I think we definitely > need something along those lines. > > However ... ;-) > Asserting the receive interrupt at the first character, while it is > architected to be only triggered at half the FIFO level, is not right. > Instead what we probably want it to use the timeout interrupt instead. I > quickly hacked something up like that: > - In vpl011_data_avail() we assert the timeout interrupt (RTI) if the > in-FIFO is not empty. This is following the idea that when this function > is called, Xen says: this is all the data I have at the moment. The > guest should be able to see the data, because Xen has no idea when and > if more data will come in. > - If we drain the in-FIFO in vpl011_mmio_read() (fifo_level becomes 0), > we clear RTI. > - We handle RXI like described in the spec: assert it in data_avail() if > the FIFO has 16 or less characters left, clear it in mmio_read() if the > FIFO has space for more than 16 characters. I think you meant - RXI should be asserted when FIFO has 16 or more characters left. > > This basically moves the trick of asserting RXI to asserting RTI > instead, which sounds architecturally cleaner. > > Let me try to clean up my approach and post it. > > Cheers, > Andre. > > > >> >> [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> >> CC: Dave Martin <dave.martin@arm.com> >> >> Changes since v11: >> - Add a build assert to check that ring buffer size is more than minimum rx fif size of 32 >> - Added a comment to explain why threshold based logic is not implemented for rx fifo >> - Moved calls to vpl011_update_interrupt_status() near where TXI/RXI status bit is set >> >> 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 | 113 ++++++++++++++++++++++++++++++------------- >> xen/include/asm-arm/vpl011.h | 2 + >> 2 files changed, 82 insertions(+), 33 deletions(-) >> >> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c >> index 0b07436..adf1711 100644 >> --- a/xen/arch/arm/vpl011.c >> +++ b/xen/arch/arm/vpl011.c >> @@ -93,24 +93,27 @@ 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; >> + vpl011_update_interrupt_status(d); >> + } >> } >> 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); >> - >> VPL011_UNLOCK(d, flags); >> >> /* >> @@ -122,6 +125,26 @@ static uint8_t vpl011_read_data(struct domain *d) >> return data; >> } >> >> +static void vpl011_update_tx_fifo_status(struct vpl011 *vpl011, >> + unsigned int fifo_level) >> +{ >> + struct xencons_interface *intf = vpl011->ring_buf; >> + unsigned int fifo_threshold; >> + >> + BUILD_BUG_ON(sizeof (intf->out) < SBSA_UART_FIFO_SIZE); >> + >> + /* >> + * 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. >> + */ >> + fifo_threshold = sizeof(intf->out) - SBSA_UART_FIFO_SIZE/2; >> + >> + if ( fifo_level <= fifo_threshold ) >> + vpl011->uartris |= TXI; >> + else >> + vpl011->uartris &= ~TXI; >> +} >> + >> static void vpl011_write_data(struct domain *d, uint8_t data) >> { >> unsigned long flags; >> @@ -146,33 +169,37 @@ 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; >> + >> 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)); >> >> - /* >> - * 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; >> + 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; >> + } >> + >> + vpl011_update_tx_fifo_status(vpl011, fifo_level); >> + >> + vpl011_update_interrupt_status(d); >> } >> + else >> + gprintk(XENLOG_ERR, "vpl011: Unexpected OUT ring buffer full\n"); >> >> vpl011->uartfr &= ~TXFE; >> >> - vpl011_update_interrupt_status(d); >> - >> VPL011_UNLOCK(d, flags); >> >> /* >> @@ -344,7 +371,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); >> >> @@ -355,28 +382,46 @@ 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; >> + >> + /* >> + * Currently, the RXI bit is getting set even if there is a single >> + * byte of data in the rx fifo. Ideally, the RXI bit should be set >> + * only if the rx fifo level reaches the threshold. >> + * >> + * However, since currently RX timeout interrupt is not >> + * implemented as there is not enough clarity in the SBSA spec, >> + * the guest may keep waiting for an interrupt to read more >> + * data. To ensure that guest reads all the data without >> + * any delay, the RXI interrupt is raised if there is RX data >> + * available without checking whether fifo level has reached >> + * the threshold. >> + * >> + * TBD: Once there is more clarity in the SBSA spec on whether RX >> + * timeout interrupt needs to be implemented, the RXI interrupt >> + * will be raised only when rx fifo level reaches the threshold. >> + */ >> 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) ) >> { >> vpl011->uartfr &= ~TXFF; >> - vpl011->uartris |= TXI; >> >> /* >> * Clear the BUSY bit as soon as space becomes available >> @@ -385,7 +430,9 @@ static void vpl011_data_avail(struct domain *d) >> */ >> vpl011->uartfr &= ~BUSY; >> >> - if ( out_ring_qsize == 0 ) >> + vpl011_update_tx_fifo_status(vpl011, out_fifo_level); >> + >> + 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; >>
diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c index 0b07436..adf1711 100644 --- a/xen/arch/arm/vpl011.c +++ b/xen/arch/arm/vpl011.c @@ -93,24 +93,27 @@ 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; + vpl011_update_interrupt_status(d); + } } 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); - VPL011_UNLOCK(d, flags); /* @@ -122,6 +125,26 @@ static uint8_t vpl011_read_data(struct domain *d) return data; } +static void vpl011_update_tx_fifo_status(struct vpl011 *vpl011, + unsigned int fifo_level) +{ + struct xencons_interface *intf = vpl011->ring_buf; + unsigned int fifo_threshold; + + BUILD_BUG_ON(sizeof (intf->out) < SBSA_UART_FIFO_SIZE); + + /* + * 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. + */ + fifo_threshold = sizeof(intf->out) - SBSA_UART_FIFO_SIZE/2; + + if ( fifo_level <= fifo_threshold ) + vpl011->uartris |= TXI; + else + vpl011->uartris &= ~TXI; +} + static void vpl011_write_data(struct domain *d, uint8_t data) { unsigned long flags; @@ -146,33 +169,37 @@ 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; + 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)); - /* - * 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; + 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; + } + + vpl011_update_tx_fifo_status(vpl011, fifo_level); + + vpl011_update_interrupt_status(d); } + else + gprintk(XENLOG_ERR, "vpl011: Unexpected OUT ring buffer full\n"); vpl011->uartfr &= ~TXFE; - vpl011_update_interrupt_status(d); - VPL011_UNLOCK(d, flags); /* @@ -344,7 +371,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); @@ -355,28 +382,46 @@ 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; + + /* + * Currently, the RXI bit is getting set even if there is a single + * byte of data in the rx fifo. Ideally, the RXI bit should be set + * only if the rx fifo level reaches the threshold. + * + * However, since currently RX timeout interrupt is not + * implemented as there is not enough clarity in the SBSA spec, + * the guest may keep waiting for an interrupt to read more + * data. To ensure that guest reads all the data without + * any delay, the RXI interrupt is raised if there is RX data + * available without checking whether fifo level has reached + * the threshold. + * + * TBD: Once there is more clarity in the SBSA spec on whether RX + * timeout interrupt needs to be implemented, the RXI interrupt + * will be raised only when rx fifo level reaches the threshold. + */ 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) ) { vpl011->uartfr &= ~TXFF; - vpl011->uartris |= TXI; /* * Clear the BUSY bit as soon as space becomes available @@ -385,7 +430,9 @@ static void vpl011_data_avail(struct domain *d) */ vpl011->uartfr &= ~BUSY; - if ( out_ring_qsize == 0 ) + vpl011_update_tx_fifo_status(vpl011, out_fifo_level); + + 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> CC: Dave Martin <dave.martin@arm.com> Changes since v11: - Add a build assert to check that ring buffer size is more than minimum rx fif size of 32 - Added a comment to explain why threshold based logic is not implemented for rx fifo - Moved calls to vpl011_update_interrupt_status() near where TXI/RXI status bit is set 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 | 113 ++++++++++++++++++++++++++++++------------- xen/include/asm-arm/vpl011.h | 2 + 2 files changed, 82 insertions(+), 33 deletions(-)