Hi Stefan,

On Mon, Jul 17, 2017 at 4:18 PM, Stefan Roese <s...@denx.de> wrote:
> Hi Bin,
>
>
> On 17.07.2017 04:13, Bin Meng wrote:
>>
>> On Fri, Jul 14, 2017 at 11:25 PM, Stefan Roese <s...@denx.de> wrote:
>>>
>>> Pasting longer lines into the U-Boot console prompt sometimes leads to
>>> characters missing. One problem here is the small 16-byte FIFO of the
>>> legacy NS16550 UART, e.g. on x86 platforms.
>>>
>>> This patch now introduces a Kconfig option to enable RX interrupt
>>> buffer support for NS16550 style UARTs. With this option enabled, I was
>>> able paste really long lines into the U-Boot console, without any
>>> characters missing.
>>>
>>
>> Great to see this improvement! Some comments below:
>>
>>> Signed-off-by: Stefan Roese <s...@denx.de>
>>> Reviewed-by: Simon Glass <s...@chromium.org>
>>> Cc: Bin Meng <bmeng...@gmail.com>
>>> ---
>>> v2:
>>> - Added Simon's RB
>>> - Added irq_free_handler to newly introduced driver remove function
>>> - Added comments to the introduced variables on the ns16550_platdata
>>>    struct
>>>
>>>   drivers/serial/Kconfig   |  10 ++++
>>>   drivers/serial/ns16550.c | 120
>>> +++++++++++++++++++++++++++++++++++++++++++++--
>>>   include/ns16550.h        |  10 ++++
>>>   3 files changed, 135 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
>>> index b7dd2ac103..8284febae3 100644
>>> --- a/drivers/serial/Kconfig
>>> +++ b/drivers/serial/Kconfig
>>> @@ -64,6 +64,16 @@ config DM_SERIAL
>>>            implements serial_putc() etc. The uclass interface is
>>>            defined in include/serial.h.
>>>
>>> +config SERIAL_IRQ_BUFFER
>>> +       bool "Enable RX interrupt buffer for serial input"
>>> +       depends on DM_SERIAL
>>> +       default n
>>
>>
>> This line can be removed, as its default state is 'n'
>
>
> Okay, will update.
>
>>> +       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 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 e0e70244ce..0c43cb1fe2 100644
>>> --- a/drivers/serial/ns16550.c
>>> +++ b/drivers/serial/ns16550.c
>>> @@ -315,6 +315,80 @@ DEBUG_UART_FUNCS
>>>   #endif
>>>
>>>   #ifdef CONFIG_DM_SERIAL
>>> +
>>> +#if CONFIG_IS_ENABLED(SERIAL_IRQ_BUFFER)
>>> +
>>> +#define BUF_COUNT      256
>>
>>
>> Should this be configurable via Kconfig?
>
>
> Good idea. I'll add it in v3.
>
>
>>> +
>>> +static void rx_fifo_to_buf(struct udevice *dev)
>>> +{
>>> +       struct NS16550 *const com_port = dev_get_priv(dev);
>>> +       struct ns16550_platdata *plat = dev->platdata;
>>> +
>>> +       /* Read all available chars into buffer */
>>> +       while ((serial_in(&com_port->lsr) & UART_LSR_DR)) {
>>> +               plat->buf[plat->wr_ptr++] = serial_in(&com_port->rbr);
>>> +               plat->wr_ptr %= BUF_COUNT;
>>> +       }
>>> +}
>>> +
>>> +static int rx_pending(struct udevice *dev)
>>> +{
>>> +       struct ns16550_platdata *plat = dev->platdata;
>>> +
>>> +       /*
>>> +        * At startup it may happen, that some already received chars are
>>> +        * "stuck" in the RX FIFO, even with the interrupt enabled. This
>>> +        * RX FIFO flushing makes sure, that these chars are read out and
>>> +        * the RX interrupts works as expected.
>>> +        */
>>> +       rx_fifo_to_buf(dev);
>>> +
>>> +       return plat->rd_ptr != plat->wr_ptr ? 1 : 0;
>>> +}
>>> +
>>> +static int rx_get(struct udevice *dev)
>>> +{
>>> +       struct ns16550_platdata *plat = dev->platdata;
>>> +       char val;
>>> +
>>> +       val = plat->buf[plat->rd_ptr++];
>>> +       plat->rd_ptr %= BUF_COUNT;
>>> +
>>> +       return val;
>>> +}
>>> +
>>> +void ns16550_handle_irq(void *data)
>>> +{
>>> +       struct udevice *dev = (struct udevice *)data;
>>> +       struct NS16550 *const com_port = dev_get_priv(dev);
>>> +
>>> +       /* Check if interrupt is pending */
>>> +       if (serial_in(&com_port->iir) & UART_IIR_NO_INT)
>>> +               return;
>>> +
>>> +       /* Flush all available characters from the RX FIFO into the RX
>>> buffer */
>>> +       rx_fifo_to_buf(dev);
>>> +}
>>> +
>>> +#else /* CONFIG_SERIAL_IRQ_BUFFER */
>>> +
>>> +static int rx_pending(struct udevice *dev)
>>> +{
>>> +       struct NS16550 *const com_port = dev_get_priv(dev);
>>> +
>>> +       return serial_in(&com_port->lsr) & UART_LSR_DR ? 1 : 0;
>>> +}
>>> +
>>> +static int rx_get(struct udevice *dev)
>>> +{
>>> +       struct NS16550 *const com_port = dev_get_priv(dev);
>>> +
>>> +       return serial_in(&com_port->rbr);
>>> +}
>>> +
>>> +#endif /* CONFIG_SERIAL_IRQ_BUFFER */
>>> +
>>>   static int ns16550_serial_putc(struct udevice *dev, const char ch)
>>>   {
>>>          struct NS16550 *const com_port = dev_get_priv(dev);
>>> @@ -340,19 +414,17 @@ static int ns16550_serial_pending(struct udevice
>>> *dev, bool input)
>>>          struct NS16550 *const com_port = dev_get_priv(dev);
>>>
>>>          if (input)
>>> -               return serial_in(&com_port->lsr) & UART_LSR_DR ? 1 : 0;
>>> +               return rx_pending(dev);
>>>          else
>>>                  return serial_in(&com_port->lsr) & UART_LSR_THRE ? 0 :
>>> 1;
>>>   }
>>>
>>>   static int ns16550_serial_getc(struct udevice *dev)
>>>   {
>>> -       struct NS16550 *const com_port = dev_get_priv(dev);
>>> -
>>> -       if (!(serial_in(&com_port->lsr) & UART_LSR_DR))
>>> +       if (!ns16550_serial_pending(dev, true))
>>>                  return -EAGAIN;
>>>
>>> -       return serial_in(&com_port->rbr);
>>> +       return rx_get(dev);
>>>   }
>>>
>>>   static int ns16550_serial_setbrg(struct udevice *dev, int baudrate)
>>> @@ -375,6 +447,34 @@ int ns16550_serial_probe(struct udevice *dev)
>>>          com_port->plat = dev_get_platdata(dev);
>>>          NS16550_init(com_port, -1);
>>>
>>> +#if CONFIG_IS_ENABLED(SERIAL_IRQ_BUFFER)
>>> +       if (gd->flags & GD_FLG_RELOC) {
>>> +               struct ns16550_platdata *plat = dev->platdata;
>>> +
>>> +               /* Allocate the RX buffer */
>>> +               plat->buf = malloc(BUF_COUNT);
>>> +
>>> +               /* Install the interrupt handler */
>>> +               irq_install_handler(plat->irq, ns16550_handle_irq, dev);
>>> +
>>> +               /* Enable RX interrupts */
>>> +               serial_out(UART_IER_RDI, &com_port->ier);
>>> +       }
>>> +#endif
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +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;
>>> +
>>> +               irq_free_handler(plat->irq);
>>
>>
>> We should also turn off interrupts on NS16550 too by writing zero to
>> com_port->ie.
>
>
> Will add in v3.
>
>>> +       }
>>> +#endif
>>> +
>>>          return 0;
>>>   }
>>>
>>> @@ -459,6 +559,15 @@ int ns16550_serial_ofdata_to_platdata(struct udevice
>>> *dev)
>>>          if (port_type == PORT_JZ4780)
>>>                  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);
>>
>>
>> Should we handle NS16550 on the PCI bus? If not, better put a comment
>> here.
>
>
> Actually I'm also using this RX IRQ buffer for the HS-UART on BayTrail
> connected via PCI (pciuart0: uart@1e,3). I've added the interrupt
> property to the DT for this to work for now. I'll check, if I can read
> the interrupt from the PCI config space instead.
>

Yes, the interrupt line register is programmed with the value of its
interrupt vector in irq_router_probe(). But you should wait for the
IRQ device to be probed before NS16550.

>>> +       if (!plat->irq) {
>>
>>
>> If this is not provided, I think we should print a warning and fall
>> back to use the original polling mode.
>
>
> Good idea, will change in v3.
>
>
>>> +               debug("ns16550 interrupt not provided\n");
>>> +               return -EINVAL;
>>> +       }
>>> +#endif
>>> +
>>>          return 0;
>>>   }
>>>   #endif
>>> @@ -506,6 +615,7 @@ U_BOOT_DRIVER(ns16550_serial) = {
>>>   #endif
>>>          .priv_auto_alloc_size = sizeof(struct NS16550),
>>>          .probe = ns16550_serial_probe,
>>> +       .remove = ns16550_serial_remove,
>>>          .ops    = &ns16550_serial_ops,
>>>          .flags  = DM_FLAG_PRE_RELOC,
>>>   };
>>> diff --git a/include/ns16550.h b/include/ns16550.h
>>> index 5fcbcd2e74..7e9944d0d9 100644
>>> --- a/include/ns16550.h
>>> +++ b/include/ns16550.h
>>> @@ -51,6 +51,10 @@
>>>    * @base:              Base register address
>>>    * @reg_shift:         Shift size of registers (0=byte, 1=16bit,
>>> 2=32bit...)
>>>    * @clock:             UART base clock speed in Hz
>>> + *
>>> + * @buf:               Pointer to the RX interrupt buffer
>>> + * @rd_ptr:            Read pointer in the RX interrupt buffer
>>> + * @wr_ptr:            Write pointer in the RX interrupt buffer
>>>    */
>>>   struct ns16550_platdata {
>>>          unsigned long base;
>>> @@ -58,6 +62,12 @@ struct ns16550_platdata {
>>>          int clock;
>>>          int reg_offset;
>>>          u32 fcr;
>>> +
>>> +       int irq;
>>> +
>>> +       char *buf;
>>> +       int rd_ptr;
>>> +       int wr_ptr;
>>>   };
>>>
>>>   struct udevice;
>>> --
>>
>>
>> I've tested the patch on MinnowMax, and pasted a really long text.
>> Below are 5 times results: (using SecureCRT as my terminal application
>> on Windows)
>>
>> => Note: when interrupt transfer complets, the xHC generates an event
>> TRB with TRB type 'Transfer Event', which is exactly the same as a
>> control or bulk transfer. In our xHCI driver, xhci_wait_foer_event()
>> checks the event TRB t ype and depending on the timing,g it may
>> wrongly return an event TRB to the caller which originates from
>> another USB device (different slot ID is checked by the driver and if
>> different a "BUG" will be thrown).
>> Unknown command 'Note:' - try 'help'
>> => Note: when interrupt transfer complets, thee xHC generates an event
>> TRB withn TRB type 'Transfer Event', which is exactly the same as a
>> control or bulk transfer. In our xHCI driver, xhci_wait_for_event()
>> checks the event TRB type and depending on the timing,g it may wrongly
>> return an event rTRB to the caller which originates from another USB
>> device (different slot ID is checked by the driver and if different a
>> "BUG" will be thrown).
>> Unknown command 'Note:' - try 'help'
>> => Note: when interrupt transfer complets, thee xHC generates an event
>> TRB withn TRB type 'Transfer Event', which is exactly the same as a
>> control or bulk transfer. In our xHCI driver, xhci_wait_for_event()
>> checks the event TRB type and depending on the timing, it may wrongly
>> return an event rTRB to the caller which originates from another USB
>> device (different slot ID is checked by the driver and if different a
>> "BUG" will be thrown).
>> Unknown command 'Note:' - try 'help'
>> => Note: when interrupt transfer complets, the xHC generates an event
>> TRB with TRB type 'Transfer Event', whicfh is exactly the same as a
>> control or bulk transfer. In our xHCI driver, xhci_wait_for_event()
>> checks the event TRB type and depending on the timing, it may wrongly
>> return an event TRB to the caller which originatres from another USB
>> device (different slot ID is checked by the driver and if different a
>> "BUG" will be thrown).
>> Unknown command 'Note:' - try 'help'
>> => Note: when interrupt transfer complets, thee xHC generates an event
>> TRB with TRB type 'Transfer Event', which is exactly the same as a
>> control or bulk transfer. In our xHCI driver, xhci_wait_for_event()
>> checks the event TRB type and depending on the timing, it may wrongly
>> return an event rTRB to the caller which originatres from another USB
>> device (different slot ID is checked by the driver and if different a
>> "BUG" will be thrown).
>> Unknown command 'Note:' - try 'help'
>>
>> As you see, none of 5 is completely correct. The 1st and 2nd have
>> issues in "t ype" and "timing,g". The 2nd/4th/5th have issues in "thee
>> xHC". The 2nd/3rd/5th have issues in "event rTRB". The 4th has
>> "whicfh".
>
>
> Interesting, I've not seen such issues. Probably since I only checked
> the length of the received / echoed lines and roughly scanned the text
> for some incorrectness. Is the length always correct in your tests?
> Does increasing the buffer size help here?
>

I increased the buffer size to 1024, but it did not help. I tested it
with the MinnowMax internal UART (0x3f8). As you can see above, the
length is not always equal due to these incorrectness.

Regards,
Bin
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Reply via email to