| Message ID | 20170713113338.13909-1-sr@denx.de |
|---|---|
| State | Superseded |
| Headers | show |
On 13 July 2017 at 05:33, 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. > > Signed-off-by: Stefan Roese <sr@denx.de> > Cc: Simon Glass <sjg@chromium.org> > Cc: Bin Meng <bmeng.cn@gmail.com> > --- > drivers/serial/Kconfig | 10 +++++ > drivers/serial/ns16550.c | 106 ++++++++++++++++++++++++++++++++++++++++++++--- > include/ns16550.h | 6 +++ > 3 files changed, 117 insertions(+), 5 deletions(-) Reviewed-by: Simon Glass <sjg@chromium.org> nits below > > 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..686c088e1d 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,21 @@ 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); Do we need to remove this in the remove() method? > + > + /* Enable RX interrupts */ > + serial_out(UART_IER_RDI, &com_port->ier); > + } > +#endif > + > return 0; > } > > @@ -459,6 +546,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 > diff --git a/include/ns16550.h b/include/ns16550.h > index 5fcbcd2e74..70d1ec8e6a 100644 > --- a/include/ns16550.h > +++ b/include/ns16550.h > @@ -58,6 +58,12 @@ struct ns16550_platdata { > int clock; > int reg_offset; > u32 fcr; > + > + int irq; > + > + char *buf; > + int rd_ptr; > + int wr_ptr; Please add comments to document these members. > }; > > struct udevice; > -- > 2.13.3 > Regards, Simon
Hi Simon, On 14.07.2017 15:50, Simon Glass wrote: > On 13 July 2017 at 05:33, 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. >> >> Signed-off-by: Stefan Roese <sr@denx.de> >> Cc: Simon Glass <sjg@chromium.org> >> Cc: Bin Meng <bmeng.cn@gmail.com> >> --- >> drivers/serial/Kconfig | 10 +++++ >> drivers/serial/ns16550.c | 106 ++++++++++++++++++++++++++++++++++++++++++++--- >> include/ns16550.h | 6 +++ >> 3 files changed, 117 insertions(+), 5 deletions(-) > > Reviewed-by: Simon Glass <sjg@chromium.org> > > nits below > >> >> 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..686c088e1d 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,21 @@ 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); > > Do we need to remove this in the remove() method? All interrupts are disabled before booting into the OS, so strictly it should not be necessary. But yes, I think its a good idea to remove the irq handler upon driver device. I'll change this in v2. >> + >> + /* Enable RX interrupts */ >> + serial_out(UART_IER_RDI, &com_port->ier); >> + } >> +#endif >> + >> return 0; >> } >> >> @@ -459,6 +546,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 >> diff --git a/include/ns16550.h b/include/ns16550.h >> index 5fcbcd2e74..70d1ec8e6a 100644 >> --- a/include/ns16550.h >> +++ b/include/ns16550.h >> @@ -58,6 +58,12 @@ struct ns16550_platdata { >> int clock; >> int reg_offset; >> u32 fcr; >> + >> + int irq; >> + >> + char *buf; >> + int rd_ptr; >> + int wr_ptr; > > Please add comments to document these members. Sure. Will add in v2. Thanks, Stefan
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..686c088e1d 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,21 @@ 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; } @@ -459,6 +546,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 diff --git a/include/ns16550.h b/include/ns16550.h index 5fcbcd2e74..70d1ec8e6a 100644 --- a/include/ns16550.h +++ b/include/ns16550.h @@ -58,6 +58,12 @@ struct ns16550_platdata { int clock; int reg_offset; u32 fcr; + + int irq; + + char *buf; + int rd_ptr; + int wr_ptr; }; struct udevice;
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> Cc: Simon Glass <sjg@chromium.org> Cc: Bin Meng <bmeng.cn@gmail.com> --- drivers/serial/Kconfig | 10 +++++ drivers/serial/ns16550.c | 106 ++++++++++++++++++++++++++++++++++++++++++++--- include/ns16550.h | 6 +++ 3 files changed, 117 insertions(+), 5 deletions(-)