From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marek Vasut <marex@denx.de>
Date: Thu, 17 Dec 2015 18:52:06 +0100
Subject: [U-Boot] [PATCH v1 05/18] drivers: serial: add driver for
	Microchip PIC32 UART controller.
In-Reply-To: <5672F0FC.6080609@microchip.com>
References: <5672F0FC.6080609@microchip.com>
Message-ID: <201512171852.06290.marex@denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On Thursday, December 17, 2015 at 06:29:32 PM, Purna Chandra Mandal wrote:

Hi!

Minor nits below.

btw do we expect MIPS to become maintained in U-Boot? That's nice :)

[...]

> diff --git a/drivers/serial/Makefile b/drivers/serial/Makefile
> index dd87147..57cd38b 100644
> --- a/drivers/serial/Makefile
> +++ b/drivers/serial/Makefile
> @@ -41,6 +41,7 @@ obj-$(CONFIG_MXS_AUART) += mxs_auart.o
>  obj-$(CONFIG_ARC_SERIAL) += serial_arc.o
>  obj-$(CONFIG_UNIPHIER_SERIAL) += serial_uniphier.o
>  obj-$(CONFIG_STM32_SERIAL) += serial_stm32.o
> +obj-$(CONFIG_PIC32_SERIAL) += serial_pic32.o
> 
>  ifndef CONFIG_SPL_BUILD
>  obj-$(CONFIG_USB_TTY) += usbtty.o
> diff --git a/drivers/serial/serial_pic32.c b/drivers/serial/serial_pic32.c
> new file mode 100644
> index 0000000..01c62e7
> --- /dev/null
> +++ b/drivers/serial/serial_pic32.c
> @@ -0,0 +1,220 @@
> +/*
> + * (c) 2015 Paul Thacker <paul.thacker@microchip.com>
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + *
> + */
> +#include <common.h>
> +#include <dm.h>
> +#include <clk.h>
> +#include <errno.h>
> +#include <config.h>
> +#include <serial.h>
> +#include <linux/bitops.h>
> +#include <common.h>
> +#include <asm/io.h>
> +#include <asm/arch-pic32/pic32.h>
> +#include <asm/arch-pic32/clock.h>
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +#define UART_ENABLE        BIT(15)
> +#define UART_ENABLE_RX        BIT(12)
> +#define UART_ENABLE_TX        BIT(10)
> +#define UART_RX_DATA_AVAIL    BIT(0)
> +#define UART_RX_OERR        BIT(1)
> +#define UART_TX_FULL        BIT(9)
> +
> +/* UART Control */
> +#define U_BASE(x)    (x)
> +#define U_MODE(x)       U_BASE(x)
> +#define U_MODECLR(x)    (U_MODE(x) + _CLR_OFFSET)
> +#define U_MODESET(x)    (U_MODE(x) + _SET_OFFSET)
> +#define U_STA(x)        (U_BASE(x) + 0x10)
> +#define U_STACLR(x)     (U_STA(x) + _CLR_OFFSET)
> +#define U_STASET(x)     (U_STA(x) + _SET_OFFSET)
> +#define U_TXREG(x)      (U_BASE(x) + 0x20)
> +#define U_RXREG(x)      (U_BASE(x) + 0x30)
> +#define U_BRG(x)        (U_BASE(x) + 0x40)

Why don't you just use uart_priv->regs + PIC32_REGISTER_OFFSET in the code?
The U_BASE is redundant and so is UMODE.

> +struct pic32_uart_priv {
> +    void __iomem *regs;
> +    ulong uartclk;
> +};
> +
> +static void pic32_serial_setbrg(void __iomem *regs, ulong uart_clk, u32
> baud) +{
> +    writel(0, U_BRG(regs));
> +    writel((uart_clk / baud / 16) - 1, U_BRG(regs));
> +    udelay(100);
> +}
> +
> +/*
> + * Initialize the serial port with the given baudrate.
> + * The settings are always 8 data bits, no parity, 1 stop bit, no start
> bits. + */
> +static int pic32_serial_init(void __iomem *regs, ulong clk, u32 baudrate)
> +{
> +    /* disable and clear mode */
> +    writel(0, U_MODE(regs));
> +    writel(0, U_STA(regs));
> +
> +    /* set baud rate generator */
> +    pic32_serial_setbrg(regs, clk, baudrate);
> +
> +    /* enable the UART for TX and RX */
> +    writel(UART_ENABLE_TX | UART_ENABLE_RX, U_STASET(regs));
> +
> +    /* enable the UART */
> +    writel(UART_ENABLE, U_MODESET(regs));
> +    return 0;
> +}
> +
> +/* Output a single byte to the serial port */
> +static void pic32_serial_putc(void __iomem *regs, const char c)
> +{
> +    /* if \n, then add a \r */
> +    if (c == '\n')
> +        pic32_serial_putc(regs, '\r');
> +
> +    /* Wait for Tx FIFO not full */
> +    while (readl(U_STA(regs)) & UART_TX_FULL)
> +        ;
> +
> +    /* stuff the tx buffer with the character */
> +    writel(c, U_TXREG(regs));
> +}
> +
> +/* Test whether a character is in the RX buffer */
> +static int pic32_serial_tstc(void __iomem *regs)
> +{
> +    /* check if rcv buf overrun error has occurred */
> +    if (readl(U_STA(regs)) & UART_RX_OERR) {
> +        readl(U_RXREG(regs));
> +
> +        /* clear OERR to keep receiving */
> +        writel(UART_RX_OERR, U_STACLR(regs));
> +    }
> +
> +    if (readl(U_STA(regs)) & UART_RX_DATA_AVAIL)
> +        return 1;    /* yes, there is data in rcv buffer */
> +    else
> +        return 0;    /* no data in rcv buffer */

return readl() & UART_RX_DATA_AVAIL; is sufficient here.

> +}
> +
> +/*
> + * Read a single byte from the rx buffer.
> + * Blocking: waits until a character is received, then returns.
> + * Return the character read directly from the UART's receive register.
> + *
> + */
> +static int pic32_serial_getc(void __iomem *regs)
> +{
> +    /* wait here until data is available */
> +    while (!pic32_serial_tstc(regs))
> +        ;
> +
> +    /* read the character from the rcv buffer */
> +    return readl(U_RXREG(regs));

return readl() & 0xff, since the return value is a signed integer.

> +}

[...]

> +U_BOOT_DRIVER(pic32_serial) = {
> +    .name        = "pic32-uart",
> +    .id        = UCLASS_SERIAL,
> +    .of_match    = pic32_uart_ids,
> +    .probe        = pic32_uart_probe,
> +    .ops        = &pic32_uart_ops,
> +    .flags        = DM_FLAG_PRE_RELOC,
> +    .ofdata_to_platdata = pic32_uart_ofdata_to_platdata,
> +    .platdata_auto_alloc_size = sizeof(struct pic32_uart_priv),

Is there some problem with tab/space conversion going on in here?

> +};
> +

[...]