[U-Boot] [PATCH] serial: ns16550: Enhancements to the RX interrupt buffer support
Bin Meng
bmeng.cn at gmail.com
Wed Aug 16 10:09:16 UTC 2017
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 :)
Regards,
Bin
More information about the U-Boot
mailing list