All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] earlycon: Allow specifying a uartclk in options
@ 2018-03-01 18:43 ` Daniel Kurtz
  0 siblings, 0 replies; 27+ messages in thread
From: Daniel Kurtz @ 2018-03-01 18:43 UTC (permalink / raw)
  Cc: adurbin, briannorris, Daniel Kurtz, Jonathan Corbet,
	Greg Kroah-Hartman, Jiri Slaby, Ingo Molnar, Thomas Gleixner,
	Christoffer Dall, Paul E. McKenney, Marc Zyngier,
	Frederic Weisbecker, David Woodhouse, Tom Saeger, Mimi Zohar,
	Levin, Alexander (Sasha Levin),
	open list:DOCUMENTATION, open list, open list:SERIAL DRIVERS

Currently when an earlycon is registered, the uartclk is assumed to be
BASE_BAUD * 16 = 1843200.  If a baud rate is specified in the earlycon
options, then 8250_early's init_port will program the UART clock divider
registers based on this assumed uartclk.

However, not all uarts have a UART clock of 1843200.  For example, the
8250_dw uart in AMD's CZ/ST uses a fixed 48 MHz clock (as specified in
cz_uart_desc in acpi_apd.c).  Thus, specifying a baud when using earlycon
on such a device will result in incorrect divider values and a wrong UART
clock.

Fix this by extending the earlycon options parameter to allow specification
of a uartclk, like so:

 earlycon=uart,mmio32,0xfedc6000,115200,48000000

If none is specified, fall-back to prior behavior - 1843200.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 Documentation/admin-guide/kernel-parameters.txt | 3 +++
 drivers/tty/serial/earlycon.c                   | 8 ++++++--
 include/linux/serial_core.h                     | 2 +-
 3 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 1d1d53f85ddd..20e72cada38e 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -958,6 +958,9 @@
 			to be equivalent to 'mmio'. 'options' are specified
 			in the same format described for "console=ttyS<n>"; if
 			unspecified, the h/w is not initialized.
+			A UART clock rate can also be appended after the baud,
+			as in <options> = 115200n8,48000000; if unspecified,
+			the default 1843200 will be used.
 
 		pl011,<addr>
 		pl011,mmio32,<addr>
diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c
index 870e84fb6e39..e846f406a1c6 100644
--- a/drivers/tty/serial/earlycon.c
+++ b/drivers/tty/serial/earlycon.c
@@ -115,12 +115,17 @@ static int __init parse_options(struct earlycon_device *device, char *options)
 	}
 
 	if (options) {
-		device->baud = simple_strtoul(options, NULL, 0);
+		char *uartclk;
+		device->baud = simple_strtoul(options, &uartclk, 0);
+		if (*uartclk++ == ',')
+			port->uartclk = simple_strtoul(uartclk, NULL, 0);
 		length = min(strcspn(options, " ") + 1,
 			     (size_t)(sizeof(device->options)));
 		strlcpy(device->options, options, length);
 	}
 
+	port->uartclk = (port->uartclk) ?: BASE_BAUD * 16;
+
 	return 0;
 }
 
@@ -134,7 +139,6 @@ static int __init register_earlycon(char *buf, const struct earlycon_id *match)
 		buf = NULL;
 
 	spin_lock_init(&port->lock);
-	port->uartclk = BASE_BAUD * 16;
 	if (port->mapbase)
 		port->membase = earlycon_map(port->mapbase, 64);
 
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index b32df49a3bd5..b772f0d20b18 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -343,7 +343,7 @@ static inline int uart_poll_timeout(struct uart_port *port)
 struct earlycon_device {
 	struct console *con;
 	struct uart_port port;
-	char options[16];		/* e.g., 115200n8 */
+	char options[32];		/* e.g., 115200n8,48000000 */
 	unsigned int baud;
 };
 
-- 
2.16.2.395.g2e18187dfd-goog


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH v2] earlycon: Allow specifying a uartclk in options
@ 2018-03-01 18:43 ` Daniel Kurtz
  0 siblings, 0 replies; 27+ messages in thread
From: Daniel Kurtz @ 2018-03-01 18:43 UTC (permalink / raw)
  Cc: adurbin, briannorris, Daniel Kurtz, Jonathan Corbet,
	Greg Kroah-Hartman, Jiri Slaby, Ingo Molnar, Thomas Gleixner,
	Christoffer Dall, Paul E. McKenney, Marc Zyngier,
	Frederic Weisbecker, David Woodhouse, Tom Saeger, Mimi Zohar,
	Levin, Alexander (Sasha Levin),
	open list:DOCUMENTATION, open list, open list:SERIAL DRIVERS

Currently when an earlycon is registered, the uartclk is assumed to be
BASE_BAUD * 16 = 1843200.  If a baud rate is specified in the earlycon
options, then 8250_early's init_port will program the UART clock divider
registers based on this assumed uartclk.

However, not all uarts have a UART clock of 1843200.  For example, the
8250_dw uart in AMD's CZ/ST uses a fixed 48 MHz clock (as specified in
cz_uart_desc in acpi_apd.c).  Thus, specifying a baud when using earlycon
on such a device will result in incorrect divider values and a wrong UART
clock.

Fix this by extending the earlycon options parameter to allow specification
of a uartclk, like so:

 earlycon=uart,mmio32,0xfedc6000,115200,48000000

If none is specified, fall-back to prior behavior - 1843200.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 Documentation/admin-guide/kernel-parameters.txt | 3 +++
 drivers/tty/serial/earlycon.c                   | 8 ++++++--
 include/linux/serial_core.h                     | 2 +-
 3 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 1d1d53f85ddd..20e72cada38e 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -958,6 +958,9 @@
 			to be equivalent to 'mmio'. 'options' are specified
 			in the same format described for "console=ttyS<n>"; if
 			unspecified, the h/w is not initialized.
+			A UART clock rate can also be appended after the baud,
+			as in <options> = 115200n8,48000000; if unspecified,
+			the default 1843200 will be used.
 
 		pl011,<addr>
 		pl011,mmio32,<addr>
diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c
index 870e84fb6e39..e846f406a1c6 100644
--- a/drivers/tty/serial/earlycon.c
+++ b/drivers/tty/serial/earlycon.c
@@ -115,12 +115,17 @@ static int __init parse_options(struct earlycon_device *device, char *options)
 	}
 
 	if (options) {
-		device->baud = simple_strtoul(options, NULL, 0);
+		char *uartclk;
+		device->baud = simple_strtoul(options, &uartclk, 0);
+		if (*uartclk++ == ',')
+			port->uartclk = simple_strtoul(uartclk, NULL, 0);
 		length = min(strcspn(options, " ") + 1,
 			     (size_t)(sizeof(device->options)));
 		strlcpy(device->options, options, length);
 	}
 
+	port->uartclk = (port->uartclk) ?: BASE_BAUD * 16;
+
 	return 0;
 }
 
@@ -134,7 +139,6 @@ static int __init register_earlycon(char *buf, const struct earlycon_id *match)
 		buf = NULL;
 
 	spin_lock_init(&port->lock);
-	port->uartclk = BASE_BAUD * 16;
 	if (port->mapbase)
 		port->membase = earlycon_map(port->mapbase, 64);
 
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index b32df49a3bd5..b772f0d20b18 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -343,7 +343,7 @@ static inline int uart_poll_timeout(struct uart_port *port)
 struct earlycon_device {
 	struct console *con;
 	struct uart_port port;
-	char options[16];		/* e.g., 115200n8 */
+	char options[32];		/* e.g., 115200n8,48000000 */
 	unsigned int baud;
 };
 
-- 
2.16.2.395.g2e18187dfd-goog

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH v2] earlycon: Allow specifying a uartclk in options
@ 2018-03-01 18:43 ` Daniel Kurtz
  0 siblings, 0 replies; 27+ messages in thread
From: Daniel Kurtz @ 2018-03-01 18:43 UTC (permalink / raw)
  Cc: adurbin, briannorris, Daniel Kurtz, Jonathan Corbet,
	Greg Kroah-Hartman, Jiri Slaby, Ingo Molnar, Thomas Gleixner,
	Christoffer Dall, Paul E. McKenney, Marc Zyngier,
	Frederic Weisbecker, David Woodhouse, Tom Saeger, Mimi Zohar,
	Levin, Alexander (Sasha Levin),
	open list:DOCUMENTATION, open list, open list:SERIAL DRIVERS

Currently when an earlycon is registered, the uartclk is assumed to be
BASE_BAUD * 16 = 1843200.  If a baud rate is specified in the earlycon
options, then 8250_early's init_port will program the UART clock divider
registers based on this assumed uartclk.

However, not all uarts have a UART clock of 1843200.  For example, the
8250_dw uart in AMD's CZ/ST uses a fixed 48 MHz clock (as specified in
cz_uart_desc in acpi_apd.c).  Thus, specifying a baud when using earlycon
on such a device will result in incorrect divider values and a wrong UART
clock.

Fix this by extending the earlycon options parameter to allow specification
of a uartclk, like so:

 earlycon=uart,mmio32,0xfedc6000,115200,48000000

If none is specified, fall-back to prior behavior - 1843200.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 Documentation/admin-guide/kernel-parameters.txt | 3 +++
 drivers/tty/serial/earlycon.c                   | 8 ++++++--
 include/linux/serial_core.h                     | 2 +-
 3 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 1d1d53f85ddd..20e72cada38e 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -958,6 +958,9 @@
 			to be equivalent to 'mmio'. 'options' are specified
 			in the same format described for "console=ttyS<n>"; if
 			unspecified, the h/w is not initialized.
+			A UART clock rate can also be appended after the baud,
+			as in <options> = 115200n8,48000000; if unspecified,
+			the default 1843200 will be used.
 
 		pl011,<addr>
 		pl011,mmio32,<addr>
diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c
index 870e84fb6e39..e846f406a1c6 100644
--- a/drivers/tty/serial/earlycon.c
+++ b/drivers/tty/serial/earlycon.c
@@ -115,12 +115,17 @@ static int __init parse_options(struct earlycon_device *device, char *options)
 	}
 
 	if (options) {
-		device->baud = simple_strtoul(options, NULL, 0);
+		char *uartclk;
+		device->baud = simple_strtoul(options, &uartclk, 0);
+		if (*uartclk++ == ',')
+			port->uartclk = simple_strtoul(uartclk, NULL, 0);
 		length = min(strcspn(options, " ") + 1,
 			     (size_t)(sizeof(device->options)));
 		strlcpy(device->options, options, length);
 	}
 
+	port->uartclk = (port->uartclk) ?: BASE_BAUD * 16;
+
 	return 0;
 }
 
@@ -134,7 +139,6 @@ static int __init register_earlycon(char *buf, const struct earlycon_id *match)
 		buf = NULL;
 
 	spin_lock_init(&port->lock);
-	port->uartclk = BASE_BAUD * 16;
 	if (port->mapbase)
 		port->membase = earlycon_map(port->mapbase, 64);
 
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index b32df49a3bd5..b772f0d20b18 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -343,7 +343,7 @@ static inline int uart_poll_timeout(struct uart_port *port)
 struct earlycon_device {
 	struct console *con;
 	struct uart_port port;
-	char options[16];		/* e.g., 115200n8 */
+	char options[32];		/* e.g., 115200n8,48000000 */
 	unsigned int baud;
 };
 
-- 
2.16.2.395.g2e18187dfd-goog

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* Re: [PATCH v2] earlycon: Allow specifying a uartclk in options
  2018-03-01 18:43 ` Daniel Kurtz
@ 2018-03-01 18:47   ` Andy Shevchenko
  -1 siblings, 0 replies; 27+ messages in thread
From: Andy Shevchenko @ 2018-03-01 18:47 UTC (permalink / raw)
  To: Daniel Kurtz
  Cc: adurbin, Brian Norris, Jonathan Corbet, Greg Kroah-Hartman,
	Jiri Slaby, Ingo Molnar, Thomas Gleixner, Christoffer Dall,
	Paul E. McKenney, Marc Zyngier, Frederic Weisbecker,
	David Woodhouse, Tom Saeger, Mimi Zohar, Levin,
	Alexander (Sasha Levin),
	open list:DOCUMENTATION, open list, open list:SERIAL DRIVERS

On Thu, Mar 1, 2018 at 8:43 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:

Please, hold on with new versions.
I'm not satisfied (yet?) by the approach.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2] earlycon: Allow specifying a uartclk in options
@ 2018-03-01 18:47   ` Andy Shevchenko
  0 siblings, 0 replies; 27+ messages in thread
From: Andy Shevchenko @ 2018-03-01 18:47 UTC (permalink / raw)
  To: Daniel Kurtz
  Cc: adurbin, Brian Norris, Jonathan Corbet, Greg Kroah-Hartman,
	Jiri Slaby, Ingo Molnar, Thomas Gleixner, Christoffer Dall,
	Paul E. McKenney, Marc Zyngier, Frederic Weisbecker,
	David Woodhouse, Tom Saeger, Mimi Zohar, Levin,
	Alexander (Sasha Levin),
	open list:DOCUMENTATION, open list, open list:SERIAL DRIVERS

On Thu, Mar 1, 2018 at 8:43 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:

Please, hold on with new versions.
I'm not satisfied (yet?) by the approach.

-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2] earlycon: Allow specifying a uartclk in options
  2018-03-01 18:47   ` Andy Shevchenko
@ 2018-03-01 19:22     ` Daniel Kurtz
  -1 siblings, 0 replies; 27+ messages in thread
From: Daniel Kurtz @ 2018-03-01 19:22 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: adurbin, Brian Norris, corbet, Greg Kroah-Hartman, jslaby, mingo,
	Thomas Gleixner, cdall, paulmck, marc.zyngier, frederic, dwmw,
	tom.saeger, zohar, alexander.levin, linux-doc, linux-kernel,
	linux-serial

On Thu, Mar 1, 2018 at 11:47 AM Andy Shevchenko <andy.shevchenko@gmail.com>
wrote:

> On Thu, Mar 1, 2018 at 8:43 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:

> Please, hold on with new versions.
> I'm not satisfied (yet?) by the approach.

Copying over your comment on v1:

> It needs to be discussed.

Sure.

> First of all, if you are going to do this you need to add a parse of
> human readable formats (IIRC kernel has helpers), i.e. "48M", "38.4M"
> and so on.

> Next, I was under impression that purpose of earlycon (in difference
> to earlyprintk) is to re-use existing drivers as fully as possible.

> So, what exactly happens in your case? Are your driver lacks of
> properly set clock? Or earlycon does simple not utilizing this
> information?

"earlycon simply does not utilize the information".

earlycon parses iotype, mapbase and baud (from options).  However, it is
hard-coded to assume that the clock used to generate the UART bitclock is
always "BASE_BAUD * 16" (1843200).  While this may be true for many UARTs,
it isn't true for AMD's CZ/ST which has a 8250_dw and uses a fixed 48 MHz
clock.  The main 8250_dw driver uses devm_clk_get to get the "baudclk" and
uses its rate to initialize uartclk.  For AMD CZ/ST, this "baudclk" is
actually a set up in acpi_apd.c when there is an acpi match for "AMD0020",
with a rate read from the .fixed_clk_rate param of the corresponding
apd_device_desc.

This patch attempts to add a way to inform earlycon about this clock.  As
noted above, the information is actually already in the kernel and used by
8250_dw - I would happy be to hear recommendations for wiring this data
into earlycon that doesn't require adding another command line arg.

I see that support was also added recently to earlycon to let it use ACPI
SPCR to choose a console and configure its parameters... but AFAICT, this
path also doesn't allow specifying the uart clock.

-Dan

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2] earlycon: Allow specifying a uartclk in options
@ 2018-03-01 19:22     ` Daniel Kurtz
  0 siblings, 0 replies; 27+ messages in thread
From: Daniel Kurtz @ 2018-03-01 19:22 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: adurbin, Brian Norris, corbet, Greg Kroah-Hartman, jslaby, mingo,
	Thomas Gleixner, cdall, paulmck, marc.zyngier, frederic, dwmw,
	tom.saeger, zohar, alexander.levin, linux-doc, linux-kernel,
	linux-serial

On Thu, Mar 1, 2018 at 11:47 AM Andy Shevchenko <andy.shevchenko@gmail.com>
wrote:

> On Thu, Mar 1, 2018 at 8:43 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:

> Please, hold on with new versions.
> I'm not satisfied (yet?) by the approach.

Copying over your comment on v1:

> It needs to be discussed.

Sure.

> First of all, if you are going to do this you need to add a parse of
> human readable formats (IIRC kernel has helpers), i.e. "48M", "38.4M"
> and so on.

> Next, I was under impression that purpose of earlycon (in difference
> to earlyprintk) is to re-use existing drivers as fully as possible.

> So, what exactly happens in your case? Are your driver lacks of
> properly set clock? Or earlycon does simple not utilizing this
> information?

"earlycon simply does not utilize the information".

earlycon parses iotype, mapbase and baud (from options).  However, it is
hard-coded to assume that the clock used to generate the UART bitclock is
always "BASE_BAUD * 16" (1843200).  While this may be true for many UARTs,
it isn't true for AMD's CZ/ST which has a 8250_dw and uses a fixed 48 MHz
clock.  The main 8250_dw driver uses devm_clk_get to get the "baudclk" and
uses its rate to initialize uartclk.  For AMD CZ/ST, this "baudclk" is
actually a set up in acpi_apd.c when there is an acpi match for "AMD0020",
with a rate read from the .fixed_clk_rate param of the corresponding
apd_device_desc.

This patch attempts to add a way to inform earlycon about this clock.  As
noted above, the information is actually already in the kernel and used by
8250_dw - I would happy be to hear recommendations for wiring this data
into earlycon that doesn't require adding another command line arg.

I see that support was also added recently to earlycon to let it use ACPI
SPCR to choose a console and configure its parameters... but AFAICT, this
path also doesn't allow specifying the uart clock.

-Dan
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2] earlycon: Allow specifying a uartclk in options
  2018-03-01 19:22     ` Daniel Kurtz
@ 2018-03-01 20:02       ` Andy Shevchenko
  -1 siblings, 0 replies; 27+ messages in thread
From: Andy Shevchenko @ 2018-03-01 20:02 UTC (permalink / raw)
  To: Daniel Kurtz
  Cc: adurbin, Brian Norris, Jonathan Corbet, Greg Kroah-Hartman,
	Jiri Slaby, Ingo Molnar, Thomas Gleixner, Christoffer Dall,
	Paul E. McKenney, Marc Zyngier, Frederic Weisbecker,
	David Woodhouse, Tom Saeger, Mimi Zohar, Levin,
	Alexander (Sasha Levin),
	Linux Documentation List, Linux Kernel Mailing List,
	open list:SERIAL DRIVERS

On Thu, Mar 1, 2018 at 9:22 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:
> On Thu, Mar 1, 2018 at 11:47 AM Andy Shevchenko <andy.shevchenko@gmail.com>
> wrote:

> "earlycon simply does not utilize the information".
>
> earlycon parses iotype, mapbase and baud (from options).  However, it is
> hard-coded to assume that the clock used to generate the UART bitclock is
> always "BASE_BAUD * 16" (1843200).  While this may be true for many UARTs,
> it isn't true for AMD's CZ/ST which has a 8250_dw and uses a fixed 48 MHz
> clock.  The main 8250_dw driver uses devm_clk_get to get the "baudclk" and
> uses its rate to initialize uartclk.  For AMD CZ/ST, this "baudclk" is
> actually a set up in acpi_apd.c when there is an acpi match for "AMD0020",
> with a rate read from the .fixed_clk_rate param of the corresponding
> apd_device_desc.
>
> This patch attempts to add a way to inform earlycon about this clock.  As
> noted above, the information is actually already in the kernel and used by
> 8250_dw - I would happy be to hear recommendations for wiring this data
> into earlycon that doesn't require adding another command line arg.

And it should not require that for sure!

I would look to this later. It's late here. I need to do a bit of
research for the answer.

> I see that support was also added recently to earlycon to let it use ACPI
> SPCR to choose a console and configure its parameters... but AFAICT, this
> path also doesn't allow specifying the uart clock.

Fix your firmware then. It should set console to 115200 like (almost)
everyone does.
Okay, configures a necessary IPs to feed UART with expected 1.8432M clock.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2] earlycon: Allow specifying a uartclk in options
@ 2018-03-01 20:02       ` Andy Shevchenko
  0 siblings, 0 replies; 27+ messages in thread
From: Andy Shevchenko @ 2018-03-01 20:02 UTC (permalink / raw)
  To: Daniel Kurtz
  Cc: adurbin, Brian Norris, Jonathan Corbet, Greg Kroah-Hartman,
	Jiri Slaby, Ingo Molnar, Thomas Gleixner, Christoffer Dall,
	Paul E. McKenney, Marc Zyngier, Frederic Weisbecker,
	David Woodhouse, Tom Saeger, Mimi Zohar, Levin,
	Alexander (Sasha Levin),
	Linux Documentation List, Linux Kernel Mailing List,
	open list:SERIAL DRIVERS

On Thu, Mar 1, 2018 at 9:22 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:
> On Thu, Mar 1, 2018 at 11:47 AM Andy Shevchenko <andy.shevchenko@gmail.com>
> wrote:

> "earlycon simply does not utilize the information".
>
> earlycon parses iotype, mapbase and baud (from options).  However, it is
> hard-coded to assume that the clock used to generate the UART bitclock is
> always "BASE_BAUD * 16" (1843200).  While this may be true for many UARTs,
> it isn't true for AMD's CZ/ST which has a 8250_dw and uses a fixed 48 MHz
> clock.  The main 8250_dw driver uses devm_clk_get to get the "baudclk" and
> uses its rate to initialize uartclk.  For AMD CZ/ST, this "baudclk" is
> actually a set up in acpi_apd.c when there is an acpi match for "AMD0020",
> with a rate read from the .fixed_clk_rate param of the corresponding
> apd_device_desc.
>
> This patch attempts to add a way to inform earlycon about this clock.  As
> noted above, the information is actually already in the kernel and used by
> 8250_dw - I would happy be to hear recommendations for wiring this data
> into earlycon that doesn't require adding another command line arg.

And it should not require that for sure!

I would look to this later. It's late here. I need to do a bit of
research for the answer.

> I see that support was also added recently to earlycon to let it use ACPI
> SPCR to choose a console and configure its parameters... but AFAICT, this
> path also doesn't allow specifying the uart clock.

Fix your firmware then. It should set console to 115200 like (almost)
everyone does.
Okay, configures a necessary IPs to feed UART with expected 1.8432M clock.

-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2] earlycon: Allow specifying a uartclk in options
  2018-03-01 20:02       ` Andy Shevchenko
  (?)
@ 2018-03-01 21:24         ` Aaron Durbin
  -1 siblings, 0 replies; 27+ messages in thread
From: Aaron Durbin @ 2018-03-01 21:24 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Daniel Kurtz, Aaron Durbin, Brian Norris, Jonathan Corbet,
	Greg Kroah-Hartman, Jiri Slaby, Ingo Molnar, Thomas Gleixner,
	Christoffer Dall, Paul E. McKenney, Marc Zyngier,
	Frederic Weisbecker, David Woodhouse, Tom Saeger, Mimi Zohar,
	Levin, Alexander (Sasha Levin),
	Linux Documentation List, Linux Kernel Mailing List,
	open list:SERIAL DRIVERS

On Thu, Mar 1, 2018 at 1:02 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Thu, Mar 1, 2018 at 9:22 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:
>> On Thu, Mar 1, 2018 at 11:47 AM Andy Shevchenko <andy.shevchenko@gmail.com>
>> wrote:
>
>> "earlycon simply does not utilize the information".
>>
>> earlycon parses iotype, mapbase and baud (from options).  However, it is
>> hard-coded to assume that the clock used to generate the UART bitclock is
>> always "BASE_BAUD * 16" (1843200).  While this may be true for many UARTs,
>> it isn't true for AMD's CZ/ST which has a 8250_dw and uses a fixed 48 MHz
>> clock.  The main 8250_dw driver uses devm_clk_get to get the "baudclk" and
>> uses its rate to initialize uartclk.  For AMD CZ/ST, this "baudclk" is
>> actually a set up in acpi_apd.c when there is an acpi match for "AMD0020",
>> with a rate read from the .fixed_clk_rate param of the corresponding
>> apd_device_desc.
>>
>> This patch attempts to add a way to inform earlycon about this clock.  As
>> noted above, the information is actually already in the kernel and used by
>> 8250_dw - I would happy be to hear recommendations for wiring this data
>> into earlycon that doesn't require adding another command line arg.
>
> And it should not require that for sure!

But it does require that. There's an input clock to the uart ip block.
That is a design constraint by the hardware and is required to make
baud calculation work.

>
> I would look to this later. It's late here. I need to do a bit of
> research for the answer.
>
>> I see that support was also added recently to earlycon to let it use ACPI
>> SPCR to choose a console and configure its parameters... but AFAICT, this
>> path also doesn't allow specifying the uart clock.
>
> Fix your firmware then. It should set console to 115200 like (almost)
> everyone does.

It's not a firmware problem. Its the driver's problem in that it
assumes an input clock to the uart block that does not reflect
reality.

> Okay, configures a necessary IPs to feed UART with expected 1.8432M clock.

That's only possible if there is a clock divider on the front end of
the uart block. For this hardware that's not the case. I actually did
this very thing on intel chromebook devices, but it was only possible
because there was a hardware divider that could be tuned to reach the
assumed clock that the code currently assumes.

>
> --
> With Best Regards,
> Andy Shevchenko

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2] earlycon: Allow specifying a uartclk in options
@ 2018-03-01 21:24         ` Aaron Durbin
  0 siblings, 0 replies; 27+ messages in thread
From: Aaron Durbin @ 2018-03-01 21:24 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Daniel Kurtz, Aaron Durbin, Brian Norris, Jonathan Corbet,
	Greg Kroah-Hartman, Jiri Slaby, Ingo Molnar, Thomas Gleixner,
	Christoffer Dall, Paul E. McKenney, Marc Zyngier,
	Frederic Weisbecker, David Woodhouse, Tom Saeger, Mimi Zohar,
	Levin, Alexander (Sasha Levin),
	Linux Documentation List, Linux Kernel Mailing List,
	open list:SERIAL DRIVERS

On Thu, Mar 1, 2018 at 1:02 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Thu, Mar 1, 2018 at 9:22 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:
>> On Thu, Mar 1, 2018 at 11:47 AM Andy Shevchenko <andy.shevchenko@gmail.com>
>> wrote:
>
>> "earlycon simply does not utilize the information".
>>
>> earlycon parses iotype, mapbase and baud (from options).  However, it is
>> hard-coded to assume that the clock used to generate the UART bitclock is
>> always "BASE_BAUD * 16" (1843200).  While this may be true for many UARTs,
>> it isn't true for AMD's CZ/ST which has a 8250_dw and uses a fixed 48 MHz
>> clock.  The main 8250_dw driver uses devm_clk_get to get the "baudclk" and
>> uses its rate to initialize uartclk.  For AMD CZ/ST, this "baudclk" is
>> actually a set up in acpi_apd.c when there is an acpi match for "AMD0020",
>> with a rate read from the .fixed_clk_rate param of the corresponding
>> apd_device_desc.
>>
>> This patch attempts to add a way to inform earlycon about this clock.  As
>> noted above, the information is actually already in the kernel and used by
>> 8250_dw - I would happy be to hear recommendations for wiring this data
>> into earlycon that doesn't require adding another command line arg.
>
> And it should not require that for sure!

But it does require that. There's an input clock to the uart ip block.
That is a design constraint by the hardware and is required to make
baud calculation work.

>
> I would look to this later. It's late here. I need to do a bit of
> research for the answer.
>
>> I see that support was also added recently to earlycon to let it use ACPI
>> SPCR to choose a console and configure its parameters... but AFAICT, this
>> path also doesn't allow specifying the uart clock.
>
> Fix your firmware then. It should set console to 115200 like (almost)
> everyone does.

It's not a firmware problem. Its the driver's problem in that it
assumes an input clock to the uart block that does not reflect
reality.

> Okay, configures a necessary IPs to feed UART with expected 1.8432M clock.

That's only possible if there is a clock divider on the front end of
the uart block. For this hardware that's not the case. I actually did
this very thing on intel chromebook devices, but it was only possible
because there was a hardware divider that could be tuned to reach the
assumed clock that the code currently assumes.

>
> --
> With Best Regards,
> Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2] earlycon: Allow specifying a uartclk in options
@ 2018-03-01 21:24         ` Aaron Durbin
  0 siblings, 0 replies; 27+ messages in thread
From: Aaron Durbin @ 2018-03-01 21:24 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Daniel Kurtz, Aaron Durbin, Brian Norris, Jonathan Corbet,
	Greg Kroah-Hartman, Jiri Slaby, Ingo Molnar, Thomas Gleixner,
	Christoffer Dall, Paul E. McKenney, Marc Zyngier,
	Frederic Weisbecker, David Woodhouse, Tom Saeger, Mimi Zohar,
	Levin, Alexander (Sasha Levin),
	Linux Documentation List, Linux Kernel Mailing List, open

On Thu, Mar 1, 2018 at 1:02 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Thu, Mar 1, 2018 at 9:22 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:
>> On Thu, Mar 1, 2018 at 11:47 AM Andy Shevchenko <andy.shevchenko@gmail.com>
>> wrote:
>
>> "earlycon simply does not utilize the information".
>>
>> earlycon parses iotype, mapbase and baud (from options).  However, it is
>> hard-coded to assume that the clock used to generate the UART bitclock is
>> always "BASE_BAUD * 16" (1843200).  While this may be true for many UARTs,
>> it isn't true for AMD's CZ/ST which has a 8250_dw and uses a fixed 48 MHz
>> clock.  The main 8250_dw driver uses devm_clk_get to get the "baudclk" and
>> uses its rate to initialize uartclk.  For AMD CZ/ST, this "baudclk" is
>> actually a set up in acpi_apd.c when there is an acpi match for "AMD0020",
>> with a rate read from the .fixed_clk_rate param of the corresponding
>> apd_device_desc.
>>
>> This patch attempts to add a way to inform earlycon about this clock.  As
>> noted above, the information is actually already in the kernel and used by
>> 8250_dw - I would happy be to hear recommendations for wiring this data
>> into earlycon that doesn't require adding another command line arg.
>
> And it should not require that for sure!

But it does require that. There's an input clock to the uart ip block.
That is a design constraint by the hardware and is required to make
baud calculation work.

>
> I would look to this later. It's late here. I need to do a bit of
> research for the answer.
>
>> I see that support was also added recently to earlycon to let it use ACPI
>> SPCR to choose a console and configure its parameters... but AFAICT, this
>> path also doesn't allow specifying the uart clock.
>
> Fix your firmware then. It should set console to 115200 like (almost)
> everyone does.

It's not a firmware problem. Its the driver's problem in that it
assumes an input clock to the uart block that does not reflect
reality.

> Okay, configures a necessary IPs to feed UART with expected 1.8432M clock.

That's only possible if there is a clock divider on the front end of
the uart block. For this hardware that's not the case. I actually did
this very thing on intel chromebook devices, but it was only possible
because there was a hardware divider that could be tuned to reach the
assumed clock that the code currently assumes.

>
> --
> With Best Regards,
> Andy Shevchenko

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2] earlycon: Allow specifying a uartclk in options
  2018-03-01 20:02       ` Andy Shevchenko
@ 2018-03-02 18:35         ` Daniel Kurtz
  -1 siblings, 0 replies; 27+ messages in thread
From: Daniel Kurtz @ 2018-03-02 18:35 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: adurbin, Brian Norris, Jonathan Corbet, Greg Kroah-Hartman,
	jslaby, mingo, Thomas Gleixner, Christoffer Dall, Paul McKenney,
	marc.zyngier, frederic, dwmw, tom.saeger, zohar, alexander.levin,
	linux-doc, linux-kernel, linux-serial

On Thu, Mar 1, 2018 at 1:02 PM Andy Shevchenko <andy.shevchenko@gmail.com>
wrote:

> On Thu, Mar 1, 2018 at 9:22 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:
> > On Thu, Mar 1, 2018 at 11:47 AM Andy Shevchenko <
andy.shevchenko@gmail.com>
> > wrote:

> > "earlycon simply does not utilize the information".
> >
> > earlycon parses iotype, mapbase and baud (from options).  However, it is
> > hard-coded to assume that the clock used to generate the UART bitclock
is
> > always "BASE_BAUD * 16" (1843200).  While this may be true for many
UARTs,
> > it isn't true for AMD's CZ/ST which has a 8250_dw and uses a fixed 48
MHz
> > clock.  The main 8250_dw driver uses devm_clk_get to get the "baudclk"
and
> > uses its rate to initialize uartclk.  For AMD CZ/ST, this "baudclk" is
> > actually a set up in acpi_apd.c when there is an acpi match for
"AMD0020",
> > with a rate read from the .fixed_clk_rate param of the corresponding
> > apd_device_desc.
> >
> > This patch attempts to add a way to inform earlycon about this clock.
As
> > noted above, the information is actually already in the kernel and used
by
> > 8250_dw - I would happy be to hear recommendations for wiring this data
> > into earlycon that doesn't require adding another command line arg.

> And it should not require that for sure!

> I would look to this later. It's late here. I need to do a bit of
> research for the answer.

> > I see that support was also added recently to earlycon to let it use
ACPI
> > SPCR to choose a console and configure its parameters... but AFAICT,
this
> > path also doesn't allow specifying the uart clock.

> Fix your firmware then. It should set console to 115200 like (almost)
> everyone does.
> Okay, configures a necessary IPs to feed UART with expected 1.8432M clock.

The console is 115200 when it is enabled.  However, the firmware does not
always enable it by default.
The problem is that the UART IP block has a fixed 48 MHz input clock, but
earlycon assumes this clock is always 1843200.

I looked a bit further, and I think this patch (or something similar) is
still required to teach generic earlycon how to handle an explicit
port->uartclk (ie, one that is not 1843200).
The extended string can then be explicitly set on the kernel command line
for this kind of hardware.

In addition, we can add another patch with a new quirk detector in
drivers/acpi/spcr.c:acpi_parse_spcr() to handle this hardware.
acpi_parse_spcr() can then use the extended option string to pass in the
appropriate UART clock to setup_eralycon().

This would again allow a user to just use the simple command line parameter
"earlycon" if the device's firmware has a correctly confiured ACPI SPCR
table.

Thanks,
-Dan

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2] earlycon: Allow specifying a uartclk in options
@ 2018-03-02 18:35         ` Daniel Kurtz
  0 siblings, 0 replies; 27+ messages in thread
From: Daniel Kurtz @ 2018-03-02 18:35 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: adurbin, Brian Norris, Jonathan Corbet, Greg Kroah-Hartman,
	jslaby, mingo, Thomas Gleixner, Christoffer Dall, Paul McKenney,
	marc.zyngier, frederic, dwmw, tom.saeger, zohar, alexander.levin,
	linux-doc, linux-kernel, linux-serial

On Thu, Mar 1, 2018 at 1:02 PM Andy Shevchenko <andy.shevchenko@gmail.com>
wrote:

> On Thu, Mar 1, 2018 at 9:22 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:
> > On Thu, Mar 1, 2018 at 11:47 AM Andy Shevchenko <
andy.shevchenko@gmail.com>
> > wrote:

> > "earlycon simply does not utilize the information".
> >
> > earlycon parses iotype, mapbase and baud (from options).  However, it is
> > hard-coded to assume that the clock used to generate the UART bitclock
is
> > always "BASE_BAUD * 16" (1843200).  While this may be true for many
UARTs,
> > it isn't true for AMD's CZ/ST which has a 8250_dw and uses a fixed 48
MHz
> > clock.  The main 8250_dw driver uses devm_clk_get to get the "baudclk"
and
> > uses its rate to initialize uartclk.  For AMD CZ/ST, this "baudclk" is
> > actually a set up in acpi_apd.c when there is an acpi match for
"AMD0020",
> > with a rate read from the .fixed_clk_rate param of the corresponding
> > apd_device_desc.
> >
> > This patch attempts to add a way to inform earlycon about this clock.
As
> > noted above, the information is actually already in the kernel and used
by
> > 8250_dw - I would happy be to hear recommendations for wiring this data
> > into earlycon that doesn't require adding another command line arg.

> And it should not require that for sure!

> I would look to this later. It's late here. I need to do a bit of
> research for the answer.

> > I see that support was also added recently to earlycon to let it use
ACPI
> > SPCR to choose a console and configure its parameters... but AFAICT,
this
> > path also doesn't allow specifying the uart clock.

> Fix your firmware then. It should set console to 115200 like (almost)
> everyone does.
> Okay, configures a necessary IPs to feed UART with expected 1.8432M clock.

The console is 115200 when it is enabled.  However, the firmware does not
always enable it by default.
The problem is that the UART IP block has a fixed 48 MHz input clock, but
earlycon assumes this clock is always 1843200.

I looked a bit further, and I think this patch (or something similar) is
still required to teach generic earlycon how to handle an explicit
port->uartclk (ie, one that is not 1843200).
The extended string can then be explicitly set on the kernel command line
for this kind of hardware.

In addition, we can add another patch with a new quirk detector in
drivers/acpi/spcr.c:acpi_parse_spcr() to handle this hardware.
acpi_parse_spcr() can then use the extended option string to pass in the
appropriate UART clock to setup_eralycon().

This would again allow a user to just use the simple command line parameter
"earlycon" if the device's firmware has a correctly confiured ACPI SPCR
table.

Thanks,
-Dan
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2] earlycon: Allow specifying a uartclk in options
  2018-03-01 21:24         ` Aaron Durbin
  (?)
@ 2018-03-03 15:38           ` Andy Shevchenko
  -1 siblings, 0 replies; 27+ messages in thread
From: Andy Shevchenko @ 2018-03-03 15:38 UTC (permalink / raw)
  To: Aaron Durbin
  Cc: Daniel Kurtz, Brian Norris, Jonathan Corbet, Greg Kroah-Hartman,
	Jiri Slaby, Ingo Molnar, Thomas Gleixner, Christoffer Dall,
	Paul E. McKenney, Marc Zyngier, Frederic Weisbecker,
	David Woodhouse, Tom Saeger, Mimi Zohar, Levin,
	Alexander (Sasha Levin),
	Linux Documentation List, Linux Kernel Mailing List,
	open list:SERIAL DRIVERS

On Thu, Mar 1, 2018 at 11:24 PM, Aaron Durbin <adurbin@chromium.org> wrote:
> On Thu, Mar 1, 2018 at 1:02 PM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Thu, Mar 1, 2018 at 9:22 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:
>>> On Thu, Mar 1, 2018 at 11:47 AM Andy Shevchenko <andy.shevchenko@gmail.com>
>>> wrote:
>>
>>> "earlycon simply does not utilize the information".
>>>
>>> earlycon parses iotype, mapbase and baud (from options).  However, it is
>>> hard-coded to assume that the clock used to generate the UART bitclock is
>>> always "BASE_BAUD * 16" (1843200).  While this may be true for many UARTs,
>>> it isn't true for AMD's CZ/ST which has a 8250_dw and uses a fixed 48 MHz
>>> clock.  The main 8250_dw driver uses devm_clk_get to get the "baudclk" and
>>> uses its rate to initialize uartclk.  For AMD CZ/ST, this "baudclk" is
>>> actually a set up in acpi_apd.c when there is an acpi match for "AMD0020",
>>> with a rate read from the .fixed_clk_rate param of the corresponding
>>> apd_device_desc.
>>>
>>> This patch attempts to add a way to inform earlycon about this clock.  As
>>> noted above, the information is actually already in the kernel and used by
>>> 8250_dw - I would happy be to hear recommendations for wiring this data
>>> into earlycon that doesn't require adding another command line arg.
>>
>> And it should not require that for sure!
>
> But it does require that. There's an input clock to the uart ip block.
> That is a design constraint by the hardware and is required to make
> baud calculation work.

I mean it should not be user's headache to provide this information to
the system.

> It's not a firmware problem.

If it's ACPI, then it's definitely firmware issue, since SPCR provides
a baudrate.

> Its the driver's problem in that it
> assumes an input clock to the uart block that does not reflect
> reality.

So, driver can't get this info from device tree or what?

>> Okay, configures a necessary IPs to feed UART with expected 1.8432M clock.
>
> That's only possible if there is a clock divider on the front end of
> the uart block. For this hardware that's not the case. I actually did
> this very thing on intel chromebook devices, but it was only possible
> because there was a hardware divider that could be tuned to reach the
> assumed clock that the code currently assumes.

OK.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2] earlycon: Allow specifying a uartclk in options
@ 2018-03-03 15:38           ` Andy Shevchenko
  0 siblings, 0 replies; 27+ messages in thread
From: Andy Shevchenko @ 2018-03-03 15:38 UTC (permalink / raw)
  To: Aaron Durbin
  Cc: Daniel Kurtz, Brian Norris, Jonathan Corbet, Greg Kroah-Hartman,
	Jiri Slaby, Ingo Molnar, Thomas Gleixner, Christoffer Dall,
	Paul E. McKenney, Marc Zyngier, Frederic Weisbecker,
	David Woodhouse, Tom Saeger, Mimi Zohar, Levin,
	Alexander (Sasha Levin),
	Linux Documentation List, Linux Kernel Mailing List,
	open list:SERIAL DRIVERS

On Thu, Mar 1, 2018 at 11:24 PM, Aaron Durbin <adurbin@chromium.org> wrote:
> On Thu, Mar 1, 2018 at 1:02 PM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Thu, Mar 1, 2018 at 9:22 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:
>>> On Thu, Mar 1, 2018 at 11:47 AM Andy Shevchenko <andy.shevchenko@gmail.com>
>>> wrote:
>>
>>> "earlycon simply does not utilize the information".
>>>
>>> earlycon parses iotype, mapbase and baud (from options).  However, it is
>>> hard-coded to assume that the clock used to generate the UART bitclock is
>>> always "BASE_BAUD * 16" (1843200).  While this may be true for many UARTs,
>>> it isn't true for AMD's CZ/ST which has a 8250_dw and uses a fixed 48 MHz
>>> clock.  The main 8250_dw driver uses devm_clk_get to get the "baudclk" and
>>> uses its rate to initialize uartclk.  For AMD CZ/ST, this "baudclk" is
>>> actually a set up in acpi_apd.c when there is an acpi match for "AMD0020",
>>> with a rate read from the .fixed_clk_rate param of the corresponding
>>> apd_device_desc.
>>>
>>> This patch attempts to add a way to inform earlycon about this clock.  As
>>> noted above, the information is actually already in the kernel and used by
>>> 8250_dw - I would happy be to hear recommendations for wiring this data
>>> into earlycon that doesn't require adding another command line arg.
>>
>> And it should not require that for sure!
>
> But it does require that. There's an input clock to the uart ip block.
> That is a design constraint by the hardware and is required to make
> baud calculation work.

I mean it should not be user's headache to provide this information to
the system.

> It's not a firmware problem.

If it's ACPI, then it's definitely firmware issue, since SPCR provides
a baudrate.

> Its the driver's problem in that it
> assumes an input clock to the uart block that does not reflect
> reality.

So, driver can't get this info from device tree or what?

>> Okay, configures a necessary IPs to feed UART with expected 1.8432M clock.
>
> That's only possible if there is a clock divider on the front end of
> the uart block. For this hardware that's not the case. I actually did
> this very thing on intel chromebook devices, but it was only possible
> because there was a hardware divider that could be tuned to reach the
> assumed clock that the code currently assumes.

OK.

-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2] earlycon: Allow specifying a uartclk in options
@ 2018-03-03 15:38           ` Andy Shevchenko
  0 siblings, 0 replies; 27+ messages in thread
From: Andy Shevchenko @ 2018-03-03 15:38 UTC (permalink / raw)
  To: Aaron Durbin
  Cc: Daniel Kurtz, Brian Norris, Jonathan Corbet, Greg Kroah-Hartman,
	Jiri Slaby, Ingo Molnar, Thomas Gleixner, Christoffer Dall,
	Paul E. McKenney, Marc Zyngier, Frederic Weisbecker,
	David Woodhouse, Tom Saeger, Mimi Zohar, Levin,
	Alexander (Sasha Levin),
	Linux Documentation List, Linux Kernel Mailing List,
	open list:SERIAL DRIVERS

On Thu, Mar 1, 2018 at 11:24 PM, Aaron Durbin <adurbin@chromium.org> wrote:
> On Thu, Mar 1, 2018 at 1:02 PM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Thu, Mar 1, 2018 at 9:22 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:
>>> On Thu, Mar 1, 2018 at 11:47 AM Andy Shevchenko <andy.shevchenko@gmail.com>
>>> wrote:
>>
>>> "earlycon simply does not utilize the information".
>>>
>>> earlycon parses iotype, mapbase and baud (from options).  However, it is
>>> hard-coded to assume that the clock used to generate the UART bitclock is
>>> always "BASE_BAUD * 16" (1843200).  While this may be true for many UARTs,
>>> it isn't true for AMD's CZ/ST which has a 8250_dw and uses a fixed 48 MHz
>>> clock.  The main 8250_dw driver uses devm_clk_get to get the "baudclk" and
>>> uses its rate to initialize uartclk.  For AMD CZ/ST, this "baudclk" is
>>> actually a set up in acpi_apd.c when there is an acpi match for "AMD0020",
>>> with a rate read from the .fixed_clk_rate param of the corresponding
>>> apd_device_desc.
>>>
>>> This patch attempts to add a way to inform earlycon about this clock.  As
>>> noted above, the information is actually already in the kernel and used by
>>> 8250_dw - I would happy be to hear recommendations for wiring this data
>>> into earlycon that doesn't require adding another command line arg.
>>
>> And it should not require that for sure!
>
> But it does require that. There's an input clock to the uart ip block.
> That is a design constraint by the hardware and is required to make
> baud calculation work.

I mean it should not be user's headache to provide this information to
the system.

> It's not a firmware problem.

If it's ACPI, then it's definitely firmware issue, since SPCR provides
a baudrate.

> Its the driver's problem in that it
> assumes an input clock to the uart block that does not reflect
> reality.

So, driver can't get this info from device tree or what?

>> Okay, configures a necessary IPs to feed UART with expected 1.8432M clock.
>
> That's only possible if there is a clock divider on the front end of
> the uart block. For this hardware that's not the case. I actually did
> this very thing on intel chromebook devices, but it was only possible
> because there was a hardware divider that could be tuned to reach the
> assumed clock that the code currently assumes.

OK.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2] earlycon: Allow specifying a uartclk in options
  2018-03-02 18:35         ` Daniel Kurtz
@ 2018-03-03 15:56           ` Andy Shevchenko
  -1 siblings, 0 replies; 27+ messages in thread
From: Andy Shevchenko @ 2018-03-03 15:56 UTC (permalink / raw)
  To: Daniel Kurtz
  Cc: Aaron Durbin, Brian Norris, Jonathan Corbet, Greg Kroah-Hartman,
	Jiri Slaby, Ingo Molnar, Thomas Gleixner, Christoffer Dall,
	Paul McKenney, Marc Zyngier, Frederic Weisbecker,
	David Woodhouse, Tom Saeger, Mimi Zohar, Levin,
	Alexander (Sasha Levin),
	Linux Documentation List, Linux Kernel Mailing List,
	open list:SERIAL DRIVERS

On Fri, Mar 2, 2018 at 8:35 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:
> On Thu, Mar 1, 2018 at 1:02 PM Andy Shevchenko <andy.shevchenko@gmail.com>
> wrote:
>> On Thu, Mar 1, 2018 at 9:22 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:
>> > On Thu, Mar 1, 2018 at 11:47 AM Andy Shevchenko <
> andy.shevchenko@gmail.com>
>> > wrote:

> the UART bitclock
> is
>> > always "BASE_BAUD * 16" (1843200).  While this may be true for many
> UARTs,
>> > it isn't true for AMD's CZ/ST which has a 8250_dw and uses a fixed 48
> MHz
>> > clock.  The main 8250_dw driver uses devm_clk_get to get the "baudclk"
> and
>> > uses its rate to initialize uartclk.  For AMD CZ/ST, this "baudclk" is
>> > actually a set up in acpi_apd.c when there is an acpi match for
> "AMD0020",
>> > with a rate read from the .fixed_clk_rate param of the corresponding
>> > apd_device_desc.

> As
>> > noted above, the information is actually already in the kernel and used
> by
>> > 8250_dw - I would happy be to hear recommendations for wiring this data
>> > into earlycon that doesn't require adding another command line arg.

Brief look at the code shows that ->setup() call back is executed
after setting initial (which is hardcoded) clock.

What you need is to either create another type of earlycon for your
device with accompanied ->setup() callback, or patch 8250_early.c.

>> > I see that support was also added recently to earlycon to let it use
> ACPI
>> > SPCR to choose a console and configure its parameters... but AFAICT,
> this
>> > path also doesn't allow specifying the uart clock.

It does specify baudrate. It means it's _firmware_ responsibility to
configure UART device properly.

>> Fix your firmware then. It should set console to 115200 like (almost)
>> everyone does.
>> Okay, configures a necessary IPs to feed UART with expected 1.8432M clock.
>
> The console is 115200 when it is enabled.  However, the firmware does not
> always enable it by default.

Another firmware bug.

> The problem is that the UART IP block has a fixed 48 MHz input clock, but
> earlycon assumes this clock is always 1843200.

> I looked a bit further, and I think this patch (or something similar) is
> still required to teach generic earlycon how to handle an explicit
> port->uartclk (ie, one that is not 1843200).
> The extended string can then be explicitly set on the kernel command line
> for this kind of hardware.

No.

> In addition, we can add another patch with a new quirk detector in
> drivers/acpi/spcr.c:acpi_parse_spcr() to handle this hardware.
> acpi_parse_spcr() can then use the extended option string to pass in the
> appropriate UART clock to setup_eralycon().

Definitely no. It's not defined in SPCR spec.

> This would again allow a user to just use the simple command line parameter
> "earlycon" if the device's firmware has a correctly confiured ACPI SPCR
> table.

NAK to the patch, see above alternatives.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2] earlycon: Allow specifying a uartclk in options
@ 2018-03-03 15:56           ` Andy Shevchenko
  0 siblings, 0 replies; 27+ messages in thread
From: Andy Shevchenko @ 2018-03-03 15:56 UTC (permalink / raw)
  To: Daniel Kurtz
  Cc: Aaron Durbin, Brian Norris, Jonathan Corbet, Greg Kroah-Hartman,
	Jiri Slaby, Ingo Molnar, Thomas Gleixner, Christoffer Dall,
	Paul McKenney, Marc Zyngier, Frederic Weisbecker,
	David Woodhouse, Tom Saeger, Mimi Zohar, Levin,
	Alexander (Sasha Levin),
	Linux Documentation List, Linux Kernel Mailing List,
	open list:SERIAL DRIVERS

On Fri, Mar 2, 2018 at 8:35 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:
> On Thu, Mar 1, 2018 at 1:02 PM Andy Shevchenko <andy.shevchenko@gmail.com>
> wrote:
>> On Thu, Mar 1, 2018 at 9:22 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:
>> > On Thu, Mar 1, 2018 at 11:47 AM Andy Shevchenko <
> andy.shevchenko@gmail.com>
>> > wrote:

> the UART bitclock
> is
>> > always "BASE_BAUD * 16" (1843200).  While this may be true for many
> UARTs,
>> > it isn't true for AMD's CZ/ST which has a 8250_dw and uses a fixed 48
> MHz
>> > clock.  The main 8250_dw driver uses devm_clk_get to get the "baudclk"
> and
>> > uses its rate to initialize uartclk.  For AMD CZ/ST, this "baudclk" is
>> > actually a set up in acpi_apd.c when there is an acpi match for
> "AMD0020",
>> > with a rate read from the .fixed_clk_rate param of the corresponding
>> > apd_device_desc.

> As
>> > noted above, the information is actually already in the kernel and used
> by
>> > 8250_dw - I would happy be to hear recommendations for wiring this data
>> > into earlycon that doesn't require adding another command line arg.

Brief look at the code shows that ->setup() call back is executed
after setting initial (which is hardcoded) clock.

What you need is to either create another type of earlycon for your
device with accompanied ->setup() callback, or patch 8250_early.c.

>> > I see that support was also added recently to earlycon to let it use
> ACPI
>> > SPCR to choose a console and configure its parameters... but AFAICT,
> this
>> > path also doesn't allow specifying the uart clock.

It does specify baudrate. It means it's _firmware_ responsibility to
configure UART device properly.

>> Fix your firmware then. It should set console to 115200 like (almost)
>> everyone does.
>> Okay, configures a necessary IPs to feed UART with expected 1.8432M clock.
>
> The console is 115200 when it is enabled.  However, the firmware does not
> always enable it by default.

Another firmware bug.

> The problem is that the UART IP block has a fixed 48 MHz input clock, but
> earlycon assumes this clock is always 1843200.

> I looked a bit further, and I think this patch (or something similar) is
> still required to teach generic earlycon how to handle an explicit
> port->uartclk (ie, one that is not 1843200).
> The extended string can then be explicitly set on the kernel command line
> for this kind of hardware.

No.

> In addition, we can add another patch with a new quirk detector in
> drivers/acpi/spcr.c:acpi_parse_spcr() to handle this hardware.
> acpi_parse_spcr() can then use the extended option string to pass in the
> appropriate UART clock to setup_eralycon().

Definitely no. It's not defined in SPCR spec.

> This would again allow a user to just use the simple command line parameter
> "earlycon" if the device's firmware has a correctly confiured ACPI SPCR
> table.

NAK to the patch, see above alternatives.

-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2] earlycon: Allow specifying a uartclk in options
  2018-03-03 15:38           ` Andy Shevchenko
  (?)
@ 2018-03-03 19:12             ` Aaron Durbin
  -1 siblings, 0 replies; 27+ messages in thread
From: Aaron Durbin @ 2018-03-03 19:12 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Aaron Durbin, Daniel Kurtz, Brian Norris, Jonathan Corbet,
	Greg Kroah-Hartman, Jiri Slaby, Ingo Molnar, Thomas Gleixner,
	Christoffer Dall, Paul E. McKenney, Marc Zyngier,
	Frederic Weisbecker, David Woodhouse, Tom Saeger, Mimi Zohar,
	Levin, Alexander (Sasha Levin),
	Linux Documentation List, Linux Kernel Mailing List,
	open list:SERIAL DRIVERS

On Sat, Mar 3, 2018 at 8:38 AM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Thu, Mar 1, 2018 at 11:24 PM, Aaron Durbin <adurbin@chromium.org> wrote:
>> On Thu, Mar 1, 2018 at 1:02 PM, Andy Shevchenko
>> <andy.shevchenko@gmail.com> wrote:
>>> On Thu, Mar 1, 2018 at 9:22 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:
>>>> On Thu, Mar 1, 2018 at 11:47 AM Andy Shevchenko <andy.shevchenko@gmail.com>
>>>> wrote:
>>>
>>>> "earlycon simply does not utilize the information".
>>>>
>>>> earlycon parses iotype, mapbase and baud (from options).  However, it is
>>>> hard-coded to assume that the clock used to generate the UART bitclock is
>>>> always "BASE_BAUD * 16" (1843200).  While this may be true for many UARTs,
>>>> it isn't true for AMD's CZ/ST which has a 8250_dw and uses a fixed 48 MHz
>>>> clock.  The main 8250_dw driver uses devm_clk_get to get the "baudclk" and
>>>> uses its rate to initialize uartclk.  For AMD CZ/ST, this "baudclk" is
>>>> actually a set up in acpi_apd.c when there is an acpi match for "AMD0020",
>>>> with a rate read from the .fixed_clk_rate param of the corresponding
>>>> apd_device_desc.
>>>>
>>>> This patch attempts to add a way to inform earlycon about this clock.  As
>>>> noted above, the information is actually already in the kernel and used by
>>>> 8250_dw - I would happy be to hear recommendations for wiring this data
>>>> into earlycon that doesn't require adding another command line arg.
>>>
>>> And it should not require that for sure!
>>
>> But it does require that. There's an input clock to the uart ip block.
>> That is a design constraint by the hardware and is required to make
>> baud calculation work.
>
> I mean it should not be user's headache to provide this information to
> the system.
>
>> It's not a firmware problem.
>
> If it's ACPI, then it's definitely firmware issue, since SPCR provides
> a baudrate.
>

earlycon is another implemetnation of driver binding/settings that is
done before the rest of the kernel driver stack. SPCR provides
baudrate, but that's only one piece to the puzzle. You need to know
the UART input clock which you admit is hard coded in the current
driver. It's not possible to configure the divisor in the UART without
knowing the external clock. That's a real dependency that can't be
worked around. The moment the code attempts to configure the baud it
has to know the input clock -- otherwise the calculation is inherently
wrong. Or are you suggesting to lie about the baud such that the the
math works out even though the values are not real?

>> Its the driver's problem in that it
>> assumes an input clock to the uart block that does not reflect
>> reality.
>
> So, driver can't get this info from device tree or what?

There is no device tree on x86. And ACPI driver binding is not fully
initialized when earlycon is being processed. Yes, this can be solved
by getting up the entire ACPI stack, but then that's not really
'early' in the kernel's initialization sequence.

>
>>> Okay, configures a necessary IPs to feed UART with expected 1.8432M clock.
>>
>> That's only possible if there is a clock divider on the front end of
>> the uart block. For this hardware that's not the case. I actually did
>> this very thing on intel chromebook devices, but it was only possible
>> because there was a hardware divider that could be tuned to reach the
>> assumed clock that the code currently assumes.
>
> OK.
>
> --
> With Best Regards,
> Andy Shevchenko

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2] earlycon: Allow specifying a uartclk in options
@ 2018-03-03 19:12             ` Aaron Durbin
  0 siblings, 0 replies; 27+ messages in thread
From: Aaron Durbin @ 2018-03-03 19:12 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Aaron Durbin, Daniel Kurtz, Brian Norris, Jonathan Corbet,
	Greg Kroah-Hartman, Jiri Slaby, Ingo Molnar, Thomas Gleixner,
	Christoffer Dall, Paul E. McKenney, Marc Zyngier,
	Frederic Weisbecker, David Woodhouse, Tom Saeger, Mimi Zohar,
	Levin, Alexander (Sasha Levin),
	Linux Documentation List, Linux Kernel Mailing List,
	open list:SERIAL DRIVERS

On Sat, Mar 3, 2018 at 8:38 AM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Thu, Mar 1, 2018 at 11:24 PM, Aaron Durbin <adurbin@chromium.org> wrote:
>> On Thu, Mar 1, 2018 at 1:02 PM, Andy Shevchenko
>> <andy.shevchenko@gmail.com> wrote:
>>> On Thu, Mar 1, 2018 at 9:22 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:
>>>> On Thu, Mar 1, 2018 at 11:47 AM Andy Shevchenko <andy.shevchenko@gmail.com>
>>>> wrote:
>>>
>>>> "earlycon simply does not utilize the information".
>>>>
>>>> earlycon parses iotype, mapbase and baud (from options).  However, it is
>>>> hard-coded to assume that the clock used to generate the UART bitclock is
>>>> always "BASE_BAUD * 16" (1843200).  While this may be true for many UARTs,
>>>> it isn't true for AMD's CZ/ST which has a 8250_dw and uses a fixed 48 MHz
>>>> clock.  The main 8250_dw driver uses devm_clk_get to get the "baudclk" and
>>>> uses its rate to initialize uartclk.  For AMD CZ/ST, this "baudclk" is
>>>> actually a set up in acpi_apd.c when there is an acpi match for "AMD0020",
>>>> with a rate read from the .fixed_clk_rate param of the corresponding
>>>> apd_device_desc.
>>>>
>>>> This patch attempts to add a way to inform earlycon about this clock.  As
>>>> noted above, the information is actually already in the kernel and used by
>>>> 8250_dw - I would happy be to hear recommendations for wiring this data
>>>> into earlycon that doesn't require adding another command line arg.
>>>
>>> And it should not require that for sure!
>>
>> But it does require that. There's an input clock to the uart ip block.
>> That is a design constraint by the hardware and is required to make
>> baud calculation work.
>
> I mean it should not be user's headache to provide this information to
> the system.
>
>> It's not a firmware problem.
>
> If it's ACPI, then it's definitely firmware issue, since SPCR provides
> a baudrate.
>

earlycon is another implemetnation of driver binding/settings that is
done before the rest of the kernel driver stack. SPCR provides
baudrate, but that's only one piece to the puzzle. You need to know
the UART input clock which you admit is hard coded in the current
driver. It's not possible to configure the divisor in the UART without
knowing the external clock. That's a real dependency that can't be
worked around. The moment the code attempts to configure the baud it
has to know the input clock -- otherwise the calculation is inherently
wrong. Or are you suggesting to lie about the baud such that the the
math works out even though the values are not real?

>> Its the driver's problem in that it
>> assumes an input clock to the uart block that does not reflect
>> reality.
>
> So, driver can't get this info from device tree or what?

There is no device tree on x86. And ACPI driver binding is not fully
initialized when earlycon is being processed. Yes, this can be solved
by getting up the entire ACPI stack, but then that's not really
'early' in the kernel's initialization sequence.

>
>>> Okay, configures a necessary IPs to feed UART with expected 1.8432M clock.
>>
>> That's only possible if there is a clock divider on the front end of
>> the uart block. For this hardware that's not the case. I actually did
>> this very thing on intel chromebook devices, but it was only possible
>> because there was a hardware divider that could be tuned to reach the
>> assumed clock that the code currently assumes.
>
> OK.
>
> --
> With Best Regards,
> Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2] earlycon: Allow specifying a uartclk in options
@ 2018-03-03 19:12             ` Aaron Durbin
  0 siblings, 0 replies; 27+ messages in thread
From: Aaron Durbin @ 2018-03-03 19:12 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Aaron Durbin, Daniel Kurtz, Brian Norris, Jonathan Corbet,
	Greg Kroah-Hartman, Jiri Slaby, Ingo Molnar, Thomas Gleixner,
	Christoffer Dall, Paul E. McKenney, Marc Zyngier,
	Frederic Weisbecker, David Woodhouse, Tom Saeger, Mimi Zohar,
	Levin, Alexander (Sasha Levin),
	Linux Documentation List, Linux Kernel Mailing List, open

On Sat, Mar 3, 2018 at 8:38 AM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Thu, Mar 1, 2018 at 11:24 PM, Aaron Durbin <adurbin@chromium.org> wrote:
>> On Thu, Mar 1, 2018 at 1:02 PM, Andy Shevchenko
>> <andy.shevchenko@gmail.com> wrote:
>>> On Thu, Mar 1, 2018 at 9:22 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:
>>>> On Thu, Mar 1, 2018 at 11:47 AM Andy Shevchenko <andy.shevchenko@gmail.com>
>>>> wrote:
>>>
>>>> "earlycon simply does not utilize the information".
>>>>
>>>> earlycon parses iotype, mapbase and baud (from options).  However, it is
>>>> hard-coded to assume that the clock used to generate the UART bitclock is
>>>> always "BASE_BAUD * 16" (1843200).  While this may be true for many UARTs,
>>>> it isn't true for AMD's CZ/ST which has a 8250_dw and uses a fixed 48 MHz
>>>> clock.  The main 8250_dw driver uses devm_clk_get to get the "baudclk" and
>>>> uses its rate to initialize uartclk.  For AMD CZ/ST, this "baudclk" is
>>>> actually a set up in acpi_apd.c when there is an acpi match for "AMD0020",
>>>> with a rate read from the .fixed_clk_rate param of the corresponding
>>>> apd_device_desc.
>>>>
>>>> This patch attempts to add a way to inform earlycon about this clock.  As
>>>> noted above, the information is actually already in the kernel and used by
>>>> 8250_dw - I would happy be to hear recommendations for wiring this data
>>>> into earlycon that doesn't require adding another command line arg.
>>>
>>> And it should not require that for sure!
>>
>> But it does require that. There's an input clock to the uart ip block.
>> That is a design constraint by the hardware and is required to make
>> baud calculation work.
>
> I mean it should not be user's headache to provide this information to
> the system.
>
>> It's not a firmware problem.
>
> If it's ACPI, then it's definitely firmware issue, since SPCR provides
> a baudrate.
>

earlycon is another implemetnation of driver binding/settings that is
done before the rest of the kernel driver stack. SPCR provides
baudrate, but that's only one piece to the puzzle. You need to know
the UART input clock which you admit is hard coded in the current
driver. It's not possible to configure the divisor in the UART without
knowing the external clock. That's a real dependency that can't be
worked around. The moment the code attempts to configure the baud it
has to know the input clock -- otherwise the calculation is inherently
wrong. Or are you suggesting to lie about the baud such that the the
math works out even though the values are not real?

>> Its the driver's problem in that it
>> assumes an input clock to the uart block that does not reflect
>> reality.
>
> So, driver can't get this info from device tree or what?

There is no device tree on x86. And ACPI driver binding is not fully
initialized when earlycon is being processed. Yes, this can be solved
by getting up the entire ACPI stack, but then that's not really
'early' in the kernel's initialization sequence.

>
>>> Okay, configures a necessary IPs to feed UART with expected 1.8432M clock.
>>
>> That's only possible if there is a clock divider on the front end of
>> the uart block. For this hardware that's not the case. I actually did
>> this very thing on intel chromebook devices, but it was only possible
>> because there was a hardware divider that could be tuned to reach the
>> assumed clock that the code currently assumes.
>
> OK.
>
> --
> With Best Regards,
> Andy Shevchenko

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2] earlycon: Allow specifying a uartclk in options
  2018-03-03 15:56           ` Andy Shevchenko
  (?)
@ 2018-03-03 19:27             ` Aaron Durbin
  -1 siblings, 0 replies; 27+ messages in thread
From: Aaron Durbin @ 2018-03-03 19:27 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Daniel Kurtz, Aaron Durbin, Brian Norris, Jonathan Corbet,
	Greg Kroah-Hartman, Jiri Slaby, Ingo Molnar, Thomas Gleixner,
	Christoffer Dall, Paul McKenney, Marc Zyngier,
	Frederic Weisbecker, David Woodhouse, Tom Saeger, Mimi Zohar,
	Levin, Alexander (Sasha Levin),
	Linux Documentation List, Linux Kernel Mailing List,
	open list:SERIAL DRIVERS

On Sat, Mar 3, 2018 at 8:56 AM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Fri, Mar 2, 2018 at 8:35 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:
>> On Thu, Mar 1, 2018 at 1:02 PM Andy Shevchenko <andy.shevchenko@gmail.com>
>> wrote:
>>> On Thu, Mar 1, 2018 at 9:22 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:
>>> > On Thu, Mar 1, 2018 at 11:47 AM Andy Shevchenko <
>> andy.shevchenko@gmail.com>
>>> > wrote:
>
>> the UART bitclock
>> is
>>> > always "BASE_BAUD * 16" (1843200).  While this may be true for many
>> UARTs,
>>> > it isn't true for AMD's CZ/ST which has a 8250_dw and uses a fixed 48
>> MHz
>>> > clock.  The main 8250_dw driver uses devm_clk_get to get the "baudclk"
>> and
>>> > uses its rate to initialize uartclk.  For AMD CZ/ST, this "baudclk" is
>>> > actually a set up in acpi_apd.c when there is an acpi match for
>> "AMD0020",
>>> > with a rate read from the .fixed_clk_rate param of the corresponding
>>> > apd_device_desc.
>
>> As
>>> > noted above, the information is actually already in the kernel and used
>> by
>>> > 8250_dw - I would happy be to hear recommendations for wiring this data
>>> > into earlycon that doesn't require adding another command line arg.
>
> Brief look at the code shows that ->setup() call back is executed
> after setting initial (which is hardcoded) clock.
>
> What you need is to either create another type of earlycon for your
> device with accompanied ->setup() callback, or patch 8250_early.c.

If I'm understand you correctly, you are suggesting new driver code
that sets the clock accordingly within the port structure such that
the baud rate calculation actually works based on the correct input
clock? Why wouldn't we just extened the generic earlycon driver like
the original patch to support providing the clock? Anything in the
earlycon path that tries to set a baud rate will fail once the
hard-coded input clock assumption doesn't hold true. Why not provide
the ability to correct the assumption for platforms that need it?

>
>>> > I see that support was also added recently to earlycon to let it use
>> ACPI
>>> > SPCR to choose a console and configure its parameters... but AFAICT,
>> this
>>> > path also doesn't allow specifying the uart clock.
>
> It does specify baudrate. It means it's _firmware_ responsibility to
> configure UART device properly.

baudrate != input clock. The issue is that once the driver code
attempts to set a baud rate w/o having the correct input clock then
things break.

>
>>> Fix your firmware then. It should set console to 115200 like (almost)
>>> everyone does.
>>> Okay, configures a necessary IPs to feed UART with expected 1.8432M clock.
>>
>> The console is 115200 when it is enabled.  However, the firmware does not
>> always enable it by default.
>
> Another firmware bug.

It's more complex than that. You seem to take the stance that the
firmware should bring every IP block out of reset and/or clock and
power gating. That then subsequently requires the kernel to enable all
those soc drivers that put those devices in power or clock gated state
to reach the maximum power savings. While that's certainly possible,
it just leads to bloated kernels. That is orthogonal to the problem of
setting baud rate w/o knowing the proper input clock frequency,
though.

If we do make the assumption the firmware sets up the uart, but the
kernel earlycon has a different baud rate specified than what firmware
set up then the baud rate calculation would be wrong as well since the
input clock isn't known. As such one cannot use earlycon when: 1.
firmware doesn't set up uart 2. firmware baud rate != earlycon
specified baud rate.  Providing the proper input clock for the device
in question solves both 1 and 2.

>
>> The problem is that the UART IP block has a fixed 48 MHz input clock, but
>> earlycon assumes this clock is always 1843200.
>
>> I looked a bit further, and I think this patch (or something similar) is
>> still required to teach generic earlycon how to handle an explicit
>> port->uartclk (ie, one that is not 1843200).
>> The extended string can then be explicitly set on the kernel command line
>> for this kind of hardware.
>
> No.
>
>> In addition, we can add another patch with a new quirk detector in
>> drivers/acpi/spcr.c:acpi_parse_spcr() to handle this hardware.
>> acpi_parse_spcr() can then use the extended option string to pass in the
>> appropriate UART clock to setup_eralycon().
>
> Definitely no. It's not defined in SPCR spec.
>
>> This would again allow a user to just use the simple command line parameter
>> "earlycon" if the device's firmware has a correctly confiured ACPI SPCR
>> table.
>
> NAK to the patch, see above alternatives.
>
> --
> With Best Regards,
> Andy Shevchenko

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2] earlycon: Allow specifying a uartclk in options
@ 2018-03-03 19:27             ` Aaron Durbin
  0 siblings, 0 replies; 27+ messages in thread
From: Aaron Durbin @ 2018-03-03 19:27 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Daniel Kurtz, Aaron Durbin, Brian Norris, Jonathan Corbet,
	Greg Kroah-Hartman, Jiri Slaby, Ingo Molnar, Thomas Gleixner,
	Christoffer Dall, Paul McKenney, Marc Zyngier,
	Frederic Weisbecker, David Woodhouse, Tom Saeger, Mimi Zohar,
	Levin, Alexander (Sasha Levin),
	Linux Documentation List, Linux Kernel Mailing List,
	open list:SERIAL DRIVERS

On Sat, Mar 3, 2018 at 8:56 AM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Fri, Mar 2, 2018 at 8:35 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:
>> On Thu, Mar 1, 2018 at 1:02 PM Andy Shevchenko <andy.shevchenko@gmail.com>
>> wrote:
>>> On Thu, Mar 1, 2018 at 9:22 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:
>>> > On Thu, Mar 1, 2018 at 11:47 AM Andy Shevchenko <
>> andy.shevchenko@gmail.com>
>>> > wrote:
>
>> the UART bitclock
>> is
>>> > always "BASE_BAUD * 16" (1843200).  While this may be true for many
>> UARTs,
>>> > it isn't true for AMD's CZ/ST which has a 8250_dw and uses a fixed 48
>> MHz
>>> > clock.  The main 8250_dw driver uses devm_clk_get to get the "baudclk"
>> and
>>> > uses its rate to initialize uartclk.  For AMD CZ/ST, this "baudclk" is
>>> > actually a set up in acpi_apd.c when there is an acpi match for
>> "AMD0020",
>>> > with a rate read from the .fixed_clk_rate param of the corresponding
>>> > apd_device_desc.
>
>> As
>>> > noted above, the information is actually already in the kernel and used
>> by
>>> > 8250_dw - I would happy be to hear recommendations for wiring this data
>>> > into earlycon that doesn't require adding another command line arg.
>
> Brief look at the code shows that ->setup() call back is executed
> after setting initial (which is hardcoded) clock.
>
> What you need is to either create another type of earlycon for your
> device with accompanied ->setup() callback, or patch 8250_early.c.

If I'm understand you correctly, you are suggesting new driver code
that sets the clock accordingly within the port structure such that
the baud rate calculation actually works based on the correct input
clock? Why wouldn't we just extened the generic earlycon driver like
the original patch to support providing the clock? Anything in the
earlycon path that tries to set a baud rate will fail once the
hard-coded input clock assumption doesn't hold true. Why not provide
the ability to correct the assumption for platforms that need it?

>
>>> > I see that support was also added recently to earlycon to let it use
>> ACPI
>>> > SPCR to choose a console and configure its parameters... but AFAICT,
>> this
>>> > path also doesn't allow specifying the uart clock.
>
> It does specify baudrate. It means it's _firmware_ responsibility to
> configure UART device properly.

baudrate != input clock. The issue is that once the driver code
attempts to set a baud rate w/o having the correct input clock then
things break.

>
>>> Fix your firmware then. It should set console to 115200 like (almost)
>>> everyone does.
>>> Okay, configures a necessary IPs to feed UART with expected 1.8432M clock.
>>
>> The console is 115200 when it is enabled.  However, the firmware does not
>> always enable it by default.
>
> Another firmware bug.

It's more complex than that. You seem to take the stance that the
firmware should bring every IP block out of reset and/or clock and
power gating. That then subsequently requires the kernel to enable all
those soc drivers that put those devices in power or clock gated state
to reach the maximum power savings. While that's certainly possible,
it just leads to bloated kernels. That is orthogonal to the problem of
setting baud rate w/o knowing the proper input clock frequency,
though.

If we do make the assumption the firmware sets up the uart, but the
kernel earlycon has a different baud rate specified than what firmware
set up then the baud rate calculation would be wrong as well since the
input clock isn't known. As such one cannot use earlycon when: 1.
firmware doesn't set up uart 2. firmware baud rate != earlycon
specified baud rate.  Providing the proper input clock for the device
in question solves both 1 and 2.

>
>> The problem is that the UART IP block has a fixed 48 MHz input clock, but
>> earlycon assumes this clock is always 1843200.
>
>> I looked a bit further, and I think this patch (or something similar) is
>> still required to teach generic earlycon how to handle an explicit
>> port->uartclk (ie, one that is not 1843200).
>> The extended string can then be explicitly set on the kernel command line
>> for this kind of hardware.
>
> No.
>
>> In addition, we can add another patch with a new quirk detector in
>> drivers/acpi/spcr.c:acpi_parse_spcr() to handle this hardware.
>> acpi_parse_spcr() can then use the extended option string to pass in the
>> appropriate UART clock to setup_eralycon().
>
> Definitely no. It's not defined in SPCR spec.
>
>> This would again allow a user to just use the simple command line parameter
>> "earlycon" if the device's firmware has a correctly confiured ACPI SPCR
>> table.
>
> NAK to the patch, see above alternatives.
>
> --
> With Best Regards,
> Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2] earlycon: Allow specifying a uartclk in options
@ 2018-03-03 19:27             ` Aaron Durbin
  0 siblings, 0 replies; 27+ messages in thread
From: Aaron Durbin @ 2018-03-03 19:27 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Daniel Kurtz, Aaron Durbin, Brian Norris, Jonathan Corbet,
	Greg Kroah-Hartman, Jiri Slaby, Ingo Molnar, Thomas Gleixner,
	Christoffer Dall, Paul McKenney, Marc Zyngier,
	Frederic Weisbecker, David Woodhouse, Tom Saeger, Mimi Zohar,
	Levin, Alexander (Sasha Levin),
	Linux Documentation List, Linux Kernel Mailing List, SE

On Sat, Mar 3, 2018 at 8:56 AM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Fri, Mar 2, 2018 at 8:35 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:
>> On Thu, Mar 1, 2018 at 1:02 PM Andy Shevchenko <andy.shevchenko@gmail.com>
>> wrote:
>>> On Thu, Mar 1, 2018 at 9:22 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:
>>> > On Thu, Mar 1, 2018 at 11:47 AM Andy Shevchenko <
>> andy.shevchenko@gmail.com>
>>> > wrote:
>
>> the UART bitclock
>> is
>>> > always "BASE_BAUD * 16" (1843200).  While this may be true for many
>> UARTs,
>>> > it isn't true for AMD's CZ/ST which has a 8250_dw and uses a fixed 48
>> MHz
>>> > clock.  The main 8250_dw driver uses devm_clk_get to get the "baudclk"
>> and
>>> > uses its rate to initialize uartclk.  For AMD CZ/ST, this "baudclk" is
>>> > actually a set up in acpi_apd.c when there is an acpi match for
>> "AMD0020",
>>> > with a rate read from the .fixed_clk_rate param of the corresponding
>>> > apd_device_desc.
>
>> As
>>> > noted above, the information is actually already in the kernel and used
>> by
>>> > 8250_dw - I would happy be to hear recommendations for wiring this data
>>> > into earlycon that doesn't require adding another command line arg.
>
> Brief look at the code shows that ->setup() call back is executed
> after setting initial (which is hardcoded) clock.
>
> What you need is to either create another type of earlycon for your
> device with accompanied ->setup() callback, or patch 8250_early.c.

If I'm understand you correctly, you are suggesting new driver code
that sets the clock accordingly within the port structure such that
the baud rate calculation actually works based on the correct input
clock? Why wouldn't we just extened the generic earlycon driver like
the original patch to support providing the clock? Anything in the
earlycon path that tries to set a baud rate will fail once the
hard-coded input clock assumption doesn't hold true. Why not provide
the ability to correct the assumption for platforms that need it?

>
>>> > I see that support was also added recently to earlycon to let it use
>> ACPI
>>> > SPCR to choose a console and configure its parameters... but AFAICT,
>> this
>>> > path also doesn't allow specifying the uart clock.
>
> It does specify baudrate. It means it's _firmware_ responsibility to
> configure UART device properly.

baudrate != input clock. The issue is that once the driver code
attempts to set a baud rate w/o having the correct input clock then
things break.

>
>>> Fix your firmware then. It should set console to 115200 like (almost)
>>> everyone does.
>>> Okay, configures a necessary IPs to feed UART with expected 1.8432M clock.
>>
>> The console is 115200 when it is enabled.  However, the firmware does not
>> always enable it by default.
>
> Another firmware bug.

It's more complex than that. You seem to take the stance that the
firmware should bring every IP block out of reset and/or clock and
power gating. That then subsequently requires the kernel to enable all
those soc drivers that put those devices in power or clock gated state
to reach the maximum power savings. While that's certainly possible,
it just leads to bloated kernels. That is orthogonal to the problem of
setting baud rate w/o knowing the proper input clock frequency,
though.

If we do make the assumption the firmware sets up the uart, but the
kernel earlycon has a different baud rate specified than what firmware
set up then the baud rate calculation would be wrong as well since the
input clock isn't known. As such one cannot use earlycon when: 1.
firmware doesn't set up uart 2. firmware baud rate != earlycon
specified baud rate.  Providing the proper input clock for the device
in question solves both 1 and 2.

>
>> The problem is that the UART IP block has a fixed 48 MHz input clock, but
>> earlycon assumes this clock is always 1843200.
>
>> I looked a bit further, and I think this patch (or something similar) is
>> still required to teach generic earlycon how to handle an explicit
>> port->uartclk (ie, one that is not 1843200).
>> The extended string can then be explicitly set on the kernel command line
>> for this kind of hardware.
>
> No.
>
>> In addition, we can add another patch with a new quirk detector in
>> drivers/acpi/spcr.c:acpi_parse_spcr() to handle this hardware.
>> acpi_parse_spcr() can then use the extended option string to pass in the
>> appropriate UART clock to setup_eralycon().
>
> Definitely no. It's not defined in SPCR spec.
>
>> This would again allow a user to just use the simple command line parameter
>> "earlycon" if the device's firmware has a correctly confiured ACPI SPCR
>> table.
>
> NAK to the patch, see above alternatives.
>
> --
> With Best Regards,
> Andy Shevchenko

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2] earlycon: Allow specifying a uartclk in options
  2018-03-01 18:43 ` Daniel Kurtz
@ 2018-03-07 23:30   ` Daniel Kurtz
  -1 siblings, 0 replies; 27+ messages in thread
From: Daniel Kurtz @ 2018-03-07 23:30 UTC (permalink / raw)
  To: Daniel Kurtz
  Cc: adurbin, Brian Norris, Jonathan Corbet, Greg Kroah-Hartman,
	jslaby, mingo, Thomas Gleixner, Christoffer Dall, Paul McKenney,
	marc.zyngier, frederic, dwmw, tom.saeger, zohar, alexander.levin,
	linux-doc, linux-kernel, linux-serial

On Thu, Mar 1, 2018 at 11:43 AM Daniel Kurtz <djkurtz@chromium.org> wrote:

> Currently when an earlycon is registered, the uartclk is assumed to be
> BASE_BAUD * 16 = 1843200.  If a baud rate is specified in the earlycon
> options, then 8250_early's init_port will program the UART clock divider
> registers based on this assumed uartclk.

> However, not all uarts have a UART clock of 1843200.  For example, the
> 8250_dw uart in AMD's CZ/ST uses a fixed 48 MHz clock (as specified in
> cz_uart_desc in acpi_apd.c).  Thus, specifying a baud when using earlycon
> on such a device will result in incorrect divider values and a wrong UART
> clock.

> Fix this by extending the earlycon options parameter to allow
specification
> of a uartclk, like so:

>   earlycon=uart,mmio32,0xfedc6000,115200,48000000

> If none is specified, fall-back to prior behavior - 1843200.

> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>

This general approach is facing resistance, so trying another more targeted
approach to work around the "BASE_BAUD=115200" assumption in arch/x86:

https://patchwork.kernel.org/patch/10265587/

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2] earlycon: Allow specifying a uartclk in options
@ 2018-03-07 23:30   ` Daniel Kurtz
  0 siblings, 0 replies; 27+ messages in thread
From: Daniel Kurtz @ 2018-03-07 23:30 UTC (permalink / raw)
  To: Daniel Kurtz
  Cc: adurbin, Brian Norris, Jonathan Corbet, Greg Kroah-Hartman,
	jslaby, mingo, Thomas Gleixner, Christoffer Dall, Paul McKenney,
	marc.zyngier, frederic, dwmw, tom.saeger, zohar, alexander.levin,
	linux-doc, linux-kernel, linux-serial

On Thu, Mar 1, 2018 at 11:43 AM Daniel Kurtz <djkurtz@chromium.org> wrote:

> Currently when an earlycon is registered, the uartclk is assumed to be
> BASE_BAUD * 16 = 1843200.  If a baud rate is specified in the earlycon
> options, then 8250_early's init_port will program the UART clock divider
> registers based on this assumed uartclk.

> However, not all uarts have a UART clock of 1843200.  For example, the
> 8250_dw uart in AMD's CZ/ST uses a fixed 48 MHz clock (as specified in
> cz_uart_desc in acpi_apd.c).  Thus, specifying a baud when using earlycon
> on such a device will result in incorrect divider values and a wrong UART
> clock.

> Fix this by extending the earlycon options parameter to allow
specification
> of a uartclk, like so:

>   earlycon=uart,mmio32,0xfedc6000,115200,48000000

> If none is specified, fall-back to prior behavior - 1843200.

> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>

This general approach is facing resistance, so trying another more targeted
approach to work around the "BASE_BAUD=115200" assumption in arch/x86:

https://patchwork.kernel.org/patch/10265587/
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 27+ messages in thread

end of thread, other threads:[~2018-03-07 23:30 UTC | newest]

Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-01 18:43 [PATCH v2] earlycon: Allow specifying a uartclk in options Daniel Kurtz
2018-03-01 18:43 ` Daniel Kurtz
2018-03-01 18:43 ` Daniel Kurtz
2018-03-01 18:47 ` Andy Shevchenko
2018-03-01 18:47   ` Andy Shevchenko
2018-03-01 19:22   ` Daniel Kurtz
2018-03-01 19:22     ` Daniel Kurtz
2018-03-01 20:02     ` Andy Shevchenko
2018-03-01 20:02       ` Andy Shevchenko
2018-03-01 21:24       ` Aaron Durbin
2018-03-01 21:24         ` Aaron Durbin
2018-03-01 21:24         ` Aaron Durbin
2018-03-03 15:38         ` Andy Shevchenko
2018-03-03 15:38           ` Andy Shevchenko
2018-03-03 15:38           ` Andy Shevchenko
2018-03-03 19:12           ` Aaron Durbin
2018-03-03 19:12             ` Aaron Durbin
2018-03-03 19:12             ` Aaron Durbin
2018-03-02 18:35       ` Daniel Kurtz
2018-03-02 18:35         ` Daniel Kurtz
2018-03-03 15:56         ` Andy Shevchenko
2018-03-03 15:56           ` Andy Shevchenko
2018-03-03 19:27           ` Aaron Durbin
2018-03-03 19:27             ` Aaron Durbin
2018-03-03 19:27             ` Aaron Durbin
2018-03-07 23:30 ` Daniel Kurtz
2018-03-07 23:30   ` Daniel Kurtz

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.