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

Stefan Roese
2017-07-17T08:18:48Z

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