diff mbox

[U-Boot,v4,2/6] serial: add UniPhier serial driver

Message ID 1409896223-15994-3-git-send-email-yamada.m@jp.panasonic.com
State Superseded
Delegated to: Masahiro Yamada
Headers show

Commit Message

Masahiro Yamada Sept. 5, 2014, 5:50 a.m. UTC
The driver for on-chip UART used on Panasonic UniPhier platform.

Signed-off-by: Masahiro Yamada <yamada.m@jp.panasonic.com>
---

Changes in v4: None
Changes in v3: None
Changes in v2:
  - Use "const unsigned int mode_x_div = 16"
      instead of "#define MODE_X_DIV   16"
  - Use DIV_ROUND_CLOSEST() macro to compute the divisor

 drivers/serial/Makefile          |   1 +
 drivers/serial/serial.c          |   2 +
 drivers/serial/serial_uniphier.c | 204 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 207 insertions(+)
 create mode 100644 drivers/serial/serial_uniphier.c

Comments

Marek Vasut Sept. 5, 2014, 10:35 a.m. UTC | #1
On Friday, September 05, 2014 at 07:50:19 AM, Masahiro Yamada wrote:
> The driver for on-chip UART used on Panasonic UniPhier platform.
> 
> Signed-off-by: Masahiro Yamada <yamada.m@jp.panasonic.com>

[...]

Hi!

> +static void uniphier_serial_putc(struct uniphier_serial *port, const char
> c) +{
> +	if (c == '\n')
> +		uniphier_serial_putc(port, '\r');

Just curious, but what is the concensus about inserting \r upon \n ? Shouldn't 
this be something that the "upper layers" do consistently ? I recall seeing this 
in some drivers and not seeing this in the others, so I wonder why this is like 
so ...

> +	while (!(readb(&port->lsr) & UART_LSR_THRE))
> +		;
> +
> +	writeb(c, &port->thr);
> +}
[...]
Best regards,
Marek Vasut
Masahiro Yamada Sept. 5, 2014, 12:03 p.m. UTC | #2
Hi Marek,



On Fri, 5 Sep 2014 12:35:18 +0200
Marek Vasut <marex@denx.de> wrote:

> On Friday, September 05, 2014 at 07:50:19 AM, Masahiro Yamada wrote:
> > The driver for on-chip UART used on Panasonic UniPhier platform.
> > 
> > Signed-off-by: Masahiro Yamada <yamada.m@jp.panasonic.com>
> 
> [...]
> 
> Hi!
> 
> > +static void uniphier_serial_putc(struct uniphier_serial *port, const char
> > c) +{
> > +	if (c == '\n')
> > +		uniphier_serial_putc(port, '\r');
> 
> Just curious, but what is the concensus about inserting \r upon \n ? Shouldn't 
> this be something that the "upper layers" do consistently ? I recall seeing this 
> in some drivers and not seeing this in the others, so I wonder why this is like 
> so ...


This converts "\n" to "\r\n".

Without this conversion, CarriageReturn is not provided,
which means the cursor goes to the next line, but
column position does not change.


For example,

printf("Hello\nWorld\n");

will be displayed on (at least my) terminal emulator like this:


Hello
     World


With the conversion code, it will be displaye as follows:

Hello
World



Perhaps the behavior might depend on
which therminal emulator you are using.
(also depend on the preference
how LF and CR are handled.)



Maybe we can move  "\n -> \r\n" logic
to the upper layer and allow users to enable/disable it
with a CONFIG_ option.




Best Regards
Masahiro Yamada
Marek Vasut Sept. 5, 2014, 12:59 p.m. UTC | #3
On Friday, September 05, 2014 at 02:03:38 PM, Masahiro Yamada wrote:
> Hi Marek,
> 
> 
> 
> On Fri, 5 Sep 2014 12:35:18 +0200
> 
> Marek Vasut <marex@denx.de> wrote:
> > On Friday, September 05, 2014 at 07:50:19 AM, Masahiro Yamada wrote:
> > > The driver for on-chip UART used on Panasonic UniPhier platform.
> > > 
> > > Signed-off-by: Masahiro Yamada <yamada.m@jp.panasonic.com>
> > 
> > [...]
> > 
> > Hi!
> > 
> > > +static void uniphier_serial_putc(struct uniphier_serial *port, const
> > > char c) +{
> > > +	if (c == '\n')
> > > +		uniphier_serial_putc(port, '\r');
> > 
> > Just curious, but what is the concensus about inserting \r upon \n ?
> > Shouldn't this be something that the "upper layers" do consistently ? I
> > recall seeing this in some drivers and not seeing this in the others, so
> > I wonder why this is like so ...
> 
> This converts "\n" to "\r\n".

Apologies, you're right. This is what I meant.

> Without this conversion, CarriageReturn is not provided,
> which means the cursor goes to the next line, but
> column position does not change.
> 
> 
> For example,
> 
> printf("Hello\nWorld\n");
> 
> will be displayed on (at least my) terminal emulator like this:
> 
> 
> Hello
>      World
> 
> 
> With the conversion code, it will be displaye as follows:
> 
> Hello
> World
> 
> Perhaps the behavior might depend on
> which therminal emulator you are using.
> (also depend on the preference
> how LF and CR are handled.)

I use minicom . You do have a point that it might be it.

> Maybe we can move  "\n -> \r\n" logic
> to the upper layer and allow users to enable/disable it
> with a CONFIG_ option.

Either that or make it even run-time configurable, esp. if this depends on the 
users' terminal setting.

Best regards,
Marek Vasut
Simon Glass Sept. 5, 2014, 4:41 p.m. UTC | #4
Hi Masahiro,

On 5 September 2014 06:03, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
> Hi Marek,
>
>
>
> On Fri, 5 Sep 2014 12:35:18 +0200
> Marek Vasut <marex@denx.de> wrote:
>
>> On Friday, September 05, 2014 at 07:50:19 AM, Masahiro Yamada wrote:
>> > The driver for on-chip UART used on Panasonic UniPhier platform.
>> >
>> > Signed-off-by: Masahiro Yamada <yamada.m@jp.panasonic.com>
>>
>> [...]
>>
>> Hi!
>>
>> > +static void uniphier_serial_putc(struct uniphier_serial *port, const char
>> > c) +{
>> > +   if (c == '\n')
>> > +           uniphier_serial_putc(port, '\r');
>>
>> Just curious, but what is the concensus about inserting \r upon \n ? Shouldn't
>> this be something that the "upper layers" do consistently ? I recall seeing this
>> in some drivers and not seeing this in the others, so I wonder why this is like
>> so ...
>
>
> This converts "\n" to "\r\n".
>
> Without this conversion, CarriageReturn is not provided,
> which means the cursor goes to the next line, but
> column position does not change.
>
>
> For example,
>
> printf("Hello\nWorld\n");
>
> will be displayed on (at least my) terminal emulator like this:
>
>
> Hello
>      World
>
>
> With the conversion code, it will be displaye as follows:
>
> Hello
> World
>
>
>
> Perhaps the behavior might depend on
> which therminal emulator you are using.
> (also depend on the preference
> how LF and CR are handled.)
>
>
>
> Maybe we can move  "\n -> \r\n" logic
> to the upper layer and allow users to enable/disable it
> with a CONFIG_ option.

Do you think we could use driver model instead? We have the serial
infrastructure in place and I will likely merge it next week.

It moves the \r\n logic to a higher level.

It also removes the need for all the horrible #define stuff you have
here to deal with multiple serial ports.

Does your board have SPL? If so, does it use serial in SPL?

Regards,
Simon
Masahiro Yamada Sept. 6, 2014, 2:49 p.m. UTC | #5
Hi Simon,


2014-09-06 1:41 GMT+09:00 Simon Glass <sjg@chromium.org>:
>> Maybe we can move  "\n -> \r\n" logic
>> to the upper layer and allow users to enable/disable it
>> with a CONFIG_ option.
>
> Do you think we could use driver model instead? We have the serial
> infrastructure in place and I will likely merge it next week.

OK.
I haven't checked it yet, but I will next week.



> It moves the \r\n logic to a higher level.
>
> It also removes the need for all the horrible #define stuff you have
> here to deal with multiple serial ports.
>
> Does your board have SPL? If so, does it use serial in SPL?

Yes, all the UniPhier boards have SPL support.

The serial in SPL is not currently supported, but I am planning to add
it in the next phase.
Masahiro Yamada Sept. 19, 2014, 12:15 p.m. UTC | #6
Hi Simon,



On Fri, 5 Sep 2014 10:41:54 -0600
Simon Glass <sjg@chromium.org> wrote:
> Do you think we could use driver model instead? We have the serial
> infrastructure in place and I will likely merge it next week.
> 
> It moves the \r\n logic to a higher level.
> 
> It also removes the need for all the horrible #define stuff you have
> here to deal with multiple serial ports.




I am seeing serial_find_console_or_panic() func
in drivers/serial/serial-uclass.c


static void serial_find_console_or_panic(void)
{
	int node;

	/* Check for a chosen console */
	node = fdtdec_get_chosen_node(gd->fdt_blob, "stdout-path");
	if (node < 0)
		node = fdtdec_get_alias_node(gd->fdt_blob, "console");
	if (!uclass_get_device_by_of_offset(UCLASS_SERIAL, node, &cur_dev))
		return;

	/*
	 * If the console is not marked to be bound before relocation, bind
	 * it anyway.
	 */
	if (node > 0 &&
	    !lists_bind_fdt(gd->dm_root, gd->fdt_blob, node, &cur_dev)) {
		if (!device_probe(cur_dev))
			return;
		cur_dev = NULL;
	}





It looks like CONFIG_DM_SERIAL depends on CONFIG_OF_CONTROL.


UniPhier SoCs do not support device tree control.
Is the driver model serial still available?
What will happen if gd->fdt_blob is not set?



Best Regards
Masahiro Yamada
Simon Glass Sept. 19, 2014, 4:30 p.m. UTC | #7
HI Masahiro,

On 19 September 2014 06:15, Masahiro Yamada <yamada.m@jp.panasonic.com>
wrote:

> Hi Simon,
>
>
>
> On Fri, 5 Sep 2014 10:41:54 -0600
> Simon Glass <sjg@chromium.org> wrote:
> > Do you think we could use driver model instead? We have the serial
> > infrastructure in place and I will likely merge it next week.
> >
> > It moves the \r\n logic to a higher level.
> >
> > It also removes the need for all the horrible #define stuff you have
> > here to deal with multiple serial ports.
>
>
>
>
> I am seeing serial_find_console_or_panic() func
> in drivers/serial/serial-uclass.c
>
>
> static void serial_find_console_or_panic(void)
> {
>         int node;
>
>         /* Check for a chosen console */
>         node = fdtdec_get_chosen_node(gd->fdt_blob, "stdout-path");
>         if (node < 0)
>                 node = fdtdec_get_alias_node(gd->fdt_blob, "console");
>         if (!uclass_get_device_by_of_offset(UCLASS_SERIAL, node, &cur_dev))
>                 return;
>
>         /*
>          * If the console is not marked to be bound before relocation, bind
>          * it anyway.
>          */
>         if (node > 0 &&
>             !lists_bind_fdt(gd->dm_root, gd->fdt_blob, node, &cur_dev)) {
>                 if (!device_probe(cur_dev))
>                         return;
>                 cur_dev = NULL;
>         }
>
>
>
>
>
> It looks like CONFIG_DM_SERIAL depends on CONFIG_OF_CONTROL.
>
>
> UniPhier SoCs do not support device tree control.
> Is the driver model serial still available?
> What will happen if gd->fdt_blob is not set?
>

Please this patch.

http://patchwork.ozlabs.org/patch/390433/

I haven't got to a pull request yet for DM, but will do this in the next
few days.

Regards,
Simon
Masahiro Yamada Sept. 20, 2014, 7:18 a.m. UTC | #8
Hi Simon,


2014-09-20 1:30 GMT+09:00 Simon Glass <sjg@chromium.org>:
>>
>> It looks like CONFIG_DM_SERIAL depends on CONFIG_OF_CONTROL.
>>
>>
>> UniPhier SoCs do not support device tree control.
>> Is the driver model serial still available?
>> What will happen if gd->fdt_blob is not set?
>>
>
> Please this patch.
>
> http://patchwork.ozlabs.org/patch/390433/
>
> I haven't got to a pull request yet for DM, but will do this in the next
> few days.
>


Can I postpone the driver-model conversion in the next phase?

My first priority is to include the core support of Panasonic SoCs in
the 2014.10 release.

We do not have a month before that.
I do not want to be aggressive now.

First, I want the current well-tested code to be upstreamed and see
where I stand.
And then I can move forward to the next development.

(Once I have a stable code in the mainline,
I can consult "git-bisect" whenever something is broken in the future
development.)
Simon Glass Sept. 22, 2014, 6:35 a.m. UTC | #9
Hi Masahiro,

On 20 September 2014 01:18, Masahiro YAMADA <yamada.m@jp.panasonic.com> wrote:
> Hi Simon,
>
>
> 2014-09-20 1:30 GMT+09:00 Simon Glass <sjg@chromium.org>:
>>>
>>> It looks like CONFIG_DM_SERIAL depends on CONFIG_OF_CONTROL.
>>>
>>>
>>> UniPhier SoCs do not support device tree control.
>>> Is the driver model serial still available?
>>> What will happen if gd->fdt_blob is not set?
>>>
>>
>> Please this patch.
>>
>> http://patchwork.ozlabs.org/patch/390433/
>>
>> I haven't got to a pull request yet for DM, but will do this in the next
>> few days.
>>
>
>
> Can I postpone the driver-model conversion in the next phase?
>
> My first priority is to include the core support of Panasonic SoCs in
> the 2014.10 release.
>
> We do not have a month before that.
> I do not want to be aggressive now.
>
> First, I want the current well-tested code to be upstreamed and see
> where I stand.
> And then I can move forward to the next development.
>
> (Once I have a stable code in the mainline,
> I can consult "git-bisect" whenever something is broken in the future
> development.)

That seems fine to me. It is good to do things in stages.

Regards,
Simon
diff mbox

Patch

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 d2eb752..d32673e 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..f8c9d92
--- /dev/null
+++ b/drivers/serial/serial_uniphier.c
@@ -0,0 +1,204 @@ 
+/*
+ * 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)
+{
+	const unsigned int mode_x_div = 16;
+	unsigned int divisor;
+
+	writeb(UART_LCR_WLS_8, &port->lcr);
+
+	divisor = DIV_ROUND_CLOSEST(CONFIG_SYS_UNIPHIER_UART_CLK,
+						mode_x_div * gd->baudrate);
+
+	writew(divisor, &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
+}