[U-Boot] [PATCH] serial: ns16550: Enhancements to the RX interrupt buffer support
Stefan Roese
sr at denx.de
Wed Aug 16 09:55:08 UTC 2017
Hi Bin,
On 16.08.2017 11:45, Bin Meng wrote:
> 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?
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
More information about the U-Boot
mailing list