| Message ID | 1404469158-22703-3-git-send-email-yamada.m@jp.panasonic.com |
|---|---|
| State | Superseded |
| Delegated to: | Albert ARIBAUD |
| Headers | show |
On Friday, July 04, 2014 at 12:19:14 PM, Masahiro Yamada wrote: > The driver for on-chip UART used on Panasonic UniPhier platform. > > Signed-off-by: Masahiro Yamada <yamada.m@jp.panasonic.com> [...] > +static void uniphier_serial_init(struct uniphier_serial *port) > +{ > + writeb(UART_LCR_WLS_8, &port->lcr); > + > +#define MODE_X_DIV 16 You can use just const unsigned here instead of #define. > + > + /* Compute divisor value. Normally, we should simply return: > + * CONFIG_SYS_NS16550_CLK) / MODE_X_DIV / gd->baudrate > + * but we need to round that value by adding 0.5. > + * Rounding is especially important at high baud rates. > + */ > + writew((CONFIG_SYS_UNIPHIER_UART_CLK + (gd->baudrate * > + (MODE_X_DIV / 2))) / (MODE_X_DIV * gd->baudrate), &port->dlr); > +} > + > +static void uniphier_serial_setbrg(struct uniphier_serial *port) > +{ > + uniphier_serial_init(port); > +} > + > +static int uniphier_serial_tstc(struct uniphier_serial *port) > +{ > + return (readb(&port->lsr) & UART_LSR_DR) != 0; > +} > + > +static int uniphier_serial_getc(struct uniphier_serial *port) > +{ > + while (!uniphier_serial_tstc(port)) > + ; > + > + return readb(&port->rbr); > +} > + > +static void uniphier_serial_putc(struct uniphier_serial *port, const char > c) +{ > + if (c == '\n') > + uniphier_serial_putc(port, '\r'); > + > + while (!(readb(&port->lsr) & UART_LSR_THRE)) > + ; I think in this function, you can avoid such completely unbounded loop. [...] Best regard, Marek Vasut
Hi Marek, On Fri, 4 Jul 2014 16:40:29 +0200 Marek Vasut <marex@denx.de> wrote: > > +static void uniphier_serial_init(struct uniphier_serial *port) > > +{ > > + writeb(UART_LCR_WLS_8, &port->lcr); > > + > > +#define MODE_X_DIV 16 > > You can use just const unsigned here instead of #define. I adjusted drivers/serial/serial_ns16550.c for my own. Is using a macro here so bad? If so, should I also fix the line 138 of drivers/serial/serial_ns16550.c ? > > +static void uniphier_serial_putc(struct uniphier_serial *port, const char > > c) +{ > > + if (c == '\n') > > + uniphier_serial_putc(port, '\r'); > > + > > + while (!(readb(&port->lsr) & UART_LSR_THRE)) > > + ; > > I think in this function, you can avoid such completely unbounded loop. > > [...] Why? Could you give me more detailed explanation? Best Regards Masahiro Yamada
On Thursday, July 10, 2014 at 12:06:50 PM, Masahiro Yamada wrote: > Hi Marek, > > > On Fri, 4 Jul 2014 16:40:29 +0200 > > Marek Vasut <marex@denx.de> wrote: > > > +static void uniphier_serial_init(struct uniphier_serial *port) > > > +{ > > > + writeb(UART_LCR_WLS_8, &port->lcr); > > > + > > > +#define MODE_X_DIV 16 > > > > You can use just const unsigned here instead of #define. > > I adjusted drivers/serial/serial_ns16550.c for my own. > > Is using a macro here so bad? > If so, should I also fix the line 138 of drivers/serial/serial_ns16550.c ? You're loosing the typechecking, that's all. > > > +static void uniphier_serial_putc(struct uniphier_serial *port, const > > > char c) +{ > > > + if (c == '\n') > > > + uniphier_serial_putc(port, '\r'); > > > + > > > + while (!(readb(&port->lsr) & UART_LSR_THRE)) > > > + ; > > > > I think in this function, you can avoid such completely unbounded loop. > > > > [...] > > Why? > Could you give me more detailed explanation? In case the LSR_THRE bit would never get cleared, this loop would keep spinning forever. On the other hand, if there was some kind of a timeout, U-Boot would be able to continue doing at least something when exiting this loop by timing out. Though you're right that something would most likely have gone really bonkers if this LSR_THRE never got cleared. Best regards, Marek Vasut
Hi Marek, On Thu, 10 Jul 2014 12:14:35 +0200 Marek Vasut <marex@denx.de> wrote: > > > > > +static void uniphier_serial_putc(struct uniphier_serial *port, const > > > > char c) +{ > > > > + if (c == '\n') > > > > + uniphier_serial_putc(port, '\r'); > > > > + > > > > + while (!(readb(&port->lsr) & UART_LSR_THRE)) > > > > + ; > > > > > > I think in this function, you can avoid such completely unbounded loop. > > > > > > [...] > > > > Why? > > Could you give me more detailed explanation? > > In case the LSR_THRE bit would never get cleared, this loop would keep spinning > forever. On the other hand, if there was some kind of a timeout, U-Boot would be > able to continue doing at least something when exiting this loop by timing out. > Though you're right that something would most likely have gone really bonkers if > this LSR_THRE never got cleared. > I guess you mentioned the case where the flow control with CTS/RTS results in the dead lock. I am not sure if we need to be so careful. Is there any other UART driver using timeout check? (What would U-Boot continue to do in case console cannot display the error message?) Best Regards Masahiro Masahiro
On Thursday, July 10, 2014 at 01:31:01 PM, Masahiro Yamada wrote: > Hi Marek, Hi! > On Thu, 10 Jul 2014 12:14:35 +0200 > > Marek Vasut <marex@denx.de> wrote: > > > > > +static void uniphier_serial_putc(struct uniphier_serial *port, > > > > > const char c) +{ > > > > > + if (c == '\n') > > > > > + uniphier_serial_putc(port, '\r'); > > > > > + > > > > > + while (!(readb(&port->lsr) & UART_LSR_THRE)) > > > > > + ; > > > > > > > > I think in this function, you can avoid such completely unbounded > > > > loop. > > > > > > > > [...] > > > > > > Why? > > > Could you give me more detailed explanation? > > > > In case the LSR_THRE bit would never get cleared, this loop would keep > > spinning forever. On the other hand, if there was some kind of a > > timeout, U-Boot would be able to continue doing at least something when > > exiting this loop by timing out. Though you're right that something > > would most likely have gone really bonkers if this LSR_THRE never got > > cleared. > > I guess you mentioned the case where the flow control with CTS/RTS > results in the dead lock. Either that or plain hardware failure. > I am not sure if we need to be so careful. > > Is there any other UART driver using timeout check? I don't know. If you feel this is not needed, then please just ignore my suggestion. I'm just pushing the usual "let's not do unbounded loops where reasonable" thing here ;-) > (What would U-Boot continue to do in case console cannot display the error > message?) It can still boot kernel, even if it cannot print on console. Best regards, Marek Vasut
Hi Marek, On Thu, 10 Jul 2014 14:37:18 +0200 Marek Vasut <marex@denx.de> wrote: > On Thursday, July 10, 2014 at 01:31:01 PM, Masahiro Yamada wrote: > > Hi Marek, > > Hi! > > > On Thu, 10 Jul 2014 12:14:35 +0200 > > > > Marek Vasut <marex@denx.de> wrote: > > > > > > +static void uniphier_serial_putc(struct uniphier_serial *port, > > > > > > const char c) +{ > > > > > > + if (c == '\n') > > > > > > + uniphier_serial_putc(port, '\r'); > > > > > > + > > > > > > + while (!(readb(&port->lsr) & UART_LSR_THRE)) > > > > > > + ; > > > > > > > > > > I think in this function, you can avoid such completely unbounded > > > > > loop. > > > > > > > > > > [...] > > > > > > > > Why? > > > > Could you give me more detailed explanation? > > > > > > In case the LSR_THRE bit would never get cleared, this loop would keep > > > spinning forever. On the other hand, if there was some kind of a > > > timeout, U-Boot would be able to continue doing at least something when > > > exiting this loop by timing out. Though you're right that something > > > would most likely have gone really bonkers if this LSR_THRE never got > > > cleared. > > > > I guess you mentioned the case where the flow control with CTS/RTS > > results in the dead lock. > > Either that or plain hardware failure. > > > I am not sure if we need to be so careful. > > > > Is there any other UART driver using timeout check? > > I don't know. If you feel this is not needed, then please just ignore my > suggestion. I'm just pushing the usual "let's not do unbounded loops where > reasonable" thing here ;-) > > > (What would U-Boot continue to do in case console cannot display the error > > message?) > > It can still boot kernel, even if it cannot print on console. > Finally, I decided to keep this as is because: [1] Our on-chip UART do not have the flow control feature. It uses only TX/RX signals. [2] So, the eternal loop occurrs only when the hardware is broken. In this case, the situation is really fatal and I don't think it is reasonable to continue to do something. [3] I checked some (but not all) other serial drivers but I could not find ones using timeout and break thing. Best Regards Masahiro Yamada
On Friday, July 11, 2014 at 07:37:13 AM, Masahiro Yamada wrote: [...] > > It can still boot kernel, even if it cannot print on console. > > Finally, I decided to keep this as is because: > > [1] Our on-chip UART do not have the flow control feature. > It uses only TX/RX signals. > > [2] So, the eternal loop occurrs only when the hardware is broken. > In this case, the situation is really fatal and I don't think > it is reasonable to continue to do something. > > [3] I checked some (but not all) other serial drivers but I could not find > ones using timeout and break thing. Hi! All right then. Best regards, Marek Vasut
diff --git a/drivers/serial/Makefile b/drivers/serial/Makefile index 571c18f..385b2f9 100644 --- a/drivers/serial/Makefile +++ b/drivers/serial/Makefile @@ -34,6 +34,7 @@ obj-$(CONFIG_BFIN_SERIAL) += serial_bfin.o obj-$(CONFIG_FSL_LPUART) += serial_lpuart.o obj-$(CONFIG_MXS_AUART) += mxs_auart.o obj-$(CONFIG_ARC_SERIAL) += serial_arc.o +obj-$(CONFIG_UNIPHIER_SERIAL) += serial_uniphier.o ifndef CONFIG_SPL_BUILD obj-$(CONFIG_USB_TTY) += usbtty.o diff --git a/drivers/serial/serial.c b/drivers/serial/serial.c index fd61a5e..9a05bef 100644 --- a/drivers/serial/serial.c +++ b/drivers/serial/serial.c @@ -157,6 +157,7 @@ serial_initfunc(sh_serial_initialize); serial_initfunc(arm_dcc_initialize); serial_initfunc(mxs_auart_initialize); serial_initfunc(arc_serial_initialize); +serial_initfunc(uniphier_serial_initialize); /** * serial_register() - Register serial driver with serial driver core @@ -250,6 +251,7 @@ void serial_initialize(void) arm_dcc_initialize(); mxs_auart_initialize(); arc_serial_initialize(); + uniphier_serial_initialize(); serial_assign(default_serial_console()->name); } diff --git a/drivers/serial/serial_uniphier.c b/drivers/serial/serial_uniphier.c new file mode 100644 index 0000000..82c754c --- /dev/null +++ b/drivers/serial/serial_uniphier.c @@ -0,0 +1,206 @@ +/* + * Copyright (C) 2012-2014 Panasonic Corporation + * Author: Masahiro Yamada <yamada.m@jp.panasonic.com> + * + * Based on serial_ns16550.c + * (C) Copyright 2000 + * Rob Taylor, Flying Pig Systems. robt@flyingpig.com. + * + * SPDX-License-Identifier: GPL-2.0+ + */ + +#include <common.h> +#include <serial.h> + +#define UART_REG(x) \ + u8 x; \ + u8 postpad_##x[3]; + +/* + * Note: Register map is slightly different from that of 16550. + */ +struct uniphier_serial { + UART_REG(rbr); /* 0x00 */ + UART_REG(ier); /* 0x04 */ + UART_REG(iir); /* 0x08 */ + UART_REG(fcr); /* 0x0c */ + u8 mcr; /* 0x10 */ + u8 lcr; + u16 __postpad; + UART_REG(lsr); /* 0x14 */ + UART_REG(msr); /* 0x18 */ + u32 __none1; + u32 __none2; + u16 dlr; + u16 __postpad2; +}; + +#define thr rbr + +/* + * These are the definitions for the Line Control Register + */ +#define UART_LCR_WLS_8 0x03 /* 8 bit character length */ + +/* + * These are the definitions for the Line Status Register + */ +#define UART_LSR_DR 0x01 /* Data ready */ +#define UART_LSR_THRE 0x20 /* Xmit holding register empty */ + +DECLARE_GLOBAL_DATA_PTR; + +static void uniphier_serial_init(struct uniphier_serial *port) +{ + writeb(UART_LCR_WLS_8, &port->lcr); + +#define MODE_X_DIV 16 + + /* Compute divisor value. Normally, we should simply return: + * CONFIG_SYS_NS16550_CLK) / MODE_X_DIV / gd->baudrate + * but we need to round that value by adding 0.5. + * Rounding is especially important at high baud rates. + */ + writew((CONFIG_SYS_UNIPHIER_UART_CLK + (gd->baudrate * + (MODE_X_DIV / 2))) / (MODE_X_DIV * gd->baudrate), &port->dlr); +} + +static void uniphier_serial_setbrg(struct uniphier_serial *port) +{ + uniphier_serial_init(port); +} + +static int uniphier_serial_tstc(struct uniphier_serial *port) +{ + return (readb(&port->lsr) & UART_LSR_DR) != 0; +} + +static int uniphier_serial_getc(struct uniphier_serial *port) +{ + while (!uniphier_serial_tstc(port)) + ; + + return readb(&port->rbr); +} + +static void uniphier_serial_putc(struct uniphier_serial *port, const char c) +{ + if (c == '\n') + uniphier_serial_putc(port, '\r'); + + while (!(readb(&port->lsr) & UART_LSR_THRE)) + ; + + writeb(c, &port->thr); +} + +static struct uniphier_serial *serial_ports[4] = { +#ifdef CONFIG_SYS_UNIPHIER_SERIAL_BASE0 + (struct uniphier_serial *)CONFIG_SYS_UNIPHIER_SERIAL_BASE0, +#else + NULL, +#endif +#ifdef CONFIG_SYS_UNIPHIER_SERIAL_BASE1 + (struct uniphier_serial *)CONFIG_SYS_UNIPHIER_SERIAL_BASE1, +#else + NULL, +#endif +#ifdef CONFIG_SYS_UNIPHIER_SERIAL_BASE2 + (struct uniphier_serial *)CONFIG_SYS_UNIPHIER_SERIAL_BASE2, +#else + NULL, +#endif +#ifdef CONFIG_SYS_UNIPHIER_SERIAL_BASE3 + (struct uniphier_serial *)CONFIG_SYS_UNIPHIER_SERIAL_BASE3, +#else + NULL, +#endif +}; + +/* Multi serial device functions */ +#define DECLARE_ESERIAL_FUNCTIONS(port) \ + static int eserial##port##_init(void) \ + { \ + uniphier_serial_init(serial_ports[port]); \ + return 0 ; \ + } \ + static void eserial##port##_setbrg(void) \ + { \ + uniphier_serial_setbrg(serial_ports[port]); \ + } \ + static int eserial##port##_getc(void) \ + { \ + return uniphier_serial_getc(serial_ports[port]); \ + } \ + static int eserial##port##_tstc(void) \ + { \ + return uniphier_serial_tstc(serial_ports[port]); \ + } \ + static void eserial##port##_putc(const char c) \ + { \ + uniphier_serial_putc(serial_ports[port], c); \ + } + +/* Serial device descriptor */ +#define INIT_ESERIAL_STRUCTURE(port, __name) { \ + .name = __name, \ + .start = eserial##port##_init, \ + .stop = NULL, \ + .setbrg = eserial##port##_setbrg, \ + .getc = eserial##port##_getc, \ + .tstc = eserial##port##_tstc, \ + .putc = eserial##port##_putc, \ + .puts = default_serial_puts, \ +} + +#if defined(CONFIG_SYS_UNIPHIER_SERIAL_BASE0) +DECLARE_ESERIAL_FUNCTIONS(0); +struct serial_device uniphier_serial0_device = + INIT_ESERIAL_STRUCTURE(0, "ttyS0"); +#endif +#if defined(CONFIG_SYS_UNIPHIER_SERIAL_BASE1) +DECLARE_ESERIAL_FUNCTIONS(1); +struct serial_device uniphier_serial1_device = + INIT_ESERIAL_STRUCTURE(1, "ttyS1"); +#endif +#if defined(CONFIG_SYS_UNIPHIER_SERIAL_BASE2) +DECLARE_ESERIAL_FUNCTIONS(2); +struct serial_device uniphier_serial2_device = + INIT_ESERIAL_STRUCTURE(2, "ttyS2"); +#endif +#if defined(CONFIG_SYS_UNIPHIER_SERIAL_BASE3) +DECLARE_ESERIAL_FUNCTIONS(3); +struct serial_device uniphier_serial3_device = + INIT_ESERIAL_STRUCTURE(3, "ttyS3"); +#endif + +__weak struct serial_device *default_serial_console(void) +{ +#if defined(CONFIG_SYS_UNIPHIER_SERIAL_BASE0) + return &uniphier_serial0_device; +#elif defined(CONFIG_SYS_UNIPHIER_SERIAL_BASE1) + return &uniphier_serial1_device; +#elif defined(CONFIG_SYS_UNIPHIER_SERIAL_BASE2) + return &uniphier_serial2_device; +#elif defined(CONFIG_SYS_UNIPHIER_SERIAL_BASE3) + return &uniphier_serial3_device; +#else +#error "No uniphier serial ports configured." +#endif +} + +void uniphier_serial_initialize(void) +{ +#if defined(CONFIG_SYS_UNIPHIER_SERIAL_BASE0) + serial_register(&uniphier_serial0_device); +#endif +#if defined(CONFIG_SYS_UNIPHIER_SERIAL_BASE1) + serial_register(&uniphier_serial1_device); +#endif +#if defined(CONFIG_SYS_UNIPHIER_SERIAL_BASE2) + serial_register(&uniphier_serial2_device); +#endif +#if defined(CONFIG_SYS_UNIPHIER_SERIAL_BASE3) + serial_register(&uniphier_serial3_device); +#endif +}
The driver for on-chip UART used on Panasonic UniPhier platform. Signed-off-by: Masahiro Yamada <yamada.m@jp.panasonic.com> --- drivers/serial/Makefile | 1 + drivers/serial/serial.c | 2 + drivers/serial/serial_uniphier.c | 206 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 209 insertions(+) create mode 100644 drivers/serial/serial_uniphier.c