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

2015-12-22 Thread Marek Vasut 
On Tuesday, December 22, 2015 at 08:44:44 AM, Wills Wang wrote:
> Signed-off-by: Wills Wang 
> ---
> 
>  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 000..6ea500a
> --- /dev/null
> +++ b/drivers/serial/serial_ar933x.c
> @@ -0,0 +1,337 @@
> +/*
> + * (C) Copyright 2015
> + * Wills Wang, 
> + *
> + * SPDX-License-Identifier:  GPL-2.0+
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +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 = (4000 / (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(, );
> + 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.

> +

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

2015-12-22 Thread Thomas Chou 

Hi Wills,

On 2015年12月22日 15:44, Wills Wang wrote:

Signed-off-by: Wills Wang 
---

  drivers/serial/Makefile|   1 +
  drivers/serial/serial_ar933x.c | 337 +
  2 files changed, 338 insertions(+)
  create mode 100644 drivers/serial/serial_ar933x.c



Please convert the serial driver to driver model. Please check 
u-boot/doc/driver-model/serial-howto.txt.


Best regards,
Thomas
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2015-12-21 Thread Wills Wang 
Signed-off-by: Wills Wang 
---

 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 000..6ea500a
--- /dev/null
+++ b/drivers/serial/serial_ar933x.c
@@ -0,0 +1,337 @@
+/*
+ * (C) Copyright 2015
+ * Wills Wang, 
+ *
+ * SPDX-License-Identifier:GPL-2.0+
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+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)
+
+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;
+   break;
+   case 2400:
+   *uart_scale = 167;
+   *uart_step  = 1321;
+   break;
+   case 4800:
+   *uart_scale = 87;
+   *uart_step  = 1384;
+   break;
+   case 9600:
+   *uart_scale = 45;
+   *uart_step  = 1447;
+   break;
+   case 14400:
+   *uart_scale = 53;
+   *uart_step  = 2548;
+   break;
+   case 19200:
+   *uart_scale = 22;
+   *uart_step  = 1447;
+   break;
+   case 28800:
+   *uart_scale = 26;
+   *uart_step  = 2548;
+   break;
+   case 38400:
+   *uart_scale = 28;
+   *uart_step  = 3649;
+   break;
+   case 56000:
+   *uart_scale = 7;
+   *uart_step  = 1468;
+   break;
+   case 57600:
+   *uart_scale = 34;
+   *uart_step  = 6606;
+   break;
+   case 115200:
+   *uart_scale = 28;
+   *uart_step  = 10947;
+   break;
+   case 128000:
+   *uart_scale = 6;
+   *uart_step  = 2936;
+   break;
+   case 153600:
+   *uart_scale = 18;
+   *uart_step  = 9563;
+   break;
+   case 230400:
+   *uart_scale = 16;
+   *uart_step  = 12834;
+   break;
+   case 25:
+   *uart_scale = 4;
+   *uart_step  = 4096;
+   break;
+   case 256000:
+   *uart_scale = 6;
+   *uart_step  = 5872;
+   break;
+   case 460800:
+   *uart_scale = 7;
+   *uart_step  = 12079;
+   break;
+   case 576000:
+   *uart_scale = 4;
+   *uart_step  = 9437;
+   break;
+   case 921600:
+   *uart_scale = 3;
+   *uart_step  = 12079;
+   break;
+   case 100:
+   *uart_scale = 2;
+   *uart_step  = 9830;
+   break;
+   case 1152000:
+   *uart_scale = 2;
+   *uart_step  = 11324;
+   break;
+   case 150:
+   *uart_scale = 0;
+   *uart_step  = 4915;
+   break;
+   case 200:
+