serial: 8250_dw: support high baudrates if possible

Message ID 1403889920-1127-1-git-send-email-elder@linaro.org
State New
Headers show

Commit Message

Alex Elder June 27, 2014, 5:25 p.m.
Currently the Synopsys DesignWare 8250 driver assumes its UART clock
runs at a fixed rate.  If a "real" clock was set up using the common
clock framework, and that clock's rate is adjustable, it may be
possible to support a wider range of baud rates by changing the
UART clock rate.

This is done by setting up a uart_port->set_termios method that
requests a clock rate change if a different rate might make it
more likely to achieve a specified baud.

Signed-off-by: Alex Elder <elder@linaro.org>
---
This patch is also available here:
    http://git.linaro.org/landing-teams/working/broadcom/kernel.git
    Branch review/dw8250_clock

 drivers/tty/serial/8250/8250_dw.c | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

Comments

One Thousand Gnomes June 28, 2014, 3:36 p.m. | #1
On Fri, 27 Jun 2014 12:25:20 -0500
> +		rate = 16 * max(115200U, (unsigned int)baud);
> +

This assumes an arbitarily configurable clock, which is not I think the
usual case. 
> +		/*
> +		 * Request a different clock rate if necessary, and
> +		 * record it if successful.
> +		 */
> +		if (rate != p->uartclk) {
> +			BUG_ON(!data->clk);
> +			if (!clk_set_rate(data->clk, (unsigned long)rate))
> +				p->uartclk = rate;
> +		}


--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alex Elder June 28, 2014, 8:15 p.m. | #2
On 06/28/2014 10:36 AM, One Thousand Gnomes wrote:
> On Fri, 27 Jun 2014 12:25:20 -0500
>> +		rate = 16 * max(115200U, (unsigned int)baud);
>> +
> 
> This assumes an arbitarily configurable clock, which is not I think the
> usual case. 

If the clock's rate can't change, this will return an error,
and the recorded rate (p->uartclk) will not be changed.

This should only matter when attempting to set a baud rate higher
than 115200.  It *is* possible that some particular high rate
will realize a better signal rate than whatever results from
requesting 16 times the baud (or even 16 * 115200).

I could make this work *only* for my particular part(s) by
relying on a different device tree compatible string and
setting a flag.  It would be nice if other implementations
could benefit from this though.

					-Alex

>> +		/*
>> +		 * Request a different clock rate if necessary, and
>> +		 * record it if successful.
>> +		 */
>> +		if (rate != p->uartclk) {
>> +			BUG_ON(!data->clk);
>> +			if (!clk_set_rate(data->clk, (unsigned long)rate))
>> +				p->uartclk = rate;
>> +		}
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
One Thousand Gnomes July 1, 2014, 10:02 p.m. | #3
On Sat, 28 Jun 2014 15:15:56 -0500
Alex Elder <elder@linaro.org> wrote:

> On 06/28/2014 10:36 AM, One Thousand Gnomes wrote:
> > On Fri, 27 Jun 2014 12:25:20 -0500
> >> +		rate = 16 * max(115200U, (unsigned int)baud);
> >> +
> > 
> > This assumes an arbitarily configurable clock, which is not I think the
> > usual case. 
> 
> If the clock's rate can't change, this will return an error,
> and the recorded rate (p->uartclk) will not be changed.

Which assumes an arbitrarily configurable clock, whereas you want to find
the correct clock and multiplier combination for the baud rate.

Most of these ports are wired to fixed clocks (which is fine) or clocks
with limited numbers of supported frequencies (which is not).

Your patch is an improvement but doesn't really fix the overall problem.
If we have enough devices with variable clocks for it to be useful then
fine - but can we merge it with a big FIXME note so that whoever comes
along wondering why their clock doesn't work or behaves very oddly can
figure it out and fix that case ?

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Alex Elder July 2, 2014, 12:15 a.m. | #4
On 07/01/2014 05:02 PM, One Thousand Gnomes wrote:
> On Sat, 28 Jun 2014 15:15:56 -0500
> Alex Elder <elder@linaro.org> wrote:
> 
>> On 06/28/2014 10:36 AM, One Thousand Gnomes wrote:
>>> On Fri, 27 Jun 2014 12:25:20 -0500
>>>> +		rate = 16 * max(115200U, (unsigned int)baud);
>>>> +
>>>
>>> This assumes an arbitarily configurable clock, which is not I think the
>>> usual case. 
>>
>> If the clock's rate can't change, this will return an error,
>> and the recorded rate (p->uartclk) will not be changed.
> 
> Which assumes an arbitrarily configurable clock, whereas you want to find
> the correct clock and multiplier combination for the baud rate.
> 
> Most of these ports are wired to fixed clocks (which is fine) or clocks
> with limited numbers of supported frequencies (which is not).

Yes, I acknowledged that in my earlier response.  But over
the weekend I decided to abandon hope that I'd be able to
verify there are no problems for affected machines, and
re-formulated my patch.
    https://lkml.org/lkml/2014/7/1/323

> Your patch is an improvement but doesn't really fix the overall problem.
> If we have enough devices with variable clocks for it to be useful then
> fine - but can we merge it with a big FIXME note so that whoever comes
> along wondering why their clock doesn't work or behaves very oddly can
> figure it out and fix that case ?

What I've done now is define a dw8250_adjustable_clk() predicate
that indicates the UART baud can be adjusted by changing the
clock rate.  Anyone who wants to activate this functionality
can just modify that function to recognize their device.

							-Alex

> Alan
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch hide | download patch | download mbox

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 51b307a..013aa9b 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -242,6 +242,43 @@  dw8250_do_pm(struct uart_port *port, unsigned int state, unsigned int old)
 		pm_runtime_put_sync_suspend(port->dev);
 }
 
+/*
+ * If our clock was configured through the common clock framework we
+ * might be able to support a wider range of baud rates by changing
+ * its frequency.  For the "traditional" baud rates (115200 or
+ * less), request the conventional 1.8432 MHz clock.  The clock
+ * frequency needs to be at least 16 times the baud rate, so
+ * use 16 times the requested rate for those higher than 115200.
+ */
+static void
+dw8250_set_termios(struct uart_port *p, struct ktermios *new,
+			struct ktermios *old)
+{
+	struct dw8250_data *data = p->private_data;
+
+	if (!IS_ERR(data->clk)) {
+		speed_t baud;
+		unsigned int rate;
+
+		baud = tty_termios_baud_rate(new);
+		BUG_ON(baud > (speed_t)UINT_MAX);
+		rate = 16 * max(115200U, (unsigned int)baud);
+
+		/*
+		 * Request a different clock rate if necessary, and
+		 * record it if successful.
+		 */
+		if (rate != p->uartclk) {
+			BUG_ON(!data->clk);
+			if (!clk_set_rate(data->clk, (unsigned long)rate))
+				p->uartclk = rate;
+		}
+	}
+
+	/* The standard code does most of the work. */
+	serial8250_do_set_termios(p, new, old);
+}
+
 static bool dw8250_dma_filter(struct dma_chan *chan, void *param)
 {
 	struct dw8250_data *data = param;
@@ -417,6 +454,7 @@  static int dw8250_probe(struct platform_device *pdev)
 	uart.port.iotype = UPIO_MEM;
 	uart.port.serial_in = dw8250_serial_in;
 	uart.port.serial_out = dw8250_serial_out;
+	uart.port.set_termios = dw8250_set_termios;
 	uart.port.private_data = data;
 
 	if (pdev->dev.of_node) {