2012-02-22 02:00:10

by Darren Hart

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

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


2012-02-22 02:00:26

by Darren Hart

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

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 <[email protected]>
CC: Tomoya MORINAGA <[email protected]>
CC: Feng Tang <[email protected]>
CC: Greg Kroah-Hartman <[email protected]>
CC: Alan Cox <[email protected]>
CC: [email protected]
---
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

2012-02-22 02:00:24

by Darren Hart

[permalink] [raw]
Subject: [PATCH 3/4] pch_uart: Add user_uartclk parameter

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 <[email protected]>
CC: Tomoya MORINAGA <[email protected]>
CC: Feng Tang <[email protected]>
CC: Greg Kroah-Hartman <[email protected]>
CC: Alan Cox <[email protected]>
CC: [email protected]
---
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

2012-02-22 02:00:22

by Darren Hart

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

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 <[email protected]>
CC: Tomoya MORINAGA <[email protected]>
CC: Feng Tang <[email protected]>
CC: Greg Kroah-Hartman <[email protected]>
CC: Alan Cox <[email protected]>
CC: [email protected]
---
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

2012-02-22 02:00:20

by Darren Hart

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

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

Signed-off-by: Darren Hart <[email protected]>
CC: Tomoya MORINAGA <[email protected]>
CC: Feng Tang <[email protected]>
CC: Greg Kroah-Hartman <[email protected]>
CC: Alan Cox <[email protected]>
CC: [email protected]
---
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

2012-02-22 03:10:13

by Tomoya MORINAGA

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

2012$BG/(B2$B7n(B22$BF|(B10:59 Darren Hart <[email protected]>:
> 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

2012-02-22 03:37:02

by Darren Hart

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



On 02/21/2012 07:10 PM, Tomoya MORINAGA wrote:
> 2012$BG/(B2$B7n(B22$BF|(B10:59 Darren Hart <[email protected]>:
>> 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

2012-02-22 04:26:38

by Tomoya MORINAGA

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

2012$BG/(B2$B7n(B22$BF|(B12:36 Darren Hart <[email protected]>:
> 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

2012-02-22 06:40:16

by Darren Hart

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



On 02/21/2012 08:26 PM, Tomoya MORINAGA wrote:
> 2012$BG/(B2$B7n(B22$BF|(B12:36 Darren Hart <[email protected]>:
>> 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

2012-02-22 08:16:23

by Tomoya MORINAGA

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

2012$BG/(B2$B7n(B22$BF|(B15:39 Darren Hart <[email protected]>:
> 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

2012-02-22 08:51:11

by Alan

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

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

2012-02-22 08:56:46

by Alan

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

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

2012-02-22 08:59:41

by Darren Hart

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



On 02/22/2012 12:16 AM, Tomoya MORINAGA wrote:
> 2012$BG/(B2$B7n(B22$BF|(B15:39 Darren Hart <[email protected]>:
>> 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

2012-02-22 09:26:54

by Feng Tang

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

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 <[email protected]>:
> >> 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

2012-02-22 09:46:44

by Darren Hart

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



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

2012-02-22 09:56:19

by Darren Hart

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



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

2012-02-24 21:55:51

by Greg Kroah-Hartman

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

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

2012-02-24 22:26:22

by Darren Hart

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



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

2012-02-24 23:43:52

by Greg Kroah-Hartman

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

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