[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