| Message ID | 20170815093344.11751-1-sr@denx.de |
|---|---|
| State | Superseded |
| Delegated to: | Bin Meng |
| Headers | show |
Hi Stefan, On Tue, Aug 15, 2017 at 5:33 PM, Stefan Roese <sr@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@denx.de> > Cc: Simon Glass <sjg@chromium.org> > Cc: Bin Meng <bmeng.cn@gmail.com> > Cc: Tom Rini <trini@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
Hi Bin, On 16.08.2017 11:45, Bin Meng wrote: > On Tue, Aug 15, 2017 at 5:33 PM, Stefan Roese <sr@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@denx.de> >> Cc: Simon Glass <sjg@chromium.org> >> Cc: Bin Meng <bmeng.cn@gmail.com> >> Cc: Tom Rini <trini@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. Thanks, Stefan
Hi Stefan, On Wed, Aug 16, 2017 at 5:55 PM, Stefan Roese <sr@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 <sr@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@denx.de> >>> Cc: Simon Glass <sjg@chromium.org> >>> Cc: Bin Meng <bmeng.cn@gmail.com> >>> Cc: Tom Rini <trini@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
Hi Bin, On 16.08.2017 12:09, Bin Meng wrote: > Hi Stefan, > > On Wed, Aug 16, 2017 at 5:55 PM, Stefan Roese <sr@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 <sr@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@denx.de> >>>> Cc: Simon Glass <sjg@chromium.org> >>>> Cc: Bin Meng <bmeng.cn@gmail.com> >>>> Cc: Tom Rini <trini@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 :) Please take a look at the latest patchset for this and let me know, what you think of it. Thanks, Stefan
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 + 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"); #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;
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@denx.de> Cc: Simon Glass <sjg@chromium.org> Cc: Bin Meng <bmeng.cn@gmail.com> Cc: Tom Rini <trini@konsulko.com> --- drivers/serial/Kconfig | 8 +++++++- drivers/serial/ns16550.c | 41 +++++++++++++++++++++++++++++++---------- include/ns16550.h | 2 +- 3 files changed, 39 insertions(+), 12 deletions(-)