[PATCH v2 0/2] serial: console: add two features

[PATCH v2 0/2] serial: console: add two features

[Masahiro Yamada]

1/2: add MMIO16 register interface support
2/2: allow to input clock frequency from kernel parameter


Changes in v2:
  - Do not change userspace-exported macros

Masahiro Yamada (2):
  serial: support register interface with 16-bit stride for console
  serial: earlycon: allow to specify uartclk in earlycon
    kernel-parameter

 Documentation/kernel-parameters.txt  |  9 +++++----
 drivers/tty/serial/8250/8250_early.c |  5 +++++
 drivers/tty/serial/8250/8250_port.c  | 20 ++++++++++++++++++++
 drivers/tty/serial/earlycon.c        | 23 ++++++++++++++++++-----
 drivers/tty/serial/serial_core.c     |  9 +++++++--
 include/linux/serial_core.h          |  1 +
 include/uapi/linux/serial.h          |  1 +
 7 files changed, 57 insertions(+), 11 deletions(-)

-- 
1.9.1

[PATCH v2 1/2] serial: support register interface with 16-bit stride for console

[Masahiro Yamada]

Currently, 8-bit (MMIO) and 32-bit (MMIO32) register strides are
supported for the 8250 console, but 16-bit (MMIO16) stride is not.
The 8250 UART device on my board is connected to a 16-bit bus
(reg-shift = <1>) and I am eager to use earlycon with it.

Refer to arch/arm/boot/dts/uniphier-support-card.dtsi:

        serialsc: uart@000b0000 {
                compatible = "ns16550a";
                reg = <0x000b0000 0x20>;
                clock-frequency = <12288000>;
                reg-shift = <1>;
        };

Signed-off-by: Masahiro Yamada <yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org>
---

Changes in v2:
  - Do not change userspace-exported macros

 Documentation/kernel-parameters.txt  |  9 +++++----
 drivers/tty/serial/8250/8250_early.c |  5 +++++
 drivers/tty/serial/8250/8250_port.c  | 20 ++++++++++++++++++++
 drivers/tty/serial/earlycon.c        | 15 +++++++++++----
 drivers/tty/serial/serial_core.c     |  9 +++++++--
 include/linux/serial_core.h          |  1 +
 include/uapi/linux/serial.h          |  1 +
 7 files changed, 50 insertions(+), 10 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 22a4b68..761f08c 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -720,16 +720,17 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 
 		uart[8250],io,<addr>[,options]
 		uart[8250],mmio,<addr>[,options]
+		uart[8250],mmio16,<addr>[,options]
 		uart[8250],mmio32,<addr>[,options]
 		uart[8250],0x<addr>[,options]
 			Start an early, polled-mode console on the 8250/16550
 			UART at the specified I/O port or MMIO address,
 			switching to the matching ttyS device later.
 			MMIO inter-register address stride is either 8-bit
-			(mmio) or 32-bit (mmio32).
-			If none of [io|mmio|mmio32], <addr> is assumed to be
-			equivalent to 'mmio'. 'options' are specified in the
-			same format described for ttyS above; if unspecified,
+			(mmio), 16-bit (mmio16), or 32-bit (mmio32).
+			If none of [io|mmio|mmio16|mmio32], <addr> is assumed
+			to be equivalent to 'mmio'. 'options' are specified in
+			the same format described for ttyS above; if unspecified,
 			the h/w is not re-initialized.
 
 		hvc<n>	Use the hypervisor console device <n>. This is for
diff --git a/drivers/tty/serial/8250/8250_early.c b/drivers/tty/serial/8250/8250_early.c
index faed05f..7aff3d8 100644
--- a/drivers/tty/serial/8250/8250_early.c
+++ b/drivers/tty/serial/8250/8250_early.c
@@ -40,6 +40,8 @@ static unsigned int __init serial8250_early_in(struct uart_port *port, int offse
 	switch (port->iotype) {
 	case UPIO_MEM:
 		return readb(port->membase + offset);
+	case UPIO_MEM16:
+		return readw(port->membase + (offset << 1));
 	case UPIO_MEM32:
 		return readl(port->membase + (offset << 2));
 	case UPIO_MEM32BE:
@@ -57,6 +59,9 @@ static void __init serial8250_early_out(struct uart_port *port, int offset, int
 	case UPIO_MEM:
 		writeb(value, port->membase + offset);
 		break;
+	case UPIO_MEM16:
+		writew(value, port->membase + (offset << 1));
+		break;
 	case UPIO_MEM32:
 		writel(value, port->membase + (offset << 2));
 		break;
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 0bbf340..f976948 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -368,6 +368,18 @@ static void mem_serial_out(struct uart_port *p, int offset, int value)
 	writeb(value, p->membase + offset);
 }
 
+static void mem16_serial_out(struct uart_port *p, int offset, int value)
+{
+	offset = offset << p->regshift;
+	writew(value, p->membase + offset);
+}
+
+static unsigned int mem16_serial_in(struct uart_port *p, int offset)
+{
+	offset = offset << p->regshift;
+	return readw(p->membase + offset);
+}
+
 static void mem32_serial_out(struct uart_port *p, int offset, int value)
 {
 	offset = offset << p->regshift;
@@ -425,6 +437,11 @@ static void set_io_from_upio(struct uart_port *p)
 		p->serial_out = mem_serial_out;
 		break;
 
+	case UPIO_MEM16:
+		p->serial_in = mem16_serial_in;
+		p->serial_out = mem16_serial_out;
+		break;
+
 	case UPIO_MEM32:
 		p->serial_in = mem32_serial_in;
 		p->serial_out = mem32_serial_out;
@@ -459,6 +476,7 @@ serial_port_out_sync(struct uart_port *p, int offset, int value)
 {
 	switch (p->iotype) {
 	case UPIO_MEM:
+	case UPIO_MEM16:
 	case UPIO_MEM32:
 	case UPIO_MEM32BE:
 	case UPIO_AU:
@@ -2447,6 +2465,7 @@ static int serial8250_request_std_resource(struct uart_8250_port *up)
 	case UPIO_TSI:
 	case UPIO_MEM32:
 	case UPIO_MEM32BE:
+	case UPIO_MEM16:
 	case UPIO_MEM:
 		if (!port->mapbase)
 			break;
@@ -2484,6 +2503,7 @@ static void serial8250_release_std_resource(struct uart_8250_port *up)
 	case UPIO_TSI:
 	case UPIO_MEM32:
 	case UPIO_MEM32BE:
+	case UPIO_MEM16:
 	case UPIO_MEM:
 		if (!port->mapbase)
 			break;
diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c
index f096360..07f7393 100644
--- a/drivers/tty/serial/earlycon.c
+++ b/drivers/tty/serial/earlycon.c
@@ -71,10 +71,16 @@ static int __init parse_options(struct earlycon_device *device, char *options)
 		return -EINVAL;
 
 	switch (port->iotype) {
+	case UPIO_MEM:
+		port->mapbase = addr;
+		break;
+	case UPIO_MEM16:
+		port->regshift = 1;
+		port->mapbase = addr;
+		break;
 	case UPIO_MEM32:
 	case UPIO_MEM32BE:
-		port->regshift = 2;	/* fall-through */
-	case UPIO_MEM:
+		port->regshift = 2;
 		port->mapbase = addr;
 		break;
 	case UPIO_PORT:
@@ -91,10 +97,11 @@ static int __init parse_options(struct earlycon_device *device, char *options)
 		strlcpy(device->options, options, length);
 	}
 
-	if (port->iotype == UPIO_MEM || port->iotype == UPIO_MEM32 ||
-	    port->iotype == UPIO_MEM32BE)
+	if (port->iotype == UPIO_MEM || port->iotype == UPIO_MEM16 ||
+	    port->iotype == UPIO_MEM32 || port->iotype == UPIO_MEM32BE)
 		pr_info("Early serial console at MMIO%s 0x%llx (options '%s')\n",
 			(port->iotype == UPIO_MEM) ? "" :
+			(port->iotype == UPIO_MEM16) ? "16" :
 			(port->iotype == UPIO_MEM32) ? "32" : "32be",
 			(unsigned long long)port->mapbase,
 			device->options);
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 603d2cc..325acce 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1819,8 +1819,8 @@ uart_get_console(struct uart_port *ports, int nr, struct console *co)
  *	@options: ptr for <options> field; NULL if not present (out)
  *
  *	Decodes earlycon kernel command line parameters of the form
- *	   earlycon=<name>,io|mmio|mmio32|mmio32be,<addr>,<options>
- *	   console=<name>,io|mmio|mmio32|mmio32be,<addr>,<options>
+ *	   earlycon=<name>,io|mmio|mmio16|mmio32|mmio32be,<addr>,<options>
+ *	   console=<name>,io|mmio|mmio16|mmio32|mmio32be,<addr>,<options>
  *
  *	The optional form
  *	   earlycon=<name>,0x<addr>,<options>
@@ -1835,6 +1835,9 @@ int uart_parse_earlycon(char *p, unsigned char *iotype, unsigned long *addr,
 	if (strncmp(p, "mmio,", 5) == 0) {
 		*iotype = UPIO_MEM;
 		p += 5;
+	} else if (strncmp(p, "mmio16,", 7) == 0) {
+		*iotype = UPIO_MEM16;
+		p += 7;
 	} else if (strncmp(p, "mmio32,", 7) == 0) {
 		*iotype = UPIO_MEM32;
 		p += 7;
@@ -2183,6 +2186,7 @@ uart_report_port(struct uart_driver *drv, struct uart_port *port)
 			 "I/O 0x%lx offset 0x%x", port->iobase, port->hub6);
 		break;
 	case UPIO_MEM:
+	case UPIO_MEM16:
 	case UPIO_MEM32:
 	case UPIO_MEM32BE:
 	case UPIO_AU:
@@ -2828,6 +2832,7 @@ int uart_match_port(struct uart_port *port1, struct uart_port *port2)
 		return (port1->iobase == port2->iobase) &&
 		       (port1->hub6   == port2->hub6);
 	case UPIO_MEM:
+	case UPIO_MEM16:
 	case UPIO_MEM32:
 	case UPIO_MEM32BE:
 	case UPIO_AU:
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 297d4fa..35aa87b 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -150,6 +150,7 @@ struct uart_port {
 #define UPIO_AU			(SERIAL_IO_AU)		/* Au1x00 and RT288x type IO */
 #define UPIO_TSI		(SERIAL_IO_TSI)		/* Tsi108/109 type IO */
 #define UPIO_MEM32BE		(SERIAL_IO_MEM32BE)	/* 32b big endian */
+#define UPIO_MEM16		(SERIAL_IO_MEM16)	/* 16b little endian */
 
 	unsigned int		read_status_mask;	/* driver specific */
 	unsigned int		ignore_status_mask;	/* driver specific */
diff --git a/include/uapi/linux/serial.h b/include/uapi/linux/serial.h
index 25331f9..5d59c3e 100644
--- a/include/uapi/linux/serial.h
+++ b/include/uapi/linux/serial.h
@@ -69,6 +69,7 @@ struct serial_struct {
 #define SERIAL_IO_AU	  4
 #define SERIAL_IO_TSI	  5
 #define SERIAL_IO_MEM32BE 6
+#define SERIAL_IO_MEM16	7
 
 #define UART_CLEAR_FIFO		0x01
 #define UART_USE_FIFO		0x02
-- 
1.9.1

Re: [PATCH v2 1/2] serial: support register interface with 16-bit stride for console

[Peter Hurley]

On 10/19/2015 11:36 PM, Masahiro Yamada wrote:
> Currently, 8-bit (MMIO) and 32-bit (MMIO32) register strides are
> supported for the 8250 console, but 16-bit (MMIO16) stride is not.
> The 8250 UART device on my board is connected to a 16-bit bus
> (reg-shift = <1>) and I am eager to use earlycon with it.
> 
> Refer to arch/arm/boot/dts/uniphier-support-card.dtsi:
> 
>         serialsc: uart@000b0000 {
>                 compatible = "ns16550a";
>                 reg = <0x000b0000 0x20>;
>                 clock-frequency = <12288000>;
>                 reg-shift = <1>;
>         };
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
> 
> Changes in v2:
>   - Do not change userspace-exported macros
> 
>  Documentation/kernel-parameters.txt  |  9 +++++----
>  drivers/tty/serial/8250/8250_early.c |  5 +++++
>  drivers/tty/serial/8250/8250_port.c  | 20 ++++++++++++++++++++
>  drivers/tty/serial/earlycon.c        | 15 +++++++++++----
>  drivers/tty/serial/serial_core.c     |  9 +++++++--
>  include/linux/serial_core.h          |  1 +
>  include/uapi/linux/serial.h          |  1 +
>  7 files changed, 50 insertions(+), 10 deletions(-)

[...]

> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 603d2cc..325acce 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -1819,8 +1819,8 @@ uart_get_console(struct uart_port *ports, int nr, struct console *co)
>   *	@options: ptr for <options> field; NULL if not present (out)
>   *
>   *	Decodes earlycon kernel command line parameters of the form
> - *	   earlycon=<name>,io|mmio|mmio32|mmio32be,<addr>,<options>
> - *	   console=<name>,io|mmio|mmio32|mmio32be,<addr>,<options>
> + *	   earlycon=<name>,io|mmio|mmio16|mmio32|mmio32be,<addr>,<options>
> + *	   console=<name>,io|mmio|mmio16|mmio32|mmio32be,<addr>,<options>
>   *
>   *	The optional form
>   *	   earlycon=<name>,0x<addr>,<options>

This hunk fails to apply. Please rebase on top of tty-next branch of
Greg's tty.git tree (git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git)

Regards,
Peter Hurley

[PATCH v2 2/2] serial: earlycon: allow to specify uartclk in earlycon kernel-parameter

[Masahiro Yamada]

The input clock frequency varies from device to device, but the
earlycon uses the fixed frequency (BASE_BAUD * 16).  It makes
impossible to set the correct divisor to the register.

This commit allows to specify the input clock frequency from the
kernel-parameter.

[Example]

    earlycon=uart8250,mmio32,0x43fb0000,115200,12288000

The input clock frequency (12288000, in this case) should be specified
after the baudrate.  If not specified, the default (BASE_BAUD * 16) is
used.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v2: None

 drivers/tty/serial/earlycon.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c
index 07f7393..8030765 100644
--- a/drivers/tty/serial/earlycon.c
+++ b/drivers/tty/serial/earlycon.c
@@ -95,6 +95,13 @@ static int __init parse_options(struct earlycon_device *device, char *options)
 		length = min(strcspn(options, " ") + 1,
 			     (size_t)(sizeof(device->options)));
 		strlcpy(device->options, options, length);
+		options = strchr(options, ',');
+		if (options) {
+			options++;
+			port->uartclk = simple_strtoul(options, NULL, 0);
+		}
+		if (!port->uartclk)
+			port->uartclk = BASE_BAUD * 16;
 	}
 
 	if (port->iotype == UPIO_MEM || port->iotype == UPIO_MEM16 ||
@@ -122,7 +129,6 @@ static int __init register_earlycon(char *buf, const struct earlycon_id *match)
 	if (buf && !parse_options(&early_console_dev, buf))
 		buf = NULL;
 
-	port->uartclk = BASE_BAUD * 16;
 	if (port->mapbase)
 		port->membase = earlycon_map(port->mapbase, 64);
 
-- 
1.9.1

Re: [PATCH v2 2/2] serial: earlycon: allow to specify uartclk in earlycon kernel-parameter

[Peter Hurley]

On 10/19/2015 11:36 PM, Masahiro Yamada wrote:
> The input clock frequency varies from device to device, but the
> earlycon uses the fixed frequency (BASE_BAUD * 16).  It makes
> impossible to set the correct divisor to the register.

So the bootloader hasn't setup the serial port?


> This commit allows to specify the input clock frequency from the
> kernel-parameter.
> 
> [Example]
> 
>     earlycon=uart8250,mmio32,0x43fb0000,115200,12288000
> 
> The input clock frequency (12288000, in this case) should be specified
> after the baudrate.  If not specified, the default (BASE_BAUD * 16) is
> used.
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
> 
> Changes in v2: None
> 
>  drivers/tty/serial/earlycon.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c
> index 07f7393..8030765 100644
> --- a/drivers/tty/serial/earlycon.c
> +++ b/drivers/tty/serial/earlycon.c
> @@ -95,6 +95,13 @@ static int __init parse_options(struct earlycon_device *device, char *options)
>  		length = min(strcspn(options, " ") + 1,
>  			     (size_t)(sizeof(device->options)));
>  		strlcpy(device->options, options, length);
> +		options = strchr(options, ',');
> +		if (options) {
> +			options++;
> +			port->uartclk = simple_strtoul(options, NULL, 0);
> +		}
> +		if (!port->uartclk)
> +			port->uartclk = BASE_BAUD * 16;
>  	}
>  
>  	if (port->iotype == UPIO_MEM || port->iotype == UPIO_MEM16 ||
> @@ -122,7 +129,6 @@ static int __init register_earlycon(char *buf, const struct earlycon_id *match)
>  	if (buf && !parse_options(&early_console_dev, buf))
>  		buf = NULL;
>  
> -	port->uartclk = BASE_BAUD * 16;
>  	if (port->mapbase)
>  		port->membase = earlycon_map(port->mapbase, 64);
>  
> 

Re: [PATCH v2 2/2] serial: earlycon: allow to specify uartclk in earlycon kernel-parameter

[Masahiro Yamada]

Hi Peter,
(+ Rob Herring, Stefan Agner)

2015-10-20 23:00 GMT+09:00 Peter Hurley <peter@hurleysoftware.com>:
> On 10/19/2015 11:36 PM, Masahiro Yamada wrote:
>> The input clock frequency varies from device to device, but the
>> earlycon uses the fixed frequency (BASE_BAUD * 16).  It makes
>> impossible to set the correct divisor to the register.
>
> So the bootloader hasn't setup the serial port?

It does.
I use U-boot and the serial port is already set up by U-boot.


But, earlycon setup functions update hardware registers.
See  early_serial8250_setup(), ingenic_early_console_setup(), etc.


Without port->uartclk set to a valid value,
the init code in earlycon setup does not make sense.


What I want to clarify is,
what should we do in the earlycon setup function?

Currently, I see
 [1] set device->con->write callback
 [2] initialize UART port registers


For [2], we need to know baudrate and input clock frequency.
(and the latter is missing, that's why my patch is here.)


In order to be independent of a boot loader, we also need
  [3]  pinctrl (pin-muxing)

But, it is difficult to handle pinctrl in the earlycon framework.


If we depend on a boot loader, [2] is meaningless.   [1] is enough.




-- 
Best Regards
Masahiro Yamada

Re: [PATCH v2 2/2] serial: earlycon: allow to specify uartclk in earlycon kernel-parameter

[Peter Hurley]

On 10/20/2015 09:20 PM, Masahiro Yamada wrote:
> Hi Peter,
> (+ Rob Herring, Stefan Agner)
> 
> 2015-10-20 23:00 GMT+09:00 Peter Hurley <peter@hurleysoftware.com>:
>> On 10/19/2015 11:36 PM, Masahiro Yamada wrote:
>>> The input clock frequency varies from device to device, but the
>>> earlycon uses the fixed frequency (BASE_BAUD * 16).  It makes
>>> impossible to set the correct divisor to the register.
>>
>> So the bootloader hasn't setup the serial port?
> 
> It does.
> I use U-boot and the serial port is already set up by U-boot.
> 
> 
> But, earlycon setup functions update hardware registers.
> See  early_serial8250_setup(), ingenic_early_console_setup(), etc.
> 
> 
> Without port->uartclk set to a valid value,
> the init code in earlycon setup does not make sense.
> 
> 
> What I want to clarify is,
> what should we do in the earlycon setup function?
> 
> Currently, I see
>  [1] set device->con->write callback
>  [2] initialize UART port registers
> 
> 
> For [2], we need to know baudrate and input clock frequency.
> (and the latter is missing, that's why my patch is here.)
> 
> 
> In order to be independent of a boot loader, we also need
>   [3]  pinctrl (pin-muxing)
> 
> But, it is difficult to handle pinctrl in the earlycon framework.
> 
> 
> If we depend on a boot loader, [2] is meaningless.   [1] is enough.

The 8250 earlycon doesn't try to initialize the hardware (other than
masking interrupts) if the baud rate is uninitialized
(!device->baud in early_serial8250_setup()).

Rather than initializing the h/w to a default of 115200 in
ingenic_early_console_setup() when device->baud == 0, it should just
mask interrupts. That way the bootloader initialization will be
preserved when the options are unspecified on the command line, such as:

	earlycon=jz4740_uart,mmio32,0x43fb0000


Regards,
Peter Hurley

PS - Apologies for not reviewing the Ingenic 8250 driver at submission
time.

Re: [PATCH v2 2/2] serial: earlycon: allow to specify uartclk in earlycon kernel-parameter

[Masahiro Yamada]

Hi Peter.


2015-10-21 21:27 GMT+09:00 Peter Hurley <peter@hurleysoftware.com>:
> On 10/20/2015 09:20 PM, Masahiro Yamada wrote:
>> Hi Peter,
>> (+ Rob Herring, Stefan Agner)
>>
>> 2015-10-20 23:00 GMT+09:00 Peter Hurley <peter@hurleysoftware.com>:
>>> On 10/19/2015 11:36 PM, Masahiro Yamada wrote:
>>>> The input clock frequency varies from device to device, but the
>>>> earlycon uses the fixed frequency (BASE_BAUD * 16).  It makes
>>>> impossible to set the correct divisor to the register.
>>>
>>> So the bootloader hasn't setup the serial port?
>>
>> It does.
>> I use U-boot and the serial port is already set up by U-boot.
>>
>>
>> But, earlycon setup functions update hardware registers.
>> See  early_serial8250_setup(), ingenic_early_console_setup(), etc.
>>
>>
>> Without port->uartclk set to a valid value,
>> the init code in earlycon setup does not make sense.
>>
>>
>> What I want to clarify is,
>> what should we do in the earlycon setup function?
>>
>> Currently, I see
>>  [1] set device->con->write callback
>>  [2] initialize UART port registers
>>
>>
>> For [2], we need to know baudrate and input clock frequency.
>> (and the latter is missing, that's why my patch is here.)
>>
>>
>> In order to be independent of a boot loader, we also need
>>   [3]  pinctrl (pin-muxing)
>>
>> But, it is difficult to handle pinctrl in the earlycon framework.
>>
>>
>> If we depend on a boot loader, [2] is meaningless.   [1] is enough.
>
> The 8250 earlycon doesn't try to initialize the hardware (other than
> masking interrupts) if the baud rate is uninitialized
> (!device->baud in early_serial8250_setup()).
>

Right.

If it is always correct to preserve the initialization done by boot-loader,
the following code in 8250_early.c does not make sense.

Delete?


        divisor = DIV_ROUND_CLOSEST(port->uartclk, 16 * device->baud);
        c = serial8250_early_in(port, UART_LCR);
        serial8250_early_out(port, UART_LCR, c | UART_LCR_DLAB);
        serial8250_early_out(port, UART_DLL, divisor & 0xff);
        serial8250_early_out(port, UART_DLM, (divisor >> 8) & 0xff);
        serial8250_early_out(port, UART_LCR, c & ~UART_LCR_DLAB);








-- 
Best Regards
Masahiro Yamada

Re: [PATCH v2 2/2] serial: earlycon: allow to specify uartclk in earlycon kernel-parameter

[Peter Hurley]

Hi Masahiro,

On 10/21/2015 11:31 AM, Masahiro Yamada wrote:
> If it is always correct to preserve the initialization done by boot-loader,
> the following code in 8250_early.c does not make sense.

It's not always correct to preserve the initialization by the bootloader.
For example, on my legacy x86 workstation, the bootloader does not
initialize the h/w so when I want 8250 earlycon, I must specify the
baud rate.

Regards,
Peter Hurley

Re: [PATCH v2 2/2] serial: earlycon: allow to specify uartclk in earlycon kernel-parameter

[Masahiro Yamada]

Hi Peter,

2015-10-22 0:35 GMT+09:00 Peter Hurley <peter@hurleysoftware.com>:
> Hi Masahiro,
>
> On 10/21/2015 11:31 AM, Masahiro Yamada wrote:
>> If it is always correct to preserve the initialization done by boot-loader,
>> the following code in 8250_early.c does not make sense.
>
> It's not always correct to preserve the initialization by the bootloader.
> For example, on my legacy x86 workstation, the bootloader does not
> initialize the h/w so when I want 8250 earlycon, I must specify the
> baud rate.

So, the input clock on the machine matches the following macro.

#define BASE_BAUD (1843200/16)


For ARM boards, we should depend on a boot loader.
Is this what you mean?

-- 
Best Regards
Masahiro Yamada

Re: [PATCH v2 2/2] serial: earlycon: allow to specify uartclk in earlycon kernel-parameter

[Rob Herring]

On Tue, Oct 20, 2015 at 8:20 PM, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> Hi Peter,
> (+ Rob Herring, Stefan Agner)
>
> 2015-10-20 23:00 GMT+09:00 Peter Hurley <peter@hurleysoftware.com>:
>> On 10/19/2015 11:36 PM, Masahiro Yamada wrote:
>>> The input clock frequency varies from device to device, but the
>>> earlycon uses the fixed frequency (BASE_BAUD * 16).  It makes
>>> impossible to set the correct divisor to the register.
>>
>> So the bootloader hasn't setup the serial port?
>
> It does.
> I use U-boot and the serial port is already set up by U-boot.

This problem is now moving into u-boot which is using DT for configuration.

> But, earlycon setup functions update hardware registers.
> See  early_serial8250_setup(), ingenic_early_console_setup(), etc.
>
>
> Without port->uartclk set to a valid value,
> the init code in earlycon setup does not make sense.
>
>
> What I want to clarify is,
> what should we do in the earlycon setup function?
>
> Currently, I see
>  [1] set device->con->write callback
>  [2] initialize UART port registers
>
>
> For [2], we need to know baudrate and input clock frequency.
> (and the latter is missing, that's why my patch is here.)

I'm missing context of what you did, but it needs to be parse-able
from a flattened tree. My suggestion would be use clock-frequency
property in the uart node. We should be able to parse that. This came
up with u-boot as well[1].


> In order to be independent of a boot loader, we also need
>   [3]  pinctrl (pin-muxing)

We have to draw the line somewhere and I think this falls below it.
What if we have to turn on a regulator for the pins or RS-232
transceiver, turn on power domain, take peripheral out of reset, and
the list goes on... Hopefully a debug path would never be that
complicated.

> But, it is difficult to handle pinctrl in the earlycon framework.

earlycon is for debug. Either fix your bootloader or you need some
platform specific setup hacks to get the h/w in a working state if you
need to debug.

Rob

[1] https://patchwork.ozlabs.org/patch/492697/

Re: [PATCH v2 2/2] serial: earlycon: allow to specify uartclk in earlycon kernel-parameter

[Masahiro Yamada]

2015-10-23 0:34 GMT+09:00 Rob Herring <robh@kernel.org>:
> On Tue, Oct 20, 2015 at 8:20 PM, Masahiro Yamada
> <yamada.masahiro@socionext.com> wrote:
>> Hi Peter,
>> (+ Rob Herring, Stefan Agner)
>>
>> 2015-10-20 23:00 GMT+09:00 Peter Hurley <peter@hurleysoftware.com>:
>>> On 10/19/2015 11:36 PM, Masahiro Yamada wrote:
>>>> The input clock frequency varies from device to device, but the
>>>> earlycon uses the fixed frequency (BASE_BAUD * 16).  It makes
>>>> impossible to set the correct divisor to the register.
>>>
>>> So the bootloader hasn't setup the serial port?
>>
>> It does.
>> I use U-boot and the serial port is already set up by U-boot.
>
> This problem is now moving into u-boot which is using DT for configuration.
>
>> But, earlycon setup functions update hardware registers.
>> See  early_serial8250_setup(), ingenic_early_console_setup(), etc.
>>
>>
>> Without port->uartclk set to a valid value,
>> the init code in earlycon setup does not make sense.
>>
>>
>> What I want to clarify is,
>> what should we do in the earlycon setup function?
>>
>> Currently, I see
>>  [1] set device->con->write callback
>>  [2] initialize UART port registers
>>
>>
>> For [2], we need to know baudrate and input clock frequency.
>> (and the latter is missing, that's why my patch is here.)
>
> I'm missing context of what you did, but it needs to be parse-able
> from a flattened tree. My suggestion would be use clock-frequency
> property in the uart node. We should be able to parse that. This came
> up with u-boot as well[1].

Right.

But, I think things have been going wrong
since SPL started to parse a device tree.




-- 
Best Regards
Masahiro Yamada