Re: [PATCH 13/20] i.MX: Add 'lpuart' serial driver

[Andrey Smirnov <andrew.smirnov@gmail.com>]

On Tue, Oct 4, 2016 at 12:13 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Mon, Oct 03, 2016 at 07:40:50AM -0700, Andrey Smirnov wrote:
>> Add 'lpuart' serial driver, based on analogous driver from U-Boot
>>
>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>> ---
>>  drivers/serial/Kconfig         |   4 +
>>  drivers/serial/Makefile        |   1 +
>>  drivers/serial/serial_lpuart.c | 217 +++++++++++++++++++++++++++++++++++++++++
>>  include/serial/lpuart.h        |  28 ++++--
>>  4 files changed, 244 insertions(+), 6 deletions(-)
>>  create mode 100644 drivers/serial/serial_lpuart.c
>>
>> diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
>> index 146bf1e..02e869a 100644
>> --- a/drivers/serial/Kconfig
>> +++ b/drivers/serial/Kconfig
>> @@ -137,4 +137,8 @@ config DRIVER_SERIAL_DIGIC
>>       bool "Canon DIGIC serial driver"
>>       depends on ARCH_DIGIC
>>
>> +config DRIVER_SERIAL_LPUART
>> +     depends on ARCH_IMX
>> +     bool "LPUART serial driver"
>> +
>>  endmenu
>> diff --git a/drivers/serial/Makefile b/drivers/serial/Makefile
>> index 189e777..7d1bae1 100644
>> --- a/drivers/serial/Makefile
>> +++ b/drivers/serial/Makefile
>> @@ -20,3 +20,4 @@ obj-$(CONFIG_DRIVER_SERIAL_AUART)           += serial_auart.o
>>  obj-$(CONFIG_DRIVER_SERIAL_CADENCE)          += serial_cadence.o
>>  obj-$(CONFIG_DRIVER_SERIAL_EFI_STDIO)                += efi-stdio.o
>>  obj-$(CONFIG_DRIVER_SERIAL_DIGIC)            += serial_digic.o
>> +obj-$(CONFIG_DRIVER_SERIAL_LPUART)           += serial_lpuart.o
>> diff --git a/drivers/serial/serial_lpuart.c b/drivers/serial/serial_lpuart.c
>> new file mode 100644
>> index 0000000..52fb6d3
>> --- /dev/null
>> +++ b/drivers/serial/serial_lpuart.c
>> @@ -0,0 +1,217 @@
>> +/*
>> + * Copyright (c) 2016 Zodiac Inflight Innovation
>> + * Author: Andrey Smirnov <andrew.smirnov@gmail.com>
>> + *
>> + * Based on analogous driver from U-Boot
>> + *
>> + * See file CREDITS for list of people who contributed to this
>> + * project.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2
>> + * as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + */
>> +
>> +#include <common.h>
>> +#include <driver.h>
>> +#include <init.h>
>> +#include <malloc.h>
>> +#include <notifier.h>
>> +#include <io.h>
>> +#include <of.h>
>> +#include <linux/err.h>
>> +#include <linux/clk.h>
>> +#include <serial/lpuart.h>
>> +
>> +struct lpuart {
>> +     struct console_device cdev;
>> +     int baudrate;
>> +     int dte_mode;
>> +     struct notifier_block notify;
>> +     struct resource *io;
>> +     void __iomem *base;
>> +     struct clk *clk;
>> +};
>> +
>> +static struct lpuart *cdev_to_lpuart(struct console_device *cdev)
>> +{
>> +     return container_of(cdev, struct lpuart, cdev);
>> +}
>> +
>> +static struct lpuart *nb_to_lpuart(struct notifier_block *nb)
>> +{
>> +     return container_of(nb, struct lpuart, notify);
>> +}
>> +
>> +static void lpuart_enable(struct lpuart *lpuart, bool on)
>> +{
>> +     u8 ctrl;
>> +
>> +     ctrl = readb(lpuart->base + UARTCR2);
>> +     if (on)
>> +             ctrl |= UARTCR2_TE | UARTCR2_RE;
>> +     else
>> +             ctrl &= ~(UARTCR2_TE | UARTCR2_RE);
>> +     writeb(ctrl, lpuart->base + UARTCR2);
>> +}
>> +
>> +static int lpuart_serial_setbaudrate(struct console_device *cdev,
>> +                                  int baudrate)
>> +{
>> +     struct lpuart *lpuart = cdev_to_lpuart(cdev);
>> +
>> +     lpuart_enable(lpuart, false);
>> +
>> +     lpuart_setbrg(lpuart->base,
>> +                   clk_get_rate(lpuart->clk),
>> +                   baudrate);
>> +
>> +     lpuart_enable(lpuart, true);
>> +
>> +     lpuart->baudrate = baudrate;
>> +
>> +     return 0;
>> +}
>> +
>> +static int lpuart_serial_getc(struct console_device *cdev)
>> +{
>> +     bool ready;
>> +     struct lpuart *lpuart = cdev_to_lpuart(cdev);
>> +
>> +     do {
>> +             const u8 sr1 = readb(lpuart->base + UARTSR1);
>> +             ready = !!(sr1 & (UARTSR1_OR | UARTSR1_RDRF));
>> +     } while (!ready);
>> +
>> +     return readb(lpuart->base + UARTDR);
>> +}
>> +
>> +static void lpuart_serial_putc(struct console_device *cdev, char c)
>> +{
>> +     lpuart_putc(cdev_to_lpuart(cdev)->base, c);
>> +}
>> +
>> +/* Test whether a character is in the RX buffer */
>> +static int lpuart_serial_tstc(struct console_device *cdev)
>> +{
>> +     return !!readb(cdev_to_lpuart(cdev)->base + UARTRCFIFO);
>> +}
>> +
>> +static void lpuart_serial_flush(struct console_device *cdev)
>> +{
>> +     bool tx_empty;
>> +     struct lpuart *lpuart = cdev_to_lpuart(cdev);
>> +
>> +     do {
>> +             const u8 sr1 = readb(lpuart->base + UARTSR1);
>> +             tx_empty = !!(sr1 & UARTSR1_TDRE);
>> +     } while (!tx_empty);
>> +}
>> +
>> +static int lpuart_clocksource_clock_change(struct notifier_block *nb,
>> +                                        unsigned long event, void *data)
>> +{
>> +     struct lpuart *lpuart = nb_to_lpuart(nb);
>> +
>> +     return lpuart_serial_setbaudrate(&lpuart->cdev, lpuart->baudrate);
>> +}
>
> This doesn't make sense in this form. I introduced this code in the i.MX
> uart driver since I had the need to change PLL rates while the uart is
> active. When this happens I had to adjust the dividers for the new uart
> base clock. The code above doesn't react to base clock changes though,
> it takes the old rate stored in lpuart->baudrate.
>
> If you don't have to adjust PLL rates while the uart is active then I
> suggest that you just remove this code.

I am not sure I understand what you mean. I modeled this part of the
code after i.MX driver (serial_imx.c) and unless I missed something
(which I am not seeing) it should work exactly the same way.

That is: parent clock changes, this notifier gets called, it sets
configured baud rate again via lpuart_serial_setbaudrate, which in
turn sets dividers based off of value it gets from
clk_get_rate(lpuart->clk).

It does use old value in lpuart->baudrate, just as i.MX driver does,
since AFAIU the purpose of this callback is to make sure that UART
operates at the originally configured baudrate despite the clock rate
change.

I feel like I am missing something, although it is 7AM where I am now,
so I wouldn't be surprised if I am :-)

>
>> @@ -225,25 +225,35 @@ static inline void lpuart_setbrg(void __iomem *base,
>>                                unsigned int refclock,
>>                                unsigned int baudrate)
>>  {
>> +     unsigned int bfra;
>>       u16 sbr;
>> +
>>       sbr = (u16) (refclock / (16 * baudrate));
>>
>>       writeb(sbr >> 8,   base + UARTBDH);
>>       writeb(sbr & 0xff, base + UARTBDL);
>> +
>> +     bfra  = DIV_ROUND_UP(2 * refclock, baudrate) - 32 * sbr;
>> +     bfra &= UARTCR4_BRFA_MASK;
>> +     writeb(bfra, base + UARTCR4);
>>  }
>>
>> -static inline void lpuart_setup(void __iomem *base,
>> -                             unsigned int refclock)
>> +static inline void lpuart_setup_with_fifo(void __iomem *base,
>> +                                       unsigned int refclock,
>> +                                       unsigned int twfifo)
>>  {
>> -
>>       /* Disable UART */
>>       writeb(0, base + UARTCR2);
>>       writeb(0, base + UARTMODEM);
>>       writeb(0, base + UARTCR1);
>>
>> -     /* Disable FIFOs */
>> -     writeb(0, base + UARTPFIFO);
>> -     writeb(0, base + UARTTWFIFO);
>> +     if (twfifo) {
>> +             writeb(UARTPFIFO_TXFE | UARTPFIFO_RXFE, base + UARTPFIFO);
>> +             writeb((u8)twfifo, base + UARTTWFIFO);
>> +     } else {
>> +             writeb(0, base + UARTPFIFO);
>> +             writeb(0, base + UARTTWFIFO);
>> +     }
>>       writeb(1, base + UARTRWFIFO);
>>       writeb(UARTCFIFO_RXFLUSH | UARTCFIFO_TXFLUSH, base + UARTCFIFO);
>>
>> @@ -252,6 +262,12 @@ static inline void lpuart_setup(void __iomem *base,
>>       writeb(UARTCR2_TE | UARTCR2_RE, base + UARTCR2);
>>  }
>>
>> +static inline void lpuart_setup(void __iomem *base,
>> +                             unsigned int refclock)
>> +{
>> +     lpuart_setup_with_fifo(base, refclock, 0x00);
>> +}
>> +
>>  static inline void lpuart_putc(void __iomem *base, int c)
>>  {
>>       if (!(readb(base + UARTCR2) & UARTCR2_TE))
>
> This was introduced earlier with this series. No need to change it, just
> create it correctly in the first place.

OK, will fix in v2.

Thanks,
Andrey

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox