[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