[PATCH 0/4] pch_uart: Cleanups, board quirks, and user uartclk parameter

[PATCH 0/4] pch_uart: Cleanups, board quirks, and user uartclk parameter

[Darren Hart]

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.

--
Darren

The following changes since commit 27e74da9800289e69ba907777df1e2085231eff7:

  i387: export 'fpu_owner_task' per-cpu variable (2012-02-20 19:34:10 -0800)

are available in the git repository at:
  git://git.infradead.org/users/dvhart/linux-2.6.git pch_uart
  http://git.infradead.org/users/dvhart/linux-2.6.git/shortlog/refs/heads/pch_uart
Darren Hart (4):
  pch_uart: Use uartclk instead of base_baud
  pch_uart: Add Fish River Island II uart clock quirks
  pch_uart: Add user_uartclk parameter
  pch_uart: Use existing default_baud in setup_console

 drivers/tty/serial/pch_uart.c |   52 +++++++++++++++++++++++++++++-----------
 1 files changed, 37 insertions(+), 15 deletions(-)

-- 
1.7.6.5

[PATCH 1/4] pch_uart: Use uartclk instead of base_baud

[Darren Hart]

The term "base baud" refers to the fastest baud rate the device can communicate
at. This is clock/16. pch_uart is using base_baud as the clock itself. Rename
the variables to be semantically correct.

Signed-off-by: Darren Hart <dvhart@linux.intel.com>
CC: Tomoya MORINAGA <tomoya.rohm@gmail.com>
CC: Feng Tang <feng.tang@intel.com>
CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
CC: Alan Cox <alan@linux.intel.com>
CC: linux-serial@vger.kernel.org
---
 drivers/tty/serial/pch_uart.c |   24 ++++++++++++------------
 1 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/tty/serial/pch_uart.c b/drivers/tty/serial/pch_uart.c
index 17ae657..c565817 100644
--- a/drivers/tty/serial/pch_uart.c
+++ b/drivers/tty/serial/pch_uart.c
@@ -203,7 +203,7 @@ enum {
 
 #define BOTH_EMPTY (UART_LSR_TEMT | UART_LSR_THRE)
 
-#define DEFAULT_BAUD_RATE 1843200 /* 1.8432MHz */
+#define DEFAULT_UARTCLK 1843200 /* 1.8432MHz */
 
 struct pch_uart_buffer {
 	unsigned char *buf;
@@ -218,7 +218,7 @@ struct eg20t_port {
 	unsigned int iobase;
 	struct pci_dev *pdev;
 	int fifo_size;
-	int base_baud;
+	int uartclk;
 	int start_tx;
 	int start_rx;
 	int tx_empty;
@@ -293,7 +293,7 @@ static const int trigger_level_16[4] = { 1, 4, 8, 14 };
 static const int trigger_level_1[4] = { 1, 1, 1, 1 };
 
 static void pch_uart_hal_request(struct pci_dev *pdev, int fifosize,
-				 int base_baud)
+				 int uartclk)
 {
 	struct eg20t_port *priv = pci_get_drvdata(pdev);
 
@@ -332,7 +332,7 @@ static int pch_uart_hal_set_line(struct eg20t_port *priv, int baud,
 	unsigned int dll, dlm, lcr;
 	int div;
 
-	div = DIV_ROUND_CLOSEST(priv->base_baud / 16, baud);
+	div = DIV_ROUND_CLOSEST(priv->uartclk / 16, baud);
 	if (div < 0 || USHRT_MAX <= div) {
 		dev_err(priv->port.dev, "Invalid Baud(div=0x%x)\n", div);
 		return -EINVAL;
@@ -1153,9 +1153,9 @@ static int pch_uart_startup(struct uart_port *port)
 	priv->tx_empty = 1;
 
 	if (port->uartclk)
-		priv->base_baud = port->uartclk;
+		priv->uartclk = port->uartclk;
 	else
-		port->uartclk = priv->base_baud;
+		port->uartclk = priv->uartclk;
 
 	pch_uart_hal_disable_interrupt(priv, PCH_UART_HAL_ALL_INT);
 	ret = pch_uart_hal_set_line(priv, default_baud,
@@ -1507,7 +1507,7 @@ static int __init pch_console_setup(struct console *co, char *options)
 		return -ENODEV;
 
 	/* setup uartclock */
-	port->uartclk = DEFAULT_BAUD_RATE;
+	port->uartclk = DEFAULT_UARTCLK;
 
 	if (options)
 		uart_parse_options(options, &baud, &parity, &bits, &flow);
@@ -1550,7 +1550,7 @@ static struct eg20t_port *pch_uart_init_port(struct pci_dev *pdev,
 	unsigned int iobase;
 	unsigned int mapbase;
 	unsigned char *rxbuf;
-	int fifosize, base_baud;
+	int fifosize, uartclk;
 	int port_type;
 	struct pch_uart_driver_data *board;
 	const char *board_name;
@@ -1566,12 +1566,12 @@ static struct eg20t_port *pch_uart_init_port(struct pci_dev *pdev,
 	if (!rxbuf)
 		goto init_port_free_txbuf;
 
-	base_baud = DEFAULT_BAUD_RATE;
+	uartclk = DEFAULT_UARTCLK;
 
 	/* quirk for CM-iTC board */
 	board_name = dmi_get_system_info(DMI_BOARD_NAME);
 	if (board_name && strstr(board_name, "CM-iTC"))
-		base_baud = 192000000; /* 192.0MHz */
+		uartclk = 192000000; /* 192.0MHz */
 
 	switch (port_type) {
 	case PORT_UNKNOWN:
@@ -1597,7 +1597,7 @@ static struct eg20t_port *pch_uart_init_port(struct pci_dev *pdev,
 	priv->rxbuf.size = PAGE_SIZE;
 
 	priv->fifo_size = fifosize;
-	priv->base_baud = base_baud;
+	priv->uartclk = uartclk;
 	priv->port_type = PORT_MAX_8250 + port_type + 1;
 	priv->port.dev = &pdev->dev;
 	priv->port.iobase = iobase;
@@ -1614,7 +1614,7 @@ static struct eg20t_port *pch_uart_init_port(struct pci_dev *pdev,
 	spin_lock_init(&priv->port.lock);
 
 	pci_set_drvdata(pdev, priv);
-	pch_uart_hal_request(pdev, fifosize, base_baud);
+	pch_uart_hal_request(pdev, fifosize, uartclk);
 
 #ifdef CONFIG_SERIAL_PCH_UART_CONSOLE
 	pch_uart_ports[board->line_no] = priv;
-- 
1.7.6.5

[PATCH 2/4] pch_uart: Add Fish River Island II uart clock quirks

[Darren Hart]

Add support for the Fish River Island II (FRI2) 64MHz UART clock following
the CM-iTC quirk handling mechanism.

Add similar UART clock quirk handling to the pch_console_setup() function
to enable kernel messages on boards with non-standard UART clocks.

Signed-off-by: Darren Hart <dvhart@linux.intel.com>
CC: Tomoya MORINAGA <tomoya.rohm@gmail.com>
CC: Feng Tang <feng.tang@intel.com>
CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
CC: Alan Cox <alan@linux.intel.com>
CC: linux-serial@vger.kernel.org
---
 drivers/tty/serial/pch_uart.c |   28 +++++++++++++++++++++++-----
 1 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/serial/pch_uart.c b/drivers/tty/serial/pch_uart.c
index c565817..b070a4a 100644
--- a/drivers/tty/serial/pch_uart.c
+++ b/drivers/tty/serial/pch_uart.c
@@ -203,7 +203,9 @@ enum {
 
 #define BOTH_EMPTY (UART_LSR_TEMT | UART_LSR_THRE)
 
-#define DEFAULT_UARTCLK 1843200 /* 1.8432MHz */
+#define DEFAULT_UARTCLK  1843200 /*   1.8432 MHz */
+#define CMITC_UARTCLK  192000000 /* 192.0000 MHz */
+#define FRI2_UARTCLK    64000000 /*  64.0000 MHz */
 
 struct pch_uart_buffer {
 	unsigned char *buf;
@@ -1488,11 +1490,13 @@ pch_console_write(struct console *co, const char *s, unsigned int count)
 
 static int __init pch_console_setup(struct console *co, char *options)
 {
+	const char *board_name;
 	struct uart_port *port;
 	int baud = 9600;
 	int bits = 8;
 	int parity = 'n';
 	int flow = 'n';
+	int uartclk;
 
 	/*
 	 * Check whether an invalid uart number has been specified, and
@@ -1506,8 +1510,18 @@ static int __init pch_console_setup(struct console *co, char *options)
 	if (!port || (!port->iobase && !port->membase))
 		return -ENODEV;
 
-	/* setup uartclock */
-	port->uartclk = DEFAULT_UARTCLK;
+	/* 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;
 
 	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;
 
-	/* 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;
 
 	switch (port_type) {
 	case PORT_UNKNOWN:
-- 
1.7.6.5

[PATCH 3/4] pch_uart: Add user_uartclk parameter

[Darren Hart]

For cases where boards with non-default clocks are not yet added to the kernel
or when the clock varies across hardware revisions, it is useful to be
able to specify the UART clock on the kernel command line.

Add the user_uartclk parameter and prefer it, if set, to the default and
board specific UART clock settings. Specify user_uartclock on the command-line
with "pch_uart.user_uartclk=48000000".

Signed-off-by: Darren Hart <dvhart@linux.intel.com>
CC: Tomoya MORINAGA <tomoya.rohm@gmail.com>
CC: Feng Tang <feng.tang@intel.com>
CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
CC: Alan Cox <alan@linux.intel.com>
CC: linux-serial@vger.kernel.org
---
 drivers/tty/serial/pch_uart.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/drivers/tty/serial/pch_uart.c b/drivers/tty/serial/pch_uart.c
index b070a4a..d00b75c 100644
--- a/drivers/tty/serial/pch_uart.c
+++ b/drivers/tty/serial/pch_uart.c
@@ -289,6 +289,7 @@ static struct pch_uart_driver_data drv_dat[] = {
 static struct eg20t_port *pch_uart_ports[PCH_UART_NR];
 #endif
 static unsigned int default_baud = 9600;
+static unsigned int user_uartclk = 0;
 static const int trigger_level_256[4] = { 1, 64, 128, 224 };
 static const int trigger_level_64[4] = { 1, 16, 32, 56 };
 static const int trigger_level_16[4] = { 1, 4, 8, 14 };
@@ -1521,7 +1522,7 @@ static int __init pch_console_setup(struct console *co, char *options)
 	if (board_name && strstr(board_name, "Fish River Island II"))
 		uartclk = FRI2_UARTCLK;
 
-	port->uartclk = uartclk;
+	port->uartclk = user_uartclk ? user_uartclk : uartclk;
 
 	if (options)
 		uart_parse_options(options, &baud, &parity, &bits, &flow);
@@ -1591,6 +1592,8 @@ static struct eg20t_port *pch_uart_init_port(struct pci_dev *pdev,
 	if (board_name && strstr(board_name, "Fish River Island II"))
 		uartclk = FRI2_UARTCLK;
 
+	uartclk = user_uartclk ? user_uartclk : uartclk;
+
 	switch (port_type) {
 	case PORT_UNKNOWN:
 		fifosize = 256; /* EG20T/ML7213: UART0 */
@@ -1803,3 +1806,4 @@ module_exit(pch_uart_module_exit);
 MODULE_LICENSE("GPL v2");
 MODULE_DESCRIPTION("Intel EG20T PCH UART PCI Driver");
 module_param(default_baud, uint, S_IRUGO);
+module_param(user_uartclk, uint, S_IRUGO);
-- 
1.7.6.5

[PATCH 4/4] pch_uart: Use existing default_baud in setup_console

[Darren Hart]

Rather than hardcode 9600, use the existing default_baud parameter (which
also defaults to 9600).

Signed-off-by: Darren Hart <dvhart@linux.intel.com>
CC: Tomoya MORINAGA <tomoya.rohm@gmail.com>
CC: Feng Tang <feng.tang@intel.com>
CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
CC: Alan Cox <alan@linux.intel.com>
CC: linux-serial@vger.kernel.org
---
 drivers/tty/serial/pch_uart.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/tty/serial/pch_uart.c b/drivers/tty/serial/pch_uart.c
index d00b75c..c2c1bac 100644
--- a/drivers/tty/serial/pch_uart.c
+++ b/drivers/tty/serial/pch_uart.c
@@ -1493,7 +1493,7 @@ static int __init pch_console_setup(struct console *co, char *options)
 {
 	const char *board_name;
 	struct uart_port *port;
-	int baud = 9600;
+	int baud = default_baud;
 	int bits = 8;
 	int parity = 'n';
 	int flow = 'n';
-- 
1.7.6.5

Re: [PATCH 0/4] pch_uart: Cleanups, board quirks, and user uartclk parameter

[Tomoya MORINAGA]

2012年2月22日10:59 Darren Hart <dvhart@linux.intel.com>:
> 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.

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.

thanks,

---
ROHM Co., Ltd.
tomoya

Re: [PATCH 0/4] pch_uart: Cleanups, board quirks, and user uartclk parameter

[Darren Hart]

On 02/21/2012 07:10 PM, Tomoya MORINAGA wrote:
> 2012年2月22日10:59 Darren Hart <dvhart@linux.intel.com>:
>> 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.
> 
> 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).

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

Re: [PATCH 0/4] pch_uart: Cleanups, board quirks, and user uartclk parameter

[Tomoya MORINAGA]

2012年2月22日12:36 Darren Hart <dvhart@linux.intel.com>:
> 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.

thanks
---
ROHM Co., Ltd.
tomoya

Re: [PATCH 0/4] pch_uart: Cleanups, board quirks, and user uartclk parameter

[Darren Hart]

On 02/21/2012 08:26 PM, Tomoya MORINAGA wrote:
> 2012年2月22日12:36 Darren Hart <dvhart@linux.intel.com>:
>> 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.

Ah, that's a good point. 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. Would you care to Ack this series and then follow-up
with a patch set the default clock to 192MHz?

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

Re: [PATCH 0/4] pch_uart: Cleanups, board quirks, and user uartclk parameter

[Tomoya MORINAGA]

2012年2月22日15:39 Darren Hart <dvhart@linux.intel.com>:
> 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?

---
ROHM Co., Ltd.
tomoya

Re: [PATCH 2/4] pch_uart: Add Fish River Island II uart clock quirks

[Alan Cox]

> +	/* 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 ?

>  
>  	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;
>  
> -	/* 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 ?

Alan

Re: [PATCH 0/4] pch_uart: Cleanups, board quirks, and user uartclk parameter

[Alan Cox]

> > 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.

> 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.

Alan

Alan

Re: [PATCH 0/4] pch_uart: Cleanups, board quirks, and user uartclk parameter

[Darren Hart]

On 02/22/2012 12:16 AM, Tomoya MORINAGA wrote:
> 2012年2月22日15:39 Darren Hart <dvhart@linux.intel.com>:
>> 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?

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.

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

Re: [PATCH 0/4] pch_uart: Cleanups, board quirks, and user uartclk parameter

[Feng Tang]

On Wed, Feb 22, 2012 at 12:59:03AM -0800, Darren Hart wrote:
> 
> 
> On 02/22/2012 12:16 AM, Tomoya MORINAGA wrote:
> > 2012年2月22日15:39 Darren Hart <dvhart@linux.intel.com>:
> >> 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?
> 
> 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.

I think long term wise, we should suggest BIOS vendor to set the UART clk
to 192MHz in chipset's release notes, since 192MHz works on Darren's,
Tomoya's and my boards.

Thanks,
Feng

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

Re: [PATCH 2/4] pch_uart: Add Fish River Island II uart clock quirks

[Darren Hart]

On 02/22/2012 12:52 AM, Alan Cox wrote:
>> +	/* 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 ?

OK, done.

> 
>>  
>>  	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;
>>  
>> -	/* 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 ?

Right, bad dvhart. Done. That cleans things up nicely. I'll resend as V2
after considering your 0/4 response....

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

Re: [PATCH 0/4] pch_uart: Cleanups, board quirks, and user uartclk parameter

[Darren Hart]

On 02/22/2012 12:58 AM, Alan Cox wrote:
>>> 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.
> 
>> 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.

OK, I'm not opposed to forcing everything to 192MHz, that would clean up
pch_uart.c quite a bit. I have heard different things about the
specification for this chipset. One statement was that 64MHz was the
maximum UART clock. Feng suggests that 192MHz is the recommended UART
clock. I need to dig up this spec and determine what it actually says.

I have V2 with Alan's feedback from 2/4 incorporated, but I'll hold off
unless people want to see it now. Seems like it will change a lot if we
force 192MHz everywhere.

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

Re: [PATCH 2/4] pch_uart: Add Fish River Island II uart clock quirks

[Greg Kroah-Hartman]

On Wed, Feb 22, 2012 at 01:46:06AM -0800, Darren Hart wrote:
> 
> 
> On 02/22/2012 12:52 AM, Alan Cox wrote:
> >> +	/* 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 ?
> 
> 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 :)

greg k-h

Re: [PATCH 2/4] pch_uart: Add Fish River Island II uart clock quirks

[Darren Hart]

On 02/24/2012 01:53 PM, Greg Kroah-Hartman wrote:
> On Wed, Feb 22, 2012 at 01:46:06AM -0800, Darren Hart wrote:
>>
>>
>> On 02/22/2012 12:52 AM, Alan Cox wrote:
>>>> +	/* 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 ?
>>
>> 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.
-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel

Re: [PATCH 2/4] pch_uart: Add Fish River Island II uart clock quirks

[Greg Kroah-Hartman]

On Fri, Feb 24, 2012 at 02:25:47PM -0800, Darren Hart wrote:
> 
> 
> On 02/24/2012 01:53 PM, Greg Kroah-Hartman wrote:
> > On Wed, Feb 22, 2012 at 01:46:06AM -0800, Darren Hart wrote:
> >>
> >>
> >> On 02/22/2012 12:52 AM, Alan Cox wrote:
> >>>> +	/* 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 ?
> >>
> >> 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.

A big stick is usually best to use on the firmware engineers...

Good luck,

greg k-h

Re: [PATCH 3/4] pch_uart: Add user_uartclk parameter

[Darren Hart]

On 03/08/2012 10:56 AM, Greg Kroah-Hartman wrote:
> On Wed, Feb 29, 2012 at 10:24:46AM -0800, Darren Hart wrote:
>> For cases where boards with non-default clocks are not yet added to the kernel
>> or when the clock varies across hardware revisions, it is useful to be
>> able to specify the UART clock on the kernel command line.
>>
>> Add the user_uartclk parameter and prefer it, if set, to the default and
>> board specific UART clock settings. Specify user_uartclock on the command-line
>> with "pch_uart.user_uartclk=48000000".
>>
>> Signed-off-by: Darren Hart <dvhart@linux.intel.com>
>> CC: Tomoya MORINAGA <tomoya.rohm@gmail.com>
>> CC: Feng Tang <feng.tang@intel.com>
>> CC: Alan Cox <alan@linux.intel.com>
>> ---
>>  drivers/tty/serial/pch_uart.c |    5 +++++
>>  1 files changed, 5 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/tty/serial/pch_uart.c b/drivers/tty/serial/pch_uart.c
>> index 3a2b0ae..105d982 100644
>> --- a/drivers/tty/serial/pch_uart.c
>> +++ b/drivers/tty/serial/pch_uart.c
>> @@ -290,6 +290,7 @@ static struct pch_uart_driver_data drv_dat[] = {
>>  static struct eg20t_port *pch_uart_ports[PCH_UART_NR];
>>  #endif
>>  static unsigned int default_baud = 9600;
>> +static unsigned int user_uartclk = 0;
>>  static const int trigger_level_256[4] = { 1, 64, 128, 224 };
>>  static const int trigger_level_64[4] = { 1, 16, 32, 56 };
>>  static const int trigger_level_16[4] = { 1, 4, 8, 14 };
>> @@ -300,6 +301,9 @@ static int pch_uart_get_uartclk(void)
>>  {
>>  	const char *cmp;
>>  
>> +	if (user_uartclk)
>> +		return user_uartclk;
>> +
>>  	cmp = dmi_get_system_info(DMI_BOARD_NAME);
>>  	if (cmp && strstr(cmp, "CM-iTC"))
>>  		return CMITC_UARTCLK;
>> @@ -1799,3 +1803,4 @@ module_exit(pch_uart_module_exit);
>>  MODULE_LICENSE("GPL v2");
>>  MODULE_DESCRIPTION("Intel EG20T PCH UART PCI Driver");
>>  module_param(default_baud, uint, S_IRUGO);
>> +module_param(user_uartclk, uint, S_IRUGO);
> 
> I don't object to this, but you should provide some documentation on
> what this option does.  How about a follow-on patch using
> MODULE_PARM_DESC() for both of these options to help people out with how
> they should be used?

Will do (now I'm traveling, so might be a day). Will include with the
rebase to linux-next.

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

Re: [PATCH 3/4] pch_uart: Add user_uartclk parameter

[Greg Kroah-Hartman]

On Wed, Feb 29, 2012 at 10:24:46AM -0800, Darren Hart wrote:
> For cases where boards with non-default clocks are not yet added to the kernel
> or when the clock varies across hardware revisions, it is useful to be
> able to specify the UART clock on the kernel command line.
> 
> Add the user_uartclk parameter and prefer it, if set, to the default and
> board specific UART clock settings. Specify user_uartclock on the command-line
> with "pch_uart.user_uartclk=48000000".
> 
> Signed-off-by: Darren Hart <dvhart@linux.intel.com>
> CC: Tomoya MORINAGA <tomoya.rohm@gmail.com>
> CC: Feng Tang <feng.tang@intel.com>
> CC: Alan Cox <alan@linux.intel.com>
> ---
>  drivers/tty/serial/pch_uart.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/tty/serial/pch_uart.c b/drivers/tty/serial/pch_uart.c
> index 3a2b0ae..105d982 100644
> --- a/drivers/tty/serial/pch_uart.c
> +++ b/drivers/tty/serial/pch_uart.c
> @@ -290,6 +290,7 @@ static struct pch_uart_driver_data drv_dat[] = {
>  static struct eg20t_port *pch_uart_ports[PCH_UART_NR];
>  #endif
>  static unsigned int default_baud = 9600;
> +static unsigned int user_uartclk = 0;
>  static const int trigger_level_256[4] = { 1, 64, 128, 224 };
>  static const int trigger_level_64[4] = { 1, 16, 32, 56 };
>  static const int trigger_level_16[4] = { 1, 4, 8, 14 };
> @@ -300,6 +301,9 @@ static int pch_uart_get_uartclk(void)
>  {
>  	const char *cmp;
>  
> +	if (user_uartclk)
> +		return user_uartclk;
> +
>  	cmp = dmi_get_system_info(DMI_BOARD_NAME);
>  	if (cmp && strstr(cmp, "CM-iTC"))
>  		return CMITC_UARTCLK;
> @@ -1799,3 +1803,4 @@ module_exit(pch_uart_module_exit);
>  MODULE_LICENSE("GPL v2");
>  MODULE_DESCRIPTION("Intel EG20T PCH UART PCI Driver");
>  module_param(default_baud, uint, S_IRUGO);
> +module_param(user_uartclk, uint, S_IRUGO);

I don't object to this, but you should provide some documentation on
what this option does.  How about a follow-on patch using
MODULE_PARM_DESC() for both of these options to help people out with how
they should be used?

thanks,

greg k-h

[PATCH 3/4] pch_uart: Add user_uartclk parameter

[Darren Hart]

For cases where boards with non-default clocks are not yet added to the kernel
or when the clock varies across hardware revisions, it is useful to be
able to specify the UART clock on the kernel command line.

Add the user_uartclk parameter and prefer it, if set, to the default and
board specific UART clock settings. Specify user_uartclock on the command-line
with "pch_uart.user_uartclk=48000000".

Signed-off-by: Darren Hart <dvhart@linux.intel.com>
CC: Tomoya MORINAGA <tomoya.rohm@gmail.com>
CC: Feng Tang <feng.tang@intel.com>
CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
CC: Alan Cox <alan@linux.intel.com>
CC: linux-serial@vger.kernel.org
---
 drivers/tty/serial/pch_uart.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/tty/serial/pch_uart.c b/drivers/tty/serial/pch_uart.c
index 3a2b0ae..105d982 100644
--- a/drivers/tty/serial/pch_uart.c
+++ b/drivers/tty/serial/pch_uart.c
@@ -290,6 +290,7 @@ static struct pch_uart_driver_data drv_dat[] = {
 static struct eg20t_port *pch_uart_ports[PCH_UART_NR];
 #endif
 static unsigned int default_baud = 9600;
+static unsigned int user_uartclk = 0;
 static const int trigger_level_256[4] = { 1, 64, 128, 224 };
 static const int trigger_level_64[4] = { 1, 16, 32, 56 };
 static const int trigger_level_16[4] = { 1, 4, 8, 14 };
@@ -300,6 +301,9 @@ static int pch_uart_get_uartclk(void)
 {
 	const char *cmp;
 
+	if (user_uartclk)
+		return user_uartclk;
+
 	cmp = dmi_get_system_info(DMI_BOARD_NAME);
 	if (cmp && strstr(cmp, "CM-iTC"))
 		return CMITC_UARTCLK;
@@ -1799,3 +1803,4 @@ module_exit(pch_uart_module_exit);
 MODULE_LICENSE("GPL v2");
 MODULE_DESCRIPTION("Intel EG20T PCH UART PCI Driver");
 module_param(default_baud, uint, S_IRUGO);
+module_param(user_uartclk, uint, S_IRUGO);
-- 
1.7.6.5