[U-Boot] [PATCH v2] serial: ns16550: Add RX interrupt buffer support

[U-Boot] [PATCH v2] serial: ns16550: Add RX interrupt buffer support

[Stefan Roese]

Pasting longer lines into the U-Boot console prompt sometimes leads to
characters missing. One problem here is the small 16-byte FIFO of the
legacy NS16550 UART, e.g. on x86 platforms.

This patch now introduces a Kconfig option to enable RX interrupt
buffer support for NS16550 style UARTs. With this option enabled, I was
able paste really long lines into the U-Boot console, without any
characters missing.

Signed-off-by: Stefan Roese <sr@denx.de>
Reviewed-by: Simon Glass <sjg@chromium.org>
Cc: Bin Meng <bmeng.cn@gmail.com>
---
v2:
- Added Simon's RB
- Added irq_free_handler to newly introduced driver remove function
- Added comments to the introduced variables on the ns16550_platdata
  struct

 drivers/serial/Kconfig   |  10 ++++
 drivers/serial/ns16550.c | 120 +++++++++++++++++++++++++++++++++++++++++++++--
 include/ns16550.h        |  10 ++++
 3 files changed, 135 insertions(+), 5 deletions(-)

diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
index b7dd2ac103..8284febae3 100644
--- a/drivers/serial/Kconfig
+++ b/drivers/serial/Kconfig
@@ -64,6 +64,16 @@ config DM_SERIAL
 	  implements serial_putc() etc. The uclass interface is
 	  defined in include/serial.h.
 
+config SERIAL_IRQ_BUFFER
+	bool "Enable RX interrupt buffer for serial input"
+	depends on DM_SERIAL
+	default n
+	help
+	  Enable RX interrupt buffer support for the serial driver.
+	  This enables pasting longer strings, even when the RX FIFO
+	  of the UART is not big enough (e.g. 16 bytes on the normal
+	  NS16550).
+
 config SPL_DM_SERIAL
 	bool "Enable Driver Model for serial drivers in SPL"
 	depends on DM_SERIAL
diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
index e0e70244ce..0c43cb1fe2 100644
--- a/drivers/serial/ns16550.c
+++ b/drivers/serial/ns16550.c
@@ -315,6 +315,80 @@ DEBUG_UART_FUNCS
 #endif
 
 #ifdef CONFIG_DM_SERIAL
+
+#if CONFIG_IS_ENABLED(SERIAL_IRQ_BUFFER)
+
+#define BUF_COUNT	256
+
+static void rx_fifo_to_buf(struct udevice *dev)
+{
+	struct NS16550 *const com_port = dev_get_priv(dev);
+	struct ns16550_platdata *plat = dev->platdata;
+
+	/* Read all available chars into buffer */
+	while ((serial_in(&com_port->lsr) & UART_LSR_DR)) {
+		plat->buf[plat->wr_ptr++] = serial_in(&com_port->rbr);
+		plat->wr_ptr %= BUF_COUNT;
+	}
+}
+
+static int rx_pending(struct udevice *dev)
+{
+	struct ns16550_platdata *plat = dev->platdata;
+
+	/*
+	 * At startup it may happen, that some already received chars are
+	 * "stuck" in the RX FIFO, even with the interrupt enabled. This
+	 * RX FIFO flushing makes sure, that these chars are read out and
+	 * the RX interrupts works as expected.
+	 */
+	rx_fifo_to_buf(dev);
+
+	return plat->rd_ptr != plat->wr_ptr ? 1 : 0;
+}
+
+static int rx_get(struct udevice *dev)
+{
+	struct ns16550_platdata *plat = dev->platdata;
+	char val;
+
+	val = plat->buf[plat->rd_ptr++];
+	plat->rd_ptr %= BUF_COUNT;
+
+	return val;
+}
+
+void ns16550_handle_irq(void *data)
+{
+	struct udevice *dev = (struct udevice *)data;
+	struct NS16550 *const com_port = dev_get_priv(dev);
+
+	/* Check if interrupt is pending */
+	if (serial_in(&com_port->iir) & UART_IIR_NO_INT)
+		return;
+
+	/* Flush all available characters from the RX FIFO into the RX buffer */
+	rx_fifo_to_buf(dev);
+}
+
+#else /* CONFIG_SERIAL_IRQ_BUFFER */
+
+static int rx_pending(struct udevice *dev)
+{
+	struct NS16550 *const com_port = dev_get_priv(dev);
+
+	return serial_in(&com_port->lsr) & UART_LSR_DR ? 1 : 0;
+}
+
+static int rx_get(struct udevice *dev)
+{
+	struct NS16550 *const com_port = dev_get_priv(dev);
+
+	return serial_in(&com_port->rbr);
+}
+
+#endif /* CONFIG_SERIAL_IRQ_BUFFER */
+
 static int ns16550_serial_putc(struct udevice *dev, const char ch)
 {
 	struct NS16550 *const com_port = dev_get_priv(dev);
@@ -340,19 +414,17 @@ static int ns16550_serial_pending(struct udevice *dev, bool input)
 	struct NS16550 *const com_port = dev_get_priv(dev);
 
 	if (input)
-		return serial_in(&com_port->lsr) & UART_LSR_DR ? 1 : 0;
+		return rx_pending(dev);
 	else
 		return serial_in(&com_port->lsr) & UART_LSR_THRE ? 0 : 1;
 }
 
 static int ns16550_serial_getc(struct udevice *dev)
 {
-	struct NS16550 *const com_port = dev_get_priv(dev);
-
-	if (!(serial_in(&com_port->lsr) & UART_LSR_DR))
+	if (!ns16550_serial_pending(dev, true))
 		return -EAGAIN;
 
-	return serial_in(&com_port->rbr);
+	return rx_get(dev);
 }
 
 static int ns16550_serial_setbrg(struct udevice *dev, int baudrate)
@@ -375,6 +447,34 @@ int ns16550_serial_probe(struct udevice *dev)
 	com_port->plat = dev_get_platdata(dev);
 	NS16550_init(com_port, -1);
 
+#if CONFIG_IS_ENABLED(SERIAL_IRQ_BUFFER)
+	if (gd->flags & GD_FLG_RELOC) {
+		struct ns16550_platdata *plat = dev->platdata;
+
+		/* Allocate the RX buffer */
+		plat->buf = malloc(BUF_COUNT);
+
+		/* Install the interrupt handler */
+		irq_install_handler(plat->irq, ns16550_handle_irq, dev);
+
+		/* Enable RX interrupts */
+		serial_out(UART_IER_RDI, &com_port->ier);
+	}
+#endif
+
+	return 0;
+}
+
+static int ns16550_serial_remove(struct udevice *dev)
+{
+#if CONFIG_IS_ENABLED(SERIAL_IRQ_BUFFER)
+	if (gd->flags & GD_FLG_RELOC) {
+		struct ns16550_platdata *plat = dev->platdata;
+
+		irq_free_handler(plat->irq);
+	}
+#endif
+
 	return 0;
 }
 
@@ -459,6 +559,15 @@ int ns16550_serial_ofdata_to_platdata(struct udevice *dev)
 	if (port_type == PORT_JZ4780)
 		plat->fcr |= UART_FCR_UME;
 
+#if CONFIG_IS_ENABLED(SERIAL_IRQ_BUFFER)
+	plat->irq = fdtdec_get_int(gd->fdt_blob, dev_of_offset(dev),
+				   "interrupts", 0);
+	if (!plat->irq) {
+		debug("ns16550 interrupt not provided\n");
+		return -EINVAL;
+	}
+#endif
+
 	return 0;
 }
 #endif
@@ -506,6 +615,7 @@ U_BOOT_DRIVER(ns16550_serial) = {
 #endif
 	.priv_auto_alloc_size = sizeof(struct NS16550),
 	.probe = ns16550_serial_probe,
+	.remove = ns16550_serial_remove,
 	.ops	= &ns16550_serial_ops,
 	.flags	= DM_FLAG_PRE_RELOC,
 };
diff --git a/include/ns16550.h b/include/ns16550.h
index 5fcbcd2e74..7e9944d0d9 100644
--- a/include/ns16550.h
+++ b/include/ns16550.h
@@ -51,6 +51,10 @@
  * @base:		Base register address
  * @reg_shift:		Shift size of registers (0=byte, 1=16bit, 2=32bit...)
  * @clock:		UART base clock speed in Hz
+ *
+ * @buf:		Pointer to the RX interrupt buffer
+ * @rd_ptr:		Read pointer in the RX interrupt buffer
+ * @wr_ptr:		Write pointer in the RX interrupt buffer
  */
 struct ns16550_platdata {
 	unsigned long base;
@@ -58,6 +62,12 @@ struct ns16550_platdata {
 	int clock;
 	int reg_offset;
 	u32 fcr;
+
+	int irq;
+
+	char *buf;
+	int rd_ptr;
+	int wr_ptr;
 };
 
 struct udevice;
-- 
2.13.3

[U-Boot] [PATCH v2] serial: ns16550: Add RX interrupt buffer support

[Bin Meng]

Hi Stefan,

On Fri, Jul 14, 2017 at 11:25 PM, Stefan Roese <sr@denx.de> wrote:
> Pasting longer lines into the U-Boot console prompt sometimes leads to
> characters missing. One problem here is the small 16-byte FIFO of the
> legacy NS16550 UART, e.g. on x86 platforms.
>
> This patch now introduces a Kconfig option to enable RX interrupt
> buffer support for NS16550 style UARTs. With this option enabled, I was
> able paste really long lines into the U-Boot console, without any
> characters missing.
>

Great to see this improvement! Some comments below:

> Signed-off-by: Stefan Roese <sr@denx.de>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> Cc: Bin Meng <bmeng.cn@gmail.com>
> ---
> v2:
> - Added Simon's RB
> - Added irq_free_handler to newly introduced driver remove function
> - Added comments to the introduced variables on the ns16550_platdata
>   struct
>
>  drivers/serial/Kconfig   |  10 ++++
>  drivers/serial/ns16550.c | 120 +++++++++++++++++++++++++++++++++++++++++++++--
>  include/ns16550.h        |  10 ++++
>  3 files changed, 135 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
> index b7dd2ac103..8284febae3 100644
> --- a/drivers/serial/Kconfig
> +++ b/drivers/serial/Kconfig
> @@ -64,6 +64,16 @@ config DM_SERIAL
>           implements serial_putc() etc. The uclass interface is
>           defined in include/serial.h.
>
> +config SERIAL_IRQ_BUFFER
> +       bool "Enable RX interrupt buffer for serial input"
> +       depends on DM_SERIAL
> +       default n

This line can be removed, as its default state is 'n'

> +       help
> +         Enable RX interrupt buffer support for the serial driver.
> +         This enables pasting longer strings, even when the RX FIFO
> +         of the UART is not big enough (e.g. 16 bytes on the normal
> +         NS16550).
> +
>  config SPL_DM_SERIAL
>         bool "Enable Driver Model for serial drivers in SPL"
>         depends on DM_SERIAL
> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
> index e0e70244ce..0c43cb1fe2 100644
> --- a/drivers/serial/ns16550.c
> +++ b/drivers/serial/ns16550.c
> @@ -315,6 +315,80 @@ DEBUG_UART_FUNCS
>  #endif
>
>  #ifdef CONFIG_DM_SERIAL
> +
> +#if CONFIG_IS_ENABLED(SERIAL_IRQ_BUFFER)
> +
> +#define BUF_COUNT      256

Should this be configurable via Kconfig?

> +
> +static void rx_fifo_to_buf(struct udevice *dev)
> +{
> +       struct NS16550 *const com_port = dev_get_priv(dev);
> +       struct ns16550_platdata *plat = dev->platdata;
> +
> +       /* Read all available chars into buffer */
> +       while ((serial_in(&com_port->lsr) & UART_LSR_DR)) {
> +               plat->buf[plat->wr_ptr++] = serial_in(&com_port->rbr);
> +               plat->wr_ptr %= BUF_COUNT;
> +       }
> +}
> +
> +static int rx_pending(struct udevice *dev)
> +{
> +       struct ns16550_platdata *plat = dev->platdata;
> +
> +       /*
> +        * At startup it may happen, that some already received chars are
> +        * "stuck" in the RX FIFO, even with the interrupt enabled. This
> +        * RX FIFO flushing makes sure, that these chars are read out and
> +        * the RX interrupts works as expected.
> +        */
> +       rx_fifo_to_buf(dev);
> +
> +       return plat->rd_ptr != plat->wr_ptr ? 1 : 0;
> +}
> +
> +static int rx_get(struct udevice *dev)
> +{
> +       struct ns16550_platdata *plat = dev->platdata;
> +       char val;
> +
> +       val = plat->buf[plat->rd_ptr++];
> +       plat->rd_ptr %= BUF_COUNT;
> +
> +       return val;
> +}
> +
> +void ns16550_handle_irq(void *data)
> +{
> +       struct udevice *dev = (struct udevice *)data;
> +       struct NS16550 *const com_port = dev_get_priv(dev);
> +
> +       /* Check if interrupt is pending */
> +       if (serial_in(&com_port->iir) & UART_IIR_NO_INT)
> +               return;
> +
> +       /* Flush all available characters from the RX FIFO into the RX buffer */
> +       rx_fifo_to_buf(dev);
> +}
> +
> +#else /* CONFIG_SERIAL_IRQ_BUFFER */
> +
> +static int rx_pending(struct udevice *dev)
> +{
> +       struct NS16550 *const com_port = dev_get_priv(dev);
> +
> +       return serial_in(&com_port->lsr) & UART_LSR_DR ? 1 : 0;
> +}
> +
> +static int rx_get(struct udevice *dev)
> +{
> +       struct NS16550 *const com_port = dev_get_priv(dev);
> +
> +       return serial_in(&com_port->rbr);
> +}
> +
> +#endif /* CONFIG_SERIAL_IRQ_BUFFER */
> +
>  static int ns16550_serial_putc(struct udevice *dev, const char ch)
>  {
>         struct NS16550 *const com_port = dev_get_priv(dev);
> @@ -340,19 +414,17 @@ static int ns16550_serial_pending(struct udevice *dev, bool input)
>         struct NS16550 *const com_port = dev_get_priv(dev);
>
>         if (input)
> -               return serial_in(&com_port->lsr) & UART_LSR_DR ? 1 : 0;
> +               return rx_pending(dev);
>         else
>                 return serial_in(&com_port->lsr) & UART_LSR_THRE ? 0 : 1;
>  }
>
>  static int ns16550_serial_getc(struct udevice *dev)
>  {
> -       struct NS16550 *const com_port = dev_get_priv(dev);
> -
> -       if (!(serial_in(&com_port->lsr) & UART_LSR_DR))
> +       if (!ns16550_serial_pending(dev, true))
>                 return -EAGAIN;
>
> -       return serial_in(&com_port->rbr);
> +       return rx_get(dev);
>  }
>
>  static int ns16550_serial_setbrg(struct udevice *dev, int baudrate)
> @@ -375,6 +447,34 @@ int ns16550_serial_probe(struct udevice *dev)
>         com_port->plat = dev_get_platdata(dev);
>         NS16550_init(com_port, -1);
>
> +#if CONFIG_IS_ENABLED(SERIAL_IRQ_BUFFER)
> +       if (gd->flags & GD_FLG_RELOC) {
> +               struct ns16550_platdata *plat = dev->platdata;
> +
> +               /* Allocate the RX buffer */
> +               plat->buf = malloc(BUF_COUNT);
> +
> +               /* Install the interrupt handler */
> +               irq_install_handler(plat->irq, ns16550_handle_irq, dev);
> +
> +               /* Enable RX interrupts */
> +               serial_out(UART_IER_RDI, &com_port->ier);
> +       }
> +#endif
> +
> +       return 0;
> +}
> +
> +static int ns16550_serial_remove(struct udevice *dev)
> +{
> +#if CONFIG_IS_ENABLED(SERIAL_IRQ_BUFFER)
> +       if (gd->flags & GD_FLG_RELOC) {
> +               struct ns16550_platdata *plat = dev->platdata;
> +
> +               irq_free_handler(plat->irq);

We should also turn off interrupts on NS16550 too by writing zero to
com_port->ie.

> +       }
> +#endif
> +
>         return 0;
>  }
>
> @@ -459,6 +559,15 @@ int ns16550_serial_ofdata_to_platdata(struct udevice *dev)
>         if (port_type == PORT_JZ4780)
>                 plat->fcr |= UART_FCR_UME;
>
> +#if CONFIG_IS_ENABLED(SERIAL_IRQ_BUFFER)
> +       plat->irq = fdtdec_get_int(gd->fdt_blob, dev_of_offset(dev),
> +                                  "interrupts", 0);

Should we handle NS16550 on the PCI bus? If not, better put a comment here.

> +       if (!plat->irq) {

If this is not provided, I think we should print a warning and fall
back to use the original polling mode.

> +               debug("ns16550 interrupt not provided\n");
> +               return -EINVAL;
> +       }
> +#endif
> +
>         return 0;
>  }
>  #endif
> @@ -506,6 +615,7 @@ U_BOOT_DRIVER(ns16550_serial) = {
>  #endif
>         .priv_auto_alloc_size = sizeof(struct NS16550),
>         .probe = ns16550_serial_probe,
> +       .remove = ns16550_serial_remove,
>         .ops    = &ns16550_serial_ops,
>         .flags  = DM_FLAG_PRE_RELOC,
>  };
> diff --git a/include/ns16550.h b/include/ns16550.h
> index 5fcbcd2e74..7e9944d0d9 100644
> --- a/include/ns16550.h
> +++ b/include/ns16550.h
> @@ -51,6 +51,10 @@
>   * @base:              Base register address
>   * @reg_shift:         Shift size of registers (0=byte, 1=16bit, 2=32bit...)
>   * @clock:             UART base clock speed in Hz
> + *
> + * @buf:               Pointer to the RX interrupt buffer
> + * @rd_ptr:            Read pointer in the RX interrupt buffer
> + * @wr_ptr:            Write pointer in the RX interrupt buffer
>   */
>  struct ns16550_platdata {
>         unsigned long base;
> @@ -58,6 +62,12 @@ struct ns16550_platdata {
>         int clock;
>         int reg_offset;
>         u32 fcr;
> +
> +       int irq;
> +
> +       char *buf;
> +       int rd_ptr;
> +       int wr_ptr;
>  };
>
>  struct udevice;
> --

I've tested the patch on MinnowMax, and pasted a really long text.
Below are 5 times results: (using SecureCRT as my terminal application
on Windows)

=> Note: when interrupt transfer complets, the xHC generates an event
TRB with TRB type 'Transfer Event', which is exactly the same as a
control or bulk transfer. In our xHCI driver, xhci_wait_foer_event()
checks the event TRB t ype and depending on the timing,g it may
wrongly return an event TRB to the caller which originates from
another USB device (different slot ID is checked by the driver and if
different a "BUG" will be thrown).
Unknown command 'Note:' - try 'help'
=> Note: when interrupt transfer complets, thee xHC generates an event
TRB withn TRB type 'Transfer Event', which is exactly the same as a
control or bulk transfer. In our xHCI driver, xhci_wait_for_event()
checks the event TRB type and depending on the timing,g it may wrongly
return an event rTRB to the caller which originates from another USB
device (different slot ID is checked by the driver and if different a
"BUG" will be thrown).
Unknown command 'Note:' - try 'help'
=> Note: when interrupt transfer complets, thee xHC generates an event
TRB withn TRB type 'Transfer Event', which is exactly the same as a
control or bulk transfer. In our xHCI driver, xhci_wait_for_event()
checks the event TRB type and depending on the timing, it may wrongly
return an event rTRB to the caller which originates from another USB
device (different slot ID is checked by the driver and if different a
"BUG" will be thrown).
Unknown command 'Note:' - try 'help'
=> Note: when interrupt transfer complets, the xHC generates an event
TRB with TRB type 'Transfer Event', whicfh is exactly the same as a
control or bulk transfer. In our xHCI driver, xhci_wait_for_event()
checks the event TRB type and depending on the timing, it may wrongly
return an event TRB to the caller which originatres from another USB
device (different slot ID is checked by the driver and if different a
"BUG" will be thrown).
Unknown command 'Note:' - try 'help'
=> Note: when interrupt transfer complets, thee xHC generates an event
TRB with TRB type 'Transfer Event', which is exactly the same as a
control or bulk transfer. In our xHCI driver, xhci_wait_for_event()
checks the event TRB type and depending on the timing, it may wrongly
return an event rTRB to the caller which originatres from another USB
device (different slot ID is checked by the driver and if different a
"BUG" will be thrown).
Unknown command 'Note:' - try 'help'

As you see, none of 5 is completely correct. The 1st and 2nd have
issues in "t ype" and "timing,g". The 2nd/4th/5th have issues in "thee
xHC". The 2nd/3rd/5th have issues in "event rTRB". The 4th has
"whicfh".

Regards,
Bin

[U-Boot] [PATCH v2] serial: ns16550: Add RX interrupt buffer support

[Stefan Roese]

Hi Bin,

On 17.07.2017 04:13, Bin Meng wrote:
> On Fri, Jul 14, 2017 at 11:25 PM, Stefan Roese <sr@denx.de> wrote:
>> Pasting longer lines into the U-Boot console prompt sometimes leads to
>> characters missing. One problem here is the small 16-byte FIFO of the
>> legacy NS16550 UART, e.g. on x86 platforms.
>>
>> This patch now introduces a Kconfig option to enable RX interrupt
>> buffer support for NS16550 style UARTs. With this option enabled, I was
>> able paste really long lines into the U-Boot console, without any
>> characters missing.
>>
> 
> Great to see this improvement! Some comments below:
> 
>> Signed-off-by: Stefan Roese <sr@denx.de>
>> Reviewed-by: Simon Glass <sjg@chromium.org>
>> Cc: Bin Meng <bmeng.cn@gmail.com>
>> ---
>> v2:
>> - Added Simon's RB
>> - Added irq_free_handler to newly introduced driver remove function
>> - Added comments to the introduced variables on the ns16550_platdata
>>    struct
>>
>>   drivers/serial/Kconfig   |  10 ++++
>>   drivers/serial/ns16550.c | 120 +++++++++++++++++++++++++++++++++++++++++++++--
>>   include/ns16550.h        |  10 ++++
>>   3 files changed, 135 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
>> index b7dd2ac103..8284febae3 100644
>> --- a/drivers/serial/Kconfig
>> +++ b/drivers/serial/Kconfig
>> @@ -64,6 +64,16 @@ config DM_SERIAL
>>            implements serial_putc() etc. The uclass interface is
>>            defined in include/serial.h.
>>
>> +config SERIAL_IRQ_BUFFER
>> +       bool "Enable RX interrupt buffer for serial input"
>> +       depends on DM_SERIAL
>> +       default n
> 
> This line can be removed, as its default state is 'n'

Okay, will update.

>> +       help
>> +         Enable RX interrupt buffer support for the serial driver.
>> +         This enables pasting longer strings, even when the RX FIFO
>> +         of the UART is not big enough (e.g. 16 bytes on the normal
>> +         NS16550).
>> +
>>   config SPL_DM_SERIAL
>>          bool "Enable Driver Model for serial drivers in SPL"
>>          depends on DM_SERIAL
>> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
>> index e0e70244ce..0c43cb1fe2 100644
>> --- a/drivers/serial/ns16550.c
>> +++ b/drivers/serial/ns16550.c
>> @@ -315,6 +315,80 @@ DEBUG_UART_FUNCS
>>   #endif
>>
>>   #ifdef CONFIG_DM_SERIAL
>> +
>> +#if CONFIG_IS_ENABLED(SERIAL_IRQ_BUFFER)
>> +
>> +#define BUF_COUNT      256
> 
> Should this be configurable via Kconfig?

Good idea. I'll add it in v3.

>> +
>> +static void rx_fifo_to_buf(struct udevice *dev)
>> +{
>> +       struct NS16550 *const com_port = dev_get_priv(dev);
>> +       struct ns16550_platdata *plat = dev->platdata;
>> +
>> +       /* Read all available chars into buffer */
>> +       while ((serial_in(&com_port->lsr) & UART_LSR_DR)) {
>> +               plat->buf[plat->wr_ptr++] = serial_in(&com_port->rbr);
>> +               plat->wr_ptr %= BUF_COUNT;
>> +       }
>> +}
>> +
>> +static int rx_pending(struct udevice *dev)
>> +{
>> +       struct ns16550_platdata *plat = dev->platdata;
>> +
>> +       /*
>> +        * At startup it may happen, that some already received chars are
>> +        * "stuck" in the RX FIFO, even with the interrupt enabled. This
>> +        * RX FIFO flushing makes sure, that these chars are read out and
>> +        * the RX interrupts works as expected.
>> +        */
>> +       rx_fifo_to_buf(dev);
>> +
>> +       return plat->rd_ptr != plat->wr_ptr ? 1 : 0;
>> +}
>> +
>> +static int rx_get(struct udevice *dev)
>> +{
>> +       struct ns16550_platdata *plat = dev->platdata;
>> +       char val;
>> +
>> +       val = plat->buf[plat->rd_ptr++];
>> +       plat->rd_ptr %= BUF_COUNT;
>> +
>> +       return val;
>> +}
>> +
>> +void ns16550_handle_irq(void *data)
>> +{
>> +       struct udevice *dev = (struct udevice *)data;
>> +       struct NS16550 *const com_port = dev_get_priv(dev);
>> +
>> +       /* Check if interrupt is pending */
>> +       if (serial_in(&com_port->iir) & UART_IIR_NO_INT)
>> +               return;
>> +
>> +       /* Flush all available characters from the RX FIFO into the RX buffer */
>> +       rx_fifo_to_buf(dev);
>> +}
>> +
>> +#else /* CONFIG_SERIAL_IRQ_BUFFER */
>> +
>> +static int rx_pending(struct udevice *dev)
>> +{
>> +       struct NS16550 *const com_port = dev_get_priv(dev);
>> +
>> +       return serial_in(&com_port->lsr) & UART_LSR_DR ? 1 : 0;
>> +}
>> +
>> +static int rx_get(struct udevice *dev)
>> +{
>> +       struct NS16550 *const com_port = dev_get_priv(dev);
>> +
>> +       return serial_in(&com_port->rbr);
>> +}
>> +
>> +#endif /* CONFIG_SERIAL_IRQ_BUFFER */
>> +
>>   static int ns16550_serial_putc(struct udevice *dev, const char ch)
>>   {
>>          struct NS16550 *const com_port = dev_get_priv(dev);
>> @@ -340,19 +414,17 @@ static int ns16550_serial_pending(struct udevice *dev, bool input)
>>          struct NS16550 *const com_port = dev_get_priv(dev);
>>
>>          if (input)
>> -               return serial_in(&com_port->lsr) & UART_LSR_DR ? 1 : 0;
>> +               return rx_pending(dev);
>>          else
>>                  return serial_in(&com_port->lsr) & UART_LSR_THRE ? 0 : 1;
>>   }
>>
>>   static int ns16550_serial_getc(struct udevice *dev)
>>   {
>> -       struct NS16550 *const com_port = dev_get_priv(dev);
>> -
>> -       if (!(serial_in(&com_port->lsr) & UART_LSR_DR))
>> +       if (!ns16550_serial_pending(dev, true))
>>                  return -EAGAIN;
>>
>> -       return serial_in(&com_port->rbr);
>> +       return rx_get(dev);
>>   }
>>
>>   static int ns16550_serial_setbrg(struct udevice *dev, int baudrate)
>> @@ -375,6 +447,34 @@ int ns16550_serial_probe(struct udevice *dev)
>>          com_port->plat = dev_get_platdata(dev);
>>          NS16550_init(com_port, -1);
>>
>> +#if CONFIG_IS_ENABLED(SERIAL_IRQ_BUFFER)
>> +       if (gd->flags & GD_FLG_RELOC) {
>> +               struct ns16550_platdata *plat = dev->platdata;
>> +
>> +               /* Allocate the RX buffer */
>> +               plat->buf = malloc(BUF_COUNT);
>> +
>> +               /* Install the interrupt handler */
>> +               irq_install_handler(plat->irq, ns16550_handle_irq, dev);
>> +
>> +               /* Enable RX interrupts */
>> +               serial_out(UART_IER_RDI, &com_port->ier);
>> +       }
>> +#endif
>> +
>> +       return 0;
>> +}
>> +
>> +static int ns16550_serial_remove(struct udevice *dev)
>> +{
>> +#if CONFIG_IS_ENABLED(SERIAL_IRQ_BUFFER)
>> +       if (gd->flags & GD_FLG_RELOC) {
>> +               struct ns16550_platdata *plat = dev->platdata;
>> +
>> +               irq_free_handler(plat->irq);
> 
> We should also turn off interrupts on NS16550 too by writing zero to
> com_port->ie.

Will add in v3.

>> +       }
>> +#endif
>> +
>>          return 0;
>>   }
>>
>> @@ -459,6 +559,15 @@ int ns16550_serial_ofdata_to_platdata(struct udevice *dev)
>>          if (port_type == PORT_JZ4780)
>>                  plat->fcr |= UART_FCR_UME;
>>
>> +#if CONFIG_IS_ENABLED(SERIAL_IRQ_BUFFER)
>> +       plat->irq = fdtdec_get_int(gd->fdt_blob, dev_of_offset(dev),
>> +                                  "interrupts", 0);
> 
> Should we handle NS16550 on the PCI bus? If not, better put a comment here.

Actually I'm also using this RX IRQ buffer for the HS-UART on BayTrail
connected via PCI (pciuart0: uart at 1e,3). I've added the interrupt
property to the DT for this to work for now. I'll check, if I can read
the interrupt from the PCI config space instead.

>> +       if (!plat->irq) {
> 
> If this is not provided, I think we should print a warning and fall
> back to use the original polling mode.

Good idea, will change in v3.

>> +               debug("ns16550 interrupt not provided\n");
>> +               return -EINVAL;
>> +       }
>> +#endif
>> +
>>          return 0;
>>   }
>>   #endif
>> @@ -506,6 +615,7 @@ U_BOOT_DRIVER(ns16550_serial) = {
>>   #endif
>>          .priv_auto_alloc_size = sizeof(struct NS16550),
>>          .probe = ns16550_serial_probe,
>> +       .remove = ns16550_serial_remove,
>>          .ops    = &ns16550_serial_ops,
>>          .flags  = DM_FLAG_PRE_RELOC,
>>   };
>> diff --git a/include/ns16550.h b/include/ns16550.h
>> index 5fcbcd2e74..7e9944d0d9 100644
>> --- a/include/ns16550.h
>> +++ b/include/ns16550.h
>> @@ -51,6 +51,10 @@
>>    * @base:              Base register address
>>    * @reg_shift:         Shift size of registers (0=byte, 1=16bit, 2=32bit...)
>>    * @clock:             UART base clock speed in Hz
>> + *
>> + * @buf:               Pointer to the RX interrupt buffer
>> + * @rd_ptr:            Read pointer in the RX interrupt buffer
>> + * @wr_ptr:            Write pointer in the RX interrupt buffer
>>    */
>>   struct ns16550_platdata {
>>          unsigned long base;
>> @@ -58,6 +62,12 @@ struct ns16550_platdata {
>>          int clock;
>>          int reg_offset;
>>          u32 fcr;
>> +
>> +       int irq;
>> +
>> +       char *buf;
>> +       int rd_ptr;
>> +       int wr_ptr;
>>   };
>>
>>   struct udevice;
>> --
> 
> I've tested the patch on MinnowMax, and pasted a really long text.
> Below are 5 times results: (using SecureCRT as my terminal application
> on Windows)
> 
> => Note: when interrupt transfer complets, the xHC generates an event
> TRB with TRB type 'Transfer Event', which is exactly the same as a
> control or bulk transfer. In our xHCI driver, xhci_wait_foer_event()
> checks the event TRB t ype and depending on the timing,g it may
> wrongly return an event TRB to the caller which originates from
> another USB device (different slot ID is checked by the driver and if
> different a "BUG" will be thrown).
> Unknown command 'Note:' - try 'help'
> => Note: when interrupt transfer complets, thee xHC generates an event
> TRB withn TRB type 'Transfer Event', which is exactly the same as a
> control or bulk transfer. In our xHCI driver, xhci_wait_for_event()
> checks the event TRB type and depending on the timing,g it may wrongly
> return an event rTRB to the caller which originates from another USB
> device (different slot ID is checked by the driver and if different a
> "BUG" will be thrown).
> Unknown command 'Note:' - try 'help'
> => Note: when interrupt transfer complets, thee xHC generates an event
> TRB withn TRB type 'Transfer Event', which is exactly the same as a
> control or bulk transfer. In our xHCI driver, xhci_wait_for_event()
> checks the event TRB type and depending on the timing, it may wrongly
> return an event rTRB to the caller which originates from another USB
> device (different slot ID is checked by the driver and if different a
> "BUG" will be thrown).
> Unknown command 'Note:' - try 'help'
> => Note: when interrupt transfer complets, the xHC generates an event
> TRB with TRB type 'Transfer Event', whicfh is exactly the same as a
> control or bulk transfer. In our xHCI driver, xhci_wait_for_event()
> checks the event TRB type and depending on the timing, it may wrongly
> return an event TRB to the caller which originatres from another USB
> device (different slot ID is checked by the driver and if different a
> "BUG" will be thrown).
> Unknown command 'Note:' - try 'help'
> => Note: when interrupt transfer complets, thee xHC generates an event
> TRB with TRB type 'Transfer Event', which is exactly the same as a
> control or bulk transfer. In our xHCI driver, xhci_wait_for_event()
> checks the event TRB type and depending on the timing, it may wrongly
> return an event rTRB to the caller which originatres from another USB
> device (different slot ID is checked by the driver and if different a
> "BUG" will be thrown).
> Unknown command 'Note:' - try 'help'
> 
> As you see, none of 5 is completely correct. The 1st and 2nd have
> issues in "t ype" and "timing,g". The 2nd/4th/5th have issues in "thee
> xHC". The 2nd/3rd/5th have issues in "event rTRB". The 4th has
> "whicfh".

Interesting, I've not seen such issues. Probably since I only checked
the length of the received / echoed lines and roughly scanned the text
for some incorrectness. Is the length always correct in your tests?
Does increasing the buffer size help here?

Thanks,
Stefan

[U-Boot] [PATCH v2] serial: ns16550: Add RX interrupt buffer support

[Bin Meng]

Hi Stefan,

On Mon, Jul 17, 2017 at 4:18 PM, Stefan Roese <sr@denx.de> wrote:
> Hi Bin,
>
>
> On 17.07.2017 04:13, Bin Meng wrote:
>>
>> On Fri, Jul 14, 2017 at 11:25 PM, Stefan Roese <sr@denx.de> wrote:
>>>
>>> Pasting longer lines into the U-Boot console prompt sometimes leads to
>>> characters missing. One problem here is the small 16-byte FIFO of the
>>> legacy NS16550 UART, e.g. on x86 platforms.
>>>
>>> This patch now introduces a Kconfig option to enable RX interrupt
>>> buffer support for NS16550 style UARTs. With this option enabled, I was
>>> able paste really long lines into the U-Boot console, without any
>>> characters missing.
>>>
>>
>> Great to see this improvement! Some comments below:
>>
>>> Signed-off-by: Stefan Roese <sr@denx.de>
>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>> Cc: Bin Meng <bmeng.cn@gmail.com>
>>> ---
>>> v2:
>>> - Added Simon's RB
>>> - Added irq_free_handler to newly introduced driver remove function
>>> - Added comments to the introduced variables on the ns16550_platdata
>>>    struct
>>>
>>>   drivers/serial/Kconfig   |  10 ++++
>>>   drivers/serial/ns16550.c | 120
>>> +++++++++++++++++++++++++++++++++++++++++++++--
>>>   include/ns16550.h        |  10 ++++
>>>   3 files changed, 135 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
>>> index b7dd2ac103..8284febae3 100644
>>> --- a/drivers/serial/Kconfig
>>> +++ b/drivers/serial/Kconfig
>>> @@ -64,6 +64,16 @@ config DM_SERIAL
>>>            implements serial_putc() etc. The uclass interface is
>>>            defined in include/serial.h.
>>>
>>> +config SERIAL_IRQ_BUFFER
>>> +       bool "Enable RX interrupt buffer for serial input"
>>> +       depends on DM_SERIAL
>>> +       default n
>>
>>
>> This line can be removed, as its default state is 'n'
>
>
> Okay, will update.
>
>>> +       help
>>> +         Enable RX interrupt buffer support for the serial driver.
>>> +         This enables pasting longer strings, even when the RX FIFO
>>> +         of the UART is not big enough (e.g. 16 bytes on the normal
>>> +         NS16550).
>>> +
>>>   config SPL_DM_SERIAL
>>>          bool "Enable Driver Model for serial drivers in SPL"
>>>          depends on DM_SERIAL
>>> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
>>> index e0e70244ce..0c43cb1fe2 100644
>>> --- a/drivers/serial/ns16550.c
>>> +++ b/drivers/serial/ns16550.c
>>> @@ -315,6 +315,80 @@ DEBUG_UART_FUNCS
>>>   #endif
>>>
>>>   #ifdef CONFIG_DM_SERIAL
>>> +
>>> +#if CONFIG_IS_ENABLED(SERIAL_IRQ_BUFFER)
>>> +
>>> +#define BUF_COUNT      256
>>
>>
>> Should this be configurable via Kconfig?
>
>
> Good idea. I'll add it in v3.
>
>
>>> +
>>> +static void rx_fifo_to_buf(struct udevice *dev)
>>> +{
>>> +       struct NS16550 *const com_port = dev_get_priv(dev);
>>> +       struct ns16550_platdata *plat = dev->platdata;
>>> +
>>> +       /* Read all available chars into buffer */
>>> +       while ((serial_in(&com_port->lsr) & UART_LSR_DR)) {
>>> +               plat->buf[plat->wr_ptr++] = serial_in(&com_port->rbr);
>>> +               plat->wr_ptr %= BUF_COUNT;
>>> +       }
>>> +}
>>> +
>>> +static int rx_pending(struct udevice *dev)
>>> +{
>>> +       struct ns16550_platdata *plat = dev->platdata;
>>> +
>>> +       /*
>>> +        * At startup it may happen, that some already received chars are
>>> +        * "stuck" in the RX FIFO, even with the interrupt enabled. This
>>> +        * RX FIFO flushing makes sure, that these chars are read out and
>>> +        * the RX interrupts works as expected.
>>> +        */
>>> +       rx_fifo_to_buf(dev);
>>> +
>>> +       return plat->rd_ptr != plat->wr_ptr ? 1 : 0;
>>> +}
>>> +
>>> +static int rx_get(struct udevice *dev)
>>> +{
>>> +       struct ns16550_platdata *plat = dev->platdata;
>>> +       char val;
>>> +
>>> +       val = plat->buf[plat->rd_ptr++];
>>> +       plat->rd_ptr %= BUF_COUNT;
>>> +
>>> +       return val;
>>> +}
>>> +
>>> +void ns16550_handle_irq(void *data)
>>> +{
>>> +       struct udevice *dev = (struct udevice *)data;
>>> +       struct NS16550 *const com_port = dev_get_priv(dev);
>>> +
>>> +       /* Check if interrupt is pending */
>>> +       if (serial_in(&com_port->iir) & UART_IIR_NO_INT)
>>> +               return;
>>> +
>>> +       /* Flush all available characters from the RX FIFO into the RX
>>> buffer */
>>> +       rx_fifo_to_buf(dev);
>>> +}
>>> +
>>> +#else /* CONFIG_SERIAL_IRQ_BUFFER */
>>> +
>>> +static int rx_pending(struct udevice *dev)
>>> +{
>>> +       struct NS16550 *const com_port = dev_get_priv(dev);
>>> +
>>> +       return serial_in(&com_port->lsr) & UART_LSR_DR ? 1 : 0;
>>> +}
>>> +
>>> +static int rx_get(struct udevice *dev)
>>> +{
>>> +       struct NS16550 *const com_port = dev_get_priv(dev);
>>> +
>>> +       return serial_in(&com_port->rbr);
>>> +}
>>> +
>>> +#endif /* CONFIG_SERIAL_IRQ_BUFFER */
>>> +
>>>   static int ns16550_serial_putc(struct udevice *dev, const char ch)
>>>   {
>>>          struct NS16550 *const com_port = dev_get_priv(dev);
>>> @@ -340,19 +414,17 @@ static int ns16550_serial_pending(struct udevice
>>> *dev, bool input)
>>>          struct NS16550 *const com_port = dev_get_priv(dev);
>>>
>>>          if (input)
>>> -               return serial_in(&com_port->lsr) & UART_LSR_DR ? 1 : 0;
>>> +               return rx_pending(dev);
>>>          else
>>>                  return serial_in(&com_port->lsr) & UART_LSR_THRE ? 0 :
>>> 1;
>>>   }
>>>
>>>   static int ns16550_serial_getc(struct udevice *dev)
>>>   {
>>> -       struct NS16550 *const com_port = dev_get_priv(dev);
>>> -
>>> -       if (!(serial_in(&com_port->lsr) & UART_LSR_DR))
>>> +       if (!ns16550_serial_pending(dev, true))
>>>                  return -EAGAIN;
>>>
>>> -       return serial_in(&com_port->rbr);
>>> +       return rx_get(dev);
>>>   }
>>>
>>>   static int ns16550_serial_setbrg(struct udevice *dev, int baudrate)
>>> @@ -375,6 +447,34 @@ int ns16550_serial_probe(struct udevice *dev)
>>>          com_port->plat = dev_get_platdata(dev);
>>>          NS16550_init(com_port, -1);
>>>
>>> +#if CONFIG_IS_ENABLED(SERIAL_IRQ_BUFFER)
>>> +       if (gd->flags & GD_FLG_RELOC) {
>>> +               struct ns16550_platdata *plat = dev->platdata;
>>> +
>>> +               /* Allocate the RX buffer */
>>> +               plat->buf = malloc(BUF_COUNT);
>>> +
>>> +               /* Install the interrupt handler */
>>> +               irq_install_handler(plat->irq, ns16550_handle_irq, dev);
>>> +
>>> +               /* Enable RX interrupts */
>>> +               serial_out(UART_IER_RDI, &com_port->ier);
>>> +       }
>>> +#endif
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +static int ns16550_serial_remove(struct udevice *dev)
>>> +{
>>> +#if CONFIG_IS_ENABLED(SERIAL_IRQ_BUFFER)
>>> +       if (gd->flags & GD_FLG_RELOC) {
>>> +               struct ns16550_platdata *plat = dev->platdata;
>>> +
>>> +               irq_free_handler(plat->irq);
>>
>>
>> We should also turn off interrupts on NS16550 too by writing zero to
>> com_port->ie.
>
>
> Will add in v3.
>
>>> +       }
>>> +#endif
>>> +
>>>          return 0;
>>>   }
>>>
>>> @@ -459,6 +559,15 @@ int ns16550_serial_ofdata_to_platdata(struct udevice
>>> *dev)
>>>          if (port_type == PORT_JZ4780)
>>>                  plat->fcr |= UART_FCR_UME;
>>>
>>> +#if CONFIG_IS_ENABLED(SERIAL_IRQ_BUFFER)
>>> +       plat->irq = fdtdec_get_int(gd->fdt_blob, dev_of_offset(dev),
>>> +                                  "interrupts", 0);
>>
>>
>> Should we handle NS16550 on the PCI bus? If not, better put a comment
>> here.
>
>
> Actually I'm also using this RX IRQ buffer for the HS-UART on BayTrail
> connected via PCI (pciuart0: uart at 1e,3). I've added the interrupt
> property to the DT for this to work for now. I'll check, if I can read
> the interrupt from the PCI config space instead.
>

Yes, the interrupt line register is programmed with the value of its
interrupt vector in irq_router_probe(). But you should wait for the
IRQ device to be probed before NS16550.

>>> +       if (!plat->irq) {
>>
>>
>> If this is not provided, I think we should print a warning and fall
>> back to use the original polling mode.
>
>
> Good idea, will change in v3.
>
>
>>> +               debug("ns16550 interrupt not provided\n");
>>> +               return -EINVAL;
>>> +       }
>>> +#endif
>>> +
>>>          return 0;
>>>   }
>>>   #endif
>>> @@ -506,6 +615,7 @@ U_BOOT_DRIVER(ns16550_serial) = {
>>>   #endif
>>>          .priv_auto_alloc_size = sizeof(struct NS16550),
>>>          .probe = ns16550_serial_probe,
>>> +       .remove = ns16550_serial_remove,
>>>          .ops    = &ns16550_serial_ops,
>>>          .flags  = DM_FLAG_PRE_RELOC,
>>>   };
>>> diff --git a/include/ns16550.h b/include/ns16550.h
>>> index 5fcbcd2e74..7e9944d0d9 100644
>>> --- a/include/ns16550.h
>>> +++ b/include/ns16550.h
>>> @@ -51,6 +51,10 @@
>>>    * @base:              Base register address
>>>    * @reg_shift:         Shift size of registers (0=byte, 1=16bit,
>>> 2=32bit...)
>>>    * @clock:             UART base clock speed in Hz
>>> + *
>>> + * @buf:               Pointer to the RX interrupt buffer
>>> + * @rd_ptr:            Read pointer in the RX interrupt buffer
>>> + * @wr_ptr:            Write pointer in the RX interrupt buffer
>>>    */
>>>   struct ns16550_platdata {
>>>          unsigned long base;
>>> @@ -58,6 +62,12 @@ struct ns16550_platdata {
>>>          int clock;
>>>          int reg_offset;
>>>          u32 fcr;
>>> +
>>> +       int irq;
>>> +
>>> +       char *buf;
>>> +       int rd_ptr;
>>> +       int wr_ptr;
>>>   };
>>>
>>>   struct udevice;
>>> --
>>
>>
>> I've tested the patch on MinnowMax, and pasted a really long text.
>> Below are 5 times results: (using SecureCRT as my terminal application
>> on Windows)
>>
>> => Note: when interrupt transfer complets, the xHC generates an event
>> TRB with TRB type 'Transfer Event', which is exactly the same as a
>> control or bulk transfer. In our xHCI driver, xhci_wait_foer_event()
>> checks the event TRB t ype and depending on the timing,g it may
>> wrongly return an event TRB to the caller which originates from
>> another USB device (different slot ID is checked by the driver and if
>> different a "BUG" will be thrown).
>> Unknown command 'Note:' - try 'help'
>> => Note: when interrupt transfer complets, thee xHC generates an event
>> TRB withn TRB type 'Transfer Event', which is exactly the same as a
>> control or bulk transfer. In our xHCI driver, xhci_wait_for_event()
>> checks the event TRB type and depending on the timing,g it may wrongly
>> return an event rTRB to the caller which originates from another USB
>> device (different slot ID is checked by the driver and if different a
>> "BUG" will be thrown).
>> Unknown command 'Note:' - try 'help'
>> => Note: when interrupt transfer complets, thee xHC generates an event
>> TRB withn TRB type 'Transfer Event', which is exactly the same as a
>> control or bulk transfer. In our xHCI driver, xhci_wait_for_event()
>> checks the event TRB type and depending on the timing, it may wrongly
>> return an event rTRB to the caller which originates from another USB
>> device (different slot ID is checked by the driver and if different a
>> "BUG" will be thrown).
>> Unknown command 'Note:' - try 'help'
>> => Note: when interrupt transfer complets, the xHC generates an event
>> TRB with TRB type 'Transfer Event', whicfh is exactly the same as a
>> control or bulk transfer. In our xHCI driver, xhci_wait_for_event()
>> checks the event TRB type and depending on the timing, it may wrongly
>> return an event TRB to the caller which originatres from another USB
>> device (different slot ID is checked by the driver and if different a
>> "BUG" will be thrown).
>> Unknown command 'Note:' - try 'help'
>> => Note: when interrupt transfer complets, thee xHC generates an event
>> TRB with TRB type 'Transfer Event', which is exactly the same as a
>> control or bulk transfer. In our xHCI driver, xhci_wait_for_event()
>> checks the event TRB type and depending on the timing, it may wrongly
>> return an event rTRB to the caller which originatres from another USB
>> device (different slot ID is checked by the driver and if different a
>> "BUG" will be thrown).
>> Unknown command 'Note:' - try 'help'
>>
>> As you see, none of 5 is completely correct. The 1st and 2nd have
>> issues in "t ype" and "timing,g". The 2nd/4th/5th have issues in "thee
>> xHC". The 2nd/3rd/5th have issues in "event rTRB". The 4th has
>> "whicfh".
>
>
> Interesting, I've not seen such issues. Probably since I only checked
> the length of the received / echoed lines and roughly scanned the text
> for some incorrectness. Is the length always correct in your tests?
> Does increasing the buffer size help here?
>

I increased the buffer size to 1024, but it did not help. I tested it
with the MinnowMax internal UART (0x3f8). As you can see above, the
length is not always equal due to these incorrectness.

Regards,
Bin

[U-Boot] [PATCH v2] serial: ns16550: Add RX interrupt buffer support

[Stefan Roese]

Hi Bin,

On 17.07.2017 11:26, Bin Meng wrote:

<snip>

>>>> +       }
>>>> +#endif
>>>> +
>>>>           return 0;
>>>>    }
>>>>
>>>> @@ -459,6 +559,15 @@ int ns16550_serial_ofdata_to_platdata(struct udevice
>>>> *dev)
>>>>           if (port_type == PORT_JZ4780)
>>>>                   plat->fcr |= UART_FCR_UME;
>>>>
>>>> +#if CONFIG_IS_ENABLED(SERIAL_IRQ_BUFFER)
>>>> +       plat->irq = fdtdec_get_int(gd->fdt_blob, dev_of_offset(dev),
>>>> +                                  "interrupts", 0);
>>>
>>>
>>> Should we handle NS16550 on the PCI bus? If not, better put a comment
>>> here.
>>
>>
>> Actually I'm also using this RX IRQ buffer for the HS-UART on BayTrail
>> connected via PCI (pciuart0: uart at 1e,3). I've added the interrupt
>> property to the DT for this to work for now. I'll check, if I can read
>> the interrupt from the PCI config space instead.
>>
> 
> Yes, the interrupt line register is programmed with the value of its
> interrupt vector in irq_router_probe(). But you should wait for the
> IRQ device to be probed before NS16550.

I'm currently working on this and this dependency with the PCI interrupt
assignment is problematic. The serial driver is (intentionally) probed
earlier and the PCI interrupts are still unassigned at that stage.

I currently have no real good idea on how to solve this. We could
add a new "probe_late() / probe_irqs_enabled()" DM function for such
cases, but this seems overly complex and adds new bloat to the DM
infrastructure.

Do you have some other (simpler) ideas on how to solve this?

<snip>

>>> I've tested the patch on MinnowMax, and pasted a really long text.
>>> Below are 5 times results: (using SecureCRT as my terminal application
>>> on Windows)
>>>
>>> => Note: when interrupt transfer complets, the xHC generates an event
>>> TRB with TRB type 'Transfer Event', which is exactly the same as a
>>> control or bulk transfer. In our xHCI driver, xhci_wait_foer_event()
>>> checks the event TRB t ype and depending on the timing,g it may
>>> wrongly return an event TRB to the caller which originates from
>>> another USB device (different slot ID is checked by the driver and if
>>> different a "BUG" will be thrown).
>>> Unknown command 'Note:' - try 'help'
>>> => Note: when interrupt transfer complets, thee xHC generates an event
>>> TRB withn TRB type 'Transfer Event', which is exactly the same as a
>>> control or bulk transfer. In our xHCI driver, xhci_wait_for_event()
>>> checks the event TRB type and depending on the timing,g it may wrongly
>>> return an event rTRB to the caller which originates from another USB
>>> device (different slot ID is checked by the driver and if different a
>>> "BUG" will be thrown).
>>> Unknown command 'Note:' - try 'help'
>>> => Note: when interrupt transfer complets, thee xHC generates an event
>>> TRB withn TRB type 'Transfer Event', which is exactly the same as a
>>> control or bulk transfer. In our xHCI driver, xhci_wait_for_event()
>>> checks the event TRB type and depending on the timing, it may wrongly
>>> return an event rTRB to the caller which originates from another USB
>>> device (different slot ID is checked by the driver and if different a
>>> "BUG" will be thrown).
>>> Unknown command 'Note:' - try 'help'
>>> => Note: when interrupt transfer complets, the xHC generates an event
>>> TRB with TRB type 'Transfer Event', whicfh is exactly the same as a
>>> control or bulk transfer. In our xHCI driver, xhci_wait_for_event()
>>> checks the event TRB type and depending on the timing, it may wrongly
>>> return an event TRB to the caller which originatres from another USB
>>> device (different slot ID is checked by the driver and if different a
>>> "BUG" will be thrown).
>>> Unknown command 'Note:' - try 'help'
>>> => Note: when interrupt transfer complets, thee xHC generates an event
>>> TRB with TRB type 'Transfer Event', which is exactly the same as a
>>> control or bulk transfer. In our xHCI driver, xhci_wait_for_event()
>>> checks the event TRB type and depending on the timing, it may wrongly
>>> return an event rTRB to the caller which originatres from another USB
>>> device (different slot ID is checked by the driver and if different a
>>> "BUG" will be thrown).
>>> Unknown command 'Note:' - try 'help'
>>>
>>> As you see, none of 5 is completely correct. The 1st and 2nd have
>>> issues in "t ype" and "timing,g". The 2nd/4th/5th have issues in "thee
>>> xHC". The 2nd/3rd/5th have issues in "event rTRB". The 4th has
>>> "whicfh".
>>
>>
>> Interesting, I've not seen such issues. Probably since I only checked
>> the length of the received / echoed lines and roughly scanned the text
>> for some incorrectness. Is the length always correct in your tests?
>> Does increasing the buffer size help here?
>>
> 
> I increased the buffer size to 1024, but it did not help. I tested it
> with the MinnowMax internal UART (0x3f8). As you can see above, the
> length is not always equal due to these incorrectness.

Thanks for testing and confirming. Currently I have no idea, where
this problem might come from. And my time is limited right now,
as I'll be on vacation for 10 days starting on Wednesday. I still think
that this RX interrupt buffer support is a big improvement over the
"polling only" mode, even in this current version. I can re-visit this
driver in a few weeks to work on a fix for the issue that you have
found.

Thanks,
Stefan

[U-Boot] [PATCH v2] serial: ns16550: Add RX interrupt buffer support

[Stefan Roese]

Hi Bin,

On 17.07.2017 11:43, Stefan Roese wrote:
> On 17.07.2017 11:26, Bin Meng wrote:
> 
> <snip>
> 
>>>>> +       }
>>>>> +#endif
>>>>> +
>>>>>           return 0;
>>>>>    }
>>>>>
>>>>> @@ -459,6 +559,15 @@ int ns16550_serial_ofdata_to_platdata(struct 
>>>>> udevice
>>>>> *dev)
>>>>>           if (port_type == PORT_JZ4780)
>>>>>                   plat->fcr |= UART_FCR_UME;
>>>>>
>>>>> +#if CONFIG_IS_ENABLED(SERIAL_IRQ_BUFFER)
>>>>> +       plat->irq = fdtdec_get_int(gd->fdt_blob, dev_of_offset(dev),
>>>>> +                                  "interrupts", 0);
>>>>
>>>>
>>>> Should we handle NS16550 on the PCI bus? If not, better put a comment
>>>> here.
>>>
>>>
>>> Actually I'm also using this RX IRQ buffer for the HS-UART on BayTrail
>>> connected via PCI (pciuart0: uart at 1e,3). I've added the interrupt
>>> property to the DT for this to work for now. I'll check, if I can read
>>> the interrupt from the PCI config space instead.
>>>
>>
>> Yes, the interrupt line register is programmed with the value of its
>> interrupt vector in irq_router_probe(). But you should wait for the
>> IRQ device to be probed before NS16550.
> 
> I'm currently working on this and this dependency with the PCI interrupt
> assignment is problematic. The serial driver is (intentionally) probed
> earlier and the PCI interrupts are still unassigned at that stage.
> 
> I currently have no real good idea on how to solve this. We could
> add a new "probe_late() / probe_irqs_enabled()" DM function for such
> cases, but this seems overly complex and adds new bloat to the DM
> infrastructure.
> 
> Do you have some other (simpler) ideas on how to solve this?

Another idea would be, to write the PCI interrupt vectors earlier
in the boot stage, before the serial driver is probed. The interrupts
don't need to be enabled for this.

What do you think of this idea?

Thanks,
Stefan

[U-Boot] [PATCH v2] serial: ns16550: Add RX interrupt buffer support

[Bin Meng]

+Simon,

Hi Stefan,

On Mon, Jul 17, 2017 at 7:18 PM, Stefan Roese <sr@denx.de> wrote:
> Hi Bin,
>
>
> On 17.07.2017 11:43, Stefan Roese wrote:
>>
>> On 17.07.2017 11:26, Bin Meng wrote:
>>
>> <snip>
>>
>>>>>> +       }
>>>>>> +#endif
>>>>>> +
>>>>>>           return 0;
>>>>>>    }
>>>>>>
>>>>>> @@ -459,6 +559,15 @@ int ns16550_serial_ofdata_to_platdata(struct
>>>>>> udevice
>>>>>> *dev)
>>>>>>           if (port_type == PORT_JZ4780)
>>>>>>                   plat->fcr |= UART_FCR_UME;
>>>>>>
>>>>>> +#if CONFIG_IS_ENABLED(SERIAL_IRQ_BUFFER)
>>>>>> +       plat->irq = fdtdec_get_int(gd->fdt_blob, dev_of_offset(dev),
>>>>>> +                                  "interrupts", 0);
>>>>>
>>>>>
>>>>>
>>>>> Should we handle NS16550 on the PCI bus? If not, better put a comment
>>>>> here.
>>>>
>>>>
>>>>
>>>> Actually I'm also using this RX IRQ buffer for the HS-UART on BayTrail
>>>> connected via PCI (pciuart0: uart at 1e,3). I've added the interrupt
>>>> property to the DT for this to work for now. I'll check, if I can read
>>>> the interrupt from the PCI config space instead.
>>>>
>>>
>>> Yes, the interrupt line register is programmed with the value of its
>>> interrupt vector in irq_router_probe(). But you should wait for the
>>> IRQ device to be probed before NS16550.
>>
>>
>> I'm currently working on this and this dependency with the PCI interrupt
>> assignment is problematic. The serial driver is (intentionally) probed
>> earlier and the PCI interrupts are still unassigned at that stage.
>>
>> I currently have no real good idea on how to solve this. We could
>> add a new "probe_late() / probe_irqs_enabled()" DM function for such
>> cases, but this seems overly complex and adds new bloat to the DM
>> infrastructure.
>>
>> Do you have some other (simpler) ideas on how to solve this?
>
>
> Another idea would be, to write the PCI interrupt vectors earlier
> in the boot stage, before the serial driver is probed. The interrupts
> don't need to be enabled for this.
>
> What do you think of this idea?
>

Yep, I believe we can move the following codes from
arch/x86/cpu/i386/interrupts.c

/* Try to set up the interrupt router, but don't require one */
ret = uclass_first_device_err(UCLASS_IRQ, &dev);
if (ret && ret != -ENODEV)
return ret;

to arch_cpu_init_dm() in board_f.c.

Simon, do you have other better solution for this?

> Thanks for testing and confirming. Currently I have no idea, where
> this problem might come from. And my time is limited right now,
> as I'll be on vacation for 10 days starting on Wednesday. I still think
> that this RX interrupt buffer support is a big improvement over the
> "polling only" mode, even in this current version. I can re-visit this
> driver in a few weeks to work on a fix for the issue that you have
> found.

Sure, let's re-visit this in the coming days.

Regards,
Bin

[U-Boot] [PATCH v2] serial: ns16550: Add RX interrupt buffer support

[Stefan Roese]

Hi Bin,

On 17.07.2017 16:49, Bin Meng wrote:
> +Simon,
> 
> Hi Stefan,
> 
> On Mon, Jul 17, 2017 at 7:18 PM, Stefan Roese <sr@denx.de> wrote:
>> Hi Bin,
>>
>>
>> On 17.07.2017 11:43, Stefan Roese wrote:
>>>
>>> On 17.07.2017 11:26, Bin Meng wrote:
>>>
>>> <snip>
>>>
>>>>>>> +       }
>>>>>>> +#endif
>>>>>>> +
>>>>>>>            return 0;
>>>>>>>     }
>>>>>>>
>>>>>>> @@ -459,6 +559,15 @@ int ns16550_serial_ofdata_to_platdata(struct
>>>>>>> udevice
>>>>>>> *dev)
>>>>>>>            if (port_type == PORT_JZ4780)
>>>>>>>                    plat->fcr |= UART_FCR_UME;
>>>>>>>
>>>>>>> +#if CONFIG_IS_ENABLED(SERIAL_IRQ_BUFFER)
>>>>>>> +       plat->irq = fdtdec_get_int(gd->fdt_blob, dev_of_offset(dev),
>>>>>>> +                                  "interrupts", 0);
>>>>>>
>>>>>>
>>>>>>
>>>>>> Should we handle NS16550 on the PCI bus? If not, better put a comment
>>>>>> here.
>>>>>
>>>>>
>>>>>
>>>>> Actually I'm also using this RX IRQ buffer for the HS-UART on BayTrail
>>>>> connected via PCI (pciuart0: uart at 1e,3). I've added the interrupt
>>>>> property to the DT for this to work for now. I'll check, if I can read
>>>>> the interrupt from the PCI config space instead.
>>>>>
>>>>
>>>> Yes, the interrupt line register is programmed with the value of its
>>>> interrupt vector in irq_router_probe(). But you should wait for the
>>>> IRQ device to be probed before NS16550.
>>>
>>>
>>> I'm currently working on this and this dependency with the PCI interrupt
>>> assignment is problematic. The serial driver is (intentionally) probed
>>> earlier and the PCI interrupts are still unassigned at that stage.
>>>
>>> I currently have no real good idea on how to solve this. We could
>>> add a new "probe_late() / probe_irqs_enabled()" DM function for such
>>> cases, but this seems overly complex and adds new bloat to the DM
>>> infrastructure.
>>>
>>> Do you have some other (simpler) ideas on how to solve this?
>>
>>
>> Another idea would be, to write the PCI interrupt vectors earlier
>> in the boot stage, before the serial driver is probed. The interrupts
>> don't need to be enabled for this.
>>
>> What do you think of this idea?
>>
> 
> Yep, I believe we can move the following codes from
> arch/x86/cpu/i386/interrupts.c
> 
> /* Try to set up the interrupt router, but don't require one */
> ret = uclass_first_device_err(UCLASS_IRQ, &dev);
> if (ret && ret != -ENODEV)
> return ret;
> 
> to arch_cpu_init_dm() in board_f.c.

This does not seem to work. This IRQ routing code is still only
called pretty late in the init process:

U-Boot 2017.09-rc1-00210-gab169101b0-dirty (Aug 10 2017 - 11:29:37 +0200)

CPU: x86_64, vendor Intel, device 30679h
DRAM:  4 GiB
MMC:   pci_mmc: 0, pci_mmc: 1, pci_mmc: 2
SF: Detected w25q64cv with page size 256 Bytes, erase size 4 KiB, total 8 MiB
Model: theadorable-x86-DFI-BT700
SF: Detected w25q64cv with page size 256 Bytes, erase size 4 KiB, total 8 MiB
Net:   No ethernet found.
irq_router_common_init (245)
Assigning IRQ 5 to PCI device 0.2.0 (INTA)
Assigning IRQ 5 to PCI device 0.11.0 (INTA)
Assigning IRQ 5 to PCI device 0.12.0 (INTA)
Assigning IRQ 5 to PCI device 0.13.0 (INTA)
Assigning IRQ 5 to PCI device 0.14.0 (INTA)
Assigning IRQ 5 to PCI device 0.15.0 (INTA)
Assigning IRQ 5 to PCI device 0.16.0 (INTA)
Assigning IRQ 5 to PCI device 0.17.0 (INTA)
Assigning IRQ 5 to PCI device 0.18.0 (INTA)
Assigning IRQ 7 to PCI device 0.18.1 (INTC)
Assigning IRQ 10 to PCI device 0.18.2 (INTD)
Assigning IRQ 6 to PCI device 0.18.3 (INTB)
Assigning IRQ 5 to PCI device 0.18.4 (INTA)
Assigning IRQ 7 to PCI device 0.18.5 (INTC)
Assigning IRQ 10 to PCI device 0.18.6 (INTD)
Assigning IRQ 6 to PCI device 0.18.7 (INTB)
Assigning IRQ 5 to PCI device 0.1c.0 (INTA)
Assigning IRQ 5 to PCI device 0.1e.0 (INTA)
Assigning IRQ 10 to PCI device 0.1e.1 (INTD)
Assigning IRQ 6 to PCI device 0.1e.2 (INTB)
Assigning IRQ 7 to PCI device 0.1e.3 (INTC)
Assigning IRQ 10 to PCI device 0.1e.4 (INTD)
Assigning IRQ 6 to PCI device 0.1e.5 (INTB)
Assigning IRQ 6 to PCI device 0.1f.3 (INTB)
scanning bus for devices...

I see that this PCI interrupt routing is taken from the dts file. An
easy solution would be, to just add an interrupt entry to the PCI
UART device tree node:

------------------------- arch/x86/dts/dfi-bt700.dtsi -------------------------
index b62e00ff1f..1ccdf5d24b 100644
@@ -115,6 +115,7 @@
 			reg-shift = <2>;
 			clock-frequency = <58982400>;
 			current-speed = <115200>;
+			interrupts = <7>;
 		};

No need to change any of the interrupt related code and its
init sequence this way.

What do you think? Would this be acceptable?

Thanks,
Stefan

[U-Boot] [U-Boot, v2] serial: ns16550: Add RX interrupt buffer support

[Tom Rini]

On Fri, Jul 14, 2017 at 05:25:54PM +0200, Stefan Roese wrote:

> Pasting longer lines into the U-Boot console prompt sometimes leads to
> characters missing. One problem here is the small 16-byte FIFO of the
> legacy NS16550 UART, e.g. on x86 platforms.
> 
> This patch now introduces a Kconfig option to enable RX interrupt
> buffer support for NS16550 style UARTs. With this option enabled, I was
> able paste really long lines into the U-Boot console, without any
> characters missing.
> 
> Signed-off-by: Stefan Roese <sr@denx.de>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> Cc: Bin Meng <bmeng.cn@gmail.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170724/b00a92be/attachment.sig>

[U-Boot] [U-Boot, v2] serial: ns16550: Add RX interrupt buffer support

[Bin Meng]

Hi Tom,

On Tue, Jul 25, 2017 at 8:44 AM, Tom Rini <trini@konsulko.com> wrote:
> On Fri, Jul 14, 2017 at 05:25:54PM +0200, Stefan Roese wrote:
>
>> Pasting longer lines into the U-Boot console prompt sometimes leads to
>> characters missing. One problem here is the small 16-byte FIFO of the
>> legacy NS16550 UART, e.g. on x86 platforms.
>>
>> This patch now introduces a Kconfig option to enable RX interrupt
>> buffer support for NS16550 style UARTs. With this option enabled, I was
>> able paste really long lines into the U-Boot console, without any
>> characters missing.
>>
>> Signed-off-by: Stefan Roese <sr@denx.de>
>> Reviewed-by: Simon Glass <sjg@chromium.org>
>> Cc: Bin Meng <bmeng.cn@gmail.com>
>
> Applied to u-boot/master, thanks!
>

I believe Stefan will send a v3 to address my comments.

Regards,
Bin

[U-Boot] [U-Boot, v2] serial: ns16550: Add RX interrupt buffer support

[Tom Rini]

On Tue, Jul 25, 2017 at 09:18:02AM +0800, Bin Meng wrote:
> Hi Tom,
> 
> On Tue, Jul 25, 2017 at 8:44 AM, Tom Rini <trini@konsulko.com> wrote:
> > On Fri, Jul 14, 2017 at 05:25:54PM +0200, Stefan Roese wrote:
> >
> >> Pasting longer lines into the U-Boot console prompt sometimes leads to
> >> characters missing. One problem here is the small 16-byte FIFO of the
> >> legacy NS16550 UART, e.g. on x86 platforms.
> >>
> >> This patch now introduces a Kconfig option to enable RX interrupt
> >> buffer support for NS16550 style UARTs. With this option enabled, I was
> >> able paste really long lines into the U-Boot console, without any
> >> characters missing.
> >>
> >> Signed-off-by: Stefan Roese <sr@denx.de>
> >> Reviewed-by: Simon Glass <sjg@chromium.org>
> >> Cc: Bin Meng <bmeng.cn@gmail.com>
> >
> > Applied to u-boot/master, thanks!
> 
> I believe Stefan will send a v3 to address my comments.

Oh poop, sorry!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170724/e20bd0dd/attachment.sig>