[U-Boot] [PATCH] serial: ns16550: Enhancements to the RX interrupt buffer support

Stefan Roese sr at denx.de
Wed Aug 16 15:51:11 UTC 2017


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 at 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 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.
> 
> 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


More information about the U-Boot mailing list