[PATCH] misc/pch_phub: Enable UART clock setting by module parameter

[PATCH] misc/pch_phub: Enable UART clock setting by module parameter

[Tomoya MORINAGA]

Currently, when a user wants to change UART clock,
needs to modify this source code by hand.
This patch enables changing UART clock by specifying UART clock
as module parameter.

Signed-off-by: Tomoya MORINAGA <tomoya.rohm@gmail.com>
---
 drivers/misc/pch_phub.c |   45 +++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 45 insertions(+), 0 deletions(-)

diff --git a/drivers/misc/pch_phub.c b/drivers/misc/pch_phub.c
index 9fbcacd..2e7b0b0 100644
--- a/drivers/misc/pch_phub.c
+++ b/drivers/misc/pch_phub.c
@@ -93,6 +93,18 @@
 
 #define PCH_PHUB_OROM_SIZE 15360
 
+#define CLKCFG_BAUDDIV2				(2 << 20)
+#define CLKCFG_BAUDDIV6				(6 << 20)
+#define CLKCFG_PLL2VCO8				(8 << 9)
+
+#define PCH_PHUB_REFCLK_48MHZ	CLKCFG_UART_48MHZ
+
+#define PCH_PHUB_REFCLK_64MHZ	(CLKCFG_UART_48MHZ | CLKCFG_BAUDDIV6 |\
+				CLKCFG_PLL2VCO8 | CLKCFG_UARTCLKSEL)
+
+#define PCH_PHUB_REFCLK_192MHZ	(CLKCFG_UART_48MHZ | CLKCFG_BAUDDIV2 |\
+				CLKCFG_PLL2VCO8 | CLKCFG_UARTCLKSEL)
+
 /**
  * struct pch_phub_reg - PHUB register structure
  * @phub_id_reg:			PHUB_ID register val
@@ -142,6 +154,8 @@ struct pch_phub_reg {
 /* SROM SPEC for MAC address assignment offset */
 static const int pch_phub_mac_offset[ETH_ALEN] = {0x3, 0x2, 0x1, 0x0, 0xb, 0xa};
 
+static unsigned int select_uart_clk;
+
 static DEFINE_MUTEX(pch_phub_mutex);
 
 /**
@@ -710,6 +724,34 @@ static int __devinit pch_phub_probe(struct pci_dev *pdev,
 
 	chip->pdev = pdev; /* Save pci device struct */
 
+	/* Reference clock setting. ML7223 bus-m doesn't have clock register. */
+	if (select_uart_clk && id->driver_data != 3) {
+		unsigned int clk_val;
+		switch (select_uart_clk) {
+		case 48000000:
+		case 48:
+			clk_val = PCH_PHUB_REFCLK_48MHZ;
+			break;
+		case 64000000:
+		case 64:
+			clk_val = PCH_PHUB_REFCLK_64MHZ;
+			break;
+		case 192000000:
+		case 192:
+			clk_val = PCH_PHUB_REFCLK_192MHZ;
+			break;
+		default:
+			dev_err(&pdev->dev,
+				"%s : Invalid clock(Only 48/64/192MHz).\n",
+				__func__);
+			goto err_sysfs_create;
+		}
+		pch_phub_read_modify_write_reg(chip,
+					      (unsigned int)CLKCFG_REG_OFFSET,
+					       clk_val, CLKCFG_UART_MASK);
+	}
+
+
 	if (id->driver_data == 1) { /* EG20T PCH */
 		const char *board_name;
 
@@ -908,3 +950,6 @@ module_exit(pch_phub_pci_exit);
 
 MODULE_DESCRIPTION("Intel EG20T PCH/LAPIS Semiconductor IOH(ML7213/ML7223) PHUB");
 MODULE_LICENSE("GPL");
+module_param(select_uart_clk, uint, S_IRUGO);
+MODULE_PARM_DESC(select_uart_clk,
+		 "Select clock for UART (48/64/192MHz). default is 1.8342MHz");
-- 
1.7.4.4

Re: [PATCH] misc/pch_phub: Enable UART clock setting by module parameter

[Arnd Bergmann]

On Wednesday 11 July 2012, Tomoya MORINAGA wrote:
> Currently, when a user wants to change UART clock,
> needs to modify this source code by hand.
> This patch enables changing UART clock by specifying UART clock
> as module parameter.
> 
> Signed-off-by: Tomoya MORINAGA <tomoya.rohm@gmail.com>

This looks like a rather nonscalable solution if you get to systems
with lots of clocks.

Given that you are doing it for the uart clock, shouldn't that be
set from the uart driver using an ioctl like other serial ports do?

What would be the use case for an end user to override the module
parameter? Is it about platform specific settings or policy?

	Arnd

Re: [PATCH] misc/pch_phub: Enable UART clock setting by module parameter

[Tomoya MORINAGA]

On Wed, Jul 11, 2012 at 7:45 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> This looks like a rather nonscalable solution if you get to systems
> with lots of clocks.

This "clock" is internal clock, not external clock.
This PacketHub provides clock to the UART module
Both the PacketHub and the UART is in 1 chip LSI which is EG20T.
So, selectable clock 1.8432MHz or 48MHz or 64MHz or 192MHz are enough.

> Given that you are doing it for the uart clock, shouldn't that be
> set from the uart driver using an ioctl like other serial ports do?
PacketHub is not serial driver but special driver. So, ioctl doesn't
suit PacketHub.

> What would be the use case for an end user to override the module
> parameter? Is it about platform specific settings or policy?
I show use case.
Currently, UART works with 1.8432MHz.
Using this clock, as you know, maximum speed is 115k.
A user wants to use 4M speed, the user need to modify pch_phun.c by hand.
If this patch is applied, a user can specify uart_clock via a modules
parameter and use 4M speed.

My reference driver for this patch is drivers/tty/serial/pch_uart.c
This driver can set uart_clock via a module parameter(user_uartclk).

Thanks.
-- 
ROHM Co., Ltd.
tomoya

Re: [PATCH] misc/pch_phub: Enable UART clock setting by module parameter

[Tomoya MORINAGA]

Hi Arnd,

Let me know this patch status.
If you have still any concern, let me know.

BTW, now I remember, Did you take part in LinuxConJapan last month ?
I also took part in this event as volunteer staff.
Additionally, I took charge of your session as time keeper.

Thanks in advance.
-- 
ROHM Co., Ltd.
tomoya

On Thu, Jul 12, 2012 at 9:54 AM, Tomoya MORINAGA <tomoya.rohm@gmail.com> wrote:
> On Wed, Jul 11, 2012 at 7:45 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> This looks like a rather nonscalable solution if you get to systems
>> with lots of clocks.
>
> This "clock" is internal clock, not external clock.
> This PacketHub provides clock to the UART module
> Both the PacketHub and the UART is in 1 chip LSI which is EG20T.
> So, selectable clock 1.8432MHz or 48MHz or 64MHz or 192MHz are enough.
>
>> Given that you are doing it for the uart clock, shouldn't that be
>> set from the uart driver using an ioctl like other serial ports do?
> PacketHub is not serial driver but special driver. So, ioctl doesn't
> suit PacketHub.
>
>> What would be the use case for an end user to override the module
>> parameter? Is it about platform specific settings or policy?
> I show use case.
> Currently, UART works with 1.8432MHz.
> Using this clock, as you know, maximum speed is 115k.
> A user wants to use 4M speed, the user need to modify pch_phun.c by hand.
> If this patch is applied, a user can specify uart_clock via a modules
> parameter and use 4M speed.
>
> My reference driver for this patch is drivers/tty/serial/pch_uart.c
> This driver can set uart_clock via a module parameter(user_uartclk).
>
> Thanks.
> --
> ROHM Co., Ltd.
> tomoya

Re: [PATCH] misc/pch_phub: Enable UART clock setting by module parameter

[Arnd Bergmann]

Hi Tomoya,

On Tuesday 24 July 2012, Tomoya MORINAGA wrote:
> Let me know this patch status.
> If you have still any concern, let me know.

Sorry for the late reply.

> BTW, now I remember, Did you take part in LinuxConJapan last month ?
> I also took part in this event as volunteer staff.
> Additionally, I took charge of your session as time keeper.
> 
> Thanks in advance.
> -- 
> ROHM Co., Ltd.
> tomoya
> 
> On Thu, Jul 12, 2012 at 9:54 AM, Tomoya MORINAGA <tomoya.rohm@gmail.com> wrote:
> > On Wed, Jul 11, 2012 at 7:45 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> >> This looks like a rather nonscalable solution if you get to systems
> >> with lots of clocks.
> >
> > This "clock" is internal clock, not external clock.
> > This PacketHub provides clock to the UART module
> > Both the PacketHub and the UART is in 1 chip LSI which is EG20T.
> > So, selectable clock 1.8432MHz or 48MHz or 64MHz or 192MHz are enough.

Right, I got this part.

> >> Given that you are doing it for the uart clock, shouldn't that be
> >> set from the uart driver using an ioctl like other serial ports do?
> > PacketHub is not serial driver but special driver. So, ioctl doesn't
> > suit PacketHub.
> >
> >> What would be the use case for an end user to override the module
> >> parameter? Is it about platform specific settings or policy?
> > I show use case.
> > Currently, UART works with 1.8432MHz.
> > Using this clock, as you know, maximum speed is 115k.
> > A user wants to use 4M speed, the user need to modify pch_phun.c by hand.
> > If this patch is applied, a user can specify uart_clock via a modules
> > parameter and use 4M speed.
> >
> > My reference driver for this patch is drivers/tty/serial/pch_uart.c
> > This driver can set uart_clock via a module parameter(user_uartclk).

It's clear that modifying the source code is not a good solution, so
I agree something should be done about it.

What I think should work better here would be to use the clk API,
so that the phub driver registers a 'struct clk' using 
(I assume) clk_register_divider_table().
The UART driver would then call clk_get() to find that clk for
the uart device and call clk_set_rate when it needs to change
that clock in order to set a different baud rate.

Does this make sense?

	Arnd

Re: [PATCH] misc/pch_phub: Enable UART clock setting by module parameter

[Tomoya MORINAGA]

On Wed, Jul 25, 2012 at 10:31 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> What I think should work better here would be to use the clk API,
> so that the phub driver registers a 'struct clk' using
> (I assume) clk_register_divider_table().
> The UART driver would then call clk_get() to find that clk for
> the uart device and call clk_set_rate when it needs to change
> that clock in order to set a different baud rate.
>
> Does this make sense?
>

Thank you for your comments. This sounds good idea.
However, it seems the latest kernel doesn't have clk_register_divider_table().
Will the clk_register_divider_table function be applied soon ?

Thanks.

-- 
ROHM Co., Ltd.
tomoya

Re: [PATCH] misc/pch_phub: Enable UART clock setting by module parameter

[Arnd Bergmann]

On Tuesday 31 July 2012, Tomoya MORINAGA wrote:
> 
> On Wed, Jul 25, 2012 at 10:31 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > What I think should work better here would be to use the clk API,
> > so that the phub driver registers a 'struct clk' using
> > (I assume) clk_register_divider_table().
> > The UART driver would then call clk_get() to find that clk for
> > the uart device and call clk_set_rate when it needs to change
> > that clock in order to set a different baud rate.
> >
> > Does this make sense?
> >
> 
> Thank you for your comments. This sounds good idea.
> However, it seems the latest kernel doesn't have clk_register_divider_table().
> Will the clk_register_divider_table function be applied soon ?

The code was just merged for v3.6, so it will be in all future kernels that
would also see your changes, but not in older kernels in case you want to
backport your patch.

	Arnd