[PATCH] 8250_dw: Support all baudrates on baytrail

[PATCH] 8250_dw: Support all baudrates on baytrail

[Loic Poulain]

In the same manner as 8250_pci, 8250_dw needs
some baytrail specific quirks to be used.
The reference clock needs to be adjusted before
divided in order to have the minimum error rate
on the baudrate.

The specific byt setup function is stored in the
driver_data field of the acpi device id via the
dw8250_acpi_desc structure.

Signed-off-by: Loic Poulain <loic.poulain@intel.com>
---
 drivers/tty/serial/8250/8250_dw.c | 91 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 87 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index ed31135..7d3e4d9 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -62,6 +62,77 @@ struct dw8250_data {
 	struct uart_8250_dma	dma;
 };
 
+struct dw8250_acpi_desc {
+	unsigned int uartclk;
+	void (*setup)(struct uart_8250_port *up);
+};
+
+#define BYT_PRV_CLK			0x800
+#define BYT_PRV_CLK_EN			(1 << 0)
+#define BYT_PRV_CLK_M_VAL_SHIFT		1
+#define BYT_PRV_CLK_N_VAL_SHIFT		16
+#define BYT_PRV_CLK_UPDATE		(1 << 31)
+
+static void byt_set_termios(struct uart_port *p, struct ktermios *termios,
+			    struct ktermios *old)
+{
+	unsigned int baud = tty_termios_baud_rate(termios);
+	unsigned int m, n;
+	u32 reg;
+
+	/*
+	* For baud rates 0.5M, 1M, 1.5M, 2M, 2.5M, 3M, 3.5M and 4M the
+	* dividers must be adjusted.
+	*
+	* uartclk = (m / n) * 100 MHz, where m <= n
+	*/
+	switch (baud) {
+	case 500000:
+	case 1000000:
+	case 2000000:
+	case 4000000:
+		m = 64;
+		n = 100;
+		p->uartclk = 64000000;
+		break;
+	case 3500000:
+		m = 56;
+		n = 100;
+		p->uartclk = 56000000;
+		break;
+	case 1500000:
+	case 3000000:
+		m = 48;
+		n = 100;
+		p->uartclk = 48000000;
+		break;
+	case 2500000:
+		m = 40;
+		n = 100;
+		p->uartclk = 40000000;
+		break;
+	default:
+		m = 2304;
+		n = 3125;
+		p->uartclk = 73728000;
+	}
+
+	/* Reset the clock */
+	reg = (m << BYT_PRV_CLK_M_VAL_SHIFT) | (n << BYT_PRV_CLK_N_VAL_SHIFT);
+	writel(reg, p->membase + BYT_PRV_CLK);
+	reg |= BYT_PRV_CLK_EN | BYT_PRV_CLK_UPDATE;
+	writel(reg, p->membase + BYT_PRV_CLK);
+
+	serial8250_do_set_termios(p, termios, old);
+}
+
+static void byt_serial_setup(struct uart_8250_port *up)
+{
+	struct uart_port *p = &up->port;
+
+	p->set_termios = byt_set_termios;
+}
+
 static inline int dw8250_modify_msr(struct uart_port *p, int offset, int value)
 {
 	struct dw8250_data *d = p->private_data;
@@ -278,6 +349,7 @@ static int dw8250_probe_acpi(struct uart_8250_port *up,
 {
 	const struct acpi_device_id *id;
 	struct uart_port *p = &up->port;
+	struct dw8250_acpi_desc *acpi_desc;
 
 	dw8250_setup_port(up);
 
@@ -290,14 +362,21 @@ static int dw8250_probe_acpi(struct uart_8250_port *up,
 	p->serial_out = dw8250_serial_out32;
 	p->regshift = 2;
 
-	if (!p->uartclk)
-		p->uartclk = (unsigned int)id->driver_data;
-
 	up->dma = &data->dma;
 
 	up->dma->rxconf.src_maxburst = p->fifosize / 4;
 	up->dma->txconf.dst_maxburst = p->fifosize / 4;
 
+	acpi_desc = (struct dw8250_acpi_desc *)id->driver_data;
+	if (!acpi_desc)
+		return 0;
+
+	if (!p->uartclk)
+		p->uartclk = acpi_desc->uartclk;
+
+	if (acpi_desc->setup)
+		acpi_desc->setup(up);
+
 	return 0;
 }
 
@@ -445,12 +524,16 @@ static const struct of_device_id dw8250_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, dw8250_of_match);
 
+static struct dw8250_acpi_desc byt_8250_desc = {
+	.setup = byt_serial_setup,
+};
+
 static const struct acpi_device_id dw8250_acpi_match[] = {
 	{ "INT33C4", 0 },
 	{ "INT33C5", 0 },
 	{ "INT3434", 0 },
 	{ "INT3435", 0 },
-	{ "80860F0A", 0 },
+	{ "80860F0A", (kernel_ulong_t)&byt_8250_desc},
 	{ },
 };
 MODULE_DEVICE_TABLE(acpi, dw8250_acpi_match);
-- 
1.8.3.2

---------------------------------------------------------------------
Intel Corporation SAS (French simplified joint stock company)
Registered headquarters: "Les Montalets"- 2, rue de Paris, 
92196 Meudon Cedex, France
Registration Number:  302 456 199 R.C.S. NANTERRE
Capital: 4,572,000 Euros

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [PATCH] 8250_dw: Support all baudrates on baytrail

[Heikki Krogerus]

Hi Loic,

On Thu, Apr 10, 2014 at 03:31:24PM +0200, Loic Poulain wrote:
> In the same manner as 8250_pci, 8250_dw needs
> some baytrail specific quirks to be used.
> The reference clock needs to be adjusted before
> divided in order to have the minimum error rate
> on the baudrate.
> 
> The specific byt setup function is stored in the
> driver_data field of the acpi device id via the
> dw8250_acpi_desc structure.
> 
> Signed-off-by: Loic Poulain <loic.poulain@intel.com>
> ---
>  drivers/tty/serial/8250/8250_dw.c | 91 +++++++++++++++++++++++++++++++++++++--
>  1 file changed, 87 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> index ed31135..7d3e4d9 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -62,6 +62,77 @@ struct dw8250_data {
>  	struct uart_8250_dma	dma;
>  };
>  
> +struct dw8250_acpi_desc {
> +	unsigned int uartclk;

This one we could actually drop while at it. The uartclk was
originally delivered as driver data in case of ACPI, but we now get a
clk for ACPI enumerated boards.

> +	void (*setup)(struct uart_8250_port *up);

Why not just have the set_termios hook here?

> +};
> +
> +#define BYT_PRV_CLK			0x800
> +#define BYT_PRV_CLK_EN			(1 << 0)
> +#define BYT_PRV_CLK_M_VAL_SHIFT		1
> +#define BYT_PRV_CLK_N_VAL_SHIFT		16
> +#define BYT_PRV_CLK_UPDATE		(1 << 31)
> +
> +static void byt_set_termios(struct uart_port *p, struct ktermios *termios,
> +			    struct ktermios *old)
> +{
> +	unsigned int baud = tty_termios_baud_rate(termios);
> +	unsigned int m, n;
> +	u32 reg;
> +
> +	/*
> +	* For baud rates 0.5M, 1M, 1.5M, 2M, 2.5M, 3M, 3.5M and 4M the
> +	* dividers must be adjusted.
> +	*
> +	* uartclk = (m / n) * 100 MHz, where m <= n
> +	*/
> +	switch (baud) {
> +	case 500000:
> +	case 1000000:
> +	case 2000000:
> +	case 4000000:
> +		m = 64;
> +		n = 100;
> +		p->uartclk = 64000000;
> +		break;
> +	case 3500000:
> +		m = 56;
> +		n = 100;
> +		p->uartclk = 56000000;
> +		break;
> +	case 1500000:
> +	case 3000000:
> +		m = 48;
> +		n = 100;
> +		p->uartclk = 48000000;
> +		break;
> +	case 2500000:
> +		m = 40;
> +		n = 100;
> +		p->uartclk = 40000000;
> +		break;
> +	default:
> +		m = 2304;
> +		n = 3125;
> +		p->uartclk = 73728000;
> +	}
> +
> +	/* Reset the clock */
> +	reg = (m << BYT_PRV_CLK_M_VAL_SHIFT) | (n << BYT_PRV_CLK_N_VAL_SHIFT);
> +	writel(reg, p->membase + BYT_PRV_CLK);
> +	reg |= BYT_PRV_CLK_EN | BYT_PRV_CLK_UPDATE;
> +	writel(reg, p->membase + BYT_PRV_CLK);
> +
> +	serial8250_do_set_termios(p, termios, old);
> +}

Well, we are already handling this same refclk in the clk that is
provided to this driver in case of BYT, so we should probable handle
this in that as well. Then we could have a more generic set_termios
where we do something like this..

static void dw8250_set_termios(struct uart_port *p, struct ktermios *termios,
                               struct ktermios *old)
{
        unsigned int baud = tty_termios_baud_rate(termios);
        struct dw8250_data *d = p->private_data;
        unsigned int rate;
        int ret;

        rate = clk_round_rate(d->clk, baud*16);
        ret = clk_set_rate(d->clk, rate);
        if (!ret)
                p->uartclk = rate;
        serial8250_do_set_termios(p, termios, old);
}

But I don't have a problem if we go ahead with this first. We can
always change it later. In any case, I'll prepare the clock driver in
drivers/acpi/acpi_lpss.c for this. Let's see what Rafael and Mika
think about it.


Thanks,

-- 
heikki

Re: [PATCH] 8250_dw: Support all baudrates on baytrail

[Tim Kryger]

On Thu, Apr 17, 2014 at 4:19 AM, Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:

> Well, we are already handling this same refclk in the clk that is
> provided to this driver in case of BYT, so we should probable handle
> this in that as well. Then we could have a more generic set_termios
> where we do something like this..
>
> static void dw8250_set_termios(struct uart_port *p, struct ktermios *termios,
>                                struct ktermios *old)
> {
>         unsigned int baud = tty_termios_baud_rate(termios);
>         struct dw8250_data *d = p->private_data;
>         unsigned int rate;
>         int ret;
>
>         rate = clk_round_rate(d->clk, baud*16);
>         ret = clk_set_rate(d->clk, rate);
>         if (!ret)
>                 p->uartclk = rate;
>         serial8250_do_set_termios(p, termios, old);
> }

On Broadcom's mobile SoCs, the external clock that feeds into the DW
UART can switch between high and low frequency parents.

The above may result in a lower frequency parent being selected in
cases where a higher frequency parent could yield better actual baud
rate.

Consider if 115,200 baud was requested and parents provided 2 MHz and
104 MHz.  This would select 2 MHz (for 125,000 baud) instead of 104
MHz (for 116,071 baud).

To get the best baud rate accuracy, the capabilities of the internal
divider need to be considered together with those of the external
clock.

-Tim

RE: [PATCH] 8250_dw: Support all baudrates on baytrail

[Poulain, Loic]

> > +struct dw8250_acpi_desc {
> > +	unsigned int uartclk;

> This one we could actually drop while at it. The uartclk was
> originally delivered as driver data in case of ACPI, but we now get a
> clk for ACPI enumerated boards.

Ok!

> > +	void (*setup)(struct uart_8250_port *up);

> Why not just have the set_termios hook here?

It was to allow any future other byt specific init
In the setup (no just set_termios). But, yes I can
point directly to set_termios.


> But I don't have a problem if we go ahead with this first. We can
> always change it later. In any case, I'll prepare the clock driver in
> drivers/acpi/acpi_lpss.c for this. Let's see what Rafael and Mika
> think about it.

Sounds good,  while waiting to get a better solution, I'm going to 
upload a new patch version.

Thanks & Regards,
Loic
---------------------------------------------------------------------
Intel Corporation SAS (French simplified joint stock company)
Registered headquarters: "Les Montalets"- 2, rue de Paris, 
92196 Meudon Cedex, France
Registration Number:  302 456 199 R.C.S. NANTERRE
Capital: 4,572,000 Euros

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.