Hi Stefan, On Wed, Aug 16, 2017 at 5:55 PM, Stefan Roese <s...@denx.de> wrote: > Hi Bin, > > > On 16.08.2017 11:45, Bin Meng wrote: >> >> On Tue, Aug 15, 2017 at 5:33 PM, Stefan Roese <s...@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 <s...@denx.de> >>> Cc: Simon Glass <s...@chromium.org> >>> Cc: Bin Meng <bmeng...@gmail.com> >>> Cc: Tom Rini <tr...@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? > > > Hugh? I'll take a look later today. > > >>> + 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. > > > Yes, but still they should work just fine (or even better). > > >> 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 .. > > > Interesting idea - thanks for all the tests. The RX buffer has evolved > quite a bit, so your idea might very well be the much easier solution > for this problem. I need to test this on the DFI board though as well, > to see if this solves the char dropping here as well.
Yes, adding a buffer without using interrupt is much easier. And I believe this support can be moved to DM serial codes, so that every serial driver can benefit from it :) Regards, Bin _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot