| Message ID | 1403889920-1127-1-git-send-email-elder@linaro.org |
|---|---|
| State | New |
| Headers | show |
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
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/
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/
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
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) {
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(+)