[U-Boot] [PATCH v2 3/4] mips: ath79: add serial driver for ar933x SOC

[Marek Vasut <marex@denx.de>]

On Tuesday, December 22, 2015 at 08:44:44 AM, Wills Wang wrote:
> Signed-off-by: Wills Wang <wills.wang@live.com>
> ---
> 
>  drivers/serial/Makefile        |   1 +
>  drivers/serial/serial_ar933x.c | 337
> +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 338
> insertions(+)
>  create mode 100644 drivers/serial/serial_ar933x.c
> 
> diff --git a/drivers/serial/Makefile b/drivers/serial/Makefile
> index dd87147..9a7ad89 100644
> --- a/drivers/serial/Makefile
> +++ b/drivers/serial/Makefile
> @@ -17,6 +17,7 @@ endif
> 
>  obj-$(CONFIG_ALTERA_UART) += altera_uart.o
>  obj-$(CONFIG_ALTERA_JTAG_UART) += altera_jtag_uart.o
> +obj-$(CONFIG_AR933X_SERIAL) += serial_ar933x.o
>  obj-$(CONFIG_ARM_DCC) += arm_dcc.o
>  obj-$(CONFIG_ATMEL_USART) += atmel_usart.o
>  obj-$(CONFIG_EFI_APP) += serial_efi.o
> diff --git a/drivers/serial/serial_ar933x.c
> b/drivers/serial/serial_ar933x.c new file mode 100644
> index 0000000..6ea500a
> --- /dev/null
> +++ b/drivers/serial/serial_ar933x.c
> @@ -0,0 +1,337 @@
> +/*
> + * (C) Copyright 2015
> + * Wills Wang, <wills.wang@live.com>
> + *
> + * SPDX-License-Identifier:	GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <asm/io.h>
> +#include <asm/addrspace.h>
> +#include <asm/types.h>
> +#include <config.h>
> +#include <serial.h>
> +#include <asm/arch/ar71xx_regs.h>
> +#include <asm/arch/ar933x_uart.h>
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +#define REG_READ(b, o)      readl(KSEG1ADDR(b + o))
> +#define REG_WRITE(b, o, v)  writel(v, KSEG1ADDR((b + o)))
> +#define UART_READ(a)        REG_READ(AR933X_UART_BASE, a)
> +#define UART_WRITE(a, v)    REG_WRITE(AR933X_UART_BASE, a, v)

Please remove these macros (see below)

> +static void ar933x_serial_get_scale_step(u32 *uart_scale, u32 *uart_step)
> +{
> +	u32 val;
> +
> +	val = REG_READ(AR71XX_RESET_BASE, AR933X_RESET_REG_BOOTSTRAP);
> +	if (val & AR933X_BOOTSTRAP_REF_CLK_40) {
> +		switch (gd->baudrate) {
> +		case 600:
> +			*uart_scale = 255;
> +			*uart_step  = 503;
> +			break;
> +		case 1200:
> +			*uart_scale = 249;
> +			*uart_step  = 983;

I suppose this cannot be calculated, right ? What about making this into a
table, something like:

struct {
	u16 baudrate;
	u16 scale;
	u16 step;
} uart_something[] {
	{ 600, 255, 503 },
	{ 1200, .... },
	....
};

and then look things up:

for (i = 0; i < ARRAY_SIZE(uart_something); i++)
	if (gd->baudrate == uart_something[i].baudrate)
		break;

if (i == ARRAY_SIZE(uart_something))
	do default handling here
[...]

> +		default:
> +			*uart_scale = (40000000 / (16 * gd->baudrate)) - 1;
> +			*uart_step = 8192;
> +		}
> +	} else {
> +		switch (gd->baudrate) {
> +		case 600:
> +			*uart_scale = 255;
> +			*uart_step  = 805;
> +			break;

You can wrap this part into the table as well then, quite easily.

[...]

> +static void ar933x_serial_setbrg(void)
> +{
> +	/* TODO: better clock calculation, baudrate, etc. */
> +	u32 val, scale, step;
> +
> +	ar933x_serial_get_scale_step(&scale, &step);
> +	val  = ((scale & AR933X_UART_CLOCK_SCALE_M)
> +			<< AR933X_UART_CLOCK_SCALE_S);
> +	val |= ((step & AR933X_UART_CLOCK_STEP_M)
> +			<< AR933X_UART_CLOCK_STEP_S);

Drop the extraneous parenthesis around the statement please.

> +	UART_WRITE(AR933X_UART_CLOCK_REG, val);

This should be just writel().

> +}
> +
> +int ar933x_serial_init(void)
> +{
> +	u32 val;
> +
> +	/*
> +	 * Set GPIO10 (UART_SO) as output and enable UART,
> +	 * BIT(15) in GPIO_FUNCTION_1 register must be written with 1
> +	 */
> +	val = REG_READ(AR71XX_GPIO_BASE, AR71XX_GPIO_REG_OE);
> +	val |= BIT(10);
> +	REG_WRITE(AR71XX_GPIO_BASE, AR71XX_GPIO_REG_OE, val);

Use setbits_le32()

> +	val = REG_READ(AR71XX_GPIO_BASE, AR71XX_GPIO_REG_FUNC);
> +	val |= (AR933X_GPIO_FUNC_UART_EN | BIT(15));
> +	REG_WRITE(AR71XX_GPIO_BASE, AR71XX_GPIO_REG_FUNC, val);

Same here.

> +	/*
> +	 * UART controller configuration:
> +	 * - no DMA
> +	 * - no interrupt
> +	 * - DCE mode
> +	 * - no flow control
> +	 * - set RX ready oride
> +	 * - set TX ready oride
> +	 */
> +	val = AR933X_UART_CS_TX_READY_ORIDE | AR933X_UART_CS_RX_READY_ORIDE
> +		| (AR933X_UART_CS_IF_MODE_DCE << AR933X_UART_CS_IF_MODE_S);
> +	UART_WRITE(AR933X_UART_CS_REG, val);
> +	ar933x_serial_setbrg();
> +
> +	return 0;
> +}
> +
> +void ar933x_serial_putc(const char c)
> +{
> +	u32 data;
> +
> +	if (c == '\n')
> +		ar933x_serial_putc('\r');
> +
> +	do {
> +		data = UART_READ(AR933X_UART_DATA_REG);
> +	} while (!(data & AR933X_UART_DATA_TX_CSR));
> +
> +	data  = (u32)c | AR933X_UART_DATA_TX_CSR;

$c is already u32, no need for the cast.

> +	UART_WRITE(AR933X_UART_DATA_REG, data);
> +}
> +
> +static int ar933x_serial_getc(void)
> +{
> +	u32 data;
> +
> +	do {
> +		data = UART_READ(AR933X_UART_DATA_REG);
> +	} while (!(data & AR933X_UART_DATA_RX_CSR));
> +
> +	data = UART_READ(AR933X_UART_DATA_REG);
> +	UART_WRITE(AR933X_UART_DATA_REG, AR933X_UART_DATA_RX_CSR);
> +	return (int)(data & AR933X_UART_DATA_TX_RX_MASK);

Please drop all the casts, they are useless.

> +}
> +
> +static int ar933x_serial_tstc(void)
> +{
> +	u32 data;
> +
> +	data = UART_READ(AR933X_UART_DATA_REG);
> +	return (data & AR933X_UART_DATA_RX_CSR) ? 1 : 0;
> +}
> +
> +static struct serial_device ar933x_serial_drv = {
> +	.name	= "ar933x_serial",
> +	.start	= ar933x_serial_init,
> +	.stop	= NULL,
> +	.setbrg	= ar933x_serial_setbrg,
> +	.putc	= ar933x_serial_putc,
> +	.puts	= default_serial_puts,
> +	.getc	= ar933x_serial_getc,
> +	.tstc	= ar933x_serial_tstc,
> +};
> +
> +void ar933x_serial_initialize(void)
> +{
> +	serial_register(&ar933x_serial_drv);
> +}
> +
> +__weak struct serial_device *default_serial_console(void)
> +{
> +	return &ar933x_serial_drv;
> +}