[PATCH v1] xen/serial: Return actual bytes stored in TX FIFO for OMAP

[PATCH v1] xen/serial: Return actual bytes stored in TX FIFO for OMAP

[Oleksandr Tyshchenko]

This is intended to decrease a time spending in transmitter
while waiting for the free space in TX FIFO.
And as result to reduce the impact of hvc on the entire system
running on OMAP5/DRA7XX based platforms.

Signed-off-by: Oleksandr Tyshchenko <oleksandr.tyshchenko@globallogic.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Julien Grall <julien.grall@citrix.com>
CC: Stefano Stabellini <stefano.stabellini@citrix.com>
---
 xen/drivers/char/omap-uart.c | 11 ++++++++++-
 xen/include/xen/8250-uart.h  |  9 +++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/xen/drivers/char/omap-uart.c b/xen/drivers/char/omap-uart.c
index d8f64ea..8c542a7 100644
--- a/xen/drivers/char/omap-uart.c
+++ b/xen/drivers/char/omap-uart.c
@@ -233,11 +233,20 @@ static int omap_uart_tx_ready(struct serial_port *port)
 {
     struct omap_uart *uart = port->uart;
     uint32_t reg;
+    uint8_t cnt;
 
     reg = omap_read(uart, UART_IER);
     omap_write(uart, UART_IER, reg | UART_IER_ETHREI);
 
-    return omap_read(uart, UART_LSR) & UART_LSR_THRE ? uart->fifo_size : 0;
+    /* Check for empty space in TX FIFO */
+    if ( omap_read(uart, UART_OMAP_SSR) & UART_OMAP_SSR_TX_FIFO_FULL_MASK )
+        return 0;
+
+    /* Check number of data bytes stored in TX FIFO */
+    cnt = omap_read(uart, UART_OMAP_TXFIFO_LVL);
+    ASSERT( cnt >= 0 && cnt <= uart->fifo_size );
+
+    return (uart->fifo_size - cnt);
 }
 
 static void omap_uart_putc(struct serial_port *port, char c)
diff --git a/xen/include/xen/8250-uart.h b/xen/include/xen/8250-uart.h
index 304b9dd..700b2d4 100644
--- a/xen/include/xen/8250-uart.h
+++ b/xen/include/xen/8250-uart.h
@@ -143,6 +143,15 @@
 /* Supplementary control register */
 #define UART_OMAP_SCR     0x10
 
+/* Supplementary status register */
+#define UART_OMAP_SSR     0x11
+
+/* Supplementary status register bitmasks */
+#define UART_OMAP_SSR_TX_FIFO_FULL_MASK (1 << 0)
+
+/* TX FIFO level register */
+#define UART_OMAP_TXFIFO_LVL   0x1A
+
 /* SCR register bitmasks */
 #define OMAP_UART_SCR_RX_TRIG_GRANU1_MASK (1 << 7)
 
-- 
1.9.1

Re: [PATCH v1] xen/serial: Return actual bytes stored in TX FIFO for OMAP

[Jan Beulich]

>>> On 27.10.15 at 11:36, <oleksandr.tyshchenko@globallogic.com> wrote:
> --- a/xen/include/xen/8250-uart.h
> +++ b/xen/include/xen/8250-uart.h
> @@ -143,6 +143,15 @@
>  /* Supplementary control register */
>  #define UART_OMAP_SCR     0x10

I think this one is already misplaced here (as is the one in the context
below the change).

> +/* Supplementary status register */
> +#define UART_OMAP_SSR     0x11
> +
> +/* Supplementary status register bitmasks */
> +#define UART_OMAP_SSR_TX_FIFO_FULL_MASK (1 << 0)
> +
> +/* TX FIFO level register */
> +#define UART_OMAP_TXFIFO_LVL   0x1A

Hence, rather than adding more 8250-unrelated things to this
header, I think this needs cleaning up.

Jan

Re: [PATCH v1] xen/serial: Return actual bytes stored in TX FIFO for OMAP

[Julien Grall]

On 27/10/15 10:50, Jan Beulich wrote:
>>>> On 27.10.15 at 11:36, <oleksandr.tyshchenko@globallogic.com> wrote:
>> --- a/xen/include/xen/8250-uart.h
>> +++ b/xen/include/xen/8250-uart.h
>> @@ -143,6 +143,15 @@
>>  /* Supplementary control register */
>>  #define UART_OMAP_SCR     0x10
> 
> I think this one is already misplaced here (as is the one in the context
> below the change).

I don't think so, the omap UART is based on the 8250 and share some
common registers.

FWIW, we do the same for ns16550.c which is using the 8250 headers...

Regards,

-- 
Julien Grall

Re: [PATCH v1] xen/serial: Return actual bytes stored in TX FIFO for OMAP

[Jan Beulich]

>>> On 27.10.15 at 11:54, <julien.grall@citrix.com> wrote:
> On 27/10/15 10:50, Jan Beulich wrote:
>>>>> On 27.10.15 at 11:36, <oleksandr.tyshchenko@globallogic.com> wrote:
>>> --- a/xen/include/xen/8250-uart.h
>>> +++ b/xen/include/xen/8250-uart.h
>>> @@ -143,6 +143,15 @@
>>>  /* Supplementary control register */
>>>  #define UART_OMAP_SCR     0x10
>> 
>> I think this one is already misplaced here (as is the one in the context
>> below the change).
> 
> I don't think so, the omap UART is based on the 8250 and share some
> common registers.

Well, if it's that way then the patch is fine, but I then question why
we have a separate omap-uart.c.

> FWIW, we do the same for ns16550.c which is using the 8250 headers...

Sure, because that's (even by its name, especially if considering the
intermediate steps) a direct successor.

Jan

Re: [PATCH v1] xen/serial: Return actual bytes stored in TX FIFO for OMAP

[Julien Grall]

On 27/10/15 11:05, Jan Beulich wrote:
>>>> On 27.10.15 at 11:54, <julien.grall@citrix.com> wrote:
>> On 27/10/15 10:50, Jan Beulich wrote:
>>>>>> On 27.10.15 at 11:36, <oleksandr.tyshchenko@globallogic.com> wrote:
>>>> --- a/xen/include/xen/8250-uart.h
>>>> +++ b/xen/include/xen/8250-uart.h
>>>> @@ -143,6 +143,15 @@
>>>>  /* Supplementary control register */
>>>>  #define UART_OMAP_SCR     0x10
>>>
>>> I think this one is already misplaced here (as is the one in the context
>>> below the change).
>>
>> I don't think so, the omap UART is based on the 8250 and share some
>> common registers.
> 
> Well, if it's that way then the patch is fine, but I then question why
> we have a separate omap-uart.c.

I've got no clue, the omap is a superset of the 8250 and actually we
already use the 8250 low-level serial for those platforms.

Linux is also having a separate driver for the OMAP UART. Oleksandr, is
there any fundamental difference between the OMAP and ns16550 driver?

Regards,

-- 
Julien Grall

Re: [PATCH v1] xen/serial: Return actual bytes stored in TX FIFO for OMAP

[Oleksandr Tyshchenko]

Hi, all.
Although OMAP and 8250 compatible UARTs have a lot of common, they
differs at least in configuration steps
since OMAP UART has such thing as "register access mode", also OMAP
UART has additional features and as a result a subset of additional
registers.

On Tue, Oct 27, 2015 at 1:15 PM, Julien Grall <julien.grall@citrix.com> wrote:
> On 27/10/15 11:05, Jan Beulich wrote:
>>>>> On 27.10.15 at 11:54, <julien.grall@citrix.com> wrote:
>>> On 27/10/15 10:50, Jan Beulich wrote:
>>>>>>> On 27.10.15 at 11:36, <oleksandr.tyshchenko@globallogic.com> wrote:
>>>>> --- a/xen/include/xen/8250-uart.h
>>>>> +++ b/xen/include/xen/8250-uart.h
>>>>> @@ -143,6 +143,15 @@
>>>>>  /* Supplementary control register */
>>>>>  #define UART_OMAP_SCR     0x10
>>>>
>>>> I think this one is already misplaced here (as is the one in the context
>>>> below the change).
>>>
>>> I don't think so, the omap UART is based on the 8250 and share some
>>> common registers.
>>
>> Well, if it's that way then the patch is fine, but I then question why
>> we have a separate omap-uart.c.
>
> I've got no clue, the omap is a superset of the 8250 and actually we
> already use the 8250 low-level serial for those platforms.
>
> Linux is also having a separate driver for the OMAP UART. Oleksandr, is
> there any fundamental difference between the OMAP and ns16550 driver?
>
> Regards,
>
> --
> Julien Grall



-- 

Oleksandr Tyshchenko | Embedded Dev
GlobalLogic
www.globallogic.com

Re: [PATCH v1] xen/serial: Return actual bytes stored in TX FIFO for OMAP

[Oleksandr Tyshchenko]

On Tue, Oct 27, 2015 at 1:33 PM, Oleksandr Tyshchenko
<oleksandr.tyshchenko@globallogic.com> wrote:
> Hi, all.
> Although OMAP and 8250 compatible UARTs have a lot of common, they
> differs at least in configuration steps
> since OMAP UART has such thing as "register access mode", also OMAP
> UART has additional features and as a result a subset of additional
> registers.
>
> On Tue, Oct 27, 2015 at 1:15 PM, Julien Grall <julien.grall@citrix.com> wrote:
>> On 27/10/15 11:05, Jan Beulich wrote:
>>>>>> On 27.10.15 at 11:54, <julien.grall@citrix.com> wrote:
>>>> On 27/10/15 10:50, Jan Beulich wrote:
>>>>>>>> On 27.10.15 at 11:36, <oleksandr.tyshchenko@globallogic.com> wrote:
>>>>>> --- a/xen/include/xen/8250-uart.h
>>>>>> +++ b/xen/include/xen/8250-uart.h
>>>>>> @@ -143,6 +143,15 @@
>>>>>>  /* Supplementary control register */
>>>>>>  #define UART_OMAP_SCR     0x10
>>>>>
>>>>> I think this one is already misplaced here (as is the one in the context
>>>>> below the change).
>>>>
>>>> I don't think so, the omap UART is based on the 8250 and share some
>>>> common registers.
>>>
>>> Well, if it's that way then the patch is fine, but I then question why
>>> we have a separate omap-uart.c.
>>
>> I've got no clue, the omap is a superset of the 8250 and actually we
>> already use the 8250 low-level serial for those platforms.

The 8250 low-level serial (debug-8250.inc) is suitable for OMAP based platforms
since performs common 8250 things without any initialization and
interrupt handling
unlike full OMAP UART driver in XEN.

>>
>> Linux is also having a separate driver for the OMAP UART. Oleksandr, is
>> there any fundamental difference between the OMAP and ns16550 driver?

The differences I have mentioned above can we treat as fundamental?
And what are you suggesting about the patch? Create a separate OMAP
specific header?

Thank you.

>>
>> Regards,
>>
>> --
>> Julien Grall
>
>
>
> --
>
> Oleksandr Tyshchenko | Embedded Dev
> GlobalLogic
> www.globallogic.com



-- 

Oleksandr Tyshchenko | Embedded Dev
GlobalLogic
www.globallogic.com

Re: [PATCH v1] xen/serial: Return actual bytes stored in TX FIFO for OMAP

[Jan Beulich]

>>> On 27.10.15 at 17:32, <oleksandr.tyshchenko@globallogic.com> wrote:
> And what are you suggesting about the patch? Create a separate OMAP
> specific header?

For #define-s used by only a single source file I'd suggest putting
them into the source file instead of into a (shared) header.

Jan

Re: [PATCH v1] xen/serial: Return actual bytes stored in TX FIFO for OMAP

[Ian Campbell]

On Tue, 2015-10-27 at 10:40 -0600, Jan Beulich wrote:
> > > > On 27.10.15 at 17:32, <oleksandr.tyshchenko@globallogic.com> wrote:
> > And what are you suggesting about the patch? Create a separate OMAP
> > specific header?
> 
> For #define-s used by only a single source file I'd suggest putting
> them into the source file instead of into a (shared) header.

I agree. (Any to be clear, any existing OMAP specific things in 8250,h
should move too).

Ian.

Re: [PATCH v1] xen/serial: Return actual bytes stored in TX FIFO for OMAP

[Oleksandr Tyshchenko]

On Tue, Nov 3, 2015 at 6:36 PM, Ian Campbell <ian.campbell@citrix.com> wrote:
> On Tue, 2015-10-27 at 10:40 -0600, Jan Beulich wrote:
>> > > > On 27.10.15 at 17:32, <oleksandr.tyshchenko@globallogic.com> wrote:
>> > And what are you suggesting about the patch? Create a separate OMAP
>> > specific header?
>>
>> For #define-s used by only a single source file I'd suggest putting
>> them into the source file instead of into a (shared) header.
>
> I agree. (Any to be clear, any existing OMAP specific things in 8250,h
> should move too).

OK. I will take it into account. Thank you

>
> Ian.
>



-- 

Oleksandr Tyshchenko | Embedded Dev
GlobalLogic
www.globallogic.com