[PATCH 26/27 v12] arm/xen: vpl011: Fix the slow early console SBSA UART output

[PATCH 26/27 v12] arm/xen: vpl011: Fix the slow early console SBSA UART output

[Bhupinder Thakur]

The early console output uses pl011_early_write() to write data. This
function waits for BUSY bit to get cleared before writing the next byte.

In the SBSA UART emulation logic, the BUSY bit was set as soon one
byte was written in the FIFO and it remained set until the FIFO was
emptied. This meant that the output was delayed as each character needed
the BUSY to get cleared.

Since the SBSA UART is getting emulated in Xen using ring buffers, it
ensures that once the data is enqueued in the FIFO, it will be received
by xenconsole so it is safe to set the BUSY bit only when FIFO becomes
full. This will ensure that pl011_early_write() is not delayed unduly
to write the data.

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>

 xen/arch/arm/vpl011.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
index f7ddccb..0b07436 100644
--- a/xen/arch/arm/vpl011.c
+++ b/xen/arch/arm/vpl011.c
@@ -159,9 +159,15 @@ static void vpl011_write_data(struct domain *d, uint8_t data)
     {
         vpl011->uartfr |= TXFF;
         vpl011->uartris &= ~TXI;
-    }
 
-    vpl011->uartfr |= BUSY;
+        /*
+         * This bit is set only when FIFO becomes full. This ensures that
+         * the SBSA UART driver can write the early console data as fast as
+         * possible, without waiting for the BUSY bit to get cleared before
+         * writing each byte.
+         */
+        vpl011->uartfr |= BUSY;
+    }
 
     vpl011->uartfr &= ~TXFE;
 
@@ -371,11 +377,16 @@ static void vpl011_data_avail(struct domain *d)
     {
         vpl011->uartfr &= ~TXFF;
         vpl011->uartris |= TXI;
+
+        /*
+         * Clear the BUSY bit as soon as space becomes available
+         * so that the SBSA UART driver can start writing more data
+         * without any further delay.
+         */
+        vpl011->uartfr &= ~BUSY;
+
         if ( out_ring_qsize == 0 )
-        {
-            vpl011->uartfr &= ~BUSY;
             vpl011->uartfr |= TXFE;
-        }
     }
 
     vpl011_update_interrupt_status(d);
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 27/27 v12] arm/xen: vpl011: Correct the logic for asserting/de-asserting SBSA UART TX interrupt

[Bhupinder Thakur]

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(-)

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;
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 27/27 v12] arm/xen: vpl011: Correct the logic for asserting/de-asserting SBSA UART TX interrupt

[Dave Martin]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 26/27 v12] arm/xen: vpl011: Fix the slow early console SBSA UART output

[Andre Przywara]

Hi Bhupinder,

first thing: As the bulk of the series has been merged now, please
restart your patch and version numbering, so a (potential) next post
should be prefixed [PATCH v3 1/2]. And please have a cover letter giving
a brief overview what this series fixes.

On 13/10/17 11:40, Bhupinder Thakur wrote:
> The early console output uses pl011_early_write() to write data. This
> function waits for BUSY bit to get cleared before writing the next byte.

... which is questionable given the actual definition of the BUSY bit in
the PL011 TRM:

============
.... The BUSY signal goes HIGH as soon as data is written to the
transmit FIFO (that is, the FIFO is non-empty) and remains asserted
HIGH while data is being transmitted. BUSY is negated only when the
transmit FIFO is empty, and the last character has been transmitted from
the shift register, ....
============

(I take it you are talking about the Linux driver in a guest here).
I think the early_write routine tries to (deliberately?) ignore the
FIFO, possibly to make sure characters really get pushed out before a
system crashes, maybe.

> 
> In the SBSA UART emulation logic, the BUSY bit was set as soon one
> byte was written in the FIFO and it remained set until the FIFO was
> emptied.

Which is correct behaviour, as this matches the PL011 TRM as quoted above.

> This meant that the output was delayed as each character needed
> the BUSY to get cleared.

But this is true as well!

> Since the SBSA UART is getting emulated in Xen using ring buffers, it
> ensures that once the data is enqueued in the FIFO, it will be received
> by xenconsole so it is safe to set the BUSY bit only when FIFO becomes
> full. This will ensure that pl011_early_write() is not delayed unduly
> to write the data.

So I can confirm that this patch fixes the very slow earlycon output
observed with the current staging HEAD.

So while this is somewhat deviating from the spec, I can see the benefit
for an emulation scenario. I believe that emulations in general might
choose implementing things a bit differently, to cope with the
fundamental differences in their environment, like the virtually endless
"FIFO" and the lack of any timing restrictions on the emulated "wire".

So unless someone comes up with a better solution, I would support
taking this patch, as this fixes a real problem.

Cheers,
Andre

> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
> ---
> CC: Julien Grall <julien.grall@arm.com>
> CC: Andre Przywara <andre.przywara@arm.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> 
>  xen/arch/arm/vpl011.c | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
> index f7ddccb..0b07436 100644
> --- a/xen/arch/arm/vpl011.c
> +++ b/xen/arch/arm/vpl011.c
> @@ -159,9 +159,15 @@ static void vpl011_write_data(struct domain *d, uint8_t data)
>      {
>          vpl011->uartfr |= TXFF;
>          vpl011->uartris &= ~TXI;
> -    }
>  
> -    vpl011->uartfr |= BUSY;
> +        /*
> +         * This bit is set only when FIFO becomes full. This ensures that
> +         * the SBSA UART driver can write the early console data as fast as
> +         * possible, without waiting for the BUSY bit to get cleared before
> +         * writing each byte.
> +         */
> +        vpl011->uartfr |= BUSY;
> +    }
>  
>      vpl011->uartfr &= ~TXFE;
>  
> @@ -371,11 +377,16 @@ static void vpl011_data_avail(struct domain *d)
>      {
>          vpl011->uartfr &= ~TXFF;
>          vpl011->uartris |= TXI;
> +
> +        /*
> +         * Clear the BUSY bit as soon as space becomes available
> +         * so that the SBSA UART driver can start writing more data
> +         * without any further delay.
> +         */
> +        vpl011->uartfr &= ~BUSY;
> +
>          if ( out_ring_qsize == 0 )
> -        {
> -            vpl011->uartfr &= ~BUSY;
>              vpl011->uartfr |= TXFE;
> -        }
>      }
>  
>      vpl011_update_interrupt_status(d);
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 26/27 v12] arm/xen: vpl011: Fix the slow early console SBSA UART output

[Dave Martin]

On Tue, Oct 17, 2017 at 10:51:07AM +0100, Andre Przywara wrote:
> Hi Bhupinder,
> 
> first thing: As the bulk of the series has been merged now, please
> restart your patch and version numbering, so a (potential) next post
> should be prefixed [PATCH v3 1/2]. And please have a cover letter giving
> a brief overview what this series fixes.
> 
> On 13/10/17 11:40, Bhupinder Thakur wrote:
> > The early console output uses pl011_early_write() to write data. This
> > function waits for BUSY bit to get cleared before writing the next byte.
> 
> ... which is questionable given the actual definition of the BUSY bit in
> the PL011 TRM:
> 
> ============
> .... The BUSY signal goes HIGH as soon as data is written to the
> transmit FIFO (that is, the FIFO is non-empty) and remains asserted
> HIGH while data is being transmitted. BUSY is negated only when the
> transmit FIFO is empty, and the last character has been transmitted from
> the shift register, ....
> ============
> 
> (I take it you are talking about the Linux driver in a guest here).
> I think the early_write routine tries to (deliberately?) ignore the
> FIFO, possibly to make sure characters really get pushed out before a
> system crashes, maybe.
> 
> > 
> > In the SBSA UART emulation logic, the BUSY bit was set as soon one
> > byte was written in the FIFO and it remained set until the FIFO was
> > emptied.
> 
> Which is correct behaviour, as this matches the PL011 TRM as quoted above.
> 
> > This meant that the output was delayed as each character needed
> > the BUSY to get cleared.
> 
> But this is true as well!
> 
> > Since the SBSA UART is getting emulated in Xen using ring buffers, it
> > ensures that once the data is enqueued in the FIFO, it will be received
> > by xenconsole so it is safe to set the BUSY bit only when FIFO becomes
> > full. This will ensure that pl011_early_write() is not delayed unduly
> > to write the data.
> 
> So I can confirm that this patch fixes the very slow earlycon output
> observed with the current staging HEAD.
> 
> So while this is somewhat deviating from the spec, I can see the benefit
> for an emulation scenario. I believe that emulations in general might
> choose implementing things a bit differently, to cope with the
> fundamental differences in their environment, like the virtually endless
> "FIFO" and the lack of any timing restrictions on the emulated "wire".
> 
> So unless someone comes up with a better solution, I would support
> taking this patch, as this fixes a real problem.

I think you get away with this, but it does violate the spec in order
to work around a feature of a correctly implemented driver.

Software can now see this, for example:

	uart_write(ch, UARTDR);
	busy = uart_read(UARTFR) & UARTFR_BUSY;
	BUG_ON(!(uart_read(UARTFR) & UARTFR_TXFE) && !busy);

which violates the spec, though I can't currently think of a good reason
for software to rely on that.


[+Rob, who wrote the original earlycon code in the amba-pl011 driver:
0d3c673e7881 ("tty/serial: pl011: add generic earlycon support")

Is there any actualy reason why we poll for !BUSY after each char in
pl011_putc()?  pl011_putc() is not exposed at all: it's only called by
pl011_console_write().

This will result in stuttering output even on hardware, though this
doesn't typically matter.

I think if the poll for !BUSY were moved to the end of
pl011_console_write(), the effect would be much less bad.]



For Xen vpl011:

This is a software emulation, so characters really are transmitted
instantaneously.

Can you drain the TX FIFO into the ring buffer immediately in
vpl011_write_data()?  Then you could always set TXFE and clear BUSY on
return to the guest, unless there is backpressure from the ring buffer.

Note that this could break with Linux versions prior to v4.1, which used
some trickery to assert the TX FIFO interrupt for the first time (see
734745caeb9f ("serial/amba-pl011: Activate TX IRQ passively")).
However, there is a subtlety in the way a real PL011 asserts the
TX/RX threshold interrupts which I think you don't implement in vpl011,
so you probably won't hit this problem in practice.  You could test with
an older kernel to find out -- but if <v4.1 kernels don't work with
current Xen for other reasons then this doesn't matter.

Cheers
---Dave

> > 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>
> > 
> >  xen/arch/arm/vpl011.c | 21 ++++++++++++++++-----
> >  1 file changed, 16 insertions(+), 5 deletions(-)
> > 
> > diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
> > index f7ddccb..0b07436 100644
> > --- a/xen/arch/arm/vpl011.c
> > +++ b/xen/arch/arm/vpl011.c
> > @@ -159,9 +159,15 @@ static void vpl011_write_data(struct domain *d, uint8_t data)
> >      {
> >          vpl011->uartfr |= TXFF;
> >          vpl011->uartris &= ~TXI;
> > -    }
> >  
> > -    vpl011->uartfr |= BUSY;
> > +        /*
> > +         * This bit is set only when FIFO becomes full. This ensures that
> > +         * the SBSA UART driver can write the early console data as fast as
> > +         * possible, without waiting for the BUSY bit to get cleared before
> > +         * writing each byte.
> > +         */
> > +        vpl011->uartfr |= BUSY;
> > +    }
> >  
> >      vpl011->uartfr &= ~TXFE;
> >  
> > @@ -371,11 +377,16 @@ static void vpl011_data_avail(struct domain *d)
> >      {
> >          vpl011->uartfr &= ~TXFF;
> >          vpl011->uartris |= TXI;
> > +
> > +        /*
> > +         * Clear the BUSY bit as soon as space becomes available
> > +         * so that the SBSA UART driver can start writing more data
> > +         * without any further delay.
> > +         */
> > +        vpl011->uartfr &= ~BUSY;
> > +
> >          if ( out_ring_qsize == 0 )
> > -        {
> > -            vpl011->uartfr &= ~BUSY;
> >              vpl011->uartfr |= TXFE;
> > -        }
> >      }
> >  
> >      vpl011_update_interrupt_status(d);
> > 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 26/27 v12] arm/xen: vpl011: Fix the slow early console SBSA UART output

[Rob Herring]

On Tue, Oct 17, 2017 at 6:19 AM, Dave Martin <Dave.Martin@arm.com> wrote:
> On Tue, Oct 17, 2017 at 10:51:07AM +0100, Andre Przywara wrote:
>> Hi Bhupinder,
>>
>> first thing: As the bulk of the series has been merged now, please
>> restart your patch and version numbering, so a (potential) next post
>> should be prefixed [PATCH v3 1/2]. And please have a cover letter giving
>> a brief overview what this series fixes.
>>
>> On 13/10/17 11:40, Bhupinder Thakur wrote:
>> > The early console output uses pl011_early_write() to write data. This
>> > function waits for BUSY bit to get cleared before writing the next byte.
>>
>> ... which is questionable given the actual definition of the BUSY bit in
>> the PL011 TRM:
>>
>> ============
>> .... The BUSY signal goes HIGH as soon as data is written to the
>> transmit FIFO (that is, the FIFO is non-empty) and remains asserted
>> HIGH while data is being transmitted. BUSY is negated only when the
>> transmit FIFO is empty, and the last character has been transmitted from
>> the shift register, ....
>> ============
>>
>> (I take it you are talking about the Linux driver in a guest here).
>> I think the early_write routine tries to (deliberately?) ignore the
>> FIFO, possibly to make sure characters really get pushed out before a
>> system crashes, maybe.
>>
>> >
>> > In the SBSA UART emulation logic, the BUSY bit was set as soon one
>> > byte was written in the FIFO and it remained set until the FIFO was
>> > emptied.
>>
>> Which is correct behaviour, as this matches the PL011 TRM as quoted above.
>>
>> > This meant that the output was delayed as each character needed
>> > the BUSY to get cleared.
>>
>> But this is true as well!
>>
>> > Since the SBSA UART is getting emulated in Xen using ring buffers, it
>> > ensures that once the data is enqueued in the FIFO, it will be received
>> > by xenconsole so it is safe to set the BUSY bit only when FIFO becomes
>> > full. This will ensure that pl011_early_write() is not delayed unduly
>> > to write the data.
>>
>> So I can confirm that this patch fixes the very slow earlycon output
>> observed with the current staging HEAD.
>>
>> So while this is somewhat deviating from the spec, I can see the benefit
>> for an emulation scenario. I believe that emulations in general might
>> choose implementing things a bit differently, to cope with the
>> fundamental differences in their environment, like the virtually endless
>> "FIFO" and the lack of any timing restrictions on the emulated "wire".
>>
>> So unless someone comes up with a better solution, I would support
>> taking this patch, as this fixes a real problem.
>
> I think you get away with this, but it does violate the spec in order
> to work around a feature of a correctly implemented driver.
>
> Software can now see this, for example:
>
>         uart_write(ch, UARTDR);
>         busy = uart_read(UARTFR) & UARTFR_BUSY;
>         BUG_ON(!(uart_read(UARTFR) & UARTFR_TXFE) && !busy);
>
> which violates the spec, though I can't currently think of a good reason
> for software to rely on that.
>
>
> [+Rob, who wrote the original earlycon code in the amba-pl011 driver:
> 0d3c673e7881 ("tty/serial: pl011: add generic earlycon support")
>
> Is there any actualy reason why we poll for !BUSY after each char in
> pl011_putc()?  pl011_putc() is not exposed at all: it's only called by
> pl011_console_write().
>
> This will result in stuttering output even on hardware, though this
> doesn't typically matter.
>
> I think if the poll for !BUSY were moved to the end of
> pl011_console_write(), the effect would be much less bad.]

I just copied the code from the arm64 earlyprintk code... Maybe it was
because on simulation (which was the main platform at the time) folks
wanted the character "on the wire". It seems to be that you could just
drop it.

Rob

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 26/27 v12] arm/xen: vpl011: Fix the slow early console SBSA UART output

[Dave Martin]

On Tue, Oct 17, 2017 at 07:53:36AM -0500, Rob Herring wrote:
> On Tue, Oct 17, 2017 at 6:19 AM, Dave Martin <Dave.Martin@arm.com> wrote:
> > On Tue, Oct 17, 2017 at 10:51:07AM +0100, Andre Przywara wrote:
> >> Hi Bhupinder,
> >>
> >> first thing: As the bulk of the series has been merged now, please
> >> restart your patch and version numbering, so a (potential) next post
> >> should be prefixed [PATCH v3 1/2]. And please have a cover letter giving
> >> a brief overview what this series fixes.
> >>
> >> On 13/10/17 11:40, Bhupinder Thakur wrote:
> >> > The early console output uses pl011_early_write() to write data. This
> >> > function waits for BUSY bit to get cleared before writing the next byte.
> >>
> >> ... which is questionable given the actual definition of the BUSY bit in
> >> the PL011 TRM:
> >>
> >> ============
> >> .... The BUSY signal goes HIGH as soon as data is written to the
> >> transmit FIFO (that is, the FIFO is non-empty) and remains asserted
> >> HIGH while data is being transmitted. BUSY is negated only when the
> >> transmit FIFO is empty, and the last character has been transmitted from
> >> the shift register, ....
> >> ============
> >>
> >> (I take it you are talking about the Linux driver in a guest here).
> >> I think the early_write routine tries to (deliberately?) ignore the
> >> FIFO, possibly to make sure characters really get pushed out before a
> >> system crashes, maybe.
> >>
> >> >
> >> > In the SBSA UART emulation logic, the BUSY bit was set as soon one
> >> > byte was written in the FIFO and it remained set until the FIFO was
> >> > emptied.
> >>
> >> Which is correct behaviour, as this matches the PL011 TRM as quoted above.
> >>
> >> > This meant that the output was delayed as each character needed
> >> > the BUSY to get cleared.
> >>
> >> But this is true as well!
> >>
> >> > Since the SBSA UART is getting emulated in Xen using ring buffers, it
> >> > ensures that once the data is enqueued in the FIFO, it will be received
> >> > by xenconsole so it is safe to set the BUSY bit only when FIFO becomes
> >> > full. This will ensure that pl011_early_write() is not delayed unduly
> >> > to write the data.
> >>
> >> So I can confirm that this patch fixes the very slow earlycon output
> >> observed with the current staging HEAD.
> >>
> >> So while this is somewhat deviating from the spec, I can see the benefit
> >> for an emulation scenario. I believe that emulations in general might
> >> choose implementing things a bit differently, to cope with the
> >> fundamental differences in their environment, like the virtually endless
> >> "FIFO" and the lack of any timing restrictions on the emulated "wire".
> >>
> >> So unless someone comes up with a better solution, I would support
> >> taking this patch, as this fixes a real problem.
> >
> > I think you get away with this, but it does violate the spec in order
> > to work around a feature of a correctly implemented driver.
> >
> > Software can now see this, for example:
> >
> >         uart_write(ch, UARTDR);
> >         busy = uart_read(UARTFR) & UARTFR_BUSY;
> >         BUG_ON(!(uart_read(UARTFR) & UARTFR_TXFE) && !busy);
> >
> > which violates the spec, though I can't currently think of a good reason
> > for software to rely on that.
> >
> >
> > [+Rob, who wrote the original earlycon code in the amba-pl011 driver:
> > 0d3c673e7881 ("tty/serial: pl011: add generic earlycon support")
> >
> > Is there any actualy reason why we poll for !BUSY after each char in
> > pl011_putc()?  pl011_putc() is not exposed at all: it's only called by
> > pl011_console_write().
> >
> > This will result in stuttering output even on hardware, though this
> > doesn't typically matter.
> >
> > I think if the poll for !BUSY were moved to the end of
> > pl011_console_write(), the effect would be much less bad.]
> 
> I just copied the code from the arm64 earlyprintk code... Maybe it was
> because on simulation (which was the main platform at the time) folks
> wanted the character "on the wire". It seems to be that you could just
> drop it.

Hmmm, the arm64 earlyprintk code

	2475ff9d2c6e ("arm64: Add simple earlyprintk support")

looks to have been derived by Catalin from arm's assembly printch/
printascii implementation, which predates git AFACT:

(Catalin, pleaes put me right if I misunderstood the history.)


arch/arm/kernel/debug.S:

ENTRY(printascii)
                addruart_current r3, r1, r2
                b       2f
1:              waituart r2, r3
                senduart r1, r3
                busyuart r2, r3
                teq     r1, #'\n'
                moveq   r1, #'\r'
                beq     1b
2:              teq     r0, #0
                ldrneb  r1, [r0], #1
                teqne   r1, #0
                bne     1b
                ret     lr
ENDPROC(printascii)

ENTRY(printch)
                addruart_current r3, r1, r2
                mov     r1, r0
                mov     r0, #0
                b       1b
ENDPROC(printch)



Russell, do you know why we wait for the UART transmitter to go
completely idle before queueing a new char?

For an individual printch this can makes sense, but it also introduces
delay for every char in printascii.

This seems to interact interestingly with virtualised UARTs, because we
may thrash between the guest and hypervisor per-char, though there
may be a way to reduce the impact of this on the emulation side.

(See above for some context)


In the pl011 earlycon code that was derived from arm64 earlycon (the
latter now deceased), pl011_putc() is not exposed at all and polling for
!BUSY other than at the end of pl011_early_write() seems unnecessary...

Crashing the platform so hard that the PL011 stops transmitting is
likely to be challenging -- e.g., turning off some clock or regulator,
making the bus lock up etc.  None of these is likely to be triggered
by pl011_early_write() itself.

Cheers
---Dave

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 26/27 v12] arm/xen: vpl011: Fix the slow early console SBSA UART output

[Russell King - ARM Linux]

On Tue, Oct 17, 2017 at 02:44:29PM +0100, Dave Martin wrote:
> arch/arm/kernel/debug.S:
> 
> ENTRY(printascii)
>                 addruart_current r3, r1, r2
>                 b       2f
> 1:              waituart r2, r3
>                 senduart r1, r3
>                 busyuart r2, r3
>                 teq     r1, #'\n'
>                 moveq   r1, #'\r'
>                 beq     1b
> 2:              teq     r0, #0
>                 ldrneb  r1, [r0], #1
>                 teqne   r1, #0
>                 bne     1b
>                 ret     lr
> ENDPROC(printascii)
> 
> ENTRY(printch)
>                 addruart_current r3, r1, r2
>                 mov     r1, r0
>                 mov     r0, #0
>                 b       1b
> ENDPROC(printch)
> 
> 
> 
> Russell, do you know why we wait for the UART transmitter to go
> completely idle before queueing a new char?

It's the only way the /debug/ (and I stress /debug/) functions know
that the character has actually been sent before allowing control to
continue.  This is important for early-crashy-debug situations, but
probably less so for early_printk.

> For an individual printch this can makes sense, but it also introduces
> delay for every char in printascii.
> 
> This seems to interact interestingly with virtualised UARTs, because we
> may thrash between the guest and hypervisor per-char, though there
> may be a way to reduce the impact of this on the emulation side.

Well, I guess re-using the early /debugging/ code for early printk is
showing more and more issues - and reusing this code in this way is
something that I've never really been a fan of.

I'd personally like to see the coupling between the two things gone -
I never really wanted that coupling in the first place.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 26/27 v12] arm/xen: vpl011: Fix the slow early console SBSA UART output

[Dave P Martin]

On Tue, Oct 17, 2017 at 03:03:02PM +0100, Russell King - ARM Linux wrote:
> On Tue, Oct 17, 2017 at 02:44:29PM +0100, Dave Martin wrote:
> > arch/arm/kernel/debug.S:
> >
> > ENTRY(printascii)
> >                 addruart_current r3, r1, r2
> >                 b       2f
> > 1:              waituart r2, r3
> >                 senduart r1, r3
> >                 busyuart r2, r3
> >                 teq     r1, #'\n'
> >                 moveq   r1, #'\r'
> >                 beq     1b
> > 2:              teq     r0, #0
> >                 ldrneb  r1, [r0], #1
> >                 teqne   r1, #0
> >                 bne     1b
> >                 ret     lr
> > ENDPROC(printascii)
> >
> > ENTRY(printch)
> >                 addruart_current r3, r1, r2
> >                 mov     r1, r0
> >                 mov     r0, #0
> >                 b       1b
> > ENDPROC(printch)
> >
> >
> >
> > Russell, do you know why we wait for the UART transmitter to go
> > completely idle before queueing a new char?
>
> It's the only way the /debug/ (and I stress /debug/) functions know
> that the character has actually been sent before allowing control to
> continue.  This is important for early-crashy-debug situations, but
> probably less so for early_printk.
>
> > For an individual printch this can makes sense, but it also introduces
> > delay for every char in printascii.
> >
> > This seems to interact interestingly with virtualised UARTs, because we
> > may thrash between the guest and hypervisor per-char, though there
> > may be a way to reduce the impact of this on the emulation side.
>
> Well, I guess re-using the early /debugging/ code for early printk is
> showing more and more issues - and reusing this code in this way is
> something that I've never really been a fan of.
>
> I'd personally like to see the coupling between the two things gone -
> I never really wanted that coupling in the first place.

Agreed.  I'll propose a patch for the amba-pl011 earlycon code so that
the flush is only per each write() call, which should at least mitigate
the impact.

For low-level debug, it makes more sense to be as conservative as
possible though: I don't see a need to change arm printch/printascii:
as you point out, these two bits of code have different purposes,
even if they have common ancestry.

Cheers
---Dave
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 26/27 v12] arm/xen: vpl011: Fix the slow early console SBSA UART output

[Bhupinder Thakur]

Hi Andre,

On 17 October 2017 at 15:21, Andre Przywara <andre.przywara@arm.com> wrote:
> Hi Bhupinder,
>
> first thing: As the bulk of the series has been merged now, please
> restart your patch and version numbering, so a (potential) next post
> should be prefixed [PATCH v3 1/2]. And please have a cover letter giving
> a brief overview what this series fixes.
>
Should I resend the patch series with a cover letter? I will also add
a reported-by tag.

> On 13/10/17 11:40, Bhupinder Thakur wrote:
>> The early console output uses pl011_early_write() to write data. This
>> function waits for BUSY bit to get cleared before writing the next byte.
>
> ... which is questionable given the actual definition of the BUSY bit in
> the PL011 TRM:
>
> ============
> .... The BUSY signal goes HIGH as soon as data is written to the
> transmit FIFO (that is, the FIFO is non-empty) and remains asserted
> HIGH while data is being transmitted. BUSY is negated only when the
> transmit FIFO is empty, and the last character has been transmitted from
> the shift register, ....
> ============
>
> (I take it you are talking about the Linux driver in a guest here).
> I think the early_write routine tries to (deliberately?) ignore the
> FIFO, possibly to make sure characters really get pushed out before a
> system crashes, maybe.
>
>>
>> In the SBSA UART emulation logic, the BUSY bit was set as soon one
>> byte was written in the FIFO and it remained set until the FIFO was
>> emptied.
>
> Which is correct behaviour, as this matches the PL011 TRM as quoted above.
>
>> This meant that the output was delayed as each character needed
>> the BUSY to get cleared.
>
> But this is true as well!
>
>> Since the SBSA UART is getting emulated in Xen using ring buffers, it
>> ensures that once the data is enqueued in the FIFO, it will be received
>> by xenconsole so it is safe to set the BUSY bit only when FIFO becomes
>> full. This will ensure that pl011_early_write() is not delayed unduly
>> to write the data.
>
> So I can confirm that this patch fixes the very slow earlycon output
> observed with the current staging HEAD.
>
> So while this is somewhat deviating from the spec, I can see the benefit
> for an emulation scenario. I believe that emulations in general might
> choose implementing things a bit differently, to cope with the
> fundamental differences in their environment, like the virtually endless
> "FIFO" and the lack of any timing restrictions on the emulated "wire".
>
> So unless someone comes up with a better solution, I would support
> taking this patch, as this fixes a real problem.
>
> Cheers,
> Andre
>
>> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
>> ---
>> CC: Julien Grall <julien.grall@arm.com>
>> CC: Andre Przywara <andre.przywara@arm.com>
>> CC: Stefano Stabellini <sstabellini@kernel.org>
>>
>>  xen/arch/arm/vpl011.c | 21 ++++++++++++++++-----
>>  1 file changed, 16 insertions(+), 5 deletions(-)
>>
>> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
>> index f7ddccb..0b07436 100644
>> --- a/xen/arch/arm/vpl011.c
>> +++ b/xen/arch/arm/vpl011.c
>> @@ -159,9 +159,15 @@ static void vpl011_write_data(struct domain *d, uint8_t data)
>>      {
>>          vpl011->uartfr |= TXFF;
>>          vpl011->uartris &= ~TXI;
>> -    }
>>
>> -    vpl011->uartfr |= BUSY;
>> +        /*
>> +         * This bit is set only when FIFO becomes full. This ensures that
>> +         * the SBSA UART driver can write the early console data as fast as
>> +         * possible, without waiting for the BUSY bit to get cleared before
>> +         * writing each byte.
>> +         */
>> +        vpl011->uartfr |= BUSY;
>> +    }
>>
>>      vpl011->uartfr &= ~TXFE;
>>
>> @@ -371,11 +377,16 @@ static void vpl011_data_avail(struct domain *d)
>>      {
>>          vpl011->uartfr &= ~TXFF;
>>          vpl011->uartris |= TXI;
>> +
>> +        /*
>> +         * Clear the BUSY bit as soon as space becomes available
>> +         * so that the SBSA UART driver can start writing more data
>> +         * without any further delay.
>> +         */
>> +        vpl011->uartfr &= ~BUSY;
>> +
>>          if ( out_ring_qsize == 0 )
>> -        {
>> -            vpl011->uartfr &= ~BUSY;
>>              vpl011->uartfr |= TXFE;
>> -        }
>>      }
>>
>>      vpl011_update_interrupt_status(d);
>>

Regards,
Bhupinder

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 27/27 v12] arm/xen: vpl011: Correct the logic for asserting/de-asserting SBSA UART TX interrupt

[Andre Przywara]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 26/27 v12] arm/xen: vpl011: Fix the slow early console SBSA UART output

[Andre Przywara]

Hi,

On 18/10/17 11:17, Bhupinder Thakur wrote:
> Hi Andre,
> 
> On 17 October 2017 at 15:21, Andre Przywara <andre.przywara@arm.com> wrote:
>> Hi Bhupinder,
>>
>> first thing: As the bulk of the series has been merged now, please
>> restart your patch and version numbering, so a (potential) next post
>> should be prefixed [PATCH v3 1/2]. And please have a cover letter giving
>> a brief overview what this series fixes.
>>
> Should I resend the patch series with a cover letter? I will also add
> a reported-by tag.

Please wait until we have settled upon a solution, especially for that
other patch. We can talk about this in our meeting later today.

Cheers,
Andre.

>> On 13/10/17 11:40, Bhupinder Thakur wrote:
>>> The early console output uses pl011_early_write() to write data. This
>>> function waits for BUSY bit to get cleared before writing the next byte.
>>
>> ... which is questionable given the actual definition of the BUSY bit in
>> the PL011 TRM:
>>
>> ============
>> .... The BUSY signal goes HIGH as soon as data is written to the
>> transmit FIFO (that is, the FIFO is non-empty) and remains asserted
>> HIGH while data is being transmitted. BUSY is negated only when the
>> transmit FIFO is empty, and the last character has been transmitted from
>> the shift register, ....
>> ============
>>
>> (I take it you are talking about the Linux driver in a guest here).
>> I think the early_write routine tries to (deliberately?) ignore the
>> FIFO, possibly to make sure characters really get pushed out before a
>> system crashes, maybe.
>>
>>>
>>> In the SBSA UART emulation logic, the BUSY bit was set as soon one
>>> byte was written in the FIFO and it remained set until the FIFO was
>>> emptied.
>>
>> Which is correct behaviour, as this matches the PL011 TRM as quoted above.
>>
>>> This meant that the output was delayed as each character needed
>>> the BUSY to get cleared.
>>
>> But this is true as well!
>>
>>> Since the SBSA UART is getting emulated in Xen using ring buffers, it
>>> ensures that once the data is enqueued in the FIFO, it will be received
>>> by xenconsole so it is safe to set the BUSY bit only when FIFO becomes
>>> full. This will ensure that pl011_early_write() is not delayed unduly
>>> to write the data.
>>
>> So I can confirm that this patch fixes the very slow earlycon output
>> observed with the current staging HEAD.
>>
>> So while this is somewhat deviating from the spec, I can see the benefit
>> for an emulation scenario. I believe that emulations in general might
>> choose implementing things a bit differently, to cope with the
>> fundamental differences in their environment, like the virtually endless
>> "FIFO" and the lack of any timing restrictions on the emulated "wire".
>>
>> So unless someone comes up with a better solution, I would support
>> taking this patch, as this fixes a real problem.
>>
>> Cheers,
>> Andre
>>
>>> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
>>> ---
>>> CC: Julien Grall <julien.grall@arm.com>
>>> CC: Andre Przywara <andre.przywara@arm.com>
>>> CC: Stefano Stabellini <sstabellini@kernel.org>
>>>
>>>  xen/arch/arm/vpl011.c | 21 ++++++++++++++++-----
>>>  1 file changed, 16 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
>>> index f7ddccb..0b07436 100644
>>> --- a/xen/arch/arm/vpl011.c
>>> +++ b/xen/arch/arm/vpl011.c
>>> @@ -159,9 +159,15 @@ static void vpl011_write_data(struct domain *d, uint8_t data)
>>>      {
>>>          vpl011->uartfr |= TXFF;
>>>          vpl011->uartris &= ~TXI;
>>> -    }
>>>
>>> -    vpl011->uartfr |= BUSY;
>>> +        /*
>>> +         * This bit is set only when FIFO becomes full. This ensures that
>>> +         * the SBSA UART driver can write the early console data as fast as
>>> +         * possible, without waiting for the BUSY bit to get cleared before
>>> +         * writing each byte.
>>> +         */
>>> +        vpl011->uartfr |= BUSY;
>>> +    }
>>>
>>>      vpl011->uartfr &= ~TXFE;
>>>
>>> @@ -371,11 +377,16 @@ static void vpl011_data_avail(struct domain *d)
>>>      {
>>>          vpl011->uartfr &= ~TXFF;
>>>          vpl011->uartris |= TXI;
>>> +
>>> +        /*
>>> +         * Clear the BUSY bit as soon as space becomes available
>>> +         * so that the SBSA UART driver can start writing more data
>>> +         * without any further delay.
>>> +         */
>>> +        vpl011->uartfr &= ~BUSY;
>>> +
>>>          if ( out_ring_qsize == 0 )
>>> -        {
>>> -            vpl011->uartfr &= ~BUSY;
>>>              vpl011->uartfr |= TXFE;
>>> -        }
>>>      }
>>>
>>>      vpl011_update_interrupt_status(d);
>>>
> 
> Regards,
> Bhupinder
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 27/27 v12] arm/xen: vpl011: Correct the logic for asserting/de-asserting SBSA UART TX interrupt

[Bhupinder Thakur]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH RFC] ARM: vPL011: use receive timeout interrupt

[Andre Przywara]

Instead of asserting the receive interrupt (RXI) on the first character
in the FIFO, lets (ab)use the receive timeout interrupt (RTI) for that
purpose. That seems to be closer to the spec and what hardware does.
Improve the readability of vpl011_data_avail() on the way.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
Hi,

this one is the approach I mentioned in the email earlier today.
It goes on top of Bhupinders v12 27/27, but should eventually be merged
into this one once we agreed on the subject. I just carved it out here
for clarity to make it clearer what has been changed.
Would be good if someone could test it.

Cheers,
Andre.
 xen/arch/arm/vpl011.c | 61 ++++++++++++++++++++++++---------------------------
 1 file changed, 29 insertions(+), 32 deletions(-)

diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
index adf1711571..ae18bddd81 100644
--- a/xen/arch/arm/vpl011.c
+++ b/xen/arch/arm/vpl011.c
@@ -105,9 +105,13 @@ static uint8_t vpl011_read_data(struct domain *d)
         if ( fifo_level == 0 )
         {
             vpl011->uartfr |= RXFE;
-            vpl011->uartris &= ~RXI;
-            vpl011_update_interrupt_status(d);
+            vpl011->uartris &= ~RTI;
         }
+
+        if ( fifo_level < sizeof(intf->in) - SBSA_UART_FIFO_SIZE / 2 )
+            vpl011->uartris &= ~RXI;
+
+        vpl011_update_interrupt_status(d);
     }
     else
         gprintk(XENLOG_ERR, "vpl011: Unexpected IN ring buffer empty\n");
@@ -129,7 +133,7 @@ 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;
+    unsigned int fifo_threshold = sizeof(intf->out) - SBSA_UART_FIFO_SIZE/2;
 
     BUILD_BUG_ON(sizeof (intf->out) < SBSA_UART_FIFO_SIZE);
 
@@ -137,8 +141,6 @@ static void vpl011_update_tx_fifo_status(struct vpl011 *vpl011,
      * 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
@@ -390,35 +392,30 @@ static void vpl011_data_avail(struct domain *d)
                                     out_cons,
                                     sizeof(intf->out));
 
-    /* Update the uart rx state if the buffer is not empty. */
-    if ( in_fifo_level != 0 )
-    {
+    /**** Update the UART RX state ****/
+
+    /* Clear the FIFO_EMPTY bit if the FIFO holds at least one character. */
+    if ( in_fifo_level > 0 )
         vpl011->uartfr &= ~RXFE;
 
-        if ( in_fifo_level == sizeof(intf->in) )
-            vpl011->uartfr |= RXFF;
+    /* Set the FIFO_FULL bit if the ring buffer is full. */
+    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.
-         */
+    /* The FIFO trigger level is fixed to half of the FIFO. */
+    if ( in_fifo_level >= sizeof(intf->in) - SBSA_UART_FIFO_SIZE / 2 )
         vpl011->uartris |= RXI;
-    }
 
-    /* Update the uart tx state if the buffer is not full. */
+    /*
+     * If the input queue is not empty, we assert the receive timeout interrupt.
+     * As we don't emulate any timing here, we ignore the actual timeout
+     * of 32 bit periods.
+     */
+    if ( in_fifo_level > 0 )
+        vpl011->uartris |= RTI;
+
+    /**** Update the UART TX state ****/
+
     if ( out_fifo_level != sizeof(intf->out) )
     {
         vpl011->uartfr &= ~TXFF;
@@ -431,13 +428,13 @@ static void vpl011_data_avail(struct domain *d)
         vpl011->uartfr &= ~BUSY;
 
         vpl011_update_tx_fifo_status(vpl011, out_fifo_level);
-
-        if ( out_fifo_level == 0 )
-            vpl011->uartfr |= TXFE;
     }
 
     vpl011_update_interrupt_status(d);
 
+    if ( out_fifo_level == 0 )
+        vpl011->uartfr |= TXFE;
+
     VPL011_UNLOCK(d, flags);
 }
 
-- 
2.14.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH RFC] ARM: vPL011: use receive timeout interrupt

[Bhupinder Thakur]

Hi Andre,

I verified this patch on qualcomm platform. It is working fine.

On 18 October 2017 at 19:11, Andre Przywara <andre.przywara@arm.com> wrote:
> Instead of asserting the receive interrupt (RXI) on the first character
> in the FIFO, lets (ab)use the receive timeout interrupt (RTI) for that
> purpose. That seems to be closer to the spec and what hardware does.
> Improve the readability of vpl011_data_avail() on the way.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
> Hi,
>
> this one is the approach I mentioned in the email earlier today.
> It goes on top of Bhupinders v12 27/27, but should eventually be merged
> into this one once we agreed on the subject. I just carved it out here
> for clarity to make it clearer what has been changed.
> Would be good if someone could test it.
>
> Cheers,
> Andre.
>  xen/arch/arm/vpl011.c | 61 ++++++++++++++++++++++++---------------------------
>  1 file changed, 29 insertions(+), 32 deletions(-)
>
> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
> index adf1711571..ae18bddd81 100644
> --- a/xen/arch/arm/vpl011.c
> +++ b/xen/arch/arm/vpl011.c
> @@ -105,9 +105,13 @@ static uint8_t vpl011_read_data(struct domain *d)
>          if ( fifo_level == 0 )
>          {
>              vpl011->uartfr |= RXFE;
> -            vpl011->uartris &= ~RXI;
> -            vpl011_update_interrupt_status(d);
> +            vpl011->uartris &= ~RTI;
>          }
> +
> +        if ( fifo_level < sizeof(intf->in) - SBSA_UART_FIFO_SIZE / 2 )
> +            vpl011->uartris &= ~RXI;
> +
> +        vpl011_update_interrupt_status(d);
I think we check if ( fifo_level < SBSA_UART_FIFO_SIZE / 2 ) which
should be a valid condition to clear the RX interrupt.

>      }
>      else
>          gprintk(XENLOG_ERR, "vpl011: Unexpected IN ring buffer empty\n");
> @@ -129,7 +133,7 @@ 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;
> +    unsigned int fifo_threshold = sizeof(intf->out) - SBSA_UART_FIFO_SIZE/2;
>
>      BUILD_BUG_ON(sizeof (intf->out) < SBSA_UART_FIFO_SIZE);
>
> @@ -137,8 +141,6 @@ static void vpl011_update_tx_fifo_status(struct vpl011 *vpl011,
>       * 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
> @@ -390,35 +392,30 @@ static void vpl011_data_avail(struct domain *d)
>                                      out_cons,
>                                      sizeof(intf->out));
>
> -    /* Update the uart rx state if the buffer is not empty. */
> -    if ( in_fifo_level != 0 )
> -    {
> +    /**** Update the UART RX state ****/
> +
> +    /* Clear the FIFO_EMPTY bit if the FIFO holds at least one character. */
> +    if ( in_fifo_level > 0 )
>          vpl011->uartfr &= ~RXFE;
>
> -        if ( in_fifo_level == sizeof(intf->in) )
> -            vpl011->uartfr |= RXFF;
> +    /* Set the FIFO_FULL bit if the ring buffer is full. */
> +    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.
> -         */
> +    /* The FIFO trigger level is fixed to half of the FIFO. */
> +    if ( in_fifo_level >= sizeof(intf->in) - SBSA_UART_FIFO_SIZE / 2 )
>          vpl011->uartris |= RXI;
Here also should not we check if ( in_fifo_level >=
SBSA_UART_FIFO_SIZE / 2 ) since it is a valid condition to raise the
RX interrupt?

> -    }
>
> -    /* Update the uart tx state if the buffer is not full. */
> +    /*
> +     * If the input queue is not empty, we assert the receive timeout interrupt.
> +     * As we don't emulate any timing here, we ignore the actual timeout
> +     * of 32 bit periods.
> +     */
> +    if ( in_fifo_level > 0 )
> +        vpl011->uartris |= RTI;
> +
> +    /**** Update the UART TX state ****/
> +
>      if ( out_fifo_level != sizeof(intf->out) )
>      {
>          vpl011->uartfr &= ~TXFF;
> @@ -431,13 +428,13 @@ static void vpl011_data_avail(struct domain *d)
>          vpl011->uartfr &= ~BUSY;
>
>          vpl011_update_tx_fifo_status(vpl011, out_fifo_level);
> -
> -        if ( out_fifo_level == 0 )
> -            vpl011->uartfr |= TXFE;
>      }
>
>      vpl011_update_interrupt_status(d);
>
> +    if ( out_fifo_level == 0 )
> +        vpl011->uartfr |= TXFE;
> +
>      VPL011_UNLOCK(d, flags);
>  }
>
> --
> 2.14.1
>

Regards,
Bhupinder

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH RFC] ARM: vPL011: use receive timeout interrupt

[Andre Przywara]

Hi,

On 18/10/17 17:32, Bhupinder Thakur wrote:
> Hi Andre,
> 
> I verified this patch on qualcomm platform. It is working fine.
> 
> On 18 October 2017 at 19:11, Andre Przywara <andre.przywara@arm.com> wrote:
>> Instead of asserting the receive interrupt (RXI) on the first character
>> in the FIFO, lets (ab)use the receive timeout interrupt (RTI) for that
>> purpose. That seems to be closer to the spec and what hardware does.
>> Improve the readability of vpl011_data_avail() on the way.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>> Hi,
>>
>> this one is the approach I mentioned in the email earlier today.
>> It goes on top of Bhupinders v12 27/27, but should eventually be merged
>> into this one once we agreed on the subject. I just carved it out here
>> for clarity to make it clearer what has been changed.
>> Would be good if someone could test it.
>>
>> Cheers,
>> Andre.
>>  xen/arch/arm/vpl011.c | 61 ++++++++++++++++++++++++---------------------------
>>  1 file changed, 29 insertions(+), 32 deletions(-)
>>
>> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
>> index adf1711571..ae18bddd81 100644
>> --- a/xen/arch/arm/vpl011.c
>> +++ b/xen/arch/arm/vpl011.c
>> @@ -105,9 +105,13 @@ static uint8_t vpl011_read_data(struct domain *d)
>>          if ( fifo_level == 0 )
>>          {
>>              vpl011->uartfr |= RXFE;
>> -            vpl011->uartris &= ~RXI;
>> -            vpl011_update_interrupt_status(d);
>> +            vpl011->uartris &= ~RTI;
>>          }
>> +
>> +        if ( fifo_level < sizeof(intf->in) - SBSA_UART_FIFO_SIZE / 2 )
>> +            vpl011->uartris &= ~RXI;
>> +
>> +        vpl011_update_interrupt_status(d);
> I think we check if ( fifo_level < SBSA_UART_FIFO_SIZE / 2 ) which
> should be a valid condition to clear the RX interrupt.

Are you sure? My understanding is that the semantics of the return value
of xencons_queued() differs between intf and outf:
- For intf, Xen fills that buffer with incoming characters. The
watermark is assumed to be (FIFO / 2), which translates into 16
characters. Now for the SBSA vUART RX side that means: "Assert the RX
interrupt if there is only room for 16 (or less) characters in the FIFO
(read: intf buffer in our case). Since we (ab)use the Xen buffer for the
FIFO, this means we warn if the number of queued characters exceeds
(buffersize - 16).
- For outf, the UART emulation fills the buffer. The SBSA vUART TX side
demands that the TX interrupt is asserted if the fill level of the
transmit FIFO is less than or equal to the 16 characters, which means:
number of queued characters is less than 16.

I think the key point is that our trigger level isn't symmetrical here,
since we have to emulate the architected 32-byte FIFO semantics for the
driver, but have a (secretly) much larger "FIFO" internally.

Do you agree with this reasoning and do I have a thinko here? Could well
be I am seriously misguided here.

Cheers,
Andre

>>      }
>>      else
>>          gprintk(XENLOG_ERR, "vpl011: Unexpected IN ring buffer empty\n");
>> @@ -129,7 +133,7 @@ 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;
>> +    unsigned int fifo_threshold = sizeof(intf->out) - SBSA_UART_FIFO_SIZE/2;
>>
>>      BUILD_BUG_ON(sizeof (intf->out) < SBSA_UART_FIFO_SIZE);
>>
>> @@ -137,8 +141,6 @@ static void vpl011_update_tx_fifo_status(struct vpl011 *vpl011,
>>       * 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
>> @@ -390,35 +392,30 @@ static void vpl011_data_avail(struct domain *d)
>>                                      out_cons,
>>                                      sizeof(intf->out));
>>
>> -    /* Update the uart rx state if the buffer is not empty. */
>> -    if ( in_fifo_level != 0 )
>> -    {
>> +    /**** Update the UART RX state ****/
>> +
>> +    /* Clear the FIFO_EMPTY bit if the FIFO holds at least one character. */
>> +    if ( in_fifo_level > 0 )
>>          vpl011->uartfr &= ~RXFE;
>>
>> -        if ( in_fifo_level == sizeof(intf->in) )
>> -            vpl011->uartfr |= RXFF;
>> +    /* Set the FIFO_FULL bit if the ring buffer is full. */
>> +    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.
>> -         */
>> +    /* The FIFO trigger level is fixed to half of the FIFO. */
>> +    if ( in_fifo_level >= sizeof(intf->in) - SBSA_UART_FIFO_SIZE / 2 )
>>          vpl011->uartris |= RXI;
> Here also should not we check if ( in_fifo_level >=
> SBSA_UART_FIFO_SIZE / 2 ) since it is a valid condition to raise the
> RX interrupt?
> 
>> -    }
>>
>> -    /* Update the uart tx state if the buffer is not full. */
>> +    /*
>> +     * If the input queue is not empty, we assert the receive timeout interrupt.
>> +     * As we don't emulate any timing here, we ignore the actual timeout
>> +     * of 32 bit periods.
>> +     */
>> +    if ( in_fifo_level > 0 )
>> +        vpl011->uartris |= RTI;
>> +
>> +    /**** Update the UART TX state ****/
>> +
>>      if ( out_fifo_level != sizeof(intf->out) )
>>      {
>>          vpl011->uartfr &= ~TXFF;
>> @@ -431,13 +428,13 @@ static void vpl011_data_avail(struct domain *d)
>>          vpl011->uartfr &= ~BUSY;
>>
>>          vpl011_update_tx_fifo_status(vpl011, out_fifo_level);
>> -
>> -        if ( out_fifo_level == 0 )
>> -            vpl011->uartfr |= TXFE;
>>      }
>>
>>      vpl011_update_interrupt_status(d);
>>
>> +    if ( out_fifo_level == 0 )
>> +        vpl011->uartfr |= TXFE;
>> +
>>      VPL011_UNLOCK(d, flags);
>>  }
>>
>> --
>> 2.14.1
>>
> 
> Regards,
> Bhupinder
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH RFC] ARM: vPL011: use receive timeout interrupt

[Julien Grall]

Hi,

On 23/10/2017 17:01, Andre Przywara wrote:
> Hi,
>
> On 18/10/17 17:32, Bhupinder Thakur wrote:
>> Hi Andre,
>>
>> I verified this patch on qualcomm platform. It is working fine.
>>
>> On 18 October 2017 at 19:11, Andre Przywara <andre.przywara@arm.com> wrote:
>>> Instead of asserting the receive interrupt (RXI) on the first character
>>> in the FIFO, lets (ab)use the receive timeout interrupt (RTI) for that
>>> purpose. That seems to be closer to the spec and what hardware does.
>>> Improve the readability of vpl011_data_avail() on the way.
>>>
>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>> ---
>>> Hi,
>>>
>>> this one is the approach I mentioned in the email earlier today.
>>> It goes on top of Bhupinders v12 27/27, but should eventually be merged
>>> into this one once we agreed on the subject. I just carved it out here
>>> for clarity to make it clearer what has been changed.
>>> Would be good if someone could test it.
>>>
>>> Cheers,
>>> Andre.
>>>  xen/arch/arm/vpl011.c | 61 ++++++++++++++++++++++++---------------------------
>>>  1 file changed, 29 insertions(+), 32 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
>>> index adf1711571..ae18bddd81 100644
>>> --- a/xen/arch/arm/vpl011.c
>>> +++ b/xen/arch/arm/vpl011.c
>>> @@ -105,9 +105,13 @@ static uint8_t vpl011_read_data(struct domain *d)
>>>          if ( fifo_level == 0 )
>>>          {
>>>              vpl011->uartfr |= RXFE;
>>> -            vpl011->uartris &= ~RXI;
>>> -            vpl011_update_interrupt_status(d);
>>> +            vpl011->uartris &= ~RTI;
>>>          }
>>> +
>>> +        if ( fifo_level < sizeof(intf->in) - SBSA_UART_FIFO_SIZE / 2 )
>>> +            vpl011->uartris &= ~RXI;
>>> +
>>> +        vpl011_update_interrupt_status(d);
>> I think we check if ( fifo_level < SBSA_UART_FIFO_SIZE / 2 ) which
>> should be a valid condition to clear the RX interrupt.
>
> Are you sure? My understanding is that the semantics of the return value
> of xencons_queued() differs between intf and outf:
> - For intf, Xen fills that buffer with incoming characters. The
> watermark is assumed to be (FIFO / 2), which translates into 16
> characters. Now for the SBSA vUART RX side that means: "Assert the RX
> interrupt if there is only room for 16 (or less) characters in the FIFO
> (read: intf buffer in our case). Since we (ab)use the Xen buffer for the
> FIFO, this means we warn if the number of queued characters exceeds
> (buffersize - 16).
> - For outf, the UART emulation fills the buffer. The SBSA vUART TX side
> demands that the TX interrupt is asserted if the fill level of the
> transmit FIFO is less than or equal to the 16 characters, which means:
> number of queued characters is less than 16.
>
> I think the key point is that our trigger level isn't symmetrical here,
> since we have to emulate the architected 32-byte FIFO semantics for the
> driver, but have a (secretly) much larger "FIFO" internally.
>
> Do you agree with this reasoning and do I have a thinko here? Could well
> be I am seriously misguided here.

xencons_queued calculates how many bytes are currently on the ring. So I 
think your description makes sense.

With (fifo_level < (SBSA_UART_FIFO_SIZE / 2)), you would only clear it 
when the ring has less than 16 bytes queued.

I have a few requests on those patches for the resender:
	- Please introduce a define for SBSA_UART_FIFO_SIZE / 2 and use it 
everywhere.
	- Please add a bit more documentation on top of the checks in 
vpl011_read_data function. The checks in vpl011_write_data looks 
well-documented.

Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH RFC] ARM: vPL011: use receive timeout interrupt

[Andre Przywara]

Hi,

On 24/10/17 12:00, Julien Grall wrote:
> Hi,
> 
> On 23/10/2017 17:01, Andre Przywara wrote:
>> Hi,
>>
>> On 18/10/17 17:32, Bhupinder Thakur wrote:
>>> Hi Andre,
>>>
>>> I verified this patch on qualcomm platform. It is working fine.
>>>
>>> On 18 October 2017 at 19:11, Andre Przywara <andre.przywara@arm.com>
>>> wrote:
>>>> Instead of asserting the receive interrupt (RXI) on the first character
>>>> in the FIFO, lets (ab)use the receive timeout interrupt (RTI) for that
>>>> purpose. That seems to be closer to the spec and what hardware does.
>>>> Improve the readability of vpl011_data_avail() on the way.
>>>>
>>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>>> ---
>>>> Hi,
>>>>
>>>> this one is the approach I mentioned in the email earlier today.
>>>> It goes on top of Bhupinders v12 27/27, but should eventually be merged
>>>> into this one once we agreed on the subject. I just carved it out here
>>>> for clarity to make it clearer what has been changed.
>>>> Would be good if someone could test it.
>>>>
>>>> Cheers,
>>>> Andre.
>>>>  xen/arch/arm/vpl011.c | 61
>>>> ++++++++++++++++++++++++---------------------------
>>>>  1 file changed, 29 insertions(+), 32 deletions(-)
>>>>
>>>> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
>>>> index adf1711571..ae18bddd81 100644
>>>> --- a/xen/arch/arm/vpl011.c
>>>> +++ b/xen/arch/arm/vpl011.c
>>>> @@ -105,9 +105,13 @@ static uint8_t vpl011_read_data(struct domain *d)
>>>>          if ( fifo_level == 0 )
>>>>          {
>>>>              vpl011->uartfr |= RXFE;
>>>> -            vpl011->uartris &= ~RXI;
>>>> -            vpl011_update_interrupt_status(d);
>>>> +            vpl011->uartris &= ~RTI;
>>>>          }
>>>> +
>>>> +        if ( fifo_level < sizeof(intf->in) - SBSA_UART_FIFO_SIZE / 2 )
>>>> +            vpl011->uartris &= ~RXI;
>>>> +
>>>> +        vpl011_update_interrupt_status(d);
>>> I think we check if ( fifo_level < SBSA_UART_FIFO_SIZE / 2 ) which
>>> should be a valid condition to clear the RX interrupt.
>>
>> Are you sure? My understanding is that the semantics of the return value
>> of xencons_queued() differs between intf and outf:
>> - For intf, Xen fills that buffer with incoming characters. The
>> watermark is assumed to be (FIFO / 2), which translates into 16
>> characters. Now for the SBSA vUART RX side that means: "Assert the RX
>> interrupt if there is only room for 16 (or less) characters in the FIFO
>> (read: intf buffer in our case). Since we (ab)use the Xen buffer for the
>> FIFO, this means we warn if the number of queued characters exceeds
>> (buffersize - 16).
>> - For outf, the UART emulation fills the buffer. The SBSA vUART TX side
>> demands that the TX interrupt is asserted if the fill level of the
>> transmit FIFO is less than or equal to the 16 characters, which means:
>> number of queued characters is less than 16.
>>
>> I think the key point is that our trigger level isn't symmetrical here,
>> since we have to emulate the architected 32-byte FIFO semantics for the
>> driver, but have a (secretly) much larger "FIFO" internally.
>>
>> Do you agree with this reasoning and do I have a thinko here? Could well
>> be I am seriously misguided here.
> 
> xencons_queued calculates how many bytes are currently on the ring. So I
> think your description makes sense.
> 
> With (fifo_level < (SBSA_UART_FIFO_SIZE / 2)), you would only clear it
> when the ring has less than 16 bytes queued.
> 
> I have a few requests on those patches for the resender:
>     - Please introduce a define for SBSA_UART_FIFO_SIZE / 2 and use it
> everywhere.
>     - Please add a bit more documentation on top of the checks in
> vpl011_read_data function. The checks in vpl011_write_data looks
> well-documented.

I am just at rewording the commit message and was planning on re-sending
the (merged) patches later today (keeping Bhupinder's authorship, of
course).

I hope that Bhupinder doesn't mind or this doesn't clash with any of his
plans.

Cheers,
Andre.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH RFC] ARM: vPL011: use receive timeout interrupt

[Bhupinder Thakur]

Hi,

On 23 October 2017 at 21:31, Andre Przywara <andre.przywara@linaro.org> wrote:
> Hi,
>
> On 18/10/17 17:32, Bhupinder Thakur wrote:
>> Hi Andre,
>>
>> I verified this patch on qualcomm platform. It is working fine.
>>
>> On 18 October 2017 at 19:11, Andre Przywara <andre.przywara@arm.com> wrote:
>>> Instead of asserting the receive interrupt (RXI) on the first character
>>> in the FIFO, lets (ab)use the receive timeout interrupt (RTI) for that
>>> purpose. That seems to be closer to the spec and what hardware does.
>>> Improve the readability of vpl011_data_avail() on the way.
>>>
>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>> ---
>>> Hi,
>>>
>>> this one is the approach I mentioned in the email earlier today.
>>> It goes on top of Bhupinders v12 27/27, but should eventually be merged
>>> into this one once we agreed on the subject. I just carved it out here
>>> for clarity to make it clearer what has been changed.
>>> Would be good if someone could test it.
>>>
>>> Cheers,
>>> Andre.
>>>  xen/arch/arm/vpl011.c | 61 ++++++++++++++++++++++++---------------------------
>>>  1 file changed, 29 insertions(+), 32 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
>>> index adf1711571..ae18bddd81 100644
>>> --- a/xen/arch/arm/vpl011.c
>>> +++ b/xen/arch/arm/vpl011.c
>>> @@ -105,9 +105,13 @@ static uint8_t vpl011_read_data(struct domain *d)
>>>          if ( fifo_level == 0 )
>>>          {
>>>              vpl011->uartfr |= RXFE;
>>> -            vpl011->uartris &= ~RXI;
>>> -            vpl011_update_interrupt_status(d);
>>> +            vpl011->uartris &= ~RTI;
>>>          }
>>> +
>>> +        if ( fifo_level < sizeof(intf->in) - SBSA_UART_FIFO_SIZE / 2 )
>>> +            vpl011->uartris &= ~RXI;
>>> +
>>> +        vpl011_update_interrupt_status(d);
>> I think we check if ( fifo_level < SBSA_UART_FIFO_SIZE / 2 ) which
>> should be a valid condition to clear the RX interrupt.
>
> Are you sure? My understanding is that the semantics of the return value
> of xencons_queued() differs between intf and outf:
> - For intf, Xen fills that buffer with incoming characters. The
> watermark is assumed to be (FIFO / 2), which translates into 16
> characters. Now for the SBSA vUART RX side that means: "Assert the RX
> interrupt if there is only room for 16 (or less) characters in the FIFO
> (read: intf buffer in our case). Since we (ab)use the Xen buffer for the
> FIFO, this means we warn if the number of queued characters exceeds
> (buffersize - 16).
> - For outf, the UART emulation fills the buffer. The SBSA vUART TX side
> demands that the TX interrupt is asserted if the fill level of the
> transmit FIFO is less than or equal to the 16 characters, which means:
> number of queued characters is less than 16.
>
> I think the key point is that our trigger level isn't symmetrical here,
> since we have to emulate the architected 32-byte FIFO semantics for the
> driver, but have a (secretly) much larger "FIFO" internally.
>
> Do you agree with this reasoning and do I have a thinko here? Could well
> be I am seriously misguided here.
>
ok. I agree with the description as it will expose the same behavior
to the driver as it would be there for a real UART where only FIFO/2
space is left.

Regards,
Bhupinder

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH RFC] ARM: vPL011: use receive timeout interrupt

[Bhupinder Thakur]

Hi Andre,


On 24 October 2017 at 16:57, Andre Przywara <andre.przywara@linaro.org> wrote:
> Hi,
>
> On 24/10/17 12:00, Julien Grall wrote:
>> Hi,
>>
>> On 23/10/2017 17:01, Andre Przywara wrote:
>>> Hi,
>>>
>>> On 18/10/17 17:32, Bhupinder Thakur wrote:
>>>> Hi Andre,
>>>>
>>>> I verified this patch on qualcomm platform. It is working fine.
>>>>
>>>> On 18 October 2017 at 19:11, Andre Przywara <andre.przywara@arm.com>
>>>> wrote:
>>>>> Instead of asserting the receive interrupt (RXI) on the first character
>>>>> in the FIFO, lets (ab)use the receive timeout interrupt (RTI) for that
>>>>> purpose. That seems to be closer to the spec and what hardware does.
>>>>> Improve the readability of vpl011_data_avail() on the way.
>>>>>
>>>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>>>> ---
>>>>> Hi,
>>>>>
>>>>> this one is the approach I mentioned in the email earlier today.
>>>>> It goes on top of Bhupinders v12 27/27, but should eventually be merged
>>>>> into this one once we agreed on the subject. I just carved it out here
>>>>> for clarity to make it clearer what has been changed.
>>>>> Would be good if someone could test it.
>>>>>
>>>>> Cheers,
>>>>> Andre.
>>>>>  xen/arch/arm/vpl011.c | 61
>>>>> ++++++++++++++++++++++++---------------------------
>>>>>  1 file changed, 29 insertions(+), 32 deletions(-)
>>>>>
>>>>> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
>>>>> index adf1711571..ae18bddd81 100644
>>>>> --- a/xen/arch/arm/vpl011.c
>>>>> +++ b/xen/arch/arm/vpl011.c
>>>>> @@ -105,9 +105,13 @@ static uint8_t vpl011_read_data(struct domain *d)
>>>>>          if ( fifo_level == 0 )
>>>>>          {
>>>>>              vpl011->uartfr |= RXFE;
>>>>> -            vpl011->uartris &= ~RXI;
>>>>> -            vpl011_update_interrupt_status(d);
>>>>> +            vpl011->uartris &= ~RTI;
>>>>>          }
>>>>> +
>>>>> +        if ( fifo_level < sizeof(intf->in) - SBSA_UART_FIFO_SIZE / 2 )
>>>>> +            vpl011->uartris &= ~RXI;
>>>>> +
>>>>> +        vpl011_update_interrupt_status(d);
>>>> I think we check if ( fifo_level < SBSA_UART_FIFO_SIZE / 2 ) which
>>>> should be a valid condition to clear the RX interrupt.
>>>
>>> Are you sure? My understanding is that the semantics of the return value
>>> of xencons_queued() differs between intf and outf:
>>> - For intf, Xen fills that buffer with incoming characters. The
>>> watermark is assumed to be (FIFO / 2), which translates into 16
>>> characters. Now for the SBSA vUART RX side that means: "Assert the RX
>>> interrupt if there is only room for 16 (or less) characters in the FIFO
>>> (read: intf buffer in our case). Since we (ab)use the Xen buffer for the
>>> FIFO, this means we warn if the number of queued characters exceeds
>>> (buffersize - 16).
>>> - For outf, the UART emulation fills the buffer. The SBSA vUART TX side
>>> demands that the TX interrupt is asserted if the fill level of the
>>> transmit FIFO is less than or equal to the 16 characters, which means:
>>> number of queued characters is less than 16.
>>>
>>> I think the key point is that our trigger level isn't symmetrical here,
>>> since we have to emulate the architected 32-byte FIFO semantics for the
>>> driver, but have a (secretly) much larger "FIFO" internally.
>>>
>>> Do you agree with this reasoning and do I have a thinko here? Could well
>>> be I am seriously misguided here.
>>
>> xencons_queued calculates how many bytes are currently on the ring. So I
>> think your description makes sense.
>>
>> With (fifo_level < (SBSA_UART_FIFO_SIZE / 2)), you would only clear it
>> when the ring has less than 16 bytes queued.
>>
>> I have a few requests on those patches for the resender:
>>     - Please introduce a define for SBSA_UART_FIFO_SIZE / 2 and use it
>> everywhere.
>>     - Please add a bit more documentation on top of the checks in
>> vpl011_read_data function. The checks in vpl011_write_data looks
>> well-documented.
>
> I am just at rewording the commit message and was planning on re-sending
> the (merged) patches later today (keeping Bhupinder's authorship, of
> course).
>
> I hope that Bhupinder doesn't mind or this doesn't clash with any of his
> plans.
It is fine with me. Thanks.

Regards,
Bhupinder

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel