Re: [PATCH] serial: 8250_dw: set clock rate

2013-05-09 Thread James Hogan 
On 09/05/13 11:29, James Hogan wrote:
> If the uart clock provided to the 8250_dw driver is adjustable it may
> not be set to the desired rate. Therefore if both a uart clock and a
> clock frequency is specified (e.g. via device tree), try and update the
> clock to match the frequency.
> 
> Unfortunately if the resulting frequency is rounded down (which is the
> default behaviour of the generic clk-divider), the 8250 core won't allow
> the highest baud rate to be used, so if an explicit frequency is
> specified we always report that to the 8250 core.

Hi,

Sorry, please ignore this patch.

I've realised that a larger (e.g. non-divided) source clock can just be
provided directly to the uart and it appears to have enough of an
internal divider for the driver to do the right thing without changing
the input clock rate, although of course that clock rate still needs
setting somewhere. I'm not sure why we didn't do this all along as it
gives a closer frequency anyway.

Thanks
James

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] serial: 8250_dw: set clock rate

2013-05-09 Thread James Hogan 
If the uart clock provided to the 8250_dw driver is adjustable it may
not be set to the desired rate. Therefore if both a uart clock and a
clock frequency is specified (e.g. via device tree), try and update the
clock to match the frequency.

Unfortunately if the resulting frequency is rounded down (which is the
default behaviour of the generic clk-divider), the 8250 core won't allow
the highest baud rate to be used, so if an explicit frequency is
specified we always report that to the 8250 core.

The device tree bindings document is also updated accordingly.

Signed-off-by: James Hogan 
Cc: Grant Likely 
Cc: Rob Herring 
Cc: Rob Landley 
Cc: Greg Kroah-Hartman 
Cc: Jiri Slaby 
Cc: Heikki Krogerus 
Cc: Alan Cox 
Cc: Jamie Iles 
Cc: Bill Pemberton 
---
 .../bindings/tty/serial/snps-dw-apb-uart.txt   |  4 +-
 drivers/tty/serial/8250/8250_dw.c  | 47 +-
 2 files changed, 39 insertions(+), 12 deletions(-)

diff --git a/Documentation/devicetree/bindings/tty/serial/snps-dw-apb-uart.txt 
b/Documentation/devicetree/bindings/tty/serial/snps-dw-apb-uart.txt
index f13f1c5..e0cfc47 100644
--- a/Documentation/devicetree/bindings/tty/serial/snps-dw-apb-uart.txt
+++ b/Documentation/devicetree/bindings/tty/serial/snps-dw-apb-uart.txt
@@ -4,9 +4,11 @@ Required properties:
 - compatible : "snps,dw-apb-uart"
 - reg : offset and length of the register set for the device.
 - interrupts : should contain uart interrupt.
-- clock-frequency : the input clock frequency for the UART.
 
 Optional properties:
+- clock-frequency : the input clock frequency for the UART. If specified in
+  addition to clocks, the clock rate will be set to this frequency.
+- clocks : the input clock specifier for the UART.
 - reg-shift : quantity to shift the register offsets by.  If this property is
   not present then the register offsets are not shifted.
 - reg-io-width : the size (in bytes) of the IO accesses that should be
diff --git a/drivers/tty/serial/8250/8250_dw.c 
b/drivers/tty/serial/8250/8250_dw.c
index beaa283..85b3b63 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -128,6 +128,7 @@ dw8250_do_pm(struct uart_port *port, unsigned int state, 
unsigned int old)
 static int dw8250_probe_of(struct uart_port *p)
 {
struct device_node  *np = p->dev->of_node;
+   struct dw8250_data  *d = p->private_data;
u32 val;
 
if (!of_property_read_u32(np, "reg-io-width", )) {
@@ -148,16 +149,13 @@ static int dw8250_probe_of(struct uart_port *p)
if (!of_property_read_u32(np, "reg-shift", ))
p->regshift = val;
 
-   /* clock got configured through clk api, all done */
-   if (p->uartclk)
-   return 0;
-
-   /* try to find out clock frequency from DT as fallback */
-   if (of_property_read_u32(np, "clock-frequency", )) {
+   /* try to find out clock frequency from DT */
+   if (!of_property_read_u32(np, "clock-frequency", )) {
+   p->uartclk = val;
+   } else if (IS_ERR(d->clk)) {
dev_err(p->dev, "clk or clock-frequency not defined\n");
return -EINVAL;
}
-   p->uartclk = val;
 
return 0;
 }
@@ -235,6 +233,7 @@ static int dw8250_probe(struct platform_device *pdev)
struct resource *irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
struct dw8250_data *data;
int err;
+   unsigned long clk_rate;
 
if (!regs || !irq) {
dev_err(>dev, "no registers/irq defined\n");
@@ -260,10 +259,6 @@ static int dw8250_probe(struct platform_device *pdev)
return -ENOMEM;
 
data->clk = devm_clk_get(>dev, NULL);
-   if (!IS_ERR(data->clk)) {
-   clk_prepare_enable(data->clk);
-   uart.port.uartclk = clk_get_rate(data->clk);
-   }
 
uart.port.iotype = UPIO_MEM;
uart.port.serial_in = dw8250_serial_in;
@@ -284,6 +279,36 @@ static int dw8250_probe(struct platform_device *pdev)
return -ENODEV;
}
 
+   /* Read the clock rate from the clock */
+   if (!IS_ERR(data->clk)) {
+   clk_prepare_enable(data->clk);
+   clk_rate = clk_get_rate(data->clk);
+   /*
+* If uartclk hasn't been explicitly specified (e.g. from device
+* tree), use the rate from the clock instead.
+*/
+   if (!uart.port.uartclk)
+   uart.port.uartclk = clk_rate;
+   /*
+* If the current clock rate differs from the rate specified,
+* try and set the clock rate.
+*/
+   if (uart.port.uartclk != clk_rate) {
+   if (!clk_set_rate(data->clk, uart.port.uartclk))
+   clk_rate = clk_get_rate(data->clk);
+   dev_info(>dev,
+"uartclk at %lu

[PATCH] serial: 8250_dw: set clock rate

2013-05-09 Thread James Hogan 
If the uart clock provided to the 8250_dw driver is adjustable it may
not be set to the desired rate. Therefore if both a uart clock and a
clock frequency is specified (e.g. via device tree), try and update the
clock to match the frequency.

Unfortunately if the resulting frequency is rounded down (which is the
default behaviour of the generic clk-divider), the 8250 core won't allow
the highest baud rate to be used, so if an explicit frequency is
specified we always report that to the 8250 core.

The device tree bindings document is also updated accordingly.

Signed-off-by: James Hogan james.ho...@imgtec.com
Cc: Grant Likely grant.lik...@linaro.org
Cc: Rob Herring rob.herr...@calxeda.com
Cc: Rob Landley r...@landley.net
Cc: Greg Kroah-Hartman gre...@linuxfoundation.org
Cc: Jiri Slaby jsl...@suse.cz
Cc: Heikki Krogerus heikki.kroge...@linux.intel.com
Cc: Alan Cox a...@linux.intel.com
Cc: Jamie Iles ja...@jamieiles.com
Cc: Bill Pemberton wf...@virginia.edu
---
 .../bindings/tty/serial/snps-dw-apb-uart.txt   |  4 +-
 drivers/tty/serial/8250/8250_dw.c  | 47 +-
 2 files changed, 39 insertions(+), 12 deletions(-)

diff --git a/Documentation/devicetree/bindings/tty/serial/snps-dw-apb-uart.txt 
b/Documentation/devicetree/bindings/tty/serial/snps-dw-apb-uart.txt
index f13f1c5..e0cfc47 100644
--- a/Documentation/devicetree/bindings/tty/serial/snps-dw-apb-uart.txt
+++ b/Documentation/devicetree/bindings/tty/serial/snps-dw-apb-uart.txt
@@ -4,9 +4,11 @@ Required properties:
 - compatible : snps,dw-apb-uart
 - reg : offset and length of the register set for the device.
 - interrupts : should contain uart interrupt.
-- clock-frequency : the input clock frequency for the UART.
 
 Optional properties:
+- clock-frequency : the input clock frequency for the UART. If specified in
+  addition to clocks, the clock rate will be set to this frequency.
+- clocks : the input clock specifier for the UART.
 - reg-shift : quantity to shift the register offsets by.  If this property is
   not present then the register offsets are not shifted.
 - reg-io-width : the size (in bytes) of the IO accesses that should be
diff --git a/drivers/tty/serial/8250/8250_dw.c 
b/drivers/tty/serial/8250/8250_dw.c
index beaa283..85b3b63 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -128,6 +128,7 @@ dw8250_do_pm(struct uart_port *port, unsigned int state, 
unsigned int old)
 static int dw8250_probe_of(struct uart_port *p)
 {
struct device_node  *np = p-dev-of_node;
+   struct dw8250_data  *d = p-private_data;
u32 val;
 
if (!of_property_read_u32(np, reg-io-width, val)) {
@@ -148,16 +149,13 @@ static int dw8250_probe_of(struct uart_port *p)
if (!of_property_read_u32(np, reg-shift, val))
p-regshift = val;
 
-   /* clock got configured through clk api, all done */
-   if (p-uartclk)
-   return 0;
-
-   /* try to find out clock frequency from DT as fallback */
-   if (of_property_read_u32(np, clock-frequency, val)) {
+   /* try to find out clock frequency from DT */
+   if (!of_property_read_u32(np, clock-frequency, val)) {
+   p-uartclk = val;
+   } else if (IS_ERR(d-clk)) {
dev_err(p-dev, clk or clock-frequency not defined\n);
return -EINVAL;
}
-   p-uartclk = val;
 
return 0;
 }
@@ -235,6 +233,7 @@ static int dw8250_probe(struct platform_device *pdev)
struct resource *irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
struct dw8250_data *data;
int err;
+   unsigned long clk_rate;
 
if (!regs || !irq) {
dev_err(pdev-dev, no registers/irq defined\n);
@@ -260,10 +259,6 @@ static int dw8250_probe(struct platform_device *pdev)
return -ENOMEM;
 
data-clk = devm_clk_get(pdev-dev, NULL);
-   if (!IS_ERR(data-clk)) {
-   clk_prepare_enable(data-clk);
-   uart.port.uartclk = clk_get_rate(data-clk);
-   }
 
uart.port.iotype = UPIO_MEM;
uart.port.serial_in = dw8250_serial_in;
@@ -284,6 +279,36 @@ static int dw8250_probe(struct platform_device *pdev)
return -ENODEV;
}
 
+   /* Read the clock rate from the clock */
+   if (!IS_ERR(data-clk)) {
+   clk_prepare_enable(data-clk);
+   clk_rate = clk_get_rate(data-clk);
+   /*
+* If uartclk hasn't been explicitly specified (e.g. from device
+* tree), use the rate from the clock instead.
+*/
+   if (!uart.port.uartclk)
+   uart.port.uartclk = clk_rate;
+   /*
+* If the current clock rate differs from the rate specified,
+* try and set the clock rate.
+*/
+   if (uart.port.uartclk != clk_rate) {
+   if

Re: [PATCH] serial: 8250_dw: set clock rate

2013-05-09 Thread James Hogan 
On 09/05/13 11:29, James Hogan wrote:
 If the uart clock provided to the 8250_dw driver is adjustable it may
 not be set to the desired rate. Therefore if both a uart clock and a
 clock frequency is specified (e.g. via device tree), try and update the
 clock to match the frequency.
 
 Unfortunately if the resulting frequency is rounded down (which is the
 default behaviour of the generic clk-divider), the 8250 core won't allow
 the highest baud rate to be used, so if an explicit frequency is
 specified we always report that to the 8250 core.

Hi,

Sorry, please ignore this patch.

I've realised that a larger (e.g. non-divided) source clock can just be
provided directly to the uart and it appears to have enough of an
internal divider for the driver to do the right thing without changing
the input clock rate, although of course that clock rate still needs
setting somewhere. I'm not sure why we didn't do this all along as it
gives a closer frequency anyway.

Thanks
James

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/