pch_uart: Cleanups, board quirks, and user uartclk parameter

drivers/tty/serial/pch_uart.c +1 -0

 	if (options)
 		uart_parse_options(options, &baud, &parity, &bits, &flow);

@@ -1566,12 +1580,16 @@ static struct eg20t_port *pch_uart_init_port(struct pci_dev *pdev,
 	if (!rxbuf)
 		goto init_port_free_txbuf;
 
+	/* Setup UART clock, checking for board specific clocks. */
 	uartclk = DEFAULT_UARTCLK;

Quoted 7 lines

drivers/tty/serial/pch_uart.c +7 -0

+	/* Setup UART clock, checking for board specific clocks. */
+	uartclk = DEFAULT_UARTCLK;
+
+	board_name = dmi_get_system_info(DMI_BOARD_NAME);
+	if (board_name && strstr(board_name, "CM-iTC"))
+		uartclk = CMITC_UARTCLK;
+

drivers/tty/serial/pch_uart.c static int __init pch_console_setup(struct console *co, char *options) · line 1524

Quoted 5 lines

drivers/tty/serial/pch_uart.c +5 -0

+	board_name = dmi_get_system_info(DMI_PRODUCT_NAME);
+	if (board_name && strstr(board_name, "Fish River Island II"))
+		uartclk = FRI2_UARTCLK;
+
+	port->uartclk = uartclk;

This is confusing, you load product name into a variable called board_name ?? perhaps "name" would be clearer ?

Quoted 11 lines

drivers/tty/serial/pch_uart.c +1 -0

 	if (options)
 		uart_parse_options(options, &baud, &parity, &bits, &flow);

@@ -1566,12 +1580,16 @@ static struct eg20t_port *pch_uart_init_port(struct pci_dev *pdev,
 	if (!rxbuf)
 		goto init_port_free_txbuf;
 
+	/* Setup UART clock, checking for board specific clocks. */
 	uartclk = DEFAULT_UARTCLK;
 

drivers/tty/serial/pch_uart.c static struct eg20t_port *pch_uart_init_port(struct pci_dev *pdev, · line 1592

Quoted 9 lines

drivers/tty/serial/pch_uart.c +5 -2

-	/* quirk for CM-iTC board */
 	board_name = dmi_get_system_info(DMI_BOARD_NAME);
 	if (board_name && strstr(board_name, "CM-iTC"))
-		uartclk = 192000000; /* 192.0MHz */
+		uartclk = CMITC_UARTCLK;
+
+	board_name = dmi_get_system_info(DMI_PRODUCT_NAME);
+	if (board_name && strstr(board_name, "Fish River Island II"))
+		uartclk = FRI2_UARTCLK;

And we have two locations so this is going to get missed on updates. Can we have one function for this please ?

Quoted 7 lines · Alan Cox

Quoted 7 lines

drivers/tty/serial/pch_uart.c +7 -0

+	/* Setup UART clock, checking for board specific clocks. */
+	uartclk = DEFAULT_UARTCLK;
+
+	board_name = dmi_get_system_info(DMI_BOARD_NAME);
+	if (board_name && strstr(board_name, "CM-iTC"))
+		uartclk = CMITC_UARTCLK;
+

Quoted 8 lines

drivers/tty/serial/pch_uart.c static int __init pch_console_setup(struct console *co, char *options) · line 1524

Quoted 5 lines

drivers/tty/serial/pch_uart.c +5 -0

+	board_name = dmi_get_system_info(DMI_PRODUCT_NAME);
+	if (board_name && strstr(board_name, "Fish River Island II"))
+		uartclk = FRI2_UARTCLK;
+
+	port->uartclk = uartclk;

This is confusing, you load product name into a variable called board_name ?? perhaps "name" would be clearer ?

OK, done.

Quoted 19 lines · Darren Hart

Quoted 15 lines · Alan Cox

drivers/tty/serial/pch_uart.c static int __init pch_console_setup(struct console *co, char *options) · line 1524

Quoted 12 lines

drivers/tty/serial/pch_uart.c +12 -0

+	/* Setup UART clock, checking for board specific clocks. */
+	uartclk = DEFAULT_UARTCLK;
+
+	board_name = dmi_get_system_info(DMI_BOARD_NAME);
+	if (board_name && strstr(board_name, "CM-iTC"))
+		uartclk = CMITC_UARTCLK;
+
+	board_name = dmi_get_system_info(DMI_PRODUCT_NAME);
+	if (board_name && strstr(board_name, "Fish River Island II"))
+		uartclk = FRI2_UARTCLK;
+
+	port->uartclk = uartclk;

This is confusing, you load product name into a variable called board_name ?? perhaps "name" would be clearer ?

Quoted 1 line

OK, done.

"Done" where? Is there a newer patch series I should be looking at here to apply? I'm guessing I can ignore this one, right?

consider it ignored :)

Quoted 20 lines · Greg Kroah-Hartman

Quoted 19 lines · Darren Hart

Quoted 15 lines · Alan Cox

drivers/tty/serial/pch_uart.c static int __init pch_console_setup(struct console *co, char *options) · line 1524

Quoted 12 lines

drivers/tty/serial/pch_uart.c +12 -0

+	/* Setup UART clock, checking for board specific clocks. */
+	uartclk = DEFAULT_UARTCLK;
+
+	board_name = dmi_get_system_info(DMI_BOARD_NAME);
+	if (board_name && strstr(board_name, "CM-iTC"))
+		uartclk = CMITC_UARTCLK;
+
+	board_name = dmi_get_system_info(DMI_PRODUCT_NAME);
+	if (board_name && strstr(board_name, "Fish River Island II"))
+		uartclk = FRI2_UARTCLK;
+
+	port->uartclk = uartclk;

This is confusing, you load product name into a variable called board_name ?? perhaps "name" would be clearer ?

Quoted 6 lines

Quoted 1 line

OK, done.

"Done" where? Is there a newer patch series I should be looking at here to apply? I'm guessing I can ignore this one, right?

consider it ignored :)

I have the V2 patch series here, tested it, was going to send it today .... then received a new revision of the hardware/firmware which hides the PCI device and the UART is driven by the 8250 driver and requires me to use 3318 as the baud ..... ***sigh***. I'm not sure what the right thing is to do right now. I'm going to have a conversation with the hardware manufacturer and the TWO firmware teams I'm working with.

If anyone has experience with this sort of mess and would like to offer a recommended course of action, I'm all ears.

This series does some minor clean-up to the pch_uart driver, adds support for the Fish River Island II UART clock, and introduces a user_uartclk parameter to aid in developing for early and changing hardware.

Note that this series is my proposed alternative solution to that provided by Tomoya MORNIAGA and Feng Tang which drops the board quirks and opts to assume a 192 MHz clock on all boards. The problem with this approach is that the CLKCFG register may have been set to something other than the 192MHz configuration by the firmware. If so, then the pch_uart will send garbage between the time the boot console is disabled and the pch_phub sets the CLKCFG register again. In my case, the pch_phub PCI probe occurs after the pch_uart_console_setup. Even if it happened before, the output up until the PCI probing would be garbage.

In order to support an early serial console, we cannot rely on the pch_phub probe function to setup the CFGCLK register. This series relies on the board quirks and doesn't force the setting of the CLKREG in the pch_phub code. Instead, it aligns with what is the default configuration (defined by firmware) for a given board. The user_uartclk provides a mechanism to force a specific uartclk if necessary.

2012年2月22日10:59 Darren Hart <[email protected]>:

Quoted 14 lines

This series does some minor clean-up to the pch_uart driver, adds support for the Fish River Island II UART clock, and introduces a user_uartclk parameter to aid in developing for early and changing hardware.

Note that this series is my proposed alternative solution to that provided by Tomoya MORNIAGA and Feng Tang which drops the board quirks and opts to assume a 192 MHz clock on all boards. The problem with this approach is that the CLKCFG register may have been set to something other than the 192MHz configuration by the firmware. If so, then the pch_uart will send garbage between the time the boot console is disabled and the pch_phub sets the CLKCFG register again. In my case, the pch_phub PCI probe occurs after the pch_uart_console_setup. Even if it happened before, the output up until the PCI probing would be garbage.

Quoted 6 lines

In order to support an early serial console, we cannot rely on the pch_phub probe function to setup the CFGCLK register. This series relies on the board quirks and doesn't force the setting of the CLKREG in the pch_phub code. Instead, it aligns with what is the default configuration (defined by firmware) for a given board. The user_uartclk provides a mechanism to force a specific uartclk if necessary.

I think UART console function(including "early serial console") is used for debug use.

So, if people who want to see the boot log correctly before pch_phub installed, the people have only to do configure uart_clock by themselves.

So, I think default uart_clock 192MHz setting is better than Darren's opinion.

Let me know your opinion.

This patch series allows for a functional early serial console as well as using the UART after boot. It leaves the CM-iTC board alone. So this seems to enable all use cases, while forcing 192MHz breaks the FRI2 early serial console. I don't see an advantage to that approach other than the obviously simpler code (which is nice, but should not trump functionality).

2012年2月22日12:36 Darren Hart <[email protected]>:

Quoted 6 lines

This patch series allows for a functional early serial console as well as using the UART after boot. It leaves the CM-iTC board alone. So this seems to enable all use cases, while forcing 192MHz breaks the FRI2 early serial console. I don't see an advantage to that approach other than the obviously simpler code (which is nice, but should not trump functionality).

Your quark "Fish River Island II" is OK.
My concern is default uart_clock remains 1.8432 MHz.
Like I said the advantage before, I think this should be 192MHz not 1.8432 MHz.

Or do you have any reason 1.8432 MHz should be set as PCH_UART default clock.

We can add a patch to this series that sets the
default to 192MHz, drops the CM-iTC quirk, and does nothing in pch_phub probe for the FRI2.

2012年2月22日15:39 Darren Hart <[email protected]>:

Quoted 3 lines

We can add a patch to this series that sets the
default to 192MHz, drops the CM-iTC quirk, and does nothing in pch_phub probe for the FRI2.

If you set the clock of pch_uart as 64MHz,
do you need to add quirk for FRI2 to pch_phub so as to provide 64MHz clock?

Quoted 5 lines · Tomoya MORINAGA

2012年2月22日15:39 Darren Hart <[email protected]>:

Quoted 3 lines

We can add a patch to this series that sets the
default to 192MHz, drops the CM-iTC quirk, and does nothing in pch_phub probe for the FRI2.

Quoted 2 lines

If you set the clock of pch_uart as 64MHz,
do you need to add quirk for FRI2 to pch_phub so as to provide 64MHz clock?

I admit the value of pch_phub eludes me a bit. In my case, the firmware sets up the CLKCFG register, so there is no need to set it manually after boot. Instead, I'm making sure the pch_uart driver defaults to what the firmware sets up.

Signature · 4 lines

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel

Quoted 3 lines

assume a 192 MHz clock on all boards. The problem with this approach is that the CLKCFG register may have been set to something other than the 192MHz configuration by the firmware.

So, I think default uart_clock 192MHz setting is better than Darren's opinion.

Quoted 3 lines

Quoted 3 lines

assume a 192 MHz clock on all boards. The problem with this approach is that the CLKCFG register may have been set to something other than the 192MHz configuration by the firmware.

So you can use the early PCI hooks or even bash the register directly in your early bootup code. You won't be the only early boot console that does this sort of thing. There are even people bitbanging PCI I²C interfaces at boot time for such purpose.

Quoted 1 line

So, I think default uart_clock 192MHz setting is better than Darren's opinion.

It's certainly easier to maintain, but it would be good to know if the setting can be written or retrieved directly in the early console setup using the early PCI ops or similar.