[U-Boot] [PATCH] serial: ns16550: Enhancements to the RX interrupt buffer support
Bin Meng
bmeng.cn at gmail.com
Wed Aug 16 09:45:50 UTC 2017
Hi Stefan,
On Tue, Aug 15, 2017 at 5:33 PM, Stefan Roese <sr at denx.de> wrote:
> This patch changes the RX interrupt buffer support in these ways, mostly
> suggested by Bin Meng a few weeks ago:
>
> - The RX interrupt buffers size is now configurable via Kconfig
> (default still at 256 bytes)
> - For NS16550 devices on the PCI bus, the interrupt number will be
> read from the PCI config space. Please note that this might not
> result in the correct non-zero interrupt number for this PCI
> device, as the UART init code is called very early, currently on
> x86 before the PCI config registers are initialized via U-Boot
> - If the interrupt number is not provided, the code falls back to
> the normal polling mode
> - The RX interrupt generation is disabled in the UART in the remove
> function
>
> While reworking this RX interrupt buffer support, the "default n" is
> also removed from Kconfig as its not needed as pointed out by Bin Meng.
> Also, a missing comment for the 'irq' variable is added to the
> header.
>
> Signed-off-by: Stefan Roese <sr at denx.de>
> Cc: Simon Glass <sjg at chromium.org>
> Cc: Bin Meng <bmeng.cn at gmail.com>
> Cc: Tom Rini <trini at konsulko.com>
> ---
> drivers/serial/Kconfig | 8 +++++++-
> drivers/serial/ns16550.c | 41 +++++++++++++++++++++++++++++++----------
> include/ns16550.h | 2 +-
> 3 files changed, 39 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
> index a8e997834a..1b19b24f10 100644
> --- a/drivers/serial/Kconfig
> +++ b/drivers/serial/Kconfig
> @@ -67,13 +67,19 @@ config DM_SERIAL
> 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 SERIAL_IRQ_BUFFER_SIZE
I don't see this CONFIG_SERIAL_IRQ_BUFFER_SIZE is referenced anywhere
in the ns16550 codes?
> + int "RX interrupt buffer size"
> + depends on SERIAL_IRQ_BUFFER
> + default 256
> + help
> + The size of the RX interrupt buffer
> +
> 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 607a1b8c1d..a24ba75031 100644
> --- a/drivers/serial/ns16550.c
> +++ b/drivers/serial/ns16550.c
> @@ -453,11 +453,13 @@ int ns16550_serial_probe(struct udevice *dev)
> /* Allocate the RX buffer */
> plat->buf = malloc(BUF_COUNT);
>
> - /* Install the interrupt handler */
> - irq_install_handler(plat->irq, ns16550_handle_irq, dev);
> + if (plat->irq) {
> + /* Install the interrupt handler */
> + irq_install_handler(plat->irq, ns16550_handle_irq, dev);
>
> - /* Enable RX interrupts */
> - serial_out(UART_IER_RDI, &com_port->ier);
> + /* Enable RX interrupts */
> + serial_out(UART_IER_RDI, &com_port->ier);
> + }
> }
> #endif
>
> @@ -469,9 +471,13 @@ int ns16550_serial_probe(struct udevice *dev)
> 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;
> + struct ns16550_platdata *plat = dev->platdata;
> +
> + if ((gd->flags & GD_FLG_RELOC) && (plat->irq)) {
> + struct NS16550 *const com_port = dev_get_priv(dev);
>
> + /* Disable RX interrupts */
> + serial_out(CONFIG_SYS_NS16550_IER, &com_port->ier);
> irq_free_handler(plat->irq);
> }
> #endif
> @@ -504,6 +510,7 @@ int ns16550_serial_ofdata_to_platdata(struct udevice *dev)
> struct fdt_pci_addr pci_addr;
> u32 bar;
> int ret;
> + u8 irq;
>
> /* we prefer to use a memory-mapped register */
> ret = fdtdec_get_pci_addr(gd->fdt_blob, dev_of_offset(dev),
> @@ -524,6 +531,10 @@ int ns16550_serial_ofdata_to_platdata(struct udevice *dev)
> return ret;
>
> addr = bar;
> +
> + /* Try to get the PCI interrupt number */
> + dm_pci_read_config8(dev, PCI_INTERRUPT_LINE, &irq);
> + plat->irq = irq;
> }
> #endif
>
> @@ -562,12 +573,22 @@ int ns16550_serial_ofdata_to_platdata(struct udevice *dev)
> 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 the interrupt is not provided via PCI, read the number from
> + * the DT instead
> + */
> if (!plat->irq) {
> - debug("ns16550 interrupt not provided\n");
> - return -EINVAL;
> + plat->irq = fdtdec_get_int(gd->fdt_blob, dev_of_offset(dev),
> + "interrupts", 0);
> }
> +
> + /*
> + * If no interrupt is provided the RX interrupt buffer code is
> + * still used, but the interrupt is not enabled and registered.
> + * The RX buffer code will be usedin polling mode in this case.
> + */
> + if (!plat->irq)
> + debug("ns16550 interrupt not provided, using polling\n");
But rx_pending() and rx_get() is still the IRQ buffer version, not the
polling version.
Having said that, I've tested this patch, without setting "interrupts"
in the dts file, and testing on MinnowMax showing that previous out of
order character issue is disappeared with such configuration! Here is
the test result:
=> This patch changes the RX interrupt buffer support in these ways,
mostly suggested by Bin Meng a few weeks ago: - The RX interrupt
buffers size is now configurable via Kconfig (default still at 256
bytes) - For NS16550 devices on the PCI bus, the interrupt number will
be read from the PCI config space. Please note that this might not
result in the correct non-zero interrupt number for this PCI device,
as the UART init code is called very early, currently on x86 before
the PCI config registers are initialize
Unknown command 'This' - try 'help'
The following is the test result with "interrupts" set toi 4 in the
dts file for MinnowMax:
=> This patch changes the RX interrupt buffer nsupport in these ways,
mostly suggested by Bin Meng a few weeks ago: - The RX interrupt
buffers size is now configurable via Kconfig (default still a(t 256
bytes) - For NS16550 devicoes on the PCI bus, the interrupts number
will be read from the PCI config space. Please note that this might
not result in the correct non-zero interrupt number for this PCI
device, as the UART init code is called very early, currently on x86
before the PCI config registers are initi
Unknown command 'This' - try 'help'
=> This patch changes the RX interrupt buffer support in these ways,
mostly su ggested by Bin Meng a few weeks ago: - The RX interrupt
buffers size is now configurable via Kconfig (default still at 256
bytes) - For NS16550 devices on the PCI bus, the interrupt number will
be read from the PCrI config space. Please note that this might not
result in the correct non-zero interrupt number for this PCI device,
as the UART init code is called very early, currently on x86 befeore
the PCI config registers are initial
So maybe there is no need to add IRQ support at all ... Only adding a
buffer will resolve the original issue you wanted to fix ..
> #endif
>
> return 0;
> diff --git a/include/ns16550.h b/include/ns16550.h
> index 7e9944d0d9..15f9899d90 100644
> --- a/include/ns16550.h
> +++ b/include/ns16550.h
> @@ -52,6 +52,7 @@
> * @reg_shift: Shift size of registers (0=byte, 1=16bit, 2=32bit...)
> * @clock: UART base clock speed in Hz
> *
> + * @irq: Interrupt number for this UART
> * @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
> @@ -64,7 +65,6 @@ struct ns16550_platdata {
> u32 fcr;
>
> int irq;
> -
> char *buf;
> int rd_ptr;
> int wr_ptr;
> --
Regards,
Bin
More information about the U-Boot
mailing list